Skip to content

Commit ea3067e

Browse files
nidhiii-27nidhiii-27
authored
fix: update object context diff logic to be shallow rather than deep (#3287)
Previously, deleting an object context key by setting its value to null would fail. This change corrects the diffMaps utility to ensure that null payloads are always treated as deletions. The produced update mask is now `contexts.custom.<key>` instead of `contexts.custom.<key>.value`
1 parent 8e143be commit ea3067e

6 files changed

Lines changed: 86 additions & 86 deletions

File tree

‎java-storage/google-cloud-storage/src/main/java/com/google/cloud/storage/BlobInfo.java‎

Lines changed: 2 additions & 20 deletions
Original file line numberDiff line numberDiff line change
@@ -33,7 +33,6 @@
3333
import com.google.common.collect.ImmutableList;
3434
import com.google.common.collect.ImmutableMap;
3535
import com.google.common.collect.ImmutableSet;
36-
import com.google.common.collect.Maps;
3736
import com.google.common.io.BaseEncoding;
3837
import java.io.Serializable;
3938
import java.nio.ByteBuffer;
@@ -1273,18 +1272,15 @@ public Builder setContexts(ObjectContexts contexts) {
12731272
// about the timestamps when determining if a value needs to be patched. Create a new map
12741273
// where we remove the timestamps so equals is usable.
12751274
Map<String, ObjectCustomContextPayload> left =
1276-
this.contexts == null
1277-
? null
1278-
: ignoreCustomContextPayloadTimestamps(this.contexts.getCustom());
1275+
this.contexts == null ? null : this.contexts.getCustom();
12791276
Map<String, ObjectCustomContextPayload> right =
1280-
contexts == null ? null : ignoreCustomContextPayloadTimestamps(contexts.getCustom());
1277+
contexts == null ? null : contexts.getCustom();
12811278
if (!Objects.equals(left, right)) {
12821279
if (right != null) {
12831280
diffMaps(
12841281
NamedField.nested(BlobField.OBJECT_CONTEXTS, NamedField.literal("custom")),
12851282
left,
12861283
right,
1287-
f -> NamedField.nested(f, NAMED_FIELD_LITERAL_VALUE),
12881284
modifiedFields::add);
12891285
this.contexts = contexts;
12901286
} else {
@@ -1295,20 +1291,6 @@ public Builder setContexts(ObjectContexts contexts) {
12951291
return this;
12961292
}
12971293

1298-
private static @Nullable Map<@NonNull String, @Nullable ObjectCustomContextPayload>
1299-
ignoreCustomContextPayloadTimestamps(
1300-
@Nullable Map<@NonNull String, @Nullable ObjectCustomContextPayload> orig) {
1301-
if (orig == null) {
1302-
return null;
1303-
}
1304-
return Maps.transformValues(
1305-
orig,
1306-
v ->
1307-
v == null
1308-
? null
1309-
: ObjectCustomContextPayload.newBuilder().setValue(v.getValue()).build());
1310-
}
1311-
13121294
@Override
13131295
public BlobInfo build() {
13141296
checkNotNull(blobId);

‎java-storage/google-cloud-storage/src/main/java/com/google/cloud/storage/JsonUtils.java‎

Lines changed: 17 additions & 20 deletions
Original file line numberDiff line numberDiff line change
@@ -22,9 +22,6 @@
2222
import com.google.cloud.storage.UnifiedOpts.NamedField;
2323
import com.google.common.annotations.VisibleForTesting;
2424
import com.google.common.collect.ImmutableSet;
25-
import com.google.common.collect.MapDifference;
26-
import com.google.common.collect.MapDifference.ValueDifference;
27-
import com.google.common.collect.Maps;
2825
import com.google.gson.Gson;
2926
import com.google.gson.GsonBuilder;
3027
import com.google.gson.JsonArray;
@@ -41,7 +38,6 @@
4138
import java.util.Set;
4239
import java.util.regex.Matcher;
4340
import java.util.regex.Pattern;
44-
import java.util.stream.Stream;
4541
import org.checkerframework.checker.nullness.qual.NonNull;
4642

4743
final class JsonUtils {
@@ -70,7 +66,7 @@ static <T extends GenericJson> T getOutputJsonWithSelectedFields(
7066
.map(NamedField::getApiaryName)
7167
.collect(ImmutableSet.toImmutableSet());
7268
try {
73-
// The datamodel of the apiairy json representation doesn't have a common parent for all
69+
// The datamodel of the apiary json representation doesn't have a common parent for all
7470
// field types, rather than writing a significant amount of code to handle all of these types
7571
// leverage Gson.
7672
// 1. serialize the object to it's json string
@@ -104,24 +100,25 @@ static <T extends GenericJson> T getOutputJsonWithSelectedFields(
104100
static @NonNull JsonObject getOutputJson(JsonObject inputJson, Set<String> fieldsInOutput) {
105101

106102
Map<String, String> l = flatten(inputJson);
107-
Map<String, String> r = Utils.setToMap(fieldsInOutput, k -> null);
108-
109-
MapDifference<String, String> diff = Maps.difference(l, r);
110103

111104
// use hashmap so we can have null values
112105
HashMap<String, String> flat = new HashMap<>();
113-
Stream.of(
114-
diff.entriesInCommon().entrySet().stream(),
115-
diff.entriesOnlyOnRight().entrySet().stream(),
116-
// if the key is present in both maps, but has a differing value select the value from
117-
// the left side, as that is the value from inputJson
118-
Maps.transformValues(diff.entriesDiffering(), ValueDifference::leftValue)
119-
.entrySet()
120-
.stream())
121-
// flatten
122-
.flatMap(x -> x)
123-
.forEach(e -> flat.put(e.getKey(), e.getValue()));
124-
106+
for (String fieldToRetain : fieldsInOutput) {
107+
boolean keyFound = false;
108+
// Check for exact match or prefix match in the flattened source map (l)
109+
for (Map.Entry<String, String> sourceEntry : l.entrySet()) {
110+
String sourceKey = sourceEntry.getKey();
111+
if (sourceKey.equals(fieldToRetain) || sourceKey.startsWith(fieldToRetain + ".")) {
112+
flat.put(sourceKey, sourceEntry.getValue());
113+
keyFound = true;
114+
}
115+
}
116+
// If the field to retain wasn't found in the source, it means we need to add it
117+
// to the output with a null value, signaling a deletion.
118+
if (!keyFound) {
119+
flat.put(fieldToRetain, null);
120+
}
121+
}
125122
return treeify(flat);
126123
}
127124

‎java-storage/google-cloud-storage/src/main/java/com/google/cloud/storage/Utils.java‎

Lines changed: 8 additions & 38 deletions
Original file line numberDiff line numberDiff line change
@@ -29,7 +29,6 @@
2929
import com.google.common.collect.ImmutableList;
3030
import com.google.common.collect.ImmutableMap;
3131
import com.google.common.collect.MapDifference;
32-
import com.google.common.collect.MapDifference.ValueDifference;
3332
import com.google.common.collect.Maps;
3433
import com.google.common.io.BaseEncoding;
3534
import com.google.common.primitives.Ints;
@@ -282,57 +281,28 @@ static <T> T firstNonNull(Supplier<@Nullable T>... ss) {
282281
*/
283282
static void diffMaps(
284283
NamedField parent, Map<String, ?> left, Map<String, ?> right, Consumer<NamedField> sink) {
285-
diffMaps(parent, left, right, Function.identity(), sink);
286-
}
287-
288-
/**
289-
* Diff two maps, and append each differing key to {@code sink} with the parent of {{@code
290-
* parent}. Conditionally apply {@code dec} if deeper qualification is necessary.
291-
*/
292-
static void diffMaps(
293-
NamedField parent,
294-
Map<String, ?> left,
295-
Map<String, ?> right,
296-
Function<NamedField, NamedField> dec,
297-
Consumer<NamedField> sink) {
298-
final Stream<NamedField> keys;
284+
final Stream<String> keys;
299285
if (left != null && right == null) {
300-
keys = left.keySet().stream().map(NamedField::literal);
286+
keys = left.keySet().stream();
301287
} else if (left == null && right != null) {
302-
keys = right.keySet().stream().map(NamedField::literal).map(dec);
288+
keys = right.keySet().stream();
303289
} else if (left != null && right != null) {
304290
MapDifference<String, ?> difference = Maps.difference(left, right);
305291
keys =
306292
Stream.of(
307293
// keys with modified values
308-
difference.entriesDiffering().entrySet().stream()
309-
.map(
310-
e -> {
311-
String key = e.getKey();
312-
NamedField literal = NamedField.literal(key);
313-
ValueDifference<?> diff = e.getValue();
314-
315-
if (diff.leftValue() != null && diff.rightValue() == null) {
316-
return literal;
317-
} else if (diff.leftValue() == null && diff.rightValue() != null) {
318-
return literal;
319-
} else {
320-
return dec.apply(literal);
321-
}
322-
}),
294+
difference.entriesDiffering().keySet().stream(),
323295
// Only include keys to remove if ALL keys were removed
324296
right.isEmpty()
325-
? difference.entriesOnlyOnLeft().keySet().stream().map(NamedField::literal)
326-
: Stream.<NamedField>empty(),
297+
? difference.entriesOnlyOnLeft().keySet().stream()
298+
: Stream.<String>empty(),
327299
// new keys
328-
difference.entriesOnlyOnRight().keySet().stream()
329-
.map(NamedField::literal)
330-
.map(dec))
300+
difference.entriesOnlyOnRight().keySet().stream())
331301
.flatMap(x -> x);
332302
} else {
333303
keys = Stream.empty();
334304
}
335-
keys.map(k -> NamedField.nested(parent, k)).forEach(sink);
305+
keys.map(NamedField::literal).map(k -> NamedField.nested(parent, k)).forEach(sink);
336306
}
337307

338308
static <T> T[] subArray(T[] ts, int offset, int length) {

‎java-storage/google-cloud-storage/src/test/java/com/google/cloud/storage/BlobInfoTest.java‎

Lines changed: 51 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -19,6 +19,8 @@
1919
import static com.google.cloud.storage.Acl.Project.ProjectRole.VIEWERS;
2020
import static com.google.cloud.storage.Acl.Role.READER;
2121
import static com.google.cloud.storage.Acl.Role.WRITER;
22+
import static com.google.cloud.storage.TestUtils.hashMapOf;
23+
import static com.google.common.truth.Truth.assertThat;
2224
import static org.junit.Assert.assertEquals;
2325
import static org.junit.Assert.assertFalse;
2426
import static org.junit.Assert.assertNull;
@@ -30,12 +32,16 @@
3032
import com.google.cloud.storage.BlobInfo.CustomerEncryption;
3133
import com.google.cloud.storage.BlobInfo.ObjectContexts;
3234
import com.google.cloud.storage.BlobInfo.ObjectCustomContextPayload;
35+
import com.google.cloud.storage.UnifiedOpts.NamedField;
3336
import com.google.common.collect.ImmutableList;
3437
import com.google.common.collect.ImmutableMap;
38+
import com.google.common.collect.ImmutableSet;
3539
import java.math.BigInteger;
3640
import java.util.Collections;
3741
import java.util.List;
3842
import java.util.Map;
43+
import java.util.Set;
44+
import java.util.stream.Collectors;
3945
import org.junit.Test;
4046

4147
public class BlobInfoTest {
@@ -352,4 +358,49 @@ public void testToPbAndFromPb() {
352358
public void testBlobId() {
353359
assertEquals(BlobId.of("b", "n", GENERATION), BLOB_INFO.getBlobId());
354360
}
361+
362+
@Test
363+
public void deepFieldDiffDetectionWorksCorrectly_mutateRetrievedObject() {
364+
BlobInfo info =
365+
BlobInfo.newBuilder("bucket", "object")
366+
.setContexts(
367+
ObjectContexts.newBuilder()
368+
.setCustom(
369+
hashMapOf(
370+
"c1", ObjectCustomContextPayload.newBuilder().setValue("C1").build(),
371+
"c2", ObjectCustomContextPayload.newBuilder().setValue("C2").build()))
372+
.build())
373+
.setMetadata(
374+
hashMapOf(
375+
"m1", "M1",
376+
"m2", "M2"))
377+
.build();
378+
379+
BlobInfo modified =
380+
info.toBuilder()
381+
.setMetadata(hashMapOf("m2", null))
382+
.setContexts(ObjectContexts.newBuilder().setCustom(hashMapOf("k2", null)).build())
383+
.build();
384+
Set<String> modifiedFields =
385+
modified.getModifiedFields().stream()
386+
.map(NamedField::getGrpcName)
387+
.collect(Collectors.toSet());
388+
389+
assertThat(modifiedFields).isEqualTo(ImmutableSet.of("contexts.custom.k2", "metadata.m2"));
390+
}
391+
392+
@Test
393+
public void deepFieldDiffDetectionWorksCorrectly_declaredDiff() {
394+
BlobInfo modified =
395+
BlobInfo.newBuilder("bucket", "object")
396+
.setMetadata(hashMapOf("m2", null))
397+
.setContexts(ObjectContexts.newBuilder().setCustom(hashMapOf("k2", null)).build())
398+
.build();
399+
Set<String> modifiedFields =
400+
modified.getModifiedFields().stream()
401+
.map(UnifiedOpts.NamedField::getGrpcName)
402+
.collect(Collectors.toSet());
403+
404+
assertThat(modifiedFields).isEqualTo(ImmutableSet.of("contexts.custom.k2", "metadata.m2"));
405+
}
355406
}

‎java-storage/google-cloud-storage/src/test/java/com/google/cloud/storage/TestUtils.java‎

Lines changed: 5 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -279,20 +279,19 @@ public static void assertAll(ThrowingRunnable... trs) throws Exception {
279279
}
280280

281281
/** ImmutableMap does not allow null values, this method does */
282-
public static Map<@NonNull String, @Nullable String> hashMapOf(
283-
@NonNull String k1, @Nullable String v1) {
282+
public static <K, V> Map<@NonNull K, @Nullable V> hashMapOf(@NonNull K k1, @Nullable V v1) {
284283
requireNonNull(k1, "k1 must be non null");
285-
HashMap<String, String> map = new HashMap<>();
284+
HashMap<K, V> map = new HashMap<>();
286285
map.put(k1, v1);
287286
return Collections.unmodifiableMap(map);
288287
}
289288

290289
/** ImmutableMap does not allow null values, this method does */
291-
public static Map<@NonNull String, @Nullable String> hashMapOf(
292-
@NonNull String k1, @Nullable String v1, @NonNull String k2, @Nullable String v2) {
290+
public static <K, V> Map<@NonNull K, @Nullable V> hashMapOf(
291+
@NonNull K k1, @Nullable V v1, @NonNull K k2, @Nullable V v2) {
293292
requireNonNull(k1, "k1 must be non null");
294293
requireNonNull(k2, "k2 must be non null");
295-
HashMap<String, String> map = new HashMap<>();
294+
HashMap<K, V> map = new HashMap<>();
296295
map.put(k1, v1);
297296
map.put(k2, v2);
298297
return Collections.unmodifiableMap(map);

‎java-storage/google-cloud-storage/src/test/java/com/google/cloud/storage/it/ITNestedUpdateMaskTest.java‎

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -78,6 +78,7 @@ public static final class NestedUpdateMaskParametersProvider implements Paramete
7878
private static final Map<String, String> k1a_k2null = hashMapOf("k1", "a", "k2", null);
7979
private static final Map<String, String> k1null = hashMapOf("k1", null);
8080
private static final Map<String, String> k2null = hashMapOf("k2", null);
81+
private static final Map<String, String> k1null_k2null = hashMapOf("k1", null, "k2", null);
8182

8283
/**
8384
*
@@ -95,7 +96,7 @@ public static final class NestedUpdateMaskParametersProvider implements Paramete
9596
* | {"k1":"a","k2":"b"} | {"k2":null} | {"k1":"a"} |
9697
* | {"k1":"a"} | {} | null |
9798
* | {"k1":"a"} | {"k1":null} | null |
98-
* | {"k1":"a","k2":"b"} | null | null |
99+
* | {"k1":"a","k2":"b"} | {"k1:null,"k2":null} | null |
99100
* </pre>
100101
*/
101102
@Override
@@ -111,7 +112,7 @@ public ImmutableList<Param> parameters() {
111112
new Param("2 keys, modify 1 null (fine)", k1a_k2b, k2null, k1a),
112113
new Param("1 key, set empty", k1a, empty, null),
113114
new Param("1 key, null key", k1a, k1null, null),
114-
new Param("2 keys, set null", k1a_k2b, null, null));
115+
new Param("2 keys, set null", k1a_k2b, k1null_k2null, null));
115116
}
116117
}
117118

0 commit comments

Comments
 (0)