Added test property for FAIL_ON_UNKNOWN_PROPERTIES configuration

[ajturnerora]

What's changed?

I have changed the new Properties() used in test RecipeSpec to be a class field. This allows customization of logic deeper in rewrite-core. In this case I have changed the JsonMapper in YamlResourceLoader to allow DeserializationFeature.FAIL_ON_UNKNOWN_PROPERTIES to be customized. The default is that DeserializationFeature.FAIL_ON_UNKNOWN_PROPERTIES is disabled as that was the existing behavior.

Users can now make their tests fail if there are unknown properties present in the recipe using
spec.failOnUnknownProperties(true)

I also had to update the RECIPE_CACHE key as the properties set can change the recipe outcome - although typically the new part of the key will be {} as no existing code could be configuring any properties.

What's your motivation?

This allows bugs for incorrectly specified recipe options to be more easily caught. E.g., the in below example both are valid recipes and will run but the second one is the true intent and it can be difficult to catch during code reviews:

Incorrect:

  - org.openrewrite.maven.RemoveRedundantDependencyVersions:
      groupId: org.apache.commons

Correct:

  - org.openrewrite.maven.RemoveRedundantDependencyVersions:
      groupPattern: org.apache.commons

Anything in particular you'd like reviewers to focus on?

If there is a better/alternate approach - although it would seem that using the Properties object is the correct way to configure object.

Anyone you would like to review specifically?

• As discussed here: Allow FAIL_ON_UNKNOWN_PROPERTIES to be enabed/disabled via property #4375
  @timtebeek

Any additional context

I made this change as we hit the exact issue describe above and it wasn't caught during PR review. If we had the option to fail tests when unnecessary arguments are specified we would likely enable it for every test case.

Checklist

• I've added unit tests to cover both positive and negative cases
• I've read and applied the recipe conventions and best practices
• I've used the IntelliJ IDEA auto-formatter on affected files

[timtebeek]

Nice and measured approach here! I'd maybe even go as far as enabling this by default when running tests in rewrite-test/src/main/java/org/openrewrite/test/RecipeSpec.java , to flag issues early through or testing framework. We have similar checks that are more strict when running tests only, such as our type validation checks. Would be interesting to see how widespread that is in downstream modules. I'd hope not of course, but certainly interesting.

[timtebeek]

I'm not quite sure about the reuse of Properties properties for configuration; we at present use that to optionally replace properties in Yaml recipe files with looked up values.

rewrite/rewrite-core/src/main/java/org/openrewrite/config/YamlResourceLoader.java

Lines 171 to 172 in 8350101

	this.yamlSource = propertyPlaceholderHelper.replacePlaceholders(
	new String(buffer.toByteArray(), StandardCharsets.UTF_8), properties);

I feel the dual use here might cause confusion in the future, but I'm also not yet sure about a better alternative.

The prevailing style in YamlResourceLoader seems to be new overloaded constructors. Perhaps another one that takes a Consumer<JsonMapper> could offer the same functionality in a different way?

[timtebeek]

Or we juse remove the line that says disable(DeserializationFeature.FAIL_ON_UNKNOWN_PROPERTIES) and see what catches fire. 🤷🏻 Looking back that seems to always have been there; not sure if that was a conscious choice or not; it seems easy to lead to confusion.

[timtebeek]

One of the tests that fails when I try that quickly was added here

• RewriteTest / YamlResourceLoader Enhancements #3517

[timtebeek]

Shannon rightfully pointed out that we might want to only and always enable FAIL_ON_UNKNOWN_PROPERTIES in tests, but keep that disabled when loading and running recipes in practice. Otherwise we get some unfortunate error handling we have to do in odd places. Just checking this in tests strikes the right balance we think, and ought to incentivize tests even more, similar to the type checks we do there.

[shanman190]

Tim and I chatted a little bit and we're going to take what's here as a half step. This'll specifically cover directly loaded YAML, but misses YAML loaded from disk via recipesFromResources. That'll get fixed in a followup PR.

[ajturnerora]

Thanks for getting to this so quickly. I feel bad that I suggest and enhancement and you did all the work 😂

Were you thinking of taking a similar approach for the other loading path? (Consumer<ObjectMapper> which I think is a good solution btw.). The only code paths I see with new YamlResourceLoader are Environment.scanX -> ClasspathScanningLoader -> YamlResourceLoader which is a short call chain to modify. I could attempt the follow up PR if you're okay with the approach of overloading the Environment.Builder constructor with

public Builder(Properties properties, Consumer<ObjectMapper>)

There aren't any other ResourceLoaders other than ClasspathScanningLoader and YamlResourceLoader so that should cover all use cases.

[timtebeek]

I'd briefly explored that option, but didn't arrive at a solution I'd liked just yet. And since this is mostly/only used in tests for now I'd abandoned it for now. You're welcome to try your hand at a non-intrusive change if you can find one. 😅