Bug Fix: error syntax in schema definitions #444

xiekeyang · 2016-11-04T07:33:49Z

Validate manifest list JSON object via schema.Validator.

To validate full fields;
To validate required fields only;
To validate customized reference manifest media type;
To validate invalid media type case;
To validate invalid field type case;

Signed-off-by: xiekeyang [email protected]

wking · 2016-11-04T08:31:14Z

schema/manifestlist_test.go

+			manifestList: `
+{
+  "schemaVersion": 2,
+  "mediaType": "invalid"


Probably also want a test where schemaVersion is 0, although support for checking that is still in flight with #404.

It is reasonable and I take it in mind. This case should be added after #404 is landed.

#404 also adds testing for the self-deceiving media type you're attempting to verify here ;). I expect this case is failing for a different reason.

It is now on line https://github.com/opencontainers/image-spec/pull/444/files#diff-b1e07ac69261c70ec3feda53adffb906R34.

I read #404. It strict the manifest and list media type, but it seems not to set up unit test for manifest list media type, it just modify Backwards Compatibility, to let manifest list be compatible to Docker's. It doesn't test invalid string.

this pr looks not duplicated with #404, right, or I miss something?

Yeah, we want both this PR and #404. Which one lands first depends on whether we're doing Test-Driven Development ;).

@wking, maybe I express inaccurately. I mean if my test case is not redundant(as my explain above), and this PR looks OK?

https://github.com/opencontainers/image-spec/pull/444/files#diff-b1e07ac69261c70ec3feda53adffb906R29,

{ "schemaVersion": 2, "mediaType": "invalid", "manifests": [ { "mediaType": "application/vnd.oci.image.manifest.v1+json", "size": 7143, "digest": "sha256:e692418e4cbaf90ca69d05a66403747baa33ee08806650b51fab815ad7fc331f", "platform": { "architecture": "ppc64le", "os": "linux" } } ] }

wking · 2016-11-04T08:32:15Z

schema/manifestlist_test.go

+    },
+    {
+      "mediaType": "application/vnd.oci.image.manifest.v1+json",
+      "size": "7682",


No need to include multiple manifests to exercise this bit of the JSON Schema. Can we drop the sha256:e69… entry?

Although it is done in manifest_test.go, but I think we should still cover it here, against that schema definition for manifest and manifest list split. After all, they are different stories.
And I agree to test without required entry case.

wking · 2016-11-04T08:34:34Z

schema/manifestlist_test.go

+      }
+    },
+    {
+      "mediaType": "foo",


This is not a valid media type (you need both a type and a subtype). I recommend using text/plain or some other off-the-shelf, non OCI media type.

We have talked about how to look and handle the media type of reference fields. I remember we agree to only omit warning for not OCI canonical media type and later, we need to add strict mode to fail it.
In unit test, I propose to use free string to show the current compatibility and reality. Using restricted string maybe conceal the defect on design.
IMO.

The descriptor spec (which manifest-list leans on) requires this to be a media type (but doesn't require it to be a specific media type). So there are a few cases for validation:

a. A media type that implementations MUST support (currently just application/vnd.oci.image.manifest.v1+json).
b. Another media type (e.g. text/plain).
c. A non-media-type string (e.g. foo), which doesn't match RFC 4288's name requirements.
d. A non-string.

We may not be able to recognize the media types in (b), but we can distinguish them from the non-media-type strings in (c). I'd expect validation to fail for (c), but am happy to PR more explicit language to the spec before you make changes here.

@wking

You are right. Actually current schema exist some bugs, to lead illegal cases passed.
I fix them in 7f792fa and 2da8885.
With those supporting, we can test and get the excepted result. After fixing, I add the necessary test cases, including what you suggested.

2da8885 MUST be done, otherwise some customized but legal media types like application/octet-stream(Docker are using) will be rejected, and TestBackwardsCompatibilityManifest be failed.

I think this PR should has been finished, PTAL, thanks.

wking · 2016-11-04T08:35:26Z

schema/manifestlist_test.go

+  "annotations": {
+    "com.example.key1": "value1",
+    "com.example.key2": "value2"
+  }


You've already exercised annotations, so I'd leave it off of tests that focused on other properties.

xiekeyang · 2016-11-04T09:38:09Z

It is undated in a0e0290.
Drop some duplicated lines, and add some case missing required fields.

wking · 2016-11-05T21:48:56Z

schema/content-descriptor.json

    },
    "size": {
      "description": "the size in bytes of the referenced object",
      "$ref": "defs.json#/definitions/int64"
    },
    "digest": {
-      "$ref": "defs-image.json#definitions/digest"
+      "description": "the cryptographic checksum digest of the object, in the pattern '<hash>:<hexadecimal digest>'",


I'm fine with “the digest of the object referenced object”, to match the existing mediaType description. For more details, folks should be reading descriptor.md.

I carry on the description from https://github.com/opencontainers/image-spec/blob/master/schema/defs-image.json#L10 directly. I think it had better to keep same words for one entry.

Ah, fair enough. I can file a PR shooting for consistency between mediaType and digest descriptions if/when this PR lands.

wking · 2016-11-05T21:55:32Z

schema/content-descriptor.json

@@ -6,14 +6,15 @@
  "properties": {
    "mediaType": {
      "description": "the mediatype of the referenced object",
-      "$ref": "defs-image.json#definitions/mediaType"
+      "$ref": "defs-image.json#/definitions/mediaType"


+1 to this. Backing docs in RFC 6901 and the current JSON Schema draft.

wking · 2016-11-05T22:46:02Z

schema/defs-image.json

@@ -4,7 +4,7 @@
    "mediaType": {
      "id": "https://opencontainers.org/schema/image/mediaType",
      "type": "string",
-      "pattern": "^[a-z]+/[0-9a-zA-Z.+]+$"
+      "pattern": "^[a-z]+/[0-9a-zA-Z-_+.]+$"


This is still not quite right. I've spun off a PR focused on this issue and descriptor tests (pulling in one of your commits from this series) in #448. Although it would be good if we could get some of these validation PRs landed :p. Having so many in the air at once is making it hard to keep them orthogonal ;).

xiekeyang · 2016-11-06T04:52:11Z

@wking
I drop my commit about media type, which is not really quite right.
I patch your commit d05baa5 directly, for go pass the unit test(maybe we need more work about d05baa5).
Our PRs seems conflict, which should be resolved after one landed.

wking · 2016-11-06T06:23:40Z

On Sat, Nov 05, 2016 at 09:52:12PM -0700, xiekeyang wrote:

I patch your commit
d05baa5
directly, for go pass the unit test…

To unpack that comment a bit:

You've cherry-picked that in in place of your previous 2da8885. And
you need one or the other to cover application/octet-stream (from 1)
not fitting the current master's media type pattern 2.

Our PRs seems conflict, which should be resolved after one landed.

Sounds good.

xiekeyang · 2016-11-17T02:00:07Z

@stevvooe @vbatts This PR is fixing definitions bug in schema. I think it to be ready, and PTAL.

xiekeyang · 2016-11-17T07:44:22Z

@opencontainers/image-spec-maintainers
Rebased. Now I think it is ready to be merged. PTAL.

stevvooe · 2016-11-21T21:12:49Z

schema/defs-image.json

@@ -4,7 +4,7 @@
    "mediaType": {
      "id": "https://opencontainers.org/schema/image/mediaType",
      "type": "string",
-      "pattern": "^[a-z]+/[0-9a-zA-Z.+]+$"
+      "pattern": "^[A-Za-z0-9][A-Za-z0-9!#$&-^_.+]{0,126}/[A-Za-z0-9][A-Za-z0-9!#$&-^_.+]{0,126}$"


Is ^ and #` really allowed? What is the source of these additions?

I wonder if we should just do an enum here. Is there a way we can define an enum, then have callers of the validator add values to it as needed?

That change is cherry-picked from #448, which focuses on updating the mediaType pattern to match RFC 6838. And yeah, for some reason they allow both # and ^ :p.

I don't think an enum would work, because descriptors are generic. On the other hand, particular instances of descriptors may not be generic, and #404 is an example of using enums in those cases.

@stevvooe enum seems too strict, which will lead customized types failed during verifying. I checked RFC 6838, the pattern really follows it. I think RFC 6838 rule should be OK here.

xiekeyang · 2016-11-22T04:23:45Z

@stevvooe We've discussed in some issues before, and finally think it is best to let json schema pattern just cover spec declaration, and check future more via strict controllable function.
Customized media type is self-defined. We should allow and can't use enum to involve it.
I pick @wking commit to use media type pattern. I review it, and sure it match rfc6838, which we had better to follow up now.

xiekeyang · 2016-11-30T01:32:15Z

@stevvooe If the above comment #444 (comment) and this patch is acceptable? Thanks a lot!

vbatts · 2016-11-30T21:37:13Z

LGTM

xiekeyang · 2016-12-01T02:03:04Z

Rebased, PTAL, thanks a lot!

vbatts · 2016-12-01T15:35:14Z

LGTM

xiekeyang · 2016-12-02T02:17:03Z

@opencontainers/image-spec-maintainers I think this is ready to me merged. PTAL, thanks.

vbatts · 2016-12-03T15:30:41Z

LGTM

xiekeyang · 2017-01-14T03:19:05Z

@opencontainers/image-spec-maintainers rebased, PTAL.

xiekeyang · 2017-01-17T10:49:20Z

I worried that schema bugs will exist in spec without this PR. That let schema work incorrectly.

stevvooe · 2017-01-17T22:31:06Z

schema/manifest_test.go

+  "config": {
+    "mediaType": "application/vnd.oci.image.config.v1+json",
+    "size": 1470,
+    "digest": "sha256:c86f7763873b6c0aae22d9h3bab59b4f5debbed6685761b5951584f6efb0633b"


Could you make this less subtle?

There is an h in the middle for anyone not keeping count.

fixed in 003ac5f#diff-6493def706b32045f8d30e7a4b860c46R101

need re-LGTM thanks a lot!

stevvooe · 2017-01-17T22:32:44Z

LGTM

I do have a nit but you can PR that later.

stevvooe · 2017-01-18T03:30:24Z

LGTM

Signed-off-by: xiekeyang <[email protected]>

Previously we didn't define what a media type was and the old regular expression (from 818a56a, schema: image manifest, 2016-04-12, #18) was stricter than the RFC. The RFC rules are in [1], and the JSON Schema regexp syntax is in [2]. In the new pattern, * [A-Za-z0-9] is restricted-name-first (ALPHA / DIGIT), with ALPHA and DIGIT defined in RFC 5234 [3]. * [A-Za-z0-9!#$&-^_.+] is restricted-name-chars. [1]: https://tools.ietf.org/html/rfc6838#section-4.2 [2]: https://tools.ietf.org/html/draft-wright-json-schema-validation-00#section-3.3 [3]: https://tools.ietf.org/html/rfc5234#appendix-B.1 Signed-off-by: W. Trevor King <[email protected]> Removed the MUST for RFC 6838 from descriptor.md. Signed-off-by: xiekeyang <[email protected]>

Signed-off-by: xiekeyang <[email protected]>

xiekeyang · 2017-01-19T12:25:42Z

@stevvooe @vbatts Rebase again, please re-LGTM, thanks a lot!

stevvooe · 2017-01-20T22:41:04Z

LGTM

xiekeyang · 2017-01-23T14:58:29Z

@vbatts Please re-LGTM, thanks.

xiekeyang · 2017-01-24T23:40:33Z

@vbatts should be ready to merge.

vbatts · 2017-01-25T18:17:14Z

LGTM

wking reviewed Nov 4, 2016

View reviewed changes

xiekeyang changed the title ~~add manifest list json object unit test~~ Fix bugs of definitions in schema and add the necessary test cases Nov 5, 2016

wking mentioned this pull request Nov 5, 2016

schema: Add descriptor tests and fix mediaType pattern #448

Merged

wking reviewed Nov 5, 2016

View reviewed changes

wking mentioned this pull request Nov 8, 2016

Support different levels validation for JSON reader stream #452

Closed

stevvooe reviewed Nov 21, 2016

View reviewed changes

xiekeyang changed the title ~~Fix bugs of definitions in schema and add the necessary test cases~~ Bug Fix: error syntax in schema definitions Nov 26, 2016

stevvooe reviewed Jan 17, 2017

View reviewed changes

xiekeyang and others added 3 commits January 19, 2017 10:02

bug fix error syntax in schema definitions

2eebaf6

Signed-off-by: xiekeyang <[email protected]>

update schema fs

889a65a

Signed-off-by: xiekeyang <[email protected]>

add manifest and list unit test cases

ad8ade3

Signed-off-by: xiekeyang <[email protected]>

vbatts merged commit 111a99d into opencontainers:master Jan 25, 2017

wking mentioned this pull request Jan 26, 2017

.travis.yml: Rebuild schema-fs in the Travis tests #530

Closed

vbatts mentioned this pull request Jan 27, 2017

Bump version for v1.0.0-rc4 #537

Merged

xiekeyang deleted the ml-unit-test branch February 20, 2017 10:56

Bug Fix: error syntax in schema definitions #444

Bug Fix: error syntax in schema definitions #444

Conversation

xiekeyang commented Nov 4, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

xiekeyang Nov 4, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

xiekeyang Nov 4, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

xiekeyang Nov 5, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

xiekeyang commented Nov 4, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

xiekeyang commented Nov 6, 2016

wking commented Nov 6, 2016

xiekeyang commented Nov 17, 2016

xiekeyang commented Nov 17, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

xiekeyang commented Nov 22, 2016

xiekeyang commented Nov 30, 2016

vbatts commented Nov 30, 2016 • edited by caniszczyk Loading

xiekeyang commented Dec 1, 2016

vbatts commented Dec 1, 2016 • edited by caniszczyk Loading

xiekeyang commented Dec 2, 2016

vbatts commented Dec 3, 2016 • edited by caniszczyk Loading

xiekeyang commented Jan 14, 2017

xiekeyang commented Jan 17, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stevvooe commented Jan 17, 2017 • edited by caniszczyk Loading

stevvooe commented Jan 18, 2017 • edited by caniszczyk Loading

xiekeyang commented Jan 19, 2017

stevvooe commented Jan 20, 2017 • edited by caniszczyk Loading

xiekeyang commented Jan 23, 2017

xiekeyang commented Jan 24, 2017

vbatts commented Jan 25, 2017 • edited by caniszczyk Loading

xiekeyang Nov 4, 2016 •

edited

Loading

xiekeyang Nov 4, 2016 •

edited

Loading

xiekeyang Nov 5, 2016 •

edited

Loading

xiekeyang commented Nov 17, 2016 •

edited

Loading

vbatts commented Nov 30, 2016 •

edited by caniszczyk

Loading

vbatts commented Dec 1, 2016 •

edited by caniszczyk

Loading

vbatts commented Dec 3, 2016 •

edited by caniszczyk

Loading

stevvooe commented Jan 17, 2017 •

edited by caniszczyk

Loading

stevvooe commented Jan 18, 2017 •

edited by caniszczyk

Loading

stevvooe commented Jan 20, 2017 •

edited by caniszczyk

Loading

vbatts commented Jan 25, 2017 •

edited by caniszczyk

Loading