fix(pubsub): reject subscription filter changes after creation - #112
Conversation
The filter is an immutable property of a subscription in GCP: "after you create a subscription, you cannot update the subscription to modify the filter". floci-gcp accepted filter changes through subscriptions.patch, which also left publish-time evaluation observably different from GCP's delivery-time semantics — a filter changed between enqueue and delivery would have been applied by GCP but not here. Reject the modification where the filter was already being checked on update, matching what real Pub/Sub returns: INVALID_ARGUMENT with "Invalid update_mask provided in the UpdateSubscriptionRequest: the 'filter' field in the Subscription is not mutable." The rejection keys off the presence of filter in the update mask rather than a comparison against the stored value, because that is what GCP does: restating the current filter is rejected too. A patch that does not name filter leaves it untouched and succeeds. Validating the filter on update is now unreachable, since no update can change it, so that call is gone; creation still validates.
|
| Filename | Overview |
|---|---|
| src/main/java/io/floci/gcp/services/pubsub/PubSubService.java | Rejects masked filter updates and safely preserves immutable filters during maskless or unrelated updates. |
| src/test/java/io/floci/gcp/services/pubsub/PubSubServiceTest.java | Covers changed, removed, added, unchanged, unmasked, and unrelated-mask filter-update cases. |
| src/test/java/io/floci/gcp/services/pubsub/PubSubRestIntegrationTest.java | Verifies REST rejection and filter preservation across masked updates. |
| compatibility-tests/sdk-test-java/src/test/java/io/floci/gcp/test/PubSubTest.java | Adds client-level compatibility coverage for immutable filters and labels-only updates. |
| docs/services/pubsub.md | Documents filter immutability, update-mask semantics, and the snapshot-and-recreate workflow. |
Reviews (2): Last reviewed commit: "fix(pubsub): keep maskless updates that ..." | Re-trigger Greptile
…orking The immutability guard treated any filter in the request body as a change, so a maskless REST patch that echoed the subscription back — a read-modify-write carrying its own unchanged filter — was rejected, where before it succeeded. With an explicit mask the presence of "filter" is the trigger, matching GCP. Without a mask GCP rejects the request outright for the missing mask, so there is no GCP behaviour to mirror and only a value that actually differs is treated as a change.
|
Thank you, this is a model follow-up. The live API verification table made this review straightforward. The change matches the documented behavior, filters cannot be updated after creation (Pub/Sub filtering docs), and rejecting on mask presence rather than value comparison matches what you measured. Structure and error handling follow the repo conventions, and the coverage on both overloads plus the SDK suite case is exactly what AGENTS.md asks for. (follow-up, separate PR) Would you be open to filing a small tracking issue for the pre-existing maskless and empty FieldMask deviation you describe under "Not addressed here"? That observation deserves better than being buried in a merged PR body. Nothing blocking from my side. Merging. |
Closes #110
Follow-up to #103, as invited in its merge comment.
Problem
The filter is immutable in GCP, but
subscriptions.patchapplied changes to it, and a patch without an update mask cleared it. This was also the one case where publish-time evaluation could diverge from GCP's delivery-time semantics: they are equivalent only while the filter cannot change between enqueue and delivery. That point is @hectorvent's, from the #103 merge comment.Behaviour, verified against the live Pub/Sub API
Rather than infer the semantics from the documentation, I checked each case against real Pub/Sub using a throwaway topic and subscription:
filter, different value400 INVALID_ARGUMENT, "the 'filter' field in the Subscription is not mutable."filter, unchanged value400 INVALID_ARGUMENT, same messagelabels200, filter preserved400, "The update_mask … must be set, and must contain a non-empty paths list."filter,labels400,labelsunchangedlabels, body carries the same filter200, filter ignoredlabels, body carries a different filter200, filter ignoredfilter, empty value, subscription has no filter400 INVALID_ARGUMENTfilter, adding a filter to an unfiltered subscription400 INVALID_ARGUMENTThree consequences worth calling out, because each is a decision that would otherwise have to be reverse-engineered from the diff:
filterin the update mask, not a comparison against the stored value (A, B, H, I). Allowing a no-op restatement is the intuitive design, and it is wrong — GCP rejects that too, even when both the stored and the supplied filter are empty.filterin the request body outside the mask is ignored, not rejected (F, G). The update mask governs. This matters for clients that read-modify-write a whole subscription, such as the Terraform provider.The error message is GCP's verbatim. The maskless branch reuses it even though no mask was supplied, since GCP cannot reach that state; a second message for an unreachable case seemed worse than the reuse.
Validating the filter on update is now unreachable — no update can change it — so that call is gone from both overloads. Creation still validates, and after this change
PubSubService.java:219is the only place in the codebase that writes a subscription filter:Tests
mvn test— 578 tests, 0 failures (Java 25 in a container). New cases cover, on both theFieldMaskand REST overloads: rejection when changing, clearing, adding and restating a filter; a filter in the request body outside the mask being ignored; a maskless patch that omits, restates or changes the filter; and that a rejected multi-field update leaves the other fields untouched.Compatibility suites run locally against a JVM build of this branch:
sdk-test-javaPubSubTest17/17 andsdk-test-pythontest_pubsub7/7, including a new SDK case assertingInvalidArgumentExceptionfor both a changed and an unchanged filter, and that a labels-only update preserves the filter.Not run locally:
sdk-test-node,sdk-test-go,sdk-test-gcloud,compat-terraform,compat-opentofuand the native GraalVM build.Docs
docs/services/pubsub.mdgains an immutability subsection under Subscription Filters, covering what is rejected, that the update mask governs, the maskless path, and GCP's documented snapshot-and-recreate path for changing a filter.Not addressed here
An empty
FieldMaskon the gRPCUpdateSubscriptionis a silent no-op where GCP would reject for a missing mask. That is the same pre-existing deviation as the REST maskless path and predates this change.