[#889]: Implement cloud events HTTP endpoint

[ctron]

This change adds support for receiving cloud events through the HTTP
binding.

The event's payload must be in the Ditto Protocol JSON.

Signed-off-by: Jens Reimann [email protected]

[thjaeckle]

Thank you very much for a great contribution 👍

I had a first look, found some things and mentioned them inline.

Other stuff which I could not add as inline-comments:

• Please adjust the documentation documentation/src/main/resources/_data/sidebars/ditto_sidebar.yaml and add the new page about the cloud events binding
  • I suggest below the "title: WebSocket protocol binding"
• please adjust the "Copyright holders" in legal/NOTICE.md
• a unit test would be great, but I suggest to add this in a follow-up PR to not reach the 1.000 LoC ;)
• please adjust the OpenAPI documentation with the new added endpoint: https://github.com/eclipse/ditto/tree/master/documentation/src/main/resources/openapi (cloud also be done in a follow-up PR)

A few "open" questions to me:

• Does the Eclipse Foundation release process allow to use "milestone" dependencies when releasing a non-milestone release? Did not find anything about that in the handbook
  • according to ClearlyDefined the cloudevents depencencies have a good enough score, so CQs are not required
• Regarding the "cloud events" fields:
  • Do we need to specify which values ce-type and ce-data-schema can take?

[ctron]

A few "open" questions to me:

• Does the Eclipse Foundation release process allow to use "milestone" dependencies when releasing a non-milestone release? Did not find anything about that in the handbook

It is a released version on Maven central. So that is stable enough. I am not aware of any limitations from the Eclipse Foundation side. Their focus is on availability and license compatibility.

• according to ClearlyDefined the cloudevents depencencies have a good enough score, so CQs are not required

Correct.

• Regarding the "cloud events" fields:

  • Do we need to specify which values ce-type and ce-data-schema can take?

Hm good question. The ca-data-schema already has the expectation in code of starting with ditto:. And that is part of the documentation. For the type, I am not sure and open for suggestions.

[ctron]

I amended the PR with changes to your feedback. I will now take a look at the documentation part of that.

[ctron]

I re-iterated over the documentation. Maybe have a second look.

[thjaeckle]

The file connectivity-protocol-bindings-cloudevents.md must now be removed and with it all links to connectivity-protocol-bindings-cloudevents.html

[thjaeckle]

Hm good question. The ca-data-schema already has the expectation in code of starting with ditto:. And that is part of the documentation. For the type, I am not sure and open for suggestions.

Like mentioned inline:

• I would suggest to remove specifying and validating the ca-dataschema again as this is optional in the CloudEvents spec
• I read again: the type is defined by the event producer, so we can't specify it

[ctron]

The file connectivity-protocol-bindings-cloudevents.md must now be removed and with it all links to connectivity-protocol-bindings-cloudevents.html

I thought I had done that. Then I had some merge issues. Will re-check.

[ctron]

Like mentioned inline:

* I would suggest to remove specifying and validating the `ca-dataschema` again as this is optional in the CloudEvents spec

My idea was to do some kind of validation to ensure that the data is actually suitable for Ditto. The field is optional, but still Ditto could require it.

But I don't have a strong opinion here. If you think it is better to remove this, then I will do that.

[thjaeckle]

My idea was to do some kind of validation to ensure that the data is actually suitable for Ditto. The field is optional, but still Ditto could require it.

But I don't have a strong opinion here. If you think it is better to remove this, then I will do that.

Ok, I would suggest to at least make it optional (so that it may be null), but if it is provided, it must start with ditto: (like it is currently implemented.

[thjaeckle]

Cloudevents Java SDK 2.0.0.RC1 was just released: https://github.com/cloudevents/sdk-java/releases/tag/2.0.0.RC1
Bleeding edge ;)

[ctron]

Cloudevents Java SDK 2.0.0.RC1 was just released: https://github.com/cloudevents/sdk-java/releases/tag/2.0.0.RC1
Bleeding edge ;)

Cool, let me trigger the clearlydefine harvester.

[thjaeckle]

I just manually invoked the /cloudevents resource.
When doing this successfully e.g. with a "create thing" Ditto Protocol message, the returned response is the same as invoking e.g. the POST /api/2/things resource: when creating a thing with 201 "created" outcome, the created thing JSON is returned.

I ask me whether this is an expected outcome.
I guess that the expected outcome would be also to get a response in Ditto Protocol JSON as I also sent this in.
Any thoughts about that, @ctron ?

[Yannic92]

I just manually invoked the /cloudevents resource.
When doing this successfully e.g. with a "create thing" Ditto Protocol message, the returned response is the same as invoking e.g. the POST /api/2/things resource: when creating a thing with 201 "created" outcome, the created thing JSON is returned.

I ask me whether this is an expected outcome.
I guess that the expected outcome would be also to get a response in Ditto Protocol JSON as I also sent this in.
Any thoughts about that, @ctron ?

I just manually invoked the /cloudevents resource.
When doing this successfully e.g. with a "create thing" Ditto Protocol message, the returned response is the same as invoking e.g. the POST /api/2/things resource: when creating a thing with 201 "created" outcome, the created thing JSON is returned.

Thought about this as well. Content negotiation would be awesome for this endpoint.
I'm however not sure if this is required for this first pull request. It's already pretty powerful. Thank you @ctron!
I think Ditto Protocol would be a reasonable default, because it's the same content-type as used for the command payload.
Maybe we could support content negotiation later?

[thjaeckle]

The spec for HTTP says on that:

The event binding is identical for both HTTP request and response messages.

So for HTTP responses there are some open issues:

• the content-mode must be preserved (when invocation did happen with "structured content mode", the response must be also in "structured content mode")
• the passed-in cloudevent headers have to be preserved and returned in the HTTP response
• IMO the response payload must also be in Ditto Protocol JSON

[ctron]

So for HTTP responses there are some open issues:

I would propose to handle responses in a new PR. There is no requirement to respond with a cloud event, Ditto can be seen as a "sink", only accepting messages without a full reply. "Full", in the sense of Cloud event response, it still would respond with an HTTP response that it accepted or rejected the HTTP request.

[ctron]

I just manually invoked the /cloudevents resource.
When doing this successfully e.g. with a "create thing" Ditto Protocol message, the returned response is the same as invoking e.g. the POST /api/2/things resource: when creating a thing with 201 "created" outcome, the created thing JSON is returned.
I ask me whether this is an expected outcome.
I guess that the expected outcome would be also to get a response in Ditto Protocol JSON as I also sent this in.
Any thoughts about that, @ctron ?

I just manually invoked the /cloudevents resource.
When doing this successfully e.g. with a "create thing" Ditto Protocol message, the returned response is the same as invoking e.g. the POST /api/2/things resource: when creating a thing with 201 "created" outcome, the created thing JSON is returned.

Thought about this as well. Content negotiation would be awesome for this endpoint.
I'm however not sure if this is required for this first pull request. It's already pretty powerful. Thank you @ctron!
I think Ditto Protocol would be a reasonable default, because it's the same content-type as used for the command payload.
Maybe we could support content negotiation later?

I am not sure what you have in mind with "content negotiation". From the HTTP binding spec's perspective there are two (or three) modes available. Which get selected through the Content-Type header. "Structured" and "binary". That is why I was referring the docs to "data content type" and not to "Content-Type", because the latter is an implementation detail of the binary encoding.

I think it is ok for Ditto to enforce the "data content type", but that would not be done through the HTTP Content-Type header, but through the parsed Cloud Event "data content type" field.

The current implementation already support both binary and structured mode. Only "batch" is not supported, but that should require some extra understanding of both communication parties anyway:

The batched mode MUST NOT be used unless solicited, and the gesture SHOULD allow the receiver to choose the maximum size of a batch.

[Yannic92]

I am not sure what you have in mind with "content negotiation". From the HTTP binding spec's perspective there are two (or three) modes available. Which get selected through the Content-Type header. "Structured" and "binary". That is why I was referring the docs to "data content type" and not to "Content-Type", because the latter is an implementation detail of the binary encoding.

What I had in mind was to define the content-type returned in the response by setting the Accept-header in the request, like describe here.

Based on this, ditto could either return the application/json which it returns for the other HTTP endpoints or it could return application/vnd.eclipse.ditto+json.

But as already mentioned: I think its not required to be part of this pull request. I however think that the default content-type should be application/vnd.eclipse.ditto+json, because that's what's provided in the request.
It seems to be intuitive for me to get the same structure of payload in the response that is provided in the request.

[ctron]

I am not sure what you have in mind with "content negotiation". From the HTTP binding spec's perspective there are two (or three) modes available. Which get selected through the Content-Type header. "Structured" and "binary". That is why I was referring the docs to "data content type" and not to "Content-Type", because the latter is an implementation detail of the binary encoding.

What I had in mind was to define the content-type returned in the response by setting the Accept-header in the request, like describe here.

Based on this, ditto could either return the application/json which it returns for the other HTTP endpoints or it could return application/vnd.eclipse.ditto+json.

But as already mentioned: I think its not required to be part of this pull request. I however think that the default content-type should be application/vnd.eclipse.ditto+json, because that's what's provided in the request.
It seems to be intuitive for me to get the same structure of payload in the response that is provided in the request.

I am not sure what the response would be? But maybe we discuss this in a new issue, which could focus in the response, or there is one. Right now, it simply accepts incoming cloud events. And the response is, that it accepted/rejected the event.

I am not sure how the Accept header maps to the cloud events spec though. Or if that is concept which should be used in this context. But we can discuss this in a new issue/PR.

[thjaeckle]

I added some (hopefully last) comments to the latest adjustments.
FMPOV we would be good to merge then and follow up (or create issues) with the discussed other stuff:

• unit tests
• OpenAPI documentation
• responding with Ditto Protocol JSON
  • potentially with what @Yannic92 suggested (negotiation): interpreting the Accept header
• accepting Ditto Protocol JSON specific content-type

What I think would be useful until the response-handling is clarified:

• statically set "response-required" to false for all incoming cloudevents
• however, use the requested acknowledgement "twin-persisted" in order to block responding to the HTTP request until successfully persisted in Ditto
  • the benefit would be that
    • in a success case, the HTTP response will be 202 (and empty payload)
    • in an error case, the HTTP response will reflect the error which occured (e.g. missing permissions, bad formatted JSON, etc.)
  • WDYT, @Yannic92 - should that work?

[ctron]

I amended the PR based on your feedback. Please double check the addition of the header, I am not 100% this is what you had in mind.

[thjaeckle]

I pushed one commit to your branch and hope you don't mind.
That was easier for me instead of starting commenting all over again.

I changed the header-thing I mentioned (response-required=false and requested-acks=twin-persisted) and ensure that it works that way - your variant was the right direction, however the invoked .setDittoHeaders(DittoHeaders) did overwrite the headers again.
For that reason, we also don't need the DittoProtocolAdapter to be initialized in a special way.

I also had to fix a bug in AbstractShardedPersistenceActor which dropped errors when no response was required, but "twin-persisted" acknowledgement was requested.

[ctron]

I pushed one commit to your branch and hope you don't mind.

Not at all! 👍

[thjaeckle]

Added mandatory follow-ups in #903