-
Notifications
You must be signed in to change notification settings - Fork 136
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
Refactoring: Validation Groups, v3 and v4 Required support, Chaining of processors #125
base: master
Are you sure you want to change the base?
Conversation
instead of the field level as per v4 json schema spec.
annotations and limit the ones chosen by the validation group being used.
Allows support of old style required field attribute and v4 required field name list. You can now also plugin in property processors to add custom functionality.
Need to come up with better shorter names.
@@ -299,8 +335,8 @@ public String getId() { | |||
return extendsextends; | |||
} | |||
|
|||
public Boolean getRequired() { | |||
return required; | |||
public Boolean getRequiredBoolean() { |
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.
Why would the type be included in accessor name? Is that for schema v4?
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.
Since I added the v4 version of required into the ObjectSchema I couldn't have 2 getRequired methods retuning different types. So I changed this one and that is the best I came up with at the time. I probably should put this back to getRequired and rename the one in ObjectSchema to requiredFields and annotate it to be @JsonProperty("required"). Thoughts?
@@ -441,6 +477,9 @@ public boolean isValueTypeSchema() { | |||
|
|||
public void set$schema(String $schema) { | |||
this.$schema = $schema; | |||
if (version == null) { | |||
this.version = JsonSchemaVersion.fromSchemaString($schema).orElse(null); |
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.
Why doesn't method just return null
?
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.
The problem is that version isn't part of the JsonSchema so it is annotated with JsonIgnore. So when deserializing the version isn't set. So what this is doing is deriving the version from the schema. Thoughts on a better way to do this? Perhaps with a custom deserializer?
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.
fromSchemaString returns an Optional so that you can set it to a default value if you wanted.
IE
JsonSchemaVersion.fromSchemaString($schema).orElse(JsonSchemaVersion.DRAFT_V4);
OR in the future maybe:
JsonSchemaVersion.fromSchemaString($schema).orElse(JsonSchemaVersion.DEFAULT); where DEFAULT could be the primarily or most recent supported version
import com.fasterxml.jackson.module.jsonSchema.property.manager.SchemaPropertyProcessorManagerApi; | ||
import com.fasterxml.jackson.module.jsonSchema.types.ObjectSchema; | ||
|
||
/** |
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.
Could you add Javadoc comments on puprose of this class? Name gets rather unwieldy at this point, but I guess it's compatible with existing verbose naming (not my choosing).
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.
I will certainly add javadoc. Please point out other places that are lacking as well and I will get them too.
I am not a fan of the name at all. Any suggestions on a better one?
Java 8 would be fine for version 3.0 of Jackson (but not 2.9). |
Actually, come to think of it. I am not sure if it'd make sense to create a new module altogether, and deprecate this one (meaning, this repo would be for 2.x; new one for 3.x). Since 3.0 of Jackson is major change, not backwards compatible, bigger changes are possible. You could make bigger changes. |
Whatever you think is best is fine with me. My rationale on the version was to keep it inline with the JsonSchema version. Since I was doing v4, although draft, I had put 0.4.0 but I knew you wouldn't want to go backwards so I did 4.0.0. We can make it whatever though. |
Short of the change to using version 8 of Java I think bringing back a few of the classes and just wrapping those names around the new ones and marking them as deprecated that it would still comply with the old API. If you wanted to keep it from breaking the old API. |
changed getRequiredBoolean back to getRequired, added the ability to add non-standard (not in the json-schema standard) key value pairs, cleaned up the creation of property processors
Refactoring to take into account validation groups and constraints on custom annotations. In doing so, made it easier to process new annotations and chain together processors. Also added the ability to support v3 required boolean and v4 required field name set.
NOTE: this is java 8 only as 7 is pretty much EOL.