You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@sling.apache.org by ra...@apache.org on 2020/09/22 14:28:11 UTC
[sling-org-apache-sling-graphql-core] branch master updated:
SLING-9753 - Return a 500 status code if the persisted query cannot be
stored in the cache
This is an automated email from the ASF dual-hosted git repository.
radu pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/sling-org-apache-sling-graphql-core.git
The following commit(s) were added to refs/heads/master by this push:
new 8301d5c SLING-9753 - Return a 500 status code if the persisted query cannot be stored in the cache
8301d5c is described below
commit 8301d5c41071e403c3f4516662e8f90170ca9b96
Author: Radu Cotescu <co...@adobe.com>
AuthorDate: Tue Sep 22 16:27:45 2020 +0200
SLING-9753 - Return a 500 status code if the persisted query cannot be stored in the cache
* changed the API for the GraphQLCacheProvider so that the cacheQuery method
can return a null for cache storing failures
* updated tests
---
pom.xml | 10 +++--
.../graphql/api/cache/GraphQLCacheProvider.java | 7 +--
.../core/cache/SimpleGraphQLCacheProvider.java | 38 ++++++++++++++--
.../sling/graphql/core/servlet/GraphQLServlet.java | 8 +++-
.../core/cache/SimpleGraphQLCacheProviderTest.java | 8 ++++
.../graphql/core/servlet/GraphQLServletTest.java | 50 ++++++++++++++++++++++
6 files changed, 108 insertions(+), 13 deletions(-)
diff --git a/pom.xml b/pom.xml
index eb79816..4684bc1 100644
--- a/pom.xml
+++ b/pom.xml
@@ -40,8 +40,10 @@
<org.ops4j.pax.exam.version>4.13.3</org.ops4j.pax.exam.version>
- <!-- To debug the pax process, override this with -D -->
- <pax.vm.options>-Xmx512M</pax.vm.options>
+ <argLine/>
+
+ <!-- this default setting allows collecting coverage info during IT -->
+ <pax.vm.options>${argLine}</pax.vm.options>
</properties>
<scm>
@@ -228,8 +230,8 @@
</dependency>
<dependency>
<groupId>org.mockito</groupId>
- <artifactId>mockito-all</artifactId>
- <version>1.10.19</version>
+ <artifactId>mockito-inline</artifactId>
+ <version>3.5.11</version>
<scope>test</scope>
</dependency>
<dependency>
diff --git a/src/main/java/org/apache/sling/graphql/api/cache/GraphQLCacheProvider.java b/src/main/java/org/apache/sling/graphql/api/cache/GraphQLCacheProvider.java
index bab13e0..7b43a94 100644
--- a/src/main/java/org/apache/sling/graphql/api/cache/GraphQLCacheProvider.java
+++ b/src/main/java/org/apache/sling/graphql/api/cache/GraphQLCacheProvider.java
@@ -42,13 +42,14 @@ public interface GraphQLCacheProvider {
@Nullable String getQuery(@NotNull String hash, @NotNull String resourceType, @Nullable String selectorString);
/**
- * Stores the {@code query} into the cache, potentially overriding a previous value.
+ * Stores the {@code query} into the cache, potentially overriding a previous value. The method will return the query's SHA-256 hash
+ * if the persisting operation was successful. If not, a {@code null} value must be returned.
*
* @param query the GraphQL query
* @param resourceType the resource type of the {@link org.apache.sling.graphql.core.servlet.GraphQLServlet} which will execute the
* query, since multiple servlets can be registered
* @param selectorString the selector string with which the {@link org.apache.sling.graphql.core.servlet.GraphQLServlet} is registered
- * @return the query's SHA-256 hash, which will be passed to the GraphQL client for query retrieval
+ * @return the query's SHA-256 hash, if the query was successfully cached; {@code null} if the query could not be cached
*/
- @NotNull String cacheQuery(@NotNull String query, @NotNull String resourceType, @Nullable String selectorString);
+ @Nullable String cacheQuery(@NotNull String query, @NotNull String resourceType, @Nullable String selectorString);
}
diff --git a/src/main/java/org/apache/sling/graphql/core/cache/SimpleGraphQLCacheProvider.java b/src/main/java/org/apache/sling/graphql/core/cache/SimpleGraphQLCacheProvider.java
index 7fe1f59..5b4a7b7 100644
--- a/src/main/java/org/apache/sling/graphql/core/cache/SimpleGraphQLCacheProvider.java
+++ b/src/main/java/org/apache/sling/graphql/core/cache/SimpleGraphQLCacheProvider.java
@@ -26,6 +26,7 @@ import java.util.HashSet;
import java.util.LinkedHashMap;
import java.util.Map;
import java.util.Objects;
+import java.util.Optional;
import java.util.Set;
import java.util.concurrent.locks.Lock;
import java.util.concurrent.locks.ReadWriteLock;
@@ -152,14 +153,17 @@ public class SimpleGraphQLCacheProvider implements GraphQLCacheProvider {
}
@Override
- @NotNull
+ @Nullable
public String cacheQuery(@NotNull String query, @NotNull String resourceType, @Nullable String selectorString) {
writeLock.lock();
try {
String hash = getHash(query);
String key = getCacheKey(hash, resourceType, selectorString);
persistedQueriesCache.put(key, query);
- return hash;
+ if (persistedQueriesCache.containsKey(key)) {
+ return hash;
+ }
+ return null;
} finally {
writeLock.unlock();
}
@@ -227,8 +231,34 @@ public class SimpleGraphQLCacheProvider implements GraphQLCacheProvider {
@Override
public String put(String key, String value) {
- currentSizeInBytes += getApproximateStringSizeInBytes(value);
- return super.put(key, value);
+ long valueSize = getApproximateStringSizeInBytes(value);
+ if (capacity <= 0 && maxSizeInBytes > 0) {
+ long newSizeInBytes;
+ boolean isReplacement = containsKey(key);
+ if (isReplacement) {
+ long oldValueSize = getApproximateStringSizeInBytes(get(key));
+ newSizeInBytes = currentSizeInBytes - oldValueSize + valueSize;
+ } else {
+ // calculate what happens after removing LRU
+ newSizeInBytes = currentSizeInBytes + valueSize;
+ Optional<String> head = this.values().stream().findFirst();
+ if (head.isPresent()) {
+ newSizeInBytes -= getApproximateStringSizeInBytes(head.get());
+ }
+ }
+ if (newSizeInBytes <= maxSizeInBytes) {
+ if (isReplacement) {
+ currentSizeInBytes = newSizeInBytes;
+ } else {
+ currentSizeInBytes += valueSize;
+ }
+ return super.put(key, value);
+ }
+ } else {
+ currentSizeInBytes += valueSize;
+ return super.put(key, value);
+ }
+ return null;
}
int getApproximateStringSizeInBytes(@NotNull String string) {
diff --git a/src/main/java/org/apache/sling/graphql/core/servlet/GraphQLServlet.java b/src/main/java/org/apache/sling/graphql/core/servlet/GraphQLServlet.java
index 2797906..7943bb8 100644
--- a/src/main/java/org/apache/sling/graphql/core/servlet/GraphQLServlet.java
+++ b/src/main/java/org/apache/sling/graphql/core/servlet/GraphQLServlet.java
@@ -284,8 +284,12 @@ public class GraphQLServlet extends SlingAllMethodsServlet {
String hash = cacheProvider.cacheQuery(rawQuery, request.getResource().getResourceType(),
request.getRequestPathInfo().getSelectorString());
- response.addHeader("Location", getLocationHeaderValue(request, hash));
- response.setStatus(HttpServletResponse.SC_CREATED);
+ if (hash != null) {
+ response.addHeader("Location", getLocationHeaderValue(request, hash));
+ response.setStatus(HttpServletResponse.SC_CREATED);
+ } else {
+ response.sendError(HttpServletResponse.SC_INTERNAL_SERVER_ERROR, "Cannot store persisted query.");
+ }
} else {
response.sendError(HttpServletResponse.SC_BAD_REQUEST, "Invalid GraphQL query.");
}
diff --git a/src/test/java/org/apache/sling/graphql/core/cache/SimpleGraphQLCacheProviderTest.java b/src/test/java/org/apache/sling/graphql/core/cache/SimpleGraphQLCacheProviderTest.java
index f0c4f5d..b42eaac 100644
--- a/src/test/java/org/apache/sling/graphql/core/cache/SimpleGraphQLCacheProviderTest.java
+++ b/src/test/java/org/apache/sling/graphql/core/cache/SimpleGraphQLCacheProviderTest.java
@@ -77,6 +77,14 @@ public class SimpleGraphQLCacheProviderTest {
// but b should still be there
assertEquals("b", provider.getQuery(bHash, "a/b/c", null));
+
+ // attempt a replacement of b
+ assertEquals(bHash, provider.cacheQuery("b", "a/b/c", null));
+ assertEquals("b", provider.getQuery(bHash, "a/b/c", null));
+
+ // and this value should not be stored since it's over the cache's limit
+ String abHash = provider.cacheQuery("ab", "a/b/c", null);
+ assertNull(abHash);
}
@Test
diff --git a/src/test/java/org/apache/sling/graphql/core/servlet/GraphQLServletTest.java b/src/test/java/org/apache/sling/graphql/core/servlet/GraphQLServletTest.java
index 8ea6265..6703d28 100644
--- a/src/test/java/org/apache/sling/graphql/core/servlet/GraphQLServletTest.java
+++ b/src/test/java/org/apache/sling/graphql/core/servlet/GraphQLServletTest.java
@@ -19,6 +19,8 @@
package org.apache.sling.graphql.core.servlet;
import java.io.IOException;
+import java.nio.charset.StandardCharsets;
+import java.util.Map;
import javax.servlet.Servlet;
@@ -28,7 +30,9 @@ import org.apache.sling.api.servlets.ServletResolverConstants;
import org.apache.sling.commons.metrics.Counter;
import org.apache.sling.commons.metrics.MetricsService;
import org.apache.sling.commons.metrics.Timer;
+import org.apache.sling.graphql.api.SchemaProvider;
import org.apache.sling.graphql.core.cache.SimpleGraphQLCacheProvider;
+import org.apache.sling.graphql.core.engine.GraphQLResourceQuery;
import org.apache.sling.graphql.core.engine.SlingDataFetcherSelector;
import org.apache.sling.graphql.core.scalars.SlingScalarsProvider;
import org.apache.sling.graphql.core.schema.RankedSchemaProviders;
@@ -39,15 +43,21 @@ import org.apache.sling.testing.mock.sling.servlet.MockSlingHttpServletResponse;
import org.junit.Before;
import org.junit.Rule;
import org.junit.Test;
+import org.junit.runner.RunWith;
+import org.mockito.MockedStatic;
+import org.mockito.junit.MockitoJUnitRunner;
import com.codahale.metrics.MetricRegistry;
import static org.junit.Assert.assertEquals;
import static org.junit.Assert.assertNotNull;
+import static org.mockito.ArgumentMatchers.any;
import static org.mockito.Matchers.anyString;
import static org.mockito.Mockito.mock;
+import static org.mockito.Mockito.mockStatic;
import static org.mockito.Mockito.when;
+@RunWith(MockitoJUnitRunner.class)
public class GraphQLServletTest {
@Rule
@@ -68,6 +78,46 @@ public class GraphQLServletTest {
}
@Test
+ public void testCachingErrors() throws IOException {
+ try (MockedStatic<GraphQLResourceQuery> graphQLResourceQueryMockedStatic = mockStatic(GraphQLResourceQuery.class)) {
+ graphQLResourceQueryMockedStatic.when(() -> GraphQLResourceQuery.isQueryValid(any(SchemaProvider.class),
+ any(SlingDataFetcherSelector.class), any(SlingScalarsProvider.class), any(Resource.class), any(String[].class),
+ anyString(), any(Map.class))).thenReturn(true);
+ RankedSchemaProviders rankedSchemaProviders = mock(RankedSchemaProviders.class);
+ context.registerService(rankedSchemaProviders);
+ SlingDataFetcherSelector slingDataFetcherSelector = mock(SlingDataFetcherSelector.class);
+ context.registerService(slingDataFetcherSelector);
+ SlingScalarsProvider slingScalarsProvider = mock(SlingScalarsProvider.class);
+ context.registerService(slingScalarsProvider);
+
+ context.registerInjectActivateService(new SimpleGraphQLCacheProvider(), "maxSize", 10);
+
+ context.registerInjectActivateService(new GraphQLServlet(), ServletResolverConstants.SLING_SERVLET_RESOURCE_TYPES, "a/b/c",
+ "persistedQueries.suffix", "/persisted");
+ GraphQLServlet servlet = (GraphQLServlet) context.getService(Servlet.class);
+ assertNotNull(servlet);
+
+ context.build().resource("/content/graphql", ResourceResolver.PROPERTY_RESOURCE_TYPE, "a/b/c").commit();
+ Resource resource = context.resourceResolver().resolve("/content/graphql");
+
+ MockSlingHttpServletResponse response = context.response();
+ MockSlingHttpServletRequest request = new MockSlingHttpServletRequest(context.bundleContext());
+ request.setMethod("POST");
+ request.setContent("{\"query\": \"{ currentResource { resourceType name } }\" }".getBytes(StandardCharsets.UTF_8));
+
+ request.setResource(resource);
+ MockRequestPathInfo requestPathInfo = (MockRequestPathInfo) request.getRequestPathInfo();
+ requestPathInfo.setExtension("gql");
+ requestPathInfo.setResourcePath(resource.getPath());
+ requestPathInfo.setSuffix("/persisted");
+
+ servlet.doPost(request, response);
+
+ assertEquals(500, response.getStatus());
+ }
+ }
+
+ @Test
public void testDisabledSuffix() throws IOException {
RankedSchemaProviders rankedSchemaProviders = mock(RankedSchemaProviders.class);
context.registerService(rankedSchemaProviders);