[MNG-7767] Tone down plugin validator

[cstamas]

This change reduces default output making it more compact, and also renames the "levels" to brief, default and verbose.

---

https://issues.apache.org/jira/browse/MNG-7767

[michael-o]

I am really confused why RepositorySystemSession and ConfigUtils are used here. They feel a bit misplaced in plugin validation, don't they?

[cstamas]

I am really confused why RepositorySystemSession and ConfigUtils are used here. They feel a bit misplaced in plugin validation, don't they?

Well, this PR does not change anything about that. But, as validation is per session, the session is really the only thing that offers itself to store things in "natural" way. MavenSession does not have any means for this, so it is resolver session data that is used. It is done like it as this guarantees that validator does do what it needs to do in correct way in single-threaded and multi-threaded mvn and also in mvnd.

[michael-o]

I am really confused why RepositorySystemSession and ConfigUtils are used here. They feel a bit misplaced in plugin validation, don't they?

Well, this PR does not change anything about that. But, as validation is per session, the session is really the only thing that offers itself to store things in "natural" way. MavenSession does not have any means for this, so it is resolver session data that is used. It is done like it as this guarantees that validator does do what it needs to do in correct way in single-threaded and multi-threaded mvn and also in mvnd.

I understand that, but still looks like because it is not the right storage for this. Maven session contains user and system properties.

[cstamas]

Maven session contains user and system properties? Well, nope, it does not, it contains request, and those are java.util.Properties that may not contains "any object" and also does not offer any concurrent methods like putIfAbsent...

[michael-o]

Maven session contains user and system properties? Well, nope, it does not, it contains request, and those are java.util.Properties that may not contains "any object" and also does not offer any concurrent methods like putIfAbsent...

https://maven.apache.org/ref/3.3.9/maven-core/apidocs/org/apache/maven/execution/MavenSession.html#getUserProperties()

#getProperty(String) gives you a string. The abuse of RSS is in indicator of missing bits in MavenSession.

[cstamas]

#getProperty(String) gives you a string. The abuse of RSS is in indicator of missing bits in MavenSession.

Again, I agree completely, but again, this PR is not about "adding missing bits to maven session"

[michael-o]

#getProperty(String) gives you a string. The abuse of RSS is in indicator of missing bits in MavenSession.

Again, I agree completely, but again, this PR is not about "adding missing bits to maven session"

I know, I don't object this PR, I just noticed this. I will raise an issue.

[michael-o]

Question: If the users passes mvn -DKEY Maven will pass true as value. Do you want to accept this silently or reject?

[michael-o]

Done: https://issues.apache.org/jira/browse/MNG-7768

[cstamas]

Question: If the users passes mvn -DKEY Maven will pass true as value. Do you want to accept this silently or reject?

Sure. The existing behavior of validation manager is aligned: it changes output level ONLY if value user passed is ANY of the 2 non-default values, otherwise is default. So to say, "true" would yield "default" as well.

[michael-o]

Question: If the users passes mvn -DKEY Maven will pass true as value. Do you want to accept this silently or reject?

Sure. The existing behavior of validation manager is aligned: it changes output level ONLY if value user passed is ANY of the 2 non-default values, otherwise is default. So to say, "true" would yield "default" as well.

Have you actually tried? I think that "true" will be returned by ConfigUtils and it will fail with IAE.

[cstamas]

Did not, just the code make it happen: validator uses resolver ConfigUtil:
https://github.com/apache/maven/pull/1092/files#diff-9478004903992887e1696cebb44ffb9cb99032f9d25db4447e734c4a26930229L68
that on other hand does this:
https://github.com/apache/maven-resolver/blob/master/maven-resolver-util/src/main/java/org/eclipse/aether/util/ConfigUtils.java#L92

[michael-o]

Did not, just the code make it happen: validator uses resolver ConfigUtil: https://github.com/apache/maven/pull/1092/files#diff-9478004903992887e1696cebb44ffb9cb99032f9d25db4447e734c4a26930229L68 that on other hand does this: https://github.com/apache/maven-resolver/blob/master/maven-resolver-util/src/main/java/org/eclipse/aether/util/ConfigUtils.java#L92

It won't return boolean, but "true" as a string.

[michael-o]

This doesn't look right:

[INFO] --- enforcer:3.1.0:enforce (ensure-no-sonatype-cipher-and-sec-dispatcher) @ apache-maven ---
[INFO] ------------------------------------------------------------------------
[INFO] Reactor Summary for Apache Maven 3.9.2-SNAPSHOT:
[INFO]
[INFO] Apache Maven ....................................... SUCCESS [  1.049 s]
[INFO] Maven Model ........................................ SUCCESS [  0.082 s]
[INFO] Maven Artifact ..................................... SUCCESS [  0.094 s]
[INFO] Maven Plugin API ................................... SUCCESS [  0.896 s]
[INFO] Maven Builder Support .............................. SUCCESS [  0.030 s]
[INFO] Maven Model Builder ................................ SUCCESS [  0.264 s]
[INFO] Maven Settings ..................................... SUCCESS [  0.032 s]
[INFO] Maven Settings Builder ............................. SUCCESS [  0.085 s]
[INFO] Maven Repository Metadata Model .................... SUCCESS [  0.044 s]
[INFO] Maven Artifact Resolver Provider ................... SUCCESS [  0.552 s]
[INFO] Maven Core ......................................... SUCCESS [  0.413 s]
[INFO] Maven SLF4J Simple Provider ........................ SUCCESS [  0.064 s]
[INFO] Maven Embedder ..................................... SUCCESS [  0.617 s]
[INFO] Maven Compat ....................................... SUCCESS [  0.310 s]
[INFO] Apache Maven Distribution .......................... SUCCESS [  0.599 s]
[INFO] ------------------------------------------------------------------------
[INFO] BUILD SUCCESS
[INFO] ------------------------------------------------------------------------
[INFO] Total time:  5.597 s
[INFO] Finished at: 2023-04-19T12:59:47+02:00
[INFO] ------------------------------------------------------------------------
[WARNING] Invalid value specified for property maven.plugin.validation: 'true'. Supported values are (case insensitive): [BRIEF, DEFAULT, VERBOSE]
[WARNING]
[WARNING] Plugin validation issues were detected in 2 plugin(s)
[WARNING]
[WARNING]  * org.apache.maven.plugins:maven-site-plugin:3.12.1
[WARNING]  * org.apache.maven.plugins:maven-enforcer-plugin:3.1.0
[WARNING]
[WARNING] For more or less details, use 'maven.plugin.validation' property with one of the values (case insensitive): [BRIEF, DEFAULT, VERBOSE]
[WARNING]

Invalid values should not warn, but fail. "true" does not work opposed to what you have stated, I see basically the same message twice. I think that the input validation should happen early, not late.

Command: /tmp/apache-maven-3.9.2-SNAPSHOT/bin/mvn validate -Dmaven.plugin.validation -V

[cstamas]

IMHO this above IS aligned with my statement: "it changes output level ONLY if value user passed is ANY of the 2 non-default values, otherwise is default"

In your case, it remained DEFAULT. Also, this property does not affect Maven functionality (build, plugin or whatever wise), it merely controls the level of output, I see no value of failing for invalid input (as in other case, it may affect output of build, ie. making it incomplete or whatever... but again, this property does not affect anything functionality wise, merely the report detail). Validation happens "as early" as plugins are about to resolve, that is already "late".

I really don't want to spread this all over the place just to achieve some "ideal" state: i'd rather keep it simple and focused. To achieve early validation, we need some early hook that would involve and perform validation, etc... is it all really worth it? Again, this is really just a splat to user face, to update the build. If he mistypes the property value, and instead of "quiet" output "default" output happens, is it the end of the world? Should the build break for it? Am unsure.

@slawekjaranowski @gnodet or anyone else? Any opinion?

[cstamas]

Am merging this PR, my parallel with log levels still stand (as the property is really just like a log level, does not alter anything else, just the amount of validation output)