Improve accessibility of Mojo configuration parameters and (partially) use it in M2E #996

HannesWell · 2022-10-18T17:04:24Z

This PR aims to improve the accessibility of Mojo configuration parameters by introducing the method IMaven.getMojoConfiguration(MavenProject , MojoExecution , IProgressMonitor ), which returns a ConfigurationElement that allows to traverse the tree that can be formed by a configuration in general.

The advantage are:

Enables access to nested elements, not only top-level elements
Enables accessing multiple elements of same name
In general enables to selectively walk the configuration element tree

The new method is intended as a replacement for the method IMaven.getMojoParameterValue(MavenProject, MojoExecution, String, Class, IProgressMonitor), which returns only the value of a top-level configuration element.
The method IMaven.getMojoParameterValue(MavenProject, String, Class, Plugin, ConfigurationContainer, String, IProgressMonitor) can probably replaced in the same manner.

The new method can for example be used in #981 or if we introduce for example a connector for the Maven clean-plugin to handle additional files to delete.

This PR is a draft that is not yet completed, method names have to/can be adjusted, java-doc has to be added as well as tests.
@mickaelistria, @laeubi what do you think?

github-actions · 2022-10-18T17:22:50Z

Unit Test Results

598 tests 592 ✔️ 8m 5s ⏱️
  94 suites     6 💤
  94 files     0 ❌

Results for commit 3c5a887.

♻️ This comment has been updated with latest results.

kwin

Looks good overall, added some minor implementation remarks.

kwin · 2022-10-18T17:27:21Z

org.eclipse.m2e.core/src/org/eclipse/m2e/core/embedder/IMaven.java

+   * @since 2.1
+   */
+  public interface ConfigurationElement {
+    boolean exists();


This seems weird, I would rather throw some exception when invalid names are passed. The top level element should always be valid (even if no explicit configuration is given in the underlying effective pom).

I agree that the top-level element should always be valid and if actually absent should be treated as empty.
But since (some) elements can be present or absent, it is also a valid workflow to check if a certain element exists and only handle it if it exists. Therefore I think throwing an exception in that case is not so convenient and wanted ConfigurationElement to be similar to Optional.

kwin · 2022-10-18T17:32:19Z

org.eclipse.m2e.core/src/org/eclipse/m2e/core/embedder/IMaven.java

+    ConfigurationParameter get(String name) throws NoSuchElementException;
+
+    // TODO: or getAll() or similar.
+    Stream<ConfigurationParameter> children(String name) throws NoSuchElementException;


name is always unique except for collections (https://maven.apache.org/guides/mini/guide-configuring-plugins.html#mapping-collections-and-arrays). I don't think that we need to support that here, I one is interested in getting the collection she/he can call get("rootElementForCollection).as(Collection.class).

That is a good point. But if one calls get("rootElementForCollection).as(Collection.class) the elements of the returned collection are (probably) of the type used in the Mojo, which are usually not available in a connector. I think it is better to have a collection/stream of ConfiguraitonElements here so that a caller can further query on this abstract/proxy level.

However, I have to admit I never actually had such case yet and therefore should create a test-case for that. Nevertheless I think it is relevant for the example given initially.

org.eclipse.m2e.core/src/org/eclipse/m2e/core/embedder/IMaven.java

kwin · 2022-10-18T17:37:19Z

org.eclipse.m2e.core/src/org/eclipse/m2e/core/internal/embedder/MavenImpl.java

  }

-  private String formatAsDirectory(String directory) {
-    return directory.replace(GROUP_SEPARATOR, PATH_SEPARATOR);
+  private class ConfigurationElementImpl implements ConfigurationElement {


I would put this and related classes to a dedicated file.

Actually I have no strong opinion about that.
But the MavenImpl class is already quite large and moving the configuration related classes into a dedicated class makes it simpler to overview that story.
-> Moved it.

org.eclipse.m2e.core/src/org/eclipse/m2e/core/internal/embedder/MavenImpl.java

kwin · 2022-10-18T17:41:02Z

org.eclipse.m2e.core/src/org/eclipse/m2e/core/internal/embedder/MavenImpl.java

+    public <T> T as(Class<T> clazz) throws NoSuchElementException {
+      if(!exists()) {
+        throw new NoSuchElementException(
+            "Plugin execution " + valueComputer.mojo.getId() + "does not have a configuration parameter " + name);


this should emit the full path to the parameter

The name/path is already the full path. When a new ConfigurationParameterImpl is created the parents path concatenated with the elements name is passed as path.

org.eclipse.m2e.core/src/org/eclipse/m2e/core/internal/embedder/MavenImpl.java

Advantages: - Enables access to nested elements - Enables accessing multiple elements of same name - In general enables to selectively walk the configuration element tree

mickaelistria · 2022-10-19T06:46:55Z

I'm skeptical in general about the idea of m2e building a different model and new classes for concepts that are mostly already expressed by Maven API. This makes m2e much harder to understand IMO, while sticking to Maven API is actually keeping things simple. For this case, I believe that instead of a IConfigurationElement model, some "MojoConfigurationHelper" that has static method to manipulate Maven's MojoExecution and other plain Maven types would in the end be simpler.

HannesWell · 2022-10-19T07:14:12Z

I'm skeptical in general about the idea of m2e building a different model and new classes for concepts that are mostly already expressed by Maven API. This makes m2e much harder to understand IMO, while sticking to Maven API is actually keeping things simple. For this case, I believe that instead of a IConfigurationElement model, some "MojoConfigurationHelper" that has static method to manipulate Maven's MojoExecution and other plain Maven types would in the end be simpler.

The configuration elements should only be read, but should not be changed (in case you mean that by manipulating).
I think what should be provided by the new method and what (probably) actually wants is simply the full PlexusConfiguration of the mojo execution. Actually the only reason why we can use the raw configuration from the dom is that it can contain unresolved property/variable references.
So instead of inventing a new API we could just reuse PlexusConfiguration and use a ready-only implementation (subclass) that resolves values on demand using the means already used at the moment.
This way a user gets a PlexusConfiguration that looks like it contains only resolved values and does not permit writes (e.g. throws a UnsupportedOperationException).
Since the PlexusConfiguration API is much older it is a bit old fashion (i.e. it uses Arrays instead of Collection/Stream), but at least users of the Maven-API are used to it.

kwin · 2022-10-28T15:07:14Z

To give more context: In #981 there was the need to evaluate nested Mojo parameters and currently there is no support for that. I proposed the simpler approach https://github.com/eclipse-m2e/m2e-core/pull/981/files#diff-367681037e5adcf9592ed08e4e4ae7e8e7372a00b38d77e4d66bcb7f8e60b59dR733. Maybe there is a third alternative you have in mind @mickaelistria...

laeubi · 2022-10-29T03:59:26Z

As mentioned elsewhere, I don't think Mojo Parameters are a concern of IMaven and we should get rid of it here, move it to the MavenToolbox or a dedicated place.

kwin · 2022-10-29T10:34:44Z

Mojo parameters can be evaluated with m2e 2.0 via IMaven already. I am fine with deprecating that method but please stay backwards compatible this time.

laeubi · 2022-10-29T10:38:46Z

Mojo parameters can be evaluated with m2e 2.0 via IMaven already.

But this does not mean we should add new ones...

I am fine with deprecating that method but please stay backwards compatible this time.

There is no backward-compatible way for deprecation.

kwin · 2022-10-29T10:46:50Z

@laeubi It seems you don’t get the point about deprecation. It gives downstream consumers time to migrate and an indication where to migrate to. Final removal of deprecated items should only be done in m2e 3.0 earliest. And deprecating a method is fully backwards compatible!

laeubi · 2022-10-29T10:48:46Z

Final removal of deprecated items should only be done in m2e 3.0 earliest

It should not, it will happen at 3.0 ...

HannesWell · 2022-10-29T10:52:47Z

Final removal of deprecated items should only be done in m2e 3.0 earliest

It should not, it will happen at 3.0 ...

Yes, with 3.0 (whenever this will be) not earlier and not later. And with #996 there will be a proper deprecation notice including the path for migration. So this time the migration should be clear. :)

HannesWell · 2022-11-23T19:50:12Z

As mentioned elsewhere, I don't think Mojo Parameters are a concern of IMaven and we should get rid of it here, move it to the MavenToolbox or a dedicated place.

A note about the remarks in #981:

Plug-in executions are usually bound to a project, so the new methods could be part of the MavenProjectFacadade or in any other project bound api.

Minor clean-ups and simplifications in IMaven and MavenImpl

0a23551

HannesWell mentioned this pull request Oct 18, 2022

Only evaluate major and minor version of enforcer java rule #981

Merged

kwin reviewed Oct 18, 2022

View reviewed changes

Improve accessibility of Mojo configuration parameters

28c0022

Advantages: - Enables access to nested elements - Enables accessing multiple elements of same name - In general enables to selectively walk the configuration element tree

HannesWell force-pushed the mojoConfig branch from 2e1aff1 to ad272c1 Compare October 18, 2022 21:53

Use enhanced Mojo-Configuration methods

3c5a887

HannesWell force-pushed the mojoConfig branch from ad272c1 to 3c5a887 Compare October 18, 2022 22:43

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improve accessibility of Mojo configuration parameters and (partially) use it in M2E #996

Improve accessibility of Mojo configuration parameters and (partially) use it in M2E #996

HannesWell commented Oct 18, 2022

github-actions bot commented Oct 18, 2022 •

edited

Loading

kwin left a comment

kwin Oct 18, 2022

HannesWell Oct 18, 2022

kwin Oct 18, 2022

HannesWell Oct 18, 2022

kwin Oct 18, 2022

HannesWell Oct 18, 2022 •

edited

Loading

kwin Oct 18, 2022

HannesWell Oct 18, 2022

mickaelistria commented Oct 19, 2022

HannesWell commented Oct 19, 2022

kwin commented Oct 28, 2022

laeubi commented Oct 29, 2022

kwin commented Oct 29, 2022

laeubi commented Oct 29, 2022

kwin commented Oct 29, 2022 •

edited

Loading

laeubi commented Oct 29, 2022 •

edited

Loading

HannesWell commented Oct 29, 2022 •

edited

Loading

HannesWell commented Nov 23, 2022

Improve accessibility of Mojo configuration parameters and (partially) use it in M2E #996

Are you sure you want to change the base?

Improve accessibility of Mojo configuration parameters and (partially) use it in M2E #996

Conversation

HannesWell commented Oct 18, 2022

github-actions bot commented Oct 18, 2022 • edited Loading

Unit Test Results

kwin left a comment

Choose a reason for hiding this comment

kwin Oct 18, 2022

Choose a reason for hiding this comment

HannesWell Oct 18, 2022

Choose a reason for hiding this comment

kwin Oct 18, 2022

Choose a reason for hiding this comment

HannesWell Oct 18, 2022

Choose a reason for hiding this comment

kwin Oct 18, 2022

Choose a reason for hiding this comment

HannesWell Oct 18, 2022 • edited Loading

Choose a reason for hiding this comment

kwin Oct 18, 2022

Choose a reason for hiding this comment

HannesWell Oct 18, 2022

Choose a reason for hiding this comment

mickaelistria commented Oct 19, 2022

HannesWell commented Oct 19, 2022

kwin commented Oct 28, 2022

laeubi commented Oct 29, 2022

kwin commented Oct 29, 2022

laeubi commented Oct 29, 2022

kwin commented Oct 29, 2022 • edited Loading

laeubi commented Oct 29, 2022 • edited Loading

HannesWell commented Oct 29, 2022 • edited Loading

HannesWell commented Nov 23, 2022

github-actions bot commented Oct 18, 2022 •

edited

Loading

HannesWell Oct 18, 2022 •

edited

Loading

kwin commented Oct 29, 2022 •

edited

Loading

laeubi commented Oct 29, 2022 •

edited

Loading

HannesWell commented Oct 29, 2022 •

edited

Loading