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

Do not update {major}.json if an older version was published #483

Merged
merged 7 commits into from
Oct 3, 2023
Merged

Do not update {major}.json if an older version was published #483

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
8ee09d0
Do not update `{major}.json` if an older version was published
cleptric Aug 3, 2023
bc32cc6
Move log stuff around
cleptric Aug 3, 2023
7300fa4
add tests
tonyo Oct 2, 2023
836a11f
Merge branch 'master' into fix-major-symlinks
tonyo Oct 2, 2023
149be13
rewrite symlink updating logic based on existing symlinks
Lms24 Oct 3, 2023
94d1ce1
apply review feedback
Lms24 Oct 3, 2023
01a3661
further simplify logger and fix link lookup
Lms24 Oct 3, 2023
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
235 changes: 235 additions & 0 deletions src/utils/__tests__/symlink.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,235 @@
import { createSymlinks } from '../symlink';
import { withTempDir } from '../files';
import { promises as fsPromises } from 'fs';

import * as path from 'path';
import * as fs from 'fs';

describe('createSymlinks', () => {
it.each([
{
_name: 'major version upgrade',
oldVersion: '0.9.0',
newVersion: '1.0.0',
expectedSymlinks: ['1.json', '1.0.json', 'latest.json'],
},
{
_name: 'minor version upgrade',
oldVersion: '0.2.3',
newVersion: '0.3.0',
expectedSymlinks: ['0.json', '0.3.json', 'latest.json'],
},
{
_name: 'patch version upgrade',
oldVersion: '0.2.3',
newVersion: '0.2.4',
expectedSymlinks: ['0.json', '0.2.json', 'latest.json'],
},
{
_name: 'old version is undefined',
oldVersion: undefined,
newVersion: '1.2.3',
expectedSymlinks: ['1.json', '1.2.json', 'latest.json'],
},
{
_name: 'updating older major (patch)',
oldVersion: '2.3.4',
newVersion: '1.2.3',
expectedSymlinks: ['1.json', '1.2.json'],
},
{
_name: 'updating older major (minor)',
oldVersion: '2.3.4',
newVersion: '1.3.0',
expectedSymlinks: ['1.json', '1.3.json'],
},
])('$_name', async ({ oldVersion, newVersion, expectedSymlinks }) =>
withTempDir(async tmpDir => {
const versionFile = `${newVersion}.json`;
const versionFilePath = path.join(tmpDir, versionFile);

createSymlinks(versionFilePath, newVersion, oldVersion);

// Check that there are no other files in the directory
const filesInDir = await fsPromises.readdir(tmpDir);
expect(filesInDir.sort()).toStrictEqual(expectedSymlinks.sort());

for (const symlinkFile of filesInDir) {
const symlinkFilePath = path.join(tmpDir, symlinkFile);

// Make sure it's a symbolic link
const fileStat = await fsPromises.lstat(symlinkFilePath);
expect(fileStat.isSymbolicLink()).toBe(true);

// Make sure it points to the right file
const symlinkDestination = await fsPromises.readlink(symlinkFilePath);
expect(symlinkDestination).toBe(versionFile);
}
}, true)
);

describe('correctly handles already existing symlinks', () => {
it('when updating an old major version', async () =>
Copy link
Member

Choose a reason for hiding this comment

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

the describe / its read backwards here -- I think they're supposed to be the other way around so the code "reads"?

Copy link
Member

Choose a reason for hiding this comment

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

I tried to "group" the three tests that cover multiple link calls and hence handle actually existing files/symlinks.
I'll just remove the describe block here to clear up the confusion.

withTempDir(async tmpDir => {
// fill the directory with some "previous" versions and symlinks
// (we also need to also the concrete version file to avoid broken symlinks)

fs.writeFileSync(path.join(tmpDir, '1.0.0.json'), 'x');
createSymlinks(path.join(tmpDir, '1.0.0.json'), '1.0.0');

fs.writeFileSync(path.join(tmpDir, '2.0.0.json'), 'x');
createSymlinks(path.join(tmpDir, '2.0.0.json'), '2.0.0', '1.0.0');

// now update 1.x (minor)
fs.writeFileSync(path.join(tmpDir, '1.5.0.json'), 'x');
createSymlinks(path.join(tmpDir, '1.5.0.json'), '1.5.0', '2.0.0');

// now update a version in between 1.x and 1.5.x
fs.writeFileSync(path.join(tmpDir, '1.2.0.json'), 'x');
createSymlinks(path.join(tmpDir, '1.2.0.json'), '1.2.0', '2.0.0');

// now update 1.5.x (patch)
fs.writeFileSync(path.join(tmpDir, '1.5.1.json'), 'x');
createSymlinks(path.join(tmpDir, '1.5.1.json'), '1.5.1', '2.0.0');

const filesInDir = await fsPromises.readdir(tmpDir);
expect(filesInDir.sort()).toStrictEqual(
[
'1.0.0.json',
'1.2.0.json',
'1.5.0.json',
'1.5.1.json',
'2.0.0.json',

'1.json',
'2.json',

'1.0.json',
'1.2.json',
'1.5.json',
'2.0.json',

'latest.json',
].sort()
);

const latestLink = await fsPromises.readlink(
path.join(tmpDir, 'latest.json')
);
const major1Link = await fsPromises.readlink(
path.join(tmpDir, '1.json')
);
const major2Link = await fsPromises.readlink(
path.join(tmpDir, '2.json')
);
const minor10Link = await fsPromises.readlink(
path.join(tmpDir, '1.0.json')
);
const minor12Link = await fsPromises.readlink(
path.join(tmpDir, '1.2.json')
);
const minor15Link = await fsPromises.readlink(
path.join(tmpDir, '1.5.json')
);
const minor20Link = await fsPromises.readlink(
path.join(tmpDir, '2.0.json')
);

expect(latestLink).toBe('2.0.0.json');

expect(major1Link).toBe('1.5.1.json');
expect(major2Link).toBe('2.0.0.json');

expect(minor10Link).toBe('1.0.0.json');
expect(minor12Link).toBe('1.2.0.json');
expect(minor15Link).toBe('1.5.1.json');
expect(minor20Link).toBe('2.0.0.json');
}, true));

it('when updating a previous minor version on the same major', async () =>
withTempDir(async tmpDir => {
fs.writeFileSync(path.join(tmpDir, '1.0.0.json'), 'x');
createSymlinks(path.join(tmpDir, '1.0.0.json'), '1.0.0');

fs.writeFileSync(path.join(tmpDir, '1.1.0.json'), 'x');
createSymlinks(path.join(tmpDir, '1.1.0.json'), '1.1.0', '1.0.0');

fs.writeFileSync(path.join(tmpDir, '1.0.1.json'), 'x');
createSymlinks(path.join(tmpDir, '1.0.1.json'), '1.0.1', '1.1.0');

const filesInDir = await fsPromises.readdir(tmpDir);
expect(filesInDir.sort()).toStrictEqual(
[
'1.0.0.json',
'1.0.1.json',
'1.1.0.json',

'1.json',

'1.0.json',
'1.1.json',

'latest.json',
].sort()
);

const latestLink = await fsPromises.readlink(
path.join(tmpDir, 'latest.json')
);
const major1Link = await fsPromises.readlink(
path.join(tmpDir, '1.json')
);
const minor10Link = await fsPromises.readlink(
path.join(tmpDir, '1.0.json')
);
const minor11Link = await fsPromises.readlink(
path.join(tmpDir, '1.1.json')
);

expect(latestLink).toBe('1.1.0.json');
expect(major1Link).toBe('1.1.0.json');
expect(minor10Link).toBe('1.0.1.json');
expect(minor11Link).toBe('1.1.0.json');
}, true));
});

// This is quite an edge case but nevertheless good to know it's covered:
it('when updating a previous patch version on the same minor', async () =>
withTempDir(async tmpDir => {
fs.writeFileSync(path.join(tmpDir, '1.0.0.json'), 'x');
createSymlinks(path.join(tmpDir, '1.0.0.json'), '1.0.0');

fs.writeFileSync(path.join(tmpDir, '1.0.2.json'), 'x');
createSymlinks(path.join(tmpDir, '1.0.2.json'), '1.0.2', '1.0.0');

fs.writeFileSync(path.join(tmpDir, '1.0.1.json'), 'x');
createSymlinks(path.join(tmpDir, '1.0.1.json'), '1.0.1', '1.0.2');

const filesInDir = await fsPromises.readdir(tmpDir);
expect(filesInDir.sort()).toStrictEqual(
[
'1.0.0.json',
'1.0.1.json',
'1.0.2.json',

'1.json',

'1.0.json',

'latest.json',
].sort()
);

const latestLink = await fsPromises.readlink(
path.join(tmpDir, 'latest.json')
);
const major1Link = await fsPromises.readlink(path.join(tmpDir, '1.json'));
const minor10Link = await fsPromises.readlink(
path.join(tmpDir, '1.0.json')
);

expect(latestLink).toBe('1.0.2.json');
expect(major1Link).toBe('1.0.2.json');
expect(minor10Link).toBe('1.0.2.json');
}));
});
27 changes: 27 additions & 0 deletions src/utils/__tests__/version.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ import {
isValidVersion,
parseVersion,
SemVer,
semVerToString,
versionGreaterOrEqualThan,
} from '../version';

Expand Down Expand Up @@ -202,3 +203,29 @@ describe('getPackageVersion', () => {
expect(isValidVersion(version)).toBe(true);
});
});

describe('semVerToString', () => {
test.each([
['basic', { major: 1, minor: 2, patch: 3 }, '1.2.3'],
[
'with pre-release',
{ major: 1, minor: 2, patch: 3, pre: 'beta.1' },
'1.2.3-beta.1',
],
[
'with build metadata',
{ major: 1, minor: 2, patch: 3, build: 'linux' },
'1.2.3+linux',
],
[
'with pre-release and build metadata',
{ major: 1, minor: 2, patch: 3, pre: 'beta.1', build: 'linux' },
'1.2.3-beta.1+linux',
],
])(
'converts a SemVer object (%s) to a string',
(_, semver, expectedString) => {
expect(semVerToString(semver)).toBe(expectedString);
}
);
});
91 changes: 74 additions & 17 deletions src/utils/symlink.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,12 @@ import * as path from 'path';

import { logger } from '../logger';
import { ConfigurationError } from './errors';
import { parseVersion, versionGreaterOrEqualThan } from './version';
import {
SemVer,
parseVersion,
semVerToString,
versionGreaterOrEqualThan,
} from './version';

/**
* Creates a symlink, overwriting the existing one
Expand All @@ -21,8 +26,9 @@ function forceSymlink(target: string, newFile: string): void {
/**
* Create symbolic links to the new version file
*
* "latest.json" link is not updated if the new version is "older" (e.g., it's
* a patch release for an older major version).
* "latest.json", "{major}.json" and "{minor}.json" links are respectively not
* updated if the new version is "older" (e.g., it's a patch release for an
* older major version) than the currently linked versions.
*
* @param versionFilePath Path to the new version file
* @param newVersion The new version
Expand All @@ -39,29 +45,80 @@ export function createSymlinks(
}
const parsedOldVersion =
(oldVersion ? parseVersion(oldVersion) : undefined) || undefined;

const baseVersionName = path.basename(versionFilePath);
const packageDir = path.dirname(versionFilePath);

// link latest, but only if the new version is "newer"
if (
parsedOldVersion &&
!versionGreaterOrEqualThan(parsedNewVersion, parsedOldVersion)
!parsedOldVersion ||
versionGreaterOrEqualThan(parsedNewVersion, parsedOldVersion)
) {
logger.warn(
`Not updating the latest version file: current version is "${oldVersion}", new version is "${newVersion}"`
);
} else {
logger.debug(
`Changing symlink for "latest.json" from version "${oldVersion}" to "${newVersion}"`
`${parsedOldVersion ? 'Updating' : 'Adding'} symlink for "latest.json" ${
parsedOldVersion ? `from ${oldVersion} ` : ''
}to "${newVersion}"`
);
forceSymlink(baseVersionName, path.join(packageDir, 'latest.json'));
}

// link major
const majorVersionLink = `${parsedNewVersion.major}.json`;
forceSymlink(baseVersionName, path.join(packageDir, majorVersionLink));
// Read possibly existing symlinks for major and minor versions of the new version
const newVersionMajorSymlinkedVersion = getExistingSymlinkedVersion(
Copy link
Member

Choose a reason for hiding this comment

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

this variable name seems overly verbose and also incorrect -- maybe existingMajorSymlink instead ? (same for the other one right below this)

Copy link
Member

Choose a reason for hiding this comment

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

I replaced it with existingLinkedMajorVersion which hopefully clarifies what this variable is holding (i.e. not the symlink but the already parsed version that this link is linking to).

path.join(packageDir, `${parsedNewVersion.major}.json`)
);
const newVersionMinorSymlinkedVersion = getExistingSymlinkedVersion(
path.join(
packageDir,
`${parsedNewVersion.major}.${parsedNewVersion.minor}.json`
)
);

// link {major}.json if there's no link yet for that major
// or if the new version is newer than the currently linked one
if (
!newVersionMajorSymlinkedVersion ||
versionGreaterOrEqualThan(parsedNewVersion, newVersionMajorSymlinkedVersion)
) {
const majorVersionLink = `${parsedNewVersion.major}.json`;
logger.debug(
`${
newVersionMajorSymlinkedVersion ? 'Updating' : 'Adding'
} symlink for "${majorVersionLink}" ${
newVersionMajorSymlinkedVersion
? `from version "${semVerToString(newVersionMajorSymlinkedVersion)}" `
: ''
}to "${newVersion}"`
);
Copy link
Member

Choose a reason for hiding this comment

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

there's way too much going on in this line -- if it's just for a debug line I'd just put the raw data in here instead of trying to make it end-user readable. should eliminate all the logic here as well since this seems error prone / more likely to break than the actual logic

Copy link
Member

Choose a reason for hiding this comment

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

I guess we can simplify it a little but while working on this, having some readable debug output was immensly helpful. (Fwiw, what we had before showed the wrong "from version" on many occasions).

Copy link
Member

Choose a reason for hiding this comment

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

I think logger.debug('symlinking', {before, after}) would probably be sufficient

Copy link
Member

Choose a reason for hiding this comment

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

updated it again

forceSymlink(baseVersionName, path.join(packageDir, majorVersionLink));
}

// link {minor}.json if there's no link yet for that minor
// or if the new version is newer than the currently linked one
if (
!newVersionMinorSymlinkedVersion ||
versionGreaterOrEqualThan(parsedNewVersion, newVersionMinorSymlinkedVersion)
) {
const minorVersionLink = `${parsedNewVersion.major}.${parsedNewVersion.minor}.json`;
logger.debug(
`${
newVersionMinorSymlinkedVersion ? 'Updating' : 'Adding'
} symlink for "${minorVersionLink}" ${
newVersionMinorSymlinkedVersion
? `from version "${semVerToString(newVersionMinorSymlinkedVersion)}" `
: ''
}to "${newVersion}"`
);
forceSymlink(baseVersionName, path.join(packageDir, minorVersionLink));
}
}

// link minor
const minorVersionLink = `${parsedNewVersion.major}.${parsedNewVersion.minor}.json`;
forceSymlink(baseVersionName, path.join(packageDir, minorVersionLink));
function getExistingSymlinkedVersion(symlinkPath: string): SemVer | null {
try {
// using lstat instead of exists because broken symlinks return false for exists
fs.lstatSync(symlinkPath);
const linkedFile = fs.readlinkSync(symlinkPath);
return parseVersion(path.basename(linkedFile));
} catch {
Copy link
Member

Choose a reason for hiding this comment

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

this try catch is overly broad -- each of these 3 statements could raise errors but I think (?) you only intend to catch errors from the first one? this could hide a programming error especially after future maintenance

Copy link
Member

Choose a reason for hiding this comment

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

good catch!

// this means the symlink doesn't exist
}
return null;
}
Loading
Loading