-
-
Notifications
You must be signed in to change notification settings - Fork 313
Should unevaluatedProperties
count annotations from if
?
#1093
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
Comments
edit: I am changing my vote to 0, in the face of compelling arguments from the other side. :) |
I also wondered why this is the specified behavior, and had intended to open an issue about it. it is counterintuitive to me that annotations from |
I absolutely intended for annotations to be collected from The validation result from
When I wrote "in the usual way" I meant collecting them and propagating them up as normal if the including when the keyword is present without either "then" or "else" means that you can use a standalone |
So are you saying that |
That seems to be the gist of it. Why do you ask? |
Do you remember what the use case you had in mind for this was @handrews ?
Yeah, that's how I read the spec, and why I asked for the issue to be raised. I certinly didn't expect this to be the case, and I'd have raised a concern if I'd have understood the implications at the time.
I actually think this is preferable design, which I often suggest, but putting minimal content in
I truly expected However, having said all that, I can't think of an example of where this (as is currently) would be a problem, and I can think of potential use cases in terms of annotation collections beyond just for This leaves me feeling a little conflicted. |
Taking @jdesrosiers implication pattern example from the first post. If you remove the [some time passes] It appears I can't build schemas today... |
It seems fine to me that Annotations are the mechanism by which keywords can know that other keywords are present. If no annotation is present, it should be equivalent to the keyword that generates it being absent. But that's not the case for In other words, |
That feels like an abuse of the keyword to me, but I could be convinced otherwise with a good example.
💯 Completely agree |
That's not unique. maxContains and minContains are also dependent on the presence of another keyword (contains), and if it is not present, these keywords do nothing |
Abuse is a bit negative.. how about "(sometimes) convenient side effect". I view the |
Ooof. I am shocked to discover that section 7.2 reads:
instead of (note the boldface addition):
The whole point of this section is that you can define an execution order for certain keywords with respect to other keywords or other keyword classes (e.g. in-place applicators), and affect the later-executing keywords with the results of the earlier-executing keywords. While this is most notably done with annotation results, it was supposed to allow using either or both types of results.
I might be missing something but you get the point. I thought I had a very clear memory of writing that paragraph with both kinds of results, but I guess I only thought about it? It was something I figured out after first writing the part about annotations, so it's not too shocking to find that I never actually fixed it, I suppose. However, the specification for
"validation result" here is synonymous with "assertion result," I'd always meant to go back and replace all of the "validation result"s with "assertion result"s as it is more generic, but apparently I never did. I consider this a simple error in section 7.2. The phrase or assertion was intended to be in that sentence and got left out by error. I'm tempted to file it as errata on the draft, that's the degree to which I consider this to just be a bug/typo. To sum up:
All of this is necessary for it (including interaction with
Yes, the |
(and yes, I missed the |
This doesn't make sense to me. If your condition is a test against property "foo", and your remaining logic has to do with property "bar", then schema authors "break[ing] up their validation into the conditional logic and remaining subschema application logic" would exactly mean putting the subschema for "foo" under I can definitely see an abstract argument that the entire Part of the whole point of |
I get where that is coming from, but if you think of a standalone |
All I meant by the "abuse of the keyword" comment was that if that's the only reason we would want
I'm not following why not collecting |
One of the big changes moving from Manatee.Json and writing json-everything was actually processing annotations. I made a lot of discoveries in doing so. My new implementation absolutely breaks |
@jdesrosiers in an annotation-based implementation, this schema (I'm including {
"if": {
"required": ["foo"],
"properties": {"foo": {"const": true}}
},
"then": {
"required": ["bar"],
"properties": {"bar": {"type": "string"}}
},
"else": true,
"unevaluatedProperties": false
} This instance should pass: If Note that {
"oneOf": [
{
"allOf": [
{
"required": ["foo"],
"properties": {"foo": {"const": true}}
},
{
"required": ["bar"],
"properties": {"bar": {"type": "string"}}
}
]
},
{
"allOf": [
{
"not": {
"required": ["foo"],
"properties": {"foo": {"const": true}}
}
},
true
]
}
],
"unevaluatedProperties": false
} Does this make it clear? These schemas are only equivalent with the behavior I intended to specify. The For |
@handrews Thanks for taking the time to write up that example, but I'm not sure it answers my question. That explains how it works, but I don't see how it explains why it can't work without |
OK how in the heck do you both change the validation behavior and say that it still "works"? What definition of "works" are you using here? I'm extremely confused at this point. The correct definition of "works" is what I wrote in the spec. The reason that that is correct is not because I'm the one who wrote it, but because That is how the keyword was intended to work, and any other outcome is wrong. |
@handrews I don't think there is any argument about what the spec says and how it currently works. The issue was about whether or not the spec should be changed. What I'm attempting to facilitate conversation around is whether there is any reason it couldn't or shouldn't be changed. Would changing it make things harder for schema authors? Would schema authors not be able to express something they couldn't before? Does it fit into the JSON Schema architecture or do implementers need to hack something to make it fit? The change would "work" if it doesn't remove the ability to express any assertions you could before and it can be implemented in a reasonable way. I think the assertions you are able to express and the ease of implementation are the same which ever way it is defined. If I'm right about that, the only questions left to answer are (1) which equal alternative do we prefer, and (2) is it worth making a breaking change for something that isn't broken even if we decide it's not what we prefer? (I'm aware that |
OK, I had thought it was more a question about what the spec says for some reason. I take it you mean this question:
And I have said repeatedly that having to duplicate the evaluation of a property that is evaluated in the Related to that objection of mine... @Relequestual I might have just realized why your comment confused me. Regarding this statement:
as a reason for {
"if": {
"required": ["foo"],
"properties": {"foo": {"const": true}}
},
"then": {
"required": ["bar"],
"properties": {"bar": {"type": "string"}}
}
} as in, "foo" was validated under But if, by remaining you meant "all logic that has to do with validating, regardless of what logic is used in the conditional," then I guess you would call this the best practice: {
"if": {
"required": ["foo"],
"properties": {"foo": {"const": true}}
},
"then": {
"required": ["foo", "bar"],
"properties": {
"foo": {"const": true},
"bar": {"type": "string"}
}
}
} as now your Ultimately I'm very much against changing this. It works in a way that is consistent with everything else and does what we said we were going to do when we implemented both @jdesrosiers you gave an example with I don't consider the "any of not-thing or stuff" conversion to be correct, so making
I'm not convinced, but I definitely dispute that these are the only considerations. It behaves consistently the way it is, and it would not be consistent the other way around. Of course if you're willing to just shove nots/anyOfs/oneOfs/allOfs around arbitrarily until they support your point, you can claim that you can express whatever either way, and if we'd never previously discussed conversions that could be a valid debate. But the Given the conversion that Evgeny used originally and I used a few comments up, annotation collection has to work the way I specified it. |
@Relequestual's argument refutes this, but you're absolutely right that this hasn't been properly explained. This is the pattern I try to teach people and I'm pretty sure is very close if not exactly what @Relequestual was talking about. {
"type": "object",
"properties": {
"foo": { "const": true },
"bar": { "type": "string" }
},
"unevaluatedProperties": false,
"allOf": [{ "$ref": "#/$defs/if-foo-is-true-then-bar-is-required" }],
"$defs": {
"if-foo-is-true-then-bar-is-required": {
"if": {
"properties": {
"foo": { "const": true }
},
"required": ["foo"]
},
"then": { "required": ["bar"] }
}
}
} There are three distinct sections. In the first section, all properties and their general constraints are defined. In the second section, any conditionals or other complex constraints are referenced using definitions that give the constraint a clear description. Developers can now read just the first few lines of straightforward schema and have a good understanding of everything this schema describes. They can continue to read if they want to learn more. If "foo" is only defined in the (Not really relevant, but I'll mention it in case it comes up ...) If the intent is that, if foo is not true then "bar" should be unevaluated rather than optional, then you would have to define "bar" in the
I can't parse half of that sentence, but I can say for sure that it is the same thing as Evgeny's pattern. I did the math just to be sure.
This statement is unnecessarily aggressive. Let's keep this civil. Boolean implication is far from "arbitrary". Implication is the standard way to express conditionals in boolean algebra. It's denoted I'm convinced by Evgeny's non-standard expression of a conditional that it can be expressed in a way that doesn't require the if schema to be negated like implication does. Therefore, any argument about how |
OK it sounds like we're in agreement on a course of (non)action, so the main point here is probably resolved (I'll leave this open in case @Relequestual still wants to chime in). [EDIT: this part came across as disparaging towards @jdesrosiers when it was absolutely not intended to- see comment further down for details and apology.] Regarding what you would recommend as a best practice for schema organization: that is pretty much the opposite of what I would recommend 😅 And while I do not maintain an implementation, I have worked with very large schema collections used by teams of wildly different technical experience levels, across global time zones, and different industries. So unlike my opinions on implementation factors, my views on what does and doesn't work in schema authorship are rooted in reality. That definitely does not mean your example is in any way "wrong," but unless we have a consensus as an org on what a best practice really is, using our personal schema design styles to justify features is not supportable. There needs to be more to it than one or even a few of us liking things a certain way. I'm not going to compare our recommendations because, again, I don't think your recommendation is wrong, and I have no desire to try to win a point here (and I don't think you're trying to do that either). Finally, on a more important point:
Yeah, that's fair. Ironically I had actually reworked it to be less aggressive but either my notion of "less aggressive" was impaired at the time or I accidentally reverted to the earlier sentence. Either way, it shouldn't have been there and I apologize. This is something that I need to do better with. I also want to bring up another aspect of keeping things civil, which is respecting past work. This is a tricky topic because of course we have changed many things over the years with JSON Schema. I started to examine that in detail in this comment, but I think that conversation belongs somewhere else. I know @Relequestual has been setting up other process sorts of things, so perhaps he has thoughts on where that should go? |
What @jdesrosiers shows in the schema in #1093 (comment) is what I'm talking about. I try to write code, and schemas, for people to read and understand. There are multiple situations where I feel this approach to writing schemas makes things easier, but I won't go into details here as it's not the place and people still need to buy my book... 😅 Just because I feel that one approach is best practice, it doesn't mean the other approach is invalid or wrong. I can understand the reasoning behind it being set this way, and we have tests for it. I was just not expecting it. On balance, I feel the best thing to do here is keep things "as is" and wait to see how the community reacts, and if any problems are identified. As I opened this issue, I'll close it. @handrews if you want to open an issue in relation to #1093 (comment), please do! I don't see why it can't, and shouldn't, allow for both, as I believe you're suggesting it should. |
It's been brought to my attention that the way I described how I would handle the scenario being discussed appears to disparage what @jdesrosiers wrote as a solution. And further, that my citing of my experience reads as implying that he lacks such experience (which is not true) as a way to invalidate his argument. I apologize for all of that- I should have been much more careful about acknowledging the validity and authority of his proposal. My intent was simply to point out that there are two valid, real-world ways to approach that problem, and therefore neither approach proves much of anything since both are valid. |
The current interpretation is that
if
does count annotations if it passes. We have tests that check for this and every current implementation behaves that way. However, @Relequestual brought up on slack that annotations should never count forif
because it's just a conditional whose purpose is to select betweenthen
andelse
.Here's an example that illustrates the consequences of making such a change.
If we don't count
if
annotations, then there is no value that matches theif
and passes validation over the whole schema.{ "foo": "then", "bar": "" }
would pass theif
/then
, but fail theunevaluatedProperties
. The "foo" property declaration would have to be added outside of theif
. If we can determine any reason why that would be a bad thing, then we shouldn't change anything. I can't think of any good reason for it.Another way to think about it is, what is the behavior of
unevaluatedProperties
using the Implication Pattern.if
/then
is supposed to be sugar for the Implication Pattern, so I would expect them to work the same withunevaluatedProperties
. Here's the same example as above except with implication,Now the annotations from the "if" don't apply because of the
not
schema. So, that indicates that it might be desirable forif
to not count annotations to be compatible with implication. To me, that's a pretty convincing argument that we go this wrong and annotations fromif
schemas should never be counted.The text was updated successfully, but these errors were encountered: