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

Gradle bump strategy #33453

Open
wants to merge 5 commits into
base: main
Choose a base branch
from
Open

Gradle bump strategy #33453

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
e961b3e
added range support to parser
ldt-fweichert Jan 7, 2025
2e43ff0
added versioning support
ldt-fweichert Jan 7, 2025
f1d94c9
fixed some stuff
ldt-fweichert Jan 7, 2025
ffa0f59
Merge branch 'main' into gradle-version-ranges
ldt-fweichert Jan 7, 2025
36c766c
removed unused variable
ldt-fweichert Jan 7, 2025
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
15 changes: 9 additions & 6 deletions lib/modules/manager/gradle/parser.spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -354,12 +354,15 @@ describe('modules/manager/gradle/parser', () => {
describe('dependencies', () => {
describe('simple dependency strings', () => {
it.each`
input | output
${'"foo:bar:1.2.3"'} | ${{ depName: 'foo:bar', currentValue: '1.2.3' }}
${'"foo:bar:1.2.3@zip"'} | ${{ depName: 'foo:bar', currentValue: '1.2.3', dataType: 'zip' }}
${'"foo:bar1:1"'} | ${{ depName: 'foo:bar1', currentValue: '1', managerData: { fileReplacePosition: 10 } }}
${'"foo:bar:x86@x86"'} | ${{ depName: 'foo:bar', currentValue: 'x86', managerData: { fileReplacePosition: 9 } }}
${'foo.bar = "foo:bar:1.2.3"'} | ${{ depName: 'foo:bar', currentValue: '1.2.3' }}
input | output
${'"foo:bar:1.2.3"'} | ${{ depName: 'foo:bar', currentValue: '1.2.3' }}
${'"foo:bar:1.2.3@zip"'} | ${{ depName: 'foo:bar', currentValue: '1.2.3', dataType: 'zip' }}
${'"foo:bar1:1"'} | ${{ depName: 'foo:bar1', currentValue: '1', managerData: { fileReplacePosition: 10 } }}
${'"foo:bar:[1.2.3, )!!1.2.3"'} | ${{ depName: 'foo:bar', currentValue: '1.2.3', managerData: { fileReplacePosition: 9 } }}
${'"foo:bar:[1.2.3, 1.2.4)"'} | ${{ depName: 'foo:bar', currentValue: '1.2.4', managerData: { fileReplacePosition: 9 } }}
${'"foo:bar:[,1.2.4)"'} | ${{ depName: 'foo:bar', currentValue: '1.2.4', managerData: { fileReplacePosition: 9 } }}
${'"foo:bar:x86@x86"'} | ${{ depName: 'foo:bar', currentValue: 'x86', managerData: { fileReplacePosition: 9 } }}
${'foo.bar = "foo:bar:1.2.3"'} | ${{ depName: 'foo:bar', currentValue: '1.2.3' }}
`('$input', ({ input, output }) => {
const { deps } = parseGradle(input);
expect(deps).toMatchObject([output]);
Expand Down
4 changes: 3 additions & 1 deletion lib/modules/manager/gradle/parser.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -127,11 +127,12 @@ export function parseProps(
): { vars: PackageVariables; deps: PackageDependency<GradleManagerData>[] } {
let offset = 0;
const vars: PackageVariables = {};
const deps: PackageDependency[] = [];
const deps: PackageDependency<GradleManagerData>[] = [];
for (const line of input.split(newlineRegex)) {
const lineMatch = propRegex.exec(line);
if (lineMatch?.groups) {
const { key, value, leftPart } = lineMatch.groups;
// TODO: we dont need to check isDependency, the parseDependency does this by itself
Copy link
Collaborator

Choose a reason for hiding this comment

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

That's correct but please keep your changes strictly limited to what's relevant for the functionality you'd like to add. Refactoring can be done separately.

if (isDependencyString(value)) {
const dep = parseDependencyString(value);
if (dep) {
Expand All @@ -145,6 +146,7 @@ export function parseProps(
});
}
} else {
const { key, value, leftPart } = lineMatch.groups;
vars[key] = {
key,
value,
Expand Down
58 changes: 41 additions & 17 deletions lib/modules/manager/gradle/utils.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,22 +1,20 @@
import upath from 'upath';
import { regEx } from '../../../util/regex';
import type { PackageDependency } from '../types';
import type {
GradleManagerData,
PackageVariables,
VariableRegistry,
} from './types';
import type { GradleManagerData, PackageVariables, VariableRegistry } from './types';

const artifactRegex = regEx(
'^[a-zA-Z][-_a-zA-Z0-9]*(?:\\.[a-zA-Z0-9][-_a-zA-Z0-9]*?)*$',
'^[a-zA-Z][-_a-zA-Z0-9]*(?:\\.[a-zA-Z0-9][-_a-zA-Z0-9]*?)*$'
);

const versionLikeRegex = regEx('^(?<version>[-_.\\[\\](),a-zA-Z0-9+]+)');
const versionLikeRegex = regEx('^(?<version>[-_.\\[\\](),a-zA-Z0-9+! ]*[-_.\\[\\](),a-zA-Z0-9+])');
Copy link
Collaborator

Choose a reason for hiding this comment

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

What's the intention here? Is it an attempt to add ! and a space to the list of allowed characters? If yes, this can be done simpler


const mavenStyleRangeRegex = regEx(/^[[\](](?<lowerBound>[-._+a-zA-Z0-9]+)?\s*,\s*(?<upperBound>[-._+a-zA-Z0-9]+)?[[\])](?:!!>[-._+a-zA-Z0-9]+)?$/)

// Extracts version-like and range-like strings
// from the beginning of input
export function versionLikeSubstring(
input: string | null | undefined,
input: string | null | undefined
): string | null {
if (!input) {
return null;
Expand All @@ -31,6 +29,17 @@ export function versionLikeSubstring(
return version;
}

export function parseMavenStyleRange(input: string): {
lowerBound: string | null, upperBound: string | null, strict: string | null
} | null {
const match = mavenStyleRangeRegex.exec(input);
const lowerBound = match?.groups?.lower_bound ?? null;
const upperBound = match?.groups?.upper_bound ?? null;
const strict = match?.groups?.strict ?? null;
if (!lowerBound && !upperBound) {return null;}
return { lowerBound, upperBound, strict };
}

export function isDependencyString(input: string): boolean {
const split = input?.split(':');
if (split?.length !== 3 && split?.length !== 4) {
Expand All @@ -44,6 +53,11 @@ export function isDependencyString(input: string): boolean {
return false;
}

const mavenStyleRange = parseMavenStyleRange(tempVersionPart);
Copy link
Collaborator

Choose a reason for hiding this comment

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

I see no need for this here. Should be enough to have versionLikeSubstring match ranges, which it already can except for those containing ! and spaces.

if (mavenStyleRange) {
return true;
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is wrong as it bypasses all subsequent checks

}

if (
tempVersionPart !== versionLikeSubstring(tempVersionPart) &&
tempVersionPart.includes('@')
Expand All @@ -57,7 +71,7 @@ export function isDependencyString(input: string): boolean {
const [groupId, artifactId, versionPart] = [
tempGroupId,
tempArtifactId,
tempVersionPart,
tempVersionPart
];
return !!(
groupId &&
Expand All @@ -70,23 +84,33 @@ export function isDependencyString(input: string): boolean {
}

export function parseDependencyString(
input: string,
input: string
): PackageDependency<GradleManagerData> | null {
if (!isDependencyString(input)) {
return null;
}
const [groupId, artifactId, FullValue] = input.split(':');
if (FullValue === versionLikeSubstring(FullValue)) {
const [groupId, artifactId, fullValue] = input.split(':');
if (fullValue === versionLikeSubstring(fullValue)) {

const mavenStyleRange = parseMavenStyleRange(fullValue);
if (mavenStyleRange) {
const updatablePart = mavenStyleRange.upperBound ?? mavenStyleRange.lowerBound;
Copy link
Collaborator

Choose a reason for hiding this comment

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

I guess ?? mavenStyleRange.lowerBound can never hapepns since parseMavenStyleRange returns null if not both lower and upper are provided?!

return {
depName: `${groupId}:${artifactId}`,
currentValue: updatablePart,
Copy link
Collaborator

Choose a reason for hiding this comment

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

currentValue is supposed to include the version value found for the dependency that is extracted. It's not the right place to decide whether to update the lower or upper bound. I think updating the lower bound is a bit pointless overall. Such a check can be done later while comparing update candidates with the full range (and the comparison logic in the versioning).

The only thing that may need adaptation here is the addition of ! and a space to the version regex.

}
}

return {
depName: `${groupId}:${artifactId}`,
currentValue: FullValue,
currentValue: fullValue
};
}
const [currentValue, dataType] = FullValue.split('@');
const [currentValue, dataType] = fullValue.split('@');
return {
depName: `${groupId}:${artifactId}`,
currentValue,
dataType,
dataType
};
}

Expand Down Expand Up @@ -176,7 +200,7 @@ export function reorderFiles(packageFiles: string[]): string[] {
export function getVars(
registry: VariableRegistry,
dir: string,
vars: PackageVariables = registry[dir] || {},
vars: PackageVariables = registry[dir] || {}
): PackageVariables {
const dirAbs = toAbsolutePath(dir);
const parentDir = upath.dirname(dirAbs);
Expand All @@ -190,7 +214,7 @@ export function getVars(
export function updateVars(
registry: VariableRegistry,
dir: string,
newVars: PackageVariables,
newVars: PackageVariables
): void {
const oldVars = registry[dir] ?? {};
registry[dir] = { ...oldVars, ...newVars };
Expand Down
78 changes: 78 additions & 0 deletions lib/modules/versioning/gradle/compare.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -253,6 +253,84 @@ export function parsePrefixRange(input: string): PrefixRange | null {
return null;
}

export type MavenStyleRange = {
lowerBound: MavenStyleRangeBound,
upperBound: MavenStyleRangeBound,
preferredVersion: string | null,
seperator: string,
}

export type MavenStyleRangeBound = {
version: string | null,
type: 'inclusive' | 'exclusive',
bracket: string,
}

const mavenStyleRangeRegex = regEx(/^(?<openingBracket>[[\](])(?<lowerBound>[-._+a-zA-Z0-9]+)?(?<seperator>\s*,\s*)(?<upperBound>[-._+a-zA-Z0-9]+)?(?<closingBracket>[[\])])(?:!!(?<preferredVersion>[-._+a-zA-Z0-9]+))?$/)

export function parseMavenStyleRange(input: string): MavenStyleRange | null {
Copy link
Collaborator

Choose a reason for hiding this comment

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

There already is mavenBasedRangeRegex and parseMavenBasedRange, which does quite the same, except that it lacks support for preferred versions. What speaks against extending the existing implementation instead of reinventing maven style range parsing as a whole?

const matchGroups = mavenStyleRangeRegex.exec(input)?.groups;
if (!matchGroups) {return null;}
const lowerVersion = matchGroups.lowerBound ?? null;
const upperVersion = matchGroups.upperBound ?? null;
const preferredVersion = matchGroups?.preferredVersion ?? null;

const validVersion = (version: string | null): boolean => version === null || isVersion(version)
if (!validVersion(lowerVersion) || !validVersion(upperVersion) || !validVersion(preferredVersion)) {return null;}
if (lowerVersion && upperVersion && compare(lowerVersion, upperVersion) === 0) {return null;}

const openingBracket = matchGroups.openingBracket;
const closingBracket = matchGroups.closingBracket;
const lowerBoundType = openingBracket === '[' ? 'inclusive' : 'exclusive';
const upperBoundType = closingBracket === ']' ? 'inclusive' : 'exclusive';
const seperator = matchGroups.seperator;

if (preferredVersion) {
if (lowerVersion) {
const delta = compare(lowerVersion, preferredVersion);
if (upperBoundType === 'inclusive' && delta !== 1) {return null;}
if (upperBoundType === 'exclusive' && delta === -1) {return null;}
}
if (upperVersion) {
const delta = compare(preferredVersion, upperVersion);
if (upperBoundType === 'inclusive' && delta !== 1) {return null;}
if (upperBoundType === 'exclusive' && delta === -1) {return null;}
}
}

return {
lowerBound: {
version: lowerVersion,
type: lowerBoundType,
bracket: openingBracket,
},
upperBound: {
version: upperVersion,
type: upperBoundType,
bracket: closingBracket,
},
preferredVersion,
seperator,
}
}

export function constructMavenStyleRange(range: MavenStyleRange): string {
let temp = '';
temp += range.lowerBound.bracket;
if (range.lowerBound.version) {
temp += range.lowerBound.version
}
temp += range.seperator;
if (range.upperBound.version) {
temp += range.upperBound.version;
}
temp += range.upperBound.bracket;
if (range.preferredVersion) {
temp += `!!${range.preferredVersion}`;
}
return temp;
}

const mavenBasedRangeRegex = regEx(
/^(?<leftBoundStr>[[\](]\s*)(?<leftVal>[-._+a-zA-Z0-9]*?)(?<separator>\s*,\s*)(?<rightVal>[-._+a-zA-Z0-9]*?)(?<rightBoundStr>\s*[[\])])$/,
);
Expand Down
7 changes: 7 additions & 0 deletions lib/modules/versioning/gradle/index.spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -265,6 +265,7 @@ describe('modules/versioning/gradle/index', () => {

it.each`
currentValue | rangeStrategy | currentVersion | newVersion | expected
${'[1.0.0,1.2.3]'} | ${'pin'} | ${'1.0.0'} | ${'1.2.4'} | ${'1.2.4'}
${'1'} | ${null} | ${null} | ${'1.1'} | ${'1.1'}
${'[1.2.3,]'} | ${null} | ${null} | ${'1.2.4'} | ${'[1.2.3,]'}
${'[1.2.3,2)'} | ${null} | ${null} | ${'2.0.0'} | ${'[1.2.3,3)'}
Expand Down Expand Up @@ -293,6 +294,12 @@ describe('modules/versioning/gradle/index', () => {
${'[1.0,1.2],[1.3,1.5['} | ${'pin'} | ${'1.0'} | ${'1.2.4'} | ${'1.2.4'}
${'[1.2.3,)'} | ${'pin'} | ${'1.2.3'} | ${'1.2.4'} | ${'1.2.4'}
${'[1.2.3,['} | ${'pin'} | ${'1.2.3'} | ${'1.2.4'} | ${'1.2.4'}
${'[1.0.0, 1.2.3]'} | ${'bump'} | ${'1.0.0'} | ${'1.2.3'} | ${'[1.2.3, 1.2.3]'}
${'[1.0.0,1.2.23]'} | ${'bump'} | ${'1.0.0'} | ${'1.2.23'} | ${'[1.2.23,1.2.23]'}
${'[1.2.3,)'} | ${'bump'} | ${'1.2.3'} | ${'1.2.4'} | ${'[1.2.4,)'}
${'[1.2.3,['} | ${'bump'} | ${'1.2.3'} | ${'1.2.4'} | ${'[1.2.4,['}
${'[1.2.3,[!!1.2.3'} | ${'bump'} | ${'1.2.3'} | ${'1.2.4'} | ${'[1.2.4,[!!1.2.4'}
${'[4.3.0, )!!4.3.0'} | ${'bump'} | ${'4.3.0'} | ${'4.4.0'} | ${'[4.4.0, )!!4.4.0'}
`(
'getNewValue($currentValue, $rangeStrategy, $currentVersion, $newVersion, $expected) === $expected',
({ currentValue, rangeStrategy, currentVersion, newVersion, expected }) => {
Expand Down
Loading