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

Add general validation principles and examples. #143

Closed
wants to merge 1 commit into from

Conversation

handrews
Copy link
Contributor

This addresses issue #55 plus concerns raised in the comments of
issue #101.

I replaced "linearity" with "independence" as I think it is
more general and intuitive.

The general considerations section has been reorganized
to start with the behavior of the empty schema, then explain
keyword independence, and finally cover container vs child
and type applicability, both of which flow directly from
keyword independence.

In draft 04, the wording obscured the connection between
keyword independence and container/child independence.
When we rewrote the array and object keywords to explicitly
classify each keyword as either validating the container
or the child, keyword independence became sufficient to
explain container/child independence.

The list of non-independent keywords has been updated, and
exceptions to the independence of parent and child schemas
have been documented. Finally, I added a comprehensive example
of the frequently-confusing lack of connection between
type and other keywords.

@awwright
Copy link
Member

I just made the spelling fixes identified...

A brief description of some of the principles is OK: By identifying patterns to look for, this helps implementors comprehend the document. But I'd like to avoid turning it into a manifesto.

The "Constraints and missing keywords" section is fine, can you split that off into a commit?

"Keyword independence" is fine.

I'm not sure what the "Validation of primitive types and child values" is doing. There's not really a useful pattern here to describe.

"Keyword applicability to instance primitive types" is really lengthy. I'm trying to make observations: "Most validation keywords..." sounds informative, "An important implication of keyword independence..." sounds more like normative text.

I would avoid using "incorrect" examples at all, like the one that lists "type": "number" inside a "not" block. Assume people are going to copy and paste examples without context.

@handrews
Copy link
Contributor Author

Thanks for the quick reply! I will break this up and re-submit the parts you think are fine as-is.

"Validation of primitive types and child values" is currently section 4.2. It's partially about the "context-free" nature of validation and I think it is important, but only if it's clear. Stating it up front was originally about making the behavior of various items/properties keywords more clear, but I think when I rewrote all of those a while back I made them clear enough on their own. You may be right that this is redundant now- I will just take it out and if I can think of anything that really needs to be said, I'll re-submit it separately.

I'll reply to the rest in a bit.

@handrews
Copy link
Contributor Author

For "Keyword applicability to instance primitive types", I added the example because we have had at least two people in the last few weeks file issues or bring up in comments that this principle baffled them, or that they thought it was wrong. I used the "multipleOf" example in one issue and someone responded that it was really helpful.

I agree that it is long, so compressing it would be good. This full example (with clear labeling of the "wrong" schema, which isn't actually wrong but just isn't useful) could move to the web site, but I really feel like some sort of example is called for rather than the one-liner that was there before that didn't really show the utility.

I'm trying to explain why type applicability is important. Perhaps just show the right way (the last code block) and explain how it works? That would be much more concise, and people could go look at the web site for more.

Let me know if you have other thoughts here, otherwise I will make a separate PR for this where we can discuss it further.

@handrews
Copy link
Contributor Author

@awwright see if this version suits you. I retained the two sections you said were fine, and moved the unchanged section to the end as I think it flows better:

  • First describe the general constraint concept and the default empty schema
  • Then explain keyword independence, the most general concept
  • Then explain type applicability (which actually follows from independence, but I left the wording completely unchanged- I'll come back to it in a separate PR)

@handrews
Copy link
Contributor Author

@awwright I stacked a commit for a much smaller change illustrating the applicability-to-types concept on top of this PR. That extra commit can be seen as a PR in my fork: handrews#1

@awwright
Copy link
Member

awwright commented Nov 17, 2016

I don't think it's accurate to say it's a 'consequence of the "type" keyword validation operating independently'.

Independence isn't the same thing as saying we try to narrowly limit the range of instances that keywords reject, so that we can avoid keywords with overlapping behavior.

Do we have any cases of people misunderstanding the section after reading it?

Finally let me take a look at this schema:

{
    "type": "number",
    "multipleOf": 1,
    "not": {
        "type": "number",
        "multipleOf": 2
    }
}

As far as I can tell, the addition of the second type:"number" doesn't do anything. All it's going to do is make the "not" successfully validate in cases already covered by the "type" keyword:

null // fails first keyword, since it's not a number
0.5 // fails second keyword, since it's not a multiple of 1
2 // fails third keyword, since it _passes_ the provided schema (instance is both number and multiple of 2)
1 // passes: since it's a number, integer, and fails the "not" schema as expected (instance fails the multipleOf:2 keyword)

I might be wrong on this though, I nearly sprained a brain muscle thinking about the "not" logic too hard.

@handrews
Copy link
Contributor Author

handrews commented Nov 17, 2016

[EDIT: I've rethought this a bit in a later comment]

I don't think it's accurate to say it's a 'consequence of the "type" keyword validation operating independently'.

I'd like to get at least one more person's opinion on that. To me it is exactly what happens and is the primary way that it makes sense to reason about this principle. The fact that we don't need an extra fundamental principle to explain this is a good thing. It means the behavior is inherently consistent already.

Independence isn't the same thing as saying we try to narrowly limit the range of instances that keywords reject, so that we can avoid keywords with overlapping behavior.

???

Independence is exactly the same as avoiding keywords with overlapping behavior. That's what it means.

Limiting the range of types is one way that this works but we do not have to add a principle for that. It is already true just because of independence.

@Relequestual can I get a 2nd opinion here? I think this is super-important.

@handrews
Copy link
Contributor Author

@awwright I removed the schema that confused you in handrews#1 . And now you've made my head hurt with the "not"... ugh, you might be right about that :-P

I just woke up so I'll think through it again after a cup of tea, and of course if the example does not actually exemplify the problem I will drop it or change it.

Are you still OK with the commits that I kept here in #143 ? I agree that the commit in handrews#1 is more questionable still.

@handrews
Copy link
Contributor Author

@awwright OK I think I see how you see this differently, in that we could have written into the rules that any value which does not explicitly satisfy the constraint will fail validation, no matter its type. That technically wouldn't tie it to the validation of "type", although the effect is the same.

I still think there's something here worth noting to make the overall theoretical framework simpler and more consistent, but I can see that my current wording is not necessarily conveying it well. Hopefully someone can help us converge on better wording (or validate one position or the other- I'm not dead set on it, of course, so if no one else who comments sees this my way I'll be happy to drop it).

@handrews
Copy link
Contributor Author

@awwright after further thought I'm setting aside the applicability change for now unless someone comes along and says they find it useful.

I've taken that out of this pull request entirely- are you OK with the remaining changes? I'm fine with leaving the PR open for two weeks, I just want to make sure that the PR as it stands now, without the "applicability" change, works for you.

@handrews
Copy link
Contributor Author

@awwright - I deleted the applicability branch from my fork. This PR is just about the default schema and independence now, so it should be what you want. If you could confirm that would be great.

@Relequestual Relequestual added this to the draft-6 (next draft) milestone Dec 5, 2016
@Relequestual
Copy link
Member

Sorry for the assignment spam @awwright - the request review function seems to be broken right now =[

@Relequestual Relequestual self-assigned this Dec 8, 2016
This paritally addresses issue json-schema-org#55 plus concerns raised in
the comments of issue json-schema-org#101.

I replaced "linearity" with "independence" as I think it is
more general and intuitive.

The general considerations section has been reorganized
to start with the behavior of the empty schema, then explain
keyword independence, and finally cover type applicability.

In draft 04, the wording obscured the connection between
keyword independence and container/child independence.
I thought I needed this primitive type vs child validation
section even with the rewritten keywords, but going over
it now based on feedback, I agree that it is superfluous.
@handrews
Copy link
Contributor Author

handrews commented Dec 13, 2016

Giving up.

[This is now so old that parts of it make no sense anyway]

@handrews handrews closed this Dec 13, 2016
handrews added a commit to handrews/json-schema-spec that referenced this pull request Dec 13, 2016
These are the leftover bits of Issue json-schema-org#55 and some clarifications
requested in a comment on issue json-schema-org#101 that have not already been
added in some other PR for some other issue.

These specific chagnes were previously approved in json-schema-org#143, but so
many other things have changed since json-schema-org#143 that most of it was
no longer relevant, so I closed it and started these changes over.

In particular, explaining {} and {"not": {}} is no longer needed
as they are covered while introducing "true" and "false" schemas
in the core specification, so that is no longer repeated in this
change.

Likewise, the parent/child validation descriptions have been
modified in several PRs and no longer has the problems that were
previously a concern.
handrews added a commit to handrews/json-schema-spec that referenced this pull request Dec 27, 2016
These are the leftover bits of Issue json-schema-org#55 and some clarifications
requested in a comment on issue json-schema-org#101 that have not already been
added in some other PR for some other issue.

These specific chagnes were previously approved in json-schema-org#143, but so
many other things have changed since json-schema-org#143 that most of it was
no longer relevant, so I closed it and started these changes over.

In particular, explaining {} and {"not": {}} is no longer needed
as they are covered while introducing "true" and "false" schemas
in the core specification, so that is no longer repeated in this
change.

Likewise, the parent/child validation descriptions have been
modified in several PRs and no longer has the problems that were
previously a concern.
@handrews handrews deleted the principles branch January 5, 2017 19:43
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