Skip to content
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

Open
wants to merge 8 commits into
base: master
Choose a base branch
from

Conversation

ammerritt
Copy link

@ammerritt ammerritt commented Nov 3, 2017

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.

Aaron Merritt added 6 commits October 31, 2017 12:40
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() {
Copy link
Member

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?

Copy link
Author

@ammerritt ammerritt Nov 4, 2017

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);
Copy link
Member

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?

Copy link
Author

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?

Copy link
Author

@ammerritt ammerritt Nov 6, 2017

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;

/**
Copy link
Member

@cowtowncoder cowtowncoder Nov 3, 2017

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).

Copy link
Author

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?

@cowtowncoder
Copy link
Member

Java 8 would be fine for version 3.0 of Jackson (but not 2.9).
However I don't understand why you'd suggest changing version to 4.0.0 -- modules are not independently versioned but rather along with core Jackson components.

@cowtowncoder
Copy link
Member

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.
I don't really care about history that much and I could give you bigger access.
Of course there is also the question of whether it needs to be under FasterXML at all -- the only real benefit is that this way I can (... and have to :) ) make releases as part of general Jackson release. I don't really use JSON Schema myself, and consequently can not help a lot with module otherwise.
As such it might be better to have module be under some other organization. At the same time I am ok with it being under FX and granting necessary access for modifications, whatever works best.

@ammerritt
Copy link
Author

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.

@ammerritt
Copy link
Author

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.

Aaron Merritt added 2 commits November 6, 2017 09:49
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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants