You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@maven.apache.org by GitBox <gi...@apache.org> on 2022/08/28 19:38:10 UTC

[GitHub] [maven] hgschmie opened a new pull request, #795: MNG-7529 alternate fix

hgschmie opened a new pull request, #795:
URL: https://github.com/apache/maven/pull/795

   @cstamas  This is the discussed alternate fix for MNG-7529. I structured it differently. The actual meat is in the e70b8c86a99d65c4f1290402f014069178499444 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:
   
    - [X] Make sure there is a [JIRA issue](https://issues.apache.org/jira/browse/MNG) 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] SUMMARY`, where you replace `MNG-XXX`
          and `SUMMARY` 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.
    - [X] 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.
    - [X] You have run the [Core IT][core-its] successfully.
   
   If your pull request is about ~20 lines of code you don't need to sign an
   [Individual Contributor License Agreement](https://www.apache.org/licenses/icla.pdf) 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](http://www.apache.org/licenses/LICENSE-2.0)
   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](http://www.apache.org/licenses/LICENSE-2.0)
   


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


[GitHub] [maven] hgschmie commented on a diff in pull request #795: MNG-7529 alternate fix

Posted by GitBox <gi...@apache.org>.
hgschmie commented on code in PR #795:
URL: https://github.com/apache/maven/pull/795#discussion_r957989082


##########
maven-resolver-provider/src/main/java/org/apache/maven/repository/internal/DefaultVersionRangeResolver.java:
##########
@@ -242,6 +231,28 @@ private Versioning readVersions( RepositorySystemSession session, RequestTrace t
         return ( versioning != null ) ? versioning : new Versioning();
     }
 
+    private Versioning filterVersionsByRepositoryType( Versioning versioning, RemoteRepository remoteRepository )
+    {
+        if ( remoteRepository == null )
+        {
+            return versioning;
+        }
+
+        Versioning filteredVersions = versioning.clone();
+
+        for ( String version : versioning.getVersions() )
+        {
+            boolean snapshotVersion = version != null && version.endsWith( SNAPSHOT );

Review 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. 



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


[GitHub] [maven] cstamas commented on a diff in pull request #795: MNG-7529 alternate fix

Posted by GitBox <gi...@apache.org>.
cstamas commented on code in PR #795:
URL: https://github.com/apache/maven/pull/795#discussion_r958093394


##########
maven-resolver-provider/src/main/java/org/apache/maven/repository/internal/DefaultVersionRangeResolver.java:
##########
@@ -242,6 +231,28 @@ private Versioning readVersions( RepositorySystemSession session, RequestTrace t
         return ( versioning != null ) ? versioning : new Versioning();
     }
 
+    private Versioning filterVersionsByRepositoryType( Versioning versioning, RemoteRepository remoteRepository )
+    {
+        if ( remoteRepository == null )
+        {
+            return versioning;
+        }
+
+        Versioning filteredVersions = versioning.clone();
+
+        for ( String version : versioning.getVersions() )
+        {
+            boolean snapshotVersion = version != null && version.endsWith( SNAPSHOT );

Review 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)?



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


[GitHub] [maven] hgschmie commented on a diff in pull request #795: MNG-7529 alternate fix

Posted by GitBox <gi...@apache.org>.
hgschmie commented on code in PR #795:
URL: https://github.com/apache/maven/pull/795#discussion_r957987649


##########
maven-resolver-provider/src/main/java/org/apache/maven/repository/internal/DefaultVersionRangeResolver.java:
##########
@@ -242,6 +231,28 @@ private Versioning readVersions( RepositorySystemSession session, RequestTrace t
         return ( versioning != null ) ? versioning : new Versioning();
     }
 
+    private Versioning filterVersionsByRepositoryType( Versioning versioning, RemoteRepository remoteRepository )
+    {
+        if ( remoteRepository == null )
+        {
+            return versioning;
+        }
+
+        Versioning filteredVersions = versioning.clone();
+
+        for ( String version : versioning.getVersions() )
+        {
+            boolean snapshotVersion = version != null && version.endsWith( SNAPSHOT );

Review 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. 



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


[GitHub] [maven] michael-o commented on pull request #795: MNG-7529 alternate fix

Posted by GitBox <gi...@apache.org>.
michael-o commented on PR #795:
URL: https://github.com/apache/maven/pull/795#issuecomment-1233183388

   Damn, you were faster than me.


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


[GitHub] [maven] hgschmie commented on a diff in pull request #795: MNG-7529 alternate fix

Posted by GitBox <gi...@apache.org>.
hgschmie commented on code in PR #795:
URL: https://github.com/apache/maven/pull/795#discussion_r958907258


##########
maven-resolver-provider/src/main/java/org/apache/maven/repository/internal/DefaultVersionRangeResolver.java:
##########
@@ -242,6 +231,28 @@ private Versioning readVersions( RepositorySystemSession session, RequestTrace t
         return ( versioning != null ) ? versioning : new Versioning();
     }
 
+    private Versioning filterVersionsByRepositoryType( Versioning versioning, RemoteRepository remoteRepository )
+    {
+        if ( remoteRepository == null )
+        {
+            return versioning;
+        }
+
+        Versioning filteredVersions = versioning.clone();
+
+        for ( String version : versioning.getVersions() )
+        {
+            boolean snapshotVersion = version != null && version.endsWith( SNAPSHOT );

Review 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.



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


[GitHub] [maven] hgschmie merged pull request #795: MNG-7529 alternate fix

Posted by GitBox <gi...@apache.org>.
hgschmie merged PR #795:
URL: https://github.com/apache/maven/pull/795


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


[GitHub] [maven] hgschmie commented on a diff in pull request #795: MNG-7529 alternate fix

Posted by GitBox <gi...@apache.org>.
hgschmie commented on code in PR #795:
URL: https://github.com/apache/maven/pull/795#discussion_r958983789


##########
maven-resolver-provider/src/main/java/org/apache/maven/repository/internal/DefaultVersionRangeResolver.java:
##########
@@ -242,6 +231,28 @@ private Versioning readVersions( RepositorySystemSession session, RequestTrace t
         return ( versioning != null ) ? versioning : new Versioning();
     }
 
+    private Versioning filterVersionsByRepositoryType( Versioning versioning, RemoteRepository remoteRepository )
+    {
+        if ( remoteRepository == null )
+        {
+            return versioning;
+        }
+
+        Versioning filteredVersions = versioning.clone();
+
+        for ( String version : versioning.getVersions() )
+        {
+            boolean snapshotVersion = version != null && version.endsWith( SNAPSHOT );

Review Comment:
   > 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.
   
   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.



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


[GitHub] [maven] cstamas commented on a diff in pull request #795: MNG-7529 alternate fix

Posted by GitBox <gi...@apache.org>.
cstamas commented on code in PR #795:
URL: https://github.com/apache/maven/pull/795#discussion_r958105690


##########
maven-resolver-provider/src/main/java/org/apache/maven/repository/internal/DefaultVersionRangeResolver.java:
##########
@@ -242,6 +231,28 @@ private Versioning readVersions( RepositorySystemSession session, RequestTrace t
         return ( versioning != null ) ? versioning : new Versioning();
     }
 
+    private Versioning filterVersionsByRepositoryType( Versioning versioning, RemoteRepository remoteRepository )
+    {
+        if ( remoteRepository == null )
+        {
+            return versioning;
+        }
+
+        Versioning filteredVersions = versioning.clone();
+
+        for ( String version : versioning.getVersions() )
+        {
+            boolean snapshotVersion = version != null && version.endsWith( SNAPSHOT );

Review 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 :smile: 
   
   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").



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


[GitHub] [maven] hgschmie commented on a diff in pull request #795: MNG-7529 alternate fix

Posted by GitBox <gi...@apache.org>.
hgschmie commented on code in PR #795:
URL: https://github.com/apache/maven/pull/795#discussion_r957672524


##########
maven-resolver-provider/src/main/java/org/apache/maven/repository/internal/DefaultVersionRangeResolver.java:
##########
@@ -242,6 +231,28 @@ private Versioning readVersions( RepositorySystemSession session, RequestTrace t
         return ( versioning != null ) ? versioning : new Versioning();
     }
 
+    private Versioning filterVersionsByRepositoryType( Versioning versioning, RemoteRepository remoteRepository )
+    {
+        if ( remoteRepository == null )
+        {
+            return versioning;
+        }
+
+        Versioning filteredVersions = versioning.clone();
+
+        for ( String version : versioning.getVersions() )
+        {
+            boolean snapshotVersion = version != null && version.endsWith( SNAPSHOT );

Review 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.
   



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


[GitHub] [maven] hgschmie commented on a diff in pull request #795: MNG-7529 alternate fix

Posted by GitBox <gi...@apache.org>.
hgschmie commented on code in PR #795:
URL: https://github.com/apache/maven/pull/795#discussion_r958978758


##########
maven-resolver-provider/src/main/java/org/apache/maven/repository/internal/DefaultVersionRangeResolver.java:
##########
@@ -242,6 +231,28 @@ private Versioning readVersions( RepositorySystemSession session, RequestTrace t
         return ( versioning != null ) ? versioning : new Versioning();
     }
 
+    private Versioning filterVersionsByRepositoryType( Versioning versioning, RemoteRepository remoteRepository )
+    {
+        if ( remoteRepository == null )
+        {
+            return versioning;
+        }
+
+        Versioning filteredVersions = versioning.clone();
+
+        for ( String version : versioning.getVersions() )
+        {
+            boolean snapshotVersion = version != null && version.endsWith( SNAPSHOT );

Review 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](https://issues.apache.org/jira/browse/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".
   
   
   



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


[GitHub] [maven] cstamas commented on a diff in pull request #795: MNG-7529 alternate fix

Posted by GitBox <gi...@apache.org>.
cstamas commented on code in PR #795:
URL: https://github.com/apache/maven/pull/795#discussion_r958105690


##########
maven-resolver-provider/src/main/java/org/apache/maven/repository/internal/DefaultVersionRangeResolver.java:
##########
@@ -242,6 +231,28 @@ private Versioning readVersions( RepositorySystemSession session, RequestTrace t
         return ( versioning != null ) ? versioning : new Versioning();
     }
 
+    private Versioning filterVersionsByRepositoryType( Versioning versioning, RemoteRepository remoteRepository )
+    {
+        if ( remoteRepository == null )
+        {
+            return versioning;
+        }
+
+        Versioning filteredVersions = versioning.clone();
+
+        for ( String version : versioning.getVersions() )
+        {
+            boolean snapshotVersion = version != null && version.endsWith( SNAPSHOT );

Review 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).... 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 :smile: 
   
   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 only, and you'd be done (as MRM merges maven metadata, making them "mixed").



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


[GitHub] [maven] cstamas commented on a diff in pull request #795: MNG-7529 alternate fix

Posted by GitBox <gi...@apache.org>.
cstamas commented on code in PR #795:
URL: https://github.com/apache/maven/pull/795#discussion_r957153178


##########
maven-resolver-provider/src/main/java/org/apache/maven/repository/internal/DefaultVersionRangeResolver.java:
##########
@@ -242,6 +231,28 @@ private Versioning readVersions( RepositorySystemSession session, RequestTrace t
         return ( versioning != null ) ? versioning : new Versioning();
     }
 
+    private Versioning filterVersionsByRepositoryType( Versioning versioning, RemoteRepository remoteRepository )
+    {
+        if ( remoteRepository == null )
+        {
+            return versioning;
+        }
+
+        Versioning filteredVersions = versioning.clone();
+
+        for ( String version : versioning.getVersions() )
+        {
+            boolean snapshotVersion = version != null && version.endsWith( SNAPSHOT );

Review Comment:
   See https://github.com/apache/maven/blob/master/maven-artifact/src/main/java/org/apache/maven/artifact/ArtifactUtils.java#L38



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


[GitHub] [maven] cstamas commented on pull request #795: MNG-7529 alternate fix

Posted by GitBox <gi...@apache.org>.
cstamas commented on PR #795:
URL: https://github.com/apache/maven/pull/795#issuecomment-1230097521

   Other than snapshot detection, looks good.


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


[GitHub] [maven] cstamas commented on a diff in pull request #795: MNG-7529 alternate fix

Posted by GitBox <gi...@apache.org>.
cstamas commented on code in PR #795:
URL: https://github.com/apache/maven/pull/795#discussion_r958105690


##########
maven-resolver-provider/src/main/java/org/apache/maven/repository/internal/DefaultVersionRangeResolver.java:
##########
@@ -242,6 +231,28 @@ private Versioning readVersions( RepositorySystemSession session, RequestTrace t
         return ( versioning != null ) ? versioning : new Versioning();
     }
 
+    private Versioning filterVersionsByRepositoryType( Versioning versioning, RemoteRepository remoteRepository )
+    {
+        if ( remoteRepository == null )
+        {
+            return versioning;
+        }
+
+        Versioning filteredVersions = versioning.clone();
+
+        for ( String version : versioning.getVersions() )
+        {
+            boolean snapshotVersion = version != null && version.endsWith( SNAPSHOT );

Review 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 :smile: 
   
   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 only, and you'd be done (as MRM merges maven metadata, making them "mixed").



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


[GitHub] [maven] hgschmie commented on a diff in pull request #795: MNG-7529 alternate fix

Posted by GitBox <gi...@apache.org>.
hgschmie commented on code in PR #795:
URL: https://github.com/apache/maven/pull/795#discussion_r957987649


##########
maven-resolver-provider/src/main/java/org/apache/maven/repository/internal/DefaultVersionRangeResolver.java:
##########
@@ -242,6 +231,28 @@ private Versioning readVersions( RepositorySystemSession session, RequestTrace t
         return ( versioning != null ) ? versioning : new Versioning();
     }
 
+    private Versioning filterVersionsByRepositoryType( Versioning versioning, RemoteRepository remoteRepository )
+    {
+        if ( remoteRepository == null )
+        {
+            return versioning;
+        }
+
+        Versioning filteredVersions = versioning.clone();
+
+        for ( String version : versioning.getVersions() )
+        {
+            boolean snapshotVersion = version != null && version.endsWith( SNAPSHOT );

Review Comment:
   looking at https://maven.apache.org/ref/3.8.6/maven-repository-metadata/repository-metadata.html#versioning, it is IMHO clear that the versions that this method checks 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. 



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org