-
Notifications
You must be signed in to change notification settings - Fork 0
Fix dropdown options reflection error with kb-sdk validate
#123
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
Bug found by the `kb_djornl` author: ``` Exception in thread "main" org.graalvm.nativeimage.MissingReflectionRegistrationError: The program tried to reflectively invoke method public us.kbase.narrativemethodstore.DropdownOptions() without it being registered for runtime reflection. * large stack trace with JsonClientCaller and Jackson methods snipped* at us.kbase.narrativemethodstore.NarrativeMethodStoreClient.validateMethod(NarrativeMethodStoreClient.java:526) at us.kbase.sdk.validator.ModuleValidator.validateMethodSpec(ModuleValidator.java:236) at us.kbase.sdk.validator.ModuleValidator.validate(ModuleValidator.java:168) at us.kbase.sdk.tester.ModuleTester.runTests(ModuleTester.java:94) at us.kbase.sdk.ModuleBuilder$TestCommand.call(ModuleBuilder.java:445) at us.kbase.sdk.ModuleBuilder$TestCommand.call(ModuleBuilder.java:431) at picocli.CommandLine.executeUserObject(CommandLine.java:2031) ``` Fixed this bug and other potential bugs around NMS classes not being fully accessible by making all methods, fields, and constructors available for reflection. Tested with the `kb_staging_exporter` app, which also has `dropdown_option` in `spec.json`, by running `kb-sdk validate`.
"--enable-url-protocols=https,http,file", | ||
"--enable-url-protocols=https,http", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
file is enabled by default
{ | ||
"name":"us.kbase.sdk.ModuleBuilder$ValidationArgs", | ||
"allDeclaredFields":true, | ||
"queryAllDeclaredMethods":true | ||
}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removed in a previous PR
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #123 +/- ##
=========================================
Coverage 69.04% 69.04%
Complexity 650 650
=========================================
Files 45 45
Lines 3609 3609
Branches 669 669
=========================================
Hits 2492 2492
Misses 868 868
Partials 249 249 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
build.gradle
Outdated
echo "Done. NOTE: Inspect the merged output carefully; the agent is better" | ||
echo "than starting from scratch but is imperfect." | ||
echo "When merging into src/main/resources/META-INF/native-image carefully" | ||
echo "check that" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
supremely picky, but if you're going to have a bulleted list, it'd be better to precede it with a colon, e.g.
echo "When merging into src/main/resources/META-INF/native-image carefully"
echo "check the following:"
echo ""
Given the likely audience size for this codebase, though, it doesn't really matter.
Isn't it nice to have a comment on your PR so that you know someone somewhere cares?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed
Bug found by the
kb_djornl
author:Fixed this bug and other potential bugs around NMS classes not being fully accessible by making all methods, fields, and constructors available for reflection.
Tested with the
kb_staging_exporter
app, which also hasdropdown_option
inspec.json
, by runningkb-sdk validate
.