[MNG-7375] prevent potential NPE in Metadata.merge(...)

[kwin]

Add unit tests for merge

Following this checklist to help us incorporate your contribution quickly and easily:

• [x] Make sure there is a JIRA issue filed for the change (usually before you start working on it). Trivial changes like typos do not require a JIRA issue. Your pull request should address just this issue, without pulling in other changes.
• [x] Each commit in the pull request should have a meaningful subject line and body.
• [x] Format the pull request title like [MNG-XXX] - Fixes bug in ApproximateQuantiles, where you replace MNG-XXX with the appropriate JIRA issue. Best practice is to use the JIRA issue title in the pull request title and in the first line of the commit message.
• [ ] Write a pull request description that is detailed enough to understand what the pull request does, how, and why.
• [x] Run mvn clean verify to make sure basic checks pass. A more thorough check will be performed on your pull request automatically.
• [ ] You have run the Core IT successfully.

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.

• [x] I hereby declare this contribution to be licenced under the Apache License Version 2.0, January 2004

• [ ] In any other case, please file an Apache Individual Contributor License Agreement.

[michael-o]

This PR hides the fact that the input data is invalid. Obviously, we need a MetadataValidator.

[kwin]

So how should the merge deal with a missing prefix? Throw an ISE or IAE? Where would the validator be hooked in?

[michael-o]

I see two issues:

• Modello generates a broken model therefore you assume the prefix to be optional.
• The complete absence of a MetadataValidator. Would have expected that we would some someting like this:

maven-settings-builder  3.8.5-SNAPSHOT
DefaultSettingsBuilder.java (5 matches)
42: import org.apache.maven.settings.validation.SettingsValidator;  
64: private SettingsValidator settingsValidator;  
71: SettingsValidator settingsValidator )  
90: public DefaultSettingsBuilder setSettingsValidator( SettingsValidator settingsValidator )  
validation
DefaultSettingsValidator.java (2 matches)
44: public class DefaultSettingsValidator  
45: implements SettingsValidator  

for the metadata, but we don't :-(

[michael-o]

@rfscholte

[kwin]

Both issues are correct but are rather follow-ups to MNG-7375. My question is about how to deal with a missing prefix within Metadata.merge (this can happen even if the XSD would say it is mandatory, e.g. by a bug like in this case through staging-maven-plugin). Would a validator really prevent null being passed to Metadata.merge?

[michael-o]

Both issues are correct but are rather follow-ups to MNG-7375. My question is about how to deal with a missing prefix within Metadata.merge (this can happen even if the XSD would say it is mandatory, e.g. by a bug like in this case through staging-maven-plugin). Would a validator really prevent null being passed to Metadata.merge?

The validator would fail the build with an appropriate error message before anything being merged. I.e., source and target are validated upfront before merge happens.

[michael-o]

I don't have access to the NEXUS issue :-(

[kwin]

As I cannot change the security level of https://issues.sonatype.org/browse/NEXUS-30749 (Sonatype change the default to private unfortunately) I also attached the PDF export to https://issues.apache.org/jira/browse/MNG-7375

[kwin]

@michael-o I now added a first draft of a MetadataValidator (inspired by SettingsValidator). Please tell me what you think!

[michael-o]

@kwin Insane! Please open a new JIRA issue for this. I am thinking whether we can introduce this in 3.8.x or must wait at least until 3.9.x. We have a lot of talks about validators with @rfscholte last year. He should be part of this discussion as well.

[kwin]

Metadata is only partially handled inside Maven (core), mostly it is now handled transparently in Maven Resolver, so probably the validator should be part of Maven Resolver and only for some places where Maven directly leverages metadata it should be reused inside Maven directly. WDYT?

Update: As the validation is Maven repository specific it needs to be called from maven-resolver-provider which is in fact part of the Maven main reactor. I added validation calls in all metadata reads from all Maven modules except the legacy "maven-compat".

[michael-o]

Metadata is only partially handled inside Maven (core), mostly it is now handled transparently in Maven Resolver, so probably the validator should be part of Maven Resolver and only for some places where Maven directly leverages metadata it should be reused inside Maven directly. WDYT?

Update: As the validation is Maven repository specific it needs to be called from maven-resolver-provider which is in fact part of the Maven main reactor. I added validation calls in all metadata reads from all Maven modules except the legacy "maven-compat".

@cstamas What is your opinion on the package of this new solution? I don't have one yet.

[cstamas]

@cstamas What is your opinion on the package of this new solution? I don't have one yet.

"handled transparently" is the key here. IMO, any validation should be next where model is, in Maven and not in Resolver, as with any other maven-specific models, Resolver have no clue about them (so resolver have no clue to parse XML into Metadata). Moreover, Resolver has no maven-specific bits, while Metadata is maven-specific bit, so IMHO it should NOT go into Resolver.

OTOH, I'd -1 on "componentizing" maven-repository-metadata, as I'd keep same separation as with "model": -model, model-builder/validator etc. So, IMO maven-repository-metadata getting added components is wrong IMHO.

Will check the rest a bit later.

[kwin]

So, IMO maven-repository-metadata getting added components is wrong IMHO.

Any suggestion where to add the default validation module then, because as you say "any validation should be next where model is"?

[michael-o]

I support @cstamas position.

So, IMO maven-repository-metadata getting added components is wrong IMHO.

Any suggestion where to add the default validation module then, because as you say "any validation should be next where model is"?

Likely like POM model or settings model. Introduce a maven-repository-metadata-builder module. That would make your new code consistent with the rest. Look at the given ones.

[cstamas]

Agreed, while in case of metadata this may seem like overkill, I still like the separation of models and logic around them (aka builder or validator or whatnot).

Ultimately, I'd like to see models moved out of maven, as they anyway barely change. Yes, we expect a bit of turbulence in upcoming maven (plans), but still, the amount of changes to models as compared to maven core etc is more like 0 to million...

[michael-o]

Correct, we pointlessly waste cycles for their generation. This applies to a lot of stuff we do.

[kwin]

Introduce a maven-repository-metadata-builder module

Done in https://github.com/apache/maven/pull/645/commits/d39be11613d20228a4e826302ef57adf19ba1194. This module currently only contains the validator interface and default implementation. Should we also have a real builder there (which would IMO only be a simple wrapper around MetadataXpp3Reader and the validator)?

[michael-o]

Introduce a maven-repository-metadata-builder module

Done in d39be11. This module currently only contains the validator interface and default implementation. Should we also have a real builder there?

Good question, I will come back to this as soon as I have completed my open votes.

[kwin]

@michael-o Any update here?

[michael-o]

No, not yet. Ping me in a week.

[michael-o]

Is this one still relevant regarding the changes done in MPLUGIN?

[kwin]

Yes, as in general metadata should be validated. This is about exposing useful error messages in case the metadata is invalid.

[michael-o]

Alright. Will check later.

[michael-o]

Do you see this for 3.8.x as well?

[kwin]

Just for 4.x for now.

[michael-o]

@cstamas @gnodet @hboutemy Any objections in general?

[gnodet]

@kwin I've rebased the PR to fix all conflicts, I haven't done an in-depth review yet, but I wonder about adding yet another module for basically a single service. I also wonder if we could implement this service natively for the v4 api: so the service interface in org.apache.maven.api.services and the implementation inside the existing maven-repository-metadata module...

[kwin]

Introducing a new module was suggested in https://github.com/apache/maven/pull/645#issuecomment-1004330642, but I am fine with the original approach as well (impl in existing module).