diff --git a/google-cloud-storage/src/main/java/com/google/cloud/storage/BlobInfo.java b/google-cloud-storage/src/main/java/com/google/cloud/storage/BlobInfo.java index aa78fa7276..a083338e74 100644 --- a/google-cloud-storage/src/main/java/com/google/cloud/storage/BlobInfo.java +++ b/google-cloud-storage/src/main/java/com/google/cloud/storage/BlobInfo.java @@ -33,7 +33,6 @@ import com.google.common.collect.ImmutableList; import com.google.common.collect.ImmutableMap; import com.google.common.collect.ImmutableSet; -import com.google.common.collect.Maps; import com.google.common.io.BaseEncoding; import java.io.Serializable; import java.nio.ByteBuffer; @@ -1273,18 +1272,15 @@ public Builder setContexts(ObjectContexts contexts) { // about the timestamps when determining if a value needs to be patched. Create a new map // where we remove the timestamps so equals is usable. Map left = - this.contexts == null - ? null - : ignoreCustomContextPayloadTimestamps(this.contexts.getCustom()); + this.contexts == null ? null : this.contexts.getCustom(); Map right = - contexts == null ? null : ignoreCustomContextPayloadTimestamps(contexts.getCustom()); + contexts == null ? null : contexts.getCustom(); if (!Objects.equals(left, right)) { if (right != null) { diffMaps( NamedField.nested(BlobField.OBJECT_CONTEXTS, NamedField.literal("custom")), left, right, - f -> NamedField.nested(f, NAMED_FIELD_LITERAL_VALUE), modifiedFields::add); this.contexts = contexts; } else { @@ -1295,20 +1291,6 @@ public Builder setContexts(ObjectContexts contexts) { return this; } - private static @Nullable Map<@NonNull String, @Nullable ObjectCustomContextPayload> - ignoreCustomContextPayloadTimestamps( - @Nullable Map<@NonNull String, @Nullable ObjectCustomContextPayload> orig) { - if (orig == null) { - return null; - } - return Maps.transformValues( - orig, - v -> - v == null - ? null - : ObjectCustomContextPayload.newBuilder().setValue(v.getValue()).build()); - } - @Override public BlobInfo build() { checkNotNull(blobId); diff --git a/google-cloud-storage/src/main/java/com/google/cloud/storage/JsonUtils.java b/google-cloud-storage/src/main/java/com/google/cloud/storage/JsonUtils.java index 50427a8cf4..f5297ee837 100644 --- a/google-cloud-storage/src/main/java/com/google/cloud/storage/JsonUtils.java +++ b/google-cloud-storage/src/main/java/com/google/cloud/storage/JsonUtils.java @@ -22,9 +22,6 @@ import com.google.cloud.storage.UnifiedOpts.NamedField; import com.google.common.annotations.VisibleForTesting; import com.google.common.collect.ImmutableSet; -import com.google.common.collect.MapDifference; -import com.google.common.collect.MapDifference.ValueDifference; -import com.google.common.collect.Maps; import com.google.gson.Gson; import com.google.gson.GsonBuilder; import com.google.gson.JsonArray; @@ -41,7 +38,6 @@ import java.util.Set; import java.util.regex.Matcher; import java.util.regex.Pattern; -import java.util.stream.Stream; import org.checkerframework.checker.nullness.qual.NonNull; final class JsonUtils { @@ -70,7 +66,7 @@ static T getOutputJsonWithSelectedFields( .map(NamedField::getApiaryName) .collect(ImmutableSet.toImmutableSet()); try { - // The datamodel of the apiairy json representation doesn't have a common parent for all + // The datamodel of the apiary json representation doesn't have a common parent for all // field types, rather than writing a significant amount of code to handle all of these types // leverage Gson. // 1. serialize the object to it's json string @@ -104,24 +100,25 @@ static T getOutputJsonWithSelectedFields( static @NonNull JsonObject getOutputJson(JsonObject inputJson, Set fieldsInOutput) { Map l = flatten(inputJson); - Map r = Utils.setToMap(fieldsInOutput, k -> null); - - MapDifference diff = Maps.difference(l, r); // use hashmap so we can have null values HashMap flat = new HashMap<>(); - Stream.of( - diff.entriesInCommon().entrySet().stream(), - diff.entriesOnlyOnRight().entrySet().stream(), - // if the key is present in both maps, but has a differing value select the value from - // the left side, as that is the value from inputJson - Maps.transformValues(diff.entriesDiffering(), ValueDifference::leftValue) - .entrySet() - .stream()) - // flatten - .flatMap(x -> x) - .forEach(e -> flat.put(e.getKey(), e.getValue())); - + for (String fieldToRetain : fieldsInOutput) { + boolean keyFound = false; + // Check for exact match or prefix match in the flattened source map (l) + for (Map.Entry sourceEntry : l.entrySet()) { + String sourceKey = sourceEntry.getKey(); + if (sourceKey.equals(fieldToRetain) || sourceKey.startsWith(fieldToRetain + ".")) { + flat.put(sourceKey, sourceEntry.getValue()); + keyFound = true; + } + } + // If the field to retain wasn't found in the source, it means we need to add it + // to the output with a null value, signaling a deletion. + if (!keyFound) { + flat.put(fieldToRetain, null); + } + } return treeify(flat); } diff --git a/google-cloud-storage/src/main/java/com/google/cloud/storage/Utils.java b/google-cloud-storage/src/main/java/com/google/cloud/storage/Utils.java index d6f96b8ad4..eece2fe79d 100644 --- a/google-cloud-storage/src/main/java/com/google/cloud/storage/Utils.java +++ b/google-cloud-storage/src/main/java/com/google/cloud/storage/Utils.java @@ -29,7 +29,6 @@ import com.google.common.collect.ImmutableList; import com.google.common.collect.ImmutableMap; import com.google.common.collect.MapDifference; -import com.google.common.collect.MapDifference.ValueDifference; import com.google.common.collect.Maps; import com.google.common.io.BaseEncoding; import com.google.common.primitives.Ints; @@ -282,57 +281,28 @@ static T firstNonNull(Supplier<@Nullable T>... ss) { */ static void diffMaps( NamedField parent, Map left, Map right, Consumer sink) { - diffMaps(parent, left, right, Function.identity(), sink); - } - - /** - * Diff two maps, and append each differing key to {@code sink} with the parent of {{@code - * parent}. Conditionally apply {@code dec} if deeper qualification is necessary. - */ - static void diffMaps( - NamedField parent, - Map left, - Map right, - Function dec, - Consumer sink) { - final Stream keys; + final Stream keys; if (left != null && right == null) { - keys = left.keySet().stream().map(NamedField::literal); + keys = left.keySet().stream(); } else if (left == null && right != null) { - keys = right.keySet().stream().map(NamedField::literal).map(dec); + keys = right.keySet().stream(); } else if (left != null && right != null) { MapDifference difference = Maps.difference(left, right); keys = Stream.of( // keys with modified values - difference.entriesDiffering().entrySet().stream() - .map( - e -> { - String key = e.getKey(); - NamedField literal = NamedField.literal(key); - ValueDifference diff = e.getValue(); - - if (diff.leftValue() != null && diff.rightValue() == null) { - return literal; - } else if (diff.leftValue() == null && diff.rightValue() != null) { - return literal; - } else { - return dec.apply(literal); - } - }), + difference.entriesDiffering().keySet().stream(), // Only include keys to remove if ALL keys were removed right.isEmpty() - ? difference.entriesOnlyOnLeft().keySet().stream().map(NamedField::literal) - : Stream.empty(), + ? difference.entriesOnlyOnLeft().keySet().stream() + : Stream.empty(), // new keys - difference.entriesOnlyOnRight().keySet().stream() - .map(NamedField::literal) - .map(dec)) + difference.entriesOnlyOnRight().keySet().stream()) .flatMap(x -> x); } else { keys = Stream.empty(); } - keys.map(k -> NamedField.nested(parent, k)).forEach(sink); + keys.map(NamedField::literal).map(k -> NamedField.nested(parent, k)).forEach(sink); } static T[] subArray(T[] ts, int offset, int length) { diff --git a/google-cloud-storage/src/test/java/com/google/cloud/storage/BlobInfoTest.java b/google-cloud-storage/src/test/java/com/google/cloud/storage/BlobInfoTest.java index 862709a5ae..6ecb483918 100644 --- a/google-cloud-storage/src/test/java/com/google/cloud/storage/BlobInfoTest.java +++ b/google-cloud-storage/src/test/java/com/google/cloud/storage/BlobInfoTest.java @@ -19,6 +19,8 @@ import static com.google.cloud.storage.Acl.Project.ProjectRole.VIEWERS; import static com.google.cloud.storage.Acl.Role.READER; import static com.google.cloud.storage.Acl.Role.WRITER; +import static com.google.cloud.storage.TestUtils.hashMapOf; +import static com.google.common.truth.Truth.assertThat; import static org.junit.Assert.assertEquals; import static org.junit.Assert.assertFalse; import static org.junit.Assert.assertNull; @@ -30,12 +32,16 @@ import com.google.cloud.storage.BlobInfo.CustomerEncryption; import com.google.cloud.storage.BlobInfo.ObjectContexts; import com.google.cloud.storage.BlobInfo.ObjectCustomContextPayload; +import com.google.cloud.storage.UnifiedOpts.NamedField; import com.google.common.collect.ImmutableList; import com.google.common.collect.ImmutableMap; +import com.google.common.collect.ImmutableSet; import java.math.BigInteger; import java.util.Collections; import java.util.List; import java.util.Map; +import java.util.Set; +import java.util.stream.Collectors; import org.junit.Test; public class BlobInfoTest { @@ -352,4 +358,49 @@ public void testToPbAndFromPb() { public void testBlobId() { assertEquals(BlobId.of("b", "n", GENERATION), BLOB_INFO.getBlobId()); } + + @Test + public void deepFieldDiffDetectionWorksCorrectly_mutateRetrievedObject() { + BlobInfo info = + BlobInfo.newBuilder("bucket", "object") + .setContexts( + ObjectContexts.newBuilder() + .setCustom( + hashMapOf( + "c1", ObjectCustomContextPayload.newBuilder().setValue("C1").build(), + "c2", ObjectCustomContextPayload.newBuilder().setValue("C2").build())) + .build()) + .setMetadata( + hashMapOf( + "m1", "M1", + "m2", "M2")) + .build(); + + BlobInfo modified = + info.toBuilder() + .setMetadata(hashMapOf("m2", null)) + .setContexts(ObjectContexts.newBuilder().setCustom(hashMapOf("k2", null)).build()) + .build(); + Set modifiedFields = + modified.getModifiedFields().stream() + .map(NamedField::getGrpcName) + .collect(Collectors.toSet()); + + assertThat(modifiedFields).isEqualTo(ImmutableSet.of("contexts.custom.k2", "metadata.m2")); + } + + @Test + public void deepFieldDiffDetectionWorksCorrectly_declaredDiff() { + BlobInfo modified = + BlobInfo.newBuilder("bucket", "object") + .setMetadata(hashMapOf("m2", null)) + .setContexts(ObjectContexts.newBuilder().setCustom(hashMapOf("k2", null)).build()) + .build(); + Set modifiedFields = + modified.getModifiedFields().stream() + .map(UnifiedOpts.NamedField::getGrpcName) + .collect(Collectors.toSet()); + + assertThat(modifiedFields).isEqualTo(ImmutableSet.of("contexts.custom.k2", "metadata.m2")); + } } diff --git a/google-cloud-storage/src/test/java/com/google/cloud/storage/TestUtils.java b/google-cloud-storage/src/test/java/com/google/cloud/storage/TestUtils.java index f493570acc..5231b8fe00 100644 --- a/google-cloud-storage/src/test/java/com/google/cloud/storage/TestUtils.java +++ b/google-cloud-storage/src/test/java/com/google/cloud/storage/TestUtils.java @@ -279,20 +279,19 @@ public static void assertAll(ThrowingRunnable... trs) throws Exception { } /** ImmutableMap does not allow null values, this method does */ - public static Map<@NonNull String, @Nullable String> hashMapOf( - @NonNull String k1, @Nullable String v1) { + public static Map<@NonNull K, @Nullable V> hashMapOf(@NonNull K k1, @Nullable V v1) { requireNonNull(k1, "k1 must be non null"); - HashMap map = new HashMap<>(); + HashMap map = new HashMap<>(); map.put(k1, v1); return Collections.unmodifiableMap(map); } /** ImmutableMap does not allow null values, this method does */ - public static Map<@NonNull String, @Nullable String> hashMapOf( - @NonNull String k1, @Nullable String v1, @NonNull String k2, @Nullable String v2) { + public static Map<@NonNull K, @Nullable V> hashMapOf( + @NonNull K k1, @Nullable V v1, @NonNull K k2, @Nullable V v2) { requireNonNull(k1, "k1 must be non null"); requireNonNull(k2, "k2 must be non null"); - HashMap map = new HashMap<>(); + HashMap map = new HashMap<>(); map.put(k1, v1); map.put(k2, v2); return Collections.unmodifiableMap(map); diff --git a/google-cloud-storage/src/test/java/com/google/cloud/storage/it/ITNestedUpdateMaskTest.java b/google-cloud-storage/src/test/java/com/google/cloud/storage/it/ITNestedUpdateMaskTest.java index e8e3a25728..e84ccf5a5e 100644 --- a/google-cloud-storage/src/test/java/com/google/cloud/storage/it/ITNestedUpdateMaskTest.java +++ b/google-cloud-storage/src/test/java/com/google/cloud/storage/it/ITNestedUpdateMaskTest.java @@ -78,6 +78,7 @@ public static final class NestedUpdateMaskParametersProvider implements Paramete private static final Map k1a_k2null = hashMapOf("k1", "a", "k2", null); private static final Map k1null = hashMapOf("k1", null); private static final Map k2null = hashMapOf("k2", null); + private static final Map k1null_k2null = hashMapOf("k1", null, "k2", null); /** * @@ -95,7 +96,7 @@ public static final class NestedUpdateMaskParametersProvider implements Paramete * | {"k1":"a","k2":"b"} | {"k2":null} | {"k1":"a"} | * | {"k1":"a"} | {} | null | * | {"k1":"a"} | {"k1":null} | null | - * | {"k1":"a","k2":"b"} | null | null | + * | {"k1":"a","k2":"b"} | {"k1:null,"k2":null} | null | * */ @Override @@ -111,7 +112,7 @@ public ImmutableList parameters() { new Param("2 keys, modify 1 null (fine)", k1a_k2b, k2null, k1a), new Param("1 key, set empty", k1a, empty, null), new Param("1 key, null key", k1a, k1null, null), - new Param("2 keys, set null", k1a_k2b, null, null)); + new Param("2 keys, set null", k1a_k2b, k1null_k2null, null)); } }