From 6042ab9cd04b247c7b03def9c4fa7f031318aa1b Mon Sep 17 00:00:00 2001 From: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com> Date: Wed, 11 Nov 2026 21:29:25 +0000 Subject: [PATCH] refactor(java): resolve the Sonar findings in the client and the Magic MDK plugin Reuse Model.NAME_OPTIONS for the options null-message, name the "elementId" key once in BridgeResult and DocGenBridge and the "Magic MDK: " title prefix once in OperationActions, flatten the nested ternaries in DocBookRenderer.rowCount and RunResult.rename into a guard and a Cell helper, let QueryResolver.parameters take its skip-or-keep decision from a helper returning the input parameter's symbol, make the long-to-double widening of an integer bound to a Real parameter explicit, and split PipelineEndToEndTest.v2TextualRunQuery into one test per stage it asserts. Co-Authored-By: jason.han --- .../java/org/openmbee/opensysml/Model.java | 2 +- .../mdk/magic/docgen/BridgeResult.java | 8 ++++-- .../mdk/magic/docgen/DocBookRenderer.java | 3 +- .../mdk/magic/actions/OperationActions.java | 7 +++-- .../mdk/magic/actions/QueryArguments.java | 2 +- .../mdk/magic/bridge/DocGenBridge.java | 8 ++++-- .../mdk/magic/engine/QueryResolver.java | 11 +++++--- .../openmbee/mdk/magic/results/RunResult.java | 13 +++++---- .../mdk/magic/PipelineEndToEndTest.java | 28 ++++++++++++++++--- 9 files changed, 57 insertions(+), 25 deletions(-) diff --git a/client/java/opensysml-client/src/main/java/org/openmbee/opensysml/Model.java b/client/java/opensysml-client/src/main/java/org/openmbee/opensysml/Model.java index cc635509ac..0279e30092 100644 --- a/client/java/opensysml-client/src/main/java/org/openmbee/opensysml/Model.java +++ b/client/java/opensysml-client/src/main/java/org/openmbee/opensysml/Model.java @@ -2334,7 +2334,7 @@ private void requireQuestion(VerifyOptions options) { private SymbolResponse symbolResponse(String symbolId, SymbolOptions options) { Objects.requireNonNull(symbolId, NAME_SYMBOL_ID); - Objects.requireNonNull(options, "options"); + Objects.requireNonNull(options, NAME_OPTIONS); if (options.includeLibraryMembers()) { connection.capabilities().require(Capabilities.SYMBOL_MEMBERS); } diff --git a/editors/mdk/mdk-bridge/src/main/java/org/openmbee/mdk/magic/docgen/BridgeResult.java b/editors/mdk/mdk-bridge/src/main/java/org/openmbee/mdk/magic/docgen/BridgeResult.java index 323a158e7b..c093ed6abd 100644 --- a/editors/mdk/mdk-bridge/src/main/java/org/openmbee/mdk/magic/docgen/BridgeResult.java +++ b/editors/mdk/mdk-bridge/src/main/java/org/openmbee/mdk/magic/docgen/BridgeResult.java @@ -75,11 +75,13 @@ public BridgeResult( tables = List.copyOf(tables); } + private static final String ELEMENT_ID = "elementId"; + public static BridgeResult from(Map flat) { List outcomes = rows(flat, "outcomes").stream() .map(row -> new Outcome( required(row, "label"), required(row, "detail"), required(row, "status"), - Optional.ofNullable(row.get("elementId")))) + Optional.ofNullable(row.get(ELEMENT_ID)))) .toList(); List diagnostics = rows(flat, "diagnostics").stream() .map(row -> new Diagnostic( @@ -112,11 +114,11 @@ private static List tables(Object value) { if (row.get("cells") instanceof List flatCells) { for (Object flatCell : flatCells) { if (flatCell instanceof Map cell) { - cells.add(new Cell(String.valueOf(cell.get("text")), optionalText(cell.get("elementId")))); + cells.add(new Cell(String.valueOf(cell.get("text")), optionalText(cell.get(ELEMENT_ID)))); } } } - rows.add(new Row(optionalText(row.get("elementId")), cells)); + rows.add(new Row(optionalText(row.get(ELEMENT_ID)), cells)); } } Object query = table.get("query"); diff --git a/editors/mdk/mdk-bridge/src/main/java/org/openmbee/mdk/magic/docgen/DocBookRenderer.java b/editors/mdk/mdk-bridge/src/main/java/org/openmbee/mdk/magic/docgen/DocBookRenderer.java index 4071e5e227..9136a6fe64 100644 --- a/editors/mdk/mdk-bridge/src/main/java/org/openmbee/mdk/magic/docgen/DocBookRenderer.java +++ b/editors/mdk/mdk-bridge/src/main/java/org/openmbee/mdk/magic/docgen/DocBookRenderer.java @@ -58,7 +58,8 @@ static DBTable rows(BridgeResult.Table answered) { } private static String rowCount(int rows) { - return rows == 0 ? "no rows" : rows + (rows == 1 ? " row" : " rows"); + if (rows == 0) return "no rows"; + return rows + (rows == 1 ? " row" : " rows"); } static String summary(BridgeResult result) { diff --git a/editors/mdk/plugin/src/main/java/org/openmbee/mdk/magic/actions/OperationActions.java b/editors/mdk/plugin/src/main/java/org/openmbee/mdk/magic/actions/OperationActions.java index 614e0627a7..3bc05cf28d 100644 --- a/editors/mdk/plugin/src/main/java/org/openmbee/mdk/magic/actions/OperationActions.java +++ b/editors/mdk/plugin/src/main/java/org/openmbee/mdk/magic/actions/OperationActions.java @@ -54,6 +54,7 @@ public final class OperationActions implements BrowserContextAMConfigurator, DiagramContextAMConfigurator { private static final String CATEGORY_ID = "org.openmbee.opensysml"; private static final String SYSML_EXTENSION = ".sysml"; + private static final String TITLE_PREFIX = "Magic MDK: "; private final Supplier engine; private final SelectionResolver resolver; @@ -331,11 +332,11 @@ public void actionPerformed(ActionEvent event) { RunRequest request = new RunRequest(operation, source, selection.subject().qualifiedName(), args); if (operation == Operation.RUN_QUERY) { ProgressStatusRunner.runWithProgressStatus( - status -> planQuery(request, index, status), "Magic MDK: " + operation.label(), true, 0); + status -> planQuery(request, index, status), TITLE_PREFIX + operation.label(), true, 0); return; } ProgressStatusRunner.runWithProgressStatus( - status -> run(request, index, status), "Magic MDK: " + operation.label(), true, 0); + status -> run(request, index, status), TITLE_PREFIX + operation.label(), true, 0); } /** Runs off the EDT: resolves the queries, runs at once when nothing needs binding, else asks on the EDT and runs again. */ @@ -369,7 +370,7 @@ private void planQuery(RunRequest request, IdentityResolver index, ProgressStatu } RunRequest bound = request.withQueryBindings(bindings.get()); ProgressStatusRunner.runWithProgressStatus( - progress -> run(bound, index, progress), "Magic MDK: " + operation.label(), true, 0); + progress -> run(bound, index, progress), TITLE_PREFIX + operation.label(), true, 0); }); } diff --git a/editors/mdk/plugin/src/main/java/org/openmbee/mdk/magic/actions/QueryArguments.java b/editors/mdk/plugin/src/main/java/org/openmbee/mdk/magic/actions/QueryArguments.java index fbdb187b72..a488db6e83 100644 --- a/editors/mdk/plugin/src/main/java/org/openmbee/mdk/magic/actions/QueryArguments.java +++ b/editors/mdk/plugin/src/main/java/org/openmbee/mdk/magic/actions/QueryArguments.java @@ -60,7 +60,7 @@ static DocumentValue value(String token, QueryParameter parameter) { case STRING -> new DocumentValue.StringValue(unquoted(scalar, token)); case INTEGER -> integer(token, parameter); case REAL -> { - if (scalar instanceof Value.IntegerValue item) yield new DocumentValue.RealValue(item.value()); + if (scalar instanceof Value.IntegerValue item) yield new DocumentValue.RealValue((double) item.value()); if (scalar instanceof Value.RealValue item) yield new DocumentValue.RealValue(item.value()); throw mismatch(token, parameter); } diff --git a/editors/mdk/plugin/src/main/java/org/openmbee/mdk/magic/bridge/DocGenBridge.java b/editors/mdk/plugin/src/main/java/org/openmbee/mdk/magic/bridge/DocGenBridge.java index 82f1e888fc..f7c3a18143 100644 --- a/editors/mdk/plugin/src/main/java/org/openmbee/mdk/magic/bridge/DocGenBridge.java +++ b/editors/mdk/plugin/src/main/java/org/openmbee/mdk/magic/bridge/DocGenBridge.java @@ -30,6 +30,8 @@ public final class DocGenBridge { public static final String METHOD = "docGen"; + private static final String ELEMENT_ID = "elementId"; + private final Supplier engine; private final SelectionResolver resolver; @@ -106,7 +108,7 @@ public static Map flatten(RunResult result) { row.put("label", outcome.label()); row.put("detail", outcome.detail()); row.put("status", outcome.status().name()); - if (outcome.elementId() != null) row.put("elementId", outcome.elementId()); + if (outcome.elementId() != null) row.put(ELEMENT_ID, outcome.elementId()); outcomes.add(row); } flat.put("outcomes", outcomes); @@ -130,12 +132,12 @@ public static Map flatten(RunResult result) { List> rows = new ArrayList<>(); for (RunResult.Row row : table.rows()) { Map flatRow = new LinkedHashMap<>(); - if (row.elementId() != null) flatRow.put("elementId", row.elementId()); + if (row.elementId() != null) flatRow.put(ELEMENT_ID, row.elementId()); List> cells = new ArrayList<>(); for (RunResult.Cell cell : row.cells()) { Map flatCell = new LinkedHashMap<>(); flatCell.put("text", cell.text()); - if (cell.elementId() != null) flatCell.put("elementId", cell.elementId()); + if (cell.elementId() != null) flatCell.put(ELEMENT_ID, cell.elementId()); cells.add(flatCell); } flatRow.put("cells", cells); diff --git a/editors/mdk/plugin/src/main/java/org/openmbee/mdk/magic/engine/QueryResolver.java b/editors/mdk/plugin/src/main/java/org/openmbee/mdk/magic/engine/QueryResolver.java index 81d9b52b8a..212f01767b 100644 --- a/editors/mdk/plugin/src/main/java/org/openmbee/mdk/magic/engine/QueryResolver.java +++ b/editors/mdk/plugin/src/main/java/org/openmbee/mdk/magic/engine/QueryResolver.java @@ -113,14 +113,17 @@ static List parameters(Model model, String query) { members.sort((a, b) -> Boolean.compare(a.inherited(), b.inherited())); Set named = new HashSet<>(); for (Symbol.Member member : members) { - if (member.library() || !named.add(member.name())) continue; - Optional symbol = model.findSymbol(member.id()); - if (symbol.isEmpty() || !"in".equals(symbol.get().metadata().get("direction"))) continue; - parameters.add(parameter(model, query, symbol.get())); + inputParameter(model, member, named).ifPresent(symbol -> parameters.add(parameter(model, query, symbol))); } return parameters; } + /** The member's symbol when it is the first {@code in} parameter of its name; a library member or a repeated name is none. */ + private static Optional inputParameter(Model model, Symbol.Member member, Set named) { + if (member.library() || !named.add(member.name())) return Optional.empty(); + return model.findSymbol(member.id()).filter(symbol -> "in".equals(symbol.metadata().get("direction"))); + } + /** Type and multiplicity come from the first declaration along the redefinition chain that states them. */ private static QueryParameter parameter(Model model, String query, Symbol symbol) { List chain = chain(model, symbol); diff --git a/editors/mdk/plugin/src/main/java/org/openmbee/mdk/magic/results/RunResult.java b/editors/mdk/plugin/src/main/java/org/openmbee/mdk/magic/results/RunResult.java index 08aa3c0bb9..5aead9c962 100644 --- a/editors/mdk/plugin/src/main/java/org/openmbee/mdk/magic/results/RunResult.java +++ b/editors/mdk/plugin/src/main/java/org/openmbee/mdk/magic/results/RunResult.java @@ -110,14 +110,17 @@ public RunResult withOutcomeNames(UnaryOperator rename) { } private static Row rename(Row row, UnaryOperator rename) { - List cells = row.cells().stream() - .map(cell -> cell.elementId() == null ? cell : new Cell( - cell.text().equals(cell.elementId()) ? rename.apply(cell.text()) : cell.text(), - rename.apply(cell.elementId()))) - .toList(); + List cells = row.cells().stream().map(cell -> rename(cell, rename)).toList(); return new Row(row.elementId() == null ? null : rename.apply(row.elementId()), cells); } + /** A cell whose text is its element id follows the renaming; any other text stays. */ + private static Cell rename(Cell cell, UnaryOperator rename) { + if (cell.elementId() == null) return cell; + String text = cell.text().equals(cell.elementId()) ? rename.apply(cell.text()) : cell.text(); + return new Cell(text, rename.apply(cell.elementId())); + } + public RunResult withLeadingDiagnostics(List leading) { if (leading.isEmpty()) return this; List merged = new ArrayList<>(leading); diff --git a/editors/mdk/plugin/src/test/java/org/openmbee/mdk/magic/PipelineEndToEndTest.java b/editors/mdk/plugin/src/test/java/org/openmbee/mdk/magic/PipelineEndToEndTest.java index ef87bd959e..88c51fdb21 100644 --- a/editors/mdk/plugin/src/test/java/org/openmbee/mdk/magic/PipelineEndToEndTest.java +++ b/editors/mdk/plugin/src/test/java/org/openmbee/mdk/magic/PipelineEndToEndTest.java @@ -4,6 +4,7 @@ import static org.junit.jupiter.api.Assertions.assertFalse; import static org.junit.jupiter.api.Assertions.assertTrue; +import java.io.IOException; import java.io.OutputStream; import java.nio.file.Files; import java.nio.file.Path; @@ -131,11 +132,14 @@ void v2TextualVerifyInstantiateAndExecute() throws Exception { assertEquals(Status.ERROR, missing.status(), missing.toString()); } - /** Run query on the v2 path: the queries of the fixture, their parameters, bindings, and the mapping of rows back. */ + private static V2TextualSource demo() throws IOException { + return new V2TextualSource(Files.readString(Path.of("src/test/resources/fixtures/demo.sysml"))); + } + + /** Run query on the v2 path: the rows a query answers, mapped back to the elements they stand for. */ @Test - void v2TextualRunQuery() throws Exception { - String text = Files.readString(Path.of("src/test/resources/fixtures/demo.sysml")); - V2TextualSource source = new V2TextualSource(text); + void v2TextualRunQueryMapsRowsBackToElements() throws Exception { + V2TextualSource source = demo(); var index = IdentityIndex.of(List.of( new SimpleElement("v", "Demo::Vehicle", "Vehicle", "vehicle"), new SimpleElement("w", "Demo::Wheel", "Wheel", "wheel"), @@ -153,7 +157,12 @@ void v2TextualRunQuery() throws Exception { parts.outcomes()); List plans = AnnotationPlanner.plan(parts, index); assertEquals(List.of("query", "vehicle", "wheel"), plans.stream().map(plan -> plan.target().handle()).toList()); + } + /** Run query on the v2 path: the parameters a plan reports, inherited and redefined ones included. */ + @Test + void v2TextualRunQueryPlansParameters() throws Exception { + V2TextualSource source = demo(); QueryPlan plan = engine.plan(new RunRequest(Operation.RUN_QUERY, source, "Demo::NoPorts", List.of()), () -> false).orElseThrow(); assertEquals(List.of("Demo::NoPorts"), plan.queries()); assertEquals(List.of( @@ -164,6 +173,12 @@ void v2TextualRunQuery() throws Exception { QueryPlan typed = engine.plan(new RunRequest(Operation.RUN_QUERY, source, "Demo::UnderPackages", List.of()), () -> false).orElseThrow(); assertEquals(List.of(new QueryParameter("Demo::UnderPackages", "roots", "KerML::Root::Element", QueryParameter.Kind.ELEMENT, true, false)), typed.parameters(), "a redefinition that types the parameter anew keeps the multiplicity it inherits"); + } + + /** Run query on the v2 path: bindings of many, none and one, and a parameter left unbound. */ + @Test + void v2TextualRunQueryBindsParameters() throws Exception { + V2TextualSource source = demo(); RunResult many = engine.run(new RunRequest(Operation.RUN_QUERY, source, "Demo::UnderPackages", List.of(), Map.of( "roots", List.of(new DocumentValue.ElementRef("Demo::Vehicle", ""), new DocumentValue.ElementRef("Demo::Lifecycle", "")))), () -> false); assertEquals(Status.PASSED, many.status(), many.toString()); @@ -185,7 +200,12 @@ void v2TextualRunQuery() throws Exception { RunResult unbound = run(Operation.RUN_QUERY, source, "Demo::PartsUnder"); assertEquals(Status.ERROR, unbound.status(), unbound.toString()); assertTrue(unbound.outcomes().get(0).detail().contains("requires binding root"), unbound.toString()); + } + /** Run query on the v2 path: a name the model lacks, a subject that is no query, and a binding no parameter takes. */ + @Test + void v2TextualRunQueryReportsWhatItCannotRun() throws Exception { + V2TextualSource source = demo(); RunResult unresolved = run(Operation.RUN_QUERY, source, "Demo::Unresolved"); assertEquals(Status.FAILED, unresolved.status(), "a name the model lacks is reported, not refused: " + unresolved); assertTrue(unresolved.outcomes().get(0).detail().contains("Demo::Nope"), unresolved.toString());