Skip to content

additionalProperties + oneOf = does not work #193

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

Closed
orbisvicis opened this issue Jan 21, 2015 · 10 comments
Closed

additionalProperties + oneOf = does not work #193

orbisvicis opened this issue Jan 21, 2015 · 10 comments

Comments

@orbisvicis
Copy link

additionalProperties does not work (does not inspect extended instances) with any of the combining schemas - allOf, anyOf, oneOf - when used with properties.

This is fine with allOf - write a pre-validation script to manually combine schemas. Not so fine with anyOf or oneOf.

Now the specification is vague (I think, I skimmed it; it might be clarified elsewhere) as to the behaviour of additionalProperties. Is there a problem changing it to support combiners?

@Julian
Copy link
Member

Julian commented Jan 21, 2015

It's not vague. This is how additionalProperties works it applies strictly to things that appear in properties within the current schema. You can have a look at some of the proposals for draft 5 to see some things that are meant to change this behavior.

@Julian Julian closed this as completed Jan 21, 2015
@orbisvicis
Copy link
Author

I was hoping you would could suggest a solution.

Argumentatively (squabbling semantics), though additionalProperties algorithm is clearly defined:

p The property set from "properties".

The definition of allOf seems to override how "properties" is interpreted:

An instance validates successfully against this keyword if it validates successfully against all schemas defined by this keyword's value.

That said, looking through the bug reports, the specification's creator seems quite involved with this project so I can't argue it isn't supposed to behave this way.

Anyway you referenced a workaround in #167 but I can't find the sample code anywhere. I'm not sure if it was specifically for allOf (pre-validation) or an implementation-hook that could also support oneOf.

So yeah... "looking" at a proposal for draft 5, without working implementation, is like watching water boil. I don't supposed jsonschema secretly supports some of these?

@Julian
Copy link
Member

Julian commented Jan 21, 2015

No, it doesn't "secretly support" anything. If you wanted an implementation, you'd write one, which is quite simple. Here's the sample code I referenced in that ticket, which you can find in one of the other related tickets here: #150

@orbisvicis
Copy link
Author

What do you think about json-schema/json-schema#151 ?

@Julian
Copy link
Member

Julian commented Jan 21, 2015

I think you'll find @fge has a proposal that uses existing specifications to do schema merging, you might find those interesting. Personally I'm in support of those I think (at least in the 10 minutes in which he explained them to me, I haven't had much time to read the mailing list or do very much otherwise recently :/).

But I'm also in support of a proposal by @geraintluff to change the default behavior of additionalProperties to do what you asked in draft 5 as well.

@orbisvicis
Copy link
Author

These problems regarding json-merge/json-patch were raised by @geraintluff

  • "... extra pre-processing step ..."
  • "... resuting JSON Schema produced by the merge has no definite URI ..."

Because the json-schema/json-schema#151 proposal - lets call it "$ref extension" - works within the existing $ref framework, these objections shouldn't apply.

My problem with json-merge, if I understand correctly, is that it indiscriminately replaces properties of the merge source, and so wouldn't work well with the "required" property. I believe json-patch can work around this, but requires a json-patch "diff" and so is like requiring multiple META_SCHEMA.

I also support the proposal (which I missed) to change the default behaviour of additionalProperties. That said extending $ref has other uses, so the former shouldn't obviate the latter.

@fge
Copy link

fge commented Jan 21, 2015

@orbisvicis

extra pre-processing step

Right now the only "pre processing step" so to speak is JSON Reference; this just adds one; what is more, JSON Reference still takes priority, and the merge process is defined by standards; therefore this is really a "non problem". Implementations just have to add a processing step in addition to JSON Reference...

resulting JSON Schema [...] has no definite URI

That is wrong. Of course it has. The URI you fetched it from.

My problem with json-merge, if I understand correctly, is that it indiscriminately replaces properties of the merge source, and so wouldn't work well with the "required" property.

Yes, which is why I have also proposed strictProperties; but as to replacing "indiscriminately", of course a merge-patch/patch will; JSON Schema is not intended to fool-proof you for all the mistakes you can make!

I also support the proposal (which I missed) to change the default behaviour of additionalProperties.

And I don't. This breaks backwards compatibility. $merge and $patch don't.

Note that $merge has already seen successful usage in production as well. I didn't propose that out of the blue...

@orbisvicis
Copy link
Author

That is wrong.

Those aren't my objections. In fact, like you, I disagree with the objections. I was just pointing out that they probably don't apply to this proposal.

JSON Schema is not intended to fool-proof you for all the mistakes you can make!

Given two schemas A and B both of which define required, json-merge-patch is unable to automatically create a schema C which merges the required values from both. It's not about making mistakes, it's about functionality.

If you say, "yes, just make the user inline the required property", then let me remind you that inlining was always a valid solution to the additionalProperties inheritance problem. The point of this proposal is to avoid any inlining.

And I don't. This breaks backwards compatibility. $merge and $patch don't.

I'm a new user, I don't have to worry about backwards compatibility :) That's a joke, you make a good point. They're still drafts though.

@fge
Copy link

fge commented Jan 21, 2015

They're still drafts though.

If you talk about JSON Patch and JSON Merge Patch, they are not; they are respectively RFC 6902 and RFC 7356.

Given two schemas A and B both of which define required, json-merge-patch is unable to automatically create a schema C which merges the required values from both.

Neither is JSON Patch. But here you talk about two independent schemas; this is not the purpose of either $merge or $patch; their intent is to apply transformations to an existing schema; nothing says that the patch to apply is a JSON Schema at all (in fact, in the $patch case they cannot be; a JSON Patch is an array).

If you have two schemas to obey, you should validate against them independently!

@orbisvicis
Copy link
Author

They're still drafts though.

I was referring to JSON schema.

See json-schema/json-schema#151 for the rest of the response; I'm merging these topics (they seem to be converging anyway).

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

No branches or pull requests

3 participants