-
Notifications
You must be signed in to change notification settings - Fork 472
Add json property type with simple validation #3927
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
Add json property type with simple validation #3927
Conversation
ddanielr
left a comment
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.
Overall changes look good. Minor feedback on some tests.
I like jackson's features over GSON and it might be a good change to fully switch over so there's a single JSON parser used in the project.
core/src/test/java/org/apache/accumulo/core/conf/PropertyTest.java
Outdated
Show resolved
Hide resolved
core/src/test/java/org/apache/accumulo/core/conf/PropertyTest.java
Outdated
Show resolved
Hide resolved
| private static final Logger log = LoggerFactory.getLogger(ConfigSetIT.class); | ||
|
|
||
| @Test | ||
| public void setInvalidJson() throws Exception { |
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.
This question is probably out of scope of the "simple validation" of this PR.
Is there a way to test setting a custom, user-defined property (created under the general.custom prefix that's of type JSON with the shell?
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.
As it is now, general.custom is a prefix and does not specify a type.
I think it would take something like a new PREFIX type, maybe something like general.custom.json that could then be used to trigger validation. There may be issues with precedence with general.custom that would short circuit general.custom.json.
Certainly something to discuss as a follow-on.
Adds a JSON PropertyType and minimal validation that verifies the property value can be parsed into a JSON object.
This was inspired by issue #3909, but does not directly address it.
This implementation was to provide a minimal check, that follow on PRs could extend / leverage to introduce additional property validation.