From 3b9f0c999770535d447beef09f7a2550c74172eb Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Manuel=20Sugawara=20=28=E2=88=A9=EF=BD=80-=C2=B4=29?= =?UTF-8?q?=E2=8A=83=E2=94=81=E7=82=8E=E7=82=8E=E7=82=8E=E7=82=8E=E7=82=8E?= Date: Wed, 20 Aug 2025 11:26:10 -0700 Subject: [PATCH 1/2] Fix a bug that prevented warning events to show up for diff CLI command The severity is set to DANGER to load the old and new models to avoid events of lesser severity to show up during loading. After this was done the option was mutated and the attempt to restore the default severity didn't work. Now, the validation option keeps a different field to distinguish the value read from the CLI from overrides to be able to tell them apart and properly reset these when needed. --- .../smithy/cli/commands/ModelBuilder.java | 8 ++--- .../smithy/cli/commands/ValidatorOptions.java | 29 ++++++++++++++++--- .../smithy/cli/commands/DiffCommandTest.java | 29 +++++++++++++++++++ .../smithy/cli/commands/diff/new2.smithy | 14 +++++++++ .../smithy/cli/commands/diff/old2.smithy | 10 +++++++ 5 files changed, 81 insertions(+), 9 deletions(-) create mode 100644 smithy-cli/src/test/resources/software/amazon/smithy/cli/commands/diff/new2.smithy create mode 100644 smithy-cli/src/test/resources/software/amazon/smithy/cli/commands/diff/old2.smithy diff --git a/smithy-cli/src/main/java/software/amazon/smithy/cli/commands/ModelBuilder.java b/smithy-cli/src/main/java/software/amazon/smithy/cli/commands/ModelBuilder.java index 5198c26814e..9adc6bda1f2 100644 --- a/smithy-cli/src/main/java/software/amazon/smithy/cli/commands/ModelBuilder.java +++ b/smithy-cli/src/main/java/software/amazon/smithy/cli/commands/ModelBuilder.java @@ -298,11 +298,9 @@ private static void discoverModelsWithClasspath(String rawClasspath, ModelAssemb } // Determine a default severity if one wasn't given, by inspecting if there is a --severity option. - private Severity resolveMinSeverity(StandardOptions standardOptions, ValidatorOptions validatorOption) { - if (defaultSeverity != null && validatorOption.severity() == null) { - validatorOption.severity(defaultSeverity); - } - return validatorOption.detectAndGetSeverity(standardOptions); + private void resolveMinSeverity(StandardOptions standardOptions, ValidatorOptions validatorOption) { + validatorOption.severityOverride(defaultSeverity); + validatorOption.detectAndSetSeverity(standardOptions); } static ModelAssembler createModelAssembler(ClassLoader classLoader) { diff --git a/smithy-cli/src/main/java/software/amazon/smithy/cli/commands/ValidatorOptions.java b/smithy-cli/src/main/java/software/amazon/smithy/cli/commands/ValidatorOptions.java index 3d15694d3ec..663741f1408 100644 --- a/smithy-cli/src/main/java/software/amazon/smithy/cli/commands/ValidatorOptions.java +++ b/smithy-cli/src/main/java/software/amazon/smithy/cli/commands/ValidatorOptions.java @@ -24,6 +24,7 @@ final class ValidatorOptions implements ArgumentReceiver { static final String SHOW_VALIDATORS = "--show-validators"; static final String HIDE_VALIDATORS = "--hide-validators"; + private Severity severityOverride; private Severity severity; private List showValidators = Collections.emptyList(); private List hideValidators = Collections.emptyList(); @@ -107,13 +108,21 @@ void severity(Severity severity) { this.severity = severity; } + /** + * Set a severity override + * + * @param severity Severity to set. + */ + void severityOverride(Severity severity) { + this.severityOverride = severity; + } + /** * Set and get the severity level, taking into account standard options that affect the default. * * @param standardOptions Standard options to query if no severity is explicitly set. - * @return Returns the resolved severity option. */ - Severity detectAndGetSeverity(StandardOptions standardOptions) { + void detectAndSetSeverity(StandardOptions standardOptions) { if (severity == null) { if (standardOptions.quiet()) { severity = Severity.DANGER; @@ -121,7 +130,6 @@ Severity detectAndGetSeverity(StandardOptions standardOptions) { severity = Severity.WARNING; } } - return severity; } /** @@ -160,6 +168,18 @@ void hideValidators(List validators) { this.hideValidators = validators; } + /** + * Returns the current severity. + * + * @return The validation severity + */ + Severity getSeverity() { + if (severityOverride != null) { + return severityOverride; + } + return severity; + } + /** * Check if the given validation event matches the show/hide settings. * @@ -169,7 +189,8 @@ void hideValidators(List validators) { * @return Return true if the event can be seen. */ boolean isVisible(ValidationEvent event) { - if (event.getSeverity().ordinal() < severity.ordinal()) { + + if (event.getSeverity().ordinal() < getSeverity().ordinal()) { return false; } diff --git a/smithy-cli/src/test/java/software/amazon/smithy/cli/commands/DiffCommandTest.java b/smithy-cli/src/test/java/software/amazon/smithy/cli/commands/DiffCommandTest.java index 76c2bb5cd62..3823228cb76 100644 --- a/smithy-cli/src/test/java/software/amazon/smithy/cli/commands/DiffCommandTest.java +++ b/smithy-cli/src/test/java/software/amazon/smithy/cli/commands/DiffCommandTest.java @@ -38,4 +38,33 @@ public void canOutputCsv() throws Exception { assertThat(lines[0], containsString("severity,id,shape,file,line,column,message,hint,suppressionReason")); assertThat(lines[1], containsString("\"ERROR\",\"ChangedShapeType\",\"smithy.example#Hello\"")); } + + @Test + public void showsWarningEventsByDefault() throws Exception { + Path oldModel = Paths.get(getClass().getResource("diff/old2.smithy").toURI()); + Path newModel = Paths.get(getClass().getResource("diff/new2.smithy").toURI()); + CliUtils.Result result = CliUtils.runSmithy("diff", + "--old", oldModel.toString(), + "--new", newModel.toString()); + + assertThat(result.code(), is(0)); + + String[] lines = result.stdout().split("(\\r\\n|\\r|\\n)"); + assertThat(lines[1], containsString("WARNING")); + assertThat(lines[1], containsString("TraitBreakingChange.Add.smithy.api#pattern")); + } + + @Test + public void doesNotShowWarningEventsWhenSeverityIsSetToDanger() throws Exception { + Path oldModel = Paths.get(getClass().getResource("diff/old2.smithy").toURI()); + Path newModel = Paths.get(getClass().getResource("diff/new2.smithy").toURI()); + CliUtils.Result result = CliUtils.runSmithy("diff", + // warning events won't be shown in the output + "--severity", "DANGER", + "--old", oldModel.toString(), + "--new", newModel.toString()); + + assertThat(result.code(), is(0)); + assertThat(result.stdout(), is("")); + } } diff --git a/smithy-cli/src/test/resources/software/amazon/smithy/cli/commands/diff/new2.smithy b/smithy-cli/src/test/resources/software/amazon/smithy/cli/commands/diff/new2.smithy new file mode 100644 index 00000000000..0708f41668c --- /dev/null +++ b/smithy-cli/src/test/resources/software/amazon/smithy/cli/commands/diff/new2.smithy @@ -0,0 +1,14 @@ +$version: "2.0" + +namespace smithy.example + + +structure StructureOne { + + @pattern("^.*$") + stringMember: String + + @clientOptional + @required + intMember: Integer +} diff --git a/smithy-cli/src/test/resources/software/amazon/smithy/cli/commands/diff/old2.smithy b/smithy-cli/src/test/resources/software/amazon/smithy/cli/commands/diff/old2.smithy new file mode 100644 index 00000000000..4a93ce012b5 --- /dev/null +++ b/smithy-cli/src/test/resources/software/amazon/smithy/cli/commands/diff/old2.smithy @@ -0,0 +1,10 @@ +$version: "2.0" + +namespace smithy.example + +structure StructureOne { + + stringMember: String + + intMember: Integer +} From 71b8988e78e6886fb582de79530b8af998362341 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Manuel=20Sugawara=20=28=E2=88=A9=EF=BD=80-=C2=B4=29?= =?UTF-8?q?=E2=8A=83=E2=94=81=E7=82=8E=E7=82=8E=E7=82=8E=E7=82=8E=E7=82=8E?= Date: Wed, 20 Aug 2025 11:40:41 -0700 Subject: [PATCH 2/2] Make spotless happy --- .../smithy/cli/commands/DiffCommandTest.java | 15 ++++++++++----- 1 file changed, 10 insertions(+), 5 deletions(-) diff --git a/smithy-cli/src/test/java/software/amazon/smithy/cli/commands/DiffCommandTest.java b/smithy-cli/src/test/java/software/amazon/smithy/cli/commands/DiffCommandTest.java index 3823228cb76..435e57060bc 100644 --- a/smithy-cli/src/test/java/software/amazon/smithy/cli/commands/DiffCommandTest.java +++ b/smithy-cli/src/test/java/software/amazon/smithy/cli/commands/DiffCommandTest.java @@ -44,8 +44,10 @@ public void showsWarningEventsByDefault() throws Exception { Path oldModel = Paths.get(getClass().getResource("diff/old2.smithy").toURI()); Path newModel = Paths.get(getClass().getResource("diff/new2.smithy").toURI()); CliUtils.Result result = CliUtils.runSmithy("diff", - "--old", oldModel.toString(), - "--new", newModel.toString()); + "--old", + oldModel.toString(), + "--new", + newModel.toString()); assertThat(result.code(), is(0)); @@ -60,9 +62,12 @@ public void doesNotShowWarningEventsWhenSeverityIsSetToDanger() throws Exception Path newModel = Paths.get(getClass().getResource("diff/new2.smithy").toURI()); CliUtils.Result result = CliUtils.runSmithy("diff", // warning events won't be shown in the output - "--severity", "DANGER", - "--old", oldModel.toString(), - "--new", newModel.toString()); + "--severity", + "DANGER", + "--old", + oldModel.toString(), + "--new", + newModel.toString()); assertThat(result.code(), is(0)); assertThat(result.stdout(), is(""));