From 55cf1f7381a422f4543dfe8cc342b1fbf9b921f4 Mon Sep 17 00:00:00 2001 From: tb06904 <141412860+tb06904@users.noreply.github.com> Date: Thu, 21 Mar 2024 11:23:23 +0000 Subject: [PATCH] Gh-3149: Add depth limit to NamedOperationResolver (#3174) * add depth limit to name op resolver * Tweak functionality for resolver * update and add limit test * update instance check to operation chains * throw exception if any unresolved named ops * add test for configurable limit --------- Co-authored-by: rj77259 <141829236+rj77259@users.noreply.github.com> Co-authored-by: GCHQDeveloper314 <94527357+GCHQDeveloper314@users.noreply.github.com> --- .../graph/hook/NamedOperationResolver.java | 157 +++++----- .../hook/NamedOperationResolverTest.java | 275 +++++++++++++----- 2 files changed, 282 insertions(+), 150 deletions(-) diff --git a/core/graph/src/main/java/uk/gov/gchq/gaffer/graph/hook/NamedOperationResolver.java b/core/graph/src/main/java/uk/gov/gchq/gaffer/graph/hook/NamedOperationResolver.java index 16eb22bfbaa..0c8a98f9741 100644 --- a/core/graph/src/main/java/uk/gov/gchq/gaffer/graph/hook/NamedOperationResolver.java +++ b/core/graph/src/main/java/uk/gov/gchq/gaffer/graph/hook/NamedOperationResolver.java @@ -28,7 +28,6 @@ import uk.gov.gchq.gaffer.named.operation.NamedOperation; import uk.gov.gchq.gaffer.operation.Operation; import uk.gov.gchq.gaffer.operation.OperationChain; -import uk.gov.gchq.gaffer.operation.Operations; import uk.gov.gchq.gaffer.store.Context; import uk.gov.gchq.gaffer.store.operation.handler.named.cache.NamedOperationCache; import uk.gov.gchq.gaffer.store.operation.handler.util.OperationHandlerUtil; @@ -37,11 +36,6 @@ import java.util.ArrayList; import java.util.Collection; import java.util.Collections; -import java.util.List; -import java.util.concurrent.CompletableFuture; -import java.util.concurrent.ExecutionException; -import java.util.concurrent.TimeUnit; -import java.util.concurrent.TimeoutException; /** * A {@link GraphHook} to resolve named operations. @@ -49,37 +43,32 @@ @JsonPropertyOrder(alphabetic = true) public class NamedOperationResolver implements GetFromCacheHook { private static final Logger LOGGER = LoggerFactory.getLogger(NamedOperationResolver.class); - // TODO: Possibly implement a depth limit rather than a hard timeout - public static final int TIMEOUT_DEFAULT = 1; - public static final TimeUnit TIME_UNIT_DEFAULT = TimeUnit.MINUTES; - private final NamedOperationCache cache; - private final int timeout; - private final TimeUnit timeUnit; + /** + * Default depth the resolver will go when checking for nested named operations + */ + public static final int DEPTH_LIMIT_DEFAULT = 3; - public NamedOperationResolver(final String suffixNamedOperationCacheName) { - this(suffixNamedOperationCacheName, TIMEOUT_DEFAULT, TIME_UNIT_DEFAULT); - } + private final int depthLimit; + private final NamedOperationCache cache; @JsonCreator public NamedOperationResolver( @JsonProperty("suffixNamedOperationCacheName") final String suffixNamedOperationCacheName, - @JsonProperty("timeout") final int timeout, - @JsonProperty("timeUnit") final TimeUnit timeUnit) { - this(new NamedOperationCache(suffixNamedOperationCacheName), timeout, timeUnit); + @JsonProperty("depthLimit") final int depthLimit) { + this(new NamedOperationCache(suffixNamedOperationCacheName), depthLimit); + } + public NamedOperationResolver(final String suffixNamedOperationCacheName) { + this(suffixNamedOperationCacheName, DEPTH_LIMIT_DEFAULT); } public NamedOperationResolver(final NamedOperationCache cache) { - this(cache, TIMEOUT_DEFAULT, TIME_UNIT_DEFAULT); + this(cache, DEPTH_LIMIT_DEFAULT); } - public NamedOperationResolver( - final NamedOperationCache cache, - final int timeout, - final TimeUnit timeUnit) { + public NamedOperationResolver(final NamedOperationCache cache, final int depthLimit) { this.cache = cache; - this.timeout = timeout; - this.timeUnit = timeUnit; + this.depthLimit = depthLimit; } @JsonGetter("suffixNamedOperationCacheName") @@ -87,29 +76,16 @@ public String getSuffixCacheName() { return cache.getSuffixCacheName(); } - @JsonGetter("timeout") - public int getTimeout() { - return timeout; - } - - @JsonGetter("timeUnit") - public TimeUnit getTimeUnit() { - return timeUnit; + @JsonGetter("depthLimit") + public int getDepthLimit() { + return depthLimit; } @Override public void preExecute(final OperationChain opChain, final Context context) { - try { - LOGGER.info("Resolving Named Operations with timeout: " + timeout); - CompletableFuture.runAsync(() -> resolveNamedOperations(opChain, context.getUser())).get(timeout, timeUnit); - LOGGER.info("Finished Named Operation resolver"); - } catch (final ExecutionException | TimeoutException e) { - throw new GafferRuntimeException("ResolverTask did not complete: " + e.getMessage(), e); - } catch (final InterruptedException e) { - LOGGER.warn("Thread interrupted", e); - Thread.currentThread().interrupt(); - } - + // Resolve the named operations in the chain + // (depth is set to zero as this is first level of recursion when checking for nested named ops) + opChain.updateOperations(resolveNamedOperations(opChain, context.getUser(), 0)); } @Override @@ -122,42 +98,67 @@ public T onFailure(final T result, final OperationChain opChain, final Co return result; } - private void resolveNamedOperations(final Operations operations, final User user) { - final List updatedOperations = new ArrayList<>(); - - operations.getOperations().forEach(operation -> { - if (operation instanceof NamedOperation) { - updatedOperations.addAll(resolveNamedOperation((NamedOperation) operation, user)); - } else { - if (operation instanceof Operations) { - resolveNamedOperations(((Operations) operation), user); + /** + * Resolves any named operations from the cache. What is meant + * by 'resolved' is turning the named operations into their respective + * {@link OperationChain}s. Ensures any supplied {@link NamedOperation}s + * actually exist in the cache and contain their correct {@link Operation}s. + * Will also run recursively to a given depth limit to ensure any nested + * {@link NamedOperation}s are also resolved from the cache. + * + * @param operation {@link NamedOperation} or {@link OperationChain} to act on. + * @param user User for the cache access. + * @param depth Current recursive depth, will use limit set in class to + * continue or not. + * @return A list of resolved operations essentially flattened into plain + * Operations and OperationChains. + */ + private Collection resolveNamedOperations(final Operation operation, final User user, final int depth) { + final Collection updatedOperations = new ArrayList<>(); + LOGGER.debug("Current resolver depth is: {}", depth); + + // If a named operation resolve the operations within it + if (operation instanceof NamedOperation) { + NamedOperation namedOperation = (NamedOperation) operation; + LOGGER.debug("Resolving named operation called: {}", namedOperation.getOperationName()); + try { + // Get the chain for the named operation from the cache + final OperationChain namedOperationChain = cache + .getNamedOperation(namedOperation.getOperationName(), user) + .getOperationChain(namedOperation.getParameters()); + // Update the operation inputs and add operation chain to the updated list + OperationHandlerUtil.updateOperationInput(namedOperationChain, namedOperation.getInput()); + + // Run again to resolve any nested operations in the chain before adding + namedOperationChain.updateOperations(resolveNamedOperations(namedOperationChain, user, depth + 1)); + updatedOperations.add(namedOperationChain); + + } catch (final CacheOperationException e) { + LOGGER.error("Exception resolving NamedOperation within the cache: {}", e.getMessage()); + return Collections.singletonList(namedOperation); + } + // If given an operationChain resolve Operations inside it + } else if (operation instanceof OperationChain) { + LOGGER.debug("Resolving Operations in Operation Chain: {}", ((OperationChain) operation).getOperations()); + for (final Operation op : ((OperationChain) operation).getOperations()) { + // Resolve if haven't hit the depth limit for named operations yet + // Note only need to check the depth here as when checking for a nested named operation it will always be + // recursively passed as an operation chain + if (depth < depthLimit) { + updatedOperations.addAll(resolveNamedOperations(op, user, depth)); + } else { + // If a NamedOperation couldn't be resolved then error + if (op instanceof NamedOperation) { + LOGGER.error("Nested depth limit hit resolving NamedOperation: {}", ((NamedOperation) op).getOperationName()); + throw new GafferRuntimeException("NamedOperation Resolver hit nested depth limit of " + depthLimit); + } + updatedOperations.add(op); } - updatedOperations.add(operation); } - }); - - operations.updateOperations((Collection) updatedOperations); - } - - private List resolveNamedOperation(final NamedOperation namedOp, final User user) { - try { - // Get the named operation chain from the cache - final OperationChain namedOperationChain = cache - .getNamedOperation(namedOp.getOperationName(), user) - .getOperationChain(namedOp.getParameters()); - - OperationHandlerUtil.updateOperationInput(namedOperationChain, namedOp.getInput()); - - // Call resolveNamedOperations again to check there are no nested named operations - resolveNamedOperations(namedOperationChain, user); - return namedOperationChain.getOperations(); - } catch (final CacheOperationException e) { - // An Exception with the cache has occurred e.g. it was unable to find named operation - // and then simply returned the original operation chain with the unresolved NamedOperation. - - // The exception from cache would otherwise be lost, so capture it here and print to LOGS. - LOGGER.error("Exception resolving NamedOperation within the cache:{}", e.getMessage()); - return Collections.singletonList(namedOp); + // If just a plain operation then nothing to resolve + } else { + updatedOperations.add(operation); } + return updatedOperations; } } diff --git a/core/graph/src/test/java/uk/gov/gchq/gaffer/graph/hook/NamedOperationResolverTest.java b/core/graph/src/test/java/uk/gov/gchq/gaffer/graph/hook/NamedOperationResolverTest.java index 98eb6655bab..866e8c945b4 100644 --- a/core/graph/src/test/java/uk/gov/gchq/gaffer/graph/hook/NamedOperationResolverTest.java +++ b/core/graph/src/test/java/uk/gov/gchq/gaffer/graph/hook/NamedOperationResolverTest.java @@ -1,5 +1,5 @@ /* - * Copyright 2017-2023 Crown Copyright + * Copyright 2017-2024 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,7 +16,6 @@ package uk.gov.gchq.gaffer.graph.hook; -import com.google.common.collect.Maps; import org.junit.jupiter.api.Test; import org.junit.jupiter.api.extension.ExtendWith; import org.mockito.Mock; @@ -41,6 +40,7 @@ import java.util.ArrayList; import java.util.Arrays; +import java.util.HashMap; import java.util.List; import java.util.Map; @@ -51,45 +51,45 @@ import static org.mockito.Mockito.verify; @ExtendWith(MockitoExtension.class) -public class NamedOperationResolverTest extends GraphHookTest { +class NamedOperationResolverTest extends GraphHookTest { - public static final String SUFFIX_CACHE_NAME = "suffix"; + static final String SUFFIX_CACHE_NAME = "suffix"; - public NamedOperationResolverTest() { + NamedOperationResolverTest() { super(NamedOperationResolver.class); } @SuppressWarnings({"unchecked", "rawtypes"}) @Test - public void shouldResolveNamedOperation(@Mock final User user, - @Mock final NamedOperationCache cache, - @Mock final NamedOperationDetail extendedNamedOperation, - @Mock final GetAdjacentIds op1, - @Mock final GetElements op2, - @Mock final Iterable input) + void shouldResolveNamedOperation(@Mock final User user, + @Mock final NamedOperationCache cache, + @Mock final NamedOperationDetail namedOpDetail, + @Mock final GetAdjacentIds op1, + @Mock final GetElements op2, + @Mock final Iterable input) throws CacheOperationException { // Given final String opName = "opName"; final NamedOperationResolver resolver = new NamedOperationResolver(cache); - final OperationChain namedOperationOpChain = new OperationChain(Arrays.asList(op1, op2)); - final Map params = null; + final OperationChain testChain = new OperationChain(Arrays.asList(op1, op2)); + final List expectedResolvedChain = Arrays.asList(testChain); given(op1.getInput()).willReturn(null); - given(cache.getNamedOperation(opName, user)).willReturn(extendedNamedOperation); - given(extendedNamedOperation.getOperationChain(params)).willReturn(namedOperationOpChain); + given(cache.getNamedOperation(opName, user)).willReturn(namedOpDetail); + given(namedOpDetail.getOperationChain(null)).willReturn(testChain); final OperationChain opChain = new OperationChain.Builder() - .first(new NamedOperation.Builder<>() - .name(opName) - .input(input) - .build()) - .build(); + .first(new NamedOperation.Builder<>() + .name(opName) + .input(input) + .build()) + .build(); // When resolver.preExecute(opChain, new Context(user)); // Then - assertThat(opChain.getOperations()).isEqualTo(namedOperationOpChain.getOperations()); + assertThat(opChain.getOperations()).isEqualTo(expectedResolvedChain); verify(op1).setInput(input); verify(op2, never()).setInput(input); @@ -97,12 +97,12 @@ public void shouldResolveNamedOperation(@Mock final User user, @SuppressWarnings({"unchecked", "rawtypes"}) @Test - public void shouldResolveNestedNamedOperation(@Mock final User user, - @Mock final NamedOperationCache cache, - @Mock final NamedOperationDetail extendedNamedOperation, - @Mock final GetAdjacentIds op1, - @Mock final GetElements op2, - @Mock final Iterable input) + void shouldResolveNestedNamedOperation(@Mock final User user, + @Mock final NamedOperationCache cache, + @Mock final NamedOperationDetail namedOpDetail, + @Mock final GetAdjacentIds op1, + @Mock final GetElements op2, + @Mock final Iterable input) throws OperationException, CacheOperationException { // Given final String opName = "opName"; @@ -113,8 +113,8 @@ public void shouldResolveNestedNamedOperation(@Mock final User user, final Map params = null; given(op1.getInput()).willReturn(null); - given(cache.getNamedOperation(opName, user)).willReturn(extendedNamedOperation); - given(extendedNamedOperation.getOperationChain(params)).willReturn(namedOperationOpChain); + given(cache.getNamedOperation(opName, user)).willReturn(namedOpDetail); + given(namedOpDetail.getOperationChain(params)).willReturn(namedOperationOpChain); final OperationChain opChain = new OperationChain.Builder() .first(new OperationChain.Builder() @@ -129,7 +129,7 @@ public void shouldResolveNestedNamedOperation(@Mock final User user, resolver.preExecute(opChain, new Context(user)); // Then - assertThat(opChain.getOperations().size()).isEqualTo(1); + assertThat(opChain.getOperations()).hasSize(1); final OperationChain nestedOpChain = (OperationChain) opChain.getOperations().get(0); assertThat(nestedOpChain.getOperations()).isEqualTo(namedOperationOpChain.getOperations()); @@ -139,13 +139,142 @@ public void shouldResolveNestedNamedOperation(@Mock final User user, @SuppressWarnings({"unchecked", "rawtypes"}) @Test - public void shouldExecuteNamedOperationWithoutOverridingInput(@Mock final User user, - @Mock final NamedOperationCache cache, - @Mock final NamedOperationDetail extendedNamedOperation, - @Mock final GetAdjacentIds op1, - @Mock final GetElements op2, - @Mock final Iterable input, - @Mock final Iterable mockIterable) + void shouldFailToResolveNestedNamedOperationsOverDefaultLimit( + @Mock final User user, + @Mock final NamedOperationCache cache, + @Mock final NamedOperationDetail namedOp1Detail, + @Mock final NamedOperationDetail namedOp2Detail, + @Mock final NamedOperationDetail namedOp3Detail, + @Mock final GetAdjacentIds op1, + @Mock final GetElements op2, + @Mock final GetElements op3, + @Mock final Iterable input) throws CacheOperationException { + // Given + final String namedOp1Name = "namedOp1"; + final String namedOp2Name = "namedOp2"; + final String namedOp3Name = "namedOp3"; + final String namedOp4Name = "namedOp4"; + final NamedOperationResolver resolver = new NamedOperationResolver(cache); + + // Setup cache returns (we can ignore named op 4 as it wont be used due to the depth limit) + given(cache.getNamedOperation(namedOp1Name, user)).willReturn(namedOp1Detail); + given(cache.getNamedOperation(namedOp2Name, user)).willReturn(namedOp2Detail); + given(cache.getNamedOperation(namedOp3Name, user)).willReturn(namedOp3Detail); + + // Create named ops + NamedOperation namedOp1 = new NamedOperation.Builder<>() + .name(namedOp1Name) + .input(input) + .build(); + NamedOperation namedOp2 = new NamedOperation.Builder<>() + .name(namedOp2Name) + .input(input) + .build(); + NamedOperation namedOp3 = new NamedOperation.Builder<>() + .name(namedOp3Name) + .input(input) + .build(); + NamedOperation namedOp4 = new NamedOperation.Builder<>() + .name(namedOp4Name) + .input(input) + .build(); + // Set up named op returns so they're nested e.g. 4 is nested in 3, 3 in 2 and so on + given(namedOp1Detail.getOperationChain(null)).willReturn( + new OperationChain.Builder() + .first(op1) + .then(namedOp2) + .build()); + given(namedOp2Detail.getOperationChain(null)).willReturn( + new OperationChain.Builder() + .first(op2) + .then(namedOp3) + .build()); + given(namedOp3Detail.getOperationChain(null)).willReturn( + new OperationChain.Builder() + .first(op3) + .then(namedOp4) + .build()); + + // Create chain with named op 1 in to see if get resolved to the limit + final OperationChain opChain = new OperationChain.Builder() + .first(namedOp1) + .build(); + // When + assertThatExceptionOfType(GafferRuntimeException.class) + .isThrownBy(() -> resolver.preExecute(opChain, new Context(user))) + .withMessageContaining("NamedOperation Resolver hit nested depth limit"); + } + + @SuppressWarnings({"unchecked", "rawtypes"}) + @Test + void shouldAllowConfigurableResolverDepthLimit( + @Mock final User user, + @Mock final NamedOperationCache cache, + @Mock final NamedOperationDetail namedOp1Detail, + @Mock final NamedOperationDetail namedOp2Detail, + @Mock final GetAdjacentIds op1, + @Mock final GetElements op2, + @Mock final GetElements op3, + @Mock final Iterable input) throws CacheOperationException { + + // Given + final String namedOp1Name = "namedOp1"; + final String namedOp2Name = "namedOp2"; + final String namedOp3Name = "namedOp3"; + // Make a resolver with a stricter depth limit + final NamedOperationResolver resolverStrict = new NamedOperationResolver(cache, 2); + + // Setup cache returns + given(cache.getNamedOperation(namedOp1Name, user)).willReturn(namedOp1Detail); + given(cache.getNamedOperation(namedOp2Name, user)).willReturn(namedOp2Detail); + + // Create named ops + NamedOperation namedOp1 = new NamedOperation.Builder<>() + .name(namedOp1Name) + .input(input) + .build(); + NamedOperation namedOp2 = new NamedOperation.Builder<>() + .name(namedOp2Name) + .input(input) + .build(); + NamedOperation namedOp3 = new NamedOperation.Builder<>() + .name(namedOp3Name) + .input(input) + .build(); + + // Set up named op returns so they're nested e.g. 3 in 2, 2 in 1 + given(namedOp1Detail.getOperationChain(null)).willReturn( + new OperationChain.Builder() + .first(op1) + .then(namedOp2) + .build()); + given(namedOp2Detail.getOperationChain(null)).willReturn( + new OperationChain.Builder() + .first(op2) + .then(namedOp3) + .build()); + + // Create chains with named op 1 in to see if get resolved to the limit + final OperationChain opChainStrict = new OperationChain.Builder() + .first(namedOp1) + .build(); + + // When resolved using the stricter limit it should fail to resolve the chain + assertThatExceptionOfType(GafferRuntimeException.class) + .isThrownBy(() -> resolverStrict.preExecute(opChainStrict, new Context(user))) + .withMessageContaining("NamedOperation Resolver hit nested depth limit"); + } + + + @SuppressWarnings({"unchecked", "rawtypes"}) + @Test + void shouldExecuteNamedOperationWithoutOverridingInput(@Mock final User user, + @Mock final NamedOperationCache cache, + @Mock final NamedOperationDetail namedOpDetail, + @Mock final GetAdjacentIds op1, + @Mock final GetElements op2, + @Mock final Iterable input, + @Mock final Iterable mockIterable) throws OperationException, CacheOperationException { // Given final String opName = "opName"; @@ -155,8 +284,8 @@ public void shouldExecuteNamedOperationWithoutOverridingInput(@Mock final User u final Map params = null; given(op1.getInput()).willReturn(mockIterable); - given(cache.getNamedOperation(opName, user)).willReturn(extendedNamedOperation); - given(extendedNamedOperation.getOperationChain(params)).willReturn(namedOpChain); + given(cache.getNamedOperation(opName, user)).willReturn(namedOpDetail); + given(namedOpDetail.getOperationChain(params)).willReturn(namedOpChain); // When final OperationChain opChain = new OperationChain.Builder() @@ -168,21 +297,21 @@ public void shouldExecuteNamedOperationWithoutOverridingInput(@Mock final User u resolver.preExecute(opChain, new Context(user)); // Then - assertThat(opChain.getOperations().get(0)).isSameAs(op1); + assertThat(opChain.getOperations().get(0)).isSameAs(namedOpChain); verify(op1, never()).setInput(input); - assertThat(opChain.getOperations().get(1)).isSameAs(op2); verify(op2, never()).setInput(input); } + @SuppressWarnings({ "rawtypes" }) @Test - public void shouldResolveNamedOperationWithParameter(@Mock final User user, + void shouldResolveNamedOperationWithParameter(@Mock final User user, @Mock final NamedOperationCache cache) throws OperationException, CacheOperationException { // Given final String opName = "opName"; final NamedOperationResolver resolver = new NamedOperationResolver(cache); - final Map paramMap = Maps.newHashMap(); + final Map paramMap = new HashMap<>(); paramMap.put("param1", 1L); final ParameterDetail param = new ParameterDetail.Builder() @@ -190,11 +319,11 @@ public void shouldResolveNamedOperationWithParameter(@Mock final User user, .description("Limit param") .valueClass(Long.class) .build(); - final Map paramDetailMap = Maps.newHashMap(); + final Map paramDetailMap = new HashMap<>(); paramDetailMap.put("param1", param); // Make a real NamedOperationDetail with a parameter - final NamedOperationDetail extendedNamedOperation = new NamedOperationDetail.Builder() + final NamedOperationDetail namedOpDetail = new NamedOperationDetail.Builder() .operationName(opName) .description("standard operation") .operationChain("{ \"operations\": [ { \"class\":\"uk.gov.gchq.gaffer.operation.impl.get.GetAllElements\" }, " @@ -202,7 +331,7 @@ public void shouldResolveNamedOperationWithParameter(@Mock final User user, .parameters(paramDetailMap) .build(); - given(cache.getNamedOperation(opName, user)).willReturn(extendedNamedOperation); + given(cache.getNamedOperation(opName, user)).willReturn(namedOpDetail); final OperationChain opChain = new OperationChain.Builder() .first(new NamedOperation.Builder<>() @@ -215,21 +344,23 @@ public void shouldResolveNamedOperationWithParameter(@Mock final User user, resolver.preExecute(opChain, new Context(user)); // Then - assertThat(opChain.getOperations().get(0)).isInstanceOf(GetAllElements.class); - assertThat(opChain.getOperations().get(1)).isInstanceOf(Limit.class); + assertThat(opChain.getOperations().get(0)) + .isInstanceOf(OperationChain.class); + assertThat(((OperationChain) opChain.getOperations().get(0)).getOperations().get(0)).isInstanceOf(GetAllElements.class); + assertThat(((OperationChain) opChain.getOperations().get(0)).getOperations().get(1)).isInstanceOf(Limit.class); // Check the parameter has been inserted - assertThat(((Limit) opChain.getOperations().get(1)).getResultLimit()).isEqualTo(1L); + assertThat(((Limit) ((OperationChain) opChain.getOperations().get(0)).getOperations().get(1)).getResultLimit()).isEqualTo(1L); } @Test - public void shouldNotExecuteNamedOperationWithParameterOfWrongType(@Mock final User user, - @Mock final NamedOperationCache cache) + void shouldNotExecuteNamedOperationWithParameterOfWrongType(@Mock final User user, + @Mock final NamedOperationCache cache) throws OperationException, CacheOperationException { // Given final String opName = "opName"; final NamedOperationResolver resolver = new NamedOperationResolver(cache); - final Map paramMap = Maps.newHashMap(); + final Map paramMap = new HashMap<>(); // A parameter of the wrong type paramMap.put("param1", new ArrayList<>()); @@ -238,11 +369,11 @@ public void shouldNotExecuteNamedOperationWithParameterOfWrongType(@Mock final U .description("Limit param") .valueClass(Long.class) .build(); - final Map paramDetailMap = Maps.newHashMap(); + final Map paramDetailMap = new HashMap<>(); paramDetailMap.put("param1", param); // Make a real NamedOperationDetail with a parameter - final NamedOperationDetail extendedNamedOperation = new NamedOperationDetail.Builder() + final NamedOperationDetail namedOpDetail = new NamedOperationDetail.Builder() .operationName(opName) .description("standard operation") .operationChain("{ \"operations\": [ { \"class\":\"uk.gov.gchq.gaffer.operation.impl.get.GetAllElements\" }, " @@ -250,10 +381,10 @@ public void shouldNotExecuteNamedOperationWithParameterOfWrongType(@Mock final U .parameters(paramDetailMap) .build(); - given(cache.getNamedOperation(opName, user)).willReturn(extendedNamedOperation); + given(cache.getNamedOperation(opName, user)).willReturn(namedOpDetail); // When - assertThatExceptionOfType(GafferRuntimeException.class) + assertThatExceptionOfType(IllegalArgumentException.class) .isThrownBy(() -> resolver.preExecute(new OperationChain.Builder() .first(new NamedOperation.Builder<>() .name(opName) @@ -264,13 +395,13 @@ public void shouldNotExecuteNamedOperationWithParameterOfWrongType(@Mock final U } @Test - public void shouldNotExecuteNamedOperationWithWrongParameterName(@Mock final User user, - @Mock final NamedOperationCache cache) + void shouldNotExecuteNamedOperationWithWrongParameterName(@Mock final User user, + @Mock final NamedOperationCache cache) throws OperationException, CacheOperationException { // Given final String opName = "opName"; final NamedOperationResolver resolver = new NamedOperationResolver(cache); - final Map paramMap = Maps.newHashMap(); + final Map paramMap = new HashMap<>(); // A parameter with the wrong name paramMap.put("param2", 1L); @@ -279,11 +410,11 @@ public void shouldNotExecuteNamedOperationWithWrongParameterName(@Mock final Use .description("Limit param") .valueClass(Long.class) .build(); - final Map paramDetailMap = Maps.newHashMap(); + final Map paramDetailMap = new HashMap<>(); paramDetailMap.put("param1", param); // Make a real NamedOperationDetail with a parameter - final NamedOperationDetail extendedNamedOperation = new NamedOperationDetail.Builder() + final NamedOperationDetail namedOpDetail = new NamedOperationDetail.Builder() .operationName(opName) .description("standard operation") .operationChain("{ \"operations\": [ { \"class\":\"uk.gov.gchq.gaffer.operation.impl.get.GetAllElements\" }, " @@ -291,10 +422,10 @@ public void shouldNotExecuteNamedOperationWithWrongParameterName(@Mock final Use .parameters(paramDetailMap) .build(); - given(cache.getNamedOperation(opName, user)).willReturn(extendedNamedOperation); + given(cache.getNamedOperation(opName, user)).willReturn(namedOpDetail); // When - assertThatExceptionOfType(GafferRuntimeException.class) + assertThatExceptionOfType(IllegalArgumentException.class) .isThrownBy(() -> resolver.preExecute(new OperationChain.Builder() .first(new NamedOperation.Builder<>() .name(opName) @@ -305,25 +436,25 @@ public void shouldNotExecuteNamedOperationWithWrongParameterName(@Mock final Use } @Test - public void shouldNotExecuteNamedOperationWithMissingRequiredArg(@Mock final User user, - @Mock final NamedOperationCache cache) + void shouldNotExecuteNamedOperationWithMissingRequiredArg(@Mock final User user, + @Mock final NamedOperationCache cache) throws OperationException, CacheOperationException { // Given final String opName = "opName"; final NamedOperationResolver resolver = new NamedOperationResolver(cache); // Don't set any parameters - final Map paramMap = Maps.newHashMap(); + final Map paramMap = new HashMap<>(); final ParameterDetail param = new ParameterDetail.Builder() .description("Limit param") .valueClass(Long.class) .required(true) .build(); - final Map paramDetailMap = Maps.newHashMap(); + final Map paramDetailMap = new HashMap<>(); paramDetailMap.put("param1", param); // Make a real NamedOperationDetail with a parameter - final NamedOperationDetail extendedNamedOperation = new NamedOperationDetail.Builder() + final NamedOperationDetail namedOpDetail = new NamedOperationDetail.Builder() .operationName(opName) .description("standard operation") .operationChain("{ \"operations\": [ { \"class\":\"uk.gov.gchq.gaffer.operation.impl.get.GetAllElements\" }, " @@ -331,10 +462,10 @@ public void shouldNotExecuteNamedOperationWithMissingRequiredArg(@Mock final Use .parameters(paramDetailMap) .build(); - given(cache.getNamedOperation(opName, user)).willReturn(extendedNamedOperation); + given(cache.getNamedOperation(opName, user)).willReturn(namedOpDetail); // When - assertThatExceptionOfType(GafferRuntimeException.class) + assertThatExceptionOfType(IllegalArgumentException.class) .isThrownBy(() -> resolver.preExecute(new OperationChain.Builder() .first(new NamedOperation.Builder<>() .name(opName) @@ -346,11 +477,11 @@ public void shouldNotExecuteNamedOperationWithMissingRequiredArg(@Mock final Use @SuppressWarnings({"unchecked", "resource", "rawtypes"}) @Test - public void shouldReturnOperationsInParameters() { + void shouldReturnOperationsInParameters() { // Given final NamedOperation namedOperation = new NamedOperation(); final Operation operation = new GetElements(); - final Map paramMap = Maps.newHashMap(); + final Map paramMap = new HashMap<>(); paramMap.put("test param", operation); namedOperation.setParameters(paramMap);