dapr / docs

Dapr user documentation, used to build docs.dapr.io
https://docs.dapr.io
Creative Commons Attribution 4.0 International
993 stars 725 forks source link

[PubSub] CloudEvent docs not correct #3677

Open olitomlinson opened 1 year ago

olitomlinson commented 1 year ago

Given this section of the docs https://docs.dapr.io/developing-applications/building-blocks/pubsub/pubsub-cloudevents/#publish-your-own-cloudevent

There are a few statements which don't appear to be true.

If the CloudEvent that was authored by the app does not contain the minimum required fields in the CloudEvent specification, the message is rejected.

Given the following request :

> POST /v1.0/publish/redis-pubsub/orders HTTP/1.1
> Host: localhost:3501
> User-Agent: insomnia/2023.4.0
> dapr-app-id: myapp3
> Content-Type: application/cloudevents+json
> Accept: */*
> Content-Length: 34

{
    "data": {
        "some": "body"
    }
}

Produces the following CE envelope. :

{
  "data": {
    "some": "body"
  },
  "pubsubname": "redis-pubsub",
  "source": "Dapr",
  "specversion": "1.0",
  "time": "2023-08-13T19:40:24Z",
  "topic": "orders",
  "traceid": "00-00000000000000000000000000000000-0000000000000000-00",
  "traceparent": "00-00000000000000000000000000000000-0000000000000000-00",
  "tracestate": "",
  "type": "com.dapr.event.sent"
}
  1. As you can see, I did not specify any of the CE required properties (id, source, specversion & type) however Dapr allowed the message to be published, it did not reject the message as the docs imply.

  2. Interestingly the resulting CE envelope (despite Daprs augmentation) is technically invalid according to the CE spec, as the Id property is not present.

Given both points above, the expected behaviour here is hard to pin point. But If I was to hazard a guess...

This section should be adjusted :

If the CloudEvent that was authored by the app does not contain the minimum required fields in the CloudEvent specification, ~the message is rejected~. Dapr adds the following fields to the CloudEvent if they are not provided:

time traceid traceparent tracestate topic pubsubname source type specversion

To become :

If the CloudEvent that was authored by the app does not contain the minimum required fields in the CloudEvent specification, these will be added automatically by Dapr.

Dapr will also add the following fields to the CloudEvent if they are not provided :

time traceid traceparent tracestate topic pubsubname source type specversion id <- NEW

This still leaves my second point above unsatisfied, so I will raise a bug for that in dapr/dapr for that.

berndverst commented 1 year ago
  1. Dapr does not provide cloud event validation. This is the obligation of the sender.
  2. Dapr does not attempt to complete a cloud event missing required information. This is the obligation of the sender.
  3. However, if empty, Dapr will set source, type, specversion, and time (other fields may receive the default null value). No other fields are modified. This should not be interpreted as an attempt to create a complete cloud event.
olitomlinson commented 1 year ago
  1. Dapr does not provide cloud event validation. This is the obligation of the sender.

So I think you are agreeing that the doc should change to :

"If the CloudEvent that was authored by the app does not contain the minimum required fields in the CloudEvent specification, these will be added automatically by Dapr."

  1. Dapr does not attempt to complete a cloud event missing required information. This is the obligation of the sender.

Hmmm....as per my examples above, given that dapr is adding source, specversion & type due to them being missing, I'd say dapr is going to lengths to try and create a minimum valid payload. However it falls short, because it doesn't provide a CloudEvent Id

msfussell commented 1 year ago

@olitomlinson - It feel like CloudEvent id should be mandatory from dapr. This seems like a bug

hhunter-ms commented 1 year ago

@msfussell can you transfer to the correct repo, if it's a bug?

msfussell commented 1 year ago

@olitomlinson - Do you agree this is a bug? Is this both a bug and a doc update?

olitomlinson commented 1 year ago

@msfussell

There needs to be a docs update for sure. The docs currently state that the message will be rejected if it doesn't meet the mandatory CE envelope spec. Which is not true.


With regards to the missing Id field, I do believe this is a bug/incomplete feature. This is why I raised this https://github.com/dapr/dapr/issues/6801

My reasoning is purely for the fact that dapr appears to be going to lengths to augment missing CE fields, specifically mandatory fields, if they've not been provided by the user/author. ( source, specversion & type)

However, the id CE field is the odd one out, dapr isn't augmenting that. Which, doesn't make sense to me.

IMO Either Dapr should attempt to augment all the mandatory fields if they are missing (to create a valid CE envelope) or none of them.