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

The serialized JSON Example do not pass against the schema #332

Closed
fabiojose opened this issue Oct 23, 2018 · 11 comments
Closed

The serialized JSON Example do not pass against the schema #332

fabiojose opened this issue Oct 23, 2018 · 11 comments
Labels

Comments

@fabiojose
Copy link
Contributor

Then validation against the schema produces the following error:

[ { keyword: 'format',
    dataPath: '.source',
    schemaPath: '#/definitions/source/format',
    params: { format: 'uri' },
    message: 'should match format "uri"' } ]

This is because of source value is /mycontext, that is an invalid URI.

A valid URI must have this format: URI = scheme ":" hier-part [ "?" query ] [ "#" fragment ]. Checkout here: https://www.ietf.org/rfc/rfc3986.txt

According to RFC 3986, Section 3, the scheme and hier-part are required.

Then, to work properly against the schema validation, I suggest an edit in the serialized JSON example, in source value example, changing it to from:/mycontext adding the scheme from:, as follows:

{
  "cloudEventsVersion":"0.1",
  "eventType":"com.example.someevent",
  "source":"from:/mycontext",
  "eventID":"A234-1234-1234",
  "eventTime":"2018-04-05T17:31:00Z",
  "comExampleExtension1":"value",
  "comExampleExtension2":{
    "otherValue":5
  },
  "contentType":"text/xml",
  "data":"<much wow=\"xml\"/>"
}

Or an adjustment in JSON Schema in source definition, changing it to another type instead of URI.

@cneijenhuis
Copy link
Contributor

Wow, good catch!

What do you think about using a common scheme, e.g. https, mailto or urn?
E.g. https://example.com/somethings/1234 looks good along com.event.someevent.

In the Context Attributes section, we have an example for eventType: com.github.pull.create. A good example for source is then https://github.com/cloudevents/spec/pull/123.
#123 is Source as a URI, a quite fitting example 😉

@fabiojose Do you want to open a PR for this one? In case you don't, I'm willing to help out.

@cneijenhuis
Copy link
Contributor

One could also change the spec to say URI Reference (see https://tools.ietf.org/html/rfc3986#section-4.1), which includes relative references such as /mycontext (I still think we can improve on that example).

This would also be more inline with current implementations, e.g. https://docs.microsoft.com/en-us/azure/event-grid/cloudevents-schema#cloudevent-schema

I think I'd prefer to force the full URI, though. In many cases, that would map to a URL that can be explored, whether it is a website, a REST API, ...

@fabiojose
Copy link
Contributor Author

@cneijenhuis, personally I'd prefer the full URI too. The problem with relatives references is that require some custom JSON Schema validator, this is common for browsers, but not so common in programming libraries. All of them treats uri type as full URI.

I think the best choice is to do some improvements on that examples. I will open a PR!

ps: nice initiative, CloudEvents! congratulations!

@duglin
Copy link
Collaborator

duglin commented Oct 24, 2018

I tend to prefer "URI-reference" instead because to me this is really nothing more than a string and we're just putting a little bit of structure around it by making it a URI to encourage a fairly consistent pattern - but still allows for a ton of freedom. In the end I suspect a lot of people will just do strcmp on it, but if they do need to break it apart then URI-reference still provides enough consistency/standardization to make it easy to automate it.

@JemDay
Copy link
Contributor

JemDay commented Oct 29, 2018

Is there any particular reason we don't simply define source as a string rather then try to force some particular syntax?

Shouldn't the structure (and interpretation) be publisher specific; surely Amazon should be able to use their ARN and Microsoft a '/' delimited path-like construct without constraint?

@cneijenhuis
Copy link
Contributor

@JemDay I think that standardizing the structure has an advantage for middlewares that want to allow routing based on the source field.
As an example, I may be interested in all issues within the cloudevents GitHub organization - so both /cloudevents/spec/issues/332 and /cloudevents/sdk-java/issues/2 are of interest to me.
I guess the middleware could always employ some regex stuff, but knowing that this is an URI, an expression like /cloudevents/*/issues/* is easier to grasp.

PS: ARN is a non-registered scheme, but is a valid URI, as far as I can see. And Microsoft is using valid relative URLs.

@JemDay
Copy link
Contributor

JemDay commented Oct 29, 2018

@cneijenhuis - I get your point but i'm unsure as to whether we should couple what the source is meant to represent and how people might choose to use it (ie a middleware topic). If AWS put their ARN into the source field i don't think you could automagically map that to a topic.

If there is such a desire then the field should be formally defined to meet that need; i personally dislike the URI-Reference approach as it feels a bit kludgy.

Agree that ARNs are private but they do follow the URN model (at least as i understand it) - in fact a URN might be more appropriate for a source if the structure needed to be formally defined.

@duglin duglin added the v0.2 label Oct 31, 2018
@cneijenhuis
Copy link
Contributor

@duglin Sorry for missing the call yesterday.

I listened to the recording - I'm not entirely sure if everyone got the implication that relative URLs are disallowed. At least I got it wrong in my implementation, and I guess Microsoft too (@clemensv can you confirm?).
CEVerify also seems to do it wrong: https://github.com/btbd/CEVerify/blob/master/main_test.go#L41

I think we should call this out in the release notes for 0.2, even though it is not a change - or have another PR to force a discussion.

@duglin
Copy link
Collaborator

duglin commented Nov 2, 2018

@cneijenhuis what do you think about changing the spec to allow for relative URIs?

@duglin
Copy link
Collaborator

duglin commented Nov 2, 2018

I'm a bit confused... the spec defines source as a URI per our data model, but the data model defines URI as this: URI - String expression conforming to URI-reference as defined in RFC 3986 §4.1.

Section 4.1 of RFC3986 defines "URI-reference" as: URI-reference = URI / relative-ref
and relative-ref as:

      relative-ref  = relative-part [ "?" query ] [ "#" fragment ]

      relative-part = "//" authority path-abempty
                    / path-absolute
                    / path-noscheme
                    / path-empty

And path-noscheme as path-noscheme = segment-nz-nc *( "/" segment ).
And segment-nz-nc as:

segment-nz-nc = 1*( unreserved / pct-encoded / sub-delims / "@" )
                 ; non-zero-length segment without any colon ":"

And to complete it we have:

   unreserved    = ALPHA / DIGIT / "-" / "." / "_" / "~"
   reserved      = gen-delims / sub-delims
   gen-delims    = ":" / "/" / "?" / "#" / "[" / "]" / "@"
   sub-delims    = "!" / "$" / "&" / "'" / "(" / ")"
                 / "*" / "+" / "," / ";" / "="

So to me URI can be just /myEndpoint, a relative URI, if we wanted. Am I not following this correctly?

@cneijenhuis
Copy link
Contributor

🤦‍♂️

Yes, as I said here #332 (comment) , according to RFC 3986 §4.1, /myEndpoint is fine.

I wasn't aware that URI is actually defined as URI-reference in the CloudEvents type system. Mea culpa, I should have checked that before commenting!

However, I feel that it is an easy mistake to make. spec.json also made that mistake, so at minimum, @fabiojose found a bug in the JSON Schema.

Now that I've put the time into finding all of that out, let me make a quick PR... 😉

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

4 participants