Thanks a lot @mryan43 ! Tagging Knut for review as he proposed something similar for constructors just today.

Hi @mryan43, looks neat! I will have a detailed look tomorrow in the morning 📓

PR is ready for another round of review.

PS: Huge thank you for all the work everyone has put in this PR so far !

Merged origin/main into this branch, please approve again for CI. This PR is still awaiting a re-review after comments were addressed.

Hi @mryan43 as you see from the number of participants, updating MavenResolutionResults after changing the Maven Pom content is a pain point for OpenRewrite, and we are very pleased by your contribution.
You already noticed the public activity dropped, please let me explain. We noticed that your solution utilizes the ExecutionContext to store Pom instances. The ExecutionContext is designed to store meta information and configurations. Accumulators of SearchRecipes, Maven Configurations and likewise are data to store in the ExecutionContext. Poms in contrast, are LST elements and the main object of work for OpenRewrite, like a person for a CRM system. With this assumption, build in storing Poms in the ExecutionContext can cause major problems in downstream products like the Moderne SaaS or Moderne CLI.
Because of this fear, we came to the conclusion that we shouldn't merge this PR with the current implementation but seek for a solution that does utilize the ExecutionContext as designed to transport meta information.

I apologize for the delay and late intervention. I hope you can use your incredible skill and motivation to find a better solution with us.

Thanks for the answer and the call we had recently.

After investigating a bit more, the ExecutionContext seems to already contain instances of the Pom class before those changes :

Could you share a bit more details about why the changes from this PR are dangerous for downstream products but those are ok ? That would help me find a better solution that fits everyone's needs

I would also like to challenge the idea that the Pom class represents a LST element.
I will argue that it is already meta-data:

• MavenVisitor extends TreeVisitor<XML,P>, it does not have a visitPom or visitDependency method for example, only methods for visiting xml tags.
• MavenVisitors being an XML visitor, they do not mutate the fields of the Pom class when visiting. That is most likely why the UpdateMavenModel visitor was created in the first place and needs to run after modifications to the XML, to update the metadata (therefore MavenResolutionResult is also meta-data)
• Poms can be returned by the MavenPomDownloader class. This class can return poms downloaded from remote artifact repositories, how are those downloaded poms LST elements in OpenRewrite ? Could they be modified by visitors for example ? obviously not...

For all those reasons... I think only XML documents and tags can be considered LST elements in the current implementation.

You are right, Poms are not actually LST elements for the reasons you mention. There is also no technical reason why LST elements can't be stored in the ExecutionContext. Sometimes that can be the best way to achieve something. The reason we discourage storing LSTs in the ExecutionContext is that these can consume a lot of memory, especially if they have type attribution attached to them as in the case of Java. In the case of Maven the MavenResolutionResult can also consume quite a lot of memory due to all the transitive dependencies. I am not sure how much of that is attributed to the Pom objects, but I think it should probably be OK and I don't really see any other option anyway.

Also note that while using the InMemoryLargeSourceSet for recipe execution this isn't a problem, because all LST are already loaded into memory anyway and thus adding a reference to the ExecutionContext doesn't occupy any additional memory. With the Moderne SaaS and CLI the situation is different as they support processing huge repos by not loading all LSTs at once into memory. By carrying LSTs forward in the ExecutionContext the memory of those LSTs can never be released.

Thanks a lot for clarifying those concerns !

I was wondering if there exists any mechanism other than the ExecutionContext to propagate the update between parent and child pom that could be better suited ? I didn't find any in my research but this is my first contribution so I might have missed something

The only alternative I can come up with is if we would register something like a UnaryOperator<Pom> (probably a dedicated functional interface would be the better alternative) in the ExecutionContext as a function to transform the parent Pom. At least theoretically this seems like it should also be doable. This function could then be applied to transform the parent POM both for the parent project and for child projects. I am just putting this out there for discussion.

I'm not convinced that we can find a better solution without heavily refactoring the whole maven module...

The question remains about what should happen to this PR.
I'm a bit sad that the fix seems to be blocked only because the architecture of the downstream closed source project.

@mryan43 What about my proposal with a transformer function? Would that end up capturing a lot of objects from the LST model in the lambda or is it a lot of work to refactor? I haven't looked at the changes very closely and it is a bit hard to tell from a distance.

I can add an interface with an implementation that contains the full Pom object and replaces it. Then downstream closed source product maintainers can replace it by whatever they want. Would that work for you ?

This would be similar to how the ExecutionContext vs InMemoryExecutionContext works

This PR is stale because it has been open for 90 days with no activity. Remove stale label or comment or this will be closed in 7 days.

How can we progress on this? Ultimately, when a recipe is crawling over a child pom, we need all the parent metadata to be up to date. I even tried multi-cycle as a quick hack to make things work, but it didn't. I didn't really dig into why, but I seem to recall some caching.

Is the function approach mentioned here being discussed?