MNG-7529 alternate fix #795
Conversation
Structure the version checks slightly different to make the separation between repository versions and the version range resolution clearer.
|
|
||
| for ( String version : versioning.getVersions() ) | ||
| { | ||
| boolean snapshotVersion = version != null && version.endsWith( SNAPSHOT ); |
There was a problem hiding this comment.
There was a problem hiding this comment.
there is a similar routine, yes. However, it can not be used here because the "versioned snapshot" match is not correct.
A remote repository would report timestamp versioned information in the <snapshotVersion> section of the maven-metadata.xml file.
Using this method actually introduces a new bug if someone accidentally chooses this version pattern as their release version. In 2,252 local maven-metadata files in my current repos, there are exactly 0 versions listed that match the data pattern, however there are 1,044 versions that end with -SNAPSHOT in the versioning section.
This can be easily reproduced by setting up a repository that supports both snapshots and release.
So suggesting to use this method (which adds an additional check) is not correct.
There was a problem hiding this comment.
looking at https://maven.apache.org/ref/3.8.6/maven-repository-metadata/repository-metadata.html#versioning, the versions that are exposed in the <version> field are either release versions or versions ending with -SNAPSHOT as this list are versions available in the remote repository. The remote repository does not expose the separate, timestamped versions but only the -SNAPSHOT version as that is what the user uses in the pom file and what the repository serves. The timestamped versions would be here: https://maven.apache.org/ref/3.8.6/maven-repository-metadata/repository-metadata.html#snapshotversion and that would be used for resolving the actual file on disk for a specific snapshot.
There was a problem hiding this comment.
If I choose to release a foo-1.0-20220829.222835-1, this would be a perfectly legitimate release with a slightly weird but totally legal version. However, by following your suggestion, this released artifact would be considered a snapshot and remove from a release repository. So it would not be considered for a version range. And that is a bug.
There was a problem hiding this comment.
If I choose to release a
foo-1.0-20220829.222835-1, this would be a perfectly legitimate release with a slightly weird but totally legal version. However, by following your suggestion, this released artifact would be considered a snapshot and remove from a release repository. So it would not be considered for a version range. And that is a bug.
Are you sure about this? Am a bit sceptical that Maven itself or some plugin (enforcer, release, versions...) would NOT handle such a version as snapshot. Also, in downstream project (the would depend on release foo-1.0-20220829.222835-1) you think the dependency would NOT be handled as snapshot but as a release (by maven itself or any of the plugins)?
There was a problem hiding this comment.
We are mixing GA and GAV level metadata here, but this is irrelevant (don't want to derail topic). What bothers me more is that we are again assuming that "remote metadata is correct" (same situation as with original code, why this bug was revealed in first place: code assumed snapshot repository will return snapshot versions, that metadata is "correct").... so I consider this reasoning fluke. Either we trust remote metadata or not. You are now fixing something by making Maven "reconsider" remote metadata, but on the other hand now you argue "what you have" in your remote metadata 😄
Having said this, am again leaning toward that this bug (MNG-7529) is more about is "bad configuration", basically your snapshot repository is broken (or in other words, your configuration is broken). For snapshot repository (the original report), all you should do in MRM to create another group that contains snapshot members only, and you'd be done (as MRM merges maven metadata, making them "mixed").
There was a problem hiding this comment.
I think blaming the user for "bad configuration" is not the right thing. If the maven settings configuration is invalid, then maven should fail right away and flag this as "not supported / illegal configuration". As it does not, maven should do the right thing, even though you feel that this is not right (spoiler alert: I did not come up with that setup, I saw setups like this at various organizations).
The assumption in the code ("snapshot repositories only delivers snapshots") is a flawed one, because the repository has no idea that it is considered as a source of "snapshots only". There is no communication from the client to the service asking for "versions that are snapshots / non-snapshots only". Same for "releases only". A repository that has both snapshots and releases (which is a very common setup for corporate repositories / proxy repos etc.) is perfectly fine to report these versions in its version list.
If any maven plugin would consider foo-1.0-20220829.222835-1 a snapshot version, this is a bug. There is no limitation on how a version is structured besides "if it ends with -SNAPSHOT, it is a snapshot version". The maven metadata from a remote repository allows mapping of -SNAPSHOT to a special, timestamp structured version which is delivered in the <snapshotVersions> data set so that the resolver can choose a remote file to download. But nevertheless, the actual artifact version is -SNAPSHOT, because that is what is in the <versions> list in the metadata. So checking the remote snapshot version against a release version is a surefire recipe to introduce another bug.
I believe that the proposed fix is correct.
There was a problem hiding this comment.
We are mixing GA and GAV level metadata here, but this is irrelevant (don't want to derail topic). What bothers me more is that we are again assuming that "remote metadata is correct" (same situation as with original code, why this bug was revealed in first place: code assumed snapshot repository will return snapshot versions, that metadata is "correct").... so I consider this reasoning fluke. Either we trust remote metadata or not. You are now fixing something by making Maven "reconsider" remote metadata, but on the other hand now you argue "what you have" in your remote metadata 😄
Having said this, am again leaning toward that this bug (MNG-7529) is more about is "bad configuration", basically your snapshot repository is broken (or in other words, your configuration is broken). For snapshot repository (the original report), all you should do in MRM to create another group that contains snapshot members only, and you'd be done (as MRM merges maven metadata, making them "mixed").
The version range resolver explicitly asks for metadata for releases and snapshots: https://github.com/apache/maven/blob/master/maven-resolver-provider/src/main/java/org/apache/maven/repository/internal/DefaultVersionRangeResolver.java#L150-L151
The repository now answers with all versions that it knows for releases and snapshots. But only the client knows what types of artifacts it has enabled for a specific repository (this is client side information). So the client, after it asked the repo for "give me any metadata that you know of" needs to filter the result based on what it has enabled a repository for. You may consider that reasoning fluke, but the code does exactly what you asked it to do. You are not reconsidering the metadata, but applying filters that only exist locally because they are configured in maven-settings after you asked the repo to "give me everything you have".
There was a problem hiding this comment.
If any maven plugin would consider
foo-1.0-20220829.222835-1a snapshot version, this is a bug. There is no limitation on how a version is structured besides "if it ends with-SNAPSHOT, it is a snapshot version". The maven metadata from a remote repository allows mapping of-SNAPSHOTto a special, timestamp structured version which is delivered in the<snapshotVersions>data set so that the resolver can choose a remote file to download. But nevertheless, the actual artifact version is-SNAPSHOT, because that is what is in the<versions>list in the metadata. So checking the remote snapshot version against a release version is a surefire recipe to introduce another bug.
Having validated that this assumption is indeed baked into maven-core, I still consider this a bug. I also feel that this should be a much wider discussion, so I brought it to the dev-list.
I will make the change to use the ArtifactUtils method, because that IMHO will allow us to fix that bug in fewer places. Introducing another private method will actually make it harder to fix.
I still believe that the timestamp check is wrong and should not be done. But given that it is all over the place (including the dependency resolver), we should have a discussion first on how to deal with across the whole maven ecosystem.
|
Other than snapshot detection, looks good. |
|
Damn, you were faster than me. |
@cstamas This is the discussed alternate fix for MNG-7529. I structured it differently. The actual meat is in the e70b8c8 fix; I put that on top of a reversal of the previous fix; I do intend to squash and merge though (not have a reversal and a full new fix).
Please let me know if that is more along the shape that you were thinking of.
Following this checklist to help us incorporate your
contribution quickly and easily:
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.
[MNG-XXX] SUMMARY, where you replaceMNG-XXXand
SUMMARYwith the appropriate JIRA issue. Best practice is to use the JIRA issuetitle in the pull request title and in the first line of the commit message.
mvn clean verifyto make sure basic checks pass. A more thorough check willbe performed on your pull request automatically.
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.