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

Backward compatibility for java-json-schema #1470

Conversation

adwait-godbole
Copy link
Contributor

@adwait-godbole adwait-godbole commented Aug 20, 2024

Hi @harrel56! Hope you're doing well. Needed a bit of your help over here.

To give you the context, this is a part of my GSoC project. So I was trying to make the harness code of your implementation to be backward compatible so as to make it possible to build container images out of it, tagging them multiple releases and going as backward as possible in time with them and eventually all this gets reported via a Bowtie report on the official website via a trend graph for now.

Although I have tried tweaking some of your code over here, my first aim was to make sure that after making the changes, the results on the latest version which right now is 1.7.1 remain the same giving some sort of an assurance that atleast I've started on the right path. Though when I tried building an image for 1.7.1 out of this current scenario of the code, the results were same for Draft2020-12 and Draft2019-09 but something seems to be going wrong with Draft7. The results are coming out to be Errors: 68, Failures: 16, Skips: 2. The Skips: 2 part was as expected but the other two are too much off from the current scenario. Can you please help me in figuring out what have I done wrong over here. Your help would be much appreciated.

Ohh and btw, the command I am using to build the Docker image locally on my machine right now looks like this

docker build --build-arg IMPLEMENTATION_VERSION=1.7.1 -t foo/java-json-schema:v1.7.1 .

Thanks.


📚 Documentation preview 📚: https://bowtie-json-schema--1470.org.readthedocs.build/en/1470/

@harrel56
Copy link
Collaborator

@adwait-godbole well, got to say this idea for a backward compatibility is very ambitious :D I'm gonna take a closer look at the code changes in a few days but in the meanwhile I have some questions:

  1. What's the earliest version we want to check? ngl it'd be easiest to just start from 1.7.0 :D There probably is no good approach for handling arbitrarily old version - I guess the approach you took with magics of java reflection is the only one that could do the job, but it's extremely prone to errors and certainly will break in the future. And I'm not sure if I really want the maintenance burden here.
  2. Is it actually worth it? From this implementation past experience there is not many changes in spec compliance. The only changes that happened are when support for draf2019-09 and draft7 were added. idk if such compliance data would make a nice chart or whatever it's gonna get presented as :)
  3. Any other implementation already went through this process?

@adwait-godbole
Copy link
Contributor Author

adwait-godbole commented Aug 21, 2024

Hi @harrel56, here are the answers to your questions above.

  1. What's the earliest version we want to check? ngl it'd be easiest to just start from 1.7.0 :D There probably is no good approach for handling arbitrarily old version - I guess the approach you took with magics of java reflection is the only one that could do the job, but it's extremely prone to errors and certainly will break in the future. And I'm not sure if I really want the maintenance burden here.

I just added a matrix-versions.json file on this PR and have successfully managed to build images of your harness for all the versions that are listed in that JSON file. Please have a look. Also now the harness is working fine on all the dialects that a version is claiming to support and giving out correct results for the same. (Yesterday it wasn't working but I managed to fix it later). That JSON file basically tells the GitHub workflows to build images of the harness only for those versions that are listed over there and then there's also another GitHub workflow that generates reports for all the supported dialects of the versions that are listed in that file which basically is the data source for the trend graph on the website.

  1. Is it actually worth it? From this implementation past experience there is not many changes in spec compliance. The only changes that happened are when support for draf2019-09 and draft7 were added. idk if such compliance data would make a nice chart or whatever it's gonna get presented as :)

That's fine. Not an issue as such. Me and @Julian have discussed this earlier on as well and he is also fine even if the implementations don't really have drastic changes in the spec compliance as you're saying and nonetheless as part of my GSoC project, getting your implementation up on the website for showing the trend is just part of my job even if there's not really anything eye catching per se in fact there would just be a flat line on the trend graph in your case
(P.S. All credits to you for writing such a great implementation! :)).

  1. Any other implementation already went through this process?

Yes! There's ruby-json_schemer, js-hyperjump and python-jsonschema (Julian's implementation :)) that have already went through this process and you can even have a look at their trend graphs below (the PR for this is not merged yet, so it's not yet live on https://bowtie.report but will be soon enough) :-

2024-08-21.20-09-38.mp4

@Julian
Copy link
Member

Julian commented Aug 21, 2024

"We" definitely aren't settled on whether to use one file or many to do this, so push back if you suspect this makes the harness uglier to maintain. In general there's two use cases to this: the main one Adwait showed off is we want to show for all implementations some notion of how they changed over time -- which yeah I think is nice even if it's flat! that's a good thing maybe! And the second is that it enables a user of Bowtie to answer questions about behavior they see for any version, even if in their codebase they're on an old one, but if they want to check whether they see is reproducible, they'd be able to (as well as check whether it's fixed by later versions). So if they're using some schema X and they see unexpected behavior, they can check whether the behavior they see is reproducible on various other versions without needing to futz with their own codebase.

This isn't a "huge" win -- so as I say feel free to push back on structure, it may be there's an even better way to do this (e.g. the obvious one is to have harnesses which don't simultaneously live in the Bowtie main branch codebase, but this makes rebuilding them if there's a bug that isn't related to the implementation harder). So yeah a lot isn't fully figured out here and could be improved on, but that's the general thinking so far!

Copy link
Collaborator

@harrel56 harrel56 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall this looks very good, I just left a few comments that might help with maintenance in the future.

Followup question: if I release a new version how is the matrix-versions.json updated? Is it somehow handled by dependabot? Same question about that gradle snippet with a default version (I'm not sure if dependabot can handle bumping such version notation)

implementations/java-json-schema/BowtieJsonSchema.java Outdated Show resolved Hide resolved
implementations/java-json-schema/BowtieJsonSchema.java Outdated Show resolved Hide resolved
implementations/java-json-schema/BowtieJsonSchema.java Outdated Show resolved Hide resolved
implementations/java-json-schema/BowtieJsonSchema.java Outdated Show resolved Hide resolved
implementations/java-json-schema/BowtieJsonSchema.java Outdated Show resolved Hide resolved
implementations/java-json-schema/BowtieJsonSchema.java Outdated Show resolved Hide resolved
Copy link
Collaborator

@harrel56 harrel56 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No more comments, happy to merge. @Julian @adwait-godbole Still have one question pending - is dependabot able to publish new version to matrix. json or should it be updated manually?

@adwait-godbole
Copy link
Contributor Author

No more comments, happy to merge. @Julian @adwait-godbole Still have one question pending - is dependabot able to publish new version to matrix. json or should it be updated manually?

@harrel56, Currently I am still working on figuring out how the matrix-versions.json should be updated whenever a dependabot update happens but regardless, I have updated this PR to NOT do any changes in the build.gradle file. Now, I am inserting the IMPLEMENTATION_VERSION from the Dockerfile itself using sed utility and replacing the version in build.gradle dynamically. So given this change, dependabot won't be confused while updating your library version over there.

@harrel56
Copy link
Collaborator

@adwait-godbole this sed looks pretty complicated - check out my approach: 90b0c7b dependabot handles it well

@adwait-godbole
Copy link
Contributor Author

Thanks @harrel56! That way is so much better!

@Julian
Copy link
Member

Julian commented Aug 26, 2024

No more comments, happy to merge. @Julian @adwait-godbole Still have one question pending - is dependabot able to publish new version to matrix. json or should it be updated manually?

@harrel56 I think ideally we'd get rid of this file! The better way I think will be we literally use the tags we have in our own GitHub container registry -- in other words, the set of versions of an implementation that we support is simply "all the ones we built once".

And then all we have to solve is how to bootstrap that list the first time we add backwards compat for an implementation -- but that's a problem we don't automate even today.

@Julian
Copy link
Member

Julian commented Aug 26, 2024

(And then I think with ^ everything "works" -- specifically dependabot will trigger new versions to get built, and old versions will simply stick around)

@Julian Julian merged commit 34e6f7a into bowtie-json-schema:main Aug 26, 2024
55 checks passed
@adwait-godbole
Copy link
Contributor Author

Can you please trigger the images.yml workflow for java-json-schema? Thanks.

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.

3 participants