[
https://issues.apache.org/jira/browse/TIKA-4755?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=18086667#comment-18086667
]
ASF GitHub Bot commented on TIKA-4755:
--------------------------------------
Copilot commented on code in PR #2880:
URL: https://github.com/apache/tika/pull/2880#discussion_r3368725326
##########
tika-core/src/test/java/org/apache/tika/config/TikaExtrasTest.java:
##########
@@ -0,0 +1,109 @@
+/*
+ * 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.tika.config;
+
+import static java.nio.charset.StandardCharsets.UTF_8;
+import static org.junit.jupiter.api.Assertions.assertEquals;
+import static org.junit.jupiter.api.Assertions.assertNotNull;
+import static org.junit.jupiter.api.Assertions.assertNull;
+import static org.junit.jupiter.api.Assertions.assertSame;
+import static org.junit.jupiter.api.Assertions.assertTrue;
+
+import java.net.URLClassLoader;
+import java.nio.file.Files;
+import java.nio.file.Path;
+import java.util.jar.JarEntry;
+import java.util.jar.JarOutputStream;
+
+import org.junit.jupiter.api.Test;
+import org.junit.jupiter.api.io.TempDir;
+
+public class TikaExtrasTest {
+
+ private static final String MARKER = "tika-extra-marker.txt";
+
+ @Test
+ public void offWhenPropertyUnset() throws Exception {
+ withProperty(null, () -> {
+ assertNull(TikaExtras.extrasDir(), "feature must be off with no
property");
+ assertTrue(TikaExtras.extraJars().isEmpty());
+ assertNull(TikaExtras.install());
+ });
+ }
+
+ @Test
+ public void emptyWhenDirMissing(@TempDir Path tmp) throws Exception {
+ Path missing = tmp.resolve("does-not-exist");
+ withProperty(missing.toString(), () -> {
+ assertTrue(TikaExtras.extraJars().isEmpty());
+ assertNull(TikaExtras.install());
+ });
+ }
+
+ @Test
+ public void loadsJarsWhenPropertySet(@TempDir Path tmp) throws Exception {
+ Path jar = tmp.resolve("extra.jar");
+ try (JarOutputStream jos = new
JarOutputStream(Files.newOutputStream(jar))) {
+ jos.putNextEntry(new JarEntry(MARKER));
+ jos.write("hi".getBytes(UTF_8));
+ jos.closeEntry();
+ }
+ ClassLoader prevCtx = Thread.currentThread().getContextClassLoader();
+ ClassLoader[] installed = new ClassLoader[1];
+ try {
+ withProperty(tmp.toString(), () -> {
+ assertEquals(1, TikaExtras.extraJars().size());
+ ClassLoader cl = TikaExtras.install();
+ installed[0] = cl;
+ assertNotNull(cl, "extras dir with a jar should install a
classloader");
+ assertNotNull(cl.getResource(MARKER), "the extra jar must be
on the classloader");
+ assertSame(cl, Thread.currentThread().getContextClassLoader());
+ });
+ } finally {
+ Thread.currentThread().setContextClassLoader(prevCtx);
+ ServiceLoader.setContextClassLoader(prevCtx);
+ // Close the URLClassLoader so it releases its handle on extra.jar
before
+ // @TempDir cleanup runs; otherwise the delete fails on Windows,
where an
+ // open file cannot be removed.
+ if (installed[0] instanceof URLClassLoader) {
+ ((URLClassLoader) installed[0]).close();
+ }
+ }
Review Comment:
This test restores ServiceLoader's global context classloader to the
*thread* context classloader (prevCtx). ServiceLoader's previous context loader
is not necessarily the same as the thread context loader (and by default it's
typically unset/null), so this can leak global state into subsequent tests.
Since this test is the only place in the test suite that mutates
ServiceLoader.CONTEXT_CLASS_LOADER, resetting it to null avoids polluting other
tests.
##########
tika-core/src/main/java/org/apache/tika/config/TikaExtras.java:
##########
@@ -0,0 +1,150 @@
+/*
+ * 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.tika.config;
+
+import java.net.URL;
+import java.net.URLClassLoader;
+import java.nio.file.DirectoryStream;
+import java.nio.file.Files;
+import java.nio.file.Path;
+import java.util.ArrayList;
+import java.util.Collections;
+import java.util.Comparator;
+import java.util.List;
+
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+/**
+ * Opt-in mechanism for adding user-supplied "extras" jars (extra
+ * {@code EncodingDetector}s, {@code Parser}s, etc.) to Tika's SPI discovery
+ * without repackaging the application.
+ *
+ * <p><b>Off by default.</b> Nothing is loaded unless the
+ * {@value #EXTRAS_DIR_PROPERTY} system property points at a directory; then
every
+ * {@code *.jar} in it is made visible to service-loading. There is no
implicit
+ * directory, and the working directory is never used.
+ *
+ * <p><b>Security:</b> this is a trusted code directory — anything in it runs
with
+ * the full privileges of the Tika process. Treat write access to it exactly
like
+ * write access to {@code lib/}; it must not be writable by less-trusted
principals
+ * (for servers, not reachable by request handling). Being opt-in keeps "we
are
+ * now loading extra code" an explicit, auditable choice.
+ */
+public final class TikaExtras {
+
+ /** System property naming the extras directory. Unset = feature off. */
+ public static final String EXTRAS_DIR_PROPERTY = "tika.extras.dir";
+
+ private static final Logger LOG =
LoggerFactory.getLogger(TikaExtras.class);
+
+ private TikaExtras() {
+ }
+
+ /**
+ * If {@value #EXTRAS_DIR_PROPERTY} is set, installs a classloader over the
+ * {@code *.jar} files in that directory as the thread + Tika
+ * {@link ServiceLoader} context classloader, so they join SPI discovery.
+ * No-op (returns {@code null}) when the property is unset or the
directory is
+ * missing/empty. Call once, before any Tika component is loaded.
+ *
+ * @return the installed classloader, or {@code null} if extras are
off/empty
+ */
+ public static ClassLoader install() {
+ List<Path> jars = extraJars();
+ if (jars.isEmpty()) {
+ return null;
+ }
+ List<URL> urls = new ArrayList<>(jars.size());
+ for (Path jar : jars) {
+ try {
+ urls.add(jar.toUri().toURL());
+ } catch (Exception e) {
+ LOG.warn("Skipping extra jar {}: {}", jar, e.toString());
+ }
+ }
+ if (urls.isEmpty()) {
+ return null;
+ }
+ ClassLoader parent = Thread.currentThread().getContextClassLoader();
+ if (parent == null) {
+ parent = TikaExtras.class.getClassLoader();
+ }
+ URLClassLoader cl = new URLClassLoader(urls.toArray(new URL[0]),
parent);
+ Thread.currentThread().setContextClassLoader(cl);
+ ServiceLoader.setContextClassLoader(cl);
+ LOG.info("{}: loaded {} extra jar(s): {}", EXTRAS_DIR_PROPERTY,
urls.size(), jars);
+ return cl;
+ }
Review Comment:
The info log reports the number of successfully-added URLs (urls.size()) but
prints the original 'jars' list, which can include entries that were skipped
due to URL conversion failures. This can produce confusing/misleading logs
(count doesn't match list).
##########
docs/modules/ROOT/pages/configuration/index.adoc:
##########
@@ -28,6 +28,33 @@ content handlers, server behavior, and the Tika Pipes
pipeline.
NOTE: Tika 3.x and earlier used XML configuration (`tika-config.xml`). See the
xref:migration-to-4x/index.adoc[Migration Guide] for details on converting to
JSON.
+== Adding extra jars (`tika.extras.dir`)
+
+To add extra components — additional ``EncodingDetector``s, ``Parser``s, or
their
+dependencies — without repackaging the application, drop their jars in a
directory
Review Comment:
The inline formatting here uses double backticks (``EncodingDetector``s /
``Parser``s), which in AsciiDoc is typically interpreted as quote markup rather
than inline code. Elsewhere in the docs, class names are formatted with single
backticks (e.g., `EncodingDetector`). This line likely renders incorrectly.
##########
tika-core/src/main/java/org/apache/tika/config/TikaExtras.java:
##########
@@ -0,0 +1,150 @@
+/*
+ * 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.tika.config;
+
+import java.net.URL;
+import java.net.URLClassLoader;
+import java.nio.file.DirectoryStream;
+import java.nio.file.Files;
+import java.nio.file.Path;
+import java.util.ArrayList;
+import java.util.Collections;
+import java.util.Comparator;
+import java.util.List;
+
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+/**
+ * Opt-in mechanism for adding user-supplied "extras" jars (extra
+ * {@code EncodingDetector}s, {@code Parser}s, etc.) to Tika's SPI discovery
+ * without repackaging the application.
+ *
+ * <p><b>Off by default.</b> Nothing is loaded unless the
+ * {@value #EXTRAS_DIR_PROPERTY} system property points at a directory; then
every
+ * {@code *.jar} in it is made visible to service-loading. There is no
implicit
+ * directory, and the working directory is never used.
+ *
+ * <p><b>Security:</b> this is a trusted code directory — anything in it runs
with
+ * the full privileges of the Tika process. Treat write access to it exactly
like
+ * write access to {@code lib/}; it must not be writable by less-trusted
principals
+ * (for servers, not reachable by request handling). Being opt-in keeps "we
are
+ * now loading extra code" an explicit, auditable choice.
+ */
+public final class TikaExtras {
+
+ /** System property naming the extras directory. Unset = feature off. */
+ public static final String EXTRAS_DIR_PROPERTY = "tika.extras.dir";
+
+ private static final Logger LOG =
LoggerFactory.getLogger(TikaExtras.class);
+
+ private TikaExtras() {
+ }
+
+ /**
+ * If {@value #EXTRAS_DIR_PROPERTY} is set, installs a classloader over the
+ * {@code *.jar} files in that directory as the thread + Tika
+ * {@link ServiceLoader} context classloader, so they join SPI discovery.
+ * No-op (returns {@code null}) when the property is unset or the
directory is
+ * missing/empty. Call once, before any Tika component is loaded.
+ *
+ * @return the installed classloader, or {@code null} if extras are
off/empty
+ */
+ public static ClassLoader install() {
+ List<Path> jars = extraJars();
+ if (jars.isEmpty()) {
+ return null;
+ }
+ List<URL> urls = new ArrayList<>(jars.size());
+ for (Path jar : jars) {
+ try {
+ urls.add(jar.toUri().toURL());
+ } catch (Exception e) {
+ LOG.warn("Skipping extra jar {}: {}", jar, e.toString());
+ }
+ }
+ if (urls.isEmpty()) {
+ return null;
+ }
+ ClassLoader parent = Thread.currentThread().getContextClassLoader();
+ if (parent == null) {
+ parent = TikaExtras.class.getClassLoader();
+ }
+ URLClassLoader cl = new URLClassLoader(urls.toArray(new URL[0]),
parent);
+ Thread.currentThread().setContextClassLoader(cl);
+ ServiceLoader.setContextClassLoader(cl);
+ LOG.info("{}: loaded {} extra jar(s): {}", EXTRAS_DIR_PROPERTY,
urls.size(), jars);
+ return cl;
+ }
+
+ /**
+ * The {@code *.jar} files in the {@value #EXTRAS_DIR_PROPERTY} directory
— for
+ * callers that extend a forked process's classpath rather than installing
a
+ * classloader. Empty when the property is unset or the directory is
+ * missing/has no jars.
+ */
+ public static List<Path> extraJars() {
+ Path dir = extrasDir();
+ if (dir == null || !Files.isDirectory(dir)) {
+ return Collections.emptyList();
+ }
+ List<Path> jars = new ArrayList<>();
+ try (DirectoryStream<Path> stream = Files.newDirectoryStream(dir,
"*.jar")) {
+ for (Path jar : stream) {
+ jars.add(jar);
+ }
+ } catch (Exception e) {
+ LOG.warn("Could not scan {}={}: {}", EXTRAS_DIR_PROPERTY, dir,
e.toString());
+ }
+ // Sort by file name so jar load order (classloader URL order /
forked-child
+ // classpath order, hence SPI precedence) is deterministic across
platforms
+ // and filesystems rather than depending on directory iteration order.
+ jars.sort(Comparator.comparing(jar -> jar.getFileName().toString()));
+ return jars;
+ }
+
+ /**
+ * Appends the {@link #extraJars()} (as absolute paths, joined with the
+ * platform path separator) to the given classpath string — for extending a
+ * forked process's {@code -cp} with the extras jars. Returns {@code
classpath}
+ * unchanged when the feature is off or the directory has no jars.
+ *
+ * @param classpath the base classpath to extend
+ * @return the classpath with any extras jars appended
+ */
+ public static String appendJarsToClasspath(String classpath) {
+ List<Path> jars = extraJars();
+ if (jars.isEmpty()) {
+ return classpath;
+ }
+ StringBuilder sb = new StringBuilder(classpath);
+ String separator = System.getProperty("path.separator");
+ for (Path jar : jars) {
+ sb.append(separator).append(jar.toAbsolutePath());
+ }
+ return sb.toString();
+ }
Review Comment:
appendJarsToClasspath assumes the incoming classpath is non-null. If callers
ever pass null (e.g., System.getProperty("java.class.path") being unavailable),
this will throw a NullPointerException when constructing StringBuilder. It also
always prefixes extras with the path separator even when the base classpath is
empty, producing a leading separator.
> Allow extras directory for users to add jars for tika-app and tika-server
> -------------------------------------------------------------------------
>
> Key: TIKA-4755
> URL: https://issues.apache.org/jira/browse/TIKA-4755
> Project: Tika
> Issue Type: Task
> Reporter: Tim Allison
> Priority: Minor
>
> We do this for tika-server in docker. We should do the same for tika-app and
> regular tika-server.
--
This message was sent by Atlassian Jira
(v8.20.10#820010)
