From af5f54274d6cb08ff2ac38c4be647c673cd332b0 Mon Sep 17 00:00:00 2001 From: James Brunton Date: Mon, 10 Aug 2026 12:00:16 +0100 Subject: [PATCH] Add defaults to calculations for ToolIO (#7289) # Description of Changes Currently when calculating the output file type for some tools, the system will get it wrong because it doesn't know about what the default parameters in tools are, so if it doesn't have a value for some key, it'll just bail out and say "it might not be compatible". This PR adds logic to `ToolIO` to read the default values set for the parameters if the tool has `ToolIOCase`s and takes them into account when figuring out the output type. I've built it with horrible Java reflection magic to avoid having to specify the default for params twice, which will make it impossible for the defaults to disagree with each other. This just runs once at startup so there's negligible performance impact. The change is easily tested with Change Parameters, which is just `add-password` behind the scenes but with the password params omitted (so Change Password is always PDF->PDF, never encrypted like Add Password). Also (somewhat hackily) fixes a bug I noticed where saving a Change Permissions step then leaving and returning to the pipeline will cause the step to be reloaded as Add Password. I've added a system to disambiguate tools which share the same endpoint (which is only these two currently). ## Currently image ## Now image --- .../swagger/ToolIOOperationCustomizer.java | 24 +++-- .../common/model/tool/ToolIOSpec.java | 46 ++++++++-- .../service/ToolIOParameterDefaults.java | 92 +++++++++++++++++++ .../common/service/ToolIORegistry.java | 5 +- .../ToolChainValidatorConformanceTest.java | 7 +- .../config/ToolIODeclarationCoverageTest.java | 33 ++++++- engine/scripts/generate_tool_models.py | 7 +- engine/src/stirling/models/tool_io.py | 29 ++++-- .../src/stirling/services/tool_io_compat.py | 9 +- .../scripts/generate-tool-api-types.mts | 2 + .../useChangePermissionsOperation.ts | 7 ++ .../hooks/tools/shared/toolAutomation.test.ts | 42 +++++++++ .../core/hooks/tools/shared/toolAutomation.ts | 28 +++++- .../hooks/tools/shared/toolOperationTypes.ts | 7 ++ frontend/editor/src/core/types/toolIO.ts | 32 +++++-- .../editor/src/core/utils/toolIOCompat.ts | 10 +- testing/tool-io-cases.json | 56 +++++++++++ 17 files changed, 390 insertions(+), 46 deletions(-) create mode 100644 app/common/src/main/java/stirling/software/common/service/ToolIOParameterDefaults.java diff --git a/app/common/src/main/java/stirling/software/common/config/swagger/ToolIOOperationCustomizer.java b/app/common/src/main/java/stirling/software/common/config/swagger/ToolIOOperationCustomizer.java index c7ea1b4f7c..b5c3a3ef87 100644 --- a/app/common/src/main/java/stirling/software/common/config/swagger/ToolIOOperationCustomizer.java +++ b/app/common/src/main/java/stirling/software/common/config/swagger/ToolIOOperationCustomizer.java @@ -1,5 +1,6 @@ package stirling.software.common.config.swagger; +import java.lang.reflect.Method; import java.util.Arrays; import java.util.LinkedHashMap; import java.util.List; @@ -18,6 +19,7 @@ import stirling.software.common.model.tool.ToolFormat; import stirling.software.common.model.tool.ToolIO; import stirling.software.common.model.tool.ToolIOCase; import stirling.software.common.model.tool.ToolIOWhen; +import stirling.software.common.service.ToolIOParameterDefaults; /** * Publishes each {@link ToolIO} into the spec as {@code x-stirling-io}, which is how the frontend @@ -49,40 +51,42 @@ public class ToolIOOperationCustomizer if (declaration == null) { return operation; } - operation.addExtension(EXTENSION_NAME, toExtension(declaration)); + operation.addExtension(EXTENSION_NAME, toExtension(declaration, handlerMethod.getMethod())); operation.setDescription(appendSummaryLine(operation.getDescription(), declaration)); return operation; } - private static Map toExtension(ToolIO declaration) { + private static Map toExtension(ToolIO declaration, Method handler) { Map extension = new LinkedHashMap<>(); extension.put("accepts", names(declaration.accepts())); extension.put("produces", declaration.produces().name()); extension.put("arity", declaration.arity().name()); if (declaration.cases().length > 0) { - extension.put("cases", cases(declaration)); + extension.put("cases", cases(declaration, handler)); } return extension; } - private static List> cases(ToolIO declaration) { - return Arrays.stream(declaration.cases()).map(ToolIOOperationCustomizer::toCase).toList(); + private static List> cases(ToolIO declaration, Method handler) { + return Arrays.stream(declaration.cases()).map(rule -> toCase(rule, handler)).toList(); } - private static Map toCase(ToolIOCase rule) { + private static Map toCase(ToolIOCase rule, Method handler) { Map entry = new LinkedHashMap<>(); - entry.put( - "when", - Arrays.stream(rule.when()).map(ToolIOOperationCustomizer::toCondition).toList()); + entry.put("when", Arrays.stream(rule.when()).map(c -> toCondition(c, handler)).toList()); entry.put("produces", rule.produces().name()); entry.put("arity", rule.arity().name()); return entry; } - private static Map toCondition(ToolIOWhen condition) { + private static Map toCondition(ToolIOWhen condition, Method handler) { Map entry = new LinkedHashMap<>(); entry.put("param", condition.param()); entry.put("matches", List.of(condition.matches())); + // The default the endpoint uses when this parameter is absent, so a step that never sends + // it still resolves. Omitted when the parameter is required with none. + ToolIOParameterDefaults.resolve(handler, condition.param()) + .ifPresent(value -> entry.put("default", value)); return entry; } diff --git a/app/common/src/main/java/stirling/software/common/model/tool/ToolIOSpec.java b/app/common/src/main/java/stirling/software/common/model/tool/ToolIOSpec.java index 146ae85133..cf627d97ef 100644 --- a/app/common/src/main/java/stirling/software/common/model/tool/ToolIOSpec.java +++ b/app/common/src/main/java/stirling/software/common/model/tool/ToolIOSpec.java @@ -5,13 +5,18 @@ import java.util.LinkedHashSet; import java.util.List; import java.util.Locale; import java.util.Map; +import java.util.Optional; import java.util.Set; /** The runtime form of a {@link ToolIO} declaration, read off a handler method once at startup. */ public record ToolIOSpec( Set accepts, ToolFormat produces, ToolArity arity, List cases) { - public record When(String param, List matches) { + /** + * @param paramDefault the value used when the parameter is absent, or null when it has no + * default - an absent parameter then leaves the case unresolved rather than defaulted. + */ + public record When(String param, List matches, String paramDefault) { boolean holdsFor(Object value) { String normalised = normalise(value); @@ -19,6 +24,14 @@ public record ToolIOSpec( } } + /** Supplies the default value a request parameter takes when a caller omits it. */ + @FunctionalInterface + public interface ParameterDefaults { + Optional defaultFor(String param); + + ParameterDefaults NONE = param -> Optional.empty(); + } + /** * Both sides of a condition are normalised at comparison, not at construction: the declaration * reaches the frontend and the engine as published data, and normalising only one side there @@ -44,25 +57,33 @@ public record ToolIOSpec( } public static ToolIOSpec from(ToolIO annotation) { + return from(annotation, ParameterDefaults.NONE); + } + + public static ToolIOSpec from(ToolIO annotation, ParameterDefaults defaults) { return new ToolIOSpec( new LinkedHashSet<>(Arrays.asList(annotation.accepts())), annotation.produces(), annotation.arity(), - Arrays.stream(annotation.cases()).map(ToolIOSpec::toCase).toList()); + Arrays.stream(annotation.cases()).map(rule -> toCase(rule, defaults)).toList()); } - private static Case toCase(ToolIOCase rule) { - List when = Arrays.stream(rule.when()).map(ToolIOSpec::toWhen).toList(); + private static Case toCase(ToolIOCase rule, ParameterDefaults defaults) { + List when = Arrays.stream(rule.when()).map(c -> toWhen(c, defaults)).toList(); return new Case(when, rule.produces(), rule.arity()); } - private static When toWhen(ToolIOWhen condition) { - return new When(condition.param(), List.of(condition.matches())); + private static When toWhen(ToolIOWhen condition, ParameterDefaults defaults) { + return new When( + condition.param(), + List.of(condition.matches()), + defaults.defaultFor(condition.param()).orElse(null)); } /** - * First matching {@link Case} wins. If none match but one reads a parameter we cannot see, the - * declared output comes back uncertain: a value we never saw might have picked another branch. + * First matching {@link Case} wins. A parameter the caller omitted resolves to its declared + * default; only a parameter with no default leaves the output uncertain, since an unseen value + * might then have picked another branch. * * @param parameters the step's configured parameters, or null when not known */ @@ -71,12 +92,17 @@ public record ToolIOSpec( for (Case rule : cases) { boolean allHold = true; for (When condition : rule.when()) { - if (parameters == null || !parameters.containsKey(condition.param())) { + Object value; + if (parameters != null && parameters.containsKey(condition.param())) { + value = parameters.get(condition.param()); + } else if (condition.paramDefault() != null) { + value = condition.paramDefault(); + } else { sawUnknownParam = true; allHold = false; continue; } - allHold &= condition.holdsFor(parameters.get(condition.param())); + allHold &= condition.holdsFor(value); } if (allHold) { return new Output(rule.produces(), rule.arity(), true); diff --git a/app/common/src/main/java/stirling/software/common/service/ToolIOParameterDefaults.java b/app/common/src/main/java/stirling/software/common/service/ToolIOParameterDefaults.java new file mode 100644 index 0000000000..d77ca63de6 --- /dev/null +++ b/app/common/src/main/java/stirling/software/common/service/ToolIOParameterDefaults.java @@ -0,0 +1,92 @@ +package stirling.software.common.service; + +import java.lang.reflect.Field; +import java.lang.reflect.Method; +import java.lang.reflect.Parameter; +import java.util.Optional; + +import io.swagger.v3.oas.annotations.media.Schema; + +import jakarta.validation.constraints.NotBlank; +import jakarta.validation.constraints.NotNull; + +import lombok.extern.slf4j.Slf4j; + +/** + * The value a request parameter takes when the caller omits it, read from the request model so a + * {@code @ToolIOCase} can be resolved even for a step that never sends the parameter it branches + * on. The default is read from the field so it cannot drift. + */ +@Slf4j +public final class ToolIOParameterDefaults { + + // Swagger's sentinel for an unset @Schema string member; not a real default value. + private static final String SCHEMA_UNSET = "##default"; + + private ToolIOParameterDefaults() {} + + /** + * The default {@code param} resolves to when absent, or empty when the parameter is required + * with no declared default - in which case an unset value leaves the output genuinely unknown + * rather than defaulted, and the chain reports it as uncertain. + * + *

Precedence: an explicit {@code @Schema(defaultValue)}, then the field's own value (a + * primitive's language default, or an initializer), then the empty string for an optional field + * left null, and finally empty for a required field with none of the above. + */ + public static Optional resolve(Method handler, String param) { + for (Parameter parameter : handler.getParameters()) { + Field field = findField(parameter.getType(), param); + if (field != null) { + return fromField(parameter.getType(), field); + } + } + return Optional.empty(); + } + + private static Optional fromField(Class owner, Field field) { + Schema schema = field.getAnnotation(Schema.class); + if (schema != null + && !schema.defaultValue().isEmpty() + && !SCHEMA_UNSET.equals(schema.defaultValue())) { + return Optional.of(schema.defaultValue()); + } + Object value = readField(owner, field); + if (value != null) { + return Optional.of(String.valueOf(value)); + } + return isRequired(field, schema) ? Optional.empty() : Optional.of(""); + } + + private static Object readField(Class owner, Field field) { + try { + Object instance = owner.getDeclaredConstructor().newInstance(); + field.setAccessible(true); + return field.get(instance); + } catch (ReflectiveOperationException | RuntimeException e) { + // A request model we cannot instantiate leaves the default unknown, which the check + // treats conservatively as uncertain. Never break startup over it. + log.warn("Could not read default of {}.{}", owner.getSimpleName(), field.getName(), e); + return null; + } + } + + private static boolean isRequired(Field field, Schema schema) { + if (schema != null && schema.requiredMode() == Schema.RequiredMode.REQUIRED) { + return true; + } + return field.isAnnotationPresent(NotNull.class) + || field.isAnnotationPresent(NotBlank.class); + } + + private static Field findField(Class type, String name) { + for (Class c = type; c != null && c != Object.class; c = c.getSuperclass()) { + try { + return c.getDeclaredField(name); + } catch (NoSuchFieldException ignored) { + // Try the superclass; request models extend a shared file-input base. + } + } + return null; + } +} diff --git a/app/common/src/main/java/stirling/software/common/service/ToolIORegistry.java b/app/common/src/main/java/stirling/software/common/service/ToolIORegistry.java index 754bfb8019..c2219536d1 100644 --- a/app/common/src/main/java/stirling/software/common/service/ToolIORegistry.java +++ b/app/common/src/main/java/stirling/software/common/service/ToolIORegistry.java @@ -67,7 +67,10 @@ public class ToolIORegistry implements ToolMetadataService, ToolIOSource { if (annotation == null) { return; } - ToolIOSpec spec = ToolIOSpec.from(annotation); + Method method = handler.getMethod(); + ToolIOSpec spec = + ToolIOSpec.from( + annotation, param -> ToolIOParameterDefaults.resolve(method, param)); for (String pattern : extractPatterns(info)) { target.put(pattern, spec); } diff --git a/app/common/src/test/java/stirling/software/common/service/ToolChainValidatorConformanceTest.java b/app/common/src/test/java/stirling/software/common/service/ToolChainValidatorConformanceTest.java index 67ba8be612..78c15d751d 100644 --- a/app/common/src/test/java/stirling/software/common/service/ToolChainValidatorConformanceTest.java +++ b/app/common/src/test/java/stirling/software/common/service/ToolChainValidatorConformanceTest.java @@ -101,7 +101,12 @@ class ToolChainValidatorConformanceTest { for (JsonNode match : condition.get("matches")) { matches.add(match.asString()); } - when.add(new ToolIOSpec.When(condition.get("param").asString(), matches)); + JsonNode paramDefault = condition.get("default"); + when.add( + new ToolIOSpec.When( + condition.get("param").asString(), + matches, + paramDefault == null ? null : paramDefault.asString())); } cases.add( new ToolIOSpec.Case( diff --git a/app/core/src/test/java/stirling/software/SPDF/config/ToolIODeclarationCoverageTest.java b/app/core/src/test/java/stirling/software/SPDF/config/ToolIODeclarationCoverageTest.java index 120bee6a2b..f6fe52f2a6 100644 --- a/app/core/src/test/java/stirling/software/SPDF/config/ToolIODeclarationCoverageTest.java +++ b/app/core/src/test/java/stirling/software/SPDF/config/ToolIODeclarationCoverageTest.java @@ -26,6 +26,7 @@ import org.springframework.web.bind.annotation.RequestMapping; import stirling.software.common.model.tool.ToolFormat; import stirling.software.common.model.tool.ToolIO; import stirling.software.common.model.tool.ToolIOSpec; +import stirling.software.common.service.ToolIOParameterDefaults; /** * Every document-transforming endpoint must declare its I/O, or it becomes a hole in the @@ -233,6 +234,32 @@ class ToolIODeclarationCoverageTest { assertEquals(Set.of("ps", "pcl", "xps"), declared); } + @Test + void anAbsentParameterResolvesToItsRequestModelDefault() { + // A pipeline step often omits a parameter a case branches on. The default is read from the + // request model, so the output resolves anyway instead of coming back uncertain. + + // Auto Rotate never sends dryRun; its default (false) means the JSON branch cannot fire. + assertEquals( + ToolFormat.PDF, + spec("/api/v1/misc/auto-rotate-pdf").resolveOutput(Map.of()).format()); + assertTrue(spec("/api/v1/misc/auto-rotate-pdf").resolveOutput(Map.of()).certain()); + + // Change Permissions posts to add-password with no password fields; both default to blank, + // so the unencrypted branch fires and it is not mistaken for producing an encrypted PDF. + assertEquals( + ToolFormat.PDF, + spec("/api/v1/security/add-password").resolveOutput(Map.of()).format()); + assertTrue(spec("/api/v1/security/add-password").resolveOutput(Map.of()).certain()); + } + + @Test + void aRequiredParameterWithNoDefaultStaysUncertainWhenAbsent() { + // pdf/text branches on outputFormat, which is required with no default. Absent, its output + // is genuinely txt-or-rtf-dependent, so it must remain uncertain rather than assume TEXT. + assertFalse(spec("/api/v1/convert/pdf/text").resolveOutput(Map.of()).certain()); + } + @Test void onlyRemovePasswordAcceptsAnEncryptedDocument() { assertTrue( @@ -288,7 +315,11 @@ class ToolIODeclarationCoverageTest { } required.add(full); if (declaration != null) { - declared.put(full, ToolIOSpec.from(declaration)); + declared.put( + full, + ToolIOSpec.from( + declaration, + param -> ToolIOParameterDefaults.resolve(method, param))); } } } diff --git a/engine/scripts/generate_tool_models.py b/engine/scripts/generate_tool_models.py index 2a5139a263..7587d8c62f 100644 --- a/engine/scripts/generate_tool_models.py +++ b/engine/scripts/generate_tool_models.py @@ -65,6 +65,8 @@ class ToolIOWhen(ApiModel): param: str matches: list[str] + # The value the endpoint uses when this parameter is absent; None when it has none. + default: str | None = None class ToolIOCase(ApiModel): @@ -378,7 +380,10 @@ def collect_tool_io(spec: dict[str, Any]) -> dict[str, dict[str, Any]]: def _render_when(condition: dict[str, Any]) -> str: - return f"ToolIOWhen(param={json.dumps(condition['param'])}, matches={json.dumps(condition['matches'])})" + parts = [f"param={json.dumps(condition['param'])}", f"matches={json.dumps(condition['matches'])}"] + if "default" in condition: + parts.append(f"default={json.dumps(condition['default'])}") + return f"ToolIOWhen({', '.join(parts)})" def _render_case(case: dict[str, Any]) -> str: diff --git a/engine/src/stirling/models/tool_io.py b/engine/src/stirling/models/tool_io.py index 8c95be59a1..da0cd5b9ae 100644 --- a/engine/src/stirling/models/tool_io.py +++ b/engine/src/stirling/models/tool_io.py @@ -58,6 +58,8 @@ class ToolIOWhen(ApiModel): param: str matches: list[str] + # The value the endpoint uses when this parameter is absent; None when it has none. + default: str | None = None class ToolIOCase(ApiModel): @@ -101,7 +103,7 @@ TOOL_IO: dict[ToolEndpoint, ToolIOSpec] = { arity=ToolArity.SIMO, cases=[ ToolIOCase( - when=[ToolIOWhen(param="singleOrMultiple", matches=["single"])], + when=[ToolIOWhen(param="singleOrMultiple", matches=["single"], default="multiple")], produces=ToolFormat.IMAGE, arity=ToolArity.SISO, ) @@ -130,15 +132,19 @@ TOOL_IO: dict[ToolEndpoint, ToolIOSpec] = { arity=ToolArity.SISO, cases=[ ToolIOCase( - when=[ToolIOWhen(param="outputFormat", matches=["ps"])], + when=[ToolIOWhen(param="outputFormat", matches=["ps"], default="eps")], produces=ToolFormat.POSTSCRIPT, arity=ToolArity.SISO, ), ToolIOCase( - when=[ToolIOWhen(param="outputFormat", matches=["pcl"])], produces=ToolFormat.PCL, arity=ToolArity.SISO + when=[ToolIOWhen(param="outputFormat", matches=["pcl"], default="eps")], + produces=ToolFormat.PCL, + arity=ToolArity.SISO, ), ToolIOCase( - when=[ToolIOWhen(param="outputFormat", matches=["xps"])], produces=ToolFormat.XPS, arity=ToolArity.SISO + when=[ToolIOWhen(param="outputFormat", matches=["xps"], default="eps")], + produces=ToolFormat.XPS, + arity=ToolArity.SISO, ), ], ), @@ -151,7 +157,7 @@ TOOL_IO: dict[ToolEndpoint, ToolIOSpec] = { arity=ToolArity.MIMO, cases=[ ToolIOCase( - when=[ToolIOWhen(param="combineIntoSinglePdf", matches=["true"])], + when=[ToolIOWhen(param="combineIntoSinglePdf", matches=["true"], default="false")], produces=ToolFormat.PDF, arity=ToolArity.MISO, ) @@ -202,7 +208,9 @@ TOOL_IO: dict[ToolEndpoint, ToolIOSpec] = { arity=ToolArity.SISO, cases=[ ToolIOCase( - when=[ToolIOWhen(param="dryRun", matches=["true"])], produces=ToolFormat.JSON, arity=ToolArity.SISO + when=[ToolIOWhen(param="dryRun", matches=["true"], default="false")], + produces=ToolFormat.JSON, + arity=ToolArity.SISO, ) ], ), @@ -223,7 +231,9 @@ TOOL_IO: dict[ToolEndpoint, ToolIOSpec] = { arity=ToolArity.SISO, cases=[ ToolIOCase( - when=[ToolIOWhen(param="sidecar", matches=["true"])], produces=ToolFormat.ZIP, arity=ToolArity.SISO + when=[ToolIOWhen(param="sidecar", matches=["true"], default="false")], + produces=ToolFormat.ZIP, + arity=ToolArity.SISO, ) ], ), @@ -242,7 +252,10 @@ TOOL_IO: dict[ToolEndpoint, ToolIOSpec] = { arity=ToolArity.SISO, cases=[ ToolIOCase( - when=[ToolIOWhen(param="password", matches=[""]), ToolIOWhen(param="ownerPassword", matches=[""])], + when=[ + ToolIOWhen(param="password", matches=[""], default=""), + ToolIOWhen(param="ownerPassword", matches=[""], default=""), + ], produces=ToolFormat.PDF, arity=ToolArity.SISO, ) diff --git a/engine/src/stirling/services/tool_io_compat.py b/engine/src/stirling/services/tool_io_compat.py index b2bb5566e9..f65f0efc22 100644 --- a/engine/src/stirling/services/tool_io_compat.py +++ b/engine/src/stirling/services/tool_io_compat.py @@ -89,11 +89,16 @@ def resolve_output(spec: ToolIOSpec, parameters: dict[str, object] | None) -> Re for rule in spec.cases: all_hold = True for condition in rule.when: - if parameters is None or condition.param not in parameters: + if parameters is not None and condition.param in parameters: + raw: object = parameters[condition.param] + elif condition.default is not None: + # The caller omitted it, so it takes the endpoint's default. + raw = condition.default + else: saw_unknown_param = True all_hold = False continue - normalised = _normalise(parameters[condition.param]) + normalised = _normalise(raw) all_hold = all_hold and any(_normalise(m) == normalised for m in condition.matches) if all_hold: return ResolvedOutput(format=rule.produces, arity=rule.arity, certain=True) diff --git a/frontend/editor/scripts/generate-tool-api-types.mts b/frontend/editor/scripts/generate-tool-api-types.mts index a805f2dfa3..d5990096af 100644 --- a/frontend/editor/scripts/generate-tool-api-types.mts +++ b/frontend/editor/scripts/generate-tool-api-types.mts @@ -258,6 +258,8 @@ export type ToolArity = ${union(vocabulary.arities as string[])}; export interface ToolIOWhen { param: string; matches: string[]; + /** The value the endpoint uses when this parameter is absent; omitted when it has none. */ + default?: string; } /** An output that applies when every condition in \`when\` holds. */ diff --git a/frontend/editor/src/core/hooks/tools/changePermissions/useChangePermissionsOperation.ts b/frontend/editor/src/core/hooks/tools/changePermissions/useChangePermissionsOperation.ts index ac5f9a7d96..fa249a6162 100644 --- a/frontend/editor/src/core/hooks/tools/changePermissions/useChangePermissionsOperation.ts +++ b/frontend/editor/src/core/hooks/tools/changePermissions/useChangePermissionsOperation.ts @@ -80,6 +80,13 @@ export const changePermissionsOperationConfig = defineSingleFileTool({ operationType: "changePermissions", endpoint: ENDPOINT, // Change Permissions is a fake endpoint for the Add Password tool defaultParameters, + // Both tools post to add-password. A permissions-only step carries none of the encryption + // fields, so it is this tool and not Add Password; keyLength, always sent by Add Password, + // is the reliable tell even when a password happens to be blank. + claimsStoredStep: (apiParams) => + !("password" in apiParams) && + !("ownerPassword" in apiParams) && + !("keyLength" in apiParams), }); export const useChangePermissionsOperation = () => { diff --git a/frontend/editor/src/core/hooks/tools/shared/toolAutomation.test.ts b/frontend/editor/src/core/hooks/tools/shared/toolAutomation.test.ts index f4cb974692..467e3ea5f7 100644 --- a/frontend/editor/src/core/hooks/tools/shared/toolAutomation.test.ts +++ b/frontend/editor/src/core/hooks/tools/shared/toolAutomation.test.ts @@ -24,6 +24,8 @@ import { SPLIT_METHODS } from "@app/constants/splitConstants"; import { redactOperationConfig } from "@app/hooks/tools/redact/useRedactOperation"; import { autoRotateOperationConfig } from "@app/hooks/tools/autoRotate/useAutoRotateOperation"; import { defaultParameters as autoRotateDefaults } from "@app/hooks/tools/autoRotate/useAutoRotateParameters"; +import { addPasswordOperationConfig } from "@app/hooks/tools/addPassword/useAddPasswordOperation"; +import { changePermissionsOperationConfig } from "@app/hooks/tools/changePermissions/useChangePermissionsOperation"; function entry(over: Partial): ToolRegistryEntry { return { @@ -239,6 +241,46 @@ describe("serialize/deserialize round-trip", () => { }); }); +describe("shared-endpoint disambiguation", () => { + const addPassword = entry({ + name: "Add Password", + automationSettings: NoopSettings, + operationConfig: asRegistryConfig(addPasswordOperationConfig), + }); + const changePermissions = entry({ + name: "Change Permissions", + automationSettings: NoopSettings, + operationConfig: asRegistryConfig(changePermissionsOperationConfig), + }); + const ADD_PASSWORD = "/api/v1/security/add-password"; + + // Permissions only, no encryption fields: this is Change Permissions. + const permsOnly = { + operation: ADD_PASSWORD, + parameters: { preventPrinting: true }, + }; + // Carries keyLength (and a password): this is Add Password, even with a blank owner password. + const withPassword = { + operation: ADD_PASSWORD, + parameters: { password: "s3cret", ownerPassword: "", keyLength: 256 }, + }; + + // Both share an endpoint, so the wrong one would win by registry order without a discriminator. + for (const [label, registry] of [ + ["add-password declared first", { addPassword, changePermissions }], + ["change-permissions declared first", { changePermissions, addPassword }], + ] as const) { + test(`each stored step reloads as its own tool (${label})`, () => { + expect(deserializeToolStep(permsOnly, registry).toolId).toBe( + "changePermissions", + ); + expect(deserializeToolStep(withPassword, registry).toolId).toBe( + "addPassword", + ); + }); + } +}); + describe("stepRequiresUpload", () => { const step = (params: Record): WorkingToolStep => ({ toolId: "compress" as ToolId, diff --git a/frontend/editor/src/core/hooks/tools/shared/toolAutomation.ts b/frontend/editor/src/core/hooks/tools/shared/toolAutomation.ts index d3874bc840..aab9e1ab10 100644 --- a/frontend/editor/src/core/hooks/tools/shared/toolAutomation.ts +++ b/frontend/editor/src/core/hooks/tools/shared/toolAutomation.ts @@ -226,11 +226,13 @@ function findToolByEndpoint( step: ToolApiStep, registry: Partial, ): [ToolId, ToolRegistryEntry] | undefined { + const staticMatches: [ToolId, ToolRegistryEntry][] = []; let dynamic: [ToolId, ToolRegistryEntry] | undefined; for (const [id, entry] of Object.entries(registry)) { const endpoint = entry?.operationConfig?.endpoint; if (typeof endpoint === "string") { - if (endpoint === step.operation) return [id as ToolId, entry]; + if (endpoint === step.operation) + staticMatches.push([id as ToolId, entry]); } else if (typeof endpoint === "function" && !dynamic) { const declared = entry?.operationConfig?.endpoints; const matched = declared @@ -239,9 +241,33 @@ function findToolByEndpoint( if (matched) dynamic = [id as ToolId, entry]; } } + if (staticMatches.length > 0) { + return disambiguateStaticMatches(staticMatches, step.parameters); + } return dynamic; } +/** + * Most endpoints belong to one tool, so the single match is returned unchanged. When several + * share an endpoint (Add Password and its permissions-only alias Change Permissions), prefer the + * specialised tool that claims the stored parameters; otherwise fall back to the general owner + * that declares no such claim. + */ +function disambiguateStaticMatches( + matches: [ToolId, ToolRegistryEntry][], + parameters: Record, +): [ToolId, ToolRegistryEntry] { + if (matches.length === 1) return matches[0]; + const claimed = matches.find(([, entry]) => + entry.operationConfig?.claimsStoredStep?.(parameters), + ); + if (claimed) return claimed; + const general = matches.find( + ([, entry]) => !entry.operationConfig?.claimsStoredStep, + ); + return general ?? matches[0]; +} + /** A stored step kept verbatim because its endpoint maps to no known tool. */ function unmappedStep(step: ToolApiStep): UnknownToolStep { return { diff --git a/frontend/editor/src/core/hooks/tools/shared/toolOperationTypes.ts b/frontend/editor/src/core/hooks/tools/shared/toolOperationTypes.ts index 60453c576f..d1f306a3f0 100644 --- a/frontend/editor/src/core/hooks/tools/shared/toolOperationTypes.ts +++ b/frontend/editor/src/core/hooks/tools/shared/toolOperationTypes.ts @@ -101,6 +101,13 @@ interface BaseToolOperationConfig { */ fromApiParams?(apiParams: ToolApiParams[TEndpoint]): Partial; + /** + * Whether a stored step belongs to this tool, used only to tell apart tools that share an endpoint. + * Receives the raw stored request body. Absent means the tool is the general owner of its + * endpoint and claims any step no specialised sibling claims. + */ + claimsStoredStep?(apiParams: Record): boolean; + /** * For custom tools: if true, success implies all input files were successfully processed. * Use this for tools like Automate or Merge where Many-to-One relationships exist diff --git a/frontend/editor/src/core/types/toolIO.ts b/frontend/editor/src/core/types/toolIO.ts index f8a034dcee..96572c65f4 100644 --- a/frontend/editor/src/core/types/toolIO.ts +++ b/frontend/editor/src/core/types/toolIO.ts @@ -72,6 +72,8 @@ export type ToolArity = "SISO" | "SIMO" | "MISO" | "MIMO"; export interface ToolIOWhen { param: string; matches: string[]; + /** The value the endpoint uses when this parameter is absent; omitted when it has none. */ + default?: string; } /** An output that applies when every condition in `when` holds. */ @@ -168,7 +170,13 @@ export const TOOL_IO: ToolIOTable = { arity: "SIMO", cases: [ { - when: [{ param: "singleOrMultiple", matches: ["single"] }], + when: [ + { + param: "singleOrMultiple", + matches: ["single"], + default: "multiple", + }, + ], produces: "IMAGE", arity: "SISO", }, @@ -207,17 +215,17 @@ export const TOOL_IO: ToolIOTable = { arity: "SISO", cases: [ { - when: [{ param: "outputFormat", matches: ["ps"] }], + when: [{ param: "outputFormat", matches: ["ps"], default: "eps" }], produces: "POSTSCRIPT", arity: "SISO", }, { - when: [{ param: "outputFormat", matches: ["pcl"] }], + when: [{ param: "outputFormat", matches: ["pcl"], default: "eps" }], produces: "PCL", arity: "SISO", }, { - when: [{ param: "outputFormat", matches: ["xps"] }], + when: [{ param: "outputFormat", matches: ["xps"], default: "eps" }], produces: "XPS", arity: "SISO", }, @@ -244,7 +252,13 @@ export const TOOL_IO: ToolIOTable = { arity: "MIMO", cases: [ { - when: [{ param: "combineIntoSinglePdf", matches: ["true"] }], + when: [ + { + param: "combineIntoSinglePdf", + matches: ["true"], + default: "false", + }, + ], produces: "PDF", arity: "MISO", }, @@ -432,7 +446,7 @@ export const TOOL_IO: ToolIOTable = { arity: "SISO", cases: [ { - when: [{ param: "dryRun", matches: ["true"] }], + when: [{ param: "dryRun", matches: ["true"], default: "false" }], produces: "JSON", arity: "SISO", }, @@ -485,7 +499,7 @@ export const TOOL_IO: ToolIOTable = { arity: "SISO", cases: [ { - when: [{ param: "sidecar", matches: ["true"] }], + when: [{ param: "sidecar", matches: ["true"], default: "false" }], produces: "ZIP", arity: "SISO", }, @@ -534,8 +548,8 @@ export const TOOL_IO: ToolIOTable = { cases: [ { when: [ - { param: "password", matches: [""] }, - { param: "ownerPassword", matches: [""] }, + { param: "password", matches: [""], default: "" }, + { param: "ownerPassword", matches: [""], default: "" }, ], produces: "PDF", arity: "SISO", diff --git a/frontend/editor/src/core/utils/toolIOCompat.ts b/frontend/editor/src/core/utils/toolIOCompat.ts index a5f9ea0fe9..473195ff7b 100644 --- a/frontend/editor/src/core/utils/toolIOCompat.ts +++ b/frontend/editor/src/core/utils/toolIOCompat.ts @@ -102,12 +102,18 @@ export function resolveOutput( for (const rule of spec.cases ?? []) { let allHold = true; for (const condition of rule.when) { - if (!parameters || !(condition.param in parameters)) { + let raw: unknown; + if (parameters && condition.param in parameters) { + raw = parameters[condition.param]; + } else if (condition.default !== undefined) { + // The caller omitted it, so it takes the endpoint's default. + raw = condition.default; + } else { sawUnknownParam = true; allHold = false; continue; } - const value = normalise(parameters[condition.param]); + const value = normalise(raw); allHold &&= condition.matches.some((match) => normalise(match) === value); } if (allHold) { diff --git a/testing/tool-io-cases.json b/testing/tool-io-cases.json index 59c2802f8d..996b2b693b 100644 --- a/testing/tool-io-cases.json +++ b/testing/tool-io-cases.json @@ -70,6 +70,33 @@ "arity": "SISO" } ] + }, + "autoRotateShape": { + "accepts": ["PDF"], + "produces": "PDF", + "arity": "SISO", + "cases": [ + { + "when": [{ "param": "dryRun", "matches": ["true"], "default": "false" }], + "produces": "JSON", + "arity": "SISO" + } + ] + }, + "changePermsShape": { + "accepts": ["PDF"], + "produces": "PDF_ENCRYPTED", + "arity": "SISO", + "cases": [ + { + "when": [ + { "param": "password", "matches": [""], "default": "" }, + { "param": "ownerPassword", "matches": [""], "default": "" } + ], + "produces": "PDF", + "arity": "SISO" + } + ] } }, "cases": [ @@ -250,6 +277,35 @@ ], "expected": [{ "stepIndex": 1, "severity": "INFO", "code": "fan-in" }] }, + { + "name": "an absent parameter takes its default, which here does not trigger the case", + "steps": [{ "spec": "autoRotateShape" }, { "spec": "pdfToPdf" }], + "expected": [] + }, + { + "name": "an explicit value overrides the default and triggers the case", + "steps": [ + { "spec": "autoRotateShape", "parameters": { "dryRun": "true" } }, + { "spec": "pdfToPdf" } + ], + "expected": [{ "stepIndex": 1, "severity": "ERROR", "code": "format-mismatch" }] + }, + { + "name": "an absent parameter whose default triggers the case resolves to that branch", + "steps": [{ "spec": "changePermsShape" }, { "spec": "pdfToPdf" }], + "expected": [] + }, + { + "name": "setting the branch parameter away from its default flips the outcome", + "steps": [ + { + "spec": "changePermsShape", + "parameters": { "password": "hunter2", "ownerPassword": "hunter2" } + }, + { "spec": "pdfToPdf" } + ], + "expected": [{ "stepIndex": 1, "severity": "ERROR", "code": "format-mismatch" }] + }, { "name": "an undeclared step warns and stops the chain being checked past it", "steps": [{ "spec": "addPassword" }, { "spec": null }, { "spec": "pdfToPdf" }],