Skip to content

VCellSoftwareVersion parses PATCH from parts[1], so patchVersion is a copy of minorVersion #2011

Description

@jcschaff

The bug

VCellSoftwareVersion splits the version number on . and then reads the wrong index for PATCH:

String[] parts = versionNumber.split("\\.");
if (parts.length > 1) {
    String major = parts[0];
    String minor = parts[1];
    majorVersion = safeParse(major);
    minorVersion = safeParse(minor);
}
if (parts.length > 2) {
    String patch = parts[1];      // <-- should be parts[2]
    patchVersion = safeParse(patch);
}

getPatchVersion() therefore returns MINOR, never PATCH. Verified against the compiled class:

version string major minor patch
Alpha_Version_8.0.28_build_01 8 0 0 (should be 28)
Alpha_Version_8.1.0_build_01 8 1 1 (should be 0)
Rel_Version_8.0.0_build_03 8 0 0

MAJOR, MINOR and BUILD are all parsed correctly — BUILD comes from a separate _-delimited token and is unaffected. Only PATCH is wrong.

How long

Two commits, the first hiding the second:

  • 2018-02-14, 85ac3c378a ("VCell 7 splash screen, conform to legacy version numbering") introduced the block with versionNumber.split("."). That argument is a regex, so . matches any character and the split returns an empty array — every part stayed -1 and the parts[1] typo was dead code.
  • 2022-09-07, f7fb0efa3b changed it to split("\\.") as a one-line drive-by inside a commit about compartment reordering. That made MAJOR/MINOR work for the first time and simultaneously made the PATCH typo live.

So the line dates from 2018 but has been live and wrong since September 2022.

Why fixing it alone would change behaviour

getPatchVersion() has exactly one consumer: the client/server mismatch dialog in ClientServerManager. (XmlReader's legacy <= 5.2 checks and the VisitorAdapter test use major/minor only.)

That check warned when major, minor or patch differed. Because patch mirrored minor, the effective policy was "warn on MAJOR or MINOR difference" — which is why the dialog stayed silent across the entire 8.0 line, where every build parsed as (8, 0, 0). It last fired at the 7.7 → 8.0 transition.

Repairing parts[2] in isolation would silently convert that into "warn on any PATCH difference". PATCH moves with nearly every release — 8.0.2 through 8.0.28 shipped in about three weeks — and desktop clients update on the user's own schedule, so most users would get "please download the latest client" on every connect.

Current state

The alerting policy has been decoupled from this bug (PR #2012): ClientServerManager now compares MAJOR and MINOR explicitly and no longer consults getPatchVersion(). Fixing parts[1]parts[2] is therefore now behaviour-neutral and safe to do on its own.

Fix

Change parts[1] to parts[2]. Add a unit test covering major/minor/patch/build extraction for a three-part version — the class has no tests. Decide separately, if ever, whether the mismatch dialog should also consider PATCH; that is a product question, not a parsing one.

Activity

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

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Type

    No type

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions