Conversation
…re() Recipes with optional parameters and a custom validate() requiring at least one to be present fail assertRecipesConfigure() because they are loaded with no arguments, leaving all optional params null. Declarative recipes (from YAML) should still be validated since their options are explicitly configured. Imperative recipes loaded with no user-provided arguments are inherently unconfigured and validation failures are expected. Add a recipeValidation flag to RecipeSpec (following the existing serializationValidation pattern) and gate validateAll() on it. In assertRecipesConfigure(), disable validation for non-DeclarativeRecipe instances. Fixes #6840
Three tests demonstrate the intention: - rejectUnconfiguredRecipeWithOptionalOrValidation: recipe with two optional params and validate() requiring at least one fails when constructed with no args (both null) and default validation - acceptUnconfiguredRecipeWithOptionalOrValidationWhenSkipped: same recipe succeeds with validateRecipe(false), as assertRecipesConfigure now does for imperative recipes - rejectConfiguredRecipeWithOptionalOrValidationStillValidated: declarative recipes still get validated (broken YAML still caught)
rewrite-test/src/main/java/org/openrewrite/test/RewriteTest.java
Outdated
Show resolved
Hide resolved
Co-authored-by: Tim te Beek <tim@moderne.io>
rewrite-test/src/main/java/org/openrewrite/test/RewriteTest.java
Outdated
Show resolved
Hide resolved
rewrite-test/src/main/java/org/openrewrite/test/RewriteTest.java
Outdated
Show resolved
Hide resolved
rewrite-test/src/main/java/org/openrewrite/test/RewriteTest.java
Outdated
Show resolved
Hide resolved
The condition for skipping validation was inverted — it ran validation for recipes WITH optional params instead of skipping them. Fixed to allMatch(isRequired) so validation is only performed for declarative recipes or recipes with no optional parameters. Updated the test to verify that an unconfigured recipe with optional params and custom validate() now succeeds instead of failing.
| // 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)) { |
There was a problem hiding this comment.
Shouldn't this be
| || recipe.getDescriptor().getOptions().stream().allMatch(OptionDescriptor::isRequired)) { | |
| || recipe.getDescriptor().getOptions().stream().noneMatch(OptionDescriptor::isRequired)) { |
There was a problem hiding this comment.
My thinking here is we can only fall to validate not provided but required options.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
isOptional or Predicat::not would make it a little easier to read
There was a problem hiding this comment.
Oh and the main problem here are custom validation methods, like or checking for two optional parameter.
There was a problem hiding this comment.
So the problem is the either/or nature of optional arguments that are both not filled here, do I understand that correctly?
There was a problem hiding this comment.
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
github-project-automation
bot
moved this from In Progress
to Ready to Review
in OpenRewrite
…ng-skip-validation-for-imperative
|
The behavior change affects a few tests. Here are the usages of asserts for exceptions in our codebases.
|
…ng-skip-validation-for-imperative
|
Thanks @timtebeek ❤️ |
MBoegers
deleted the
mboegers/recipe-loading-skip-validation-for-imperative
branch
2 participants
Summary
assertRecipesConfigure()now skipsvalidateAll()for imperative recipes loaded with no user-provided argumentsDeclarative recipes (from YAML) still get full validation to catch misconfigured options
Adds
recipeValidationflag toRecipeSpecfollowing the existingserializationValidationpatternFixes RecipeLoadingTest fails for recipes with optional parameters and custom validate() #6840
Test plan
./gradlew :rewrite-test:testpassesRecipeLoadingTestin a recipe repo with optional-param + validate() recipes no longer fails