Skip to content

Commit 0136a2a

Browse files
nwbirnienwbirnie
authored
Fix field masks for presence with default value #344 (#345)
1 parent 8c7bd14 commit 0136a2a

File tree

3 files changed

+94
-70
lines changed

3 files changed

+94
-70
lines changed

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

+5-4
Original file line numberDiff line numberDiff line change
@@ -17,7 +17,6 @@
1717
import com.google.common.base.Preconditions;
1818
import com.google.protobuf.Descriptors.Descriptor;
1919
import com.google.protobuf.Descriptors.FieldDescriptor;
20-
import com.google.protobuf.Descriptors.FieldDescriptor.Type;
2120
import com.google.protobuf.FieldMask;
2221
import com.google.protobuf.GeneratedMessageV3;
2322
import com.google.protobuf.Message;
@@ -76,12 +75,14 @@ private static void compare(
7675
mask.addPaths(fieldName);
7776
}
7877
} else {
78+
boolean hasValueChanged =
79+
original.hasField(field) != modified.hasField(field)
80+
|| !Objects.equals(originalValue, modifiedValue);
7981
switch (field.getJavaType()) {
8082
case MESSAGE:
8183
// Because getField never returns null, we use hasField to distinguish null
8284
// from empty message when getType() == MESSAGE
83-
if (original.hasField(field) != modified.hasField(field)
84-
|| !Objects.equals(originalValue, modifiedValue)) {
85+
if (hasValueChanged) {
8586
if (isWrapperType(field.getMessageType())) {
8687
// For wrapper types, just emit the field name.
8788
mask.addPaths(fieldName);
@@ -103,7 +104,7 @@ private static void compare(
103104
case BYTE_STRING:
104105
case ENUM:
105106
// Handle all java types except MESSAGE
106-
if (!Objects.equals(originalValue, modifiedValue)) {
107+
if (hasValueChanged) {
107108
mask.addPaths(fieldName);
108109
}
109110
break;

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

+75
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,75 @@
1+
// Copyright 2018 Google LLC
2+
//
3+
// Licensed under the Apache License, Version 2.0 (the "License");
4+
// you may not use this file except in compliance with the License.
5+
// You may obtain a copy of the License at
6+
//
7+
// https://www.apache.org/licenses/LICENSE-2.0
8+
//
9+
// Unless required by applicable law or agreed to in writing, software
10+
// distributed under the License is distributed on an "AS IS" BASIS,
11+
// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
12+
// See the License for the specific language governing permissions and
13+
// limitations under the License.
14+
15+
package com.google.ads.googleads.lib.utils;
16+
17+
import com.google.ads.googleads.test.Resource;
18+
import com.google.ads.googleads.test.TestCase;
19+
import com.google.ads.googleads.test.TestSuite;
20+
import com.google.common.base.Charsets;
21+
import com.google.common.truth.Truth;
22+
import com.google.protobuf.FieldMask;
23+
import com.google.protobuf.TextFormat;
24+
import java.io.IOException;
25+
import java.io.InputStream;
26+
import java.io.InputStreamReader;
27+
import java.util.ArrayList;
28+
import java.util.Collection;
29+
import java.util.List;
30+
import org.junit.Test;
31+
import org.junit.runner.RunWith;
32+
import org.junit.runners.Parameterized;
33+
import org.junit.runners.Parameterized.Parameters;
34+
35+
/** Tests the {@link FieldMasks} utility. */
36+
@RunWith(Parameterized.class)
37+
public class FieldMasksParameterizedTest {
38+
@Parameters(name = "{index}: {0}")
39+
public static Collection<Object[]> data() throws IOException {
40+
InputStream stream =
41+
FieldMasksParameterizedTest.class.getResourceAsStream("/testdata/test_cases.textproto");
42+
Readable readable = new InputStreamReader(stream, Charsets.UTF_8);
43+
44+
TestSuite.Builder builder = TestSuite.newBuilder();
45+
TextFormat.merge(readable, builder);
46+
TestSuite testSuite = builder.build();
47+
48+
List<Object[]> result = new ArrayList<>();
49+
for (TestCase testCase : testSuite.getTestCasesList()) {
50+
result.add(new Object[] {testCase.getDescription(), testCase});
51+
}
52+
return result;
53+
}
54+
55+
private final TestCase testCase;
56+
57+
public FieldMasksParameterizedTest(String description, TestCase testCase) {
58+
this.testCase = testCase;
59+
}
60+
61+
@Test
62+
public void testFieldMaskCompare() {
63+
FieldMask actual =
64+
FieldMasks.compare(testCase.getOriginalResource(), testCase.getModifiedResource());
65+
Truth.assertThat(testCase.getExpectedMask()).isEqualTo(actual);
66+
}
67+
68+
@Test
69+
public void testFieldMaskAllSetFieldsOf() {
70+
Resource resource = testCase.getModifiedResource();
71+
FieldMask actual = FieldMasks.allSetFieldsOf(resource);
72+
FieldMask expected = FieldMasks.compare(resource.getDefaultInstanceForType(), resource);
73+
Truth.assertThat(expected).isEqualTo(actual);
74+
}
75+
}

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

+14-66
Original file line numberDiff line numberDiff line change
@@ -1,76 +1,24 @@
1-
// Copyright 2018 Google LLC
2-
//
3-
// Licensed under the Apache License, Version 2.0 (the "License");
4-
// you may not use this file except in compliance with the License.
5-
// You may obtain a copy of the License at
6-
//
7-
// https://www.apache.org/licenses/LICENSE-2.0
8-
//
9-
// Unless required by applicable law or agreed to in writing, software
10-
// distributed under the License is distributed on an "AS IS" BASIS,
11-
// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
12-
// See the License for the specific language governing permissions and
13-
// limitations under the License.
14-
151
package com.google.ads.googleads.lib.utils;
162

17-
import com.google.ads.googleads.test.Resource;
18-
import com.google.ads.googleads.test.TestCase;
19-
import com.google.ads.googleads.test.TestSuite;
20-
import com.google.common.base.Charsets;
21-
import com.google.common.truth.Truth;
22-
import com.google.protobuf.FieldMask;
23-
import com.google.protobuf.TextFormat;
24-
import java.io.IOException;
25-
import java.io.InputStream;
26-
import java.io.InputStreamReader;
27-
import java.util.ArrayList;
28-
import java.util.Collection;
29-
import java.util.List;
3+
import static org.junit.Assert.assertEquals;
4+
5+
import com.google.ads.googleads.v5.resources.Campaign;
6+
import com.google.common.collect.ImmutableList;
307
import org.junit.Test;
318
import org.junit.runner.RunWith;
32-
import org.junit.runners.Parameterized;
33-
import org.junit.runners.Parameterized.Parameters;
9+
import org.junit.runners.JUnit4;
3410

35-
/**
36-
* Tests the {@link FieldMasks} utility.
37-
*/
38-
@RunWith(Parameterized.class)
11+
@RunWith(JUnit4.class)
3912
public class FieldMasksTest {
40-
@Parameters(name = "{index}: {0}")
41-
public static Collection<Object[]> data() throws IOException {
42-
InputStream stream = FieldMasksTest.class.getResourceAsStream("/testdata/test_cases.textproto");
43-
Readable readable = new InputStreamReader(stream, Charsets.UTF_8);
44-
45-
TestSuite.Builder builder = TestSuite.newBuilder();
46-
TextFormat.merge(readable, builder);
47-
TestSuite testSuite = builder.build();
48-
49-
List<Object[]> result = new ArrayList<>();
50-
for (TestCase testCase : testSuite.getTestCasesList()) {
51-
result.add(new Object[] {testCase.getDescription(), testCase});
52-
}
53-
return result;
54-
}
55-
56-
private final TestCase testCase;
57-
58-
public FieldMasksTest(String description, TestCase testCase) {
59-
this.testCase = testCase;
60-
}
61-
62-
@Test
63-
public void testFieldMaskCompare() {
64-
FieldMask actual =
65-
FieldMasks.compare(testCase.getOriginalResource(), testCase.getModifiedResource());
66-
Truth.assertThat(testCase.getExpectedMask()).isEqualTo(actual);
67-
}
6813

14+
// Regression test for https://github.com/googleads/google-ads-java/issues/344.
6915
@Test
70-
public void testFieldMaskAllSetFieldsOf() {
71-
Resource resource = testCase.getModifiedResource();
72-
FieldMask actual = FieldMasks.allSetFieldsOf(resource);
73-
FieldMask expected = FieldMasks.compare(resource.getDefaultInstanceForType(), resource);
74-
Truth.assertThat(expected).isEqualTo(actual);
16+
public void optionalField_withDefaultValue_detectsChange() {
17+
Campaign.Builder builder = Campaign.newBuilder();
18+
builder.getManualCpcBuilder().setEnhancedCpcEnabled(false);
19+
Campaign campaign = builder.build();
20+
assertEquals(
21+
ImmutableList.of("manual_cpc.enhanced_cpc_enabled"),
22+
FieldMasks.allSetFieldsOf(campaign).getPathsList());
7523
}
7624
}

0 commit comments

Comments
 (0)