Storage - STG104 Container Change Feed#49307
Conversation
There was a problem hiding this comment.
Pull request overview
Adds support in azure-storage-blob-changefeed for newer change feed schema variants (V6–V8), including container change feed-related event types and new optional event data fields, and strengthens deserialization testing using representative payloads.
Changes:
- Added schema V6/V7/V8 JSON payloads and a new deserialization-focused test suite covering the new optional fields.
- Extended changefeed models/deserialization to surface
createTime,lastAccessTime, andrestoredContainerVersion. - Introduced/expanded enums for additional event types and operation names to cover newer schema behaviors.
Reviewed changes
Copilot reviewed 10 out of 10 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| sdk/storage/azure-storage-blob-changefeed/src/test/resources/EventSchemaV6.json | Adds a schema V6 sample payload used by deserialization tests. |
| sdk/storage/azure-storage-blob-changefeed/src/test/resources/EventSchemaV7.json | Adds a schema V7 sample payload (includes lastAccessTime). |
| sdk/storage/azure-storage-blob-changefeed/src/test/resources/EventSchemaV8.json | Adds a schema V8 sample payload (includes restoredContainerVersion). |
| sdk/storage/azure-storage-blob-changefeed/src/test/java/com/azure/storage/blob/changefeed/MockedChangefeedResources.java | Updates mocked event data construction to include newly added optional fields. |
| sdk/storage/azure-storage-blob-changefeed/src/test/java/com/azure/storage/blob/changefeed/implementation/models/BlobChangefeedEventDeserializationTests.java | New unit tests validating enum parsing and optional field deserialization across schema versions. |
| sdk/storage/azure-storage-blob-changefeed/src/test/java/com/azure/storage/blob/changefeed/ChunkTests.java | Updates mocked Avro record creation to include new optional keys (null values) for compatibility. |
| sdk/storage/azure-storage-blob-changefeed/src/main/java/com/azure/storage/blob/changefeed/models/BlobOperationName.java | Adds a new ExpandableStringEnum for operation names used by the change feed. |
| sdk/storage/azure-storage-blob-changefeed/src/main/java/com/azure/storage/blob/changefeed/models/BlobChangefeedEventType.java | Adds new event type constants for newer schema/container change feed events. |
| sdk/storage/azure-storage-blob-changefeed/src/main/java/com/azure/storage/blob/changefeed/models/BlobChangefeedEventData.java | Adds default getters for new optional fields (creation time, last access time, restored container version). |
| sdk/storage/azure-storage-blob-changefeed/src/main/java/com/azure/storage/blob/changefeed/implementation/models/InternalBlobChangefeedEventData.java | Implements parsing/storage of the newly added optional fields and includes them in equals/hashCode/toString. |
|
looks good so far - please resolve the open comments and analyze errors |
| cfEventData.put("createTime", null); | ||
| cfEventData.put("lastAccessTime", null); | ||
| cfEventData.put("restoredContainerVersion", null); |
There was a problem hiding this comment.
are these supposed to be null?
| return new InternalBlobChangefeedEventData("PutBlob", "clientRequestId", "requestId", "etag", | ||
| "application/octet-stream", 100L, BlobType.BLOCK_BLOB, 0L, "destinationUrl", "sourceUrl", "", false, | ||
| "sequencer"); | ||
| "sequencer", null, null, null); |
There was a problem hiding this comment.
are these supposed to be null?
| @Test | ||
| public void schemaV6FullEventDeserializes() { | ||
| Map<String, Object> eventMap = buildEventRecord(r -> { | ||
| r.put("eventType", "BlobCreated"); | ||
| r.put("data", buildDataRecord(d -> { | ||
| d.put("api", "PutBlob"); | ||
| d.put("contentOffset", CONTENT_OFFSET); | ||
| d.put("createTime", CREATE_TIME); | ||
| })); | ||
| }); | ||
|
|
||
| InternalBlobChangefeedEvent event = InternalBlobChangefeedEvent.fromRecord(eventMap); | ||
|
|
||
| assertEquals(BlobChangefeedEventType.BLOB_CREATED, event.getEventType()); | ||
| assertEquals("PutBlob", event.getData().getApi()); | ||
| assertEquals(CONTENT_OFFSET, event.getData().getContentOffset()); | ||
| assertEquals(OffsetDateTime.parse(CREATE_TIME), event.getData().getCreationTime()); | ||
| assertNull(event.getData().getLastAccessTime()); | ||
| assertNull(event.getData().getRestoredContainerVersion()); | ||
| } |
There was a problem hiding this comment.
for all 3 schema full deserializing tests, instead of asserting only the new fields, I think it would be valuable to assert all of the existing fields the model exposes. mostly because these are the only tests exercising a realistic e2e payload and asserting all of the fields would help us catch cross-field regressions.
| cfEventData.put("createTime", null); | ||
| cfEventData.put("lastAccessTime", null); | ||
| cfEventData.put("restoredContainerVersion", null); |
No description provided.