Closed josemariavillar closed 2 years ago
Hi @josemariavillar, thanks for reporting your issue!
I removed the redundant maybeUpdateModel
, but the update will still occur here.
This is an interesting report because the update comes with trade-offs. :)
In 7.17, the maven model wasn't updated after the parent pom changed, which allowed invalid dependencies to exist.
However, the lack of an update also meant that other recipes like AddDependency
weren't able to detect the new versions and could add unnecessary version tags.
In 7.18 the pom is re-resolved after changes that affect the dependencies, like in ChangeParentPom
.
Updating the model allows other recipes to detect the current state of the pom.
@josemariavillar, I think dependencies that would be invalid after a change to the parent pom would need to be removed first.
@tkvangorder what are your thoughts on ChangeParentPom
and Jose's issue with RemoveDependency
?
Good morning @traceyyoshima and @tkvangorder,
I understand that updating the POM that exists in context each time a maven recipe is applied is useful to solve the problems that have arisen when including new dependencies, for example as you comment, it could add unnecessary version tags.
What I do not see useful and can create more problems than benefits, is that continuously, every time a recipe is applied, the POM is validated; I consider that it would be more practical if this validation is done once the execution of the whole set of maven recipes is finished, since I understand that the objective is that the resulting POM is valid.
@josemariavillar Sorry for the delay in response.
Generally, we are wanting a recipe to be self-contained and follow the pattern of "do no harm". So, when a recipe completes, it should leave a source file in a valid state. We also want to provide these building-block recipes to do common things (like removing dependencies and changing parent versions). If a recipe is going to make a change that impacts the semantic model, it should update that model because the next recipe may rely on those model changes to make further changes.
We also strive to make sure that recipes can be executed in any order to achieve the same result. I say "strive" because there are situations where two operations require a specific ordering. There are really two approaches to achieving a forced order:
For your specific example, if you are looking for a quick way to work around this issue, I would simply reorder your declarative recipes such that the dependency removals are before the "change parent".
If you are looking to publish a recipe within your company that is going to be used by many teams, I would suggest the use of an imperative recipe where you can explicitly order the operations. (and then update the semantic model once).
Finally, if you are having to make your own recipe to migrate to a new version of spring-boot, we would love to capture any gaps/issues you are running into and make sure we update our rewrite-spring
project to make this easier for you and anyone else attempting the same types of migrations.
If you need some help with how to achieve the recommendations, I can provide some examples. Thanks for the feedback!
Good morning,
I am having problems running the group of recipes that I had defined when testing with version 7.20.0-SNAPSHOT. In my case the first thing I do is to make a change of parent in the POM, and then the rest of recipes leaving correct resulting POM taking into account the new parent.
With this version, that in most of the maven recipes is included maybeUpdateModel method, which in addition to update the POM of the context, validates it.
https://github.com/openrewrite/rewrite/blob/a160b46c7e79294ad9703611f45c9d56767037df/rewrite-maven/src/main/java/org/openrewrite/maven/ChangeParentPom.java#L172
Being an individual validation for each recipe, it causes that if at that moment the POM is not valid, an exception is generated, failing the execution of that recipe.
https://github.com/openrewrite/rewrite/blob/a160b46c7e79294ad9703611f45c9d56767037df/rewrite-maven/src/main/java/org/openrewrite/maven/tree/ResolvedPom.java#L486-L488
I attach a test case.
Child project: https://github.com/josemariavillar/test_project/tree/error_update_model Parent project: https://github.com/josemariavillar/test-project-parent/tree/error_update_model
As you can see, in the following example, I run the following group of recipes where the first thing I do is to make a parent change and then remove those dependencies that are no longer needed and can cause problems:
The result obtained is:
As you can see, in this example, the ChangeParentPom recipe is not executed because an exception occurs when the version of the io.springfox:springfox-swagger2 dependency is not found, which will be removed later.
With version 7.17.1 it works without problems.