Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

gh-2573: Replace hasTrait usages with Operation #2574

Merged
merged 13 commits into from
Jan 28, 2022
Merged
Show file tree
Hide file tree
Changes from 10 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 1 addition & 8 deletions core/graph/src/main/java/uk/gov/gchq/gaffer/graph/Graph.java
Original file line number Diff line number Diff line change
Expand Up @@ -443,21 +443,14 @@ public String getDescription() {
return config.getDescription();
}

/**
* @param storeTrait the store trait to check
* @return true if the store has the given trait.
*/
public boolean hasTrait(final StoreTrait storeTrait) {
return store.hasTrait(storeTrait);
}

/**
* Returns all the {@link StoreTrait}s for the contained {@link Store}
* implementation
*
* @return a {@link Set} of all of the {@link StoreTrait}s that the store
* has.
*/
@Deprecated
public Set<StoreTrait> getStoreTraits() {
return store.getTraits();
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -20,9 +20,13 @@
import org.slf4j.LoggerFactory;

import uk.gov.gchq.gaffer.commonutil.StreamUtil;
import uk.gov.gchq.gaffer.core.exception.GafferRuntimeException;
import uk.gov.gchq.gaffer.graph.Graph;
import uk.gov.gchq.gaffer.graph.GraphConfig;
import uk.gov.gchq.gaffer.operation.OperationException;
import uk.gov.gchq.gaffer.store.Context;
import uk.gov.gchq.gaffer.store.StoreTrait;
import uk.gov.gchq.gaffer.store.operation.HasTrait;
import uk.gov.gchq.gaffer.store.schema.Schema;
import uk.gov.gchq.gaffer.store.schema.TypeDefinition;
import uk.gov.gchq.koryphe.impl.predicate.AgeOff;
Expand Down Expand Up @@ -52,7 +56,17 @@ public static Graph createGraph(final String graphId, final String cacheStorePro
.addSchema(createSchema(timeToLive));

final Graph graph = graphBuilder.build();
if (!graph.hasTrait(StoreTrait.STORE_VALIDATION)) {

Boolean hasStoreValidation;
try {
hasStoreValidation = graph.execute(new HasTrait.Builder()
.trait(StoreTrait.STORE_VALIDATION)
.currentTraits(false)
.build(), new Context());
} catch (final OperationException e) {
throw new GafferRuntimeException("Error performing HasTrait Operation while creating Graph.", e);
}
if (null == hasStoreValidation || !hasStoreValidation) {
LOGGER.warn("Gaffer JSON export graph does not have {} trait so results may not be aged off.", StoreTrait.STORE_VALIDATION.name());
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -47,6 +47,7 @@
import static org.junit.jupiter.api.Assertions.assertSame;
import static org.junit.jupiter.api.Assertions.assertTrue;
import static org.mockito.Mockito.mock;
import static org.mockito.Mockito.times;
import static org.mockito.Mockito.verify;

public class ExportToGafferResultCacheHandlerTest {
Expand Down Expand Up @@ -131,7 +132,7 @@ public void shouldHandleOperationByDelegatingToAnNewExporter() throws OperationE
// Then
assertSame(handlerResult, results);
final ArgumentCaptor<OperationChain> opChain = ArgumentCaptor.forClass(OperationChain.class);
verify(cacheStore).execute(opChain.capture(), Mockito.any(Context.class));
verify(cacheStore, times(3)).execute(opChain.capture(), Mockito.any(Context.class));
assertEquals(1, opChain.getValue().getOperations().size());
assertTrue(opChain.getValue().getOperations().get(0) instanceof AddElements);
final GafferResultCacheExporter exporter = context.getExporter(GafferResultCacheExporter.class);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -50,6 +50,7 @@
import static org.junit.jupiter.api.Assertions.assertTrue;
import static org.mockito.BDDMockito.given;
import static org.mockito.Mockito.mock;
import static org.mockito.Mockito.times;
import static org.mockito.Mockito.verify;

public class GetGafferResultCacheExportHandlerTest {
Expand Down Expand Up @@ -131,7 +132,7 @@ public void shouldHandleOperationByDelegatingToAnNewExporter() throws OperationE
// Then
assertThat((Iterable) handlerResult).isEmpty();
final ArgumentCaptor<OperationChain> opChain = ArgumentCaptor.forClass(OperationChain.class);
verify(cacheStore).execute(opChain.capture(), Mockito.any());
verify(cacheStore, times(3)).execute(opChain.capture(), Mockito.any());
assertThat(opChain.getValue().getOperations()).hasSize(1);
assertTrue(opChain.getValue().getOperations().get(0) instanceof GetElements);
final GafferResultCacheExporter exporter = context.getExporter(GafferResultCacheExporter.class);
Expand Down
27 changes: 12 additions & 15 deletions core/store/src/main/java/uk/gov/gchq/gaffer/store/Store.java
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@
import uk.gov.gchq.gaffer.commonutil.CloseableUtil;
import uk.gov.gchq.gaffer.commonutil.ExecutorService;
import uk.gov.gchq.gaffer.commonutil.iterable.CloseableIterable;
import uk.gov.gchq.gaffer.core.exception.GafferRuntimeException;
import uk.gov.gchq.gaffer.data.element.Element;
import uk.gov.gchq.gaffer.data.element.IdentifierType;
import uk.gov.gchq.gaffer.data.element.id.EntityId;
Expand Down Expand Up @@ -282,9 +283,9 @@ public void initialise(final String graphId, final Schema schema, final StorePro
startCacheServiceLoader(properties);
this.jobTracker = createJobTracker();

addOpHandlers();
t92549 marked this conversation as resolved.
Show resolved Hide resolved
optimiseSchema();
validateSchemas();
addOpHandlers();
addExecutorService(properties);

if (properties.getJobTrackerEnabled() && !jobsRescheduled) {
Expand Down Expand Up @@ -334,19 +335,6 @@ public void updateJsonSerialiser() {
updateJsonSerialiser(getProperties());
}

/**
* Returns true if the Store can handle the provided trait and false if it
* cannot.
*
* @param storeTrait the Class of the Processor to be checked.
* @return true if the Processor can be handled and false if it cannot.
*/
@Deprecated
public boolean hasTrait(final StoreTrait storeTrait) {
final Set<StoreTrait> traits = getTraits();
return null != traits && traits.contains(storeTrait);
}

/**
* Returns the {@link uk.gov.gchq.gaffer.store.StoreTrait}s for this store.
* Most stores should support FILTERING.
Expand Down Expand Up @@ -687,7 +675,16 @@ public void setGraphLibrary(final GraphLibrary library) {
}

public void optimiseSchema() {
schema = schemaOptimiser.optimise(schema, hasTrait(StoreTrait.ORDERED));
Boolean isOrdered;
try {
isOrdered = execute(new HasTrait.Builder()
.trait(StoreTrait.ORDERED)
.currentTraits(false)
.build(), new Context());
} catch (final OperationException e) {
throw new GafferRuntimeException("Error performing HasTrait Operation while optimising schema.", e);
}
schema = schemaOptimiser.optimise(schema, null == isOrdered ? false : isOrdered);
}

public void validateSchemas() {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -23,16 +23,20 @@
import uk.gov.gchq.gaffer.store.operation.GetTraits;
import uk.gov.gchq.gaffer.store.operation.HasTrait;

import java.util.HashMap;
import java.util.Set;

import static java.util.Objects.isNull;


public class HasTraitHandler implements OutputOperationHandler<HasTrait, Boolean> {

@Override
public Boolean doOperation(final HasTrait operation, final Context context, final Store store) throws OperationException {
final Set<StoreTrait> traits = store.execute(new GetTraits.Builder()
.currentTraits(operation.isCurrentTraits())
.options(operation.getOptions())
//deep copy options
.options(isNull(operation.getOptions()) ? new HashMap<>() : new HashMap<>(operation.getOptions()))
t92549 marked this conversation as resolved.
Show resolved Hide resolved
.build(), context
);
if (null == traits) {
Expand Down
13 changes: 5 additions & 8 deletions core/store/src/test/java/uk/gov/gchq/gaffer/store/StoreTest.java
Original file line number Diff line number Diff line change
Expand Up @@ -157,6 +157,7 @@
import static org.junit.jupiter.api.Assertions.assertSame;
import static org.junit.jupiter.api.Assertions.assertTrue;
import static org.junit.jupiter.api.Assertions.fail;
import static org.mockito.AdditionalAnswers.returnsFirstArg;
import static org.mockito.ArgumentMatchers.any;
import static org.mockito.ArgumentMatchers.anyLong;
import static org.mockito.ArgumentMatchers.eq;
Expand All @@ -166,6 +167,7 @@
import static org.mockito.Mockito.never;
import static org.mockito.Mockito.times;
import static org.mockito.Mockito.verify;
import static org.mockito.Mockito.when;
import static uk.gov.gchq.gaffer.store.StoreTrait.INGEST_AGGREGATION;
import static uk.gov.gchq.gaffer.store.StoreTrait.ORDERED;
import static uk.gov.gchq.gaffer.store.StoreTrait.PRE_AGGREGATION_FILTERING;
Expand Down Expand Up @@ -195,6 +197,7 @@ public void setup() {
JSONSerialiser.update();

schemaOptimiser = mock(SchemaOptimiser.class);
when(schemaOptimiser.optimise(any(Schema.class), any(Boolean.class))).then(returnsFirstArg());
GCHQDev404 marked this conversation as resolved.
Show resolved Hide resolved
operationChainValidator = mock(OperationChainValidator.class);
store = new StoreImpl();
given(operationChainValidator.validate(any(OperationChain.class), any(User.class), any(Store.class))).willReturn(new ValidationResult());
Expand Down Expand Up @@ -369,7 +372,6 @@ public void shouldCloseOperationIfExceptionThrown() throws Exception {

@Test
public void shouldThrowExceptionIfOperationChainIsInvalid() throws OperationException, StoreException {
// Given
// Given
final Schema schema = createSchemaMock();
final StoreProperties properties = mock(StoreProperties.class);
Expand Down Expand Up @@ -1165,8 +1167,8 @@ public ArrayList<Operation> getDoUnhandledOperationCalls() {
}

@Override
public void optimiseSchema() {
schemaOptimiser.optimise(getSchema(), hasTrait(StoreTrait.ORDERED));
protected SchemaOptimiser createSchemaOptimiser() {
return schemaOptimiser;
}

@Override
Expand Down Expand Up @@ -1259,11 +1261,6 @@ public ArrayList<Operation> getDoUnhandledOperationCalls() {
return doUnhandledOperationCalls;
}

@Override
public void optimiseSchema() {
schemaOptimiser.optimise(getSchema(), hasTrait(StoreTrait.ORDERED));
}

@Override
protected JobTracker createJobTracker() {
if (getProperties().getJobTrackerEnabled()) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -38,7 +38,9 @@
import uk.gov.gchq.gaffer.operation.impl.get.GetElements;
import uk.gov.gchq.gaffer.operation.impl.output.ToCsv;
import uk.gov.gchq.gaffer.operation.impl.output.ToSet;
import uk.gov.gchq.gaffer.store.Context;
import uk.gov.gchq.gaffer.store.StoreTrait;
import uk.gov.gchq.gaffer.store.operation.HasTrait;
import uk.gov.gchq.gaffer.types.FreqMap;
import uk.gov.gchq.gaffer.types.function.FreqMapExtractor;
import uk.gov.gchq.gaffer.user.User;
Expand Down Expand Up @@ -140,8 +142,8 @@ public void checkM4JunctionCount() throws OperationException {

@Test
public void checkM4Junction17To16RoadUse() throws OperationException, ParseException {
assumeTrue(this.graph.hasTrait(StoreTrait.INGEST_AGGREGATION), "Skipping test as the store does not implement required trait.");
assumeTrue(this.graph.hasTrait(StoreTrait.PRE_AGGREGATION_FILTERING), "Skipping test as the store does not implement required trait.");
assumeTrue(this.graph.execute(new HasTrait.Builder().trait(StoreTrait.INGEST_AGGREGATION).currentTraits(false).build(), new Context()), "Skipping test as the store does not implement required trait.");
assumeTrue(this.graph.execute(new HasTrait.Builder().trait(StoreTrait.PRE_AGGREGATION_FILTERING).currentTraits(false).build(), new Context()), "Skipping test as the store does not implement required trait.");
assertNotNull(this.graph, "graph is null");

final GetElements query = new GetElements.Builder()
Expand Down Expand Up @@ -196,7 +198,7 @@ public void checkM4Junction17To16RoadUse() throws OperationException, ParseExcep

@Test
public void checkM4Junction16Use() throws OperationException {
assumeTrue(this.graph.hasTrait(StoreTrait.QUERY_AGGREGATION), "Skipping test as the store does not implement required trait.");
assumeTrue(this.graph.execute(new HasTrait.Builder().trait(StoreTrait.QUERY_AGGREGATION).currentTraits(false).build(), new Context()), "Skipping test as the store does not implement required trait.");
assertNotNull(this.graph, "graph is null");

final GetElements query = new GetElements.Builder()
Expand Down Expand Up @@ -231,10 +233,10 @@ public void checkM4Junction16Use() throws OperationException {

@Test
public void checkRoadJunctionsInSouthWestHeavilyUsedByBusesIn2000() throws OperationException, ParseException {
assumeTrue(this.graph.hasTrait(StoreTrait.QUERY_AGGREGATION), "Skipping test as the store does not implement required trait.");
assumeTrue(this.graph.hasTrait(StoreTrait.TRANSFORMATION), "Skipping test as the store does not implement required trait.");
assumeTrue(this.graph.hasTrait(StoreTrait.PRE_AGGREGATION_FILTERING), "Skipping test as the store does not implement required trait.");
assumeTrue(this.graph.hasTrait(StoreTrait.POST_AGGREGATION_FILTERING), "Skipping test as the store does not implement required trait.");
assumeTrue(this.graph.execute(new HasTrait.Builder().trait(StoreTrait.QUERY_AGGREGATION).currentTraits(false).build(), new Context()), "Skipping test as the store does not implement required trait.");
assumeTrue(this.graph.execute(new HasTrait.Builder().trait(StoreTrait.TRANSFORMATION).currentTraits(false).build(), new Context()), "Skipping test as the store does not implement required trait.");
assumeTrue(this.graph.execute(new HasTrait.Builder().trait(StoreTrait.PRE_AGGREGATION_FILTERING).currentTraits(false).build(), new Context()), "Skipping test as the store does not implement required trait.");
assumeTrue(this.graph.execute(new HasTrait.Builder().trait(StoreTrait.POST_AGGREGATION_FILTERING).currentTraits(false).build(), new Context()), "Skipping test as the store does not implement required trait.");
assertNotNull(this.graph, "graph is null");

final Date JAN_01_2000 = new SimpleDateFormat("yyyy-MM-dd").parse("2000-01-01");
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -43,9 +43,11 @@
import uk.gov.gchq.gaffer.operation.data.EntitySeed;
import uk.gov.gchq.gaffer.operation.impl.add.AddElements;
import uk.gov.gchq.gaffer.operation.io.Output;
import uk.gov.gchq.gaffer.store.Context;
import uk.gov.gchq.gaffer.store.StoreProperties;
import uk.gov.gchq.gaffer.store.StoreTrait;
import uk.gov.gchq.gaffer.store.TestTypes;
import uk.gov.gchq.gaffer.store.operation.HasTrait;
import uk.gov.gchq.gaffer.store.schema.Schema;
import uk.gov.gchq.gaffer.store.schema.SchemaEdgeDefinition;
import uk.gov.gchq.gaffer.store.schema.SchemaEntityDefinition;
Expand Down Expand Up @@ -212,7 +214,7 @@ protected void validateTest() throws Exception {
}
}

protected void validateTraits() {
protected void validateTraits() throws OperationException {
final Collection<StoreTrait> requiredTraits = new ArrayList<>();
for (final Annotation annotation : method.getDeclaredAnnotations()) {
if (annotation.annotationType().equals(TraitRequirement.class)) {
Expand All @@ -222,7 +224,7 @@ protected void validateTraits() {
}

for (final StoreTrait requiredTrait : requiredTraits) {
assumeThat(graph.hasTrait(requiredTrait)).as("Skipping test as the store does not implement all required traits.").isTrue();
assumeThat(graph.execute(new HasTrait.Builder().trait(requiredTrait).currentTraits(false).build(), new Context())).as("Skipping test as the store does not implement all required traits.").isTrue();
}
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -66,6 +66,7 @@
import uk.gov.gchq.gaffer.store.StoreException;
import uk.gov.gchq.gaffer.store.StoreTrait;
import uk.gov.gchq.gaffer.store.TestTypes;
import uk.gov.gchq.gaffer.store.operation.HasTrait;
import uk.gov.gchq.gaffer.store.operation.handler.OperationHandler;
import uk.gov.gchq.gaffer.store.operation.handler.generate.GenerateElementsHandler;
import uk.gov.gchq.gaffer.store.operation.handler.generate.GenerateObjectsHandler;
Expand Down Expand Up @@ -163,9 +164,9 @@ public void shouldCreateAStoreUsingGraphIdWithNamespace() throws Exception {
}

@Test
public void shouldBeAnOrderedStore() {
assertTrue(BYTE_ENTITY_STORE.hasTrait(StoreTrait.ORDERED));
assertTrue(GAFFER_1_KEY_STORE.hasTrait(StoreTrait.ORDERED));
public void shouldBeAnOrderedStore() throws OperationException {
assertTrue(BYTE_ENTITY_STORE.execute(new HasTrait.Builder().trait(StoreTrait.ORDERED).currentTraits(false).build(), new Context()));
assertTrue(GAFFER_1_KEY_STORE.execute(new HasTrait.Builder().trait(StoreTrait.ORDERED).currentTraits(false).build(), new Context()));
}

@Test
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -78,7 +78,8 @@ private void validateAllGraphsIdViews(final Operation op, final User user, final
if (graphIdValid) {
currentResult = new ValidationResult();
clonedOp.addOption(FederatedStoreConstants.KEY_OPERATION_OPTIONS_GRAPH_IDS, graphId);
if (!graph.hasTrait(StoreTrait.DYNAMIC_SCHEMA)) {
// Deprecated function still in use due to Federated GetTraits bug with DYNAMIC_SCHEMA
if (!graph.getStoreTraits().contains(StoreTrait.DYNAMIC_SCHEMA)) {
t92549 marked this conversation as resolved.
Show resolved Hide resolved
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note: This should be fixed by #2580

super.validateViews(clonedOp, user, store, currentResult);
}
if (currentResult.isValid()) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -130,7 +130,8 @@ public static <OP extends Operation> OP updateOperationForGraph(final OP operati
// then clone the operation and add the new view.
if (validView.hasGroups()) {
((OperationView) resultOp).setView(validView);
} else if (!graph.hasTrait(StoreTrait.DYNAMIC_SCHEMA)) {
// Deprecated function still in use due to Federated GetTraits bug with DYNAMIC_SCHEMA
t92549 marked this conversation as resolved.
Show resolved Hide resolved
} else if (!graph.getStoreTraits().contains(StoreTrait.DYNAMIC_SCHEMA)) {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note: This should be fixed by #2580

// The view has no groups so the operation would return
// nothing, so we shouldn't execute the operation.
resultOp = null;
Expand Down
Loading