Skip to content

Consider robustness for VersionIdentifier parsing #2725

Description

@hohwille

Task summary

Small review leftover from PR #2586:

  • The method VersionIdentifer.of(String) parses a given String and shall return the parsed VersionIdentifier instance or null if the given String was already null or pure non-sense and obviously not a version.
  • Examples of "non-sense" are multiline texts (e.g. we wanted to get a version from a git tag name but the git output returned a multi-line error or warning message instead of the expected tag name), content with other ASCII-control characters such as tab. We currently have an assertion for this case but that seems wrong to me since we should also have handling for that if assertions are disabled.
  • There are cases where our main purpose is to process a specific version e.g. when we install a tool with a version given by the end-user or taken from the configured version variable. If that version is "non-sense", then it would be good to have fast fail behaviour and get an appropriate error message for the end user (without stacktrace) like The value "hello world สวัสดีชาวโลก Привет, мир 魑魅魍魉" does not seem to be a valid version identifier.
  • However, in some other case we might receive a list of versions from a package manager and be rather robust and only log warnings but ignore and proceed in case one such version is somehow messy. It is not nice coding style if we then always have to do try { version = VersionIdentifier.of(input); ....} catch (RuntimeException e) { LOG.warn(...); }.

Maybe we should offer two variants to parse a VersionIdentifier:

  • ofRequired method should throw a dedicated VersionParseException derived from CliException and never return null.
  • ofOptional method will log a warning if version cannot be parsed and return null.
  • To avoid pointless exception catching both methods should delegate to a private and internal of method variant that will get a boolean flag telling to throw exception or log a warning in case the given input is non-sense.
  • We should then go through our current usages of of method and decide which one should go to which replaced variant and ensure the existing of method does not exist any more. IMHO most cases should use ofRequired but places where we have explicit null checks in the using code, should use ofOptional.

Additional context

No response

Activity

  1. added theissue type on Oct 22, 2026
  2. moved this from 🆕 New to 🏗 In progress in IDEasy boardon Oct 22, 2026
  3. hohwille commented on Oct 23, 2026

    @hohwille
    MemberAuthor

    From Retest of #2586

    Skipping invalid npm version '0.0.8888888888' for package '@google/gemini-cli': For input string: "8888888888"
    Skipping invalid npm version '0.0.88888888888' for package '@google/gemini-cli': For input string: "88888888888"
    Skipping invalid npm version '0.0.8999999999' for package '@google/gemini-cli': For input string: "8999999999"
    Skipping invalid npm version '0.0.89999999999' for package '@google/gemini-cli': For input string: "89999999999"
    Skipping invalid npm version '0.0.899999999999' for package '@google/gemini-cli': For input string: "899999999999"
    Skipping invalid npm version '0.0.8999999999999' for package '@google/gemini-cli': For input string: "8999999999999"
    Skipping invalid npm version '0.0.89999999999999' for package '@google/gemini-cli': For input string: "89999999999999"
    Skipping invalid npm version '0.0.8999999999999999' for package '@google/gemini-cli': For input string: "8999999999999999"
    

    This does not make sense to me. Why are these invalid versions?
    Are we using int instead of long for the numeric representation of a segment number?
    Can we make the code more robust so in case of an overflow we set the number to MAX_VALUE but keep the full information in the String representation and ensure we can rebuild the version as String properly.
    Surely we then would still have issues comparing 0.0.123456789012345678901234567890 with 0.0.123456789012345678901234567891.
    Maybe we can also keep the information that we had an overflow so those two versions would not become equal but rather be um-comparable or even better then fallback to String comparison (not lexicographic but simply by digits stripping leading zeros, if the remainder of one is longer, then the number is bigger, if equal compare digit by digit LTR and detect greater/lower on first mismatch).

  4. added 11 commits that reference this issue on Oct 23, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Labels

Type

Projects

Milestone

No milestone

Relationships

None yet

Development

No branches or pull requests

Issue actions