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

Fix: validating same schema multiple times when it has $id #142

Merged
merged 1 commit into from
Oct 25, 2023

Conversation

cykoder
Copy link
Contributor

@cykoder cykoder commented Oct 11, 2023

AJV throws an error if you compile a schema with same ID twice. See: https://ajv.js.org/guide/managing-schemas.html

Signed-off-by: Sam Hellawell <sshellawell@gmail.com>
@vmidyllic
Copy link
Collaborator

@cykoder can you, please, add info on how to reproduce the issue? in which cases schema is compiled twice?

@cykoder
Copy link
Contributor Author

cykoder commented Oct 25, 2023

thought it was pretty self explanitory, any schema document with an $id field will be cached by AJV. simply try use a schema document with an $id field twice with the JS-SDK and you will get an error stopping credential issuance with that schema. example: https://schema.dock.io/CourseCertificate-V1-1695036447159.json

@vmidyllic vmidyllic merged commit d3f63d3 into 0xPolygonID:main Oct 25, 2023
1 check passed
@vmidyllic
Copy link
Collaborator

Thank you for your contribution.

Also, it may be helpful: while I was testing schemas you provided I encountered several issues: additionalProperties":false - this prevents the creation of credentials with credentialStatus field, as well as validation of the credentials with proof property in case it is not deleted.

Also, in the ld context there is a redefintion of protected term issuer from w3c schema. Playground example.

@cykoder
Copy link
Contributor Author

cykoder commented Oct 25, 2023

@vmidyllic no worries, thanks for merging.

additionalProperties":false - this prevents the creation of credentials with credentialStatus field,

in this case the schema wasnt used for credentials which require it, for our platform we allow additional properties by default. but thanks for the spot.

Also, in the ld context there is a redefintion of protected term issuer from w3c schema

interesting - using the jsonld library we have there are no errors about redefined terms as it should be nested requiring the CourseCertificate type. will check that edge case out (its meant to support credentialSubject.issuer)

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