Add @JsonIgnoreProperties(ignoreUnknown=true) to all shared-store DTOs for forward compatibility#3218
Conversation
…s for forward compatibility Without this annotation, adding a new field to any of these classes causes UnrecognizedPropertyException on older nodes during rolling deploys. This affects Account/Container metadata (MySQL), storage stats (MySQL), Helix maintenance records (ZK), and compaction policy info (local store). Classes fixed: - MigrationConfig (outer class; inner classes already had it) - RampControl - DatasetBuilder - HostAccountStorageStatsWrapper - HostPartitionClassStorageStatsWrapper - StatsHeader - HostAccountStorageStats - HostPartitionClassStorageStats - HelixParticipant.MaintenanceRecord - CompactionPolicySwitchInfo Each class includes forward compatibility tests (unknown fields ignored) and backward compatibility tests (missing optional fields use defaults). Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
crliao
left a comment
crliao
left a comment
There was a problem hiding this comment.
- https://github.com/linkedin/ambry/blob/master/ambry-store/src/main/java/com/github/ambry/store/CompactionPolicyCounter.java#L24 also using
JsonAutoDetect, vulnerable to the same incident scenario
ambry-api/src/test/java/com/github/ambry/server/StorageStatsTest.java
Outdated
Show resolved
Hide resolved
| public void testHostPartitionClassStorageStatsWrapperForwardCompatibility() throws Exception { | ||
| com.fasterxml.jackson.annotation.JsonIgnoreProperties annotation = | ||
| HostPartitionClassStorageStatsWrapper.class.getAnnotation( | ||
| com.fasterxml.jackson.annotation.JsonIgnoreProperties.class); | ||
| Assert.assertNotNull("HostPartitionClassStorageStatsWrapper must have @JsonIgnoreProperties", annotation); | ||
| Assert.assertTrue("ignoreUnknown must be true", annotation.ignoreUnknown()); |
There was a problem hiding this comment.
this test doesn't actually deserialize any JSON
There was a problem hiding this comment.
Fixed — made HostPartitionClassStorageStatsWrapper Jackson-deserializable by adding a default constructor, @JsonAutoDetect, and making fields non-final (consistent with HostAccountStorageStatsWrapper). Test now
deserializes JSON with "someNewField":"futureValue" and verifies it's ignored.
| */ | ||
| @Test | ||
| public void testHostAccountStorageStatsForwardCompatibility() throws Exception { | ||
| // Build a valid HostAccountStorageStats, serialize, inject unknown field, deserialize |
There was a problem hiding this comment.
no unknown field injected , should inject a field with wrong type to prove @JsonIgnoreProperties protects @JsonAnySetter
String json = "{\"1\":{\"1\":{\"1\":{\"logicalStorageUsage\":100,\"physicalStorageUsage\":200,\"numberOfBlobs\":5}}},\"someNewField\":\"futureValue\"}";
HostAccountStorageStats deserialized = objectMapper.readValue(json, HostAccountStorageStats.class);
There was a problem hiding this comment.
@JsonAnySetter takes precedence over @JsonIgnoreProperties in Jackson. When Jackson encounters "someNewField":"futureValue", it routes to @JsonAnySetter and tries to deserialize "futureValue" as Map<Short, Map<Short, ContainerStorageStats>>, which fails before the method body even executes.
HostAccountStorageStats by design is a flat map where all top-level keys are partition IDs. Forward compatibility for non-partition metadata fields is handled at the wrapper level —HostAccountStorageStatsWrapper has @JsonIgnoreProperties(ignoreUnknown=true) and its test already verifies unknown fields are ignored.
| * Forward compatibility: HostPartitionClassStorageStats round-trip with valid data. | ||
| */ | ||
| @Test | ||
| public void testHostPartitionClassStorageStatsForwardCompatibility() throws Exception { |
There was a problem hiding this comment.
same here , no unknown field injected
There was a problem hiding this comment.
This matters specifically because both classes use @JsonAnySetter — without @JsonIgnoreProperties, an unknown field like "someNewField": "futureValue" would reach @JsonAnySetter and fail trying to deserialize "futureValue" as a nested Map.
The annotation intercepts it before that happens, but the current tests don't exercise that path.
There was a problem hiding this comment.
HostPartitionClassStorageStats does not use @JsonAnySetter, so @JsonIgnoreProperties(ignoreUnknown=true) works here. Test now injects "someNewField":"futureValue" and verifies it's silently ignored during deserialization.
Addresses review comment: CompactionPolicyCounter uses @JsonAutoDetect and is vulnerable to the same forward compatibility issue as CompactionPolicySwitchInfo. Added annotation and compatibility tests. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
- Remove unused ArrayList import - Make HostPartitionClassStorageStatsWrapper Jackson-deserializable (add default constructor, @JsonAutoDetect, non-final fields) consistent with HostAccountStorageStatsWrapper - Replace reflection-only test with actual JSON deserialization for HostPartitionClassStorageStatsWrapper - Inject unknown fields in HostPartitionClassStorageStats test to exercise @JsonIgnoreProperties - Inject extra partition entry in HostAccountStorageStats test to verify forward compatibility via @JsonAnySetter Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
2 participants
Summary
Without this annotation, adding a new field to any of these classes causes UnrecognizedPropertyException on older nodes during rolling deploys. This affects Account/Container metadata (MySQL), storage stats (MySQL), Helix maintenance records (ZK), and compaction policy info (local store).
Classes fixed:
Testing Done