You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@maven.apache.org by GitBox <gi...@apache.org> on 2021/02/26 07:26:47 UTC
[GitHub] [maven] hboutemy opened a new pull request #451: [MNG-7107] relax profile id validation, different from coordinate id
hboutemy opened a new pull request #451:
URL: https://github.com/apache/maven/pull/451
see https://issues.apache.org/jira/browse/MNG-7107
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven] rfscholte commented on a change in pull request #451: [MNG-7107] relax profile id validation, different from coordinate id
Posted by GitBox <gi...@apache.org>.
rfscholte commented on a change in pull request #451:
URL: https://github.com/apache/maven/pull/451#discussion_r583624081
##########
File path: maven-model-builder/src/main/java/org/apache/maven/model/validation/DefaultModelValidator.java
##########
@@ -916,7 +958,7 @@ private boolean isValidIdWithWildCards( String id )
private boolean isValidIdWithWildCardCharacter( char c )
Review comment:
This methodname is now weird: we're going from an general `isValidIdWithWildCardCharacter` to a more explicit `isValidCoordinateIdCharacter`.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven] michael-o edited a comment on pull request #451: [MNG-7107] relax profile id validation, different from coordinate id
Posted by GitBox <gi...@apache.org>.
michael-o edited a comment on pull request #451:
URL: https://github.com/apache/maven/pull/451#issuecomment-786917698
@hboutemy Why didn't you wait for me to test? This one is incomplete. It misses a handling for these:
```
diff --git a/maven-model-builder/src/main/java/org/apache/maven/model/validation/DefaultModelValidator.java b/maven-model-builder/src/main/java/org/apache/maven/model/validation/DefaultModelValidator.java
index 2e71520d1..f3e686851 100644
--- a/maven-model-builder/src/main/java/org/apache/maven/model/validation/DefaultModelValidator.java
+++ b/maven-model-builder/src/main/java/org/apache/maven/model/validation/DefaultModelValidator.java
@@ -921,7 +921,7 @@ private boolean isValidProfileId( String id )
return false;
default:
}
- return true;
+ return !( id.contains( "," ) || id.contains( " " ) );
}
@SuppressWarnings( "checkstyle:parameternumber" )
diff --git a/maven-model-builder/src/test/java/org/apache/maven/model/validation/DefaultModelValidatorTest.java b/maven-model-builder/src/test/java/org/apache/maven/model/validation/DefaultModelValidatorTest.java
index 5de48c4f2..7645b16d5 100644
--- a/maven-model-builder/src/test/java/org/apache/maven/model/validation/DefaultModelValidatorTest.java
+++ b/maven-model-builder/src/test/java/org/apache/maven/model/validation/DefaultModelValidatorTest.java
@@ -409,12 +409,17 @@ public void testInvalidProfileId()
{
SimpleProblemCollector result = validateRaw( "invalid-profile-ids.xml" );
- assertViolations( result, 0, 4, 0 );
+ assertViolations( result, 0, 9, 0 );
assertTrue( result.getErrors().get( 0 ).contains( "+invalid-id" ) );
assertTrue( result.getErrors().get( 1 ).contains( "-invalid-id" ) );
assertTrue( result.getErrors().get( 2 ).contains( "!invalid-id" ) );
assertTrue( result.getErrors().get( 3 ).contains( "?invalid-id" ) );
+ assertTrue( result.getErrors().get( 4 ).contains( "-?invalid-id" ) );
+ assertTrue( result.getErrors().get( 5 ).contains( "+?invalid-id" ) );
+ assertTrue( result.getErrors().get( 6 ).contains( "!?invalid-id" ) );
+ assertTrue( result.getErrors().get( 7 ).contains( "invalid,id" ) );
+ assertTrue( result.getErrors().get( 8 ).contains( "invalid id" ) );
}
public void testDuplicateProfileId()
diff --git a/maven-model-builder/src/test/resources/poms/validation/invalid-profile-ids.xml b/maven-model-builder/src/test/resources/poms/validation/invalid-profile-ids.xml
index 74b670b9d..7419fd83d 100644
--- a/maven-model-builder/src/test/resources/poms/validation/invalid-profile-ids.xml
+++ b/maven-model-builder/src/test/resources/poms/validation/invalid-profile-ids.xml
@@ -37,6 +37,21 @@ under the License.
<profile>
<id>?invalid-id</id>
</profile>
+ <profile>
+ <id>-?invalid-id</id>
+ </profile>
+ <profile>
+ <id>+?invalid-id</id>
+ </profile>
+ <profile>
+ <id>!?invalid-id</id>
+ </profile>
+ <profile>
+ <id>invalid,id</id>
+ </profile>
+ <profile>
+ <id>invalid id</id>
+ </profile>
<profile>
<id>valid-id</id>
</profile>
```
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven] rfscholte commented on a change in pull request #451: [MNG-7107] relax profile id validation, different from coordinate id
Posted by GitBox <gi...@apache.org>.
rfscholte commented on a change in pull request #451:
URL: https://github.com/apache/maven/pull/451#discussion_r583625188
##########
File path: maven-model-builder/src/main/java/org/apache/maven/model/validation/DefaultModelValidator.java
##########
@@ -850,36 +853,75 @@ private boolean validateId( String prefix, String fieldName, ModelProblemCollect
}
else
{
- if ( !isValidId( id ) )
+ if ( !isValidCoordinateId( id ) )
{
addViolation( problems, severity, version, prefix + fieldName, sourceHint,
- "with value '" + id + "' does not match a valid id pattern.", tracker );
+ "with value '" + id + "' does not match a valid coordinate id pattern.", tracker );
return false;
}
- validIds.add( id );
+ validCoordinateIds.add( id );
return true;
}
}
- private boolean isValidId( String id )
+ private boolean isValidCoordinateId( String id )
{
for ( int i = 0; i < id.length(); i++ )
{
char c = id.charAt( i );
- if ( !isValidIdCharacter( c ) )
+ if ( !isValidCoordinateIdCharacter( c ) )
{
return false;
}
}
return true;
}
-
- private boolean isValidIdCharacter( char c )
+ private boolean isValidCoordinateIdCharacter( char c )
{
return c >= 'a' && c <= 'z' || c >= 'A' && c <= 'Z' || c >= '0' && c <= '9' || c == '-' || c == '_' || c == '.';
}
+ @SuppressWarnings( "checkstyle:parameternumber" )
+ private boolean validateProfileId( String prefix, String fieldName, ModelProblemCollector problems,
+ Severity severity, Version version, String id, String sourceHint,
+ InputLocationTracker tracker )
+ {
+ if ( validProfileIds.contains( id ) )
+ {
+ return true;
+ }
+ if ( !validateStringNotEmpty( prefix, fieldName, problems, severity, version, id, sourceHint, tracker ) )
+ {
+ return false;
+ }
+ else
+ {
+ if ( !isValidProfileId( id ) )
+ {
+ addViolation( problems, severity, version, prefix + fieldName, sourceHint,
+ "with value '" + id + "' does not match a valid profile id pattern.", tracker );
+ return false;
+ }
+ validProfileIds.add( id );
+ return true;
+ }
+ }
+
+ private boolean isValidProfileId( String id )
+ {
+ switch ( id.charAt( 0 ) )
+ { // avoid first character that has special CLI meaning in "mvn -P xxx"
Review comment:
IMO a profileId should never contain a comma or a space
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven] Tibor17 commented on pull request #451: [MNG-7107] relax profile id validation, different from coordinate id
Posted by GitBox <gi...@apache.org>.
Tibor17 commented on pull request #451:
URL: https://github.com/apache/maven/pull/451#issuecomment-786499982
As I have understood one old feature request that this pattern `-my-profile` would exclude the profile `my-profile`.
So I understood that the minus is part of the expression. Similar to the old expression `!my-profile`.
If `-` and `!` are the reserved characters, then other characters can be used, IMHO. WDYT?
Of course the limitation might be imposed to the first character and not the other, in this case. WDYT?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven] hboutemy commented on a change in pull request #451: [MNG-7107] relax profile id validation, different from coordinate id
Posted by GitBox <gi...@apache.org>.
hboutemy commented on a change in pull request #451:
URL: https://github.com/apache/maven/pull/451#discussion_r583782873
##########
File path: maven-model-builder/src/main/java/org/apache/maven/model/validation/DefaultModelValidator.java
##########
@@ -850,36 +853,75 @@ private boolean validateId( String prefix, String fieldName, ModelProblemCollect
}
else
{
- if ( !isValidId( id ) )
+ if ( !isValidCoordinateId( id ) )
{
addViolation( problems, severity, version, prefix + fieldName, sourceHint,
- "with value '" + id + "' does not match a valid id pattern.", tracker );
+ "with value '" + id + "' does not match a valid coordinate id pattern.", tracker );
return false;
}
- validIds.add( id );
+ validCoordinateIds.add( id );
return true;
}
}
- private boolean isValidId( String id )
+ private boolean isValidCoordinateId( String id )
{
for ( int i = 0; i < id.length(); i++ )
{
char c = id.charAt( i );
- if ( !isValidIdCharacter( c ) )
+ if ( !isValidCoordinateIdCharacter( c ) )
{
return false;
}
}
return true;
}
-
- private boolean isValidIdCharacter( char c )
+ private boolean isValidCoordinateIdCharacter( char c )
{
return c >= 'a' && c <= 'z' || c >= 'A' && c <= 'Z' || c >= '0' && c <= '9' || c == '-' || c == '_' || c == '.';
}
+ @SuppressWarnings( "checkstyle:parameternumber" )
+ private boolean validateProfileId( String prefix, String fieldName, ModelProblemCollector problems,
+ Severity severity, Version version, String id, String sourceHint,
+ InputLocationTracker tracker )
+ {
+ if ( validProfileIds.contains( id ) )
+ {
+ return true;
+ }
+ if ( !validateStringNotEmpty( prefix, fieldName, problems, severity, version, id, sourceHint, tracker ) )
+ {
+ return false;
+ }
+ else
+ {
+ if ( !isValidProfileId( id ) )
+ {
+ addViolation( problems, severity, version, prefix + fieldName, sourceHint,
+ "with value '" + id + "' does not match a valid profile id pattern.", tracker );
+ return false;
+ }
+ validProfileIds.add( id );
+ return true;
+ }
+ }
+
+ private boolean isValidProfileId( String id )
+ {
+ switch ( id.charAt( 0 ) )
+ { // avoid first character that has special CLI meaning in "mvn -P xxx"
Review comment:
yes, there are good ideas during the discussion on improvements for profile and coordinate id checks: I'll let that as improvements "up for grabs": I needed early fix to stop failing on "jdk9+" profile id
Now I'll let improvement ideas go at their own pace
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven] rfscholte commented on a change in pull request #451: [MNG-7107] relax profile id validation, different from coordinate id
Posted by GitBox <gi...@apache.org>.
rfscholte commented on a change in pull request #451:
URL: https://github.com/apache/maven/pull/451#discussion_r583625188
##########
File path: maven-model-builder/src/main/java/org/apache/maven/model/validation/DefaultModelValidator.java
##########
@@ -850,36 +853,75 @@ private boolean validateId( String prefix, String fieldName, ModelProblemCollect
}
else
{
- if ( !isValidId( id ) )
+ if ( !isValidCoordinateId( id ) )
{
addViolation( problems, severity, version, prefix + fieldName, sourceHint,
- "with value '" + id + "' does not match a valid id pattern.", tracker );
+ "with value '" + id + "' does not match a valid coordinate id pattern.", tracker );
return false;
}
- validIds.add( id );
+ validCoordinateIds.add( id );
return true;
}
}
- private boolean isValidId( String id )
+ private boolean isValidCoordinateId( String id )
{
for ( int i = 0; i < id.length(); i++ )
{
char c = id.charAt( i );
- if ( !isValidIdCharacter( c ) )
+ if ( !isValidCoordinateIdCharacter( c ) )
{
return false;
}
}
return true;
}
-
- private boolean isValidIdCharacter( char c )
+ private boolean isValidCoordinateIdCharacter( char c )
{
return c >= 'a' && c <= 'z' || c >= 'A' && c <= 'Z' || c >= '0' && c <= '9' || c == '-' || c == '_' || c == '.';
}
+ @SuppressWarnings( "checkstyle:parameternumber" )
+ private boolean validateProfileId( String prefix, String fieldName, ModelProblemCollector problems,
+ Severity severity, Version version, String id, String sourceHint,
+ InputLocationTracker tracker )
+ {
+ if ( validProfileIds.contains( id ) )
+ {
+ return true;
+ }
+ if ( !validateStringNotEmpty( prefix, fieldName, problems, severity, version, id, sourceHint, tracker ) )
+ {
+ return false;
+ }
+ else
+ {
+ if ( !isValidProfileId( id ) )
+ {
+ addViolation( problems, severity, version, prefix + fieldName, sourceHint,
+ "with value '" + id + "' does not match a valid profile id pattern.", tracker );
+ return false;
+ }
+ validProfileIds.add( id );
+ return true;
+ }
+ }
+
+ private boolean isValidProfileId( String id )
+ {
+ switch ( id.charAt( 0 ) )
+ { // avoid first character that has special CLI meaning in "mvn -P xxx"
Review comment:
IMO a profileId should never contain a comma or a space, as it would confuse the commandline
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven] hboutemy edited a comment on pull request #451: [MNG-7107] relax profile id validation, different from coordinate id
Posted by GitBox <gi...@apache.org>.
hboutemy edited a comment on pull request #451:
URL: https://github.com/apache/maven/pull/451#issuecomment-786778197
> It would be worth for future maintenance to create a new class checking these characters in profile id because both classes DefaultModelValidator and MavenCli are chcecking the first (and second character ?) character, so it would worth to change the algorithm in one place instead of two.
I understand the general idea, but IMHO, this would create more complexity (with dependency tree between artifacts, and abstraction) than real benefit.
But don't hesitate to work on this later if you really think there is a benefit
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven] hboutemy commented on pull request #451: [MNG-7107] relax profile id validation, different from coordinate id
Posted by GitBox <gi...@apache.org>.
hboutemy commented on pull request #451:
URL: https://github.com/apache/maven/pull/451#issuecomment-787015632
@Tibor17 don't hesitate to implement, I'm eager to see the relationship between maven-model-builder and maven-embedder
@michael-o sorry, I had already much feedback, and my intent was not to implement any and all validations but just relax a too hard check that was in master and made current HEAD unusable
don't hesitate to add even more checks if you want in a further issue: I'm not against by don't have time for that
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven] michael-o commented on pull request #451: [MNG-7107] relax profile id validation, different from coordinate id
Posted by GitBox <gi...@apache.org>.
michael-o commented on pull request #451:
URL: https://github.com/apache/maven/pull/451#issuecomment-786568088
I will test your change later this day...
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven] Tibor17 edited a comment on pull request #451: [MNG-7107] relax profile id validation, different from coordinate id
Posted by GitBox <gi...@apache.org>.
Tibor17 edited a comment on pull request #451:
URL: https://github.com/apache/maven/pull/451#issuecomment-786791885
There does not need to be an abstraction. Two methods in one class which means that the developer is not able to skip the first method if modifying another one.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven] Tibor17 commented on pull request #451: [MNG-7107] relax profile id validation, different from coordinate id
Posted by GitBox <gi...@apache.org>.
Tibor17 commented on pull request #451:
URL: https://github.com/apache/maven/pull/451#issuecomment-786506325
@michael-o
You mean something like this?
`-P ?+my-profile` or `-P +my-profile`, is it an expression? If it is, what kind of meaning?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven] Tibor17 commented on pull request #451: [MNG-7107] relax profile id validation, different from coordinate id
Posted by GitBox <gi...@apache.org>.
Tibor17 commented on pull request #451:
URL: https://github.com/apache/maven/pull/451#issuecomment-786791885
There does not need to be an abstraction. Two methods in one class which means that the developer is not able to change the first method if modifying another one.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven] michael-o commented on pull request #451: [MNG-7107] relax profile id validation, different from coordinate id
Posted by GitBox <gi...@apache.org>.
michael-o commented on pull request #451:
URL: https://github.com/apache/maven/pull/451#issuecomment-786923709
There likely more chars which would be forbidded, e.g., <, >, |, ~, ;, $, %, ", ', [, ], *, \. They all have some meaning either in POSIX shell, bash, PowerShell or Windows cmd. Likely a whilelist after the first/second char would make more sense!
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven] hboutemy commented on a change in pull request #451: [MNG-7107] relax profile id validation, different from coordinate id
Posted by GitBox <gi...@apache.org>.
hboutemy commented on a change in pull request #451:
URL: https://github.com/apache/maven/pull/451#discussion_r583783212
##########
File path: maven-model-builder/src/main/java/org/apache/maven/model/validation/DefaultModelValidator.java
##########
@@ -916,7 +958,7 @@ private boolean isValidIdWithWildCards( String id )
private boolean isValidIdWithWildCardCharacter( char c )
Review comment:
ok, I'll rename this method also
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven] michael-o commented on pull request #451: [MNG-7107] relax profile id validation, different from coordinate id
Posted by GitBox <gi...@apache.org>.
michael-o commented on pull request #451:
URL: https://github.com/apache/maven/pull/451#issuecomment-786917698
@hboutemy Why didn't you wait for me to test? This one is incomplete. It misses a handling for these:
```
diff --git a/maven-model-builder/src/test/resources/poms/validation/invalid-profile-ids.xml b/maven-model-builder/src/test/resources/poms/validation/invalid-profile-ids.xml
index 74b670b9d..7419fd83d 100644
--- a/maven-model-builder/src/test/resources/poms/validation/invalid-profile-ids.xml
+++ b/maven-model-builder/src/test/resources/poms/validation/invalid-profile-ids.xml
@@ -37,6 +37,21 @@ under the License.
<profile>
<id>?invalid-id</id>
</profile>
+ <profile>
+ <id>-?invalid-id</id>
+ </profile>
+ <profile>
+ <id>+?invalid-id</id>
+ </profile>
+ <profile>
+ <id>!?invalid-id</id>
+ </profile>
+ <profile>
+ <id>invalid,id</id>
+ </profile>
+ <profile>
+ <id>invalid id</id>
+ </profile>
<profile>
<id>valid-id</id>
</profile>
```
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven] michael-o commented on pull request #451: [MNG-7107] relax profile id validation, different from coordinate id
Posted by GitBox <gi...@apache.org>.
michael-o commented on pull request #451:
URL: https://github.com/apache/maven/pull/451#issuecomment-786508314
>
>
> @michael-o
> You mean something like this?
> `-P ?+my-profile` or `-P +my-profile`, is it an expression? If it is, what kind of meaning?
Yes and both are valid iinput. In both cases the profile name is `my-profile`.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven] hboutemy commented on pull request #451: [MNG-7107] relax profile id validation, different from coordinate id
Posted by GitBox <gi...@apache.org>.
hboutemy commented on pull request #451:
URL: https://github.com/apache/maven/pull/451#issuecomment-786549495
I coded the few cases I thought I knew about `-P` in `mvn` command line
but it seems I don't know what `?` means: I don't see it in code https://github.com/apache/maven/blob/master/maven-embedder/src/main/java/org/apache/maven/cli/MavenCli.java#L1475
and I missed `!`, that IIUC seems to be a synonym to `-`
I'll add `!` as excluded first character, but for `?`, I'll need some explanations as why we should not accept that char
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven] michael-o commented on pull request #451: [MNG-7107] relax profile id validation, different from coordinate id
Posted by GitBox <gi...@apache.org>.
michael-o commented on pull request #451:
URL: https://github.com/apache/maven/pull/451#issuecomment-786504825
I would still ike to have clarified for `?, `-`, `+`, and `!`.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven] michael-o commented on a change in pull request #451: [MNG-7107] relax profile id validation, different from coordinate id
Posted by GitBox <gi...@apache.org>.
michael-o commented on a change in pull request #451:
URL: https://github.com/apache/maven/pull/451#discussion_r583942707
##########
File path: maven-model-builder/src/main/java/org/apache/maven/model/validation/DefaultModelValidator.java
##########
@@ -850,36 +853,75 @@ private boolean validateId( String prefix, String fieldName, ModelProblemCollect
}
else
{
- if ( !isValidId( id ) )
+ if ( !isValidCoordinateId( id ) )
{
addViolation( problems, severity, version, prefix + fieldName, sourceHint,
- "with value '" + id + "' does not match a valid id pattern.", tracker );
+ "with value '" + id + "' does not match a valid coordinate id pattern.", tracker );
return false;
}
- validIds.add( id );
+ validCoordinateIds.add( id );
return true;
}
}
- private boolean isValidId( String id )
+ private boolean isValidCoordinateId( String id )
{
for ( int i = 0; i < id.length(); i++ )
{
char c = id.charAt( i );
- if ( !isValidIdCharacter( c ) )
+ if ( !isValidCoordinateIdCharacter( c ) )
{
return false;
}
}
return true;
}
-
- private boolean isValidIdCharacter( char c )
+ private boolean isValidCoordinateIdCharacter( char c )
{
return c >= 'a' && c <= 'z' || c >= 'A' && c <= 'Z' || c >= '0' && c <= '9' || c == '-' || c == '_' || c == '.';
}
+ @SuppressWarnings( "checkstyle:parameternumber" )
+ private boolean validateProfileId( String prefix, String fieldName, ModelProblemCollector problems,
+ Severity severity, Version version, String id, String sourceHint,
+ InputLocationTracker tracker )
+ {
+ if ( validProfileIds.contains( id ) )
+ {
+ return true;
+ }
+ if ( !validateStringNotEmpty( prefix, fieldName, problems, severity, version, id, sourceHint, tracker ) )
+ {
+ return false;
+ }
+ else
+ {
+ if ( !isValidProfileId( id ) )
+ {
+ addViolation( problems, severity, version, prefix + fieldName, sourceHint,
+ "with value '" + id + "' does not match a valid profile id pattern.", tracker );
+ return false;
+ }
+ validProfileIds.add( id );
+ return true;
+ }
+ }
+
+ private boolean isValidProfileId( String id )
+ {
+ switch ( id.charAt( 0 ) )
+ { // avoid first character that has special CLI meaning in "mvn -P xxx"
Review comment:
I don't understand the urgent need for the `jdk9+`. This is alpha code, why did you put this into production?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven] Tibor17 commented on pull request #451: [MNG-7107] relax profile id validation, different from coordinate id
Posted by GitBox <gi...@apache.org>.
Tibor17 commented on pull request #451:
URL: https://github.com/apache/maven/pull/451#issuecomment-786574176
It would be worth for future maintenance to create a new class checking these characters in profile id because both classes `DefaultModelValidator` and `MavenCli` are chcecking the first (and second character `?`) character, so it would worth to change the algorithm in one place instead of two.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven] michael-o commented on pull request #451: [MNG-7107] relax profile id validation, different from coordinate id
Posted by GitBox <gi...@apache.org>.
michael-o commented on pull request #451:
URL: https://github.com/apache/maven/pull/451#issuecomment-786551240
A recent change make profile activatoins mandatory, unless it is prefixed with `?`. See https://github.com/apache/maven/commit/8defd169651db30fe0ac3ad3f318e471f0241d02.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven] Tibor17 commented on pull request #451: [MNG-7107] relax profile id validation, different from coordinate id
Posted by GitBox <gi...@apache.org>.
Tibor17 commented on pull request #451:
URL: https://github.com/apache/maven/pull/451#issuecomment-786503191
I think this implementation covers my requirement mentioned in Jira.
Go ahead with the commit.
Feel free to add a test into it.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven] asfgit merged pull request #451: [MNG-7107] relax profile id validation, different from coordinate id
Posted by GitBox <gi...@apache.org>.
asfgit merged pull request #451:
URL: https://github.com/apache/maven/pull/451
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven] hboutemy commented on pull request #451: [MNG-7107] relax profile id validation, different from coordinate id
Posted by GitBox <gi...@apache.org>.
hboutemy commented on pull request #451:
URL: https://github.com/apache/maven/pull/451#issuecomment-786778197
bq. It would be worth for future maintenance to create a new class checking these characters in profile id because both classes DefaultModelValidator and MavenCli are chcecking the first (and second character ?) character, so it would worth to change the algorithm in one place instead of two.
I understand the general idea, but IMHO, this would create more complexity (with dependency tree between artifacts, and abstraction) than real benefit.
But don't hesitate to work on this later if you really think there is a benefit
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven] hboutemy commented on pull request #451: [MNG-7107] relax profile id validation, different from coordinate id
Posted by GitBox <gi...@apache.org>.
hboutemy commented on pull request #451:
URL: https://github.com/apache/maven/pull/451#issuecomment-786552743
ok, I found "optional profile activation" https://github.com/apache/maven/blob/master/maven-embedder/src/main/java/org/apache/maven/cli/MavenCli.java#L1515
IMHO, this will require some reference documentation in https://maven.apache.org/ref/current/maven-embedder/
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org