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

Validation conflict for "id" property #177

Closed
vearutop opened this issue Apr 10, 2017 · 7 comments
Closed

Validation conflict for "id" property #177

vearutop opened this issue Apr 10, 2017 · 7 comments

Comments

@vearutop
Copy link

According to spec https://github.com/json-schema-org/json-schema-spec/blob/master/archive/draft-04/schema.json#L32

        "id": {
            "type": "string",
            "format": "uri"
        },

Format uri is invalid for local references (https://github.com/json-schema-org/JSON-Schema-Test-Suite/blob/master/tests/draft4/optional/format.json#L44):

            {
                "description": "an invalid URI though valid URI reference",
                "data": "abc",
                "valid": false
            }

Which makes test data invalid for id property here https://github.com/json-schema-org/JSON-Schema-Test-Suite/blob/master/tests/draft4/refRemote.json#L57

            "items": {
                "id": "folder/",
                "items": {"$ref": "folderInteger.json"}
            }
@handrews
Copy link
Contributor

@vearutop The draft-04 meta-schema corresponds to draft-zyp-json-schema-04 and draft-fge-json-schema-validation-00. In those drafts, the "uri" format was unclearly specified and broadly interpreted to mean both URI and URI Reference.

This was not fixed until draft-wright-json-schema-validation-00 with "uriref" (which becomes "uri-ref" in the next draft for consistency with "date-time" and other compound names).

So I believe this test is correct against the draft-04 meta-schema. Or at least it enforces the way that the majority of implementations interpreted an unclear specification.

@vearutop
Copy link
Author

@handrews Which test you consider correct: format.json or refRemote.json?

I see inconsistency in these three code snippets that could be resolved in one of three ways:

  • id should not have "format":"uri", possibly it could have "format":"uri-reference" as in latest schema revision (not draft-04 though)
  • change test to allow "format:"uri" for local references (which does not make much sense for all other uri use-cases).
  • remove base URI change tests from refRemote.json

So my end-user problem is how to pass both tests gracefully in this unclear situation. Could you advice a solution?

Is format.json test marked as optional because of this?

@handrews
Copy link
Contributor

@vearutop ugh, I see what you mean now, my apologies.

format.json is marked optional because supporting the "format" keyword is optional: Conforming implementations are allowed to ignore it.

As for which is correct, I'm going to defer to others on that. I suspect many users of draft-04 expect "format": "uri" to accept URI References, but I'll leave it to @Julian (who owns the test suite), @epoberezkin (who I think added base URI test) and/or @awwright (who clarified how it all worked in the following draft) to make the call here. @Julian and @epoberezkin both maintain implementations and probably know what their users expect.

@Julian
Copy link
Member

Julian commented Apr 10, 2017

I've just gotten back from London, and now will be a bit tied up until Thursday, but basically the way I wrote the tests, and the way I think most implementers ended up going, was to call "format": "url" a mistake in the meta-schema for id -- i.e. to interpret url as being only URLs, but to indeed allow url references for the id validator.

Some of the tests have changed recently though, in an effort to make them more consistent, but clearly we're not all the way there yet.

If anyone has a chance to weigh in here, would love to hear it, otherwise I'll try to have a look later in the week.

@erayd
Copy link

erayd commented Apr 22, 2017

...was to call "format": "url" a mistake in the meta-schema...

@Julian Does that also apply to $ref for draft-03?

bighappyface pushed a commit to jsonrainbow/json-schema that referenced this issue May 16, 2017
…hema bug (#419)

* Split "uri" format into "uri" and "uri-reference"

* Correct format for id & $ref in draft-03/04 meta-schemas

See json-schema-org/JSON-Schema-Test-Suite#177 (comment)
erayd added a commit to erayd/json-schema that referenced this issue May 16, 2017
…hema bug (jsonrainbow#419)

* Split "uri" format into "uri" and "uri-reference"

* Correct format for id & $ref in draft-03/04 meta-schemas

See json-schema-org/JSON-Schema-Test-Suite#177 (comment)
bighappyface pushed a commit to jsonrainbow/json-schema that referenced this issue May 16, 2017
* Split $objectDefinition into $schema and $properties (#411)

Object validation attempts to use a single variable to store both the
object definition, and its properties. This causes validation to be
incomplete where "properties" is not set, but "additionalProperties" is.

This commit fixes both bugs in issue #353.

* Issue-414: Allow The Option of T or space for Date time. (#415)

* Testcase for minProperties with properties defined (#416)

+ Fix Test

* Tweak phpdocumentor dependency to avoid install conflicts (#421)

* [BUGFIX] Cast empty schema arrays to object (#409)

* Cast root to object

* Use function_exists to allow polyfill compatibility

* Move array->object conversion to SchemaConstraint & SchemaStorage

Fixes issue #408

* fix bug when applying defaults for array items when the schema is for (#405)

all items and add support for minItems when applying defaults

* [BUGFIX] Split "uri" format into "uri" & "uri-reference", fix meta-schema bug (#419)

* Split "uri" format into "uri" and "uri-reference"

* Correct format for id & $ref in draft-03/04 meta-schemas

See json-schema-org/JSON-Schema-Test-Suite#177 (comment)
@handrews
Copy link
Contributor

@Julian @vearutop @erayd my inclination is to leave things as they are here:

  • I don't think there's a whole lot of activity with draft-03 these days, beyond migrating off of it. I'm certainly not inclined spend time on fixing anything about it.
  • I agree with @Julian's view of there being a bug in the draft-04 meta-schema. I'll note that the draft-04 meta-schema doesn't actually even include the format keyword :-P It uses it, but does not describe it!

If we "fix" anything, I'd be inclined to re-publish the draft-04 schema with bugs fixed since it's not a normative part of the I-D. @awwright any thoughts on that?

I think that means we can close this, as the test case is correct.

@handrews
Copy link
Contributor

handrews commented Nov 8, 2017

The draft-04 meta-schemas have been fixed and re-published, so there is no longer a conflict here.

@handrews handrews closed this as completed Nov 8, 2017
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

4 participants