Skip to content
This repository was archived by the owner on Nov 2, 2023. It is now read-only.

Please consider adding json-kotlin-schema and -codegen … #361

Merged
merged 5 commits into from
May 21, 2021

Conversation

pwall567
Copy link
Contributor

…to the list of implementations.

@netlify
Copy link

netlify bot commented Oct 25, 2020

Deploy Preview for condescending-hopper-c3ed30 ready!

Built with commit 5e72ef0

https://deploy-preview-361--condescending-hopper-c3ed30.netlify.app

@Relequestual
Copy link
Member

Sorry for the delay here.
Are you using the official test suite? I couldn't find it.

@pwall567
Copy link
Contributor Author

Thanks - I was wondering what had happened 😄

Test suite? I have to confess I wasn't aware of it. I've located it now and I'll check my code against it over the weekend.

Expect an update shortly.

@Relequestual
Copy link
Member

Yeah, we were stretched thin and wanted to deliver draft 2020-12 to allow the OpenAPI 3.1 release to use it.
But also, now, I'm full time JSON Sceham!

Feel free to join our slack server =]

@pwall567
Copy link
Contributor Author

OK, I have now run the test suite against my implementation (for anyone interested, the code is at src/test/kotlin/net/pwall/json/schema/testsuite/TestSuiteTests.kt). I chose to skip some of the tests - there are some areas of functionality not yet addressed by this implementation, for example $recursiveRef, dependentSchemas etc. Of the tests actually used, 620 passed and 0 failed.

The tests showed up not only a couple of bugs (now fixed), but also a few misinterpretations on my part. For example, it takes a very close reading of the spec to realise that 3.0 should validate as a valid integer. In this, the test suite has been very helpful.

There are other areas not touched on by the test suite, for example the implementations of assertions in my software is patchy. The "Basic" output is reasonably complete (with a full implementation of definition and instance pointers) but the "Detailed" output is definitely not. And the "Verbose" output has not yet been attempted.

With all these reservations, I hope my software can be considered for inclusion in the lists. The README file in the project clearly states what is covered by the current implementation, and with any luck this will satisfy a very large subset of the potential uses of the software.

@karenetheridge
Copy link
Member

IMHO, if there are core parts of the spec that are not implemented (e.g. non-optional keywords, annotation collection - basically anything the spec indicates is a MUST rather than a SHOULD or a MAY), we should be indicating that in the list of implementations. Perhaps a legend can be created so these caveats can be indicated in some sort of shorthand. Often-times one is reaching for an implementation and won't need the bits that aren't implemented, but it is frustrating embarking on a code integration only to discover later on that a needed bit of functionality isn't there.

@pwall567
Copy link
Contributor Author

This has been marked as "On hold" for over a month now - is it the policy of the site managers to include only implementations that guarantee 100% coverage of the spec?

I believe these implementations (validation and code generation) are valuable additions to their respective lists, and they offer useful resources for the Kotlin development world.

I have a more complete implementation (including 2020-12) under development, but it is always valuable to get feedback on the current version from more widespread usage.

@Relequestual - would you mind taking another look at my PR, and if you still find that my software does not qualify for inclusion in the lists, could you please suggest specific areas that need to be addressed?

Thanks!

@Relequestual
Copy link
Member

This has been marked as "On hold" for over a month now - is it the policy of the site managers to include only implementations that guarantee 100% coverage of the spec?

I believe these implementations (validation and code generation) are valuable additions to their respective lists, and they offer useful resources for the Kotlin development world.

Peter, sorry it's been on hold for so long. Although we now have one FTE our capacity is still limited.

We don't yet have a policy on this, and we review each PR as issues arise. (We are working on forming a policy.)

In the list of things not implemented, you list $recursiveRef, $recursiveAnchor, and $vocabulary.
I don't see how you can have a draft 2019-09 implementation without those keywords.
Without them, you are not able to process the meta-schema, and therefore you cannot validate provided schemas.

So, while you may have a partial draft-07 implementation, I don't see how you can have a 2019-09 implementation.

It makes it difficult to approve you PR, as by listing it on the site, even though we now specify it's not a recommendation, there's no clear way for people to see the level of reported compliance.

I'm not saying we should immediatly close this PR, but more I would like to hear your thoughts based on the above.

@pwall567
Copy link
Contributor Author

Hi Ben, thanks for getting back to me so promptly. I accept that it is not correct to say that my implementation is 2019-09 compliant when it doesn't cover some of the major features that distinguish this version. I listed 2019-09 because I had used that as my source document when developing, and I have included features from that specification such as minContains, maxContains, format: uuid and others. But I understand that this is not sufficient to qualify for a listing under 2019-09 and I have removed that from my PR. (I have at the same time extended my entry in the code generation list to include the fact that I also generate Java and TypeScript, as well as Kotlin.)

I hope this addresses your concerns.

@Relequestual
Copy link
Member

Thanks for understanding.

One final nitpick, which I should have asked in advance: It would be helpful if you could specify which draft you support in your projects read me, such like "Kotlin implementation of JSON Schema draft-07".

@pwall567
Copy link
Contributor Author

Done. 👍

@pwall567
Copy link
Contributor Author

Also made similar changes to the code generation project README, and to the GitHub summary text.

@Relequestual
Copy link
Member

Looking at your tests, it looks like you're only testing using the 2019-09 draft tests. Is that correct?
If not, where do I see your tests using the draft-07 tests?

@Relequestual Relequestual self-requested a review May 18, 2021 13:22
@pwall567
Copy link
Contributor Author

Fair comment. I added the tests when I still thought Draft 2019-09 was the target. I'll modify the test invocations to use Draft-07, but it's late at night here (Australia) so that will have to wait until the morning. Thanks for your help with all this.

@Relequestual
Copy link
Member

No problem, we are used to working in multiple timezones 😅

Feel free to join our friendly slack. You'll be in good company, including others in your timezone!

@karenetheridge
Copy link
Member

I don't think we should leave off an implementation entirely because it misses some bits, but we should try to list those limitations in short form.

@Relequestual
Copy link
Member

Sure, but as you know, we currently haven't defined an approach to achive this.

@pwall567
Copy link
Contributor Author

OK, the code that invokes the test suite has been switched to use the Draft-07 tests. As previously, some tests are skipped because they relate features not yet implemented (e.g.dependencies), but of the tests executed, 696 passed and 0 failed.

@Relequestual
Copy link
Member

I hadn't realised you weren't fully compliant with draft-07.
What happens when you encounter a keyword that's known but unimplemented such as dependencies?

Either way, I'd like to see the "notes" part of this PR changed to reflect there are some keyword not yet implemented. Something like "Full compliance in progress"

(We need to work out an approach for defining this across all implementation listings, so for now notes will have to do.)

@pwall567
Copy link
Contributor Author

Unrecognised keywords are silently ignored, as specified by Section 4.3.1.

I had always hoped to be very clear about the extent to which this implementation meets the specification, and I have extended the "notes" entry of my PR to better reflect the current state.

@Relequestual
Copy link
Member

Unknown keywords should be ignored, but these are keywords which you DO know about but choose not to implement. I'd say there's quite a difference here.

I'll review the PR changes shortly. Thanks!

@Relequestual Relequestual merged commit 71be5ec into json-schema-org:master May 21, 2021
@pwall567
Copy link
Contributor Author

Thanks very much.

You're right, there is a difference between unknown and unimplemented keywords. My first reaction was to treat unimplemented the same as unknown, but perhaps a warning may be more appropriate. I'll look into that.

@pwall567 pwall567 deleted the add-json-kotlin-schema branch May 23, 2021 13:45
@karenetheridge
Copy link
Member

I wrote a "lite" implementation that doesn't support some keywords (e.g. $dynamicRef), and when it encounters those keywords, it doesn't just fail to evaluate, it throws an exception -- to make absolutely sure that a schema with this keyword isn't accidentally used and produces an unexpected result (but perhaps not noticed as a problem).

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants