Add awsGeneratedTags field to S3Bucket in s3-event-notifications#7138
Add awsGeneratedTags field to S3Bucket in s3-event-notifications#7138ylyu-eng wants to merge 1 commit into
Conversation
alextwoods
left a comment
There was a problem hiding this comment.
Overall looking good!
This also needs a change log - you can create one with ./scripts/new-change
| this.name = name; | ||
| this.ownerIdentity = ownerIdentity; | ||
| this.arn = arn; | ||
| this.awsGeneratedTags = awsGeneratedTags; |
There was a problem hiding this comment.
We should probably do a Collections.unmodifiableMap + defensive copy on this.
There was a problem hiding this comment.
Good call-out — my concern is applying it only to awsGeneratedTags would introduce inconsistency with peer collection fields like S3#objectAnnotation which today are stored by reference without a defensive copy.
There was a problem hiding this comment.
Yeah - I think it was a miss that objectAnnotation wasn't defensively copied. Defensive copying is the standard pattern across the rest of the SDK when dealing with user provided collections.
|
|
||
| @Test | ||
| void awsGeneratedTags_whenSet_isWritten() { | ||
| Map<String, String> tags = new LinkedHashMap<>(); |
There was a problem hiding this comment.
I think in the reader tests there are tests for null and empty string tag values - we should probably those here as well.
There was a problem hiding this comment.
Thanks for capturing this, addressing.
| this(name, ownerIdentity, arn, null); | ||
| } | ||
|
|
||
| public S3Bucket(String name, UserIdentity ownerIdentity, String arn, Map<String, String> awsGeneratedTags) { |
There was a problem hiding this comment.
The 4 arg constructor here is unfortunately starting to get a little long - our SDK standard practice for cases like this is to @deprecate the 3 arg constructor and introduce a builder.
There was a problem hiding this comment.
awsGeneratedTags are populated by AWS services. External SDK callers have no way to generate system tags themselves — they only read getAwsGeneratedTags() off events the SDK has parsed. Deprecating the 3-arg constructor would surface deprecation warnings on legitimate customer code that has no reason to migrate (they can't meaningfully set the new field). That feels like a misleading signal.
There was a problem hiding this comment.
Yeah - this is a fair point - I think we can skip adding @deprecated to that constructor, but I do still think introducing the Builder makes sense here rather than a new 4-arg constructor.
2d9dbdb to
e22d9fb
Compare
Amazon S3 event notifications delivered to SNS, SQS, and Lambda now
include an `awsGeneratedTags` map on the `bucket` object when system
tags are enabled on the source bucket. The Java SDK model did not
round-trip this field, so consumers had to fall back to raw JSON
parsing to read the tags.
This change adds `Map<String, String> awsGeneratedTags` to `S3Bucket`
and threads it through the reader and writer:
* `S3Bucket`: adds `awsGeneratedTags` field with a getter, a fluent
`S3Bucket.builder()` for constructing instances with the new
field, and updates `equals`/`hashCode`/`toString`. The pre-existing
three-argument constructor is preserved (undeprecated) so existing
callers are source- and behavior-compatible. Tags are defensively
copied on construction and returned as an unmodifiable view.
* `DefaultS3EventNotificationReader`: parses `awsGeneratedTags` as
`Map<String, String>`. Absent field yields `null`. Null value
entries are preserved. Non-string values throw
`IllegalArgumentException` with a helpful message including the
offending key and value.
* `DefaultS3EventNotificationWriter`: emits `awsGeneratedTags` only
when the map is non-null and non-empty, matching S3's server-side
`@JsonInclude(NON_EMPTY)` behavior so round-tripped output is
indistinguishable from what S3 emits.
Also bumps `eventVersion` fixtures in the module test files from
`2.4` to `2.5` to reflect the schema version emitted alongside this
launch.
The change is fully additive:
* Older SDKs remain compatible with new payloads (unknown fields
are ignored during parse).
* Newer SDKs remain compatible with old payloads
(`getAwsGeneratedTags()` returns `null` when the field is absent).
e22d9fb to
ee45a15
Compare
Motivation and Context
Amazon S3 event notifications delivered to SNS, SQS, and Lambda will include a new
awsGeneratedTagsmap on thebucketobject when system tags are enabled on the source bucket. The Java SDK model did not round-trip this field, so consumers had to fall back to raw JSON parsing to read the tags. This PR adds first-class support for the field on both the reader and writer sides.Related: SIM P459766202 (internal launch coordination).
Note to reviewers: This PR targets
masterfor review only. The intent is to retarget to a feature branch once the AWS Java SDK team creates one, and merge on launch day.Modifications
S3Bucket: newMap<String, String> awsGeneratedTagsfield with getter, a fluentS3Bucket.builder()for constructing instances with the new field, and updates toequals/hashCode/toString. The pre-existing three-argument constructor is preserved so existing callers are source- and behavior-compatible. Tags are defensively copied on construction and returned as an unmodifiable view.DefaultS3EventNotificationReader: parsesawsGeneratedTagsasMap<String, String>using aLinkedHashMap(preserves S3 emission order). Absent field yieldsnull. Null value entries are preserved. Non-string values throwIllegalArgumentExceptionwith a helpful message including the offending key and value.DefaultS3EventNotificationWriter: emitsawsGeneratedTagsonly when the map is non-null and non-empty, matching S3 server-side@JsonInclude(NON_EMPTY)behavior so round-tripped output is indistinguishable from what S3 emits.eventVersionfixtures in the module test files from2.4to2.5to reflect the schema version emitted alongside this launch.Testing
Added tests in three files:
S3BucketTest(new): builder + 3-arg constructor defaults,toStringbehavior (omit-when-null, include-when-present with key/value assertions across multiple tags), defensive-copy of the tag map, and unmodifiable view via the getter.S3EventNotificationReaderTest: parse when present, absent, empty; null value preserved; empty string preserved; non-string value throws; boolean value throws; non-object throws.S3EventNotificationWriterTest: written when set (with round-trip); omitted when null; omitted when empty; null tag value preserved (round-trip); empty-string tag value preserved (round-trip).Local run of
./mvnw test -pl :s3-event-notificationspasses with 34 tests, 0 failures.Screenshots (if appropriate)
N/A
Types of changes
Checklist
mvn installsucceeds (module-level tests pass)scripts/new-changescript and following the instructions. Commit the new file created by the script in.changes/next-releasewith your changes.License