Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

maven: stop querying repositories once one returns a result #5872

Merged
merged 4 commits into from
Oct 12, 2022
Merged

maven: stop querying repositories once one returns a result #5872

merged 4 commits into from
Oct 12, 2022

Conversation

jakecoffman
Copy link
Member

Per the Maven docs:

Remote repository URLs are queried in the following order for artifacts until one returns a valid result

(emphasis mine)

So when Dependabot checks for updated versions, if it finds some it should stop querying. If the user has outdated dependencies in a registry that may be on purpose, and we should honor those versions presented and not try to find more.

I found "that augment the central repo" a good test of this as it failed once I made this change, so I fixed it and didn't feel the need to write another test.

@jakecoffman jakecoffman requested a review from a team as a code owner October 11, 2022 19:15
@jakecoffman jakecoffman force-pushed the jakecoffman/maven-find-then-stop branch from a73d862 to 2907e5a Compare October 11, 2022 19:22
landongrindheim
landongrindheim approved these changes Oct 11, 2022
View reviewed changes
Copy link
Member

@landongrindheim landongrindheim left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Huge improvement!

maven/spec/dependabot/maven/update_checker/version_finder_spec.rb
Comment on lines -712 to +715
its(:count) { is_expected.to eq(87) }
its(:count) { is_expected.to eq(17) }
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🤯

maven/lib/dependabot/maven/update_checker/version_finder.rb Outdated Show resolved Hide resolved
mctofu
mctofu reviewed Oct 11, 2022
View reviewed changes
Copy link
Contributor

@mctofu mctofu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good! One question is whether the repos are currently in the right order where the first to return is the one we want?

@jakecoffman
Copy link
Member Author

@mctofu It seems to be, but I noticed the specs are using match_array which doesn't check order. I'll switch to using eq since that's important now.

jakecoffman and others added 2 commits October 11, 2022 15:16
@jakecoffman
assert order since that is important
3912e67
@jakecoffman @landongrindheim
easier to understand
dbec4b2 
Co-authored-by: Landon Grindheim <landon.grindheim@gmail.com>
@jakecoffman
Copy link
Member Author

I'll just state here that the rules from the docs linked above are:

  1. effective settings:
    A. Global settings.xml
    B. User settings.xml
  2. local effective build POM:
    A. Local pom.xml
    B. Parent POMs, recursively
    C. Super POM
  3. effective POMs from dependency path to the artifact

1 and 3 don't apply because Dependabot doesn't support settings.xml, and runs in a clean environment.

Rule 2 is captured pretty succinctly in this recursive section of code.

I've updated the tests and they look correct.

mctofu
mctofu approved these changes Oct 11, 2022
View reviewed changes
Copy link
Contributor

@mctofu mctofu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🎉

@jakecoffman jakecoffman merged commit 42c1413 into main Oct 12, 2022
@jakecoffman jakecoffman deleted the jakecoffman/maven-find-then-stop branch October 12, 2022 13:09
@pavera pavera mentioned this pull request Oct 31, 2022
Merged
@jeffwidman jeffwidman mentioned this pull request Apr 20, 2023
Closed
1 task
ugexe added a commit to maxmind/minfraud-api-java that referenced this pull request Nov 1, 2023
@ugexe
Remove ossrh-snapshots repository
83e9a52 
Dependabot will stop looking for maven dependencies once it finds
a repository that returns any number of results, even if none of
those results match the requested version. This removes the ossrh
repository because it brings in snapshot repositories that prevent
dependabot from falling back to the central repository. See:
dependabot/dependabot-core#5872
@ugexe ugexe mentioned this pull request Nov 1, 2023
Merged
ugexe added a commit to maxmind/GeoIP2-java that referenced this pull request Nov 1, 2023
@ugexe
Remove ossrh-snapshots repository
4ba494b 
Dependabot will stop looking for maven dependencies once it finds
a repository that returns any number of results, even if none of
those results match the requested version. This removes the ossrh
repository because it brings in snapshot repositories that prevent
dependabot from falling back to the central repository. See:
dependabot/dependabot-core#5872
@ugexe ugexe mentioned this pull request Nov 1, 2023
Merged
ugexe added a commit to maxmind/MaxMind-DB-Reader-java that referenced this pull request Nov 1, 2023
@ugexe
Remove ossrh-snapshots repository
f89492b 
Dependabot will stop looking for maven dependencies once it finds
a repository that returns any number of results, even if none of
those results match the requested version. This removes the ossrh
repository because it brings in snapshot repositories that prevent
dependabot from falling back to the central repository. See:
dependabot/dependabot-core#5872
@ugexe ugexe mentioned this pull request Nov 1, 2023
Merged
@daniel-beck daniel-beck mentioned this pull request Mar 27, 2024
Open
@olamy
Copy link

olamy commented Mar 28, 2024

Hi,
I came into this PR.
I will give you my POV as Maven committer since 2007, Maven PMC member and been chair of the project few years.

You are right regarding

Remote repository URLs are queried in the following order for artifacts until one returns a valid result

But this applies when Maven is querying artifact foo in version x.
So if Maven finds repositories x, y, and z and finds artifact foo in version 1 in repository x, Maven will stop here, of course.
Now the Maven build needs artifact foo in version 2, but this one is not available in repository x, but Maven finds it in repository y. Maven will use it from repository y. And then artifact foo in version 3 can be in repository z.
This means Maven can search multiple repositories for a given version.
As dependabot have a goal of proposing an upgrade of version and most of the time the highest version, this highest version can be in another repository than one containing the current (old?) version.
Ideally, as Maven repository managers are working (see https://maven.apache.org/repository-management.html, dependabot should scan all repositories and aggregate all the available metadata for an artifact and so propose the highest version for an artifact available in all repositories.
Please let me know if this makes sense.

@daniel-beck daniel-beck mentioned this pull request Mar 28, 2024
Open
1 task
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mctofu mctofu mctofu approved these changes

@landongrindheim landongrindheim landongrindheim approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@jakecoffman @olamy @mctofu @landongrindheim