Repository navigation
Conversation
|
I’ve moved this PR to draft while I revise the change to preserve the existing Please hold off on further review for now. I’ll update the patch and mark the PR ready once the revisions and tests are complete. Thanks for your patience. |
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
The writer has unresolved conflicting-attribute and nil-deletion handling issues.
Get a fresh assessment by requesting another Copilot review.
Review effort: Lite
Findings: 1
Open (1)
What changed in this PR
Updates Pub/Sub binary CloudEvents encoding to write data content type as ce-datacontenttype while retaining legacy decoding.
Changes:
- Updates outgoing attribute mapping.
- Adds encoding, round-trip, and compatibility tests.
- Updates publish-request assertions.
| File | Summary | Findings |
|---|---|---|
protocol/pubsub/v2/write_pubsub_message.go |
Changes outgoing attribute mapping. | Critical (1 vote): Remove conflicting legacy Content-Type when setting the new attribute. Moderate (1 vote): Return immediately when deleting a nil-valued attribute. |
protocol/pubsub/v2/write_pubsub_message_test.go |
Tests binary encoding and round trips. | — |
protocol/pubsub/v2/protocol_test.go |
Expects the new publish attribute. | — |
protocol/pubsub/v2/message_test.go |
Tests decoding both attribute names. | — |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
7c53e92 to
762008a
Compare
pbandj082
changed the title
|
The revisions are complete, and this PR is ready for review again. The patch retains legacy The local unit-test script passed across all 28 modules. GitHub Actions currently reports Thanks for your patience. |
|
@duglin would like your 👀 on this one. Ideally, we'd normalize all to lower when comparing values to simplify the logic, but that in itself is a breaking change. |
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
Structured-mode reuse must clear stale ce-datacontenttype and legacy Content-Type attributes.
Get a fresh assessment by requesting another Copilot review.
Review effort: Lite
Findings: 1
Resolved since last review (1)
| if b.Attributes == nil { | ||
| b.Attributes = make(map[string]string) | ||
| } | ||
| b.Attributes[contentType] = f.MediaType() |
There was a problem hiding this comment.
Addressed in 702a45b4.
This follow-up addresses a pre-existing issue with reusing a destination Pub/Sub message: attributes or data omitted by the next event could remain from the previous event.
This goes beyond #812's content-type mapping fix, but I'm including it as a separate commit in this PR because it was identified while addressing the review feedback on stale content-type attributes.
The change clears ce-*, content-type, and legacy Content-Type attributes while preserving custom Pub/Sub attributes. It also clears previous data when starting binary output. Regression tests cover both binary and structured output, for CloudEvents 0.3 and 1.0.
I believe this was purposely done to avoid duplicating information already present in the message and because we wanted CE to use existing technology whenever possible, not reinvent things - which in this case means: use the well-established HTTP headers if they exist. |
|
Thanks for the feedback, @duglin and @embano1. @duglin, I understand the HTTP binding reference as background on the design principle of reusing a protocol’s existing content-type metadata and avoiding duplicate information. This PR changes Pub/Sub @embano1, I understand the normalization suggestion to mean normalizing attribute names to lowercase when comparing them within the Pub/Sub binding. Given the compatibility implications you mentioned, I’ve understood this as a possible future improvement rather than a required change in this PR. |
|
@pbandj082 it sounds to me like you're saying Google's pub/sub is wrong and violates the spec, right? I'd prefer if we started with getting them to fix their bug - after all, it's just in a sample (at least per that page you referenced). |
|
Thanks, @duglin. I may not have explained my interpretation clearly. I wasn't intending to say that Google's Pub/Sub violates the CloudEvents spec. I was reading that document as a Pub/Sub-specific binding draft. My understanding was that the prohibition in HTTP binding §3.1.1 applies to HTTP headers, while the CloudEvents core specification leaves the content-type mapping rules to the respective protocol bindings. I was also relying on the draft's text beyond the examples: §3.1.3 requires all CloudEvents attributes to be mapped individually to Pub/Sub attributes, and §3.1.3.1 defines the That is the interpretation behind this PR. I recognize that the draft itself may need clarification if this was not the intended Pub/Sub mapping. |
Publish ce-datacontenttype alongside legacy Content-Type and keep both attributes synchronized when setting or deleting the data content type. Use lowercase content-type to detect and publish structured events, with an uppercase fallback for legacy input. Prefer a known structured format over duplicated ce-specversion metadata. Reject conflicting binary content-type and ce-datacontenttype values before copying the event. Document compatibility changes and cover attribute combinations, prepopulated destinations, encoding changes, and CloudEvents 0.3 and 1.0. Fixes cloudevents#812. Related to cloudevents#1119. Signed-off-by: Masahiro Yanagita <pbandj082@gmail.com>
Clear ce-* attributes and both content-type keys before writing the next binary or structured event, while preserving custom Pub/Sub attributes. Reset binary data so an event without data cannot retain the previous body. Address a pre-existing destination-reuse issue identified while reviewing the content-type mapping fix. Add regression tests for changed or omitted data content types and minimal events in both output encodings and CloudEvents versions 0.3 and 1.0. Signed-off-by: Masahiro Yanagita <pbandj082@gmail.com>
702a45b to
c9412dc
Compare
Binary Pub/Sub messages currently publish the event data content type only as
Content-Type, so consumers cannot filter once-datacontenttype. Publishce-datacontenttypewhile retainingContent-Typefor existing consumers. Both attributes are updated and deleted together, including when the destination message already contains either attribute.Encoding detection uses lowercase
content-type, falling back to legacyContent-Typeonly when the lowercase attribute is absent. A known CloudEvents format selects Structured mode even whence-specversionis also present. Structured publishing sets lowercasecontent-typeto the event format's media type; the JSON body'sdatacontenttypedescribes the event data. These rules follow the Google Cloud Pub/Sub binding draft, with the uppercase fallback retained for compatibility.Binary reads accept
content-typeorce-datacontenttype. When both are present, their values must match exactly; conflicting values return an error before event attributes or data are copied. LegacyContent-Typesupplies the data content type only when neither attribute is present. Binary publishing writesce-datacontenttypeand legacyContent-Type, without adding lowercasecontent-type.Compatibility notes:
Content-Typefor their data content type, and legacy Structured messages without an outerce-specversion, remain supported.content-typedetermines the encoding whenever present, even if empty or unrecognized. For example, a message withContent-Type: application/cloudevents+json,content-type: application/json, and no Pub/Subce-specversionpreviously decoded as Structured when its body was a valid CloudEvent. It now hasEncodingUnknown, andToEventreturnsunknown Message encoding, even if the JSON body containsspecversion.content-type: application/cloudevents+json, a supportedce-specversion, and no legacyContent-Type, a message previously decoded as Binary. It now decodes as Structured, using the CloudEvent in the body; a body that is not a valid structured CloudEvent can therefore fail conversion.content-typeandce-datacontenttypevalues are now rejected. Equality is exact, including empty strings. When either canonical attribute is present, conflicting legacyContent-Typeis ignored. The decoded data content type can therefore change; conflicting prefixed and legacy values previously depended on map iteration order.content-typeon the destination, preventing a stale Structured media type when reusing a message. It publishes the data content type throughce-datacontenttypeand legacyContent-Type.A CloudEvents format in the selected content-type attribute is interpreted as Structured. This change does not add Binary round-trip support for nested CloudEvents carrying a legacy CloudEvents content type.
Regression tests cover CloudEvents 0.3 and 1.0, individual and combined content-type attributes, conflicts, prepopulated destination attributes, updates and deletions, encoding changes, Structured metadata duplication, and a missing
specversionin the JSON body. The Pub/Sub test server verifies both outgoing Binary attributes.Closes #812. Related to #1119.
Validation:
cd protocol/pubsub/v2 && GOTOOLCHAIN=go1.26.0 go test -race -timeout 30s -count=1 ./...cd protocol/pubsub/v2 && GOTOOLCHAIN=go1.26.0 go vet ./...GOTOOLCHAIN=go1.26.0 ./hack/unit-test.sh(all 28 modules passed in a clean temporary copy on macOS containing the final working-tree changes)gofmtandgit diff --check