Fix field masks for presence with default value #344

google-ads/src/main/java/com/google/ads/googleads/lib/utils/FieldMasks.java

@@ -17,7 +17,6 @@
 import com.google.common.base.Preconditions;
 import com.google.protobuf.Descriptors.Descriptor;
 import com.google.protobuf.Descriptors.FieldDescriptor;
-import com.google.protobuf.Descriptors.FieldDescriptor.Type;
 import com.google.protobuf.FieldMask;
 import com.google.protobuf.GeneratedMessageV3;
 import com.google.protobuf.Message;
@@ -76,12 +75,14 @@ private static void compare(
           mask.addPaths(fieldName);
         }
       } else {
+        boolean hasValueChanged =
+            original.hasField(field) != modified.hasField(field)
+                || !Objects.equals(originalValue, modifiedValue);
         switch (field.getJavaType()) {
           case MESSAGE:
             // Because getField never returns null, we use hasField to distinguish null
             // from empty message when getType() == MESSAGE
-            if (original.hasField(field) != modified.hasField(field)
-                || !Objects.equals(originalValue, modifiedValue)) {
+            if (hasValueChanged) {
               if (isWrapperType(field.getMessageType())) {
                 // For wrapper types, just emit the field name.
                 mask.addPaths(fieldName);
@@ -103,7 +104,7 @@ private static void compare(
           case BYTE_STRING:
           case ENUM:
             // Handle all java types except MESSAGE
-            if (!Objects.equals(originalValue, modifiedValue)) {
+            if (hasValueChanged) {
               mask.addPaths(fieldName);
             }
             break;

google-ads/src/test/java/com/google/ads/googleads/lib/utils/FieldMasksParameterizedTest.java

@@ -0,0 +1,75 @@
+// Copyright 2018 Google LLC
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+//     https://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+package com.google.ads.googleads.lib.utils;
+
+import com.google.ads.googleads.test.Resource;
+import com.google.ads.googleads.test.TestCase;
+import com.google.ads.googleads.test.TestSuite;
+import com.google.common.base.Charsets;
+import com.google.common.truth.Truth;
+import com.google.protobuf.FieldMask;
+import com.google.protobuf.TextFormat;
+import java.io.IOException;
+import java.io.InputStream;
+import java.io.InputStreamReader;
+import java.util.ArrayList;
+import java.util.Collection;
+import java.util.List;
+import org.junit.Test;
+import org.junit.runner.RunWith;
+import org.junit.runners.Parameterized;
+import org.junit.runners.Parameterized.Parameters;
+
+/** Tests the {@link FieldMasks} utility. */
+@RunWith(Parameterized.class)
+public class FieldMasksParameterizedTest {
+  @Parameters(name = "{index}: {0}")
+  public static Collection<Object[]> data() throws IOException {
+    InputStream stream =
+        FieldMasksParameterizedTest.class.getResourceAsStream("/testdata/test_cases.textproto");
+    Readable readable = new InputStreamReader(stream, Charsets.UTF_8);
+
+    TestSuite.Builder builder = TestSuite.newBuilder();
+    TextFormat.merge(readable, builder);
+    TestSuite testSuite = builder.build();
+
+    List<Object[]> result = new ArrayList<>();
+    for (TestCase testCase : testSuite.getTestCasesList()) {
+      result.add(new Object[] {testCase.getDescription(), testCase});
+    }
+    return result;
+  }
+
+  private final TestCase testCase;
+
+  public FieldMasksParameterizedTest(String description, TestCase testCase) {
+    this.testCase = testCase;
+  }
+
+  @Test
+  public void testFieldMaskCompare() {
+    FieldMask actual =
+        FieldMasks.compare(testCase.getOriginalResource(), testCase.getModifiedResource());
+    Truth.assertThat(testCase.getExpectedMask()).isEqualTo(actual);
+  }
+
+  @Test
+  public void testFieldMaskAllSetFieldsOf() {
+    Resource resource = testCase.getModifiedResource();
+    FieldMask actual = FieldMasks.allSetFieldsOf(resource);
+    FieldMask expected = FieldMasks.compare(resource.getDefaultInstanceForType(), resource);
+    Truth.assertThat(expected).isEqualTo(actual);
+  }
+}

google-ads/src/test/java/com/google/ads/googleads/lib/utils/FieldMasksTest.java

@@ -1,76 +1,24 @@
-// Copyright 2018 Google LLC
-//
-// Licensed under the Apache License, Version 2.0 (the "License");
-// you may not use this file except in compliance with the License.
-// You may obtain a copy of the License at
-//
-//     https://www.apache.org/licenses/LICENSE-2.0
-//
-// Unless required by applicable law or agreed to in writing, software
-// distributed under the License is distributed on an "AS IS" BASIS,
-// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
-// See the License for the specific language governing permissions and
-// limitations under the License.
-
 package com.google.ads.googleads.lib.utils;
 
-import com.google.ads.googleads.test.Resource;
-import com.google.ads.googleads.test.TestCase;
-import com.google.ads.googleads.test.TestSuite;
-import com.google.common.base.Charsets;
-import com.google.common.truth.Truth;
-import com.google.protobuf.FieldMask;
-import com.google.protobuf.TextFormat;
-import java.io.IOException;
-import java.io.InputStream;
-import java.io.InputStreamReader;
-import java.util.ArrayList;
-import java.util.Collection;
-import java.util.List;
+import static org.junit.Assert.assertEquals;
+
+import com.google.ads.googleads.v5.resources.Campaign;
+import com.google.common.collect.ImmutableList;
 import org.junit.Test;
 import org.junit.runner.RunWith;
-import org.junit.runners.Parameterized;
-import org.junit.runners.Parameterized.Parameters;
+import org.junit.runners.JUnit4;
 
-/**
- * Tests the {@link FieldMasks} utility.
- */
-@RunWith(Parameterized.class)
+@RunWith(JUnit4.class)
 public class FieldMasksTest {
-  @Parameters(name = "{index}: {0}")
-  public static Collection<Object[]> data() throws IOException {
-    InputStream stream = FieldMasksTest.class.getResourceAsStream("/testdata/test_cases.textproto");
-    Readable readable = new InputStreamReader(stream, Charsets.UTF_8);
-
-    TestSuite.Builder builder = TestSuite.newBuilder();
-    TextFormat.merge(readable, builder);
-    TestSuite testSuite = builder.build();
-
-    List<Object[]> result = new ArrayList<>();
-    for (TestCase testCase : testSuite.getTestCasesList()) {
-      result.add(new Object[] {testCase.getDescription(), testCase});
-    }
-    return result;
-  }
-
-  private final TestCase testCase;
-
-  public FieldMasksTest(String description, TestCase testCase) {
-    this.testCase = testCase;
-  }
-
-  @Test
-  public void testFieldMaskCompare() {
-    FieldMask actual =
-        FieldMasks.compare(testCase.getOriginalResource(), testCase.getModifiedResource());
-    Truth.assertThat(testCase.getExpectedMask()).isEqualTo(actual);
-  }
 
+  // Regression test for https://github.com/googleads/google-ads-java/issues/344.
   @Test
-  public void testFieldMaskAllSetFieldsOf() {
-    Resource resource = testCase.getModifiedResource();
-    FieldMask actual = FieldMasks.allSetFieldsOf(resource);
-    FieldMask expected = FieldMasks.compare(resource.getDefaultInstanceForType(), resource);
-    Truth.assertThat(expected).isEqualTo(actual);
+  public void optionalField_withDefaultValue_detectsChange() {
+    Campaign.Builder builder = Campaign.newBuilder();
+    builder.getManualCpcBuilder().setEnhancedCpcEnabled(false);
+    Campaign campaign = builder.build();
+    assertEquals(
+        ImmutableList.of("manual_cpc.enhanced_cpc_enabled"),
+        FieldMasks.allSetFieldsOf(campaign).getPathsList());
   }
 }