[#384] Do not modify the project model in effective-pom - #414
Conversation
|
Fixes #384
I clone the model first, so only the copy that gets written is sorted. The test checks that the project's properties are still the same object, with the same contents, after the goal runs. It fails without the clone. |
|
Also, I have a question the template says it's not needed for changes of ~20 lines, but this PR adds a ~120-line test class. Should I file an ICLA before this can be merged? |
5bf7f4b to
27e1cfa
Compare
| */ | ||
| private void writeEffectivePom(MavenProject project, XMLWriter writer) throws MojoExecutionException { | ||
| Model pom = project.getModel(); | ||
| Model pom = project.getModel().clone(); |
There was a problem hiding this comment.
how sure are we that clone works here?
There was a problem hiding this comment.
Hey @elharo, sorry for late reply.
So I think .clone() works here because in the original version the pom in cleanModel(pom) has a reference to the original Model, so cleaning/sorting the properties there overwrites the original ones. That's the issue in #384. Since you mentioned the goal should be read only, I pass a clone() of the Model to cleanModel() instead, so only the copy gets changed.
I am still new to this codebase, so if there is a better way I am happy to change it.
There was a problem hiding this comment.
My question is more does this return a Cloneable object? What's the contract on Model?
| @MojoParameter(name = "output", value = "${outputPath}") | ||
| void testExecuteDoesNotModifyProjectModel(EffectivePomMojo mojo) throws Exception { | ||
| // snapshot of the contents before the mojo runs, to detect in-place modification | ||
| expectedProperties = new Properties(); |
There was a problem hiding this comment.
maybe this should be a local variable?
| private MavenSession mavenSession; | ||
|
|
||
| @Mock | ||
| private Log log; |
There was a problem hiding this comment.
can probably use a real Log
There was a problem hiding this comment.
I removed the log entirely in new commit because it's not used.
|
|
||
| @BeforeEach | ||
| void setup() throws IOException { | ||
| originalProperties = new Properties(); |
There was a problem hiding this comment.
the field can be initialized directly
| originalProperties.setProperty("b.property", "b-value"); | ||
| originalProperties.setProperty("a.property", "a-value"); | ||
|
|
||
| model = new Model(); |
There was a problem hiding this comment.
the field can be initialized directly
| */ | ||
| private void writeEffectivePom(MavenProject project, XMLWriter writer) throws MojoExecutionException { | ||
| Model pom = project.getModel(); | ||
| Model pom = project.getModel().clone(); |
There was a problem hiding this comment.
My question is more does this return a Cloneable object? What's the contract on Model?
| /** | ||
| * The effective-pom goal only displays the model, so it must not modify the project it reads from. | ||
| * | ||
| * @throws Exception in case of errors. |
2 participants
Following this checklist to help us incorporate your
contribution quickly and easily:
Note that commits might be squashed by a maintainer on merge.
This may not always be possible but is a best-practice.
mvn verifyto make sure basic checks pass.A more thorough check will be performed on your pull request automatically.
mvn -Prun-its verify).If your pull request is about ~20 lines of code you don't need to sign an
Individual Contributor License Agreement if you are unsure
please ask on the developers list.
To make clear that you license your contribution under
the Apache License Version 2.0, January 2004
you have to acknowledge this by using the following check-box.