You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@sling.apache.org by bd...@apache.org on 2020/06/09 14:22:06 UTC
[sling-org-apache-sling-graphql-core] branch master updated:
Restrict sling/ data fetcher name prefix to org.apache.sling.* class names
This is an automated email from the ASF dual-hosted git repository.
bdelacretaz 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 16c451c Restrict sling/ data fetcher name prefix to org.apache.sling.* class names
16c451c is described below
commit 16c451ccd8532ad4e00fd0d0d4a34a82a3f99176
Author: Bertrand Delacretaz <bd...@apache.org>
AuthorDate: Tue Jun 9 16:21:52 2020 +0200
Restrict sling/ data fetcher name prefix to org.apache.sling.* class names
---
README.md | 22 +++---
.../core/engine/SlingDataFetcherSelector.java | 24 ++++++
.../com/example/fetchers/DoNothingFetcher.java | 32 ++++++++
.../core/engine/GraphQLResourceQueryTest.java | 48 ++----------
.../engine/ScriptedDataFetcherProviderTest.java | 3 +-
.../engine/SlingDataFetcherNameValidationTest.java | 2 +
.../core/engine/SlingDataFetcherSelectorTest.java | 88 ++++++++++++++++++++++
.../apache/sling/graphql/core/mocks/TestUtil.java | 52 +++++++++++++
src/test/resources/test-schema.txt | 6 +-
9 files changed, 223 insertions(+), 54 deletions(-)
diff --git a/README.md b/README.md
index 4af0cb1..e135aac 100644
--- a/README.md
+++ b/README.md
@@ -56,14 +56,14 @@ The default provider makes an internal Sling request with for the current Resour
This allows the Sling script/servlet resolution mechanism and its script engines to be used to generate
schemas dynamically, taking request selectors into account.
-## DataFetcher selection with Schema Directives
+## SlingDataFetcher selection with Schema Directives
The GraphQL schemas used by this module can be enhanced using
[schema directives](http://spec.graphql.org/June2018/#sec-Language.Directives)
(see also the [Apollo docs](https://www.apollographql.com/docs/graphql-tools/schema-directives/) for how those work)
-that select specific `DataFetcher` to return the appropriate data.
+that select specific `SlingDataFetcher` services to return the appropriate data.
-A default `DataFetcher` is used for types and fields which have no such annotation.
+A default data fetcher is used for types and fields which have no such annotation.
Here's a simple example, the test code has more:
@@ -82,13 +82,17 @@ Here's a simple example, the test code has more:
farenheit: Int @fetcher(name:"test/pipe" options:"farenheit")
}
-For now, the names of those `DataFetcher`s are in the form
+The names of those `SlingDataFetcher` services are in the form
<namespace>/<name>
-Where `<namespace>` selects a source (OSGi service) of `DataFetcher` and `<name>`
-selects a specific fetcher from that source.
+In these names, The `sling` namespace is reserved for `SlingDataFetcher` services
+which hava Java package names that start with `org.apache.sling`.
+
+`SlingDataFetcher` services can also be provided by scripts. This is experimental for
+now, see the tests for more info.
+
+The `<options>` and `<source>` arguments of the directive can be used by the
+`SlingDataFetcher` services to influence their behavior.
+
-The `<options>` and `<source>` arguments of the directive are used by some of those
-`DataFetcher` according to their own specification. See this module's tests
-for examples.
diff --git a/src/main/java/org/apache/sling/graphql/core/engine/SlingDataFetcherSelector.java b/src/main/java/org/apache/sling/graphql/core/engine/SlingDataFetcherSelector.java
index e674110..a4666bc 100644
--- a/src/main/java/org/apache/sling/graphql/core/engine/SlingDataFetcherSelector.java
+++ b/src/main/java/org/apache/sling/graphql/core/engine/SlingDataFetcherSelector.java
@@ -37,6 +37,16 @@ import java.io.IOException;
@Component(service=SlingDataFetcherSelector.class)
public class SlingDataFetcherSelector {
+ /** Fetchers which have a name starting with this prefix must be
+ * under the {#link RESERVED_PACKAGE_PREFIX} package.
+ */
+ public static final String RESERVED_NAME_PREFIX = "sling/";
+
+ /** Package name prefix for fetchers which have names starting
+ * with the {#link RESERVED_NAME_PREFIX}.
+ */
+ public static final String RESERVED_PACKAGE_PREFIX = "org.apache.sling.";
+
private BundleContext bundleContext;
@Reference
@@ -66,10 +76,24 @@ public class SlingDataFetcherSelector {
throw new IOException(String.format("Got %d services for %s, expected just one", refs.length, filter));
}
result = (SlingDataFetcher<Object>)bundleContext.getService(refs[0]);
+ validateResult(name, result);
}
return result;
}
+ private void validateResult(String name, SlingDataFetcher<?> fetcher) throws IOException {
+ if(name.startsWith(RESERVED_NAME_PREFIX)) {
+ final String className = fetcher.getClass().getName();
+ if(!fetcher.getClass().getName().startsWith(RESERVED_PACKAGE_PREFIX)) {
+ throw new IOException(
+ String.format(
+ "Invalid SlingDataFetcher %s:"
+ + " fetcher names starting with '%s' are reserved for Apache Sling Java packages",
+ className, RESERVED_NAME_PREFIX));
+ }
+ }
+ }
+
/** @return a SlingDataFetcher, or null if none available. First tries to get an
* OSGi SlingDataFetcher service, and if not found tries to find a scripted SlingDataFetcher.
*/
diff --git a/src/test/java/com/example/fetchers/DoNothingFetcher.java b/src/test/java/com/example/fetchers/DoNothingFetcher.java
new file mode 100644
index 0000000..20a5f79
--- /dev/null
+++ b/src/test/java/com/example/fetchers/DoNothingFetcher.java
@@ -0,0 +1,32 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package com.example.fetchers;
+
+import org.apache.sling.graphql.api.SlingDataFetcher;
+import org.apache.sling.graphql.api.SlingDataFetcherEnvironment;
+
+/** SlingDataFetcher used to test the requirement for sling/* fetcher
+ * names to point to classes under the org.apache.sling package
+ */
+public class DoNothingFetcher implements SlingDataFetcher<Object> {
+ @Override
+ public Object get(SlingDataFetcherEnvironment e) throws Exception {
+ return null;
+ }
+}
\ No newline at end of file
diff --git a/src/test/java/org/apache/sling/graphql/core/engine/GraphQLResourceQueryTest.java b/src/test/java/org/apache/sling/graphql/core/engine/GraphQLResourceQueryTest.java
index acae5cc..09775d7 100644
--- a/src/test/java/org/apache/sling/graphql/core/engine/GraphQLResourceQueryTest.java
+++ b/src/test/java/org/apache/sling/graphql/core/engine/GraphQLResourceQueryTest.java
@@ -35,7 +35,6 @@ import java.util.UUID;
import org.apache.sling.api.resource.Resource;
import org.apache.sling.api.servlets.ServletResolver;
import org.apache.sling.graphql.api.SchemaProvider;
-import org.apache.sling.graphql.api.SlingDataFetcher;
import org.apache.sling.testing.mock.osgi.junit.OsgiContext;
import org.apache.sling.graphql.core.json.JsonSerializer;
import org.apache.sling.graphql.core.mocks.DigestDataFetcher;
@@ -43,6 +42,7 @@ import org.apache.sling.graphql.core.mocks.EchoDataFetcher;
import org.apache.sling.graphql.core.mocks.FailingDataFetcher;
import org.apache.sling.graphql.core.mocks.MockSchemaProvider;
import org.apache.sling.graphql.core.mocks.MockScriptServlet;
+import org.apache.sling.graphql.core.mocks.TestUtil;
import org.junit.Before;
import org.junit.Rule;
import org.junit.Test;
@@ -59,27 +59,6 @@ public class GraphQLResourceQueryTest {
@Rule
public final OsgiContext context = new OsgiContext();
- private void assertNestedException(Throwable t, Class<?> clazz, String messageContains) {
- boolean found = false;
- while(t != null) {
- if(t.getClass().equals(clazz) && t.getMessage().contains(messageContains)) {
- found = true;
- break;
- }
- t = t.getCause();
- }
- if(!found) {
- fail(String.format("Did not get %s exception with message containing '%s'",
- clazz.getName(), messageContains));
- }
- }
-
- private ServiceRegistration<?> registerSlingDataFetcher(String name, SlingDataFetcher<?> f) {
- final Dictionary<String, Object> props = new Hashtable<>();
- props.put(SlingDataFetcher.NAME_SERVICE_PROPERTY, name);
- return context.bundleContext().registerService(SlingDataFetcher.class, f, props);
- }
-
@Before
public void setup() {
schemaProvider = new MockSchemaProvider("test-schema");
@@ -92,11 +71,11 @@ public class GraphQLResourceQueryTest {
final Dictionary<String, Object> staticData = new Hashtable<>();
staticData.put("test", true);
- registerSlingDataFetcher("echoNS/echo", new EchoDataFetcher(null));
- registerSlingDataFetcher("failure/fail", new FailingDataFetcher());
- registerSlingDataFetcher("test/static", new EchoDataFetcher(staticData));
- registerSlingDataFetcher("test/fortyTwo", new EchoDataFetcher(42));
- registerSlingDataFetcher("test/digest", new DigestDataFetcher());
+ TestUtil.registerSlingDataFetcher(context.bundleContext(), "echoNS/echo", new EchoDataFetcher(null));
+ TestUtil.registerSlingDataFetcher(context.bundleContext(), "failure/fail", new FailingDataFetcher());
+ TestUtil.registerSlingDataFetcher(context.bundleContext(), "test/static", new EchoDataFetcher(staticData));
+ TestUtil.registerSlingDataFetcher(context.bundleContext(), "test/fortyTwo", new EchoDataFetcher(42));
+ TestUtil.registerSlingDataFetcher(context.bundleContext(), "sling/digest", new DigestDataFetcher());
// Our MockScriptServlet to simulates a script for unit tests, for the
// integration tests we use a real script
@@ -179,25 +158,14 @@ public class GraphQLResourceQueryTest {
}
@Test
- public void duplicateProviderTest() throws Exception {
- registerSlingDataFetcher("test/static", new EchoDataFetcher(42));
- try {
- queryJSON("{ currentResource { path } }", null);
- fail("Expected query to fail");
- } catch(Exception e) {
- assertNestedException(e, IOException.class, "expected just one");
- }
- }
-
- @Test
public void invalidFetcherNamesTest() throws Exception {
schemaProvider = new MockSchemaProvider("failing-schema");
- final ServiceRegistration<?> reg = registerSlingDataFetcher("missingSlash", new EchoDataFetcher(42));
+ final ServiceRegistration<?> reg = TestUtil.registerSlingDataFetcher(context.bundleContext(), "missingSlash", new EchoDataFetcher(42));
try {
queryJSON("{ currentResource { missingSlash } }", null);
fail("Expected query to fail");
} catch(Exception e) {
- assertNestedException(e, IOException.class, "does not match");
+ TestUtil.assertNestedException(e, IOException.class, "does not match");
} finally {
reg.unregister();
}
diff --git a/src/test/java/org/apache/sling/graphql/core/engine/ScriptedDataFetcherProviderTest.java b/src/test/java/org/apache/sling/graphql/core/engine/ScriptedDataFetcherProviderTest.java
index adbd3d9..b41bdb0 100644
--- a/src/test/java/org/apache/sling/graphql/core/engine/ScriptedDataFetcherProviderTest.java
+++ b/src/test/java/org/apache/sling/graphql/core/engine/ScriptedDataFetcherProviderTest.java
@@ -44,7 +44,7 @@ public class ScriptedDataFetcherProviderTest {
private ScriptedDataFetcherProvider provider;
private static final String FETCHER_NAME = "this/fetcher";
- private String scriptResult;
+
private ServletResolver servletResolver;
private Servlet servletOnly;
private Servlet servletAndScript;
@@ -55,7 +55,6 @@ public class ScriptedDataFetcherProviderTest {
@Before
public void setup() {
servletOnly = Mockito.mock(Servlet.class);
- scriptResult = "SCRIPT_RESULT_" + UUID.randomUUID();
servletAndScript = new MockScriptServlet();
servletResolver = Mockito.mock(ServletResolver.class);
diff --git a/src/test/java/org/apache/sling/graphql/core/engine/SlingDataFetcherNameValidationTest.java b/src/test/java/org/apache/sling/graphql/core/engine/SlingDataFetcherNameValidationTest.java
index 13b916e..bd14552 100644
--- a/src/test/java/org/apache/sling/graphql/core/engine/SlingDataFetcherNameValidationTest.java
+++ b/src/test/java/org/apache/sling/graphql/core/engine/SlingDataFetcherNameValidationTest.java
@@ -44,6 +44,8 @@ public class SlingDataFetcherNameValidationTest {
result.add(new Object[] {"uno/1/x42", true});
result.add(new Object[] {"uno_due/tre", true});
result.add(new Object[] {"the:colon/bad", false});
+ result.add(new Object[] {"/startingslash", false});
+ result.add(new Object[] {"/starting/ending", false});
return result;
}
diff --git a/src/test/java/org/apache/sling/graphql/core/engine/SlingDataFetcherSelectorTest.java b/src/test/java/org/apache/sling/graphql/core/engine/SlingDataFetcherSelectorTest.java
new file mode 100644
index 0000000..3795e02
--- /dev/null
+++ b/src/test/java/org/apache/sling/graphql/core/engine/SlingDataFetcherSelectorTest.java
@@ -0,0 +1,88 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.sling.graphql.core.engine;
+
+import static org.hamcrest.Matchers.not;
+import static org.hamcrest.Matchers.nullValue;
+import static org.junit.Assert.assertThat;
+import static org.junit.Assert.fail;
+
+import java.io.IOException;
+
+import com.example.fetchers.DoNothingFetcher;
+
+import org.apache.sling.graphql.api.SlingDataFetcher;
+import org.apache.sling.graphql.core.mocks.DigestDataFetcher;
+import org.apache.sling.graphql.core.mocks.EchoDataFetcher;
+import org.apache.sling.graphql.core.mocks.TestUtil;
+import org.apache.sling.testing.mock.osgi.junit.OsgiContext;
+import org.junit.Before;
+import org.junit.Rule;
+import org.junit.Test;
+import org.mockito.Mockito;
+
+public class SlingDataFetcherSelectorTest {
+
+ @Rule
+ public final OsgiContext context = new OsgiContext();
+
+ private SlingDataFetcherSelector selector;
+
+ @Before
+ public void setup() {
+ final ScriptedDataFetcherProvider sdfp = Mockito.mock(ScriptedDataFetcherProvider.class);
+ context.bundleContext().registerService(ScriptedDataFetcherProvider.class, sdfp, null);
+ context.registerInjectActivateService(new SlingDataFetcherSelector());
+ selector = context.getService(SlingDataFetcherSelector.class);
+
+ TestUtil.registerSlingDataFetcher(context.bundleContext(), "sling/digest", new DigestDataFetcher());
+ TestUtil.registerSlingDataFetcher(context.bundleContext(), "sling/shouldFail", new DoNothingFetcher());
+ TestUtil.registerSlingDataFetcher(context.bundleContext(), "example/ok", new DoNothingFetcher());
+ TestUtil.registerSlingDataFetcher(context.bundleContext(), "sling/duplicate", new EchoDataFetcher(451));
+ TestUtil.registerSlingDataFetcher(context.bundleContext(), "sling/duplicate", new DigestDataFetcher());
+ }
+
+ @Test
+ public void acceptableName() throws Exception {
+ final SlingDataFetcher<Object> sdf = selector.getSlingFetcher("example/ok");
+ assertThat(sdf, not(nullValue()));
+ }
+
+ @Test
+ public void reservedNameOk() throws Exception {
+ final SlingDataFetcher<Object> sdf = selector.getSlingFetcher("sling/digest");
+ assertThat(sdf, not(nullValue()));
+ }
+
+ @Test
+ public void reservedNameError() throws Exception {
+ try {
+ selector.getSlingFetcher("sling/shouldFail");
+ fail("Expected getSlingFetcher to fail");
+ } catch(Exception e) {
+ TestUtil.assertNestedException(e, IOException.class, DoNothingFetcher.class.getName());
+ TestUtil.assertNestedException(e, IOException.class, "starting with 'sling/' are reserved for Apache Sling");
+ }
+ }
+
+ @Test(expected=IOException.class)
+ public void duplicateFetcherError() throws Exception {
+ final SlingDataFetcher<Object> sdf = selector.getSlingFetcher("sling/duplicate");
+ }
+}
\ No newline at end of file
diff --git a/src/test/java/org/apache/sling/graphql/core/mocks/TestUtil.java b/src/test/java/org/apache/sling/graphql/core/mocks/TestUtil.java
new file mode 100644
index 0000000..7679108
--- /dev/null
+++ b/src/test/java/org/apache/sling/graphql/core/mocks/TestUtil.java
@@ -0,0 +1,52 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package org.apache.sling.graphql.core.mocks;
+
+import static org.junit.Assert.fail;
+
+import java.util.Dictionary;
+import java.util.Hashtable;
+
+import org.apache.sling.graphql.api.SlingDataFetcher;
+import org.osgi.framework.BundleContext;
+import org.osgi.framework.ServiceRegistration;
+
+public class TestUtil {
+ public static ServiceRegistration<?> registerSlingDataFetcher(BundleContext bc, String name, SlingDataFetcher<?> f) {
+ final Dictionary<String, Object> props = new Hashtable<>();
+ props.put(SlingDataFetcher.NAME_SERVICE_PROPERTY, name);
+ return bc.registerService(SlingDataFetcher.class, f, props);
+ }
+
+ public static void assertNestedException(Throwable t, Class<?> clazz, String messageContains) {
+ boolean found = false;
+ while(t != null) {
+ if(t.getClass().equals(clazz) && t.getMessage().contains(messageContains)) {
+ found = true;
+ break;
+ }
+ t = t.getCause();
+ }
+ if(!found) {
+ fail(String.format("Did not get %s exception with message containing '%s'",
+ clazz.getName(), messageContains));
+ }
+ }
+}
\ No newline at end of file
diff --git a/src/test/resources/test-schema.txt b/src/test/resources/test-schema.txt
index 37d47e9..0bcaa5d 100644
--- a/src/test/resources/test-schema.txt
+++ b/src/test/resources/test-schema.txt
@@ -39,13 +39,13 @@ type SlingResource {
path: String
resourceType: String
- pathMD5: String @fetcher(name:"test/digest" options:"md5" source:"path")
+ pathMD5: String @fetcher(name:"sling/digest" options:"md5" source:"path")
# SHA256 digest of the path
- pathSHA256: String @fetcher(name:"test/digest" options:"sha-256" source:"path")
+ pathSHA256: String @fetcher(name:"sling/digest" options:"sha-256" source:"path")
# MD5 digest of the resource type
- resourceTypeMD5: String @fetcher(name:"test/digest" options:"md5" source:"resourceType")
+ resourceTypeMD5: String @fetcher(name:"sling/digest" options:"md5" source:"resourceType")
nullValue: String @fetcher(name:"echoNS/echo" options:"null")