Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -19,11 +19,11 @@
import org.junit.jupiter.api.Test;
import org.openrewrite.DocumentExample;
import org.openrewrite.Issue;
import org.openrewrite.Validated;
import org.openrewrite.test.RewriteTest;
import org.openrewrite.test.SourceSpec;

import static org.assertj.core.api.Assertions.assertThat;
import static org.assertj.core.api.Assertions.assertThatExceptionOfType;
import static org.openrewrite.java.Assertions.mavenProject;
import static org.openrewrite.maven.Assertions.pomXml;

Expand Down Expand Up @@ -765,33 +765,17 @@ void changeProfileManagedDependencyGroupIdAndArtifactId() {
@Issue("https://github.com/openrewrite/rewrite-java-dependencies/issues/55")
@Test
void requireNewGroupIdOrNewArtifactId() {
assertThatExceptionOfType(AssertionError.class)
.isThrownBy(() -> rewriteRun(
spec -> spec.recipe(new ChangeDependencyGroupIdAndArtifactId(
"javax.activation",
"javax.activation-api",
null,
null,
null,
null
))
)).withMessageContaining("newGroupId OR newArtifactId must be different from before");
ChangeDependencyGroupIdAndArtifactId recipe = new ChangeDependencyGroupIdAndArtifactId("javax.activation", "javax.activation-api", null, null, null, null);;
assertThat(recipe.validate().failures()).extracting(Validated.Invalid::getMessage)
.contains("newGroupId OR newArtifactId must be different from before");
}

@Issue("https://github.com/openrewrite/rewrite-java-dependencies/issues/55")
@Test
void requireNewGroupIdOrNewArtifactIdToBeDifferentFromBefore() {
assertThatExceptionOfType(AssertionError.class)
.isThrownBy(() -> rewriteRun(
spec -> spec.recipe(new ChangeDependencyGroupIdAndArtifactId(
"javax.activation",
"javax.activation-api",
"javax.activation",
null,
null,
null
))
)).withMessageContaining("newGroupId OR newArtifactId must be different from before");
ChangeDependencyGroupIdAndArtifactId recipe = new ChangeDependencyGroupIdAndArtifactId("javax.activation", "javax.activation-api", "javax.activation", null, null, null);
assertThat(recipe.validate().failures()).extracting(Validated.Invalid::getMessage)
.contains("newGroupId OR newArtifactId must be different from before");
}

@Test
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -18,10 +18,10 @@
import org.junit.jupiter.api.Test;
import org.openrewrite.DocumentExample;
import org.openrewrite.Issue;
import org.openrewrite.Validated;
import org.openrewrite.test.RewriteTest;

import static org.assertj.core.api.Assertions.assertThat;
import static org.assertj.core.api.Assertions.assertThatExceptionOfType;
import static org.openrewrite.maven.Assertions.pomXml;

class ChangeManagedDependencyGroupIdAndArtifactIdTest implements RewriteTest {
Expand Down Expand Up @@ -79,16 +79,9 @@ void changeManagedDependencyGroupIdAndArtifactId() {
@Issue("https://github.com/openrewrite/rewrite-java-dependencies/issues/55")
@Test
void requireNewGroupIdOrNewArtifactIdToBeDifferentFromBefore() {
assertThatExceptionOfType(AssertionError.class)
.isThrownBy(() -> rewriteRun(
spec -> spec.recipe(new ChangeManagedDependencyGroupIdAndArtifactId(
"javax.activation",
"javax.activation-api",
"javax.activation",
"javax.activation-api",
null
))
)).withMessageContaining("newGroupId OR newArtifactId must be different from before");
ChangeManagedDependencyGroupIdAndArtifactId recipe = new ChangeManagedDependencyGroupIdAndArtifactId("javax.activation", "javax.activation-api", "javax.activation", "javax.activation-api", null);
assertThat(recipe.validate().failures()).extracting(Validated.Invalid::getMessage)
.contains("newGroupId OR newArtifactId must be different from before");
}

@Test
Expand Down
18 changes: 13 additions & 5 deletions rewrite-test/src/main/java/org/openrewrite/test/RewriteTest.java
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@
import org.jspecify.annotations.Nullable;
import org.openrewrite.*;
import org.openrewrite.config.CompositeRecipe;
import org.openrewrite.config.DeclarativeRecipe;
import org.openrewrite.config.Environment;
import org.openrewrite.config.OptionDescriptor;
import org.openrewrite.internal.*;
Expand Down Expand Up @@ -226,11 +227,18 @@ default void rewriteRun(Consumer<RecipeSpec> spec, SourceSpec<?>... sourceSpecs)
for (SourceSpec<?> s : sourceSpecs) {
s.customizeExecutionContext.accept(ctx);
}
List<Validated<Object>> validations = new ArrayList<>();
recipe.validateAll(ctx, validations);
assertThat(validations.stream().filter(Validated::isInvalid))
.as("Recipe validation must have no failures")
.isEmpty();

// Imperative recipes are loaded with no user-provided arguments, so all optional parameters are null.
// Skip recipe validation for these since custom validate() methods (e.g. requiring at least one of several
// optional parameters) would fail on an unconfigured instance.
if (recipe instanceof DeclarativeRecipe
|| recipe.getDescriptor().getOptions().stream().allMatch(OptionDescriptor::isRequired)) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't this be

Suggested change
|| recipe.getDescriptor().getOptions().stream().allMatch(OptionDescriptor::isRequired)) {
|| recipe.getDescriptor().getOptions().stream().noneMatch(OptionDescriptor::isRequired)) {

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My thinking here is we can only fall to validate not provided but required options.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, required options are fine, the validation fills them. Problematic is a recipe that contains at least one non required.
WithnonMatch(isRequired) we would allow recipes with only optional options.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

isOptional or Predicat::not would make it a little easier to read

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh and the main problem here are custom validation methods, like or checking for two optional parameter.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So the problem is the either/or nature of optional arguments that are both not filled here, do I understand that correctly?

Copy link
Contributor Author

@MBoegers MBoegers Mar 3, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, that's the problem. You can find these recipes even in the core, f.ex. ChangeDependency.
Maybe the Validated#or is the real problem, other recipes that use a similar structure work with if, f.e. o.o.docker.ChangeFrom
But these would also fail for RewriteTest#assertRecipesConfigure

List<Validated<Object>> validations = new ArrayList<>();
recipe.validateAll(ctx, validations);
assertThat(validations.stream().filter(Validated::isInvalid))
.as("Recipe validation must have no failures")
.isEmpty();
}

Map<Parser.Builder, List<SourceSpec<?>>> sourceSpecsByParser = new HashMap<>();
List<Parser.Builder> methodSpecParsers = testMethodSpec.parsers;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -21,9 +21,11 @@
import lombok.Value;
import org.jspecify.annotations.NullMarked;
import org.jspecify.annotations.Nullable;
import org.junit.jupiter.api.Nested;
import org.junit.jupiter.api.Test;
import org.openrewrite.*;
import org.openrewrite.test.RewriteTest;
import org.openrewrite.test.SourceSpecs;
import org.openrewrite.test.TypeValidation;
import org.openrewrite.text.PlainText;
import org.openrewrite.text.PlainTextVisitor;
Expand All @@ -35,6 +37,7 @@

import static java.util.Collections.emptyList;
import static org.assertj.core.api.Assertions.assertThat;
import static org.assertj.core.api.Assertions.assertThatThrownBy;
import static org.junit.jupiter.api.Assertions.assertThrows;
import static org.openrewrite.test.SourceSpecs.text;

Expand Down Expand Up @@ -121,7 +124,7 @@ void rejectRecipeValidationFailure() {
description: Deliberately has a non-existent recipe in its recipe list to trigger a validation failure.
recipeList:
- org.openrewrite.DoesNotExist

""", "org.openrewrite.RefersToNonExistentRecipe")
));
}
Expand Down Expand Up @@ -152,6 +155,59 @@ void allowScannerEdit() {
text("foo")
);
}

@Nested
class RecipeValidation {

@Test
void validateRecipeWithNoOptions() {
rewriteRun(
spec -> spec.recipe(new RecipeWithNoOptions()),
new SourceSpecs[0]
);
}

@Test
void validateRecipeWithOnlyRequiredOptionsPositive() {
rewriteRun(
spec -> spec.recipe(new RecipeWithRequiredOptionValidateNoBlank("Test")),
new SourceSpecs[0]
);
}

@Test
void validateRecipeWithOnlyRequiredOptionsNegative() {
assertThatThrownBy(() ->
rewriteRun(
spec -> spec.recipe(new RecipeWithRequiredOptionValidateNoBlank("")),
new SourceSpecs[0]
)
);
}

@Test
void skipValidationForRecipeWithOptionalOptions() {
rewriteRun(
spec -> spec.recipe(new RecipeWithOptionalOrValidation(null, null)),
new SourceSpecs[0]
);
}

@Test
void validateDeclarativeRecipe() {
assertThrows(AssertionError.class, () ->
rewriteRun(
spec -> spec.recipeFromYaml("""
type: specs.openrewrite.org/v1beta/recipe
name: org.openrewrite.test.internal.StillValidated
displayName: Still validated
description: Declarative recipe with a non-existent sub-recipe should still fail validation.
recipeList:
- org.openrewrite.DoesNotExist
""", "org.openrewrite.test.internal.StillValidated")
));
}
}
}

@EqualsAndHashCode(callSuper = false)
Expand Down Expand Up @@ -230,7 +286,7 @@ class CreatesTwoFilesSamePath extends ScanningRecipe<AtomicBoolean> {
String displayName = "Creates two source files with the same path";

String description = "A source file's path must be unique. " +
"This recipe creates two source files with the same path to show that the test framework helps protect against this mistake.";
"This recipe creates two source files with the same path to show that the test framework helps protect against this mistake.";

@Override
public AtomicBoolean getInitialValue(ExecutionContext ctx) {
Expand Down Expand Up @@ -296,10 +352,10 @@ class RecipeWithDescriptionListOfLinks extends Recipe {

@Getter
final String description = """
A fancy description.
For more information, see:
- [link 1](https://example.com/link1)
- [link 2](https://example.com/link2)""";
A fancy description.
For more information, see:
- [link 1](https://example.com/link1)
- [link 2](https://example.com/link2)""";
}

@NullMarked
Expand All @@ -310,10 +366,10 @@ class RecipeWithDescriptionListOfDescribedLinks extends Recipe {

@Getter
final String description = """
A fancy description.
For more information, see:
- First Resource [link 1](https://example.com/link1).
- Second Resource [link 2](https://example.com/link2).""";
A fancy description.
For more information, see:
- First Resource [link 1](https://example.com/link1).
- Second Resource [link 2](https://example.com/link2).""";
}

@NullMarked
Expand Down Expand Up @@ -353,3 +409,57 @@ public Collection<? extends SourceFile> generate(AtomicBoolean acc, ExecutionCon
.build());
}
}

@Value
@EqualsAndHashCode(callSuper = false)
class RecipeWithNoOptions extends Recipe {
String displayName = "Recipe with no options";
String description = "Has no configurable options at all.";
}

@Value
@NullMarked
@EqualsAndHashCode(callSuper = false)
class RecipeWithRequiredOptionValidateNoBlank extends Recipe {
String displayName = "Recipe with required option";
String description = "Has a single required parameter.";

@Option(displayName = "Required param",
description = "A required parameter.")
String requiredParam;

@Override
public Validated<Object> validate() {
return super.validate()
.and(Validated.notBlank("Required param", requiredParam));
}
}

@Value
@NullMarked
@EqualsAndHashCode(callSuper = false)
class RecipeWithOptionalOrValidation extends Recipe {
String displayName = "Recipe with optional OR validation";
String description = "Has two optional parameters where at least one must be set.";

@Option(displayName = "Option A",
description = "First optional parameter.",
example = "valueA",
required = false)
@Nullable
String optionA;

@Option(displayName = "Option B",
description = "Second optional parameter.",
example = "valueB",
required = false)
@Nullable
String optionB;

@Override
public Validated<Object> validate() {
return super.validate().and(
Validated.required("optionA", optionA)
.or(Validated.required("optionB", optionB)));
}
}
60 changes: 29 additions & 31 deletions rewrite-yaml/src/test/java/org/openrewrite/yaml/MergeYamlTest.java
Original file line number Diff line number Diff line change
Expand Up @@ -18,9 +18,10 @@
import org.junit.jupiter.api.Test;
import org.openrewrite.DocumentExample;
import org.openrewrite.Issue;
import org.openrewrite.Validated;
import org.openrewrite.test.RewriteTest;

import static org.junit.jupiter.api.Assertions.assertThrows;
import static org.assertj.core.api.Assertions.assertThat;
import static org.openrewrite.yaml.Assertions.yaml;
import static org.openrewrite.yaml.MergeYaml.InsertMode.After;
import static org.openrewrite.yaml.MergeYaml.InsertMode.Before;
Expand Down Expand Up @@ -3059,22 +3060,21 @@ void preventKeysToBeAppendedToPreviousCommentIfManyLineBreaks() {

@Test
void invalidYaml() {
assertThrows(AssertionError.class, () -> rewriteRun(
spec -> spec
.recipe(new MergeYaml(
"$.some.object",
//language=text
"""
script: |-ParseError
""",
false,
"name",
null,
null,
null,
null
))
));
MergeYaml recipe = new MergeYaml(
"$.some.object",
//language=text
"""
script: |-ParseError
""",
false,
"name",
null,
null,
null,
null
);
assertThat(recipe.validate().failures()).extracting(Validated.Invalid::getMessage)
.contains("Must be valid YAML");
}

@Test
Expand Down Expand Up @@ -3144,20 +3144,18 @@ void createNewKeysFalse() {
@SuppressWarnings("DataFlowIssue")
@Test
void sourceNull() {
assertThrows(AssertionError.class, () ->
rewriteRun(
spec -> spec
.recipe(new MergeYaml(
"$.some.object",
null,
false,
"name",
null,
null,
null,
null
))
));
MergeYaml recipe = new MergeYaml(
"$.some.object",
null,
false,
"name",
null,
null,
null,
null
);
assertThat(recipe.validate().failures()).extracting(Validated.Invalid::getMessage)
.contains("Must be valid YAML");
}

@Test
Expand Down