Skip to content
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
39 changes: 39 additions & 0 deletions src/store/DocumentManagementService.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -393,6 +393,17 @@ describe("DocumentManagementService", () => {
expect(versions).toEqual(["2.0.0", "2.0.0-beta", "1.0.0"]);
expect(mockStore.queryUniqueVersions).toHaveBeenCalledWith(library); // Fix: Use mockStoreInstance
});

it("should include partial versions coercible to semver (e.g. '1.20', '5'), sorted descending", async () => {
const library = "test-lib";
mockStore.queryUniqueVersions.mockResolvedValue(["1.20", "5", "stable", "2.0.0"]);

const versions = await docService.listVersions(library);
// "stable" is not coercible to semver and stays excluded; "1.20" and
// "5" are real partial versions (major.minor / major-only) and must
// not be silently dropped just because they aren't full X.Y.Z.
expect(versions).toEqual(["5", "2.0.0", "1.20"]);
});
});

describe("findBestVersion", () => {
Expand Down Expand Up @@ -509,6 +520,34 @@ describe("DocumentManagementService", () => {
docService.findBestVersion(library, "invalid-format"),
).rejects.toThrow(VersionNotFoundInStoreError);
});

it("should resolve a partial version like '1.20' stored exactly as-is", async () => {
mockStore.queryUniqueVersions.mockResolvedValue(["1.20", "2.0.0"]);
mockStore.checkDocumentExists.mockResolvedValue(false);

const result = await docService.findBestVersion(library, "1.20");
// Must return the original stored string ("1.20"), not a coerced
// "1.20.0" — downstream lookups key off what's actually in the store.
expect(result).toEqual({ bestMatch: "1.20", hasUnversioned: false });
});

it("should pick the highest partial version when no target is given", async () => {
mockStore.queryUniqueVersions.mockResolvedValue(["1.20", "5", "2.0.0"]);
mockStore.checkDocumentExists.mockResolvedValue(false);

const result = await docService.findBestVersion(library);
expect(result).toEqual({ bestMatch: "5", hasUnversioned: false });
});

it("should still fall through to unversioned for a non-semver label like 'stable'", async () => {
// "stable" is not a version at all — it should never resolve as a
// bestMatch, with or without the partial-version fix.
mockStore.queryUniqueVersions.mockResolvedValue(["1.20", "stable"]);
mockStore.checkDocumentExists.mockResolvedValue(true);

const result = await docService.findBestVersion(library, "stable");
expect(result).toEqual({ bestMatch: null, hasUnversioned: true });
});
});

describe("listLibraries", () => {
Expand Down
25 changes: 22 additions & 3 deletions src/store/DocumentManagementService.ts
Original file line number Diff line number Diff line change
Expand Up @@ -268,7 +268,11 @@ export class DocumentManagementService {
*/
async listVersions(library: string): Promise<string[]> {
const versions = await this.store.queryUniqueVersions(library);
const validVersions = versions.filter((v) => semver.valid(v));
// Accept anything coercible to semver (e.g. "1.20", "5"), not just strict
// X.Y.Z strings — otherwise real-world partial versions are silently
// dropped and become invisible to findBestVersion(). Non-version labels
// like "stable"/"latest" correctly still fail coercion and stay excluded.
const validVersions = versions.filter((v) => semver.coerce(v) !== null);
return sortVersionsDescending(validVersions);
}

Expand Down Expand Up @@ -320,8 +324,22 @@ export class DocumentManagementService {

let bestMatch: string | null = null;

// semver.maxSatisfying() requires every candidate to itself be a fully
// valid X.Y.Z semver string — a stored version like "1.20" fails that
// even though listVersions() now accepts it. Match on coerced versions,
// then map the winner back to its original stored string so downstream
// lookups (e.g. checkDocumentExists) use the string that's actually in
// the store.
const coercedToOriginal = new Map<string, string>();

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Could we avoid keeping just one original label for each normalized version? If both a partial version and its full form are stored, one replaces the other here, and looking up the partial form can return a different version label. Please add coverage for both stored forms.

Something like this:

type VersionCandidate = {
  stored: string;
  normalized: string;
  strict: boolean;
};

for (const v of versionStrings) {
const coerced = semver.coerce(v);

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

This destroys prerelease labels (2.0.0-beta becomes 2.0.0), so a request for the prerelease can fail or it can win over the real stable release. A small regression test covering both would help.

if (coerced) coercedToOriginal.set(coerced.version, v);
}
const coercedCandidates = Array.from(coercedToOriginal.keys());

if (!targetVersion || targetVersion === "latest") {
bestMatch = semver.maxSatisfying(versionStrings, "*");
const coercedBest = semver.maxSatisfying(coercedCandidates, "*");
bestMatch = coercedBest ? (coercedToOriginal.get(coercedBest) ?? null) : null;
} else {
const versionRegex = /^(\d+)(?:\.(?:x(?:\.x)?|\d+(?:\.(?:x|\d+))?))?$|^$/;
if (!semver.valid(targetVersion) && !versionRegex.test(targetVersion)) {
Expand All @@ -338,7 +356,8 @@ export class DocumentManagementService {
range = `${range} || <=${targetVersion}`;
}
// If it was already a valid range (like '1.x'), use it directly
bestMatch = semver.maxSatisfying(versionStrings, range);
const coercedBest = semver.maxSatisfying(coercedCandidates, range);
bestMatch = coercedBest ? (coercedToOriginal.get(coercedBest) ?? null) : null;
}
}

Expand Down