Repository navigation
fix: call gomtm bridge methods with correct casing - #4
Merged
Merged
Conversation
📝 WalkthroughWalkthrough
Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
app/src/main/java/com/gomtm/android/swarm/SwarmRuntime.kt (1)
17-23: Add coverage for the PascalCase bridge shape.These fallback lists are the core fix, but the current fakes in
app/src/test/java/com/gomtm/android/swarm/SwarmRuntimeTest.kt:53-57andapp/src/test/java/com/gomtm/android/swarm/SwarmRuntimeTest.kt:59-88still only expose lowercase members. A second fake bridge/config withStartNode/StopNode/GetState/DrainLogsandSet*setters would keep the exact AAR casing this PR targets from regressing silently.Also applies to: 29-31, 37-37, 46-52
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/main/java/com/gomtm/android/swarm/SwarmRuntime.kt` around lines 17 - 23, Tests currently only create fakes exposing lowercase bridge/config members, so add a second fake in SwarmRuntimeTest that exposes the PascalCase AAR casing (StartNode, StopNode, GetState, DrainLogs and SetBootstrapAddr/SetNodeName/SetAutoReconnect setters) and use it in the existing test cases (the blocks around the current fakes) to assert that invokeStaticByNames, setStringProperty and setBooleanProperty correctly handle PascalCase names; ensure the fake exposes both method and property names referenced in SwarmRuntime (StartNode/StopNode/GetState/DrainLogs and Set* setters) so the PR’s casing fallback lists are covered and won’t regress silently.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@app/src/main/java/com/gomtm/android/swarm/SwarmRuntime.kt`:
- Around line 110-136: The setters setStringProperty and setBooleanProperty
currently only catch NoSuchMethodException, so any exception thrown by the
invoked setter is left wrapped in InvocationTargetException; update both
functions to catch InvocationTargetException around
targetClass.getMethod(...).invoke(...) and unwrap its cause (as
invokeStaticByNames does) so the original exception is surfaced—rethrow the
underlying cause (or wrap it in an IllegalStateException while preserving it as
the cause) to maintain consistent error reporting with invokeStaticByNames and
reference these methods by name when making the change.
---
Nitpick comments:
In `@app/src/main/java/com/gomtm/android/swarm/SwarmRuntime.kt`:
- Around line 17-23: Tests currently only create fakes exposing lowercase
bridge/config members, so add a second fake in SwarmRuntimeTest that exposes the
PascalCase AAR casing (StartNode, StopNode, GetState, DrainLogs and
SetBootstrapAddr/SetNodeName/SetAutoReconnect setters) and use it in the
existing test cases (the blocks around the current fakes) to assert that
invokeStaticByNames, setStringProperty and setBooleanProperty correctly handle
PascalCase names; ensure the fake exposes both method and property names
referenced in SwarmRuntime (StartNode/StopNode/GetState/DrainLogs and Set*
setters) so the PR’s casing fallback lists are covered and won’t regress
silently.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 1b5f5102-3a2b-4acf-aea7-abf45f8ebab4
📒 Files selected for processing (1)
app/src/main/java/com/gomtm/android/swarm/SwarmRuntime.kt
Comment on lines
+110
to
+136
| private fun setStringProperty(targetClass: Class<*>, target: Any, methodNames: List<String>, value: String) { | ||
| var lastMissing: NoSuchMethodException? = null | ||
| for (name in methodNames) { | ||
| try { | ||
| targetClass.getMethod(name, String::class.java).invoke(target, value) | ||
| return | ||
| } catch (error: NoSuchMethodException) { | ||
| lastMissing = error | ||
| continue | ||
| } | ||
| } | ||
| throw IllegalStateException("config setter not found: ${methodNames.joinToString()}", lastMissing) | ||
| } | ||
|
|
||
| private fun setBooleanProperty(targetClass: Class<*>, target: Any, methodName: String, value: Boolean) { | ||
| private fun setBooleanProperty(targetClass: Class<*>, target: Any, methodNames: List<String>, value: Boolean) { | ||
| val booleanType = Boolean::class.javaPrimitiveType ?: Boolean::class.javaObjectType | ||
| targetClass.getMethod(methodName, booleanType).invoke(target, value) | ||
| var lastMissing: NoSuchMethodException? = null | ||
| for (name in methodNames) { | ||
| try { | ||
| targetClass.getMethod(name, booleanType).invoke(target, value) | ||
| return | ||
| } catch (error: NoSuchMethodException) { | ||
| lastMissing = error | ||
| continue | ||
| } | ||
| } | ||
| throw IllegalStateException("config setter not found: ${methodNames.joinToString()}", lastMissing) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
find . -type f -name "SwarmRuntime.kt" | head -5Repository: codeh007/gomtm-android
Length of output: 125
🏁 Script executed:
cat -n app/src/main/java/com/gomtm/android/swarm/SwarmRuntime.kt | head -150Repository: codeh007/gomtm-android
Length of output: 7100
🏁 Script executed:
rg "invokeStaticByNames" -A 20 app/src/main/java/com/gomtm/android/swarm/SwarmRuntime.ktRepository: codeh007/gomtm-android
Length of output: 2104
Unwrap setter failures the same way as bridge call failures.
Lines 114 and 129 call Method.invoke(), which wraps exceptions thrown by the invoked method in InvocationTargetException. The setStringProperty and setBooleanProperty helpers only catch NoSuchMethodException, leaving any failure from the actual setter invocation (e.g., SetBootstrapAddr, SetNodeName, SetAutoReconnect) wrapped in an opaque reflection exception. However, invokeStaticByNames (lines 102–105) already unwraps these failures, creating an inconsistency in error reporting.
Patch
private fun setStringProperty(targetClass: Class<*>, target: Any, methodNames: List<String>, value: String) {
var lastMissing: NoSuchMethodException? = null
for (name in methodNames) {
try {
targetClass.getMethod(name, String::class.java).invoke(target, value)
return
} catch (error: NoSuchMethodException) {
lastMissing = error
continue
+ } catch (error: InvocationTargetException) {
+ val cause = error.targetException ?: error.cause ?: error
+ throw IllegalStateException(cause.message ?: cause.javaClass.simpleName, cause)
}
}
throw IllegalStateException("config setter not found: ${methodNames.joinToString()}", lastMissing)
}
private fun setBooleanProperty(targetClass: Class<*>, target: Any, methodNames: List<String>, value: Boolean) {
val booleanType = Boolean::class.javaPrimitiveType ?: Boolean::class.javaObjectType
var lastMissing: NoSuchMethodException? = null
for (name in methodNames) {
try {
targetClass.getMethod(name, booleanType).invoke(target, value)
return
} catch (error: NoSuchMethodException) {
lastMissing = error
continue
+ } catch (error: InvocationTargetException) {
+ val cause = error.targetException ?: error.cause ?: error
+ throw IllegalStateException(cause.message ?: cause.javaClass.simpleName, cause)
}
}
throw IllegalStateException("config setter not found: ${methodNames.joinToString()}", lastMissing)
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| private fun setStringProperty(targetClass: Class<*>, target: Any, methodNames: List<String>, value: String) { | |
| var lastMissing: NoSuchMethodException? = null | |
| for (name in methodNames) { | |
| try { | |
| targetClass.getMethod(name, String::class.java).invoke(target, value) | |
| return | |
| } catch (error: NoSuchMethodException) { | |
| lastMissing = error | |
| continue | |
| } | |
| } | |
| throw IllegalStateException("config setter not found: ${methodNames.joinToString()}", lastMissing) | |
| } | |
| private fun setBooleanProperty(targetClass: Class<*>, target: Any, methodName: String, value: Boolean) { | |
| private fun setBooleanProperty(targetClass: Class<*>, target: Any, methodNames: List<String>, value: Boolean) { | |
| val booleanType = Boolean::class.javaPrimitiveType ?: Boolean::class.javaObjectType | |
| targetClass.getMethod(methodName, booleanType).invoke(target, value) | |
| var lastMissing: NoSuchMethodException? = null | |
| for (name in methodNames) { | |
| try { | |
| targetClass.getMethod(name, booleanType).invoke(target, value) | |
| return | |
| } catch (error: NoSuchMethodException) { | |
| lastMissing = error | |
| continue | |
| } | |
| } | |
| throw IllegalStateException("config setter not found: ${methodNames.joinToString()}", lastMissing) | |
| private fun setStringProperty(targetClass: Class<*>, target: Any, methodNames: List<String>, value: String) { | |
| var lastMissing: NoSuchMethodException? = null | |
| for (name in methodNames) { | |
| try { | |
| targetClass.getMethod(name, String::class.java).invoke(target, value) | |
| return | |
| } catch (error: NoSuchMethodException) { | |
| lastMissing = error | |
| continue | |
| } catch (error: InvocationTargetException) { | |
| val cause = error.targetException ?: error.cause ?: error | |
| throw IllegalStateException(cause.message ?: cause.javaClass.simpleName, cause) | |
| } | |
| } | |
| throw IllegalStateException("config setter not found: ${methodNames.joinToString()}", lastMissing) | |
| } | |
| private fun setBooleanProperty(targetClass: Class<*>, target: Any, methodNames: List<String>, value: Boolean) { | |
| val booleanType = Boolean::class.javaPrimitiveType ?: Boolean::class.javaObjectType | |
| var lastMissing: NoSuchMethodException? = null | |
| for (name in methodNames) { | |
| try { | |
| targetClass.getMethod(name, booleanType).invoke(target, value) | |
| return | |
| } catch (error: NoSuchMethodException) { | |
| lastMissing = error | |
| continue | |
| } catch (error: InvocationTargetException) { | |
| val cause = error.targetException ?: error.cause ?: error | |
| throw IllegalStateException(cause.message ?: cause.javaClass.simpleName, cause) | |
| } | |
| } | |
| throw IllegalStateException("config setter not found: ${methodNames.joinToString()}", lastMissing) | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@app/src/main/java/com/gomtm/android/swarm/SwarmRuntime.kt` around lines 110 -
136, The setters setStringProperty and setBooleanProperty currently only catch
NoSuchMethodException, so any exception thrown by the invoked setter is left
wrapped in InvocationTargetException; update both functions to catch
InvocationTargetException around targetClass.getMethod(...).invoke(...) and
unwrap its cause (as invokeStaticByNames does) so the original exception is
surfaced—rethrow the underlying cause (or wrap it in an IllegalStateException
while preserving it as the cause) to maintain consistent error reporting with
invokeStaticByNames and reference these methods by name when making the change.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Why\nSwarmRuntime currently invokes bridge methods using lower-case names (startNode/getState/etc). The gomobile AAR exports StartNode/GetState/etc, so Start/Stop/Probe never execute.\n\n## What\n- Add method-name fallbacks for Start/Stop and probe getters (GetState/GetPeerID/GetBootstrapAddr/GetLastError/GetDiscoveredPeers/DrainLogs).\n- Add setter fallbacks for Config (SetBootstrapAddr/SetNodeName/SetAutoReconnect).\n- Keep probe tolerant if methods are missing; Start/Stop now raise clear error if no method found.\n\n## Result\nStart node now correctly calls into the gomtm AAR on Android, enabling swarm join and peer discovery.\n
Summary by CodeRabbit