-
Notifications
You must be signed in to change notification settings - Fork 34
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
spec: Payload nits #15
Comments
Absolutely agree, let"s do that!
I agree that the "non-squashed" version is superior, but I think we should probably "strongly recommend it" rather than require it. I think we should probably discuss this stance further, but that was at least the initial idea behind these things.
I think when it comes to thin vs full (or anything in between): the right answer is "it depends". You probably shouldn"t be sending PII in webhooks for exactly the reasons you mentioned. Though the more data you send the easier it is for tools like Zapier to do really cool things with it. I think it"s good to think about it like you would about transactional email: you probably don"t want to send multi MB emails or include health records as an attachment, but it"s often useful to include an invoice, your name, etc. Looking forward to hearing your thoughts! |
I agree with this, in the meantime we describes the
Definitely, maybe another stance would be to create a spec for the payload, for the signature (which involves also headers) and for the other operational. Then a producer would be able to easily implement the signature part has it doesn"t requires and payload. (just a thought)
I"d say maybe not put PII in the examples might be a good thing then? |
Yup, I fully agree. They can unlock more value the more they support, and following the strict data structure is part of it.
I don"t think we necessarily need to split it to different specs, I think it"s OK to have them support different parts. We can even go further and define conformity levels, e.g: "full" means everything, "level 1" is just signature, "level 2" is signature + event types, etc. I think there are multiple ways around it, but we should probably focus on simplicity to start.
Yeah. 😅 We should probably switch to an example that has company name, some dollar amount, and a few opaque IDs or something. Got anything in mind? |
That"s kind of what i was meaning but
I"ll propose a change then |
Why not just recommend using the structure defined by https://cloudevents.io? They have already spent a lot of time thinking about event structure. I think Standard Webhooks and CloudEvents would work very well together. They have their own recommendations for WebHooks but yours seem more sophisticated and closer to what"s currently out there. Maybe you could work together on some level. |
@linuxbasic, there"s a ticket for it that I"ve been meaning to comment on. The gist is: we don"t currently have any requirements for how to structure the payloads, only recommendations. I think the recommendations are very close to cloudevents, e.g. the type is compatible, timestamp uses the same format for both (but slightly differently named). I think a worthy goal is to have the standard webhooks recommendation be fully compatible so that people can use cloud events and still benefit from tools that follow the recommendations here. Though we definitely need to think about this further. |
I"m aware that this spec only makes recommendations regarding the payload, and that"s a good decision. I can imagine that it is easier for people to justify the switch of Standard Webhooks if they don"t need to rewrite their payload structure. Like @rikbosch, it came into my mind that the payload recommendation looks a lot like a CloudEvent when I read the spec. My thought was that it would maybe make your life easier by just pointing the readers of your spec at the CloudEvents spec if they do not already have a clear vision for their payload structure. This way, you and your colleagues don"t need to spend brainpower and time discussing how to make the specs compatible. I also agree with @rikbosch that it would be advantageous for this spec to focus on the consumer/producer interactions (like security, and reliability) and treat the payload as a black box as it anyways can be anything. The CloudEvent folks should probably do the same regarding their WebHook recommendation at some point. |
If we switch payload to cloudevent spec then there is lot of benefit we can leverage. Majorly we can now go beyond JSON. We can then package binary payload and use content type to indicate what is the body. Binary payload with compression etc.. will be so much beneficial. |
Few comments regarding the payload spec.
timestamp
Currently the spec states
the timestamp of when the event occurred
. This also should mention the format; i"d vote forRFC 3339
format: https://datatracker.ietf.org/doc/html/rfc3339data
spec states:
It can either be passed as part of the data property, or squashed as part of the top-level object.
The drawback of having it squashed at the top level is that it could create collision. Let"s say i have
user.created
event and my payload is afull
and we also return the type of usertype: foo
; we have a collision. We should enforce the producer/consumer data in thedata
property. Added to this that would make us able to provide a JSON Schema for the standard-webhooks spec.full payload
I think we also should mention we might want to prefer using
thin payloads
because it reduces the potential of leeking PIIs over the channels.The text was updated successfully, but these errors were encountered: