gh-2881: Problems with getOriginalSchema in Federated and Proxy Stores (

#2884) * gh-2881-initial attempt * gh-2881 ProxyStore GetOriginalSchema. * gh-2881 FederatedStore getSchema * Checkstyle * PMD fix * Simplifying FederatedGraphStorage to pass through operations and replace getSchema method with operation * Spotless apply * Fix for op in wrong place * Rename to use compactSchema - as used by the Operation Match current behaviour by not getting this by default * Add more details to Javadoc * CompactSchema rename for Proxy methods * Add Schema tests to ProxyStoreIT * Remove deprecated methods, replace with Operation * Migrate tests from GraphStorageTest to StoreSchemaTest * Code quality fixes and Copyright * gh-2881 fetch schema inline builder change. --------- Co-authored-by: GCHQDev404 <GCHQDev404@users.noreply.github.com> Co-authored-by: t92549 <80890692+t92549@users.noreply.github.com> Co-authored-by: GCHQDeveloper314 <94527357+GCHQDeveloper314@users.noreply.github.com>
gchq · Feb 21, 2023 · 9002d7e · 9002d7e
1 parent 5f22ae6
commit 9002d7e
Show file tree

Hide file tree

Showing 15 changed files with 307 additions and 188 deletions.
diff --git a/...ederated-store/src/main/java/uk/gov/gchq/gaffer/federatedstore/FederatedGraphStorage.java b/...ederated-store/src/main/java/uk/gov/gchq/gaffer/federatedstore/FederatedGraphStorage.java
@@ -27,17 +27,10 @@
 import uk.gov.gchq.gaffer.commonutil.JsonUtil;
 import uk.gov.gchq.gaffer.commonutil.exception.OverwritingException;
 import uk.gov.gchq.gaffer.commonutil.pair.Pair;
-import uk.gov.gchq.gaffer.core.exception.GafferRuntimeException;
-import uk.gov.gchq.gaffer.data.elementdefinition.exception.SchemaException;
 import uk.gov.gchq.gaffer.federatedstore.exception.StorageException;
-import uk.gov.gchq.gaffer.federatedstore.operation.FederatedOperation;
 import uk.gov.gchq.gaffer.graph.GraphConfig;
 import uk.gov.gchq.gaffer.graph.GraphSerialisable;
-import uk.gov.gchq.gaffer.store.Context;
 import uk.gov.gchq.gaffer.store.library.GraphLibrary;
-import uk.gov.gchq.gaffer.store.operation.GetSchema;
-import uk.gov.gchq.gaffer.store.schema.Schema;
-import uk.gov.gchq.gaffer.store.schema.Schema.Builder;
 import uk.gov.gchq.gaffer.user.User;
 
 import java.util.ArrayList;
@@ -234,35 +227,6 @@ public List<GraphSerialisable> get(final User user, final List<String> graphIds,
         return Collections.unmodifiableList(rtn);
     }
 
-    @Deprecated
-    public Schema getSchema(final FederatedOperation<Void, Object> operation, final Context context) {
-        if (null == context || null == context.getUser()) {
-            // no user then return an empty schema
-            return new Schema();
-        }
-        final List<String> graphIds = isNull(operation) ? null : operation.getGraphIds();
-
-        final Stream<GraphSerialisable> graphs = getStream(context.getUser(), graphIds);
-        final Builder schemaBuilder = new Builder();
-        try {
-            if (nonNull(operation) && operation.hasPayloadOperation() && operation.payloadInstanceOf(GetSchema.class) && ((GetSchema) operation.getPayloadOperation()).isCompact()) {
-                graphs.forEach(gs -> {
-                    try {
-                        schemaBuilder.merge(gs.getGraph().execute((GetSchema) operation.getPayloadOperation(), context));
-                    } catch (final Exception e) {
-                        throw new GafferRuntimeException("Unable to fetch schema from graph " + gs.getGraphId(), e);
-                    }
-                });
-            } else {
-                graphs.forEach(g -> schemaBuilder.merge(g.getSchema(graphLibrary)));
-            }
-        } catch (final SchemaException e) {
-            final List<String> resultGraphIds = getStream(context.getUser(), graphIds).map(GraphSerialisable::getGraphId).collect(Collectors.toList());
-            throw new SchemaException("Unable to merge the schemas for all of your federated graphs: " + resultGraphIds + ". You can limit which graphs to query for using the FederatedOperation.graphIds option.", e);
-        }
-        return schemaBuilder.build();
-    }
-
     private void validateAllGivenGraphIdsAreVisibleForUser(final User user, final Collection<String> graphIds, final String adminAuth) {
         if (null != graphIds) {
             final Collection<String> visibleIds = getAllIds(user, adminAuth);

diff --git a/...ation/federated-store/src/main/java/uk/gov/gchq/gaffer/federatedstore/FederatedStore.java b/...ation/federated-store/src/main/java/uk/gov/gchq/gaffer/federatedstore/FederatedStore.java
@@ -58,6 +58,7 @@
 import uk.gov.gchq.gaffer.federatedstore.util.MergeSchema;
 import uk.gov.gchq.gaffer.graph.GraphSerialisable;
 import uk.gov.gchq.gaffer.operation.Operation;
+import uk.gov.gchq.gaffer.operation.OperationException;
 import uk.gov.gchq.gaffer.operation.impl.Validate;
 import uk.gov.gchq.gaffer.operation.impl.add.AddElements;
 import uk.gov.gchq.gaffer.operation.impl.function.Aggregate;
@@ -108,7 +109,6 @@
 import static uk.gov.gchq.gaffer.federatedstore.FederatedStoreProperties.STORE_CONFIGURED_GRAPHIDS;
 import static uk.gov.gchq.gaffer.federatedstore.FederatedStoreProperties.STORE_CONFIGURED_MERGE_FUNCTIONS;
 import static uk.gov.gchq.gaffer.federatedstore.util.FederatedStoreUtil.getCleanStrings;
-import static uk.gov.gchq.gaffer.federatedstore.util.FederatedStoreUtil.getFederatedWrappedSchema;
 import static uk.gov.gchq.gaffer.federatedstore.util.FederatedStoreUtil.loadStoreConfiguredGraphIdsListFrom;
 import static uk.gov.gchq.gaffer.federatedstore.util.FederatedStoreUtil.loadStoreConfiguredMergeFunctionMapFrom;
 
@@ -324,34 +324,50 @@ public List<String> getAllGraphIds(final User user, final boolean userRequesting
     }
 
     /**
-     * Get {@link Schema} for this FederatedStore
+     * This method exists for compatibility only. It will
+     * always return a blank {@link Schema}. Either use the
+     * {@link FederatedStore#getSchema} method and supply a
+     * {@link Context}, or ideally use the {@link GetSchema}
+     * operation instead.
      *
-     * @return schema
+     * @return {@link Schema} blank schema
      */
     @Override
     public Schema getSchema() {
-        return getSchema((Context) null);
+        return getSchema(new Context(), true);
     }
 
     /**
-     * Get {@link Schema} for this FederatedStore
+     * This method exists for compatibility only. It will
+     * always return a blank {@link Schema}. Either use the
+     * {@link FederatedStore#getSchema} method and supply a
+     * {@link Context}, or ideally use the {@link GetSchema}
+     * operation instead.
      *
-     * @param context context with User.
-     * @return schema
+     * @return {@link Schema} blank schema
      */
-    public Schema getSchema(final Context context) {
-        return getSchema(getFederatedWrappedSchema(), context);
+    @Override
+    public Schema getOriginalSchema() {
+        return getSchema(new Context(), false);
     }
 
     /**
-     * Get {@link Schema} for this FederatedStore
+     * Get {@link Schema} for this FederatedStore.
+     * <p>
+     * This will return a merged schema of the original schemas
+     * or the optimised compact schemas of the stores inside
+     * this FederatedStore.
      *
-     * @param operation operation with graphIds.
-     * @param context   context with User.
+     * @param context context with valid User
+     * @param getCompactSchema if true, gets the optimised compact schemas
      * @return schema
      */
-    public Schema getSchema(final FederatedOperation operation, final Context context) {
-        return graphStorage.getSchema(operation, context);
+    public Schema getSchema(final Context context, final boolean getCompactSchema) {
+        try {
+            return execute(new GetSchema.Builder().compact(getCompactSchema).build(), context);
+        } catch (final OperationException e) {
+            throw new GafferRuntimeException("Unable to execute GetSchema Operation", e);
+        }
     }
 
     /**
@@ -589,8 +605,8 @@ private List<GraphSerialisable> getDefaultGraphs(final User user, final IFederat
 
             final List<String> graphIds = new ArrayList<>(storeConfiguredGraphIds);
             final List<String> federatedStoreSystemUser = getAllGraphIds(new User.Builder()
-                    .userId(FEDERATED_STORE_SYSTEM_USER)
-                    .opAuths(this.getProperties().getAdminAuth()).build(),
+                            .userId(FEDERATED_STORE_SYSTEM_USER)
+                            .opAuths(this.getProperties().getAdminAuth()).build(),
                     true);
             graphIds.retainAll(federatedStoreSystemUser);
 

diff --git a/...in/java/uk/gov/gchq/gaffer/federatedstore/operation/FederatedOperationChainValidator.java b/...in/java/uk/gov/gchq/gaffer/federatedstore/operation/FederatedOperationChainValidator.java
@@ -1,5 +1,5 @@
 /*
- * Copyright 2016-2022 Crown Copyright
+ * Copyright 2016-2023 Crown Copyright
  *
  * Licensed under the Apache License, Version 2.0 (the "License");
  * you may not use this file except in compliance with the License.
@@ -16,13 +16,16 @@
 
 package uk.gov.gchq.gaffer.federatedstore.operation;
 
+import uk.gov.gchq.gaffer.core.exception.GafferRuntimeException;
 import uk.gov.gchq.gaffer.data.elementdefinition.view.View;
 import uk.gov.gchq.gaffer.federatedstore.FederatedStore;
 import uk.gov.gchq.gaffer.graph.GraphSerialisable;
 import uk.gov.gchq.gaffer.operation.Operation;
+import uk.gov.gchq.gaffer.operation.OperationException;
 import uk.gov.gchq.gaffer.store.Context;
 import uk.gov.gchq.gaffer.store.Store;
 import uk.gov.gchq.gaffer.store.StoreTrait;
+import uk.gov.gchq.gaffer.store.operation.GetSchema;
 import uk.gov.gchq.gaffer.store.operation.OperationChainValidator;
 import uk.gov.gchq.gaffer.store.schema.Schema;
 import uk.gov.gchq.gaffer.store.schema.ViewValidator;
@@ -35,7 +38,6 @@
 import java.util.stream.Collectors;
 
 import static java.util.Objects.nonNull;
-import static uk.gov.gchq.gaffer.federatedstore.util.FederatedStoreUtil.getFederatedWrappedSchema;
 import static uk.gov.gchq.gaffer.federatedstore.util.FederatedStoreUtil.shallowCloneWithDeepOptions;
 
 /**
@@ -51,9 +53,13 @@ public FederatedOperationChainValidator(final ViewValidator viewValidator) {
 
     @Override
     protected Schema getSchema(final Operation op, final User user, final Store store) {
-        return (op instanceof FederatedOperation)
-                ? ((FederatedStore) store).getSchema(getFederatedWrappedSchema().graphIds(((FederatedOperation) op).getGraphIds()), new Context(user))
-                : ((FederatedStore) store).getSchema(getFederatedWrappedSchema(), new Context(user));
+        try {
+            return (op instanceof FederatedOperation)
+                    ? store.execute(new FederatedOperation.Builder().<Void, Schema>op(new GetSchema()).graphIds(((FederatedOperation<?, ?>) op).getGraphIds()).build(), new Context(user))
+                    : store.execute(new GetSchema(), new Context(user));
+        } catch (final OperationException e) {
+            throw new GafferRuntimeException("Unable to execute GetSchema Operation", e);
+        }
     }
 
     @Override

diff --git a/.../java/uk/gov/gchq/gaffer/federatedstore/operation/handler/FederatedDelegateToHandler.java b/.../java/uk/gov/gchq/gaffer/federatedstore/operation/handler/FederatedDelegateToHandler.java
@@ -1,5 +1,5 @@
 /*
- * Copyright 2022 Crown Copyright
+ * Copyright 2022-2023 Crown Copyright
  *
  * Licensed under the Apache License, Version 2.0 (the "License");
  * you may not use this file except in compliance with the License.
@@ -44,7 +44,7 @@ public Iterable<? extends Element> doOperation(final InputOutput<Iterable<? exte
                 || ValidateHandler.class.isAssignableFrom(handler.getClass())
                 || AggregateHandler.class.isAssignableFrom(handler.getClass())) {
             // Use the doOperation which requires a schema.
-            return (Iterable<? extends Element>) ((OperationWithSchemaHandler) handler).doOperation(operation, ((FederatedStore) store).getSchema(context));
+            return (Iterable<? extends Element>) ((OperationWithSchemaHandler) handler).doOperation(operation, ((FederatedStore) store).getSchema(context, true));
         } else {
             return handler.doOperation(operation, context, store);
         }

diff --git a/...erated-store/src/main/java/uk/gov/gchq/gaffer/federatedstore/util/FederatedStoreUtil.java b/...erated-store/src/main/java/uk/gov/gchq/gaffer/federatedstore/util/FederatedStoreUtil.java
@@ -280,11 +280,6 @@ public static String getDeprecatedGraphIds(final Operation operation) throws Gaf
         return deprecatedGraphIds;
     }
 
-    @Deprecated
-    public static FederatedOperation<Void, Iterable<Schema>> getFederatedWrappedSchema() {
-        return new FederatedOperation.Builder().<Void, Iterable<Schema>>op(new GetSchema()).build();
-    }
-
     /**
      * Return a clone of the given operations with a deep clone of options.
      * <p>

diff --git a/...ated-store/src/test/java/uk/gov/gchq/gaffer/federatedstore/FederatedGraphStorageTest.java b/...ated-store/src/test/java/uk/gov/gchq/gaffer/federatedstore/FederatedGraphStorageTest.java
@@ -1,5 +1,5 @@
 /*
- * Copyright 2017-2022 Crown Copyright
+ * Copyright 2017-2023 Crown Copyright
  *
  * Licensed under the Apache License, Version 2.0 (the "License");
  * you may not use this file except in compliance with the License.
@@ -50,7 +50,6 @@
 import static org.assertj.core.api.Assertions.assertThatIllegalArgumentException;
 import static org.junit.jupiter.api.Assertions.assertEquals;
 import static org.junit.jupiter.api.Assertions.assertFalse;
-import static org.junit.jupiter.api.Assertions.assertNotEquals;
 import static org.junit.jupiter.api.Assertions.assertTrue;
 import static org.mockito.Mockito.mock;
 import static uk.gov.gchq.gaffer.federatedstore.FederatedGraphStorage.GRAPH_IDS_NOT_VISIBLE;
@@ -60,9 +59,6 @@
 import static uk.gov.gchq.gaffer.federatedstore.FederatedStoreTestUtil.ENTITIES;
 import static uk.gov.gchq.gaffer.federatedstore.FederatedStoreTestUtil.GRAPH_ID_ACCUMULO;
 import static uk.gov.gchq.gaffer.federatedstore.FederatedStoreTestUtil.STRING;
-import static uk.gov.gchq.gaffer.federatedstore.FederatedStoreTestUtil.contextAuthUser;
-import static uk.gov.gchq.gaffer.federatedstore.FederatedStoreTestUtil.contextBlankUser;
-import static uk.gov.gchq.gaffer.federatedstore.FederatedStoreTestUtil.contextTestUser;
 import static uk.gov.gchq.gaffer.federatedstore.FederatedStoreTestUtil.loadAccumuloStoreProperties;
 import static uk.gov.gchq.gaffer.federatedstore.FederatedStoreTestUtil.resetForFederatedTests;
 import static uk.gov.gchq.gaffer.store.TestTypes.DIRECTED_EITHER;
@@ -295,82 +291,6 @@ public void shouldNotGetGraphForBlankUserWithIncorrectId() throws Exception {
                 .withMessage(String.format(GRAPH_IDS_NOT_VISIBLE, singleton(X)));
     }
 
-    @Test
-    @Deprecated // TODO FS move to FedSchema Tests, when getSchema is deleted
-    public void shouldChangeSchemaWhenAddingGraphB() throws Exception {
-        //given
-        graphStorage.put(graphSerialisableA, auth1Access);
-        final Schema schemaA = graphStorage.getSchema(null, contextTestUser());
-        assertEquals(1, schemaA.getTypes().size());
-        assertEquals(String.class, schemaA.getType(STRING + 1).getClazz());
-        assertEquals(getEntityDefinition(1), schemaA.getElement(ENTITIES + 1));
-        graphStorage.put(graphSerialisableB, auth1Access);
-        final Schema schemaAB = graphStorage.getSchema(null, contextTestUser());
-        assertNotEquals(schemaA, schemaAB);
-        assertEquals(2, schemaAB.getTypes().size());
-        assertEquals(String.class, schemaAB.getType(STRING + 1).getClazz());
-        assertEquals(String.class, schemaAB.getType(STRING + 2).getClazz());
-        assertEquals(getEntityDefinition(1), schemaAB.getElement(ENTITIES + 1));
-        assertEquals(getEntityDefinition(2), schemaAB.getElement(ENTITIES + 2));
-    }
-
-
-    @Test
-    @Deprecated // TODO FS move to FedSchema Tests, when getSchema is deleted
-    public void shouldGetSchemaForOwningUser() throws Exception {
-        graphStorage.put(graphSerialisableA, auth1Access);
-        graphStorage.put(graphSerialisableB, new FederatedAccess(singleton(X), X));
-        final Schema schema = graphStorage.getSchema(null, contextTestUser());
-        assertNotEquals(2, schema.getTypes().size(), "Revealing hidden schema");
-        assertEquals(1, schema.getTypes().size());
-        assertEquals(String.class, schema.getType(STRING + 1).getClazz());
-        assertEquals(getEntityDefinition(1), schema.getElement(ENTITIES + 1));
-    }
-
-    @Test
-    @Deprecated // TODO FS move to FedSchema Tests, when getSchema is deleted
-    public void shouldNotGetSchemaForOwningUserWhenBlockingReadAccessPredicateConfigured() throws Exception {
-        graphStorage.put(graphSerialisableA, blockingReadAccess);
-        graphStorage.put(graphSerialisableB, new FederatedAccess(singleton(X), X));
-        final Schema schema = graphStorage.getSchema(null, contextTestUser());
-        assertNotEquals(2, schema.getTypes().size(), "Revealing hidden schema");
-        assertEquals(0, schema.getTypes().size(), "Revealing hidden schema");
-    }
-
-    @Test
-    @Deprecated // TODO FS move to FedSchema Tests, when getSchema is deleted
-    public void shouldGetSchemaForAuthUser() throws Exception {
-        graphStorage.put(graphSerialisableA, auth1Access);
-        graphStorage.put(graphSerialisableB, new FederatedAccess(singleton(X), X));
-        final Schema schema = graphStorage.getSchema(null, contextAuthUser());
-        assertNotEquals(2, schema.getTypes().size(), "Revealing hidden schema");
-        assertEquals(1, schema.getTypes().size());
-        assertEquals(String.class, schema.getType(STRING + 1).getClazz());
-        assertEquals(getEntityDefinition(1), schema.getElement(ENTITIES + 1));
-    }
-
-    @Test
-    @Deprecated // TODO FS move to FedSchema Tests, when getSchema is deleted
-    public void shouldNotGetSchemaForBlankUser() throws Exception {
-        graphStorage.put(graphSerialisableA, auth1Access);
-        graphStorage.put(graphSerialisableB, new FederatedAccess(singleton(X), X));
-        final Schema schema = graphStorage.getSchema(null, contextBlankUser());
-        assertNotEquals(2, schema.getTypes().size(), "Revealing hidden schema");
-        assertEquals(0, schema.getTypes().size(), "Revealing hidden schema");
-    }
-
-    @Test
-    @Deprecated // TODO FS move to FedSchema Tests, when getSchema is deleted
-    public void shouldGetSchemaForBlankUserWhenPermissiveReadAccessPredicateConfigured() throws Exception {
-        graphStorage.put(graphSerialisableA, permissiveReadAccess);
-        graphStorage.put(graphSerialisableB, new FederatedAccess(singleton(X), X));
-        final Schema schema = graphStorage.getSchema(null, contextBlankUser());
-        assertNotEquals(2, schema.getTypes().size(), "Revealing hidden schema");
-        assertEquals(1, schema.getTypes().size());
-        assertEquals(String.class, schema.getType(STRING + 1).getClazz());
-        assertEquals(getEntityDefinition(1), schema.getElement(ENTITIES + 1));
-    }
-
     @Test
     public void shouldRemoveForOwningUser() throws Exception {
         //given