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 applicator meta-schema for unevaluatedProperties #883

Merged

Conversation

jdesrosiers
Copy link
Member

While doing meta validation of the proposed OAS 3.1 meta-schema, I got an unexpected error that turned out to be caused by an error in the Draft 2019-09 applicator meta-schema for unevaluatedProperties. The OAS 3.1 meta-schema uses "unevaluatedProperties": false, which failed meta-validation because false is not an object.

@jdesrosiers
Copy link
Member Author

Has this fallen through the cracks? I'm surprised no one has acknowledged this yet. It's pretty important that this get fixed.

@handrews
Copy link
Contributor

handrews commented Apr 7, 2020

@jdesrosiers TBH I'm struggling to give a crap about anything tech-related right now.

As for this, yikes, yeah that needs to be fixed. In theory, that should roll the meta-schema date to 2020-04, but we're also hoping to have a new draft out (@Relequestual is keeping that moving).

This might arguably be a big enough bug to warrant replacing it in-place under 2019-09? idk, we didn't entirely sort out what the criteria are. @gregsdennis this is the sort of thing you've generally cared about, what do you think we should do here? I'm open to suggestion.

@Relequestual
Copy link
Member

Yikes! How did we miss this!?
I think this warrants a straight up in-place patch for 2019-09.

Given there's only two implementations (that we are aware of), and only ONE of them supports unevaluatedProperties, let's ping @gregsdennis to see if he's OK with an in-place patch on this. Assuming he is, we will need to also push this out to the website.

@Relequestual
Copy link
Member

@jdesrosiers As an aside, I'm struggling to find time to get moving at the pace I'd like. I'm working from home, yet it feels like I have less spare time, and am pretty exhausted. I have a tiny human too.
I want to step up my game this week, but I could have sworn it was only Tuesday today...

@gregsdennis
Copy link
Member

I'm good with this

@Relequestual
Copy link
Member

@jdesrosiers Given you're the other implementation, I am assuming you're OK with this change, and are happy to pick it up in your implementation too?

If so, I'll make this happen as soon as I can.

@awwright
Copy link
Member

awwright commented Apr 8, 2020

How did we miss this!?

No implementations, I'm guessing? Are there any tests related to this?

@Relequestual
Copy link
Member

I'm not sure how you would test this.
You would to create instances which contained inproper value types for each keyword, run validation, and check the results.
However, if you created the tests based on the meta-schema, you would have created an invalid test. The meta-schema is kind of an authority, although we say the spec document superseeds the meta-schema in all cases, should there be inconsistencies.

@Relequestual
Copy link
Member

I'm going to just do this. If there are any consequences, we can handle them.

@Relequestual Relequestual merged commit 4a122aa into json-schema-org:master Apr 8, 2020
Relequestual added a commit that referenced this pull request Apr 8, 2020
Fix applicator meta-schema for unevaluatedProperties
Relequestual added a commit to Relequestual/json-schema-org.github.io that referenced this pull request Apr 8, 2020
@jdesrosiers
Copy link
Member Author

I am assuming you're OK with this change

Yep. In fact I'm already bundling the patched version in my implementation. It affects my implementation even though I don't have unevaluatedProperties support yet. If someone uses unevaluatedProperties in a schema, instead of ignoring it, my implementation would throw a meta-validation error.

Are there any tests related to this?

The tests for unevaluatedProperties are minimal (I'm working on a more comprehensive set), but they are enough to expose this error for any implementation that uses the meta-schema for meta-validation. Even a value of false will cause a validation error.

@jdesrosiers
Copy link
Member Author

Also, I'm sorry if my nudge came off as pushy. I know everyone is doing their best. I just wanted to make sure this didn't get lost.

@Relequestual
Copy link
Member

@jdesrosiers Not at all. I even encourage nagging via slack DM or mention! Allows me to easily create a reminder that way too.

@handrews
Copy link
Contributor

handrews commented Apr 8, 2020

@jdesrosiers totally fine, my comment was intended just as an explanation of why I haven't been on top of things recently.

@gregsdennis
Copy link
Member

Are there any tests related to this?

There aren't in the suite, but the meta-schemas are supposed to be self-validating, so I have a few unit tests that check that. Unfortunately, this passes the self-validation because it's the true/false schemas that fail it, not the object form. So while tests technically exist, they didn't pick this up.

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

Successfully merging this pull request may close these issues.

None yet

5 participants