gnodet-bot commented on code in PR #11904:
URL: https://github.com/apache/maven/pull/11904#discussion_r4329353851
##########
impl/maven-impl/src/main/java/org/apache/maven/impl/model/DefaultModelValidator.java:
##########
@@ -1307,6 +1371,182 @@ private void validate20RawDependencies(
}
}
+ private void validateDependencyIdAttribute(ModelProblemCollector problems,
Dependency dependency, String prefix) {
+ String id = dependency.getId();
+ if (id == null || id.isEmpty()) {
+ return;
+ }
+
+ String remaining = id;
+ boolean hasOptionalMarker = false;
+ if (remaining.endsWith("?")) {
+ hasOptionalMarker = true;
+ remaining = remaining.substring(0, remaining.length() - 1);
+ }
+
+ boolean hasScopeMarker = false;
+ int atIndex = remaining.lastIndexOf('@');
+ if (atIndex >= 0) {
+ hasScopeMarker = true;
+ remaining = remaining.substring(0, atIndex);
+ }
+
+ int colonCount = remaining.length() - remaining.replace(":",
"").length();
+ if (colonCount < 1 || colonCount > 4) {
+ addViolation(
+ problems,
+ Severity.ERROR,
+ Version.V42,
+ prefix + "id",
+ null,
+ "has invalid format '" + id + "', must be
'groupId:artifactId[:]', "
+ + "'groupId:artifactId:version', "
+ + "'groupId:artifactId:type:[version]', or "
+ + "'groupId:artifactId:type:classifier:[version]'.
"
+ + "An optional '@scope' suffix and/or trailing '?'
may be appended.",
+ dependency);
+ return;
+ }
+ if (!isNullOrEmpty(dependency.getGroupId())) {
+ addViolation(
+ problems,
+ Severity.ERROR,
+ Version.V42,
+ prefix + "groupId",
+ null,
+ "must not be specified when 'id' attribute is used.",
+ dependency);
+ }
+ if (!isNullOrEmpty(dependency.getArtifactId())) {
+ addViolation(
+ problems,
+ Severity.ERROR,
+ Version.V42,
+ prefix + "artifactId",
+ null,
+ "must not be specified when 'id' attribute is used.",
+ dependency);
+ }
+ if (!isNullOrEmpty(dependency.getVersion())) {
+ addViolation(
+ problems,
+ Severity.ERROR,
+ Version.V42,
+ prefix + "version",
+ null,
+ "must not be specified when 'id' attribute is used.",
+ dependency);
+ }
+ String[] parts = remaining.split(":", -1);
+ if (parts.length >= 3
+ && !parts[2].isEmpty()
+ && !isNullOrEmpty(dependency.getType())
Review Comment:
**Off-by-one in threshold: `>= 3` should be `>= 4`.**
The `id` format is:
- 3-part: `g:a:version` — position 2 is **version**, not type
- 4-part: `g:a:type:[version]` — position 2 is **type**
- 5-part: `g:a:type:classifier:[version]`
With `parts.length >= 3`, any 3-part id (`g:a:1.0`) will trigger this check:
`parts[2]="1.0"` is non-empty and the id does **not** include a type — so the
error `"type must not be specified when 'id' attribute includes type"` fires
incorrectly when the user adds `<type>pom</type>` to a 3-part id.
The old `colonCount >= 3` was correct because `colonCount` equalled
`parts.length - 1`, so `colonCount >= 3` ↔ `parts.length >= 4`.
```suggestion
if (parts.length >= 4
```
##########
impl/maven-impl/src/main/java/org/apache/maven/impl/model/DefaultModelValidator.java:
##########
@@ -1307,6 +1371,182 @@ private void validate20RawDependencies(
}
}
+ private void validateDependencyIdAttribute(ModelProblemCollector problems,
Dependency dependency, String prefix) {
+ String id = dependency.getId();
+ if (id == null || id.isEmpty()) {
+ return;
+ }
+
+ String remaining = id;
+ boolean hasOptionalMarker = false;
+ if (remaining.endsWith("?")) {
+ hasOptionalMarker = true;
+ remaining = remaining.substring(0, remaining.length() - 1);
+ }
+
+ boolean hasScopeMarker = false;
+ int atIndex = remaining.lastIndexOf('@');
+ if (atIndex >= 0) {
+ hasScopeMarker = true;
+ remaining = remaining.substring(0, atIndex);
+ }
+
+ int colonCount = remaining.length() - remaining.replace(":",
"").length();
+ if (colonCount < 1 || colonCount > 4) {
+ addViolation(
+ problems,
+ Severity.ERROR,
+ Version.V42,
+ prefix + "id",
+ null,
+ "has invalid format '" + id + "', must be
'groupId:artifactId[:]', "
+ + "'groupId:artifactId:version', "
+ + "'groupId:artifactId:type:[version]', or "
+ + "'groupId:artifactId:type:classifier:[version]'.
"
+ + "An optional '@scope' suffix and/or trailing '?'
may be appended.",
+ dependency);
+ return;
+ }
+ if (!isNullOrEmpty(dependency.getGroupId())) {
+ addViolation(
+ problems,
+ Severity.ERROR,
+ Version.V42,
+ prefix + "groupId",
+ null,
+ "must not be specified when 'id' attribute is used.",
+ dependency);
+ }
+ if (!isNullOrEmpty(dependency.getArtifactId())) {
+ addViolation(
+ problems,
+ Severity.ERROR,
+ Version.V42,
+ prefix + "artifactId",
+ null,
+ "must not be specified when 'id' attribute is used.",
+ dependency);
+ }
+ if (!isNullOrEmpty(dependency.getVersion())) {
+ addViolation(
+ problems,
+ Severity.ERROR,
+ Version.V42,
+ prefix + "version",
+ null,
+ "must not be specified when 'id' attribute is used.",
+ dependency);
+ }
+ String[] parts = remaining.split(":", -1);
+ if (parts.length >= 3
+ && !parts[2].isEmpty()
+ && !isNullOrEmpty(dependency.getType())
+ && !"jar".equals(dependency.getType())) {
+ addViolation(
+ problems,
+ Severity.ERROR,
+ Version.V42,
+ prefix + "type",
+ null,
+ "must not be specified when 'id' attribute includes type.",
+ dependency);
+ }
+ if (parts.length >= 4 && !parts[3].isEmpty() &&
!isNullOrEmpty(dependency.getClassifier())) {
Review Comment:
**Off-by-one in threshold: `>= 4` should be `>= 5`.**
A classifier appears at position 3 only in the 5-part format
`g:a:type:classifier:[version]`. In the 4-part format `g:a:type:version`,
position 3 is the **version**.
With `parts.length >= 4`, a 4-part id like `g:a:war:1.0` sets
`parts[3]="1.0"` (version) as non-empty, and the check fires: the user gets
`"classifier must not be specified when 'id' attribute includes classifier"`
even though the id has **no classifier** in it. The old `colonCount >= 4` was
correct for the same reason as above (`colonCount` = `parts.length - 1`).
```suggestion
if (parts.length >= 5 && !parts[3].isEmpty() &&
!isNullOrEmpty(dependency.getClassifier())) {
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
