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.

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

feat(composer): filter releases using php constraint #19851

Closed
wants to merge 2 commits into from
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
55 changes: 55 additions & 0 deletions lib/modules/datasource/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ import { regEx } from '../../util/regex';
import { trimTrailingSlash } from '../../util/url';
import * as allVersioning from '../versioning';
import datasources from './api';
import { GithubTagsDatasource } from './github-tags';
import { addMetaData } from './metadata';
import { setNpmrc } from './npm';
import { resolveRegistryUrl } from './npm/npmrc';
Expand Down Expand Up @@ -415,6 +416,60 @@ export async function getPkgReleases(
version.matches(constraintValue, releaseConstraint)
);
});
} else {
if (constraintName === 'php') {
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Ideally don't hardcode this so that it can be reused for other ecosystems in future without a rewrite (especially python and node)

Copy link
Contributor

@herndlm herndlm Jan 15, 2023

Choose a reason for hiding this comment

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

While skimming through the code yesterday I thought the filtering could be done directly in the packagist lookup. Was I wrong or did you out it here to be potentially reusable on purpose?
UPDATE: nevermind, understood it I think

const composerVersioning = allVersioning.get('composer');
if (composerVersioning.isValid(constraintValue)) {
const lookupConfig: GetPkgReleasesConfig = {
datasource: GithubTagsDatasource.id,
depName: 'php/php-src',
extractVersion: '^php-(?<version>\\S+)',
};
const phpVersions = (
await getPkgReleases(lookupConfig)
)?.releases.map((release) => release.version);
Comment on lines +428 to +430
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I suggest that this is done once per run, for optimization reasons

if (!phpVersions) {
logger.warn('Could not fetch php releases for compatibility check');
return null;
}
Comment on lines +431 to +434
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This is a huge problem if it ever happens, not sure whether to throw an ExternalHostError or not

Choose a reason for hiding this comment

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

You can cache this right? Because having to look this up for every single PHP run sounds wasteful. And if it happens, blow out and try again later?

const matchingVersions = phpVersions.filter((version) =>
composerVersioning.matches(version, constraintValue)
);
Comment on lines +435 to +437
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This should also be done once per run per constraint

logger.debug(
`Found ${matchingVersions.length} matching php versions`
);
if (matchingVersions.length) {
const originalReleaseCount = res.releases.length;
res.releases = res.releases.filter((release) => {
const constraint = release.constraints?.[constraintName];
if (!is.nonEmptyArray(constraint)) {
// A release with no constraints is OK
return true;
}
return constraint.some(
// If any of the release's constraints match, then it's OK
(releaseConstraint) => {
const releaseMatchingVersions = phpVersions.filter(
(version) =>
composerVersioning.matches(version, releaseConstraint)
);
Comment on lines +452 to +455
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Should also be calculated once per run per constraint


const isMatch = matchingVersions.every((version) =>
releaseMatchingVersions.includes(version)
);
Comment on lines +457 to +459
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This could also be cached per run

return isMatch;
}
);
});
const filteredReleaseCount = res.releases.length;
logger.debug(
`${filteredReleaseCount} of ${originalReleaseCount} releases have matching constraints`
);
}
} else {
logger.warn({ constraintValue }, 'Invalid php constraint');
}
}
}
}
// Strip constraints from releases result
Expand Down
6 changes: 6 additions & 0 deletions lib/modules/datasource/packagist/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -133,10 +133,16 @@ export class PackagistDatasource extends Datasource {
if (release.source?.url) {
dep.sourceUrl = release.source.url;
}
const constraints: Record<string, string[]> = {};
if (release.require?.php) {
constraints.php = [release.require.php];
}

return {
version: parsedVersion.replace(regEx(/^v/), ''),
gitRef: parsedVersion,
releaseTimestamp: release.time,
constraints,
};
});
return dep;
Expand Down
17 changes: 15 additions & 2 deletions lib/modules/datasource/packagist/schema.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@ describe('modules/datasource/packagist/schema', () => {
homepage: null,
source: null,
time: null,
require: null,
};

expect(ComposerRelease.parse({ version: '1.2.3' })).toEqual(
Expand Down Expand Up @@ -66,7 +67,13 @@ describe('modules/datasource/packagist/schema', () => {
expect(ComposerReleaseArray.parse([1, 2, 3])).toEqual([]);
expect(ComposerReleaseArray.parse(['foobar'])).toEqual([]);
expect(ComposerReleaseArray.parse([{ version: '1.2.3' }])).toEqual([
{ version: '1.2.3', homepage: null, source: null, time: null },
{
version: '1.2.3',
homepage: null,
source: null,
time: null,
require: null,
},
]);
});
});
Expand All @@ -85,7 +92,13 @@ describe('modules/datasource/packagist/schema', () => {
},
})
).toEqual([
{ version: '1.2.3', homepage: null, source: null, time: null },
{
version: '1.2.3',
homepage: null,
source: null,
time: null,
require: null,
},
]);
});
});
Expand Down
8 changes: 8 additions & 0 deletions lib/modules/datasource/packagist/schema.ts
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@ export const ComposerRelease = z.object({
.transform((x) => x.url)
.nullable()
.catch(null),
require: z.object({ php: z.string() }).nullable().catch(null),
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
require: z.object({ php: z.string() }).nullable().catch(null),
require:
.object({
php: z.string(),
})
.transform((x) => x.php)
.nullable()
.catch(null),

time: z.string().nullable().catch(null),
});

Expand Down Expand Up @@ -59,6 +60,7 @@ export function parsePackagesResponses(

for (const packagesResponse of packagesResponses) {
const releaseArray = parsePackagesResponse(packageName, packagesResponse);
let phpConstraint: string | undefined;
for (const composerRelease of releaseArray) {
const version = composerRelease.version.replace(/^v/, '');
const gitRef = composerRelease.version;
Expand All @@ -69,6 +71,12 @@ export function parsePackagesResponses(
dep.releaseTimestamp = composerRelease.time;
}

phpConstraint ??= composerRelease.require?.php;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
phpConstraint ??= composerRelease.require?.php;
phpConstraint ??= composerRelease.require;

Copy link
Collaborator

Choose a reason for hiding this comment

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

Since we care for just single field inside nested structure, we transform it up for simpler retrieval.


if (phpConstraint) {
dep.constraints = { php: [phpConstraint] };
}

releases.push(dep);

if (!maxVersion || versioning.isGreaterThan(version, maxVersion)) {
Expand Down