[GitHub] [maven] michael-o commented on a diff in pull request #812: [MNG-6437] Better support for path and uri in property interpolation

[GitBox <gi...@apache.org>]

michael-o commented on code in PR #812:
URL: https://github.com/apache/maven/pull/812#discussion_r990817790


##########
maven-core/src/main/java/org/apache/maven/plugin/PluginParameterExpressionEvaluatorV4.java:
##########
@@ -343,13 +352,13 @@ else if ( expression.startsWith( "settings" ) )
 
                 if ( pathSeparator > 0 )
                 {
-                    String pathExpression = expression.substring( 1, pathSeparator );
+                    String pathExpression = expression.substring( 0, pathSeparator );
                     value = ReflectionValueExtractor.evaluate( pathExpression, session.getSettings() );
                     value = value + expression.substring( pathSeparator );
                 }
                 else
                 {
-                    value = ReflectionValueExtractor.evaluate( expression.substring( 1 ), session.getSettings() );
+                    value = ReflectionValueExtractor.evaluate( expression, session.getSettings() );
                 }
             }
             catch ( Exception e )

Review Comment:
   Can you explain why there is now a shift in the string?



##########
maven-core/src/test/java/org/apache/maven/plugin/PluginParameterExpressionEvaluatorV4Test.java:
##########
@@ -0,0 +1,104 @@
+package org.apache.maven.plugin;
+
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License.  You may obtain a copy of the License at
+ *
+ *  http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied.  See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+import javax.inject.Inject;
+
+import java.io.File;
+import java.nio.file.Path;
+import java.nio.file.Paths;
+import java.util.ArrayList;
+import java.util.Collections;
+import java.util.List;
+import java.util.Map;
+import java.util.Properties;
+
+import org.apache.maven.AbstractCoreMavenComponentTestCase;
+import org.apache.maven.api.Session;
+import org.apache.maven.artifact.Artifact;
+import org.apache.maven.artifact.ArtifactUtils;
+import org.apache.maven.artifact.repository.ArtifactRepository;
+import org.apache.maven.execution.DefaultMavenExecutionRequest;
+import org.apache.maven.execution.DefaultMavenExecutionResult;
+import org.apache.maven.execution.MavenExecutionRequest;
+import org.apache.maven.execution.MavenSession;
+import org.apache.maven.internal.impl.DefaultSession;
+import org.apache.maven.model.Build;
+import org.apache.maven.model.Dependency;
+import org.apache.maven.model.Model;
+import org.apache.maven.plugin.descriptor.MojoDescriptor;
+import org.apache.maven.plugin.descriptor.PluginDescriptor;
+import org.apache.maven.project.DuplicateProjectException;
+import org.apache.maven.project.MavenProject;
+import org.apache.maven.repository.RepositorySystem;
+import org.codehaus.plexus.MutablePlexusContainer;
+import org.codehaus.plexus.PlexusContainer;
+import org.codehaus.plexus.component.configurator.expression.ExpressionEvaluator;
+import org.codehaus.plexus.util.dag.CycleDetectedException;
+import org.junit.jupiter.api.Test;
+
+import static org.codehaus.plexus.testing.PlexusExtension.getTestFile;
+import static org.junit.jupiter.api.Assertions.assertEquals;
+import static org.junit.jupiter.api.Assertions.assertNotNull;
+import static org.junit.jupiter.api.Assertions.assertSame;
+import static org.junit.jupiter.api.Assertions.assertTrue;
+
+public class PluginParameterExpressionEvaluatorV4Test
+    extends AbstractCoreMavenComponentTestCase
+{
+
+    @Test
+    public void testUri()
+            throws Exception
+    {
+        Path path = Paths.get( "" ).toAbsolutePath();
+
+        MavenSession mavenSession = createMavenSession( null );
+        mavenSession.getRequest().setBaseDirectory( path.toFile() );
+        mavenSession.getRequest().setMultiModuleProjectDirectory( path.toFile() );
+
+        Object result = new PluginParameterExpressionEvaluatorV4( mavenSession.getSession(), null )
+                .evaluate( "${session.multiModuleProjectDirectory.uri}" );
+        assertEquals( path.toUri(), result );
+    }
+
+    @Test
+    public void testPath()
+            throws Exception
+    {
+        Path path = Paths.get( "" ).toAbsolutePath();
+
+        MavenSession mavenSession = createMavenSession( null );
+        mavenSession.getRequest().setBaseDirectory( path.toFile() );
+        mavenSession.getRequest().setMultiModuleProjectDirectory( path.toFile() );
+
+        Object result = new PluginParameterExpressionEvaluatorV4( mavenSession.getSession(), null )
+                .evaluate( "${session.multiModuleProjectDirectory/target}" );

Review Comment:
   I am a bit confused where this is documented with the `/`.



##########
maven-core/src/main/java/org/apache/maven/plugin/ReflectionValueExtractor.java:
##########
@@ -0,0 +1,366 @@
+package org.apache.maven.plugin;
+
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License.  You may obtain a copy of the License at
+ *
+ *  http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied.  See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+import java.lang.ref.WeakReference;
+import java.lang.reflect.Array;
+import java.lang.reflect.InvocationTargetException;
+import java.lang.reflect.Method;
+import java.util.Arrays;
+import java.util.List;
+import java.util.Map;
+import java.util.WeakHashMap;
+
+import org.codehaus.plexus.util.StringUtils;
+import org.codehaus.plexus.util.introspection.ClassMap;
+
+/**
+ * <p>
+ * Using simple dotted expressions to extract the values from an Object instance, For example we might want to extract a
+ * value like: <code>project.build.sourceDirectory</code>
+ * </p>
+ * <p>
+ * The implementation supports indexed, nested and mapped properties similar to the JSP way.
+ * </p>
+ * <p>
+ * In addition to usual getters using {@code getXxx} or {@code isXxx} suffixes, accessors
+ * using {code asXxx} or {@ocode toXxx} prefixes are also supported.
+ * </p>
+ */
+public class ReflectionValueExtractor
+{
+    private static final Class<?>[] CLASS_ARGS = new Class[0];
+
+    private static final Object[] OBJECT_ARGS = new Object[0];
+
+    /**
+     * Use a WeakHashMap here, so the keys (Class objects) can be garbage collected. This approach prevents permgen
+     * space overflows due to retention of discarded classloaders.
+     */
+    private static final Map<Class<?>, WeakReference<ClassMap>> CLASS_MAPS = new WeakHashMap<>();
+
+    static final int EOF = -1;
+
+    static final char PROPERTY_START = '.';
+
+    static final char INDEXED_START = '[';
+
+    static final char INDEXED_END = ']';
+
+    static final char MAPPED_START = '(';
+
+    static final char MAPPED_END = ')';
+
+    static class Tokenizer
+    {
+        final String expression;
+
+        int idx;
+
+        Tokenizer( String expression )
+        {
+            this.expression = expression;
+        }
+
+        public int peekChar()
+        {
+            return idx < expression.length() ? expression.charAt( idx ) : EOF;
+        }
+
+        public int skipChar()
+        {
+            return idx < expression.length() ? expression.charAt( idx++ ) : EOF;
+        }
+
+        public String nextToken( char delimiter )
+        {
+            int start = idx;
+
+            while ( idx < expression.length() && delimiter != expression.charAt( idx ) )
+            {
+                idx++;
+            }
+
+            // delimiter MUST be present
+            if ( idx <= start || idx >= expression.length() )
+            {
+                return null;
+            }
+
+            return expression.substring( start, idx++ );
+        }
+
+        public String nextPropertyName()
+        {
+            final int start = idx;
+
+            while ( idx < expression.length() && Character.isJavaIdentifierPart( expression.charAt( idx ) ) )
+            {
+                idx++;
+            }
+
+            // property name does not require delimiter
+            if ( idx <= start || idx > expression.length() )
+            {
+                return null;
+            }
+
+            return expression.substring( start, idx );
+        }
+
+        public int getPosition()
+        {
+            return idx < expression.length() ? idx : EOF;
+        }
+
+        // to make tokenizer look pretty in debugger
+        @Override
+        public String toString()
+        {
+            return idx < expression.length() ? expression.substring( idx ) : "<EOF>";
+        }
+    }
+
+    private ReflectionValueExtractor()
+    {
+    }
+
+    /**
+     * <p>The implementation supports indexed, nested and mapped properties.</p>
+     *
+     * <ul>
+     * <li>nested properties should be defined by a dot, i.e. "user.address.street"</li>
+     * <li>indexed properties (java.util.List or array instance) should be contains <code>(\\w+)\\[(\\d+)\\]</code>
+     * pattern, i.e. "user.addresses[1].street"</li>
+     * <li>mapped properties should be contains <code>(\\w+)\\((.+)\\)</code> pattern, i.e.
+     * "user.addresses(myAddress).street"</li>

Review Comment:
   JSP EL uses the following syntax for maps: `mapInstance['string-key']`. I have never seen with `()`.



-- 
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: issues-unsubscribe@maven.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org