diff --git a/api/src/org/labkey/api/ApiModule.java b/api/src/org/labkey/api/ApiModule.java index 407bae5d230..b8c0867b594 100644 --- a/api/src/org/labkey/api/ApiModule.java +++ b/api/src/org/labkey/api/ApiModule.java @@ -23,6 +23,7 @@ import org.jetbrains.annotations.NotNull; import org.json.JSONObject; import org.labkey.api.action.ApiXmlWriter; +import org.labkey.api.action.ConcurrencyLimiter; import org.labkey.api.action.SpringActionController; import org.labkey.api.admin.SubfolderWriter; import org.labkey.api.assay.AssayResultsFileWriter; @@ -180,6 +181,7 @@ import org.labkey.api.util.SystemMaintenanceStartupListener; import org.labkey.api.util.URIUtil; import org.labkey.api.util.URLHelper; +import org.labkey.api.util.XmlBeansUtil; import org.labkey.api.util.emailTemplate.EmailTemplate; import org.labkey.api.view.ActionURL; import org.labkey.api.view.FileServlet; @@ -411,6 +413,7 @@ public void registerServlets(ServletContext servletCtx) ChecksumUtil.TestCase.class, CollectionUtils.TestCase.class, Compress.TestCase.class, + ConcurrencyLimiter.TestCase.class, Constants.TestCase.class, ConvertHelper.TestCase.class, CspCommentScanner.TestCase.class, @@ -482,7 +485,8 @@ public void registerServlets(ServletContext servletCtx) TabLoader.HeaderMatchTest.class, Table.IsSelectTestCase.class, URIUtil.TestCase.class, - ValidEmail.TestCase.class + ValidEmail.TestCase.class, + XmlBeansUtil.TestCase.class ); } diff --git a/api/src/org/labkey/api/action/ConcurrencyLimit.java b/api/src/org/labkey/api/action/ConcurrencyLimit.java new file mode 100644 index 00000000000..923ade67d15 --- /dev/null +++ b/api/src/org/labkey/api/action/ConcurrencyLimit.java @@ -0,0 +1,54 @@ +/* + * Copyright (c) 2026 LabKey Corporation + * + * Licensed 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.labkey.api.action; + +import java.lang.annotation.ElementType; +import java.lang.annotation.Retention; +import java.lang.annotation.Target; + +/** + * Caps the number of requests executing an action at the same time, server-wide, for actions expensive enough that a + * few simultaneous callers can exhaust heap or the request thread pool. + *

+ * {@link ConcurrencyLimiter} enforces the limit from {@link SpringActionController#handleRequest}, after the permission + * check and around the whole action including rendering. A request that can't get a permit within + * {@link #timeoutSeconds()} is rejected with a 429 and never executes the action. + *

+ * Not inherited: a subclass of an annotated action is unlimited unless it declares its own {@code @ConcurrencyLimit}. + */ +@Retention(java.lang.annotation.RetentionPolicy.RUNTIME) +@Target(ElementType.TYPE) +public @interface ConcurrencyLimit +{ + int DEFAULT_TIMEOUT_SECONDS = 2; + int DEFAULT_RETRY_AFTER_SECONDS = 30; + String DEFAULT_MESSAGE = ""; + + /** Maximum number of requests allowed to execute this action concurrently, across the whole server. Must be positive. */ + int value(); + + /** + * How long an incoming request waits for a permit before it's rejected with a 429. Keep it short - a parked thread + * still consumes a connector thread, so a long wait just moves the exhaustion problem. + */ + long timeoutSeconds() default DEFAULT_TIMEOUT_SECONDS; + + /** Value sent in the {@code Retry-After} response header when a request is rejected. */ + int retryAfterSeconds() default DEFAULT_RETRY_AFTER_SECONDS; + + /** Message sent to the client when a request is rejected. Defaults to a generic message when not specified. */ + String message() default DEFAULT_MESSAGE; +} diff --git a/api/src/org/labkey/api/action/ConcurrencyLimiter.java b/api/src/org/labkey/api/action/ConcurrencyLimiter.java new file mode 100644 index 00000000000..edfe754761a --- /dev/null +++ b/api/src/org/labkey/api/action/ConcurrencyLimiter.java @@ -0,0 +1,244 @@ +/* + * Copyright (c) 2026 LabKey Corporation + * + * Licensed 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.labkey.api.action; + +import org.apache.logging.log4j.Logger; +import org.jetbrains.annotations.NotNull; +import org.jetbrains.annotations.Nullable; +import org.junit.Assert; +import org.junit.Test; +import org.labkey.api.util.logging.LogHelper; +import org.labkey.api.view.TooManyRequestsException; +import org.labkey.api.view.ViewContext; + +import java.util.Map; +import java.util.concurrent.ConcurrentHashMap; +import java.util.concurrent.Semaphore; +import java.util.concurrent.TimeUnit; +import java.util.concurrent.atomic.AtomicBoolean; + +/** + * Enforces {@link ConcurrencyLimit} on behalf of {@link SpringActionController}. Holds one {@link Semaphore} per + * action class that declares the annotation. + */ +public class ConcurrencyLimiter +{ + private static final Logger LOG = LogHelper.getLogger(ConcurrencyLimiter.class, "Rejects requests to actions that limit their concurrency when too many are already in flight"); + + static final String GENERIC_MESSAGE = "The server is already handling as many simultaneous requests for this operation as it allows. Please retry in a few moments."; + + /** + * One limiter (and therefore one set of permits) per action class. {@link ConcurrentHashMap} won't store a null + * value, so unannotated actions map to {@link #UNLIMITED}. + */ + private static final Map, ConcurrencyLimiter> LIMITERS = new ConcurrentHashMap<>(); + + /** + * Limiter for an action that declares no limit. + */ + private static final ConcurrencyLimiter UNLIMITED = new ConcurrencyLimiter(); + + /** + * Handed out by {@link #UNLIMITED}; holds no permit, so closing it must do nothing. + */ + private static final Permit NOOP_PERMIT = () -> {}; + + /** + * Returned by {@link #acquire}; release the permit by closing it, ideally via try-with-resources + */ + public interface Permit extends AutoCloseable + { + @Override + void close(); + } + + /** + * Reserve one of the action's permits, if it declares a {@link ConcurrencyLimit}. + * + * @return a {@link Permit} that the caller must close once the action has finished executing + * @throws TooManyRequestsException if no permit becomes available within the action's configured timeout + */ + public static Permit acquire(@NotNull Class actionClass, @Nullable ViewContext context) + { + // This runs on every request, and computeIfAbsent() locks the bin even on a hit unless the key happens to be + // its first node, so take the lock-free get() whenever the limiter is already resolved. + ConcurrencyLimiter limiter = LIMITERS.get(actionClass); + + if (null == limiter) + limiter = LIMITERS.computeIfAbsent(actionClass, ConcurrencyLimiter::resolve); + + return limiter.acquirePermit(context); + } + + private static ConcurrencyLimiter resolve(Class actionClass) + { + ConcurrencyLimit limit = actionClass.getDeclaredAnnotation(ConcurrencyLimit.class); + + return null == limit ? UNLIMITED : new ConcurrencyLimiter(actionClass, limit); + } + + /** + * All three are null for the {@link #UNLIMITED} sentinel and non-null for every other instance. + */ + private final Class _actionClass; + private final ConcurrencyLimit _limit; + private final Semaphore _semaphore; + + private ConcurrencyLimiter() + { + _actionClass = null; + _limit = null; + _semaphore = null; + } + + private ConcurrencyLimiter(@NotNull Class actionClass, @NotNull ConcurrencyLimit limit) + { + if (limit.value() < 1) + throw new IllegalStateException("@ConcurrencyLimit on " + actionClass.getName() + " must allow at least one concurrent request, but was " + limit.value()); + + _actionClass = actionClass; + _limit = limit; + // Fair, so a steady stream of new requests can't starve one that's already waiting + _semaphore = new Semaphore(limit.value(), true); + } + + private Permit acquirePermit(@Nullable ViewContext context) + { + if (this == UNLIMITED) + return NOOP_PERMIT; + + try + { + if (!_semaphore.tryAcquire(_limit.timeoutSeconds(), TimeUnit.SECONDS)) + throw reject(context); + } + catch (InterruptedException e) + { + Thread.currentThread().interrupt(); + throw reject(context); + } + + // Releasing twice would permanently inflate the pool and quietly defeat the limit, so make close() idempotent + AtomicBoolean released = new AtomicBoolean(); + return () -> { + if (released.compareAndSet(false, true)) + _semaphore.release(); + }; + } + + private TooManyRequestsException reject(@Nullable ViewContext context) + { + LOG.info("Rejecting request to {} for user {} in {}: {} requests already in progress", _actionClass.getName(), + null == context ? "" : context.getUser(), + null == context || null == context.getContainer() ? "" : context.getContainer().getPath(), + _limit.value()); + + String message = _limit.message().isEmpty() ? GENERIC_MESSAGE : _limit.message(); + return new TooManyRequestsException(message, _limit.retryAfterSeconds()); + } + + public static class TestCase extends Assert + { + @ConcurrencyLimit(value = 2, timeoutSeconds = 0, retryAfterSeconds = 7, message = "Slow down") + private static class LimitedAction + { + } + + /** + * Deliberately carries no annotation of its own - the limit is not inherited. + */ + private static class SubclassAction extends LimitedAction + { + } + + @ConcurrencyLimit(1) + private static class SingleRequestAction + { + } + + private static class UnlimitedAction + { + } + + @ConcurrencyLimit(0) + private static class BadLimitAction + { + } + + @Test + public void testUnlimitedActionIsNotThrottled() + { + // Many more than any limit would allow, all held at once + for (int i = 0; i < 100; i++) + //noinspection resource + ConcurrencyLimiter.acquire(UnlimitedAction.class, null); + } + + @Test + public void testRejectsBeyondLimit() + { + try (Permit p1 = acquire(LimitedAction.class, null); Permit p2 = acquire(LimitedAction.class, null)) + { + assertNotNull(p1); + assertNotNull(p2); + + @SuppressWarnings("resource") TooManyRequestsException e = assertThrows(TooManyRequestsException.class, () -> acquire(LimitedAction.class, null)); + assertEquals(TooManyRequestsException.SC_TOO_MANY_REQUESTS, e.getStatus()); + assertEquals("Slow down", e.getMessage()); + assertEquals(7, e.getRetryAfterSeconds()); + } + + // Permits are back after the try-with-resources released them + acquire(LimitedAction.class, null).close(); + } + + @Test + public void testLimitIsNotInherited() + { + // More than the superclass's limit of two + try (Permit p1 = acquire(SubclassAction.class, null); Permit p2 = acquire(SubclassAction.class, null); Permit p3 = acquire(SubclassAction.class, null)) + { + assertNotNull(p1); + assertNotNull(p2); + assertNotNull(p3); + + // The superclass still has both of its own permits available + try (Permit p4 = acquire(LimitedAction.class, null); Permit p5 = acquire(LimitedAction.class, null)) + { + assertNotNull(p4); + assertNotNull(p5); + } + } + } + + @Test + public void testGenericMessage() + { + try (Permit ignored = acquire(SingleRequestAction.class, null)) + { + //noinspection resource + assertEquals(GENERIC_MESSAGE, assertThrows(TooManyRequestsException.class, () -> acquire(SingleRequestAction.class, null)).getMessage()); + } + } + + @Test + public void testNonPositiveLimitIsRejected() + { + //noinspection resource + assertThrows(IllegalStateException.class, () -> acquire(BadLimitAction.class, null)); + } + } +} diff --git a/api/src/org/labkey/api/action/SpringActionController.java b/api/src/org/labkey/api/action/SpringActionController.java index cdbfe3c27c7..359eec144bc 100644 --- a/api/src/org/labkey/api/action/SpringActionController.java +++ b/api/src/org/labkey/api/action/SpringActionController.java @@ -532,11 +532,16 @@ public ModelAndView handleRequest(HttpServletRequest request, @NotNull HttpServl QueryService.get().setEnvironment(QueryService.Environment.ACTION, actionAnnotation.value()); } - beforeAction(controller); - ModelAndView mv = controller.handleRequest(request, response); - if (mv != null) + // After the permission check so unauthorized requests can't consume @ConcurrencyLimit permits, and held + // through rendering because the action's memory is live for that whole time. + try (ConcurrencyLimiter.Permit ignored = ConcurrencyLimiter.acquire(actionClass, context)) { - renderInTemplate(context, controller, pageConfig, mv); + beforeAction(controller); + ModelAndView mv = controller.handleRequest(request, response); + if (mv != null) + { + renderInTemplate(context, controller, pageConfig, mv); + } } } catch (HttpRequestMethodNotSupportedException x) diff --git a/api/src/org/labkey/api/assay/transform/DataTransformService.java b/api/src/org/labkey/api/assay/transform/DataTransformService.java index 70d6886b2bb..4e421e95efc 100644 --- a/api/src/org/labkey/api/assay/transform/DataTransformService.java +++ b/api/src/org/labkey/api/assay/transform/DataTransformService.java @@ -157,6 +157,11 @@ public TransformResult transformAndValidate( bindings.put(ExternalScriptEngine.WORKING_DIRECTORY, scriptDir.toNioPathForWrite().toString()); bindings.put(ExternalScriptEngine.SCRIPT_PATH, scriptFile.toNioPathForRead().toFile().getAbsolutePath()); + bindings.put(ExternalScriptEngine.INVOCATION_LABEL, "transform protocol=" + context.getProtocol().getRowId() + + " name='" + context.getProtocol().getName() + "'" + + " operation=" + operation.name() + + " script='" + scriptFile.getName() + "'" + + " container=" + context.getContainer().getPath()); Map paramMap = new HashMap<>(); diff --git a/api/src/org/labkey/api/dataiterator/DataIteratorUtil.java b/api/src/org/labkey/api/dataiterator/DataIteratorUtil.java index 620b31fd639..6958c128865 100644 --- a/api/src/org/labkey/api/dataiterator/DataIteratorUtil.java +++ b/api/src/org/labkey/api/dataiterator/DataIteratorUtil.java @@ -60,13 +60,15 @@ import java.util.function.Consumer; import java.util.function.Function; import java.util.function.Supplier; +import java.util.function.UnaryOperator; import java.util.stream.Stream; import java.util.stream.StreamSupport; /** - * User: matthewb - * Date: 2011-05-31 - * Time: 12:52 PM + * Static helpers for assembling DataIterator pipelines: matching a source iterator's columns to a target + * TableInfo's columns (by property URI, name, import alias, or JDBC-legal name) and copying or merging the + * result into that table. Also provides adapters that present an iterator as scrollable, map-based, + * map-transforming, or a Stream of maps. */ public class DataIteratorUtil { @@ -373,6 +375,28 @@ public static void closeQuietly(DataIterator it) } } + /** + * DataIteratorBuilder.getDataIterator() calls this to simplify implementing the success or close() input contract + */ + public static @Nullable DataIterator wrapOrClose(DataIteratorBuilder in, DataIteratorContext context, UnaryOperator wrapper) + { + DataIterator di = in.getDataIterator(context); + if (null == di) + return null; + + try + { + DataIterator out = wrapper.apply(di); + if (null == out) + closeQuietly(di); + return out; + } + catch (RuntimeException | Error e) + { + closeQuietly(di); + throw e; + } + } /* * Wrapping functions to add functionality to existing DataIterators diff --git a/api/src/org/labkey/api/reports/ExternalScriptEngine.java b/api/src/org/labkey/api/reports/ExternalScriptEngine.java index 64a79d1a439..3a73c57e13d 100644 --- a/api/src/org/labkey/api/reports/ExternalScriptEngine.java +++ b/api/src/org/labkey/api/reports/ExternalScriptEngine.java @@ -80,11 +80,17 @@ public class ExternalScriptEngine extends AbstractScriptEngine implements LabKey /** Timeout in seconds. */ public static final String TIMEOUT = "external.script.engine.timeout"; + /** Caller-supplied identity for the invocation log; the engine knows the duration but not which report or assay design it ran for. */ + public static final String INVOCATION_LABEL = "external.script.engine.invocationLabel"; + public static final String DEFAULT_WORKING_DIRECTORY = "ExternalScript"; private static final Pattern scriptCmdPattern = Pattern.compile("'([^']+)'|\\\"([^\\\"]+)\\\"|(^[^\\s]+)|(\\s[^\\s^'^\\\"]+)"); private FileLike _workingDirectory; + /** Set when runProcess() kills the script, so the kill's exit code isn't logged as a second, separate failure. */ + private boolean _timedOut; + protected ExternalScriptEngineDefinition _def; protected Writer _originalWriter; @@ -110,7 +116,15 @@ public boolean isBinary(FileLike file) } @Override - public Object eval(String script, ScriptContext context) throws ScriptException + public final Object eval(String script, ScriptContext context) throws ScriptException + { + // final so every engine in this hierarchy is timed from one place; subclasses override evalScript() + _timedOut = false; + return ScriptInvocationLog.time(getClass().getSimpleName(), ScriptInvocationLog.label(context), + () -> evalScript(script, context)); + } + + protected Object evalScript(String script, ScriptContext context) throws ScriptException { List extensions = getFactory().getExtensions(); @@ -139,6 +153,8 @@ protected Object eval(FileLike scriptFile, ScriptContext context) throws ScriptE int exitCode = runProcess(context, pb, output, timeout, TimeUnit.SECONDS); if (exitCode != 0) { + if (!_timedOut) + ScriptInvocationLog.nonZeroExit(getClass().getSimpleName(), ScriptInvocationLog.label(context), exitCode); throw new ScriptException("An error occurred when running the script '" + scriptFile.getName() + "', exit code: " + exitCode + ".\n" + output); } else @@ -384,6 +400,8 @@ protected int runProcess(ScriptContext context, LabKeyProcessBuilder pb, StringB String msg = "Process killed after exceeding timeout of " + timeout + " " + timeoutUnit.name().toLowerCase() + "\n"; output.append(msg); + _timedOut = true; + ScriptInvocationLog.timedOut(getClass().getSimpleName(), ScriptInvocationLog.label(context), timeout, timeoutUnit); if (writer != null) writer.write(msg); } diff --git a/api/src/org/labkey/api/reports/ScriptInvocationLog.java b/api/src/org/labkey/api/reports/ScriptInvocationLog.java new file mode 100644 index 00000000000..77a0ac55b22 --- /dev/null +++ b/api/src/org/labkey/api/reports/ScriptInvocationLog.java @@ -0,0 +1,94 @@ +/* + * Copyright (c) 2026 LabKey Corporation + * + * Licensed 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.labkey.api.reports; + +import org.apache.logging.log4j.Logger; +import org.jetbrains.annotations.Nullable; +import org.labkey.api.util.logging.LogHelper; + +import javax.script.ScriptContext; +import javax.script.ScriptException; +import java.util.concurrent.TimeUnit; +import java.util.regex.Pattern; + +/** + * Start/completion/duration logging for external script invocations. Separate class so it gets its own logger + * category, enabling timing without the engines' unrelated debug output. Messages are key=value to keep log + * analysis parser-free. + */ +public final class ScriptInvocationLog +{ + private static final Logger LOG = LogHelper.getLogger(ScriptInvocationLog.class, "R and Python script invocation start, completion, and duration"); + + private static final Pattern LINE_BREAKS = Pattern.compile("[\\r\\n]+"); + + private ScriptInvocationLog() + { + } + + public interface ScriptBody + { + T run() throws ScriptException; + } + + /** + * The caller's INVOCATION_LABEL binding, or null if the caller didn't set one. Labels embed user-supplied report + * and assay design names, so line breaks are collapsed to keep one invocation on one line. + */ + @Nullable + public static String label(ScriptContext context) + { + Object label = context == null ? null : context.getAttribute(ExternalScriptEngine.INVOCATION_LABEL, ScriptContext.ENGINE_SCOPE); + return label == null ? null : LINE_BREAKS.matcher(String.valueOf(label)).replaceAll(" "); + } + + /** + * Start is DEBUG because it only matters for diagnosing a hang, where a start with no completion is the sole evidence. + * The failure line deliberately omits the exception message: for a non-zero exit that message carries the script's + * entire stdout/stderr, which can be huge and can echo the transform session's API key. + */ + public static T time(String engine, @Nullable String label, ScriptBody body) throws ScriptException + { + LOG.debug("script start engine={} label={}", engine, label); + long start = System.nanoTime(); + boolean completed = false; + try + { + T result = body.run(); + completed = true; + return result; + } + finally + { + // finally rather than catch so an Error still logs a duration, which a hang never does + long durationMs = TimeUnit.NANOSECONDS.toMillis(System.nanoTime() - start); + if (completed) + LOG.info("script done engine={} label={} durationMs={}", engine, label, durationMs); + else + LOG.warn("script failed engine={} label={} durationMs={}", engine, label, durationMs); + } + } + + public static void timedOut(String engine, @Nullable String label, long timeout, TimeUnit unit) + { + LOG.warn("script timeout engine={} label={} timeout={} unit={}", engine, label, timeout, unit.name().toLowerCase()); + } + + public static void nonZeroExit(String engine, @Nullable String label, int exitCode) + { + LOG.warn("script exit engine={} label={} exitCode={}", engine, label, exitCode); + } +} diff --git a/api/src/org/labkey/api/reports/report/ExternalScriptEngineReport.java b/api/src/org/labkey/api/reports/report/ExternalScriptEngineReport.java index 1cab97ee43a..a394302d5a0 100644 --- a/api/src/org/labkey/api/reports/report/ExternalScriptEngineReport.java +++ b/api/src/org/labkey/api/reports/report/ExternalScriptEngineReport.java @@ -344,6 +344,10 @@ protected Object runScript(ScriptEngine engine, ViewContext context, ListA real loopback server rather than a {@code ProxySelector}, which is global JVM state visible to every other + * thread in a running server. + * + *

The probe only sees references it hosts, so a test proving "nothing external at all" must point every reference + * in its fixture at {@link #url}. Not thread-safe across tests: start and close one per test. + * + *

Test-only, but here so that other modules can use it for their own testing. + */ +public class ExternalReferenceProbe implements AutoCloseable +{ + /** A DTD body that is well-formed enough for a parser to accept once it has been fetched. */ + public static final String DTD_BODY = ""; + /** An entity replacement body that is visible in the parsed document if expansion happened. */ + public static final String ENTITY_BODY = "external-content-was-fetched"; + + private final HttpServer _server; + private final List _contacted = new CopyOnWriteArrayList<>(); + private final Map _bodies = new ConcurrentHashMap<>(); + + private ExternalReferenceProbe(HttpServer server) + { + _server = server; + } + + public static ExternalReferenceProbe start() throws IOException + { + HttpServer server = HttpServer.create(new InetSocketAddress(InetAddress.getLoopbackAddress(), 0), 0); + ExternalReferenceProbe probe = new ExternalReferenceProbe(server); + server.createContext("/", exchange -> { + String path = exchange.getRequestURI().getPath(); + probe._contacted.add(path); + byte[] body = probe._bodies.getOrDefault(path, "").getBytes(StandardCharsets.UTF_8); + exchange.sendResponseHeaders(200, body.length); + try (OutputStream out = exchange.getResponseBody()) + { + out.write(body); + } + }); + server.start(); + return probe; + } + + /** + * Register {@code body} at {@code path} and return the absolute URL to embed in the XML under test. + * @param path must start with "/", e.g. "/external.dtd" + */ + public String url(@NotNull String path, @NotNull String body) + { + _bodies.put(path, body); + return "http://" + _server.getAddress().getHostString() + ":" + _server.getAddress().getPort() + path; + } + + /** Paths the XML machinery actually requested, in order. */ + public @NotNull List contactedPaths() + { + return List.copyOf(_contacted); + } + + public boolean wasContacted() + { + return !_contacted.isEmpty(); + } + + public void assertNotContacted(String message) + { + Assert.assertTrue(message + " -- external reference(s) were resolved over the network: " + _contacted, + _contacted.isEmpty()); + } + + @Override + public void close() + { + _server.stop(0); + } +} diff --git a/api/src/org/labkey/api/util/FileType.java b/api/src/org/labkey/api/util/FileType.java index e96330741d2..fcac2b38769 100644 --- a/api/src/org/labkey/api/util/FileType.java +++ b/api/src/org/labkey/api/util/FileType.java @@ -16,6 +16,7 @@ package org.labkey.api.util; import org.apache.commons.io.IOCase; +import org.apache.logging.log4j.Logger; import org.apache.tika.detect.DefaultDetector; import org.apache.tika.detect.Detector; import org.apache.tika.io.TikaInputStream; @@ -27,6 +28,7 @@ import org.junit.Assert; import org.junit.Test; import org.labkey.api.pipeline.file.FileAnalysisJobSupport; +import org.labkey.api.util.logging.LogHelper; import org.labkey.vfs.FileLike; import java.io.File; @@ -38,20 +40,31 @@ import java.util.Arrays; import java.util.Collections; import java.util.List; +import java.util.Locale; import java.util.Objects; /** - * FileType + * Captures a file naming convention via an ordered list of suffixes (usually extensions, but any name-ending works). + * One is the canonical suffix used when creating new files. Optional constraints on MIME content type, directory-ness, + * and file header contents. * - * @author brendanx + * Matching ignores case unless {@link #setCaseSensitiveOnCaseSensitiveFileSystems} is set. + * + * Because suffixes match by name-ending, a broad type swallows names belonging to a more specific one — pepXML's ".xml" + * also matches protXML's ".pep-prot.xml". Register the specific type via {@link #addAntiFileType} to exclude it. + * + * Subclasses distinguish types that share an extension (".txt", ".xml") by overriding {@link #isHeaderMatch}; the base + * implementation never matches on header alone. */ public class FileType implements Serializable { private static final Detector DETECTOR = new DefaultDetector(MimeTypes.getDefaultMimeTypes()); + private static final Logger LOG = LogHelper.getLogger(FileType.class, "file suffix matching"); // For serialization protected FileType() {} + /** Asks the job for a file under each suffix in priority order, falling back to {@link #getDefaultName} when none exist. */ public FileLike findInputFile(FileAnalysisJobSupport support, String baseName) { if (_suffixes.size() > 1) @@ -406,12 +419,13 @@ private String toLowerIfCaseInsensitive(String s) { return s; } - return s.toLowerCase(); + // ROOT, not the default locale: in Turkish 'I' lowercases to dotless 'ı', so ".MZID" would stop matching ".mzid" + return s.toLowerCase(Locale.ROOT); } /** - * Finds the best suffix based on priority order, strips it off, and returns the remainder. If there is no matching - * suffix, returns the original file name. + * Strips the longest matching suffix and returns the remainder, or the original file name if nothing matches. Longest + * rather than first-in-list, so ".msprefix.mzXML" isn't reduced to "foo.msprefix". */ public String getBaseName(File file) { @@ -444,7 +458,7 @@ public String getBaseName(@NotNull java.nio.file.Path file) else if (_supportGZ.booleanValue()) // TPP treats .xml.gz as a native read format { String sgz = s+".gz"; - if (fileName.endsWith(sgz)) + if (toLowerIfCaseInsensitive(fileName).endsWith(toLowerIfCaseInsensitive(sgz))) { if ((null==suffix) || (sgz.length()>suffix.length())) { @@ -453,7 +467,14 @@ else if (_supportGZ.booleanValue()) // TPP treats .xml.gz as a native read forma } } } - assert suffix != null : "Could not find matching suffix even though types match"; + if (suffix == null) + { + // Unreachable unless isType() and this loop disagree; warn so it stays visible with assertions disabled + String message = "Could not find matching suffix for " + fileName + " even though types match: " + this + ", supportGZ: " + _supportGZ; + assert false : message; + LOG.warn(message); + return fileName; + } return fileName.substring(0, fileName.length() - suffix.length()); } @@ -508,7 +529,9 @@ public boolean isType(String filePath) } /** - * Checks if the path matches any of the suffixes and the file header if provided. + * Matches in order: reject if an anti-type matches, accept on content type (detected from the header via Tika when not + * supplied), accept on suffix (with the header, if given) and finally accept on header alone, that last only when + * the caller supplied no content type, since a caller-supplied type that didn't match is authoritative. */ public boolean isType(@Nullable String filePath, @Nullable String contentType, byte @Nullable[] header) { @@ -529,7 +552,7 @@ public boolean isType(@Nullable String filePath, @Nullable String contentType, b if (contentType != null) { - contentType = contentType.toLowerCase().trim(); + contentType = contentType.toLowerCase(Locale.ROOT).trim(); if (_contentTypes.contains(contentType)) return true; } @@ -578,16 +601,19 @@ protected static String detectContentType(String fileName, byte[] header) } } + /** Whether the name is exactly basename plus one of the suffixes, unlike {@link #isType}, which accepts any name ending in a suffix. */ public boolean isMatch(String name, String basename) { + String normalizedName = toLowerIfCaseInsensitive(name); for (String suffix : _suffixes) { - if (name.equalsIgnoreCase(basename + suffix)) + String normalizedBase = toLowerIfCaseInsensitive(basename + suffix); + if (normalizedName.equals(normalizedBase)) { return true; } // TPP treats .xml.gz as a native format - if (_supportGZ.booleanValue() && name.equals(basename + suffix+".gz")) + if (_supportGZ.booleanValue() && normalizedName.equals(normalizedBase + ".gz")) { return true; } @@ -620,7 +646,7 @@ public boolean equals(Object o) if (!Objects.equals(_defaultSuffix, fileType._defaultSuffix)) return false; if (!Objects.equals(_antiTypes, fileType._antiTypes)) return false; - return !(!Objects.equals(_suffixes, fileType._suffixes)); + return Objects.equals(_suffixes, fileType._suffixes); } public String getDefaultSuffix() @@ -649,6 +675,7 @@ public String toString() return (_dir == null || !_dir.booleanValue() ? _suffixes.toString() : _suffixes + "/"); } + /** The subset of types that at least one of the files matches, in the order given by {@code types}. */ @NotNull public static List findTypes(@NotNull List types, @NotNull List files) { @@ -689,6 +716,7 @@ public void setExtensionsMutuallyExclusive(boolean extensionsMutuallyExclusive) _extensionsMutuallyExclusive = extensionsMutuallyExclusive; } + /** The default suffix with everything through the first dot removed (".pep.xml" yields "pep.xml"); used as a pipeline input/output role name. */ public String getDefaultRole() { if (_defaultSuffix.contains(".")) @@ -703,6 +731,7 @@ public boolean isCaseSensitiveOnCaseSensitiveFileSystems() return _caseSensitiveOnCaseSensitiveFileSystems; } + /** Opt out of the default case-insensitive suffix matching, deferring to the file system: on a case-insensitive one (Windows, default macOS) matching stays case-insensitive. */ public void setCaseSensitiveOnCaseSensitiveFileSystems(boolean caseSensitiveOnCaseSensitiveFileSystems) { _caseSensitiveOnCaseSensitiveFileSystems = caseSensitiveOnCaseSensitiveFileSystems; @@ -754,7 +783,51 @@ public void test() assertFalse(ftt.isType("test.foo.bar")); assertTrue(ftt.isType("test.foo")); assertTrue(ftt.isType("test.bar")); + } + @Test + public void testCaseInsensitiveBaseName() + { + FileType ft = new FileType(Arrays.asList(".foo", ".bar"), ".foo", gzSupportLevel.SUPPORT_GZ); + + // getBaseName() must handle everything isType() accepts, including mixed case with .gz + assertEquals("test", ft.getBaseName(Path.of("test.foo"))); + assertEquals("test", ft.getBaseName(Path.of("test.FOO"))); + assertEquals("test", ft.getBaseName(Path.of("test.foo.gz"))); + assertEquals("test", ft.getBaseName(Path.of("test.FOO.gz"))); + assertEquals("test", ft.getBaseName(Path.of("test.bAr.gZ"))); + + // strongest match wins regardless of case + FileType ftPrefix = new FileType(Arrays.asList(".mzxml", ".msprefix.mzxml"), ".mzxml", gzSupportLevel.SUPPORT_GZ); + assertEquals("test", ftPrefix.getBaseName(Path.of("test.MSPREFIX.mzXML"))); + assertEquals("test", ftPrefix.getBaseName(Path.of("test.MSPREFIX.mzXML.GZ"))); + + // the suffix itself may be mixed case, as massSpecDataFileType's ".mzXML" is + FileType ftMixedSuffix = new FileType(".mzXML", gzSupportLevel.SUPPORT_GZ); + assertEquals("test", ftMixedSuffix.getBaseName(Path.of("test.mzXML"))); + assertEquals("test", ftMixedSuffix.getBaseName(Path.of("test.mzxml"))); + assertEquals("test", ftMixedSuffix.getBaseName(Path.of("test.mzxml.gz"))); + assertEquals("test", ftMixedSuffix.getBaseName(Path.of("test.MZXML.GZ"))); + assertTrue(ftMixedSuffix.isMatch("test.mzxml.gz", "test")); + + // no match at all still returns the original file name + assertEquals("test.unrelated", ft.getBaseName(Path.of("test.unrelated"))); + + // isMatch() follows the same case rules as isType(), for both the plain and the .gz forms + assertTrue(ft.isMatch("test.FOO", "test")); + assertTrue(ft.isMatch("test.FOO.gz", "test")); + assertTrue(ft.isMatch("test.bAr.gZ", "test")); + assertFalse(ft.isMatch("test.unrelated", "test")); + + // A type that opts into case sensitivity still agrees with itself on either kind of file system: + // isType(), getBaseName() and isMatch() all accept the file, or none of them do + FileType ftCase = new FileType(".foo", gzSupportLevel.SUPPORT_GZ); + ftCase.setCaseSensitiveOnCaseSensitiveFileSystems(true); + String mixedCase = "test.FOO.gz"; + boolean accepted = ftCase.isType(mixedCase); + assertEquals(accepted ? "test" : mixedCase, ftCase.getBaseName(Path.of(mixedCase))); + assertEquals(accepted, ftCase.isMatch(mixedCase, "test")); + assertTrue(ftCase.isMatch("test.foo.gz", "test")); } } } diff --git a/api/src/org/labkey/api/util/XmlBeansUtil.java b/api/src/org/labkey/api/util/XmlBeansUtil.java index 69cbf03e430..19a0fe2f2ce 100644 --- a/api/src/org/labkey/api/util/XmlBeansUtil.java +++ b/api/src/org/labkey/api/util/XmlBeansUtil.java @@ -1,200 +1,605 @@ -/* - * Copyright (c) 2009-2026 LabKey Corporation - * - * Licensed 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.labkey.api.util; - -import org.apache.xmlbeans.XmlCursor; -import org.apache.xmlbeans.XmlError; -import org.apache.xmlbeans.XmlException; -import org.apache.xmlbeans.XmlObject; -import org.apache.xmlbeans.XmlOptions; -import org.apache.xmlbeans.XmlTokenSource; -import org.jetbrains.annotations.Nullable; -import org.labkey.api.data.Container; -import org.labkey.api.portal.ProjectUrls; -import org.labkey.api.security.User; -import org.labkey.api.settings.LookAndFeelProperties; -import org.xml.sax.SAXException; - -import javax.xml.XMLConstants; -import javax.xml.parsers.DocumentBuilderFactory; -import javax.xml.parsers.ParserConfigurationException; -import javax.xml.parsers.SAXParserFactory; -import javax.xml.stream.XMLInputFactory; -import java.util.Collection; -import java.util.Date; -import java.util.LinkedList; - -public class XmlBeansUtil -{ - private XmlBeansUtil() - { - } - - // Standard options used by folder export - public static XmlOptions getDefaultSaveOptions() - { - XmlOptions options = new XmlOptions(); - options.setSavePrettyPrint(); - options.setUseDefaultNamespace(); - options.setCharacterEncoding("UTF-8"); - options.setSaveCDataEntityCountThreshold(0); - options.setSaveCDataLengthThreshold(0); - options.setSaveAggressiveNamespaces(); // causes the saver to reduce the number of namespace declarations - - return options; - } - - // Standard options used for parsing to enable validation. - public static XmlOptions getDefaultParseOptions() - { - XmlOptions options = new XmlOptions(); - options.setLoadLineNumbers(); - - return options; - } - - @Deprecated // Use the version below, and pass in details (filename, etc.) - public static void validateXmlDocument(XmlObject doc) throws XmlValidationException - { - validateXmlDocument(doc, null); - } - - // Details can be filename, etc. to help admin narrow down the source of the problem - public static void validateXmlDocument(XmlObject doc, @Nullable String details) throws XmlValidationException - { - XmlOptions options = getDefaultParseOptions(); - Collection errorList = new LinkedList<>(); - options.setErrorListener(errorList); - - if (!doc.validate(options)) - throw new XmlValidationException(errorList, doc.schemaType().toString(), details); - } - - public static String getErrorMessage(XmlException ex) - { - if (ex.getError() != null) - return getErrorMessage(ex.getError()); - return ex.getMessage(); - } - - public static String getErrorMessage(XmlError error) - { - StringBuilder sb = new StringBuilder(); - sb.append(error.toString()); - if (error.getLine() > 0) - { - sb.append(" (line ").append(error.getLine()); - if (error.getColumn() > 0) - sb.append(", column ").append(error.getColumn()); - sb.append(")"); - } - return sb.toString(); - } - - // Insert standard export comment explaining where the data lives, who exported it, and when - public static void addStandardExportComment(XmlTokenSource doc, Container c, User user) - { - String urlString = PageFlowUtil.urlProvider(ProjectUrls.class).getBeginURL(c).getURIString(); - if (urlString.endsWith("?")) - urlString = urlString.substring(0, urlString.length() - 1); - String shortName = LookAndFeelProperties.getInstance(c).getShortName(); - String comment = "Exported from " + shortName + " at " + urlString + " by " + user.getFriendlyName() + " on " + new Date(); - addComment(doc, comment); - } - - public static void addComment(XmlTokenSource doc, String comment) - { - try (XmlCursor cursor = doc.newCursor()) - { - cursor.insertComment(comment); - } - } - - /** - * XML parsing factories preconfigured to prevent XML external entity references (XXE). - * These are static and are unfortunately mutable. We could switch to a factory pattern to create - * freshly configured factories. - */ - public static final SAXParserFactory SAX_PARSER_FACTORY; - public static final SAXParserFactory SAX_PARSER_FACTORY_ALLOWING_DOCTYPE; - public static final XMLInputFactory XML_INPUT_FACTORY; - public static final DocumentBuilderFactory DOCUMENT_BUILDER_FACTORY; - public static final DocumentBuilderFactory DOCUMENT_BUILDER_FACTORY_ALLOWING_DOCTYPE; - - static - { - //noinspection XMLInputFactory - XML_INPUT_FACTORY = XMLInputFactory.newInstance(); - XML_INPUT_FACTORY.setProperty(XMLInputFactory.SUPPORT_DTD, false); - XML_INPUT_FACTORY.setProperty(XMLInputFactory.IS_SUPPORTING_EXTERNAL_ENTITIES, false); - - try - { - SAX_PARSER_FACTORY = saxParserFactory(false); - SAX_PARSER_FACTORY_ALLOWING_DOCTYPE = saxParserFactory(true); - - DOCUMENT_BUILDER_FACTORY = documentBuilderFactory(false); - // Use the ALLOWING_DOCTYPE variant when parsing XML that contains a declaration (e.g. NCBI's eSummary responses) - DOCUMENT_BUILDER_FACTORY_ALLOWING_DOCTYPE = documentBuilderFactory(true); - } - catch (ParserConfigurationException | SAXException e) - { - throw UnexpectedException.wrap(e); - } - } - - private static SAXParserFactory saxParserFactory(boolean allowDocType) throws SAXException, ParserConfigurationException - { - //noinspection XMLInputFactory - SAXParserFactory result = SAXParserFactory.newInstance(); - result.setNamespaceAware(true); - result.setFeature("http://xml.org/sax/features/validation", false); - result.setFeature("http://apache.org/xml/features/nonvalidating/load-external-dtd", false); - - // Disable features that could lead to XXE or other vulnerabilities - // Keep in sync with ModuleArchive.nameFromModuleXML() - if (!allowDocType) - { - result.setFeature("http://apache.org/xml/features/disallow-doctype-decl", true); - } - result.setFeature("http://xml.org/sax/features/external-general-entities", false); - result.setFeature("http://xml.org/sax/features/external-parameter-entities", false); - result.setFeature(XMLConstants.FEATURE_SECURE_PROCESSING, true); - return result; - } - - private static DocumentBuilderFactory documentBuilderFactory(boolean allowDocType) throws ParserConfigurationException - { - //noinspection XMLInputFactory - DocumentBuilderFactory result = DocumentBuilderFactory.newInstance(); - result.setNamespaceAware(true); - - // Disable features that could lead to XXE or other vulnerabilities. - // When allowDocType is true the DOCTYPE declaration is permitted. External entity - // resolution remains disabled, so XXE protection is still in effect. - if (!allowDocType) - { - result.setFeature("http://apache.org/xml/features/disallow-doctype-decl", true); - } - result.setFeature("http://xml.org/sax/features/external-general-entities", false); - result.setFeature("http://xml.org/sax/features/external-parameter-entities", false); - result.setFeature("http://apache.org/xml/features/nonvalidating/load-external-dtd", false); - result.setFeature(XMLConstants.FEATURE_SECURE_PROCESSING, true); - result.setXIncludeAware(false); - result.setExpandEntityReferences(false); - return result; - } -} +/* + * Copyright (c) 2009-2026 LabKey Corporation + * + * Licensed 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.labkey.api.util; + +import org.apache.logging.log4j.Logger; +import org.apache.xmlbeans.XmlCursor; +import org.apache.xmlbeans.XmlError; +import org.apache.xmlbeans.XmlException; +import org.apache.xmlbeans.XmlObject; +import org.apache.xmlbeans.XmlOptions; +import org.apache.xmlbeans.XmlTokenSource; +import org.jetbrains.annotations.Nullable; +import org.junit.Assert; +import org.junit.Test; +import org.labkey.api.data.Container; +import org.labkey.api.portal.ProjectUrls; +import org.labkey.api.security.User; +import org.labkey.api.settings.LookAndFeelProperties; +import org.labkey.api.util.logging.LogHelper; +import org.w3c.dom.ls.LSInput; +import org.w3c.dom.ls.LSResourceResolver; +import org.xml.sax.SAXException; +import org.xml.sax.SAXNotRecognizedException; +import org.xml.sax.SAXNotSupportedException; + +import javax.xml.XMLConstants; +import javax.xml.parsers.DocumentBuilderFactory; +import javax.xml.parsers.ParserConfigurationException; +import javax.xml.parsers.SAXParserFactory; +import javax.xml.stream.XMLInputFactory; +import javax.xml.transform.stream.StreamSource; +import javax.xml.validation.Schema; +import javax.xml.validation.SchemaFactory; +import javax.xml.validation.Validator; +import java.io.File; +import java.io.FileOutputStream; +import java.io.IOException; +import java.io.InputStream; +import java.io.Reader; +import java.io.StringReader; +import java.net.URI; +import java.net.URL; +import java.nio.charset.StandardCharsets; +import java.nio.file.Files; +import java.nio.file.Path; +import java.util.Collection; +import java.util.Date; +import java.util.LinkedList; +import java.util.function.Function; +import java.util.jar.JarEntry; +import java.util.jar.JarOutputStream; + +public class XmlBeansUtil +{ + private static final Logger LOG = LogHelper.getLogger(XmlBeansUtil.class, "XML schema and validator XXE hardening"); + + private XmlBeansUtil() + { + } + + // Standard options used by folder export + public static XmlOptions getDefaultSaveOptions() + { + XmlOptions options = new XmlOptions(); + options.setSavePrettyPrint(); + options.setUseDefaultNamespace(); + options.setCharacterEncoding("UTF-8"); + options.setSaveCDataEntityCountThreshold(0); + options.setSaveCDataLengthThreshold(0); + options.setSaveAggressiveNamespaces(); // causes the saver to reduce the number of namespace declarations + + return options; + } + + // Standard options used for parsing to enable validation. + public static XmlOptions getDefaultParseOptions() + { + XmlOptions options = new XmlOptions(); + options.setLoadLineNumbers(); + + return options; + } + + @Deprecated // Use the version below, and pass in details (filename, etc.) + public static void validateXmlDocument(XmlObject doc) throws XmlValidationException + { + validateXmlDocument(doc, null); + } + + // Details can be filename, etc. to help admin narrow down the source of the problem + public static void validateXmlDocument(XmlObject doc, @Nullable String details) throws XmlValidationException + { + XmlOptions options = getDefaultParseOptions(); + Collection errorList = new LinkedList<>(); + options.setErrorListener(errorList); + + if (!doc.validate(options)) + throw new XmlValidationException(errorList, doc.schemaType().toString(), details); + } + + public static String getErrorMessage(XmlException ex) + { + if (ex.getError() != null) + return getErrorMessage(ex.getError()); + return ex.getMessage(); + } + + public static String getErrorMessage(XmlError error) + { + StringBuilder sb = new StringBuilder(); + sb.append(error.toString()); + if (error.getLine() > 0) + { + sb.append(" (line ").append(error.getLine()); + if (error.getColumn() > 0) + sb.append(", column ").append(error.getColumn()); + sb.append(")"); + } + return sb.toString(); + } + + // Insert standard export comment explaining where the data lives, who exported it, and when + public static void addStandardExportComment(XmlTokenSource doc, Container c, User user) + { + String urlString = PageFlowUtil.urlProvider(ProjectUrls.class).getBeginURL(c).getURIString(); + if (urlString.endsWith("?")) + urlString = urlString.substring(0, urlString.length() - 1); + String shortName = LookAndFeelProperties.getInstance(c).getShortName(); + String comment = "Exported from " + shortName + " at " + urlString + " by " + user.getFriendlyName() + " on " + new Date(); + addComment(doc, comment); + } + + public static void addComment(XmlTokenSource doc, String comment) + { + try (XmlCursor cursor = doc.newCursor()) + { + cursor.insertComment(comment); + } + } + + /** + * XML parsing factories preconfigured to prevent XML external entity references (XXE). + * These are static and are unfortunately mutable. We could switch to a factory pattern to create + * freshly configured factories. + */ + public static final SAXParserFactory SAX_PARSER_FACTORY; + public static final SAXParserFactory SAX_PARSER_FACTORY_ALLOWING_DOCTYPE; + public static final XMLInputFactory XML_INPUT_FACTORY; + public static final DocumentBuilderFactory DOCUMENT_BUILDER_FACTORY; + public static final DocumentBuilderFactory DOCUMENT_BUILDER_FACTORY_ALLOWING_DOCTYPE; + + static + { + //noinspection XMLInputFactory + XML_INPUT_FACTORY = XMLInputFactory.newInstance(); + XML_INPUT_FACTORY.setProperty(XMLInputFactory.SUPPORT_DTD, false); + XML_INPUT_FACTORY.setProperty(XMLInputFactory.IS_SUPPORTING_EXTERNAL_ENTITIES, false); + + try + { + SAX_PARSER_FACTORY = saxParserFactory(false); + SAX_PARSER_FACTORY_ALLOWING_DOCTYPE = saxParserFactory(true); + + DOCUMENT_BUILDER_FACTORY = documentBuilderFactory(false); + // Use the ALLOWING_DOCTYPE variant when parsing XML that contains a declaration (e.g. NCBI's eSummary responses) + DOCUMENT_BUILDER_FACTORY_ALLOWING_DOCTYPE = documentBuilderFactory(true); + } + catch (ParserConfigurationException | SAXException e) + { + throw UnexpectedException.wrap(e); + } + } + + private static SAXParserFactory saxParserFactory(boolean allowDocType) throws SAXException, ParserConfigurationException + { + //noinspection XMLInputFactory + SAXParserFactory result = SAXParserFactory.newInstance(); + result.setNamespaceAware(true); + result.setFeature("http://xml.org/sax/features/validation", false); + result.setFeature("http://apache.org/xml/features/nonvalidating/load-external-dtd", false); + + // Disable features that could lead to XXE or other vulnerabilities + // Keep in sync with ModuleArchive.nameFromModuleXML() + if (!allowDocType) + { + result.setFeature("http://apache.org/xml/features/disallow-doctype-decl", true); + } + result.setFeature("http://xml.org/sax/features/external-general-entities", false); + result.setFeature("http://xml.org/sax/features/external-parameter-entities", false); + result.setFeature(XMLConstants.FEATURE_SECURE_PROCESSING, true); + return result; + } + + private static DocumentBuilderFactory documentBuilderFactory(boolean allowDocType) throws ParserConfigurationException + { + //noinspection XMLInputFactory + DocumentBuilderFactory result = DocumentBuilderFactory.newInstance(); + result.setNamespaceAware(true); + + // Disable features that could lead to XXE or other vulnerabilities. + // When allowDocType is true the DOCTYPE declaration is permitted. External entity + // resolution remains disabled, so XXE protection is still in effect. + if (!allowDocType) + { + result.setFeature("http://apache.org/xml/features/disallow-doctype-decl", true); + } + result.setFeature("http://xml.org/sax/features/external-general-entities", false); + result.setFeature("http://xml.org/sax/features/external-parameter-entities", false); + result.setFeature("http://apache.org/xml/features/nonvalidating/load-external-dtd", false); + result.setFeature(XMLConstants.FEATURE_SECURE_PROCESSING, true); + result.setXIncludeAware(false); + result.setExpandEntityReferences(false); + return result; + } + + /** + * A {@link SchemaFactory} hardened against XXE (CWE-611). Not thread-safe, so get a fresh instance per call. + * The schema document itself must still be trusted: local file: and jar: references stay enabled so bundled schemas + * can import their sibling + */ + public static SchemaFactory schemaFactory() + { + //noinspection SchemaFactory + SchemaFactory factory = SchemaFactory.newInstance(XMLConstants.W3C_XML_SCHEMA_NS_URI); + require(() -> factory.setFeature(XMLConstants.FEATURE_SECURE_PROCESSING, true)); + // Xerces rejects both, but set for the JDK implementation + attempt(() -> factory.setProperty(XMLConstants.ACCESS_EXTERNAL_DTD, ""), XMLConstants.ACCESS_EXTERNAL_DTD); + attempt(() -> factory.setProperty(XMLConstants.ACCESS_EXTERNAL_SCHEMA, "file,jar"), XMLConstants.ACCESS_EXTERNAL_SCHEMA); + // Bundled schemas import sibling XSDs, so local references must still resolve + factory.setResourceResolver(LOCAL_ONLY_RESOLVER); + return factory; + } + + // The Validator resolves entities in the instance document independently of the SchemaFactory, so it must be locked down separately. + public static Validator hardenValidator(Validator validator) + { + require(() -> validator.setFeature(XMLConstants.FEATURE_SECURE_PROCESSING, true)); + // Xerces accepts these but never applies them to StreamSource input: StreamValidatorHelper builds its own XML11Configuration and copies properties, not features + attempt(() -> validator.setFeature("http://xml.org/sax/features/external-general-entities", false), "external-general-entities"); + attempt(() -> validator.setFeature("http://xml.org/sax/features/external-parameter-entities", false), "external-parameter-entities"); + attempt(() -> validator.setFeature("http://apache.org/xml/features/nonvalidating/load-external-dtd", false), "load-external-dtd"); + attempt(() -> validator.setProperty(XMLConstants.ACCESS_EXTERNAL_DTD, ""), XMLConstants.ACCESS_EXTERNAL_DTD); + attempt(() -> validator.setProperty(XMLConstants.ACCESS_EXTERNAL_SCHEMA, ""), XMLConstants.ACCESS_EXTERNAL_SCHEMA); + // Stored as the ENTITY_RESOLVER property, which that copy does carry across, so this is what actually blocks XXE under Xerces + validator.setResourceResolver(REFUSE_ALL_RESOLVER); + return validator; + } + + /** Resolves to nothing, so a refused reference expands to the empty string instead of being fetched. */ + private static final LSInput EMPTY_INPUT = new LSInput() + { + @Override public Reader getCharacterStream() { return new StringReader(""); } + @Override public void setCharacterStream(Reader characterStream) { } + @Override public InputStream getByteStream() { return null; } + @Override public void setByteStream(InputStream byteStream) { } + @Override public String getStringData() { return ""; } + @Override public void setStringData(String stringData) { } + @Override public String getSystemId() { return null; } + @Override public void setSystemId(String systemId) { } + @Override public String getPublicId() { return null; } + @Override public void setPublicId(String publicId) { } + @Override public String getBaseURI() { return null; } + @Override public void setBaseURI(String baseURI) { } + @Override public String getEncoding() { return StandardCharsets.UTF_8.name(); } + @Override public void setEncoding(String encoding) { } + @Override public boolean getCertifiedText() { return false; } + @Override public void setCertifiedText(boolean certifiedText) { } + }; + + /** The instance document is the attacker-supplied half of every call site, and never legitimately references anything external. */ + private static final LSResourceResolver REFUSE_ALL_RESOLVER = (_, _, _, systemId, _) -> { + LOG.warn("Refused external reference to {} while validating XML", systemId); + return EMPTY_INPUT; + }; + + /** Debug rather than warn: bundled xenc-schema-11.xsd still declares a w3.org DOCTYPE, so this fires on every SAML schema compile. */ + private static final LSResourceResolver LOCAL_ONLY_RESOLVER = (_, _, _, systemId, baseURI) -> { + if (isLocal(systemId, baseURI)) + return null; // fall through to the default resolver + LOG.debug("Refused non-local schema reference to {} while compiling XML schema", systemId); + return EMPTY_INPUT; + }; + + /** A jar: base URI is opaque, so {@link URI#resolve} leaves a relative reference untouched -- it stays scheme-less, which counts as local. */ + private static boolean isLocal(@Nullable String systemId, @Nullable String baseURI) + { + if (systemId == null) + return true; + + try + { + URI uri = URI.create(systemId); + if (!uri.isAbsolute() && baseURI != null) + uri = URI.create(baseURI).resolve(uri); + return isLocalScheme(uri); + } + catch (IllegalArgumentException e) + { + return false; // unparseable, so not something we're willing to vouch for + } + } + + private static boolean isLocalScheme(URI uri) + { + String scheme = uri.getScheme(); + + if (scheme == null) + return true; + + // A protocol-relative "//host/x.xsd" resolves against a file: base into file://host/x.xsd, which Windows maps + // to a UNC path and fetches over SMB. Java emits file:/path and file:///path, both of which parse to a null + // authority, so requiring that costs nothing. + if ("file".equalsIgnoreCase(scheme)) + { + String authority = uri.getAuthority(); + return authority == null || "localhost".equalsIgnoreCase(authority); + } + + // jar:!/ is opaque, so the scheme above says nothing about what gets fetched -- the inner URL does + if ("jar".equalsIgnoreCase(scheme)) + { + String ssp = uri.getSchemeSpecificPart(); + int separator = ssp.indexOf("!/"); + return isLocalScheme(URI.create(separator < 0 ? ssp : ssp.substring(0, separator))); + } + + return false; + } + + @FunctionalInterface + private interface XmlSetting + { + void apply() throws SAXException; + } + + // FEATURE_SECURE_PROCESSING is honored by every JAXP implementation, so failure to set it is fatal. + private static void require(XmlSetting setting) + { + try + { + setting.apply(); + } + catch (SAXException e) + { + throw UnexpectedException.wrap(e); + } + } + + // Implementations recognize different subsets of these controls (Xerces rejects the accessExternal* properties, XERCESJ-1654), so skip a rejected setting rather than aborting the rest. + private static void attempt(XmlSetting setting, String name) + { + try + { + setting.apply(); + } + catch (SAXNotRecognizedException | SAXNotSupportedException e) + { + LOG.debug("XML implementation does not recognize {}; relying on the other hardening settings", name); + } + catch (SAXException e) + { + throw UnexpectedException.wrap(e); + } + } + + /** + * Covers the XXE (CWE-611) contract of {@link #schemaFactory()} and {@link #hardenValidator(Validator)}, asserting + * on observable behavior rather than on which properties were set: a real server resolves Xerces instead of the + * JDK's JAXP, the two honor different subsets of the controls, and {@link #attempt} swallows the rejections -- so + * the hardening can compile, look correct, and do nothing. + */ + public static class TestCase extends Assert + { + private static final String TRIVIAL_XSD = + "" + + "" + + ""; + + // ---------- hardenValidator(): instance-document XXE ---------- + + @Test + public void hardenedValidatorRefusesExternalDtdSubset() throws Exception + { + assertHardenedValidatorRefuses("external DTD subset", probe -> + "" + + "hello"); + } + + @Test + public void hardenedValidatorRefusesExternalGeneralEntity() throws Exception + { + assertHardenedValidatorRefuses("external general entity", probe -> + "]>" + + "&x;"); + } + + @Test + public void hardenedValidatorRefusesExternalParameterEntity() throws Exception + { + assertHardenedValidatorRefuses("external parameter entity", probe -> + "") + "\">%p;]>" + + "hello"); + } + + /** + * Harness self-check: without it a URL typo would make the three tests above pass with no protection in place. + * If the platform default ever becomes safe on its own, delete this rather than weakening those assertions. + */ + @Test + public void probeDetectsFetchWhenValidatorIsNotHardened() throws Exception + { + try (ExternalReferenceProbe probe = ExternalReferenceProbe.start()) + { + String xml = "" + + "hello"; + //noinspection SchemaFactory + SchemaFactory raw = SchemaFactory.newInstance(XMLConstants.W3C_XML_SCHEMA_NS_URI); + Validator validator = raw.newSchema(new StreamSource(new StringReader(TRIVIAL_XSD))).newValidator(); + validateIgnoringErrors(validator, xml); + + assertTrue("Probe must observe a fetch from an unhardened validator, otherwise the hardening " + + "tests in this class prove nothing", probe.wasContacted()); + } + } + + // ---------- schemaFactory(): schema-document XXE ---------- + + @Test + public void schemaFactoryRefusesExternalDoctypeInSchemaDocument() throws Exception + { + try (ExternalReferenceProbe probe = ExternalReferenceProbe.start()) + { + String xsd = "") + "\">" + TRIVIAL_XSD; + compileIgnoringErrors(schemaFactory(), new StreamSource(new StringReader(xsd))); + + probe.assertNotContacted("schemaFactory() must not fetch a DOCTYPE declared by a schema document"); + } + } + + @Test + public void schemaFactoryRefusesRemoteImport() throws Exception + { + try (ExternalReferenceProbe probe = ExternalReferenceProbe.start()) + { + compileIgnoringErrors(schemaFactory(), new StreamSource(new StringReader(remoteImportSchema(probe)))); + + probe.assertNotContacted("schemaFactory() must not fetch a remote xs:import"); + } + } + + /** + * Harness self-check for the two tests above, mirroring {@link #probeDetectsFetchWhenValidatorIsNotHardened}: + * a compile that aborts before it ever walks the import graph satisfies assertNotContacted while proving nothing. + */ + @Test + public void probeDetectsFetchWhenSchemaFactoryIsNotHardened() throws Exception + { + try (ExternalReferenceProbe probe = ExternalReferenceProbe.start()) + { + //noinspection SchemaFactory + SchemaFactory raw = SchemaFactory.newInstance(XMLConstants.W3C_XML_SCHEMA_NS_URI); + compileIgnoringErrors(raw, new StreamSource(new StringReader(remoteImportSchema(probe)))); + + assertTrue("Probe must observe a fetch from an unhardened factory, otherwise the schemaFactory() " + + "tests in this class prove nothing", probe.wasContacted()); + } + } + + // ---------- over-blocking guards ---------- + // Bundled schemas compose sibling XSDs by relative path, from a jar: URL when deployed and a file: URL in a dev build. + + @Test + public void schemaFactoryCompilesLocalImportChainFromFileUrl() throws Exception + { + Path dir = Files.createTempDirectory("xmlBeansUtilFile"); + + try + { + writeSchemaPair(dir); + Schema schema = schemaFactory().newSchema(dir.resolve("main.xsd").toUri().toURL()); + + assertNotNull("A local, relative xs:import must still resolve from a file: URL", schema); + } + finally + { + FileUtil.deleteDir(dir.toFile()); + } + } + + @Test + public void schemaFactoryCompilesLocalImportChainFromJarUrl() throws Exception + { + Path dir = Files.createTempDirectory("xmlBeansUtilJar"); + + try + { + writeSchemaPair(dir); + File jar = FileUtil.appendName(dir.toFile(), "schemas.jar"); + + try (JarOutputStream out = new JarOutputStream(new FileOutputStream(jar))) + { + for (String name : new String[]{"main.xsd", "imported.xsd"}) + { + out.putNextEntry(new JarEntry("schemas/" + name)); + out.write(Files.readAllBytes(dir.resolve(name))); + out.closeEntry(); + } + } + + URL url = URI.create("jar:" + jar.toURI() + "!/schemas/main.xsd").toURL(); + Schema schema = schemaFactory().newSchema(url); + + assertNotNull("A local, relative xs:import must still resolve from a jar: URL, which is how " + + "schemas are loaded in a deployed server", schema); + } + finally + { + FileUtil.deleteDir(dir.toFile()); + } + } + + // ---------- helpers ---------- + + private void assertHardenedValidatorRefuses(String vector, Function instanceDoc) throws Exception + { + try (ExternalReferenceProbe probe = ExternalReferenceProbe.start()) + { + String xml = instanceDoc.apply(probe); + Validator validator = hardenValidator(schemaFactory() + .newSchema(new StreamSource(new StringReader(TRIVIAL_XSD))) + .newValidator()); + validateIgnoringErrors(validator, xml); + + probe.assertNotContacted("hardenValidator() must not resolve the " + vector + " of an instance document"); + } + } + + /** Whether the document is schema-valid is beside the point; the fetch is the signal. */ + private void validateIgnoringErrors(Validator validator, String xml) throws IOException + { + try + { + validator.validate(new StreamSource(new StringReader(xml))); + } + catch (SAXException ignored) + { + } + } + + /** A schema whose only external reference is a remote xs:import pointed at the probe. */ + private String remoteImportSchema(ExternalReferenceProbe probe) + { + String remote = probe.url("/remote.xsd", + ""); + + return "" + + "" + + "" + + ""; + } + + /** Likewise: a refused reference may or may not surface as a compile error. */ + private void compileIgnoringErrors(SchemaFactory factory, StreamSource source) + { + try + { + factory.newSchema(source); + } + catch (SAXException ignored) + { + } + } + + /** The relative-import shape every bundled LabKey schema chain uses. */ + private void writeSchemaPair(Path dir) throws IOException + { + Files.writeString(dir.resolve("imported.xsd"), + "" + + "" + + "", StandardCharsets.UTF_8); + + Files.writeString(dir.resolve("main.xsd"), + "" + + "" + + "" + + "" + + "" + + "", StandardCharsets.UTF_8); + } + } +} diff --git a/api/src/org/labkey/api/view/TooManyRequestsException.java b/api/src/org/labkey/api/view/TooManyRequestsException.java new file mode 100644 index 00000000000..2fefb0d4d04 --- /dev/null +++ b/api/src/org/labkey/api/view/TooManyRequestsException.java @@ -0,0 +1,41 @@ +/* + * Copyright (c) 2026 LabKey Corporation + * + * Licensed 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.labkey.api.view; + +/** + * The server is refusing to handle this request right now because too many similar requests are already in flight. + * Rendered as an HTTP 429 with a {@code Retry-After} header telling the client how long to wait. + * + * @see org.labkey.api.action.ConcurrencyLimit + */ +public class TooManyRequestsException extends HttpStatusException +{ + public static final int SC_TOO_MANY_REQUESTS = 429; + + private final int _retryAfterSeconds; + + public TooManyRequestsException(String message, int retryAfterSeconds) + { + super(message, null, SC_TOO_MANY_REQUESTS); + _retryAfterSeconds = retryAfterSeconds; + } + + /** @return the number of seconds to advertise in the {@code Retry-After} response header */ + public int getRetryAfterSeconds() + { + return _retryAfterSeconds; + } +} diff --git a/core/src/org/labkey/core/CoreMcp.java b/core/src/org/labkey/core/CoreMcp.java index d7b49652b6b..cfd39dc0292 100644 --- a/core/src/org/labkey/core/CoreMcp.java +++ b/core/src/org/labkey/core/CoreMcp.java @@ -222,4 +222,22 @@ public ReadResourceResult getRDataAnalysisGuide() throws IOException ) )); } + + @McpResource( + uri = "resource://org/labkey/core/Reports.md", + mimeType = "application/markdown", + name = "LabKey Reports: Converting a Script Guide", + description = "Required reading before converting an R or Python/Jupyter analysis script into a saved LabKey Report. Covers data-bound vs standalone reports, the R substitution-token/knitr model, the Jupyter report_config.json/ReportConfig model, report authorization, and UI-saved vs file-based module reports.") + public ReadResourceResult getReportsGuide() throws IOException + { + incrementResourceRequestCount("Reports"); + String markdown = IOUtils.resourceToString("org/labkey/core/Reports.md", null, CoreModule.class.getClassLoader()); + return new ReadResourceResult(List.of( + new McpSchema.TextResourceContents( + "resource://org/labkey/core/Reports.md", + "application/markdown", + markdown + ) + )); + } } diff --git a/core/src/org/labkey/core/Reports.md b/core/src/org/labkey/core/Reports.md new file mode 100644 index 00000000000..6f3f1b1d5c8 --- /dev/null +++ b/core/src/org/labkey/core/Reports.md @@ -0,0 +1,275 @@ +# Converting a Script into a LabKey Report + +This guide is the delta between "a script that talks to LabKey over HTTP" and "a script that runs *inside* LabKey as a saved Report." Read `DataAnalysis_Python.md` or `DataAnalysis_R.md` first to build the analysis script (external execution model: explicit connection, API key, `select_rows`/`labkey.selectRows` HTTP calls). This guide covers what changes when that script becomes a Report (server-side execution model: injected context, no connection code, viewing-user permissions). See `FileBasedModules.md` for general file-based module structure — this guide goes deeper on the report-specific pieces it only summarizes. + +Jump to the track that matches your language: +- R → [R Report Track](#r-report-track) +- Python/Jupyter → [Python / Jupyter Report Track](#python--jupyter-report-track) + +Everything else on this page (data-bound vs. not, authorization, UI vs. file-based module) applies to both languages. + +## Report Type Landscape + +LabKey has several built-in report types: Query Report (renders a query view, no script), Attachment Report (uploaded static document), Link Report (URL pointer), JavaScript Report (runs in the *viewer's browser*, not server-side), R Report, Jupyter Report, and Query Snapshot (a persisted table, not really a "report"). **R Reports** and **Jupyter Reports** are the two paths this guide covers for turning an analyst-authored script into a server-side report. (A generic `ExternalScriptEngineReport`/`InternalScriptEngineReport` mechanism also exists for other JSR223-compatible engines an admin configures — historically used for Perl — but there's no conversion track for it here.) + +## Data-Bound vs. Not Data-Bound + +A report is "data-bound" if its descriptor has a `schemaName` property (usually with `queryName` and optionally `viewName`). This one property controls everything about whether a query result set gets piped into the script: + +- **Data-bound**: LabKey runs the query as the *viewing user*. For **R**, the result set is streamed into the script's working directory before the script executes, and read automatically into `labkey.data` (see [R Report Track](#r-report-track)). For **Jupyter**, nothing is pre-staged — LabKey only writes a *query descriptor* (schema/query/columns/filters) into `report_config.json`, and `get_report_data()` uses that descriptor to make its own HTTP call back to the server *at cell-execution time*. Don't assume calling it is "free" the way reading `labkey.data` is — it's a live query, subject to the viewing user's live permissions and to query latency, at whatever moment the cell runs (see [Python / Jupyter Report Track](#python--jupyter-report-track)). +- **Not data-bound**: no query result set is ever provided. The script must fetch its own data at runtime (Rlabkey `labkey.selectRows()`, or Jupyter's `get_report_api_wrapper()`) — the same external-API model as `DataAnalysis_Python.md`/`DataAnalysis_R.md`. + +### Setting it via the UI + +- **R Report from a Data Grid** (Charts/Reports menu on a grid) — bound to that grid's query/view/filters. +- **R Report independent of a grid** (⚙ → Manage Views → Add Report → R Report) — not bound; listed under "Uncategorized" in Manage Views. + +### Setting it via a file-based module + +Placing an R script two directories deep under `reports/schemas/` auto-derives the binding from the path — no XML needed: + +``` +resources/reports/schemas///MyReport.r +``` + +Real example from the `ehr` module — `MyReport.r`'s sibling `.report.xml` doesn't even need a `schemaName` property, because the directory position (`schemas/study/Weight/`) already supplies `schemaName=study`, `queryName=Weight`: + +`server/modules/ehrModules/ehr/resources/reports/schemas/study/Weight/Weight Graph.report.xml`: +```xml + + + + query + + +``` + +`server/modules/ehrModules/ehr/resources/reports/schemas/study/Weight/Weight Graph.r` (excerpt) — `labkey.data` is populated purely because of the directory it lives in: +```r +library(lattice); +labkey.data$date = as.Date(labkey.data$date); +size = length(unique(labkey.data$id)); +png(filename="${imgout:graph.png}", width=800, height=(400 * size)); +xyplot(weight ~ date | id, data=labkey.data, layout=c(1,size), xlab="Date", ylab="Weight (kg)"); +dev.off(); +``` + +A script placed directly at `reports/schemas/foo.r` (only one path segment, no schema/query subfolders) gets no binding at all — it's the not-data-bound case. + +**This path-based auto-derivation works for R but NOT for Jupyter.** `.ipynb` files never parse `schemaName`/`queryName` from their directory position — a data-bound Jupyter module report must set both explicitly via `` entries in its `.report.xml` (see [Python / Jupyter Report Track](#python--jupyter-report-track)). + +## Authorization & Execution Model + +Two independent layers govern what happens when a report runs: + +**1. Who can view/edit the report object.** Every report has an access setting — public (readable by anyone with permission on the underlying data), private (creator only), or custom (explicit ACL). Editing a *shared* report someone else created requires the `EditSharedReportPermission` role; editing your own private report just requires ownership. + +**2. What data the script can see at execution time — and this is the critical fact:** + +> **The script always executes under the *viewing* user's LabKey permissions, not the author's.** When a data-bound report runs, LabKey resolves the query using the current viewer's session (`context.getUser()`), so `labkey.data` / `get_report_data()` reflects that viewer's row- and column-level permissions and any custom-view filters — not whoever wrote the script. + +Any live callback the script makes back into LabKey (an Rlabkey API call, or Jupyter's `get_report_api_wrapper()`) carries a short-lived credential scoped to that same viewing session — never a hardcoded credential: +- R: an in-script `labkey.apiKey` variable, resolved per-session. +- Jupyter: `X-LABKEY-APIKEY` / `X-LABKEY-USERID` / `X-LABKEY-EMAIL` HTTP headers sent to the notebook execution service. + +**Authoring gate.** Creating or editing an R or Jupyter report requires the **Trusted Analyst** (a Premium-only role — on a non-Premium build only Platform Developer/Site Admin can satisfy it) or **Platform Developer** role. For **R reports specifically**, if you're not a Platform Developer the script engine must additionally be configured as **sandboxed** (an admin-declared isolation claim — LabKey does not verify it). This gate exists because R script execution is **not sandboxed by default**: an R script can call `system()` and reach the OS shell with the LabKey server process's own privileges, bypassing LabKey's permission model entirely for anything outside the app (file access, network calls, etc.). Treat authoring an R report as granting real code-execution capability, not just a data query. **Jupyter reports have no equivalent sandbox check** — and don't need one, since a Jupyter report never executes in-process on the LabKey host; it always ships the notebook over HTTP to a separate, admin-configured execution service (see [Python / Jupyter Report Track](#python--jupyter-report-track)). + +**`runInBackground`** (a `` on the report descriptor) routes execution through a pipeline job instead of the request thread. This changes *where* the script runs, not *whose* permissions it runs under. **This only works for R reports.** Jupyter reports can't run in the background this way — the "Run in background" option is hidden in the designer for Jupyter reports, and setting the property another way has no effect (the code paths that would honor it all gate on the report being an R report first). + +## R Report Track + +### What's automatically injected + +If the report is data-bound (see above), LabKey prepends a textual prolog to your script before it runs, defining: + +```r +labkey.data # data frame — the query result set (only if data-bound) +labkey.url.base # e.g. "http://localhost:8080/labkey/" +labkey.url.path # container path portion of the current URL +labkey.url.params # list of URL params / applied grid filters, or NULL +labkey.user.email # the viewing user's email +labkey.file.root # absolute path to the container's file root, or NULL +labkey.pipeline.root # absolute path to the pipeline root, or NULL +labkey.apiKey # short-lived API key scoped to the viewing session +labkey.url(controller, action, list) # helper: builds a LabKey URL +labkey.resolveLSID(lsid) # helper: builds a resolveLSID URL +``` + +`quit()` is overridden to raise an error instead of killing the R session — a script that calls `quit()`/`q()` expecting to stop execution will instead see an error, by design. + +Column names in `labkey.data` are lower-cased and sanitized: a bare space becomes an underscore, but most special characters are spelled out rather than collapsed to `_` — e.g. `CD4+` → `cd4_plus_`, and a slash is spelled out too (`Weight/Height` → `weight_fs_height`, not `weight_height`). Run `names(labkey.data)` early when porting a script — don't assume the original column names survive, and don't assume a slash just becomes an underscore. + +### Substitution tokens + +Tokens have the form `${id:name}` and are resolved to real file paths before the script runs (for outputs) or after it finishes (LabKey then renders/serves whatever file was written). **Modern syntax**: put a comment line `#${id:name}` immediately before the line that consumes it: + +```r +#${imgout:graph.png} +png(filename="${imgout:graph.png}"); +plot(labkey.data$x, labkey.data$y); +dev.off(); +``` + +The older bare inline form (`${id:name}` with no leading `#`) still works but is deprecated — prefer the comment-line form in new scripts. + +| Token | Purpose | +|---|---| +| `${input_data}` / `${input_data:name}` | path to the query-result TSV (usually consumed automatically via `labkey.data`, not written by hand) | +| `${tsvout:name}` | write a TSV, rendered inline as an HTML grid | +| `${txtout:name}` | plain text output, rendered inline | +| `${consoleout:name}` | captured console/stdout output | +| `${htmlout:name}` | raw (unescaped) HTML, rendered inline | +| `${svgout:name}` | SVG image, rendered inline | +| `${imgout:name}` | generic raster image (default extension `jpg`) | +| `${jpgout:name}` / `${pngout:name}` | JPEG / PNG image specifically | +| `${pdfout:name}` | downloadable PDF | +| `${psout:name}` | downloadable Postscript | +| `${fileout:name}` | any other downloadable file type | +| `${jsonout:name}` | JSON output | +| `${hrefout:name}` | a link/URL to a produced artifact | +| `${knitrout:name}` | knitr-rendered document | + +Use `regex(...)` inside a token — e.g. `${fileout:regex(.*?\.gct)}` — when the script generates files whose exact names aren't known ahead of time; LabKey maps any file matching the pattern to that output slot. + +A separate set of tokens is substituted both into the engine invocation command line *and* — if you reference them directly — into the script body itself, since both substitution passes share the same replacement map: `${scriptName}`, `${scriptFile}`, `${workingDir}`, `${apikey}`, `${rLabkeySessionId}`, `${httpSessionId}`, `${sessionCookieName}`, `${baseServerURL}`, `${containerPath}`. + +**`${srcDirectory}` does not work for Reports** despite being defined alongside this family — it's only ever populated for assay *transform* scripts, a different feature. If you reference it in a report script (e.g. `source("${srcDirectory}/util.R")`), it will not resolve, and — unlike the command-line substitution pass, which silently strips unmatched tokens — the script-body substitution pass writes it out **verbatim**, so the script fails at runtime trying to open a file literally named `${srcDirectory}/...`. Don't use it when porting a script into an R report. + +### Knitr support (`.rhtml` / `.rmd`) + +Both extensions are valid report script files with the same injected variables and token substitution available inside their chunks: + +- **`.rhtml`** — an HTML page with knitr chunks delimited by HTML comments: + ```html + + ``` +- **`.rmd`** — Markdown with fenced knitr chunks: + ````markdown + ```{r blood-pressure-scatter, echo=FALSE} + plot(labkey.data$diastolicbloodpressure, labkey.data$systolicbloodpressure) + ``` + ```` + +Real examples in the `scriptpad` test module: `server/testAutomation/modules/scriptpad/resources/reports/schemas/script_rhtml.rhtml` and `script_rmd.rmd` both build the same blood-pressure scatter plot from `labkey.data`. + +If a knitr report needs an external JS/CSS library, declare it in the `.report.xml` sidecar's `` block rather than loading it from the script — `kable.rmd`'s sidecar: + +`server/testAutomation/modules/scriptpad/resources/reports/schemas/kable.report.xml`: +```xml + + + + The uncool query + false + + + + + + + + + +``` + +### Batch-mode gotchas when porting a standalone script + +- No interactive session: `library()`/`help()`/`data()` GUI popups don't work — call `library(...)` explicitly at the top of the script (every run is a fresh R instance, so nothing is pre-loaded across runs). +- Nothing opens a graphics device for you. Explicitly call `png()`/`Cairo()`/`pdf()`/etc. and always call `dev.off()` when finished — a script that relied on an interactive plot window will silently produce nothing. +- On headless Unix servers, base `jpeg()`/`png()` may fail without X11 — use the `Cairo` or `GDD` packages instead. +- Output from anything not evaluated at the top level (a `source()`d file, a function body) is not auto-printed — wrap it in `print()`. This is standard R semantics, but easy to forget when porting a script that always ran top-level in an interactive console. + +### Conversion checklist + +- Replace `read.csv("local_file.csv")` / hardcoded paths with `labkey.data` (if data-bound) or an explicit `${input_data}` read (if not). +- Redirect every plot from an interactive window or `ggsave()` to `${imgout:...}`/`${pngout:...}` + explicit `dev.off()`. +- Add explicit `print()` wherever the original script relied on top-level auto-printing inside a function or sourced file. +- Audit and remove/justify any `system()` calls — they run with the LabKey server's OS privileges, not the viewer's. +- Confirm column names via `names(labkey.data)` — don't assume the source data's original casing/characters survived sanitization, and remember slashes are spelled out (`_fs_`), not collapsed to `_`. +- Don't reference `${srcDirectory}` in a ported script — it doesn't resolve for Reports and will be written verbatim into the generated script file, causing a runtime file-not-found error. + +## Python / Jupyter Report Track + +**There is no plain-`.py` report path — in the UI or in a file-based module.** LabKey has no Python-language script engine wired into a report builder, and file-based module report discovery only recognizes `.r`/`.rmd`/`.rhtml`, `.ipynb`, `.js`, and `.xml` — never `.py`. The only supported mechanism for a Python-language report is a **Jupyter Report** (`.ipynb`), a Premium feature that requires an admin-configured Jupyter Report Engine (a separate Docker-hosted execution service). If someone asks for "a Python report," what they need is a Jupyter Report. + +### What's injected — fundamentally different from R + +Jupyter reports get **no** auto-populated `labkey.data`-equivalent variable. Instead, LabKey writes a `report_config.json` alongside the notebook before execution: + +```json +{ + "baseUrl": "http://localhost:8080", + "contextPath": "/labkey", + "scriptName": "myReport.ipynb", + "containerPath": "/my studies/demo", + "parameters": [["pageId", "study.DATA_ANALYSIS"]], + "version": 1 +} +``` + +A `sourceQuery` block (the bound schema/query/view/filters) is added to `report_config.json` **only if the report is data-bound** — otherwise there's nothing for the notebook to load. The notebook must explicitly import and call a bundled helper module to get anything: + +```python +from ReportConfig import get_report_api_wrapper, get_report_data, get_report_parameters +print(get_report_parameters()) +print(get_report_data()) +``` + +Authentication for the notebook's execution is carried via HTTP headers to the execution service (`X-LABKEY-APIKEY`, `X-LABKEY-USERID`, `X-LABKEY-EMAIL`, `X-LABKEY-CSRF`) — not an in-notebook variable like R's `labkey.apiKey`. + +**No `${...}` substitution tokens apply to Jupyter reports.** Output capture relies entirely on the notebook's native cell-output rendering, the same as when you open the `.ipynb` in Jupyter directly — there's no LabKey-side token replacement for plots or files. + +### Making a Jupyter report data-bound + +Since path-based auto-derivation doesn't apply to `.ipynb`, a data-bound Jupyter module report must set `schemaName`/`queryName` explicitly in its `.report.xml`: + +```xml + + + + study + Weight + + +``` + +### Conversion checklist + +- Replace the notebook's original data-loading cell with `get_report_data()` (data-bound) or `get_report_api_wrapper()` + an explicit query call (not data-bound) — there's no automatic equivalent to R's `labkey.data`. +- Confirm the Jupyter engine's Docker image actually has your notebook's package dependencies installed — there's no per-report package install step. +- Don't port any `${imgout:...}`-style output-capture code from an R report you're referencing — it has no effect in a Jupyter report. Rely on normal notebook cell output. +- Don't port `runInBackground`/"run as pipeline job" expectations from an R report — Jupyter reports can't run this way (the option is hidden in the designer, and setting it another way is simply ignored at execution time). +- Remember this is a Premium feature — confirm a Jupyter Report Engine is configured (⚙ → Site → Admin Console → Views and Scripting) before assuming the report type is available. + +## Saved via UI vs. Saved in a File-Based Module + +| | UI-saved | File-based module | +|---|---|---| +| Storage | Database row, per folder | `resources/reports/schemas///name.` in the module's source tree | +| Editable by end users | Yes, via the Source tab (if permitted) | No — only via the premium ModuleEditor tool, which writes back into the module source, not a per-folder DB row | +| Scope | One folder, one author (unless explicitly shared) | Ships with the module; runs identically everywhere the module is enabled | +| Deployment | Immediate | Hot-deployed — no server restart needed | + +A file-based report's optional `name.report.xml` sidecar (same basename as the script) uses the `http://labkey.org/query/xml` `ReportDescriptor` schema. Three real shapes from this repo, in increasing complexity: + +**Minimal** (just sets a property) — `Weight Graph.report.xml`, shown above under [Data-Bound vs. Not Data-Bound](#data-bound-vs-not-data-bound). + +**With external dependencies** — `kable.report.xml`, shown above under [R Report Track](#r-report-track). + +**Remote engine / session sharing** — `server/testAutomation/modules/scriptpad/resources/reports/schemas/script_rserve.report.xml`: +```xml + + + Rserve Session Sharing + + + + + + + + + +``` diff --git a/core/src/org/labkey/core/webdav/DavController.java b/core/src/org/labkey/core/webdav/DavController.java index 82dac842a0b..f171e828460 100644 --- a/core/src/org/labkey/core/webdav/DavController.java +++ b/core/src/org/labkey/core/webdav/DavController.java @@ -80,7 +80,6 @@ import org.labkey.api.security.User; import org.labkey.api.security.UserManager; import org.labkey.api.security.permissions.AbstractContainerScopingTest; -import org.labkey.api.security.permissions.BrowserDeveloperPermission; import org.labkey.api.security.permissions.ReadPermission; import org.labkey.api.security.roles.AuthorRole; import org.labkey.api.security.roles.EditorRole; @@ -3888,7 +3887,7 @@ private void fireFileMovedEvent(@NotNull WebdavResource dest, @NotNull WebdavRes boolean isSafeCopy(WebdavResource src, WebdavResource dest) { // Don't allow creating text/html via rename (circumventing script checking) - if (src.isFile() && !getContainer().hasPermission(getUser(), BrowserDeveloperPermission.class)) + if (src.isFile() && !getUser().isTrustedBrowserDev()) { String contentTypeSrc = Objects.toString(src.getContentType(),""); String contentTypeDest = Objects.toString(dest.getContentType(),""); diff --git a/experiment/src/org/labkey/experiment/ExpDataIterators.java b/experiment/src/org/labkey/experiment/ExpDataIterators.java index 841ce6bf8d2..893b020109b 100644 --- a/experiment/src/org/labkey/experiment/ExpDataIterators.java +++ b/experiment/src/org/labkey/experiment/ExpDataIterators.java @@ -212,60 +212,58 @@ public CounterDataIteratorBuilder(@NotNull DataIteratorBuilder in, Container con @Override public DataIterator getDataIterator(DataIteratorContext context) { - DataIterator pre = _in.getDataIterator(context); - if (pre == null) - return null; // can happen if context has errors + return DataIteratorUtil.wrapOrClose(_in, context, pre -> { + SimpleTranslator counterTranslator = new SimpleTranslator(pre, context); + counterTranslator.setDebugName("Counter Def"); + Set skipColumns = new CaseInsensitiveHashSet(); + Map columnNameMap = DataIteratorUtil.createColumnNameMap(pre); - SimpleTranslator counterTranslator = new SimpleTranslator(pre, context); - counterTranslator.setDebugName("Counter Def"); - Set skipColumns = new CaseInsensitiveHashSet(); - Map columnNameMap = DataIteratorUtil.createColumnNameMap(pre); - - for (CounterDefinition counterDefinition : _expTable.getCounterDefinitions()) - { - Set attachedColumnNames = counterDefinition.getAttachedColumnNames(); - skipColumns.addAll(attachedColumnNames); - - // validate we have all the paired columns - List pairedIndexes = new IntArrayList(); - for (String pairedColumnName : counterDefinition.getPairedColumnNames()) + for (CounterDefinition counterDefinition : _expTable.getCounterDefinitions()) { - Integer i = columnNameMap.get(pairedColumnName); - if (i == null) - { - // immediately return error iterator tied to the input DataIterator instead of counterTranslator - ValidationException setupError = new ValidationException(); - setupError.addGlobalError("Paired column '" + pairedColumnName + "' is required for counter '" + counterDefinition.getCounterName() + "'"); - return ErrorIterator.wrap(pre, context, true, setupError); - } - else - { - pairedIndexes.add(i); - } - } + Set attachedColumnNames = counterDefinition.getAttachedColumnNames(); + skipColumns.addAll(attachedColumnNames); - // add a sequence column for each of the attached columns - for (String columnName : attachedColumnNames) - { - Integer i = columnNameMap.get(columnName); - ColumnInfo column; - if (null != i) + // validate we have all the paired columns + List pairedIndexes = new IntArrayList(); + for (String pairedColumnName : counterDefinition.getPairedColumnNames()) { - column = pre.getColumnInfo(i); - skipColumns.add(columnName); + Integer i = columnNameMap.get(pairedColumnName); + if (i == null) + { + // immediately return error iterator tied to the input DataIterator instead of counterTranslator + ValidationException setupError = new ValidationException(); + setupError.addGlobalError("Paired column '" + pairedColumnName + "' is required for counter '" + counterDefinition.getCounterName() + "'"); + return ErrorIterator.wrap(pre, context, true, setupError); + } + else + { + pairedIndexes.add(i); + } } - else + + // add a sequence column for each of the attached columns + for (String columnName : attachedColumnNames) { - column = _expTable.getColumn(columnName); - } + Integer i = columnNameMap.get(columnName); + ColumnInfo column; + if (null != i) + { + column = pre.getColumnInfo(i); + skipColumns.add(columnName); + } + else + { + column = _expTable.getColumn(columnName); + } - counterTranslator.addPairedSequenceColumn(column, i, _container, counterDefinition, pairedIndexes, _sequencePrefix, _id, 100); + counterTranslator.addPairedSequenceColumn(column, i, _container, counterDefinition, pairedIndexes, _sequencePrefix, _id, 100); + } } - } - counterTranslator.selectAll(skipColumns); + counterTranslator.selectAll(skipColumns); - return LoggingDataIterator.wrap(counterTranslator); + return LoggingDataIterator.wrap(counterTranslator); + }); } } @@ -347,11 +345,8 @@ public AliquotRollupDataIteratorBuilder(@NotNull DataIteratorBuilder in, Contain @Override public DataIterator getDataIterator(DataIteratorContext context) { - DataIterator pre = _in.getDataIterator(context); - if (pre == null) - return null; // can happen if context has errors - - return LoggingDataIterator.wrap(new AliquotRollupDataIterator(pre, context, _container)); + return DataIteratorUtil.wrapOrClose(_in, context, + pre -> LoggingDataIterator.wrap(new AliquotRollupDataIterator(pre, context, _container))); } } @@ -510,11 +505,8 @@ public AliasDataIteratorBuilder(@NotNull DataIteratorBuilder in, Container conta @Override public DataIterator getDataIterator(DataIteratorContext context) { - DataIterator di = _in.getDataIterator(context); - if (di == null) - return null; // can happen if context has errors - - return LoggingDataIterator.wrap(new AliasDataIterator(di, context, _container, _user, _expAliasTable, _dataType, _isSample)); + return DataIteratorUtil.wrapOrClose(_in, context, + di -> LoggingDataIterator.wrap(new AliasDataIterator(di, context, _container, _user, _expAliasTable, _dataType, _isSample))); } } @@ -631,11 +623,8 @@ public AutoLinkToStudyDataIteratorBuilder(@NotNull DataIteratorBuilder in, UserS @Override public DataIterator getDataIterator(DataIteratorContext context) { - DataIterator pre = _in.getDataIterator(context); - if (pre == null) - return null; // can happen if context has errors - - return LoggingDataIterator.wrap(new AutoLinkToStudyDataIterator(DataIteratorUtil.wrapMap(pre, false), _schema, _container, _user, _sampleType)); + return DataIteratorUtil.wrapOrClose(_in, context, + pre -> LoggingDataIterator.wrap(new AutoLinkToStudyDataIterator(DataIteratorUtil.wrapMap(pre, false), _schema, _container, _user, _sampleType))); } } @@ -754,11 +743,8 @@ public FlagDataIteratorBuilder(@NotNull DataIteratorBuilder in, User user, boole @Override public DataIterator getDataIterator(DataIteratorContext context) { - DataIterator pre = _in.getDataIterator(context); - if (pre == null) - return null; // can happen if context has errors - - return LoggingDataIterator.wrap(new FlagDataIterator(pre, context, _user, _isSample, _expObject, _container)); + return DataIteratorUtil.wrapOrClose(_in, context, + pre -> LoggingDataIterator.wrap(new FlagDataIterator(pre, context, _user, _isSample, _expObject, _container))); } } @@ -895,21 +881,19 @@ public DerivationDataIteratorBuilder(DataIteratorBuilder pre, Container containe @Override public DataIterator getDataIterator(DataIteratorContext context) { - DataIterator di = _pre.getDataIterator(context); - if (di == null) - return null; // can happen if context has errors + return DataIteratorUtil.wrapOrClose(_pre, context, di -> { + if (context.getConfigParameters().containsKey(SampleTypeUpdateServiceDI.Options.SkipDerivation)) + return di; - if (context.getConfigParameters().containsKey(SampleTypeUpdateServiceDI.Options.SkipDerivation)) - return di; - - if (context.getInsertOption() != QueryUpdateService.InsertOption.UPDATE) - di = new DerivationDataIterator(di, context, _container, _user, _currentDataType, _isSample, _skipAliquot); - else if (_isSample) - di = new SampleUpdateDerivationDataIterator(di, context, _container, _user, _currentDataType, _checkRequiredParents); - else - di = new DataUpdateDerivationDataIterator(di, context, _container, _user, _currentDataType, _checkRequiredParents); + if (context.getInsertOption() != QueryUpdateService.InsertOption.UPDATE) + di = new DerivationDataIterator(di, context, _container, _user, _currentDataType, _isSample, _skipAliquot); + else if (_isSample) + di = new SampleUpdateDerivationDataIterator(di, context, _container, _user, _currentDataType, _checkRequiredParents); + else + di = new DataUpdateDerivationDataIterator(di, context, _container, _user, _currentDataType, _checkRequiredParents); - return LoggingDataIterator.wrap(di); + return LoggingDataIterator.wrap(di); + }); } } @@ -2124,11 +2108,8 @@ public SearchIndexIteratorBuilder(DataIteratorBuilder pre, Function LoggingDataIterator.wrap(new SearchIndexIterator(pre, context, _indexFunction))); } } @@ -2365,10 +2346,11 @@ public PersistDataIteratorBuilder setFileLinkDirectory(String dir) @Override public DataIterator getDataIterator(DataIteratorContext context) { - DataIterator input = _in.getDataIterator(context); - if (null == input) - return null; // Can happen if context has errors + return DataIteratorUtil.wrapOrClose(_in, context, input -> build(input, context)); + } + private DataIterator build(DataIterator input, DataIteratorContext context) + { // useTransactionAuditCache already set for import and merge in AbstractQueryImportAction.createDataIteratorContext if (context.getInsertOption() == QueryUpdateService.InsertOption.INSERT) { @@ -2536,26 +2518,21 @@ public SampleUpdateOnlyValidatorsIteratorBuilder(@NotNull DataIteratorBuilder in @Override public DataIterator getDataIterator(DataIteratorContext context) { - DataIterator di = _in.getDataIterator(context); - if (di == null) - return null; // can happen if context has errors - - ValidatorIterator validate = new ValidatorIterator(di, context, _container, _user); - Map map = DataIteratorUtil.createColumnNameMap(validate); - - Integer index = map.get(Name.name()); - if (index != null) - { - ColumnInfo column = di.getColumnInfo(index); - validate.addValidator(index, new RequiredValidator(column.getColumnName(), column.getJdbcType(), false, false, "Sample name cannot be blank")); - } + return DataIteratorUtil.wrapOrClose(_in, context, di -> { + ValidatorIterator validate = new ValidatorIterator(di, context, _container, _user); + Map map = DataIteratorUtil.createColumnNameMap(validate); - // Add other column validators here... + Integer index = map.get(Name.name()); + if (index != null) + { + ColumnInfo column = di.getColumnInfo(index); + validate.addValidator(index, new RequiredValidator(column.getColumnName(), column.getJdbcType(), false, false, "Sample name cannot be blank")); + } - if (validate.hasValidators()) - di = validate; + // Add other column validators here... - return LoggingDataIterator.wrap(di); + return LoggingDataIterator.wrap(validate.hasValidators() ? validate : di); + }); } } @@ -2575,11 +2552,8 @@ public SampleNameChangeDataIteratorBuilder(@NotNull DataIteratorBuilder in, User @Override public DataIterator getDataIterator(DataIteratorContext context) { - DataIterator di = _in.getDataIterator(context); - if (di == null) - return null; // can happen if context has errors - - return LoggingDataIterator.wrap(new SampleNameChangeDataIterator(di, context, _user, _canUpdateNames)); + return DataIteratorUtil.wrapOrClose(_in, context, + di -> LoggingDataIterator.wrap(new SampleNameChangeDataIterator(di, context, _user, _canUpdateNames))); } } @@ -3202,11 +3176,8 @@ public MultiDataTypeCrossProjectDataIteratorBuilder(@NotNull User user, @NotNull @Override public DataIterator getDataIterator(DataIteratorContext context) { - DataIterator di = _in.getDataIterator(context); - if (di == null) - return null; // can happen if context has errors - - return LoggingDataIterator.wrap(new MultiDataTypeCrossProjectDataIterator(di, context, _container, _user, _isCrossType, _dataType, _isSamples)); + return DataIteratorUtil.wrapOrClose(_in, context, + di -> LoggingDataIterator.wrap(new MultiDataTypeCrossProjectDataIterator(di, context, _container, _user, _isCrossType, _dataType, _isSamples))); } } @@ -3232,11 +3203,8 @@ public SampleStatusCheckIteratorBuilder(@NotNull DataIteratorBuilder in, Contain @Override public DataIterator getDataIterator(DataIteratorContext context) { - DataIterator pre = _in.getDataIterator(context); - if (pre == null) - return null; // can happen if context has errors - - return LoggingDataIterator.wrap(new SampleStatusCheckDataIterator(pre, context, _container)); + return DataIteratorUtil.wrapOrClose(_in, context, + pre -> LoggingDataIterator.wrap(new SampleStatusCheckDataIterator(pre, context, _container))); } } diff --git a/experiment/src/org/labkey/experiment/ExperimentModule.java b/experiment/src/org/labkey/experiment/ExperimentModule.java index 5cd675c69c4..ebd0baf6802 100644 --- a/experiment/src/org/labkey/experiment/ExperimentModule.java +++ b/experiment/src/org/labkey/experiment/ExperimentModule.java @@ -132,6 +132,7 @@ import org.labkey.experiment.api.ExperimentServiceImpl; import org.labkey.experiment.api.ExperimentStressTest; import org.labkey.experiment.api.GraphAlgorithms; +import org.labkey.experiment.api.ImportAbortResourceTestCase; import org.labkey.experiment.api.LineageTest; import org.labkey.experiment.api.LogDataType; import org.labkey.experiment.api.Protocol; @@ -1135,6 +1136,7 @@ public Collection getSummary(Container c) ExperimentServiceImpl.ParseInputOutputAliasTestCase.class, ExperimentServiceImpl.TestCase.class, ExperimentStressTest.class, + ImportAbortResourceTestCase.class, LineagePerfTest.class, LineageTest.class, OntologyManager.TestCase.class, diff --git a/experiment/src/org/labkey/experiment/api/ExpDataClassDataTableImpl.java b/experiment/src/org/labkey/experiment/api/ExpDataClassDataTableImpl.java index 8ac51e4d2f5..db15c9d080a 100644 --- a/experiment/src/org/labkey/experiment/api/ExpDataClassDataTableImpl.java +++ b/experiment/src/org/labkey/experiment/api/ExpDataClassDataTableImpl.java @@ -978,10 +978,12 @@ private static boolean isReservedHeader(String name) public DataIterator getDataIterator(DataIteratorContext context) { _context = context; - DataIterator input = _in.getDataIterator(context); - if (null == input) - return null; // Can happen if context has errors + return DataIteratorUtil.wrapOrClose(_in, context, input -> build(input, context)); + } + /** Returning null here (after adding an error) or throwing leaves `input` to be closed by wrapOrClose. */ + private DataIterator build(DataIterator input, DataIteratorContext context) + { boolean isMerge = context.getInsertOption() == QueryUpdateService.InsertOption.MERGE; boolean isUpdate = context.getInsertOption() == QueryUpdateService.InsertOption.UPDATE; diff --git a/experiment/src/org/labkey/experiment/api/ImportAbortResourceTestCase.java b/experiment/src/org/labkey/experiment/api/ImportAbortResourceTestCase.java new file mode 100644 index 00000000000..d1336d725b0 --- /dev/null +++ b/experiment/src/org/labkey/experiment/api/ImportAbortResourceTestCase.java @@ -0,0 +1,217 @@ +/* + * Copyright (c) 2026 LabKey Corporation + * + * Licensed 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.labkey.experiment.api; + +import org.junit.AfterClass; +import org.junit.Assert; +import org.junit.BeforeClass; +import org.junit.Test; +import org.labkey.api.collections.CaseInsensitiveHashMap; +import org.labkey.api.data.Container; +import org.labkey.api.data.TableInfo; +import org.labkey.api.data.TableSelector; +import org.labkey.api.dataiterator.DataIterator; +import org.labkey.api.dataiterator.DataIteratorBuilder; +import org.labkey.api.dataiterator.DataIteratorContext; +import org.labkey.api.dataiterator.MapDataIterator; +import org.labkey.api.dataiterator.WrapperDataIterator; +import org.labkey.api.exp.api.ExpSampleType; +import org.labkey.api.exp.api.ExperimentService; +import org.labkey.api.exp.api.SampleTypeService; +import org.labkey.api.exp.query.ExpSchema; +import org.labkey.api.exp.query.SamplesSchema; +import org.labkey.api.gwt.client.model.GWTPropertyDescriptor; +import org.labkey.api.query.QueryService; +import org.labkey.api.query.QueryUpdateService; +import org.labkey.api.query.QueryUpdateService.InsertOption; +import org.labkey.api.security.User; +import org.labkey.api.settings.OptionalFeatureService; +import org.labkey.api.util.JunitUtil; +import org.labkey.api.util.TestContext; +import org.labkey.experiment.ExpDataIterators; + +import java.io.IOException; +import java.util.ArrayList; +import java.util.Collections; +import java.util.List; +import java.util.Map; + +/** + * A DataIteratorBuilder that bails out after its input has been built must close that input. The input is + * frequently a query-backed iterator holding a live ResultSet, its Statement, and a pooled Connection; dropping + * it strands all three until the ResultSetImpl Cleaner runs at GC, which on a short-scheduled ETL can exhaust + * the pool. Asserting on close() rather than on pool counts keeps this deterministic - no GC, no timing. + */ +public class ImportAbortResourceTestCase extends Assert +{ + private static Container _c; + private static User _user; + private static boolean _restoreAllowRowIdMerge; + + @BeforeClass + public static void setUp() + { + JunitUtil.deleteTestContainer(); + _c = JunitUtil.getTestContainer(); + _user = TestContext.get().getUser(); + + // These tests drive the RowId-on-merge rejection, so the server-wide opt-out must be off. + _restoreAllowRowIdMerge = OptionalFeatureService.get().isFeatureEnabled(ExperimentService.EXPERIMENTAL_FEATURE_ALLOW_ROW_ID_MERGE); + if (_restoreAllowRowIdMerge) + OptionalFeatureService.get().setFeatureEnabled(ExperimentService.EXPERIMENTAL_FEATURE_ALLOW_ROW_ID_MERGE, false, _user); + } + + @AfterClass + public static void tearDown() + { + if (_restoreAllowRowIdMerge) + OptionalFeatureService.get().setFeatureEnabled(ExperimentService.EXPERIMENTAL_FEATURE_ALLOW_ROW_ID_MERGE, true, _user); + JunitUtil.deleteTestContainer(); + } + + /** A row source that records every iterator it hands out, so an unclosed one is a deterministic failure. */ + private static class RecordingSource implements DataIteratorBuilder + { + private final DataIteratorBuilder _rows; + private final List _built = new ArrayList<>(); + + RecordingSource(List> rows) + { + _rows = MapDataIterator.of(rows); + } + + @Override + public DataIterator getDataIterator(DataIteratorContext context) + { + CloseRecordingDataIterator di = new CloseRecordingDataIterator(_rows.getDataIterator(context)); + _built.add(di); + return di; + } + + private static class CloseRecordingDataIterator extends WrapperDataIterator + { + private boolean _closed = false; + + CloseRecordingDataIterator(DataIterator di) + { + super(di); + } + + @Override + public void close() throws IOException + { + _closed = true; + super.close(); + } + } + } + + private void assertAllSourcesClosed(RecordingSource source) + { + assertFalse("source iterator was never built, so this test proves nothing", source._built.isEmpty()); + for (RecordingSource.CloseRecordingDataIterator di : source._built) + assertTrue("abandoned source DataIterator was left open", di._closed); + } + + /** Runs a merge expected to abort on a validation error, then asserts the source was closed. */ + private void assertMergeAbortClosesSource(TableInfo table, List> rows, String expectedError) throws Exception + { + RecordingSource source = new RecordingSource(rows); + DataIteratorContext context = new DataIteratorContext(); + context.setInsertOption(InsertOption.MERGE); + + QueryUpdateService qus = table.getUpdateService(); + assertNotNull("no QueryUpdateService for " + table.getName(), qus); + + // loadRows returns 0 whenever the context has errors, so it says nothing about what was written - count the table. + assertEquals("import reported inserted rows", 0, qus.loadRows(_user, _c, source, context, null)); + assertTrue("expected a validation error", context.getErrors().hasErrors()); + assertTrue("unexpected error: " + context.getErrors().getMessage(), + context.getErrors().getMessage().contains(expectedError)); + assertEquals("expected the import to abort without inserting", 0L, new TableSelector(table).getRowCount()); + assertAllSourcesClosed(source); + } + + private TableInfo createDataClassTable(String name) throws Exception + { + List props = List.of(new GWTPropertyDescriptor("prop", "string")); + ExperimentServiceImpl.get().createDataClass(_c, _user, name, null, props, Collections.emptyList(), null, null); + TableInfo table = QueryService.get().getUserSchema(_user, _c, ExpSchema.SCHEMA_EXP_DATA).getTable(name); + assertNotNull("could not resolve data class table " + name, table); + return table; + } + + private TableInfo createSampleTypeTable(String name) throws Exception + { + List props = List.of( + new GWTPropertyDescriptor("name", "string"), + new GWTPropertyDescriptor("prop", "string")); + ExpSampleType st = SampleTypeService.get().createSampleType(_c, _user, name, null, props, Collections.emptyList(), -1, -1, -1, -1, null); + TableInfo table = QueryService.get().getUserSchema(_user, _c, SamplesSchema.SCHEMA_NAME).getTable(st.getName()); + assertNotNull("could not resolve sample type table " + name, table); + return table; + } + + @Test + public void testDataClassRowIdOnMergeClosesSource() throws Exception + { + TableInfo table = createDataClassTable("AbortRowIdMerge"); + List> rows = List.of(CaseInsensitiveHashMap.of("Name", "D-1", "RowId", 1, "prop", "a")); + assertMergeAbortClosesSource(table, rows, "RowId is not accepted when merging data"); + } + + @Test + public void testDataClassLsidOnlyKeyOnMergeClosesSource() throws Exception + { + TableInfo table = createDataClassTable("AbortLsidMerge"); + // LSID as the only key column is rejected; Name and RowId are deliberately absent. + List> rows = List.of(CaseInsensitiveHashMap.of("LSID", "urn:lsid:labkey.com:Data.Folder-1:abort", "prop", "a")); + assertMergeAbortClosesSource(table, rows, "LSID is no longer accepted as a key for data"); + } + + @Test + public void testSampleTypeRowIdOnMergeClosesSource() throws Exception + { + TableInfo table = createSampleTypeTable("AbortRowIdMergeSamples"); + List> rows = List.of(CaseInsensitiveHashMap.of("Name", "S-1", "RowId", 1, "prop", "a")); + assertMergeAbortClosesSource(table, rows, "RowId is not accepted when merging samples"); + } + + @Test + public void testSampleTypeLsidOnlyKeyOnMergeClosesSource() throws Exception + { + TableInfo table = createSampleTypeTable("AbortLsidMergeSamples"); + List> rows = List.of(CaseInsensitiveHashMap.of("LSID", "urn:lsid:labkey.com:Sample.Folder-1:abort", "prop", "a")); + assertMergeAbortClosesSource(table, rows, "LSID is no longer accepted as a key for sample"); + } + + /** + * The other half of the contract: a builder whose iterator constructor throws must also close its input. + * AliasDataIteratorBuilder stands in for every ExpDataIterators builder routed through DataIteratorUtil.wrapOrClose. + */ + @Test + public void testBuilderThrowClosesSource() + { + RecordingSource source = new RecordingSource(List.of(CaseInsensitiveHashMap.of("Name", "D-1", "prop", "a"))); + DataIteratorContext context = new DataIteratorContext(); + context.setInsertOption(InsertOption.UPDATE); + + // A map-backed source doesn't support getExistingRecord(), which the AliasDataIterator ctor rejects on update. + DataIteratorBuilder builder = new ExpDataIterators.AliasDataIteratorBuilder(source, _c, _user, null, null, false); + assertThrows(IllegalArgumentException.class, () -> builder.getDataIterator(context)); + assertAllSourcesClosed(source); + } +} diff --git a/experiment/src/org/labkey/experiment/api/SampleTypeUpdateServiceDI.java b/experiment/src/org/labkey/experiment/api/SampleTypeUpdateServiceDI.java index 39b5dabde21..495b28e1caf 100644 --- a/experiment/src/org/labkey/experiment/api/SampleTypeUpdateServiceDI.java +++ b/experiment/src/org/labkey/experiment/api/SampleTypeUpdateServiceDI.java @@ -1267,10 +1267,12 @@ public PreTriggerDataIteratorBuilder(@NotNull ExpSampleTypeImpl sampleType, ExpM @Override public DataIterator getDataIterator(DataIteratorContext context) { - DataIterator di = builder.getDataIterator(context); - if (di == null) - return null; // can happen if context has errors + return DataIteratorUtil.wrapOrClose(builder, context, di -> build(di, context)); + } + /** Returning null here (after adding an error) or throwing leaves `di` to be closed by wrapOrClose. */ + private DataIterator build(DataIterator di, DataIteratorContext context) + { boolean isMerge = context.getInsertOption() == InsertOption.MERGE; boolean isUpdate = context.getInsertOption() == InsertOption.UPDATE; diff --git a/experiment/src/org/labkey/experiment/api/property/PropertyServiceImpl.java b/experiment/src/org/labkey/experiment/api/property/PropertyServiceImpl.java index 5972ccb28ce..60e685e6a6b 100644 --- a/experiment/src/org/labkey/experiment/api/property/PropertyServiceImpl.java +++ b/experiment/src/org/labkey/experiment/api/property/PropertyServiceImpl.java @@ -98,6 +98,7 @@ import java.util.Set; import java.util.concurrent.ConcurrentHashMap; import java.util.concurrent.CopyOnWriteArrayList; +import java.util.function.Predicate; import java.util.stream.Collectors; import java.util.stream.Stream; @@ -276,13 +277,7 @@ public List getDomains(Container container) @Override public List getDomains(Container container, User user, boolean includeProjectAndShared) { - List result = new ArrayList<>(); - for (DomainDescriptor dd : OntologyManager.getDomainDescriptors(container, user, includeProjectAndShared)) - { - result.add(new DomainImpl(dd)); - } - - return Collections.unmodifiableList(result); + return streamDomains(container, user, includeProjectAndShared, _ -> true).toList(); } @Override @@ -294,27 +289,33 @@ public List getDomains(Container container, User user, @Nullab @Override public List getDomains(Container container, User user, @NotNull DomainKind dk, boolean includeProjectAndShared) { - // Domain.getDomainKind() can be slow. Instead just ask the passed-in dk if the domain matches or not. - return getDomains(container, user, includeProjectAndShared) - .stream() - .filter(d -> dk.getPriority(d.getTypeURI()) != null) - .collect(Collectors.toList()); + // Domain.getDomainKind() can be slow. Instead, just ask the passed-in dk if the domain matches or not. + return streamDomains(container, user, includeProjectAndShared, dd -> dk.getPriority(dd.getDomainURI()) != null).toList(); } @Override public Stream getDomainsStream(Container container, User user, @Nullable Set domainKinds, @Nullable Set domainNames, boolean includeProjectAndShared) { - Stream stream = getDomains(container, user, includeProjectAndShared) - .stream() - .filter(d -> d.getDomainKind() != null); + Predicate filter = dd -> dd.getDomainKind() != null; if (domainKinds != null && !domainKinds.isEmpty()) - stream = stream.filter(d -> domainKinds.contains(d.getDomainKind().getKindName())); + filter = filter.and(dd -> domainKinds.contains(dd.getDomainKind().getKindName())); if (domainNames != null && !domainNames.isEmpty()) - stream = stream.filter(d -> domainNames.contains(d.getName())); + filter = filter.and(dd -> domainNames.contains(dd.getName())); - return stream; + return streamDomains(container, user, includeProjectAndShared, filter); + } + + /** + * Filter runs on the DomainDescriptor because constructing a DomainImpl loads and clones the domain's + * PropertyDescriptors, so materializing one per domain in the container is very expensive. + */ + private Stream streamDomains(Container container, User user, boolean includeProjectAndShared, Predicate filter) + { + return OntologyManager.getDomainDescriptors(container, user, includeProjectAndShared).stream() + .filter(filter) + .map(DomainImpl::new); } @Override diff --git a/issues/src/org/labkey/issue/IssuesController.java b/issues/src/org/labkey/issue/IssuesController.java index 3571d564775..9cb4cd7c5dc 100644 --- a/issues/src/org/labkey/issue/IssuesController.java +++ b/issues/src/org/labkey/issue/IssuesController.java @@ -112,6 +112,7 @@ import org.labkey.api.security.roles.OwnerRole; import org.labkey.api.security.roles.ReaderRole; import org.labkey.api.security.roles.RoleManager; +import org.labkey.api.security.roles.SubmitterRole; import org.labkey.api.util.ButtonBuilder; import org.labkey.api.util.CSRFUtil; import org.labkey.api.util.DOM; @@ -2548,4 +2549,59 @@ private void deleteProjects() _projectA = null; } } + + /** + * GitHub Issue 1317 regression test. + */ + public static class GetIssuePermissionTestCase extends AbstractContainerScopingTest + { + private static final String ISSUE_TITLE = "getIssue() permission test issue"; + + @Test + public void testSubmitterCannotReadIssue() throws Exception + { + Container c = createIssuesContainer("Submitter"); + int issueId = createIssue(c); + + // A reader should be able to access the issue + User reader = createUserInRole(c, ReaderRole.class); + IssueObject asReader = IssueManager.getIssue(c, reader, issueId); + assertNotNull("Reader should be able to read the issue", asReader); + assertEquals(ISSUE_TITLE, asReader.getTitle()); + + // A submitter holds InsertPermission but not ReadPermission, so the issue shouldn't be accessible. + User submitter = createUserInRole(c, SubmitterRole.class); + assertNull("Submitter should not be able to read an issue", IssueManager.getIssue(c, submitter, issueId)); + assertNull("Submitter should not be able to read an issue in an unspecified container", IssueManager.getIssue(null, submitter, issueId)); + } + + private Container createIssuesContainer(String name) + { + Container c = createContainer(name, ModuleLoader.getInstance().getModule(IssuesModule.NAME)); + + IssueListDef def = new IssueListDef(); + def.setName(IssueListDef.DEFAULT_ISSUE_LIST_NAME); + def.setLabel(IssueListDef.DEFAULT_ISSUE_LIST_NAME); + def.setKind(IssueDefDomainKind.NAME); + def.beforeInsert(getAdmin(), c.getId()); + def.save(getAdmin()); + + return c; + } + + private int createIssue(Container c) + { + User admin = getAdmin(); + IssueObject issue = new IssueObject(); + issue.open(c, admin); + issue.setAssignedTo(admin.getUserId()); + issue.setTitle(ISSUE_TITLE); + issue.setPriority("3"); + issue.setIssueDefName(IssueListDef.DEFAULT_ISSUE_LIST_NAME); + ObjectFactory.Registry.getFactory(IssueObject.class).toMap(issue, issue.getProperties()); + IssueManager.saveIssue(admin, c, issue); + + return issue.getIssueId(); + } + } } diff --git a/issues/src/org/labkey/issue/IssuesModule.java b/issues/src/org/labkey/issue/IssuesModule.java index 1fa0c05d492..10f8622699a 100644 --- a/issues/src/org/labkey/issue/IssuesModule.java +++ b/issues/src/org/labkey/issue/IssuesModule.java @@ -193,7 +193,8 @@ public ActionURL getTabURL(Container c, User user) return Set.of( org.labkey.issue.model.IssueManager.TestCase.class, org.labkey.issue.IssuesController.MoveActionContainerScopingTestCase.class, - org.labkey.issue.IssuesController.GetUsersForGroupScopingTestCase.class + org.labkey.issue.IssuesController.GetUsersForGroupScopingTestCase.class, + org.labkey.issue.IssuesController.GetIssuePermissionTestCase.class ); } diff --git a/issues/src/org/labkey/issue/model/IssueManager.java b/issues/src/org/labkey/issue/model/IssueManager.java index 087da9c9a22..7fefbcc9595 100644 --- a/issues/src/org/labkey/issue/model/IssueManager.java +++ b/issues/src/org/labkey/issue/model/IssueManager.java @@ -243,14 +243,21 @@ public static IssueObject getIssue( private static IssueObject _getIssue(@Nullable Container c, User user, int issueId) { IssueObject issue = _getRawIssue(c, issueId); + if (issue == null) + return null; - if (issue != null && issue.getIssueDefId() != null) - { - // container may initially be null if we don't care about a specific folder, but we need the - // correct domain for the provisioned table properties associated with the issue - if (c == null) - c = ContainerManager.getForId(issue.getContainerId()); + // container may initially be null if we don't care about a specific folder, but we need the + // correct domain for the provisioned table properties associated with the issue + if (c == null) + c = ContainerManager.getForId(issue.getContainerId()); + // GitHub Issue 1317: explicitly check for read access on the target container before querying to + // avoid Submitter roles from accessing the table and then hitting an exception during the read. + if (c == null || !c.hasPermission(user, ReadPermission.class)) + return null; + + if (issue.getIssueDefId() != null) + { IssueListDef issueListDef = getIssueListDef(issue.getContainerFromId(), issue.getIssueDefId()); UserSchema userSchema = QueryService.get().getUserSchema(user, c, IssuesQuerySchema.SCHEMA_NAME); TableInfo table = userSchema.getTable(issueListDef.getName()); diff --git a/pipeline/src/org/labkey/pipeline/api/ScriptTaskImpl.java b/pipeline/src/org/labkey/pipeline/api/ScriptTaskImpl.java index a9c8fe56c8b..5aa6534f74e 100644 --- a/pipeline/src/org/labkey/pipeline/api/ScriptTaskImpl.java +++ b/pipeline/src/org/labkey/pipeline/api/ScriptTaskImpl.java @@ -170,6 +170,9 @@ else if (factory._scriptPath != null) bindings.put(ExternalScriptEngine.SCRIPT_PATH, scriptFile.toNioPathForRead().toFile().toString()); bindings.put(ExternalScriptEngine.WORKING_DIRECTORY, _wd.getDir().toNioPathForRead().toString()); + bindings.put(ExternalScriptEngine.INVOCATION_LABEL, "pipeline task=" + _factory.getId() + + " job=" + getJob().getJobGUID() + + " container=" + container.getPath()); // Thread the timeout option through to the external script engine if (_factory.getTimeout() != null && _factory.getTimeout() > 0) diff --git a/query/src/org/labkey/query/controllers/QueryController.java b/query/src/org/labkey/query/controllers/QueryController.java index b4ebe9d8ed5..64743343c08 100644 --- a/query/src/org/labkey/query/controllers/QueryController.java +++ b/query/src/org/labkey/query/controllers/QueryController.java @@ -60,6 +60,7 @@ import org.labkey.api.action.ApiSimpleResponse; import org.labkey.api.action.ApiUsageException; import org.labkey.api.action.ApiVersion; +import org.labkey.api.action.ConcurrencyLimit; import org.labkey.api.action.ConfirmAction; import org.labkey.api.action.ExportAction; import org.labkey.api.action.ExportException; @@ -2719,9 +2720,7 @@ private boolean isFilterOrSort(String dataRegionName, String param) return true; if ("sort".equals(check)) return true; - if (check.equals("containerFilterName")) - return true; - return false; + return check.equals("containerFilterName"); } @RequiresPermission(ReadPermission.class) @@ -5649,7 +5648,7 @@ public boolean equals(Object o) if (o == null || getClass() != o.getClass()) return false; DataSourceInfo that = (DataSourceInfo) o; - return sourceName != null ? sourceName.equals(that.sourceName) : that.sourceName == null; + return Objects.equals(sourceName, that.sourceName); } @Override @@ -7576,11 +7575,16 @@ public void setSchemas(Map>> schemas) } } + /** + * Analyzing a folder holds the full TableInfo/ColumnInfo graph for every query in it for the life of the request, + * so avoid running to many concurrently to avoid overwhelming the heap. + */ + @ConcurrencyLimit(value = 10, message = "Too many query dependency analyses are already running. Please retry in a few moments.") @RequiresPermission(ReadPermission.class) public static class AnalyzeQueriesAction extends ReadOnlyApiAction { @Override - public Object execute(Object o, BindException errors) throws Exception + public Object execute(Object o, BindException errors) { JSONObject ret = new JSONObject(); @@ -7614,7 +7618,9 @@ public Object execute(Object o, BindException errors) throws Exception } else { - ret.put("success", false); + // must be an error rather than an empty graph, which the client reports as "no dependencies" + errors.reject(ERROR_MSG, "Query dependency analysis is not available on this server."); + return null; } return ret; } diff --git a/query/src/org/labkey/query/controllers/SqlController.java b/query/src/org/labkey/query/controllers/SqlController.java index 94d4b1f020f..7e9f28e9fb9 100644 --- a/query/src/org/labkey/query/controllers/SqlController.java +++ b/query/src/org/labkey/query/controllers/SqlController.java @@ -140,6 +140,8 @@ public static class SqlForm private String sql; private String sep = null; private String eol = null; + private Integer offset = null; + private Integer limit = null; private final Parameters parameters = new Parameters(); public Double getApiVersion() @@ -234,6 +236,26 @@ public void setCompact(boolean compact) if (compact) this.format = Format.compact; } + + public Integer getOffset() + { + return offset; + } + + public void setOffset(Integer offset) + { + this.offset = offset; + } + + public Integer getLimit() + { + return limit; + } + + public void setLimit(Integer limit) + { + this.limit = limit; + } } public static class SqlExecute @@ -510,6 +532,7 @@ public ExecuteResult execute(Writer out) throws SQLException, IOException /// Execute a LabKey SQL query and return results as plain text. Designed for lightweight programmatic access without the overhead of QueryView/JSON API responses. /// This action routes value rendering through DisplayColumn.getTsvFormattedValue() for correct /// type dispatch (e.g. dates as ISO-8601, multi-value columns via their DisplayColumn subclass). + /// Accepts optional `offset`/`limit` parameters to page through large result sets; omitting both returns every row (unbounded). @RequiresPermission(ReadPermission.class) @Marshal(Marshaller.Jackson) public static class ExecuteAction extends ReadOnlyApiAction @@ -555,11 +578,17 @@ public Object execute(SqlForm form, BindException errors) throws ServletExceptio var format = form.getFormat(); getViewContext().getResponse().setContentType(format.getContentType()); + + // No offset/limit specified means unbounded, matching this action's original (pre-paging) behavior. + int offset = null == form.getOffset() || form.getOffset() < 0 ? 0 : form.getOffset(); + int limit = null == form.getLimit() || form.getLimit() <= 0 ? Integer.MAX_VALUE : form.getLimit(); + try { new SqlExecute(getViewContext(), userSchema, form.getSql()) .format(format, form.getSep(), form.getEol()) .parameters(form.getParameterMap()) + .page(offset, limit) .execute(getViewContext().getResponse().getWriter()); } catch (QueryParseException x) @@ -628,6 +657,11 @@ public void tearDown() } private MockHttpServletResponse executeSql(String schemaName, String sql, Format format) throws Exception + { + return executeSql(schemaName, sql, format, null, null); + } + + private MockHttpServletResponse executeSql(String schemaName, String sql, Format format, Integer offset, Integer limit) throws Exception { ActionURL url = new ActionURL("sql", "execute", _folder); if (schemaName != null) @@ -636,6 +670,10 @@ private MockHttpServletResponse executeSql(String schemaName, String sql, Format url.addParameter("sql", sql); if (null != format) url.addParameter("format", format.name()); + if (null != offset) + url.addParameter("offset", offset.toString()); + if (null != limit) + url.addParameter("limit", limit.toString()); return ViewServlet.GET(url, TestContext.get().getUser(), null); } @@ -893,5 +931,64 @@ public void testLimit_greaterThanRowCount() throws Exception assertEquals(3, result.rows()); assertTrue("Expected result to be complete", result.complete()); } + + @Test + public void testExecuteAction_limit() throws Exception + { + // 3 rows in table (Alice, Bob, Carol ordered by Name), limit=2 → only the first 2 rows + MockHttpServletResponse response = executeSql("lists", + "SELECT Name FROM " + LIST_NAME + " ORDER BY Name", Format.tsv, null, 2); + assertEquals(HttpServletResponse.SC_OK, response.getStatus()); + + String[] lines = response.getContentAsString().split("\n"); + assertEquals(3, lines.length); // header + 2 data rows + assertEquals("Name", lines[0]); + assertEquals("Alice", lines[1]); + assertEquals("Bob", lines[2]); + } + + @Test + public void testExecuteAction_offset() throws Exception + { + // offset=1 → skip Alice, return Bob and Carol + MockHttpServletResponse response = executeSql("lists", + "SELECT Name FROM " + LIST_NAME + " ORDER BY Name", Format.tsv, 1, null); + assertEquals(HttpServletResponse.SC_OK, response.getStatus()); + + String[] lines = response.getContentAsString().split("\n"); + assertEquals(3, lines.length); // header + 2 data rows + assertEquals("Name", lines[0]); + assertEquals("Bob", lines[1]); + assertEquals("Carol", lines[2]); + } + + @Test + public void testExecuteAction_offsetAndLimit() throws Exception + { + // offset=1, limit=1 → skip Alice, return just Bob + MockHttpServletResponse response = executeSql("lists", + "SELECT Name FROM " + LIST_NAME + " ORDER BY Name", Format.tsv, 1, 1); + assertEquals(HttpServletResponse.SC_OK, response.getStatus()); + + String[] lines = response.getContentAsString().split("\n"); + assertEquals(2, lines.length); // header + 1 data row + assertEquals("Name", lines[0]); + assertEquals("Bob", lines[1]); + } + + @Test + public void testExecuteAction_noLimitReturnsAll() throws Exception + { + // Omitting offset/limit entirely must preserve the original unbounded behavior. + MockHttpServletResponse response = executeSql("lists", + "SELECT Name FROM " + LIST_NAME + " ORDER BY Name", Format.tsv); + assertEquals(HttpServletResponse.SC_OK, response.getStatus()); + + String[] lines = response.getContentAsString().split("\n"); + assertEquals(4, lines.length); // header + 3 data rows + assertEquals("Alice", lines[1]); + assertEquals("Bob", lines[2]); + assertEquals("Carol", lines[3]); + } } } diff --git a/query/webapp/query/browser/Caches.js b/query/webapp/query/browser/Caches.js index daf03e13445..40c899f0af9 100644 --- a/query/webapp/query/browser/Caches.js +++ b/query/webapp/query/browser/Caches.js @@ -169,6 +169,13 @@ Ext4.define('LABKEY.query.browser.cache.QueryDetails', { Ext4.define('LABKEY.query.browser.cache.QueryDependencies', { singleton: true, + /** + * Maximum number of analyzeQueries.api requests to keep in flight at once. Each request builds the full + * TableInfo/ColumnInfo graph for one folder and holds it for the life of the request, so dispatching one + * request per folder can exhaust the server's request thread pool and its heap on a site-level analysis. + */ + MAX_CONCURRENT_REQUESTS : 4, + constructor : function() { this.callParent(); @@ -178,9 +185,17 @@ Ext4.define('LABKEY.query.browser.cache.QueryDependencies', { clear : function() { this.queries = undefined; this.currentContainer = undefined; + this.analyzedContainerPath = undefined; this.totalContainers = 0; this.containers = []; - this.error = undefined; + this.activeContainers = {}; + this.activeRequests = {}; + this.activeCount = 0; + this.finishedCount = 0; + this.analysisComplete = false; + this.cancelled = false; + this.lastResponse = undefined; + this.errors = []; }, getCacheKey : function(container, schemaName, queryName) { @@ -191,19 +206,22 @@ Ext4.define('LABKEY.query.browser.cache.QueryDependencies', { return this.queries[this.getCacheKey(container, schemaName, queryName)]; }, - load : function(containerPath, success, failure, scope) { + load : function(containerPath, success, failure, scope, containers) { if (!this.queries) { this.analyzeQueries({ containerPath : containerPath, - success : function(resp, opts){ - this.processDependencies(resp); + containers : containers, + // callbacks take the accumulated result plus the response and options of the request that ended the + // analysis; dropping either leaves the caller unable to report what actually went wrong + success : function(result, response, options){ + this.processDependencies(result); if (Ext4.isFunction(success)){ - success.call(scope || this, resp, opts); + success.call(scope || this, result, response, options); } }, - failure : function(resp, opts) { - failure.call(scope || this, resp, opts); + failure : function(result, response, options) { + failure.call(scope || this, result, response, options); }, scope : this }); @@ -244,79 +262,154 @@ Ext4.define('LABKEY.query.browser.cache.QueryDependencies', { // hits the server endpoint (premium only) to create the dependency graph analyzeQueries : function(config) { - function fixupJsonResponse(json, response, options, container) { - this.currentContainer = container; - if (json) { - if (json.success) { + // merge one container's response into the accumulated dependency lists + function accumulateResponse(container, json, response, options) { + // any response that isn't success:true failed, even when it carries no error message, and an unparseable + // one leaves json null; swallowing either makes the dependency report look empty rather than broken + if (!json || !json.success) { + this.errors.push({containerPath: container, response: response, options: options}); + return; + } - var key,toKey,fromKey; - var objects = json.objects; + var key,toKey,fromKey; + var objects = json.objects; - var dependantsMap = {}; - var dependeesMap = {}; + var dependantsMap = {}; + var dependeesMap = {}; - for (var edge = 0; edge < json.graph.length; edge++) { - fromKey = json.graph[edge][0]; - toKey = json.graph[edge][1]; + for (var edge = 0; edge < json.graph.length; edge++) { + fromKey = json.graph[edge][0]; + toKey = json.graph[edge][1]; - // objects I am dependant on are my dependees - dependeesMap[fromKey] = dependeesMap[fromKey] || []; - dependeesMap[fromKey].push(objects[toKey]); + // objects I am dependant on are my dependees + dependeesMap[fromKey] = dependeesMap[fromKey] || []; + dependeesMap[fromKey].push(objects[toKey]); - // objects are dependant on me are my dependants - dependantsMap[toKey] = dependantsMap[toKey] || []; - dependantsMap[toKey].push(objects[fromKey]); - } + // objects are dependant on me are my dependants + dependantsMap[toKey] = dependantsMap[toKey] || []; + dependantsMap[toKey].push(objects[fromKey]); + } - for (key in dependeesMap) { - if (dependeesMap.hasOwnProperty(key)) { - let from = objects[key]; - // limit dependants to only queries in the current folder - if (LABKEY.container.id === from.containerId) { - this.dependeesList.push({from: from, to: dependeesMap[key]}); - } - } + for (key in dependeesMap) { + if (dependeesMap.hasOwnProperty(key)) { + let from = objects[key]; + // limit dependants to only queries in the current folder + if (LABKEY.container.id === from.containerId) { + this.dependeesList.push({from: from, to: dependeesMap[key]}); } + } + } - for (key in dependantsMap) { - if (dependantsMap.hasOwnProperty(key)) { - let to = objects[key]; - // limit dependants to only queries in the current folder - if (LABKEY.container.id === to.containerId) { - this.dependantsList.push({to:to, from:dependantsMap[key]}); - } - } + for (key in dependantsMap) { + if (dependantsMap.hasOwnProperty(key)) { + let to = objects[key]; + // limit dependants to only queries in the current folder + if (LABKEY.container.id === to.containerId) { + this.dependantsList.push({to:to, from:dependantsMap[key]}); } } - else if (json.error) { - // only save the first error (if multiple) - if (!this.error) - this.error = {response: response, options: options}; - } } + } - this.removeContainer(container); - if (this.containers.length === 0) { - var callback = this.error ? LABKEY.Utils.getOnFailure(config) : LABKEY.Utils.getOnSuccess(config); - var resp = response; - var opts = options; - if (this.error) { - resp = this.error.response; - opts = this.error.options; - } + // the analysis is finished only once the queue has drained AND every dispatched request has returned + function checkComplete() { + if (this.cancelled || this.analysisComplete || this.activeCount > 0 || this.containers.length > 0) { + return; + } + this.analysisComplete = true; + + var failed = this.errors.length > 0; + var callback = failed ? LABKEY.Utils.getOnFailure(config) : LABKEY.Utils.getOnSuccess(config); + var last = this.lastResponse || {}; + var resp = last.response; + var opts = last.options; + if (failed) { + resp = this.errors[0].response; + opts = this.errors[0].options; + } - if (callback) { - var success = this.error ? false : (json ? json.success : false); - callback.call(this, {success: success, dependants: this.dependantsList, dependees: this.dependeesList}, resp, opts); + if (callback) { + var success = failed ? false : (last.json ? last.json.success : false); + callback.call(this, {success: success, dependants: this.dependantsList, dependees: this.dependeesList}, resp, opts); + } + } + + function requestComplete(container, json, response, options) { + // ignore the abort callbacks that cancel() triggers, and any duplicate callback for a container that has + // already been accounted for + if (this.cancelled || !this.activeContainers[container]) { + return; + } + delete this.activeContainers[container]; + delete this.activeRequests[container]; + this.activeCount--; + this.finishedCount++; + this.lastResponse = {response: response, options: options, json: json}; + + accumulateResponse.call(this, container, json, response, options); + pump.call(this); + } + + function dispatch(container) { + this.activeContainers[container] = true; + this.activeCount++; + this.currentContainer = container; + + this.activeRequests[container] = LABKEY.Ajax.request({ + url: LABKEY.ActionURL.buildURL('query', 'analyzeQueries.api', container), + method: 'GET', + scope: this, + success: function(resp, options){ + var json = null; + try { + json = LABKEY.Utils.decode(resp.responseText); + } + catch (e) { + console.warn('Invalid JSON returned from analyzeQueries.api : ' + resp.responseText); + console.warn('Response URL : ' + resp.responseURL); + + // leave json null and finish processing this container + } + requestComplete.call(this, container, json, resp, options); + }, + failure: function(resp, options){ + console.warn('Analyze query request failed : ' + resp.responseText); + requestComplete.call(this, container, {error: true}, resp, options); } + }); + } + + // keep up to MAX_CONCURRENT_REQUESTS requests in flight, then test for completion + function pump() { + while (this.activeCount < this.MAX_CONCURRENT_REQUESTS && this.containers.length > 0) { + dispatch.call(this, this.containers.shift()); } + checkComplete.call(this); } // initialize class data structures this.dependantsList = []; this.dependeesList = []; - this.containers = []; + this.containers = []; // container paths queued but not yet requested + this.activeContainers = {}; // container path -> true for each request in flight + this.activeRequests = {}; // container path -> XMLHttpRequest, so cancel() can abort them + this.activeCount = 0; + this.finishedCount = 0; + this.analysisComplete = false; + this.cancelled = false; + this.lastResponse = undefined; + this.errors = []; + this.analyzedContainerPath = config.containerPath; + + // the caller may have already resolved the scope via loadContainerCounts(), in which case there is nothing to look up + if (config.containers) { + this.containers = config.containers.slice(); + this.totalContainers = this.containers.length; + pump.call(this); + return; + } + this.containers.push(config.containerPath || LABKEY.container.path); let includeSubfolders = config.containerPath != null; @@ -324,6 +417,9 @@ Ext4.define('LABKEY.query.browser.cache.QueryDependencies', { LABKEY.Security.getContainers({ containerPath : config.containerPath, includeSubfolders : includeSubfolders, + includeWorkbookChildren : false, + includeStandardProperties : false, + includeEffectivePermissions : false, scope : this, success : function(json){ if (includeSubfolders) { @@ -332,31 +428,9 @@ Ext4.define('LABKEY.query.browser.cache.QueryDependencies', { }, this); } - // analyze queries for each container + // analyze queries for each container, at most MAX_CONCURRENT_REQUESTS at a time this.totalContainers = this.containers.length; - Ext4.each(this.containers, function(c){ - LABKEY.Ajax.request({ - url: LABKEY.ActionURL.buildURL('query', 'analyzeQueries.api', c), - method: 'GET', - scope: this, - success: function(resp, options){ - try { - fixupJsonResponse.call(this, LABKEY.Utils.decode(resp.responseText), resp, options, c); - } - catch (e) { - console.warn('Invalid JSON returned from analyzeQueries.api : ' + resp.responseText); - console.warn('Response URL : ' + resp.responseURL); - - // pass in a null json response and finish processing this container - fixupJsonResponse.call(this, null, resp, options, c); - } - }, - failure: function(resp, options){ - console.warn('Analyze query request failed : ' + resp.responseText); - fixupJsonResponse.call(this, {error: true}, resp, options, c); - } - }); - }, this); + pump.call(this); }, failure : function(json, resp, options) { var callback = LABKEY.Utils.getOnFailure(config); @@ -374,18 +448,98 @@ Ext4.define('LABKEY.query.browser.cache.QueryDependencies', { }, this); }, - removeContainer : function(container) { - let idx = this.containers.indexOf(container); - if (idx != -1) { - this.containers.splice(idx, 1); + /** + * Resolve, once per page load, the folders each analysis scope would process, so the UI can show the cost of an + * analysis before starting one and then hand the resolved list straight to analyzeQueries(). The site-level tree is + * a superset of the project-level one, so a single request covers both scopes. + * + * Workbooks are excluded: they hold no custom queries, but on some sites they outnumber real folders by orders of + * magnitude, and each one would otherwise cost an analyzeQueries.api request. + */ + loadContainerCounts : function(success, failure, scope) { + if (this.containerScopes) { + success.call(scope || this, this.containerScopes); + return; } + + LABKEY.Security.getContainers({ + containerPath : '/', + includeSubfolders : true, + includeWorkbookChildren : false, + // only id/name/path are needed, and resolving effective permissions for every folder on the site is by far + // the most expensive part of this call + includeStandardProperties : false, + includeEffectivePermissions : false, + scope : this, + success : function(json) { + let site = []; + + // a folder the user can't read is still in the tree if it has readable descendants, but analyzing it + // would just 403, so key off the id that toJSON() only emits when the user has read permission + function collect(container) { + if (container.id && container.path) { + site.push(container.path); + } + Ext4.each(container.children, collect); + } + collect(json); + + this.containerScopes = {'/' : site}; + + if (LABKEY.project) { + let projectPath = LABKEY.project.path.replace(/\/+$/, ''); + this.containerScopes[LABKEY.project.path] = Ext4.Array.filter(site, function(path) { + return path === projectPath || path.indexOf(projectPath + '/') === 0; + }); + } + + success.call(scope || this, this.containerScopes); + }, + failure : function(json, resp, options) { + if (Ext4.isFunction(failure)) { + failure.call(scope || this, json, resp, options); + } + } + }); + }, + + // one entry per container whose analysis failed, in the order the responses came back + getErrors : function() { + return this.errors || []; + }, + + // what the cached dependency graph covers: a null containerPath means only the folder the analysis was run from + getAnalysisScope : function() { + return {containerPath: this.analyzedContainerPath, folderCount: this.totalContainers}; + }, + + // the folders loadContainerCounts() resolved for an analysis scope, or undefined if it hasn't run + getScopeContainers : function(containerPath) { + return this.containerScopes ? this.containerScopes[containerPath] : undefined; + }, + + // abort an analysis in progress; responses that are already in flight are ignored rather than accumulated + cancel : function() { + this.cancelled = true; + this.containers = []; + this.activeContainers = {}; + this.activeCount = 0; + + let requests = this.activeRequests || {}; + this.activeRequests = {}; + Ext4.Object.each(requests, function(container, request) { + if (request && Ext4.isFunction(request.abort)) { + request.abort(); + } + }); }, // return the current progress for this loader getProgress : function() { return { currentContainer: this.currentContainer, - progress: 1.0 - (this.containers.length / this.totalContainers) + // totalContainers isn't known until the getContainers() call returns + progress: this.totalContainers ? (this.finishedCount / this.totalContainers) : 0 }; } }); \ No newline at end of file diff --git a/query/webapp/query/browser/view/Dependencies.js b/query/webapp/query/browser/view/Dependencies.js index bd7e948bb03..9bd8668af11 100644 --- a/query/webapp/query/browser/view/Dependencies.js +++ b/query/webapp/query/browser/view/Dependencies.js @@ -14,28 +14,37 @@ Ext4.define('LABKEY.query.browser.view.Dependencies', { this.addEvents('dependencychanged'); this.errorTpl = new Ext4.XTemplate( - '
An Error Occurred Analyzing Queries : {exception}
', - '
',
-                '
{url}
',
-                '
{exceptionClass}
',
-                '',
-                '
{.}
',
+                '
',
+                '
Errors during analysis
',
+                '',
+                    '
',
+                        '
{containerPath:htmlEncode}
',
+                        '',
+                            '
{.:htmlEncode}
',
+                        '',
+                    '
',
                 '',
-                '
', - '' + '' ); }, initComponent : function() { this.enableBubble('dependencychanged'); this.dependencyCache = LABKEY.query.browser.cache.QueryDependencies; + + // the query browser can be opened at the site root, where there is no current project to analyze + this.projectPath = LABKEY.project ? LABKEY.project.path : undefined; + this.analysisPath = this.projectPath || '/'; + + const loading = ''; + this.items = [{ xtype: 'box', cls: 'lk-cf-instructions', width: '75%', - html: 'This will allow an administrator to perform a query dependency analysis across folders. The user has ' + - 'the option to analyze at the site wide level which will include all folders on the server or at the project level. ' + - 'which will include the current project and all sub folders.' + html: 'By default, the Dependency Report shown when you view a query or table in the schema browser covers ' + + 'only the current folder. This analysis proactively loads references in other folders as well, ' + + 'so that dependencies from elsewhere on the server are included.' },{ xtype: 'form', border: false, @@ -46,56 +55,121 @@ Ext4.define('LABKEY.query.browser.view.Dependencies', { }, items: [{ xtype: 'radio', - fieldLabel: 'Site Level', - checked: true, + itemId: 'lk-cfd-project-scope', + fieldLabel: this.projectPath ? 'Current project, ' + Ext4.htmlEncode(this.projectPath) : 'Current project', + boxLabel: loading, + checked: !!this.projectPath, + disabled: !this.projectPath, name: 'depth', scope: this, handler: function (cmp, checked) { if (checked) - this.analysisPath = '/'; + this.analysisPath = this.projectPath; } },{ xtype: 'radio', - fieldLabel: 'Current Project Level', + itemId: 'lk-cfd-site-scope', + fieldLabel: 'Site-wide', + boxLabel: loading, + checked: !this.projectPath, name: 'depth', scope: this, handler: function (cmp, checked) { if (checked) - this.analysisPath = LABKEY.project.path; + this.analysisPath = '/'; } + },{ + xtype: 'box', + itemId: 'lk-cfd-scope-status', + padding: '5 0 0 0', + html: loading + ' Counting the folders each option would analyze. This can take a moment on a large site.' }], buttonAlign : 'left', buttons : [ - {text : 'Start Analysis', handler : this.startAnalysis, scope : this} + {text : 'Start Analysis', itemId: 'lk-cfd-start', disabled: true, handler : this.startAnalysis, scope : this} ] },{ xtype: 'box', + itemId: 'lk-cfd-error', padding: 10, - id: 'lk-dependency-progress-bar' + hidden: true }]; this.callParent(); + + this.on('afterrender', this.loadScopeCounts, this); + }, + + // resolve the folder counts up front so the cost of each option is visible before an analysis is started + loadScopeCounts : function() { + this.dependencyCache.loadContainerCounts(function(scopes) { + // the tab is closable, so it may be gone by the time the container tree comes back + if (this.isDestroyed) + return; + + this.setScopeCount('#lk-cfd-site-scope', scopes['/']); + this.setScopeCount('#lk-cfd-project-scope', this.projectPath ? scopes[this.projectPath] : undefined); + this.down('#lk-cfd-scope-status').hide(); + this.down('#lk-cfd-start').enable(); + }, function() { + if (this.isDestroyed) + return; + + this.setScopeCount('#lk-cfd-site-scope', undefined); + this.setScopeCount('#lk-cfd-project-scope', undefined); + this.down('#lk-cfd-scope-status').update('Unable to determine how many folders each option would analyze.'); + + // still startable: analyzeQueries() walks the container tree itself when it isn't handed a folder list + this.down('#lk-cfd-start').enable(); + }, this); + }, + + setScopeCount : function(itemId, containers) { + let radio = this.down(itemId); + if (radio) { + let label = ''; + if (containers) { + label = containers.length === 1 ? '1 folder' : containers.length.toLocaleString() + ' folders'; + } + radio.setBoxLabel(label); + } }, startAnalysis : function() { - // display a progress bar (even if it renders under the mask) - let pb = Ext4.create('Ext.ProgressBar', { - renderTo: 'lk-dependency-progress-bar', - width: 500 + // resolved by loadScopeCounts(), so the analysis doesn't walk the container tree a second time + let containers = this.dependencyCache.getScopeContainers(this.analysisPath); + + this.analysisRunning = true; + this.down('#lk-cfd-start').disable(); + this.hideError(); + + this.progressBar = Ext4.create('Ext.ProgressBar', {width: 500}); + + // modal, both to keep Stop Analysis reachable and because QueryDependencies is a singleton that a query + // details page would otherwise start a second, competing analysis on + this.progressWindow = Ext4.create('Ext.window.Window', { + title: 'Analyzing Query Dependencies', + modal: true, + closable: false, + draggable: false, + bodyPadding: 10, + items: [this.progressBar], + buttons: [{text: 'Stop Analysis', handler: this.stopAnalysis, scope: this}] }); - Ext4.TaskManager.start({ + this.progressWindow.show(); + + this.progressTask = Ext4.TaskManager.start({ interval: 250, delay: 1000, scope: this, run: function(){ let info = this.dependencyCache.getProgress(); - pb.updateProgress(info.progress, info.currentContainer, true); + if (this.progressBar) + this.progressBar.updateProgress(info.progress, info.currentContainer, true); } }); function loadSuccessHandler(json, resp, opts) { - pb.destroy(); - Ext4.TaskManager.stopAll(); - this.parent.getEl().unmask(); + this.endAnalysis(); Ext4.Msg.alert('Cross Folder Dependencies', 'The query analysis has completed successfully', function () { this.fireEvent('dependencychanged'); @@ -104,37 +178,83 @@ Ext4.define('LABKEY.query.browser.view.Dependencies', { } function loadFailureHandler(json, resp, opts) { - pb.destroy(); - Ext4.TaskManager.stopAll(); - this.parent.getEl().unmask(); - var error = this.getErrorMessageFromResponse(resp, opts); - var dialog = Ext4.create('Ext.window.Window', { - layout: 'fit', - draggable: false, - modal: true, - closable: true, - title: 'Error', - items: [{ - xtype: 'box', - tpl: this.errorTpl, - data: error, - autoScroll: true - }], - buttons: [{ - text: 'Close', - handler: function() { - dialog.close(); - } - }], - scope: this - }); - dialog.show(); + this.endAnalysis(); + this.showErrors(resp, opts); } // clear the cache and re-load using the configured path - this.parent.getEl().mask(); this.dependencyCache.clear(); - this.dependencyCache.load(this.analysisPath, loadSuccessHandler, loadFailureHandler, this); + this.dependencyCache.load(this.analysisPath, loadSuccessHandler, loadFailureHandler, this, containers); + }, + + stopAnalysis : function() { + if (!this.analysisRunning) + return; + + this.dependencyCache.cancel(); + this.endAnalysis(); + + // the partial graph isn't usable, so drop it and let a later analysis rebuild it + this.dependencyCache.clear(); + Ext4.Msg.alert('Cross Folder Dependencies', 'The query analysis was stopped.'); + }, + + endAnalysis : function() { + this.analysisRunning = false; + + if (this.progressTask) { + Ext4.TaskManager.stop(this.progressTask); + this.progressTask = undefined; + } + if (this.progressWindow) { + let win = this.progressWindow; + this.progressWindow = undefined; + this.progressBar = undefined; + + // destroys the progress bar along with it + win.close(); + } + this.down('#lk-cfd-start').enable(); + }, + + /** + * Lists every container whose analysis failed. resp/opts describe the failure that ended the analysis and are only + * used when it failed before any container was requested, such as when the container list itself couldn't be loaded. + */ + showErrors : function(resp, opts) { + let errors = this.dependencyCache.getErrors(); + if (errors.length === 0) + errors = [{containerPath: this.analysisPath, response: resp, options: opts}]; + + let byContainer = {}; + let grouped = []; + Ext4.each(errors, function(error) { + let messages = byContainer[error.containerPath]; + if (!messages) { + messages = byContainer[error.containerPath] = []; + grouped.push({containerPath: error.containerPath, messages: messages}); + } + messages.push(this.getErrorMessage(error.response, error.options)); + }, this); + + let box = this.down('#lk-cfd-error'); + box.update(this.errorTpl.apply(grouped)); + box.show(); + }, + + getErrorMessage : function(response, opts) { + // getErrorMessageFromResponse() reads responseURL off the response unconditionally + if (!response) + return 'Unknown error'; + + // a JSON body that carries no exception (a bare success:false, say) leaves nothing to show + return this.getErrorMessageFromResponse(response, opts).exception || 'Unknown error'; + }, + + hideError : function() { + let box = this.down('#lk-cfd-error'); + box.update(''); + box.hide(); }, getErrorMessageFromResponse : function (response, opts){ diff --git a/query/webapp/query/browser/view/QueryDetails.js b/query/webapp/query/browser/view/QueryDetails.js index 404569d7f78..646d29454b0 100644 --- a/query/webapp/query/browser/view/QueryDetails.js +++ b/query/webapp/query/browser/view/QueryDetails.js @@ -483,6 +483,9 @@ Ext4.define('LABKEY.query.browser.view.QueryDetails', { formatDependencies : function () { const dependencies = this.queriesCache.getDependencies(LABKEY.container.id, this.schemaName, this.queryName); + const heading = '

Dependency Report

'; + const subject = this.queryDetails.isUserDefined ? 'query' : 'table'; + const scope = this.formatDependencyScope(); // issue : 40993 sort dependencies by type, schemaName and name let sortFn = function(a, b){ @@ -505,8 +508,6 @@ Ext4.define('LABKEY.query.browser.view.QueryDetails', { dependencies.dependents.sort(sortFn); let tpl = new Ext4.XTemplate( - '

Dependency Report

', - 'The queries or tables that this query or table depends on and the queries or tables that depend on it.', '', '', '
', @@ -619,9 +620,34 @@ Ext4.define('LABKEY.query.browser.view.QueryDetails', { return { tag: 'div', cls: 'lk-qd-dependencies', - html: tpl.apply(dependencies) + // the heading and intro stay out of the XTemplate, which would parse a '{' in the scope's folder path + html: heading + + 'The queries or tables that this ' + subject + ' depends on and the queries or tables ' + + 'that depend on it. ' + scope + '' + + tpl.apply(dependencies) }; } + + // the analysis ran and found nothing; a failed analysis renders its own message instead + return { + tag: 'div', + cls: 'lk-qd-dependencies', + html: heading + 'There are no dependencies to or from this ' + subject + '. ' + scope + '' + }; + }, + + // states which folders the cached dependency graph was built from, since it covers only the current folder until a + // cross folder analysis is run + formatDependencyScope : function() { + const scope = this.queriesCache.getAnalysisScope(); + + if (!scope.containerPath) + return 'Searched this folder only.'; + + if (scope.containerPath === '/') + return 'Searched all folders on this site.'; + + return 'Searched ' + Ext4.htmlEncode(scope.containerPath) + ' and its subfolders.'; }, hasProperties: function (o) { @@ -839,6 +865,12 @@ Ext4.define('LABKEY.query.browser.view.QueryDetails', { renderQueryDetails : function() { this.getContent().removeAll(); + // analyzeQueries.api is backed by a premium service, and without it there is no graph to report on + if (!this.parent.hasQueryAnalysisService) { + this.getContent().add(this.formatQueryDetails(this.queryDetails)); + return; + } + // add a temporary placeholder for the query dependencies but don't block the entire page this.getContent().add(this.formatQueryDetails(this.queryDetails), { xtype : 'box', @@ -849,14 +881,20 @@ Ext4.define('LABKEY.query.browser.view.QueryDetails', { scope : this, fn : function(cmp) { cmp.getEl().mask('loading dependencies'); - this.queriesCache.load(null, this.refreshQueryDependencies, LABKEY.Utils.getCallbackWrapper(function(error) { + + let onError = LABKEY.Utils.getCallbackWrapper(function(error) { this.removeQueryDependencies(); this.getContent().add({ xtype : 'box', itemId : 'lk-dependency-report', html : '
Failed to load dependency information. ' + Ext4.htmlEncode(error.exception ? error.exception : ''), }); - }, this, true), this); + }, this, true); + + this.queriesCache.load(null, this.refreshQueryDependencies, function(result, response, options) { + // load() leads with the accumulated result, but the server's message is on the response + onError.call(this, response, options); + }, this); } } }