Avoid NPE in isArtifactRegistered when module artifact is null #519 - #549
Conversation
EarModule.getArtifact() is null until resolveArtifact runs. The stream used null.equals(artifact) and blew up. Treat a missing artifact as not registered.
|
|
||
| private static boolean isArtifactRegistered(Artifact a, List<EarModule> currentList) { | ||
| return currentList.stream().anyMatch(em -> em.getArtifact().equals(a)); | ||
| static boolean isArtifactRegistered(Artifact a, List<EarModule> currentList) { |
|
Renamed the parameter to artifact. |
| return currentList.stream().anyMatch(em -> em.getArtifact().equals(a)); | ||
| static boolean isArtifactRegistered(Artifact artifact, List<EarModule> currentList) { | ||
| return currentList.stream() | ||
| .anyMatch(em -> em.getArtifact() != null && em.getArtifact().equals(artifact)); |
There was a problem hiding this comment.
Check me but I think this is simpler if you just flip the comparison:
em -> artifact.equals(em.getArtifact())
|
Flipped the comparison to artifact.equals(em.getArtifact()). |
| class AbstractEarMojoTest extends AbstractEarTestBase { | ||
|
|
||
| @Test | ||
| void isArtifactRegisteredSkipsUnresolvedModule() { |
There was a problem hiding this comment.
I'm not fond of testing what are effectively private methods made non-private only for a test. Is there any way to test this through the public API? Maybe with an integration test? If there's no observable difference without using private access, there might not actually be a problem.
execute() always resolves user modules first, so a unit test of the helper was not a public-API repro. Keep artifact.equals so a null getArtifact() still does not NPE.
|
execute() always calls resolveArtifact on user modules before this check, so an unresolved module never reaches it. I dropped the unit test and made the helper private again. Left artifact.equals so a null getArtifact() still does not NPE. |
elharo
left a comment
elharo
left a comment
There was a problem hiding this comment.
since it no longer skips, PR description and title should be updated
AzazelSensei
changed the title
|
Updated the title and description to match the equals flip. |
|
@elharo Please assign appropriate label to PR according to the type of change. |
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).EarModule.getArtifact()is documented as null until the module is resolved.isArtifactRegisteredcalledequalson that value, so a null artifact threw NPE.execute()always resolves user modules first, so that path does not reach an unresolved module. I flipped the comparison toartifact.equals(em.getArtifact())so a null module artifact still does not NPE.