Bug Fixes: Maven Classifier, SemVer #1924

a1flecke · 2020-06-19T20:03:46Z

Add support for maven dependency classifiers

    classifier:
    The classifier distinguishes artifacts that were built from the same POM but differ in content. It is some optional and arbitrary string that - if present - is appended to the artifact name just after the version number.

    As a motivation for this element, consider for example a project that offers an artifact targeting Java 11 but at the same time also an artifact that still supports Java 1.8. The first artifact could be equipped with the classifier jdk11 and the second one with jdk8 such that clients can choose which one to use.

    Another common use case for classifiers is to attach secondary artifacts to the project's main artifact. If you browse the Maven central repository, you will notice that the classifiers sources and javadoc are used to deploy the project source code and API docs along with the packaged class files.

Build Identifiers should not be used in SemVer comparisions
- Dependabot was not ignoring the build identifier when determing if a dependency update was a semver update

aaiezza · 2020-06-19T20:12:04Z

This would be a great addition to dependabot! Thanks for making this @a1flecke!

a1flecke · 2020-06-22T12:59:58Z

@feelepxyz All checks pass. My employer is encountering both of these bugs which is lowering the utility of dependabot for us. Please let me know if there is something that I need to do for this PR.

maven/lib/dependabot/maven.rb

feelepxyz · 2020-06-22T13:44:29Z

maven/lib/dependabot/maven/update_checker/version_finder.rb

@@ -257,15 +258,16 @@ def dependency_metadata_url(repository_url)
        end

        def dependency_files_url(repository_url, version)
-          group_id, artifact_id = dependency.name.split(":")
+          group_id, artifact_id, classifier = dependency.name.split(":")


Looks like this also needs to change in dependency_metadata_url

Looks like there's also quite a few instances where we split and get the last element as the artifact id in the MetadataFinder, e.g. https://github.com/dependabot/dependabot-core/blob/master/maven/lib/dependabot/maven/metadata_finder.rb#L26

@feelepxyz From what I can tell the classifier itself it not used in the url for fetching maven metadata. Mulesoft dependencies use classifiers a lot, an example being the pom file in this repo I created for a Mulesoft support ticket. I checked a couple of the metadata paths for those dependencies and only group_id and artifact_id were needed.

Thank you for catching the other .last calls. I will fix those

feelepxyz

Thanks for putting this together and writing all the tests 🙇 added a few minor comments. Could you also add a test case in the file_updater_spec.rb updating io.mockk in the basic pom file you edited?

* [Add support for maven dependency classifiers](https://maven.apache.org/pom.html#dependencies) >classifier: >The classifier distinguishes artifacts that were built from the same POM but differ in content. It is some optional and arbitrary string that - if present - is appended to the artifact name just after the version number. >As a motivation for this element, consider for example a project that offers an artifact targeting Java 11 but at the same time also an artifact that still supports Java 1.8. The first artifact could be equipped with the classifier jdk11 and the second one with jdk8 such that clients can choose which one to use. >Another common use case for classifiers is to attach secondary artifacts to the project's main artifact. If you browse the Maven central repository, you will notice that the classifiers sources and javadoc are used to deploy the project source code and API docs along with the packaged class files. * [Build Identifiers should not be used in SemVer comparisions](https://semver.org/#spec-item-11) * Dependabot was not ignoring the build identifier when determing if a dependency update was a semver update

Co-authored-by: Philip Harrison <philip@mailharrison.com>

a1flecke · 2020-06-22T18:35:29Z

@feelepxyz I could use some assistance here. I have been fighting what appears to be some flaky tests with VCR and requests to rubygems Are these tests known to have issues?

Example results when running locally is VS Code devcontainer using the same rspec seed:

Finished in 2 minutes 38.6 seconds (files took 1.29 seconds to load)
680 examples, 0 failures

Randomized with seed 50232

[dependabot-core-dev] ~/dependabot-core/bundler $ pwd
/home/dependabot/dependabot-core/bundler

a1flecke · 2020-06-23T12:50:46Z

@feelepxyz It seems like the backports gem is missing a spec fixture in "fixtures/ruby/rubygems_responses/. Can you provide direction on how I generate the info-backports file?

I believe this helper will then setup the missing HTTP stub

The CircleCI failure.

     Failure/Error: raise VCR::Errors::UnhandledHTTPRequestError.new(vcr_request)
 
     
 
     Dependabot::SharedHelpers::ChildProcessFailed:
 
       Child process raised VCR::Errors::UnhandledHTTPRequestError with message: 
 
     
 
       ================================================================================
 
       An HTTP request has been made that VCR does not know how to handle:
 
         GET https://index.rubygems.org/info/backports

feelepxyz · 2020-06-23T13:30:02Z

@feelepxyz It seems like the backports gem is missing a spec fixture in "fixtures/ruby/rubygems_responses/. Can you provide direction on how I generate the info-backports file?

We currently don't have any tooling but if you copy this https://index.rubygems.org/info/backports into a file named info-backports and add it https://github.com/dependabot/dependabot-core/blob/master/bundler/spec/fixtures/ruby/rubygems_responses/ it should work.

These stubs are pretty annoying as it seems like VCR/webmock don't always pick up that they are needed. Possibly because we run Bundler in a forked process 🤷‍♂️

maven/lib/dependabot/maven/metadata_finder.rb

maven/lib/dependabot/maven/update_checker/version_finder.rb

Co-authored-by: Philip Harrison <philip@mailharrison.com>

feelepxyz

This is great! Thanks again and big 🙌 for the thorough testing 🙇 hopefully tests will pass now 🤞

feelepxyz · 2020-06-23T15:00:30Z

Looks like maven, common and bundler are passing so going to this 🎉

a1flecke · 2020-06-23T15:02:00Z

@feelepxyz Thanks for your help with this.

honnix · 2021-08-03T16:18:09Z

Sorry for digging into this old thread. This is a great improvement btw. Thanks for doing this.

We hit a corner case of this change so would like to clarify whether it is intended or not.

When checking whether a dependency is internal, meaning some submodule of a project, classifier is appended to the full name; however when building up the list of internal dependencies, there is no classifier appended because it is hard if not impossible to known. So the problem we have now is Dependabot would try to check newer version of a submodule if in parent pom.xml we have:

<dependencyManagement>
  <dependencies>
    <groupId>foo</groupId>
    <artifactId>bar</artifactId>
    <version>${project.version}</version>
    <classifier>foobar</classifier>
  </dependencies>
</dependencyManagement>

Because it believes foo:bar:${project.version}:foobar is not an internal dependency.

Due to another no-so-standard setup we have, a submodule would have an auto-bumped version of artifacts published every time CI passes in main branch (without changing source code), and the result of that is the published artifacts have higher version than the project's. In this case, for this classified dependency, Dependabot would try to bump the project version in parent pom.xml because it is a property, and that is not what we expect since it results an invalid parent pom.xml because all children still refer to the old version in parent section.

My question is, whether this is intended change? Would it be better we exclude classifier when checking internal dependencies? A possible consequence of skipping classifier is that the classified artifact is indeed not published by a submodule of the current project, but the probability of this setup would be very low.

a1flecke · 2021-08-03T17:36:47Z

@honnix Unfortunately a lot of time has passed since I created this PR. Given the complexity of Maven I cannot be 100% certain, but you are likely accurate that using a classifier for intra-project dependencies is not going to work since the classifier comes into play when an artifact is published.

honnix · 2021-08-03T19:51:18Z

@a1flecke Thanks for replying and the confirmation. I will try to work on a PR to address this.

a1flecke force-pushed the maven-classifier branch from c98cc04 to 08af3c8 Compare June 22, 2020 11:47

feelepxyz reviewed Jun 22, 2020

View reviewed changes

maven/lib/dependabot/maven.rb Outdated Show resolved Hide resolved

feelepxyz reviewed Jun 22, 2020

View reviewed changes

a1flecke and others added 2 commits June 22, 2020 12:00

Update maven/lib/dependabot/maven.rb

d9bd96c

Co-authored-by: Philip Harrison <philip@mailharrison.com>

a1flecke force-pushed the maven-classifier branch 3 times, most recently from 88cf5a7 to dbb7740 Compare June 22, 2020 18:10

a1flecke requested a review from feelepxyz June 23, 2020 11:55

Address PR requests

a2779cf

a1flecke force-pushed the maven-classifier branch from dbb7740 to a2779cf Compare June 23, 2020 11:56

feelepxyz reviewed Jun 23, 2020

View reviewed changes

maven/lib/dependabot/maven/metadata_finder.rb Outdated Show resolved Hide resolved

feelepxyz reviewed Jun 23, 2020

View reviewed changes

maven/lib/dependabot/maven/update_checker/version_finder.rb Outdated Show resolved Hide resolved

a1flecke and others added 3 commits June 23, 2020 08:38

Update maven/lib/dependabot/maven/update_checker/version_finder.rb

5fdeeb1

Co-authored-by: Philip Harrison <philip@mailharrison.com>

Update maven/lib/dependabot/maven/metadata_finder.rb

4498942

Co-authored-by: Philip Harrison <philip@mailharrison.com>

Add backports info

c62a494

feelepxyz approved these changes Jun 23, 2020

View reviewed changes

feelepxyz merged commit 55ce883 into dependabot:master Jun 23, 2020

a1flecke mentioned this pull request Jul 7, 2020

What release is Github running #2292

Closed

honnix mentioned this pull request Aug 3, 2021

Skip classifier when checking for internal dep #4117

Merged

jakecoffman mentioned this pull request Sep 6, 2023

Maven: fix classifier being part of the dependency name #7980

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Bug Fixes: Maven Classifier, SemVer #1924

Bug Fixes: Maven Classifier, SemVer #1924

a1flecke commented Jun 19, 2020 •

edited

Loading

aaiezza commented Jun 19, 2020

a1flecke commented Jun 22, 2020

feelepxyz Jun 22, 2020

feelepxyz Jun 22, 2020

a1flecke Jun 22, 2020

feelepxyz left a comment •

edited

Loading

a1flecke commented Jun 22, 2020

a1flecke commented Jun 23, 2020

feelepxyz commented Jun 23, 2020

feelepxyz left a comment

feelepxyz commented Jun 23, 2020

a1flecke commented Jun 23, 2020

honnix commented Aug 3, 2021

a1flecke commented Aug 3, 2021

honnix commented Aug 3, 2021

Bug Fixes: Maven Classifier, SemVer #1924

Bug Fixes: Maven Classifier, SemVer #1924

Conversation

a1flecke commented Jun 19, 2020 • edited Loading

aaiezza commented Jun 19, 2020

a1flecke commented Jun 22, 2020

feelepxyz Jun 22, 2020

Choose a reason for hiding this comment

feelepxyz Jun 22, 2020

Choose a reason for hiding this comment

a1flecke Jun 22, 2020

Choose a reason for hiding this comment

feelepxyz left a comment • edited Loading

Choose a reason for hiding this comment

a1flecke commented Jun 22, 2020

a1flecke commented Jun 23, 2020

feelepxyz commented Jun 23, 2020

feelepxyz left a comment

Choose a reason for hiding this comment

feelepxyz commented Jun 23, 2020

a1flecke commented Jun 23, 2020

honnix commented Aug 3, 2021

a1flecke commented Aug 3, 2021

honnix commented Aug 3, 2021

a1flecke commented Jun 19, 2020 •

edited

Loading

feelepxyz left a comment •

edited

Loading