Skip to content

fix: typo in RequestedVersionComparator comparing v1 against itself#2528

Draft
maxandersen wants to merge 1 commit into
mainfrom
fix/requested-version-comparator-typo
Draft

fix: typo in RequestedVersionComparator comparing v1 against itself#2528
maxandersen wants to merge 1 commit into
mainfrom
fix/requested-version-comparator-typo

Conversation

@maxandersen

Copy link
Copy Markdown
Collaborator

Bug

RequestedVersionComparator.compare() had a typo on line 361 of JavaUtil.java:

int n1 = minRequestedVersion(v1);
int n2 = minRequestedVersion(v1); // should be v2

Both values were derived from v1, so the comparator was version-blind — it could never distinguish different version numbers, only the + suffix.

This affected:

  • Directives.javaVersion().max() across multiple //JAVA directives would not reliably pick the highest version
  • ProjectBuilder.updateProject(Source, ...) — merging java versions from source dependencies could let a lower version silently win

Fix

One-character fix: v1v2.

Added TestRequestedVersionComparator covering null handling, same/different versions, exact vs open versions, and max() selection.

The comparator used minRequestedVersion(v1) for both n1 and n2,
making it unable to distinguish different version numbers. This
meant that e.g. .max() across multiple //JAVA directives would
not reliably pick the highest version.
@coderabbitai

coderabbitai Bot commented Jun 13, 2026

Copy link
Copy Markdown
Contributor

Important

Review skipped

Auto reviews are limited based on label configuration.

🏷️ Required labels (at least one) (1)
  • ai-review

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro Plus

Run ID: 634b8b42-bc43-4d4d-b0c4-aa9e91f78774

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

  • 🔍 Trigger review

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@maxandersen

Copy link
Copy Markdown
Collaborator Author

@quintesse just spotted this by random - I guess we haven't had many projects with multiple //JAVA statements.

it does reveal that apparently we have code trying to pick the highest version rather than the first one, but this bug makes it actaully pick the first one - which is what I expected...

so not just merging this in but something we should figure out - should first one wins or highest win?

@maxandersen maxandersen marked this pull request as draft June 13, 2026 06:12
@quintesse

Copy link
Copy Markdown
Contributor

My intuition says the highest should win, because if we're including code that says it needs a higher Java version then I'd assume it wouldn't work with a lower one. Now You could say "then the main script should update its //JAVA line", and that wouldn't be wrong, but then I'd at least want a waning saying that you probably should update your //JAVA line because of a conflict... and that's more difficult than just picking the highest by default :-)

@quintesse

Copy link
Copy Markdown
Contributor

At least it seems that devkitman doesn't have that particular code (most of that file was copied to devkitman)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants