ISPN-6589 ProtobufIndexedFieldProvider may mistakenly report nested f…

…ields as unindexed

remote-query/remote-query-server/src/main/java/org/infinispan/query/remote/impl/ProtobufIndexedFieldProvider.java

@@ -7,6 +7,7 @@
 import org.infinispan.query.remote.impl.indexing.IndexingMetadata;
 
 import java.util.List;
+import java.util.function.BiFunction;
 
 /**
  * Tests if a field is indexed by examining the Protobuf metadata.
@@ -18,36 +19,24 @@ final class ProtobufIndexedFieldProvider implements BooleShannonExpansion.Indexe
 
    private final Descriptor messageDescriptor;
 
-   public ProtobufIndexedFieldProvider(Descriptor messageDescriptor) {
+   ProtobufIndexedFieldProvider(Descriptor messageDescriptor) {
+      if (messageDescriptor == null) {
+         throw new IllegalArgumentException("argument cannot be null");
+      }
       this.messageDescriptor = messageDescriptor;
    }
 
    @Override
    public boolean isIndexed(List<String> propertyPath) {
-      Descriptor md = messageDescriptor;
-      int i = 0;
-      for (String p : propertyPath) {
-         i++;
-         FieldDescriptor field = md.findFieldByName(p);
-         if (field == null) {
-            break;
-         }
-         if (field.getJavaType() == JavaType.MESSAGE) {
-            md = field.getMessageType();
-         } else {
-            if (i == propertyPath.size()) {
-               IndexingMetadata indexingMetadata = messageDescriptor.getProcessedAnnotation(IndexingMetadata.INDEXED_ANNOTATION);
-               return indexingMetadata == null || indexingMetadata.isFieldIndexed(field.getNumber());
-            } else {
-               break;
-            }
-         }
-      }
-      return false;
+      return getMetadata(propertyPath, IndexingMetadata::isFieldIndexed);
    }
 
    @Override
    public boolean isStored(List<String> propertyPath) {
+      return getMetadata(propertyPath, IndexingMetadata::isFieldStored);
+   }
+
+   private boolean getMetadata(List<String> propertyPath, BiFunction<IndexingMetadata, Integer, Boolean> metadataFun) {
       Descriptor md = messageDescriptor;
       int i = 0;
       for (String p : propertyPath) {
@@ -56,15 +45,15 @@ public boolean isStored(List<String> propertyPath) {
          if (field == null) {
             break;
          }
+         IndexingMetadata indexingMetadata = md.getProcessedAnnotation(IndexingMetadata.INDEXED_ANNOTATION);
+         boolean res = indexingMetadata == null || metadataFun.apply(indexingMetadata, field.getNumber());
+         if (!res) {
+            break;
+         }
          if (field.getJavaType() == JavaType.MESSAGE) {
             md = field.getMessageType();
          } else {
-            if (i == propertyPath.size()) {
-               IndexingMetadata indexingMetadata = messageDescriptor.getProcessedAnnotation(IndexingMetadata.INDEXED_ANNOTATION);
-               return indexingMetadata == null || indexingMetadata.isFieldStored(field.getNumber());
-            } else {
-               break;
-            }
+            return i == propertyPath.size();
          }
       }
       return false;

remote-query/remote-query-server/src/test/java/org/infinispan/query/remote/impl/ProtobufIndexedFieldProviderTest.java

@@ -0,0 +1,73 @@
+package org.infinispan.query.remote.impl;
+
+import org.infinispan.configuration.cache.ConfigurationBuilder;
+import org.infinispan.configuration.cache.Index;
+import org.infinispan.manager.EmbeddedCacheManager;
+import org.infinispan.protostream.FileDescriptorSource;
+import org.infinispan.protostream.SerializationContext;
+import org.infinispan.test.SingleCacheManagerTest;
+import org.infinispan.test.fwk.TestCacheManagerFactory;
+import org.infinispan.transaction.TransactionMode;
+import org.testng.annotations.Test;
+
+import java.util.Arrays;
+
+import static org.junit.Assert.assertFalse;
+import static org.junit.Assert.assertTrue;
+
+/**
+ * Test that ProtobufIndexedFieldProvider correctly identifies indexed fields based on the protostream annotations
+ * applied in the test model (bank.proto).
+ *
+ * @author anistor@redhat.com
+ * @since 9.0
+ */
+@Test(groups = "functional", testName = "query.remote.impl.ProtobufIndexedFieldProviderTest")
+public class ProtobufIndexedFieldProviderTest extends SingleCacheManagerTest {
+
+   private static final String PROTO_DEFINITIONS =
+         "/** @Indexed */ message User {\n" +
+               "\n" +
+               "   /** @IndexedField */ required string name = 1;\n" +
+               "\n" +
+               "   required string surname = 2;\n" +
+               "\n" +
+               "   /** @Indexed(true) */" +
+               "   message Address {\n" +
+               "      /** @IndexedField */ required string street = 10;\n" +
+               "      required string postCode = 20;\n" +
+               "   }\n" +
+               "\n" +
+               "   /** @IndexedField */ repeated Address indexedAddresses = 3; \n" +
+               "\n" +
+               "   /** @IndexedField(index=false, store=false) */ repeated Address unindexedAddresses = 4;\n" +
+               "}\n";
+
+   protected EmbeddedCacheManager createCacheManager() throws Exception {
+      ConfigurationBuilder cfg = getDefaultStandaloneCacheConfig(true);
+      cfg.transaction().transactionMode(TransactionMode.TRANSACTIONAL)
+            .indexing().index(Index.ALL)
+            .addProperty("default.directory_provider", "ram")
+            .addProperty("lucene_version", "LUCENE_CURRENT");
+      return TestCacheManagerFactory.createCacheManager(cfg);
+   }
+
+   public void testProvider() throws Exception {
+      SerializationContext serCtx = ProtobufMetadataManagerImpl.getSerializationContext(cacheManager);
+      serCtx.registerProtoFiles(FileDescriptorSource.fromString("user_definition.proto", PROTO_DEFINITIONS));
+      ProtobufIndexedFieldProvider userIndexedFieldProvider = new ProtobufIndexedFieldProvider(serCtx.getMessageDescriptor("User"));
+      ProtobufIndexedFieldProvider addressIndexedFieldProvider = new ProtobufIndexedFieldProvider(serCtx.getMessageDescriptor("User.Address"));
+
+      // try top level attributes
+      assertTrue(userIndexedFieldProvider.isIndexed(Arrays.asList("name")));
+      assertFalse(userIndexedFieldProvider.isIndexed(Arrays.asList("surname")));
+      assertTrue(addressIndexedFieldProvider.isIndexed(Arrays.asList("street")));
+      assertFalse(addressIndexedFieldProvider.isIndexed(Arrays.asList("postCode")));
+
+      // try nested attributes
+      assertTrue(userIndexedFieldProvider.isIndexed(Arrays.asList("indexedAddresses", "street")));
+      assertFalse(userIndexedFieldProvider.isIndexed(Arrays.asList("indexedAddresses", "postCode")));
+      assertFalse(userIndexedFieldProvider.isIndexed(Arrays.asList("unindexedAddresses", "street")));
+      assertFalse(userIndexedFieldProvider.isIndexed(Arrays.asList("unindexedAddresses", "postCode")));
+   }
+}