search

FasterXML / jackson-module-jsonSchema

Module for generating JSON Schema (v3) definitions from POJOs
368 stars 136 forks source link

Altered ObjectSchema to make "dependencies" an object instead of list. #120

Closed seanstaley closed 7 years ago

seanstaley commented 7 years ago

In the JSON schema the dependencies variable is declared an object instead of a collection or array of items. Therefore, when the JSON schema is generated for an object it is invalid because dependencies is a list.

{
  ...,
  "dependencies": [],
  ...
}

This should be instead:

{
  ...,
  "dependencies": {},
  ...
}

Essentially, I changed ObjectSchema.java to have a Map of String keys and Object values. The Dependency objects provided do not have a JSON creation implementation defined as of yet so I thought that a map should do for now to conform to the spec.

seanstaley commented 7 years ago

I forgot to mention that this ties directly to issue #119

cowtowncoder commented 7 years ago

@seanstaley So how will backwards compatibility issues be handled? I agree that reading of the spec does point out need for JSON Object.

Also, it would be good to have tests for verifying (changed) behavior.

seanstaley commented 7 years ago

I agree with the testing and validating the behavior. I manually tested this on my machine, but I'll spend some time adding some tests for this.

As far as backwards compatibility issues go, I can't imagine that there will be too many issues since this portion of the JSON schema doesn't appear like it was completely finished and doesn't pass schema validation in the first place. I tried to keep the method signatures the same as what they were before so there would be minimal impact to users that might already be using this functionality.

Ultimately, if backwards compatibility becomes an issue then finishing or reverting this portion of the schema definition would become really important.

cowtowncoder commented 7 years ago

@seanstaley unit test(s) would be very useful.

I am worried about backwards compatibility mostly because regardless of whether result is compliant with specification (it isn't), there's a good chance someone somewhere is using and relying on existing behavior. I can not really evaluate likelihood of that (or severity of breakage), but have learnt that reliance on observed behavior is one of prime laws of nature in software implementaiton world.... :-) (alas!)

seanstaley commented 7 years ago

@cowtowncoder, I added unit tests that cover both of the types of dependencies that a JSON schema could have in my latest commit. Found some issues with the original code too after creating the tests and got that all sorted out. Glad you brought that up! 💯

Yeah I would tend to agree with backwards compatibility, however in this instance I don't think that maintaining that compatibility makes sense. I thought about this topic a lot since you brought it up, but ultimately it doesn't make sense to keep this around.

If one did make code changes around this it would most likely to conform the schema generation to produce an actual valid JSON schema. Currently, you take the output of this schema generation tool and put it against any JSON validator and it fails. I don't think that we could or should provide backwards compatibility for this bug. I imagine most people get around this issue by simply changing [] to {} in the generated schema either in their source code or in the output, which we could mitigate with this pull request.

If you guys are still unsure about this fix and what it needs to resolve, I think a solid idea would be to make this part of a minor release. This way, if someone has issues with these changes they could go back to the previous version. Then, depending on the number of issues that come in around this part of the schema, you could determine if you want to revert these changes for future releases or keep it in for a later time when this feature can be fully implemented by the FasterXML team.

What do you think?

cowtowncoder commented 7 years ago

Sorry for slow follow-up. Yes, I think this makes sense; and we'll have 2.9 release candidates to see what the effect might be (if developers do test with those).

One last (?) thing: if I haven't yet asked for Contributor License Agreement:

https://github.com/FasterXML/jackson/blob/master/contributor-agreement.pdf

then I need to do that now. Usually it's easiest to print it, fill & sign, scan/take-photo, email to info at fasterxml dot com. Only needs to be done before first contribution, one-time hassle. When we have that, I'll go ahead and merge this in master for 2.9(.0-SNAPSHOT).

seanstaley commented 7 years ago

@cowtowncoder Sounds like a plan. I sent in the form just a moment ago.