keith-turner commented on code in PR #55:
URL:
https://github.com/apache/accumulo-classloaders/pull/55#discussion_r2748570780
##########
modules/local-caching-classloader/src/test/java/org/apache/accumulo/classloader/lcc/LocalCachingContextClassLoaderFactoryTest.java:
##########
@@ -168,11 +169,53 @@ public static void afterAll() throws Exception {
public void beforeEach() throws Exception {
baseCacheDir = tempDir.resolve("base");
ConfigurationCopy acuConf = new ConfigurationCopy(
- Map.of(CACHE_DIR_PROPERTY,
baseCacheDir.toAbsolutePath().toUri().toURL().toExternalForm()));
+ Map.of(CACHE_DIR_PROPERTY,
baseCacheDir.toAbsolutePath().toUri().toURL().toExternalForm(),
+ UPDATE_FAILURE_GRACE_PERIOD_MINS, "1", ALLOWED_URLS_PATTERN,
".*"));
FACTORY = new LocalCachingContextClassLoaderFactory();
FACTORY.init(() -> new ConfigurationImpl(acuConf));
}
+ @Test
+ public void testAllowedUrls() throws Exception {
+ // use a different factory than other tests; only allow file: URLs
+ ConfigurationCopy acuConf = new ConfigurationCopy(
+ Map.of(CACHE_DIR_PROPERTY,
baseCacheDir.toAbsolutePath().toUri().toURL().toExternalForm(),
+ ALLOWED_URLS_PATTERN, "file:.*"));
+ var factory = new LocalCachingContextClassLoaderFactory();
+ factory.init(() -> new ConfigurationImpl(acuConf));
+
+ // case 1: all URLs pass (normal case, covered by other tests)
+
+ // case 2: context definition URL fails to match the pattern
+ var ex = assertThrows(ContextClassLoaderException.class,
+ () -> factory.getClassLoader(hdfsAllContext.toExternalForm()));
+ assertTrue(ex.getCause() instanceof IllegalArgumentException);
+ assertTrue(ex.getCause().getMessage().contains("Context definition
location (hdfs:"));
+
+ // case 3a: context definition URL matches, but resource URL should fail
to match the pattern,
+ // but it works anyway, because the resources were downloaded already by a
different instance
+ // (in this case, by a less restrictive FACTORY instance) and no new
connection is made
+ FACTORY.getClassLoader(hdfsAllContext.toExternalForm());
+ factory.getClassLoader(localAllContext.toExternalForm()); // same resources
+
+ // case 3b: context definition URL matches, but resource URL fails to
match the pattern
+ // in this case, we use a new context definition, with a resource that
doesn't exist locally
+ var newResources = new LinkedHashSet<Resource>();
+ var badUrl = "http://localhost/some/path";;
+ newResources.add(new Resource(new URL(badUrl), "MD5", BAD_MD5));
+ var context2 = new ContextDefinition(MONITOR_INTERVAL_SECS, newResources);
+ var disallowedContext =
tempDir.resolve("context-with-disallowed-resource-url.json");
+ Files.writeString(disallowedContext, context2.toJson(),
StandardOpenOption.CREATE,
+ StandardOpenOption.TRUNCATE_EXISTING);
+ ex = assertThrows(ContextClassLoaderException.class,
+ () ->
factory.getClassLoader(disallowedContext.toUri().toURL().toExternalForm()));
+ assertTrue(ex.getCause() instanceof IllegalStateException);
+ assertTrue(ex.getCause().getCause() instanceof ExecutionException);
+ assertTrue(ex.getCause().getCause().getCause() instanceof
IllegalArgumentException);
+ assertTrue(ex.getCause().getCause().getCause().getMessage().contains(
+ "Resource location (" + badUrl + ")"),
ex.getCause().getCause().getCause()::getMessage);
+ }
Review Comment:
I created a follow on issue. These should be easy to test and its important
to test the classloader does not work when there is no allowed url or an
invalid one set. Maybe the behavior change in the code, it was failing in init
at one point. The test does not have to check for failure in init, it can
check for failure from getting a classloader instead.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
