From 03db2dee5c49f57659964c6774c4230116ca26ec Mon Sep 17 00:00:00 2001 From: Lucio Lelii Date: Mon, 14 Sep 2026 14:46:01 +0200 Subject: [PATCH] Ask an upload row which source it is on, then show only that branch The row offered both ways of giving a file at once and greyed out the one not in use, which still leaves the reader working out which half is live. It reads better as what it is: one choice, then the fields that choice needs - an input name, or which global input holds the file. Greying was all the schema could express, so this adds the annotation for showing a field only in the state it belongs to. The distinction earns the second annotation: a field that still tells the reader something while unavailable should stay and grey, but the branch nobody picked is not unavailable, it is irrelevant. A row saved before the choice existed says which branch it is on by what it carries, so it is stamped on read rather than left reading as the default and demanding an input name it never had. Co-Authored-By: Claude Opus 5 (1M context) --- .../configurations/JsonSchemaProducer.java | 75 ++++++++++++++++++- .../configurations/MCPAgentUploadInput.java | 65 +++++++++++----- .../annotations/UiEnabledWhen.java | 8 -- .../annotations/UiRequiredWhen.java | 3 - .../annotations/UiVisibleWhen.java | 25 +++++++ .../MCPAgentUploadGlobalSchemaTest.java | 30 ++++---- .../MCPAgentUploadInputJsonTest.java | 20 +++++ 7 files changed, 178 insertions(+), 48 deletions(-) create mode 100644 src/main/java/it/cnr/isti/workflow/manager/configurations/annotations/UiVisibleWhen.java diff --git a/src/main/java/it/cnr/isti/workflow/manager/blocks/configurations/JsonSchemaProducer.java b/src/main/java/it/cnr/isti/workflow/manager/blocks/configurations/JsonSchemaProducer.java index bdc51cd..3ee8e1b 100644 --- a/src/main/java/it/cnr/isti/workflow/manager/blocks/configurations/JsonSchemaProducer.java +++ b/src/main/java/it/cnr/isti/workflow/manager/blocks/configurations/JsonSchemaProducer.java @@ -45,6 +45,7 @@ import it.cnr.isti.workflow.manager.configurations.annotations.SchemaAllowedValu import it.cnr.isti.workflow.manager.configurations.annotations.UiDescription; import it.cnr.isti.workflow.manager.configurations.annotations.UiRequiredWhen; import it.cnr.isti.workflow.manager.configurations.annotations.UiEnabledWhen; +import it.cnr.isti.workflow.manager.configurations.annotations.UiVisibleWhen; import it.cnr.isti.workflow.manager.configurations.annotations.UiLabel; import it.cnr.isti.workflow.manager.configurations.annotations.UiOrder; import it.cnr.isti.workflow.manager.configurations.annotations.UiOptionsFromNode; @@ -99,6 +100,7 @@ public class JsonSchemaProducer { Map, Map> structuralMap = collectStructuralMetadata(type); Map, Map> uiOptionalGroupMap = collectUiOptionalGroupMetadata(type); Map, Map> uiEnabledWhenMap = collectUiEnabledWhenMetadata(type); + Map, Map> uiVisibleWhenMap = collectUiVisibleWhenMetadata(type); Map, Map> uiOrderMap = collectUiOrderMetadata(type); Map, Map> uiOptionsFromNodeMap = collectUiOptionsFromNodeMetadata(type); Map, Map> uiRequiredWhenMap = collectUiRequiredWhenMetadata(type); @@ -116,6 +118,7 @@ public class JsonSchemaProducer { applyStructuralMetadata(root, getMergedMetadata(structuralMap, type)); applyUiOptionalGroupMetadata(root, getMergedMetadata(uiOptionalGroupMap, type)); applyUiEnabledWhenMetadata(root, getMergedMetadata(uiEnabledWhenMap, type)); + applyUiVisibleWhenMetadata(root, getMergedMetadata(uiVisibleWhenMap, type)); applyUiOrderMetadata(root, type, getMergedMetadata(uiOrderMap, type)); applyUiOptionsFromNodeMetadata(root, getMergedMetadata(uiOptionsFromNodeMap, type)); applyUiRequiredWhenMetadata(root, getMergedMetadata(uiRequiredWhenMap, type)); @@ -162,6 +165,7 @@ public class JsonSchemaProducer { applyStructuralMetadata(classSchema, getMergedMetadata(structuralMap, matchedClass)); applyUiOptionalGroupMetadata(classSchema, getMergedMetadata(uiOptionalGroupMap, matchedClass)); applyUiEnabledWhenMetadata(classSchema, getMergedMetadata(uiEnabledWhenMap, matchedClass)); + applyUiVisibleWhenMetadata(classSchema, getMergedMetadata(uiVisibleWhenMap, matchedClass)); applyUiOrderMetadata(classSchema, matchedClass, getMergedMetadata(uiOrderMap, matchedClass)); applyUiOptionsFromNodeMetadata(classSchema, getMergedMetadata(uiOptionsFromNodeMap, matchedClass)); applyUiRequiredWhenMetadata(classSchema, getMergedMetadata(uiRequiredWhenMap, matchedClass)); @@ -1311,6 +1315,73 @@ public class JsonSchemaProducer { } } + private Map, Map> collectUiVisibleWhenMetadata(Class rootClass) { + Map, Map> result = new HashMap<>(); + Set> visited = new HashSet<>(); + Queue> queue = new ArrayDeque<>(); + queue.add(rootClass); + + while (!queue.isEmpty()) { + Class current = queue.poll(); + if (current == null || !visited.add(current) || isTerminalType(current)) { + continue; + } + + Map metadata = new LinkedHashMap<>(); + for (Field field : current.getDeclaredFields()) { + UiVisibleWhen annotation = field.getAnnotation(UiVisibleWhen.class); + if (annotation != null) { + metadata.put(field.getName(), annotation); + } + enqueueRelatedTypes(queue, field.getGenericType(), field.getType()); + } + + if (current.isRecord()) { + for (RecordComponent component : current.getRecordComponents()) { + UiVisibleWhen annotation = component.getAnnotation(UiVisibleWhen.class); + if (annotation != null) { + metadata.put(component.getName(), annotation); + } + enqueueRelatedTypes(queue, component.getGenericType(), component.getType()); + } + } + + if (!metadata.isEmpty()) { + result.put(current, metadata); + } + } + + return result; + } + + private void applyUiVisibleWhenMetadata(ObjectNode classSchema, Map metadata) { + if (metadata == null || metadata.isEmpty()) { + return; + } + JsonNode propertiesNode = classSchema.get("properties"); + if (!(propertiesNode instanceof ObjectNode properties)) { + return; + } + for (Entry entry : metadata.entrySet()) { + JsonNode propNode = properties.get(entry.getKey()); + if (!(propNode instanceof ObjectNode propertySchema)) { + continue; + } + UiVisibleWhen dependency = entry.getValue(); + ObjectNode visibleWhen = propertySchema.putObject("x-ui-visible-when"); + visibleWhen.put("field", dependency.field()); + if (!dependency.equals().isBlank()) { + visibleWhen.put("equals", dependency.equals()); + } + if (dependency.equalsAny().length > 0) { + ArrayNode equalsAny = visibleWhen.putArray("in"); + for (String value : dependency.equalsAny()) { + equalsAny.add(value); + } + } + } + } + private Map, Map> collectUiEnabledWhenMetadata(Class rootClass) { Map, Map> result = new HashMap<>(); Set> visited = new HashSet<>(); @@ -1466,8 +1537,6 @@ public class JsonSchemaProducer { } if (dependency.present()) { enabledWhen.put("present", true); - } else if (dependency.absent()) { - enabledWhen.put("present", false); } } } @@ -1527,8 +1596,6 @@ public class JsonSchemaProducer { } if (dependency.present()) { requiredWhen.put("present", true); - } else if (dependency.absent()) { - requiredWhen.put("present", false); } } } diff --git a/src/main/java/it/cnr/isti/workflow/manager/blocks/configurations/MCPAgentUploadInput.java b/src/main/java/it/cnr/isti/workflow/manager/blocks/configurations/MCPAgentUploadInput.java index 00894d6..94d3618 100644 --- a/src/main/java/it/cnr/isti/workflow/manager/blocks/configurations/MCPAgentUploadInput.java +++ b/src/main/java/it/cnr/isti/workflow/manager/blocks/configurations/MCPAgentUploadInput.java @@ -8,30 +8,37 @@ import it.cnr.isti.workflow.manager.configurations.annotations.FieldRetriever; import it.cnr.isti.workflow.manager.configurations.annotations.SchemaAllowedValues; import it.cnr.isti.workflow.manager.configurations.annotations.UiContextKeys; import it.cnr.isti.workflow.manager.configurations.annotations.UiDescription; -import it.cnr.isti.workflow.manager.configurations.annotations.UiEnabledWhen; import it.cnr.isti.workflow.manager.configurations.annotations.UiLabel; import it.cnr.isti.workflow.manager.configurations.annotations.UiRequiredWhen; +import it.cnr.isti.workflow.manager.configurations.annotations.UiVisibleWhen; import jakarta.validation.constraints.Size; /** * Declares a file that is forwarded with an MCP agent query. * *

Where the file comes from is the one thing this has to say, and the two ways are alternatives: - * a port of its own, named by {@link #name}, or a global input of the flow, named by - * {@link #globalInput}. Filling either one greys out the other, so the row states one source rather - * than leaving both on offer and the reader guessing which wins. + * a port of its own, uploaded to the step, or a global input of the flow. {@link #source} says + * which, and only that branch is shown - the fields of the other one are not unavailable, they are + * irrelevant, and leaving them on screen asks the reader to work out which half of the row is live. */ public record MCPAgentUploadInput( /** - * The port this file is uploaded to, which is why it is only meaningful without a global: - * it is the input's name, and an attachment taken from a global has no port to name. + * Which of the two ways this file arrives. The default lives in {@link #effectiveSource()} + * and is declared here too, so the editor can say which one an untouched row is using rather + * than only that it is using one. */ + @SchemaAllowedValues(value = { SOURCE_INPUT, SOURCE_GLOBAL }, defaultValue = SOURCE_INPUT) + @UiLabel("File comes from") + @UiDescription("Uploaded to an input of this step, or taken from a global input of the flow.") + @JsonProperty(required = false) String source, + + /** The port this file is uploaded to: the input's name. */ @Size(max = 64) @UiLabel("Input name") @UiDescription("Name of the input this file is uploaded to.") - @UiEnabledWhen(field = "globalInput", absent = true) - @UiRequiredWhen(field = "globalInput", absent = true) + @UiVisibleWhen(field = "source", equalsAny = { SOURCE_INPUT, "" }) + @UiRequiredWhen(field = "source", equalsAny = { SOURCE_INPUT, "" }) @JsonProperty(required = false) String name, /** @@ -44,47 +51,67 @@ public record MCPAgentUploadInput( @SchemaAllowedValues({ "IMAGE", "DOCUMENT" }) @UiLabel("Expected kind") @UiDescription("Filters the file picker. What the file is taken to be is read from the file itself.") - @UiEnabledWhen(field = "globalInput", absent = true) + @UiVisibleWhen(field = "source", equalsAny = { SOURCE_INPUT, "" }) @JsonProperty(required = false) MCPAgentUploadKind kind, @UiLabel("Several files") - @UiEnabledWhen(field = "globalInput", absent = true) + @UiVisibleWhen(field = "source", equalsAny = { SOURCE_INPUT, "" }) @JsonProperty(required = false) Boolean multiple, /** - * The flow global input this attachment is taken from, instead of a port of its own. + * The flow global input this attachment is taken from. * - *

Left blank, the block grows an input port and the file is uploaded to that step. Named, - * the file is whatever the run's global input holds - so one document uploaded once at the + *

The file is whatever the run's global input holds, so one document uploaded once at the * start of a run can be attached by every agent that needs it, rather than being uploaded * again per step. A global input reaches a block only by being named like this; it never - * feeds a port. + * feeds a port, which is why this branch grows no input on the block. */ - @UiLabel("From global input") - @UiDescription("Take the file from a global input of the flow instead of uploading it to this step.") - @UiEnabledWhen(field = "name", absent = true) + @UiLabel("Global input") + @UiDescription("Which global input of the flow holds the file.") + @UiVisibleWhen(field = "source", equals = SOURCE_GLOBAL) + @UiRequiredWhen(field = "source", equals = SOURCE_GLOBAL) @FieldRetriever(name = "GlobalInputs", url = "/secure-retriever/GlobalInputs/inputs/items?type=FILE", dependsOn = { UiContextKeys.FLOW_ID }, requiresAuth = true) @JsonProperty(required = false) String globalInput) { + public static final String SOURCE_INPUT = "INPUT"; + public static final String SOURCE_GLOBAL = "GLOBAL"; + /** * Absent means single, which is what a half-filled row means while it is being configured. The * component was a primitive, so the editor posting a row before that box had been touched failed * to deserialize at all, and the block update came back as a bad request naming "multiple" - * with nothing in the editor to connect that to the row being filled in. + * + *

A row written before the choice existed says which branch it is on by what it carries, so + * it is stamped here rather than left reading as the default and demanding an input name it + * never had. */ public MCPAgentUploadInput { multiple = multiple != null && multiple; + if ((source == null || source.isBlank()) && globalInput != null && !globalInput.isBlank()) { + source = SOURCE_GLOBAL; + } } public MCPAgentUploadInput(String name, MCPAgentUploadKind kind, boolean multiple) { - this(name, kind, multiple, null); + this(null, name, kind, multiple, null); + } + + public MCPAgentUploadInput(String name, MCPAgentUploadKind kind, boolean multiple, String globalInput) { + this(null, name, kind, multiple, globalInput); + } + + @JsonIgnore + public String effectiveSource() { + return source == null || source.isBlank() ? SOURCE_INPUT : source; } /** Whether this attachment comes from a global input rather than a port of its own. */ @JsonIgnore public boolean isFromGlobalInput() { - return globalInput != null && !globalInput.isBlank(); + return SOURCE_GLOBAL.equalsIgnoreCase(effectiveSource()) + && globalInput != null && !globalInput.isBlank(); } /** diff --git a/src/main/java/it/cnr/isti/workflow/manager/configurations/annotations/UiEnabledWhen.java b/src/main/java/it/cnr/isti/workflow/manager/configurations/annotations/UiEnabledWhen.java index 3598ad2..05f7e53 100644 --- a/src/main/java/it/cnr/isti/workflow/manager/configurations/annotations/UiEnabledWhen.java +++ b/src/main/java/it/cnr/isti/workflow/manager/configurations/annotations/UiEnabledWhen.java @@ -16,14 +16,6 @@ public @interface UiEnabledWhen { boolean present() default false; - /** - * Enabled only while the referenced field is empty, which is how two fields that are - * alternatives to each other say so: filling either one greys out the other. Distinct from - * {@link #present()} because a primitive boolean cannot tell "not specified" from "false", so - * the absent case needed a flag of its own. - */ - boolean absent() default false; - String group() default ""; } diff --git a/src/main/java/it/cnr/isti/workflow/manager/configurations/annotations/UiRequiredWhen.java b/src/main/java/it/cnr/isti/workflow/manager/configurations/annotations/UiRequiredWhen.java index 50b7181..3685d4e 100644 --- a/src/main/java/it/cnr/isti/workflow/manager/configurations/annotations/UiRequiredWhen.java +++ b/src/main/java/it/cnr/isti/workflow/manager/configurations/annotations/UiRequiredWhen.java @@ -16,7 +16,4 @@ public @interface UiRequiredWhen { boolean present() default false; - /** Required only while the referenced field is empty. See UiEnabledWhen#absent. */ - boolean absent() default false; - } diff --git a/src/main/java/it/cnr/isti/workflow/manager/configurations/annotations/UiVisibleWhen.java b/src/main/java/it/cnr/isti/workflow/manager/configurations/annotations/UiVisibleWhen.java new file mode 100644 index 0000000..cc0344f --- /dev/null +++ b/src/main/java/it/cnr/isti/workflow/manager/configurations/annotations/UiVisibleWhen.java @@ -0,0 +1,25 @@ +package it.cnr.isti.workflow.manager.configurations.annotations; + +import java.lang.annotation.ElementType; +import java.lang.annotation.Retention; +import java.lang.annotation.RetentionPolicy; +import java.lang.annotation.Target; + +/** + * Shows a field only in the state it belongs to, rather than greying it out there. + * + *

The difference is worth an annotation of its own. {@link UiEnabledWhen} suits a field that + * still tells the reader something while it is unavailable - a name that will be used once the mode + * changes. It suits a branch of a choice badly: the fields of the branch nobody picked are not + * unavailable, they are irrelevant, and leaving them on screen greyed asks the reader to work out + * which half of the form is the live one. + */ +@Target({ ElementType.FIELD, ElementType.RECORD_COMPONENT }) +@Retention(RetentionPolicy.RUNTIME) +public @interface UiVisibleWhen { + String field(); + + String equals() default ""; + + String[] equalsAny() default {}; +} diff --git a/src/test/java/it/cnr/isti/workflow/manager/blocks/configurations/MCPAgentUploadGlobalSchemaTest.java b/src/test/java/it/cnr/isti/workflow/manager/blocks/configurations/MCPAgentUploadGlobalSchemaTest.java index 1ba07ef..2be7e7b 100644 --- a/src/test/java/it/cnr/isti/workflow/manager/blocks/configurations/MCPAgentUploadGlobalSchemaTest.java +++ b/src/test/java/it/cnr/isti/workflow/manager/blocks/configurations/MCPAgentUploadGlobalSchemaTest.java @@ -2,7 +2,6 @@ package it.cnr.isti.workflow.manager.blocks.configurations; import static org.junit.jupiter.api.Assertions.assertEquals; import static org.junit.jupiter.api.Assertions.assertNotNull; -import static org.junit.jupiter.api.Assertions.assertFalse; import static org.junit.jupiter.api.Assertions.assertNull; import static org.junit.jupiter.api.Assertions.assertTrue; @@ -80,23 +79,26 @@ public class MCPAgentUploadGlobalSchemaTest { } @Test - public void theTwoWaysToGiveAFileAreOfferedAsAlternatives() { - // Filling either greys out the other, so the row states one source instead of leaving both - // on offer with nothing saying which one wins. + public void theRowAsksWhichOfTheTwoSourcesItIsOn() { + // One choice up front, then only that branch: the other branch's fields are not unavailable, + // they are irrelevant. JsonNode upload = schemaProducer.generateSchemaNode(MCPAgentBlockConfiguration.class) .get("definitions").get("MCPAgentUploadInput").get("properties"); - assertEquals("globalInput", upload.get("name").get("x-ui-enabled-when").get("field").asString()); - assertFalse(upload.get("name").get("x-ui-enabled-when").get("present").asBoolean()); - assertEquals("globalInput", upload.get("kind").get("x-ui-enabled-when").get("field").asString()); - assertEquals("globalInput", upload.get("multiple").get("x-ui-enabled-when").get("field").asString()); + assertEquals("INPUT", upload.get("source").get("default").asString()); - assertEquals("name", upload.get("globalInput").get("x-ui-enabled-when").get("field").asString()); - assertFalse(upload.get("globalInput").get("x-ui-enabled-when").get("present").asBoolean()); + for (String branchField : new String[] { "name", "kind", "multiple" }) { + JsonNode rule = upload.get(branchField).get("x-ui-visible-when"); + assertEquals("source", rule.get("field").asString(), branchField); + assertEquals("INPUT", rule.get("in").get(0).asString(), branchField); + } - // And a port with no name is the one thing a row cannot be saved as, so it is required only - // while no global is chosen. - assertEquals("globalInput", upload.get("name").get("x-ui-required-when").get("field").asString()); - assertFalse(upload.get("name").get("x-ui-required-when").get("present").asBoolean()); + JsonNode globalRule = upload.get("globalInput").get("x-ui-visible-when"); + assertEquals("source", globalRule.get("field").asString()); + assertEquals("GLOBAL", globalRule.get("equals").asString()); + + // Each branch asks for the one thing it cannot do without, and only on its own branch. + assertEquals("source", upload.get("name").get("x-ui-required-when").get("field").asString()); + assertEquals("GLOBAL", upload.get("globalInput").get("x-ui-required-when").get("equals").asString()); } } diff --git a/src/test/java/it/cnr/isti/workflow/manager/blocks/configurations/MCPAgentUploadInputJsonTest.java b/src/test/java/it/cnr/isti/workflow/manager/blocks/configurations/MCPAgentUploadInputJsonTest.java index 3904d29..78da801 100644 --- a/src/test/java/it/cnr/isti/workflow/manager/blocks/configurations/MCPAgentUploadInputJsonTest.java +++ b/src/test/java/it/cnr/isti/workflow/manager/blocks/configurations/MCPAgentUploadInputJsonTest.java @@ -94,4 +94,24 @@ public class MCPAgentUploadInputJsonTest { assertEquals(MCPAgentUploadInput.MCPAgentUploadKind.DOCUMENT, upload.kind()); } + + @Test + public void stampsARowWrittenBeforeTheChoiceExisted() { + // Saved flows carry rows with no source at all. What such a row holds says which branch it + // is on, so it must not read as the default and demand an input name it never had. + MCPAgentUploadInput upload = mapper.readValue( + "{\"globalInput\":\"document\"}", MCPAgentUploadInput.class); + + assertEquals(MCPAgentUploadInput.SOURCE_GLOBAL, upload.source()); + assertTrue(upload.isFromGlobalInput()); + } + + @Test + public void aRowWithNoSourceAndNoGlobalIsAnInputRow() { + MCPAgentUploadInput upload = mapper.readValue( + "{\"name\":\"planDoc\"}", MCPAgentUploadInput.class); + + assertEquals(MCPAgentUploadInput.SOURCE_INPUT, upload.effectiveSource()); + assertFalse(upload.isFromGlobalInput()); + } }