Skip to content

tests: add failing test case #139

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

Merged
merged 1 commit into from
Oct 24, 2020
Merged

Conversation

zepatrik
Copy link
Contributor

This adds test cases to demonstrate #138
I am not really sure where to start fixing this. It is important to note that the case using draft 2019-09 passes, while it fails when using draft 07.
If there is any direction you can point me to, I will try to fix this.

@GerryFerdinandus
Copy link

Additional information.
By removing the "$schema" definition line will also pass the test.

@zepatrik
Copy link
Contributor Author

zepatrik commented Oct 21, 2020

I guess because it defaults to the most recent one, that is draft 2019-09?

@ChALkeR
Copy link
Contributor

ChALkeR commented Oct 23, 2020

I'll make a fix for this.
The issue is that schemasafe always adds a type: 'string' default to the propertyNames schema here:

const nameSchema = typeof names === 'object' ? { type: 'string', ...names } : names

But in pre-2019 mode, combining $ref and any other keywords ignores everything except $ref per spec, this is why it complains on unprocessed type.

A simple fix would be to replace { type: 'string', ...names } with { type: 'string', allOf: [names]} on the line above, but I'll recheck if there is a cleaner solution.

As a work-around, { allOf: [{ $ref: ... }] } should work with all versions.

@zepatrik
Copy link
Contributor Author

Ok nice, thanks for your response 👍

@ChALkeR
Copy link
Contributor

ChALkeR commented Oct 24, 2020

CI didn't fail here as it should have because it was not enabled for PRs from external forks.
Should be fixed in acf9751.

Original issue fixed in aa8fd57.

This is not the correct location for this test, as test/extra-tests/ref.383.json is manually synced with json-schema-org/JSON-Schema-Test-Suite#383 (as are other prs in that dir), but I'll fix (and document) that post-merge. It wasn't documented.
Upd: done in e582f39...4faa854.

Thanks again!

@ChALkeR ChALkeR merged commit e582f39 into ExodusMovement:master Oct 24, 2020
@zepatrik zepatrik deleted the issue-138-demo branch October 24, 2020 23:53
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