Fix handling of ignored FieldDescriptors on sub-message fields.

This causes Ben's example test to fail as expected: [] ------------- Created by MOE: https://github.com/google/moe MOE_MIGRATED_REVID=135172594
google · Oct 5, 2016 · 485a3f7 · 485a3f7
1 parent 198e0d8
commit 485a3f7
Show file tree

Hide file tree

Showing 2 changed files with 75 additions and 16 deletions.
diff --git a/extensions/proto/src/main/java/com/google/common/truth/extensions/proto/FieldScopeImpl.java b/extensions/proto/src/main/java/com/google/common/truth/extensions/proto/FieldScopeImpl.java
@@ -70,8 +70,8 @@ public boolean isIgnored(
           addSubMessages(fieldDescriptor, message2, subMessages);
         }
 
-        return !includesField(
+        Context context = Context.create(descriptor, fieldPath, fieldDescriptor, subMessages);
-            Context.create(descriptor, fieldPath, fieldDescriptor, subMessages), cache);
+        return !matchesContentOfSubMessages(context, cache) && !matchesFieldPath(context, cache);
       }
     };
   }
@@ -145,7 +145,17 @@ static Context create(
   }
 
   /** Whether or not this implementation includes the specified specific field path. */
-  abstract boolean includesField(Context context, Cache cache);
+  abstract boolean matchesFieldPath(Context context, Cache cache);
+
+  /**
+   * Whether or not this implementation matches some sub-field of one of the messages at the end of
+   * the field path. Necessary for negation to work with arbitrary FieldDescriptors.
+   *
+   * <p>Returns false by default, since most FieldScopeImpls don't inspect sub-message contents.
+   */
+  boolean matchesContentOfSubMessages(Context context, Cache cache) {
+    return false;
+  }
 
   /**
    * Performs any validation that requires a Descriptor to validate against.
@@ -205,14 +215,14 @@ public FieldScope allowingFieldDescriptors(FieldDescriptor... fieldDescriptors)
   private static final FieldScope ALL =
       new FieldScopeImpl() {
         @Override
-        boolean includesField(Context context, Cache cache) {
+        boolean matchesFieldPath(Context context, Cache cache) {
           return true;
         }
       };
   private static final FieldScope NONE =
       new FieldScopeImpl() {
         @Override
-        boolean includesField(Context context, Cache cache) {
+        boolean matchesFieldPath(Context context, Cache cache) {
           return false;
         }
       };
@@ -245,7 +255,7 @@ void validate(Descriptor descriptor) {
     }
 
     @Override
-    boolean includesField(Context context, Cache cache) {
+    boolean matchesFieldPath(Context context, Cache cache) {
       return fieldNumberTree.matches(context.fieldPath(), context.field());
     }
   }
@@ -272,7 +282,7 @@ private abstract static class FieldMatcherBaseScopeImpl extends FieldScopeImpl {
     abstract boolean matchesFieldDescriptor(Descriptor descriptor, FieldDescriptor fieldDescriptor);
 
     @Override
-    boolean includesField(Context context, Cache cache) {
+    boolean matchesFieldPath(Context context, Cache cache) {
       if (context.field().isPresent()
           && matchesFieldDescriptor(context.descriptor(), context.field().get())) {
         return true;
@@ -286,12 +296,16 @@ && matchesFieldDescriptor(context.descriptor(), specificFieldDescriptor)) {
         }
       }
 
+      return false;
+    }
+
+    @Override
+    boolean matchesContentOfSubMessages(Context context, Cache cache) {
       for (Message message : context.messageFields()) {
         if (messageHasMatchingField(context, cache, message)) {
           return true;
         }
       }
-
       return false;
     }
 
@@ -387,6 +401,18 @@ private abstract static class CompoundFieldScopeImpl extends FieldScopeImpl {
       elements = ImmutableList.of(firstElem, secondElem);
     }
 
+    @Override
+    final boolean matchesContentOfSubMessages(Context context, Cache cache) {
+      // We ignore compound logic for checking sub-messages, since we want to ensure we inspect
+      // any sub-message which could match a FieldScopeImpl at a deeper path.
+      for (FieldScopeImpl impl : elements) {
+        if (impl.matchesContentOfSubMessages(context, cache)) {
+          return true;
+        }
+      }
+      return false;
+    }
+
     @Override
     final void validate(Descriptor descriptor) {
       for (FieldScopeImpl elem : elements) {
@@ -401,9 +427,9 @@ private static final class IntersectionScopeImpl extends CompoundFieldScopeImpl
     }
 
     @Override
-    boolean includesField(Context context, Cache cache) {
+    boolean matchesFieldPath(Context context, Cache cache) {
-      return elements.get(0).includesField(context, cache)
+      return elements.get(0).matchesFieldPath(context, cache)
-          && elements.get(1).includesField(context, cache);
+          && elements.get(1).matchesFieldPath(context, cache);
     }
   }
 
@@ -413,9 +439,9 @@ private static final class UnionScopeImpl extends CompoundFieldScopeImpl {
     }
 
     @Override
-    boolean includesField(Context context, Cache cache) {
+    boolean matchesFieldPath(Context context, Cache cache) {
-      return elements.get(0).includesField(context, cache)
+      return elements.get(0).matchesFieldPath(context, cache)
-          || elements.get(1).includesField(context, cache);
+          || elements.get(1).matchesFieldPath(context, cache);
     }
   }
 
@@ -425,8 +451,8 @@ private static final class NegationScopeImpl extends CompoundFieldScopeImpl {
     }
 
     @Override
-    boolean includesField(Context context, Cache cache) {
+    boolean matchesFieldPath(Context context, Cache cache) {
-      return !elements.get(0).includesField(context, cache);
+      return !elements.get(0).matchesFieldPath(context, cache);
     }
   }
 

diff --git a/extensions/proto/src/test/java/com/google/common/truth/extensions/proto/FieldScopesTest.java b/extensions/proto/src/test/java/com/google/common/truth/extensions/proto/FieldScopesTest.java
@@ -231,6 +231,39 @@ public void testIgnoreSubMessageField() {
     }
   }
 
+  @Test
+  public void testIgnoreFieldOfSubMessage() {
+    // Ignore o_int of sub message fields.
+    Message message = parse("o_int: 1 o_sub_test_message: { o_int: 2 r_string: \"foo\" }");
+    Message diffMessage1 = parse("o_int: 2 o_sub_test_message: { o_int: 2 r_string: \"foo\" }");
+    Message diffMessage2 = parse("o_int: 1 o_sub_test_message: { o_int: 2 r_string: \"bar\" }");
+    Message eqMessage = parse("o_int: 1 o_sub_test_message: { o_int: 3 r_string: \"foo\" }");
+
+    FieldDescriptor fieldDescriptor =
+        getFieldDescriptor("o_sub_test_message").getMessageType().findFieldByName("o_int");
+    FieldScope partialScope = FieldScopes.ignoringFieldDescriptors(fieldDescriptor);
+
+    expectThat(diffMessage1).withPartialScope(partialScope).isNotEqualTo(message);
+    expectThat(diffMessage2).withPartialScope(partialScope).isNotEqualTo(message);
+    expectThat(eqMessage).withPartialScope(partialScope).isEqualTo(message);
+
+    try {
+      assertThat(diffMessage1).withPartialScope(partialScope).isEqualTo(message);
+      fail("Expected error.");
+    } catch (AssertionError e) {
+      expectIsEqualToFailed(e);
+      expectSubstr(e, "modified: o_int: 1 -> 2");
+    }
+
+    try {
+      assertThat(diffMessage2).withPartialScope(partialScope).isEqualTo(message);
+      fail("Expected error.");
+    } catch (AssertionError e) {
+      expectIsEqualToFailed(e);
+      expectSubstr(e, "modified: o_sub_test_message.r_string[0]: \"foo\" -> \"bar\"");
+    }
+  }
+
   @Test
   public void testIgnoringAllButOneFieldOfSubMessage() {
     // Consider all of TestMessage, but none of o_sub_test_message, except