[MNG-8214] Improve model velocity template to support subclasses (#1660)

Make the constructors protected and take the Builder as argument

api/maven-api-plugin/pom.xml

@@ -39,6 +39,11 @@ under the License.
       <groupId>org.apache.maven</groupId>
       <artifactId>maven-api-xml</artifactId>
     </dependency>
+    <dependency>
+      <groupId>org.junit.jupiter</groupId>
+      <artifactId>junit-jupiter-api</artifactId>
+      <scope>test</scope>
+    </dependency>
   </dependencies>
 
   <build>

api/maven-api-plugin/src/test/java/org/apache/maven/api/plugin/descriptor/another/ExtendedPluginDescriptorTest.java

@@ -0,0 +1,77 @@
+/*
+ * 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.
+ */
+package org.apache.maven.api.plugin.descriptor.another;
+
+import org.apache.maven.api.plugin.descriptor.PluginDescriptor;
+import org.junit.jupiter.api.Test;
+
+import static org.junit.jupiter.api.Assertions.assertEquals;
+
+/**
+ * Verifies that subclasses from generated model classes are possible.
+ */
+class ExtendedPluginDescriptorTest {
+
+    /**
+     * A subclass of the generated class {@link PluginDescriptor} that adds an additional field.
+     */
+    public static class ExtendedPluginDescriptor extends PluginDescriptor {
+
+        private final String additionalField;
+
+        protected ExtendedPluginDescriptor(Builder builder) {
+            super(builder);
+            this.additionalField = builder.additionalField;
+        }
+
+        public String getAdditionalField() {
+            return additionalField;
+        }
+
+        public static class Builder extends PluginDescriptor.Builder {
+            protected String additionalField;
+
+            public Builder() {
+                super(false);
+            }
+
+            public Builder additionalField(String additionalField) {
+                this.additionalField = additionalField;
+                return this;
+            }
+
+            public ExtendedPluginDescriptor build() {
+                return new ExtendedPluginDescriptor(this);
+            }
+        }
+    }
+
+    @Test
+    void testExtendedPluginDescriptor() {
+        ExtendedPluginDescriptor.Builder builder = new ExtendedPluginDescriptor.Builder();
+        // make sure to call the subclasses' builder methods first, otherwise fluent API would not work
+        builder.additionalField("additional")
+                .groupId("org.apache.maven")
+                .artifactId("maven-plugin-api")
+                .version("1.0.0");
+        ExtendedPluginDescriptor descriptor = builder.build();
+        assertEquals("additional", descriptor.getAdditionalField());
+        assertEquals("org.apache.maven", descriptor.getGroupId());
+    }
+}

src/mdo/model.vm

@@ -136,64 +136,51 @@ public class ${class.name}
     final ${type} $field.name;
     #end
     #if ( $locationTracking )
-      #if ( ! $class.superClass )
-    /** Locations */
+    /** Locations (this potentially hides the same name field from the super class) */
     final Map<Object, InputLocation> locations;
+      #if ( ! $class.superClass )
     /** Location tracking */
     final InputLocation importedFrom;
       #end
     #end
 
     /**
-      * Constructor for this class, package protected.
+      * Constructor for this class, to be called from its subclasses and {@link Builder}.
       * @see Builder#build()
       */
-    ${class.name}(
-    #if ( $class == $root )
-        String namespaceUri,
-        String modelEncoding,
-    #end
-    #foreach ( $field in $allFields )
-      #set ( $sep = "#if(${locationTracking}||$field!=${allFields[${allFields.size()} - 1]}),#end" )
-      #set ( $type = ${types.getOrDefault($field,${types.getOrDefault($field.type,$field.type)})} )
-      #if ( $type.startsWith("List<") )
-        #set ( $type = ${type.replace('List<','Collection<')} )
-      #end
-        $type $field.name${sep}
-    #end
-    #if ( $locationTracking )
-        Map<Object, InputLocation> locations,
-        InputLocation importedFrom
-    #end
-    ) {
+    protected ${class.name}(Builder builder) {
     #if ( $class.superClass )
-        super(
-      #foreach ( $field in $inheritedFields )
-        #set ( $sep = "#if(${locationTracking}||$field!=${inheritedFields[${inheritedFields.size()} - 1]}),#end" )
-            ${field.name}${sep}
-      #end
-      #if ( $locationTracking )
-            locations,
-            importedFrom
-      #end
-        );
+        super(builder);
     #end
     #if ( $class == $root )
-        this.namespaceUri = namespaceUri;
-        this.modelEncoding = modelEncoding;
+        this.namespaceUri = builder.namespaceUri != null ? builder.namespaceUri : (builder.base != null ? builder.base.namespaceUri : null);
+        this.modelEncoding = builder.modelEncoding != null ? builder.modelEncoding : (builder.base != null ? builder.base.modelEncoding :  "UTF-8");
     #end
     #foreach ( $field in $class.getFields($version) )
       #if ( $field.type == "java.util.List" || $field.type == "java.util.Properties" || $field.type == "java.util.Map" )
-        this.${field.name} = ImmutableCollections.copy(${field.name});
+        this.${field.name} = ImmutableCollections.copy(builder.${field.name} != null ? builder.${field.name} : (builder.base != null ? builder.base.${field.name} : null));
       #else
-        this.${field.name} = ${field.name};
+        #if ( $field.type == "boolean" || $field.type == "int" )
+        this.${field.name} = builder.${field.name} != null ? builder.${field.name} : (builder.base != null ? builder.base.${field.name} : ${field.defaultValue});
+        #else
+        this.${field.name} = builder.${field.name} != null ? builder.${field.name} : (builder.base != null ? builder.base.${field.name} : null);
+        #end
       #end
     #end
     #if ( $locationTracking )
+        Map<Object, InputLocation> newlocs = builder.locations != null ? builder.locations : Collections.emptyMap();
+        Map<Object, InputLocation> oldlocs = builder.base != null && builder.base.locations != null ? builder.base.locations : Collections.emptyMap();
       #if ( ! $class.superClass )
-        this.locations = ImmutableCollections.copy(locations);
-        this.importedFrom = importedFrom;
+        Map<Object, InputLocation> mutableLocations = new HashMap<>();
+        this.importedFrom = builder.importedFrom;
+        mutableLocations.put("", newlocs.containsKey("") ? newlocs.get("") : oldlocs.get(""));
+      #else
+        Map<Object, InputLocation> mutableLocations = new HashMap<>(super.locations);
       #end
+        #foreach ( $field in $class.getFields($version) )
+        mutableLocations.put("${field.name}", newlocs.containsKey("${field.name}") ? newlocs.get("${field.name}") : oldlocs.get("${field.name}"));
+        #end
+        this.locations = Collections.unmodifiableMap(mutableLocations);
     #end
     }
 
@@ -250,7 +237,7 @@ public class ${class.name}
     }
 
     #end
-    #if ( $locationTracking && !$class.superClass )
+    #if ( $locationTracking )
     /**
      * Gets the location of the specified field in the input source.
      */
@@ -265,6 +252,7 @@ public class ${class.name}
         return locations != null ? locations.keySet() : null;
     }
 
+    #if ( !$class.superClass )
     /**
      * Gets the input location that caused this model to be read.
      */
@@ -273,6 +261,7 @@ public class ${class.name}
         return importedFrom;
     }
 
+    #end
     #end
     /**
      * Creates a new builder with this object as the basis.
@@ -406,7 +395,7 @@ public class ${class.name}
         InputLocation importedFrom;
     #end
 
-        Builder(boolean withDefaults) {
+        protected Builder(boolean withDefaults) {
     #if ( $class.superClass )
             super(withDefaults);
     #end
@@ -424,7 +413,7 @@ public class ${class.name}
             }
         }
 
-        Builder(${class.name} base, boolean forceCopy) {
+        protected Builder(${class.name} base, boolean forceCopy) {
     #if ( $class.superClass )
             super(base, forceCopy);
     #end
@@ -493,40 +482,15 @@ public class ${class.name}
     #end
         @Nonnull
         public ${class.name} build() {
+            // this method should not contain any logic other than creating (or reusing) an object in order to ease subclassing
             if (base != null
     #foreach ( $field in $allFields )
                     && (${field.name} == null || ${field.name} == base.${field.name})
     #end
             ) {
                 return base;
             }
-    #if ( $locationTracking )
-            Map<Object, InputLocation> newlocs = this.locations != null ? this.locations : Collections.emptyMap();
-            Map<Object, InputLocation> oldlocs = this.base != null && this.base.locations != null ? this.base.locations : Collections.emptyMap();
-            Map<Object, InputLocation> locations = new HashMap<>();
-            locations.put("", newlocs.containsKey("") ? newlocs.get("") : oldlocs.get(""));
-      #foreach ( $field in $allFields )
-            locations.put("${field.name}", newlocs.containsKey("${field.name}") ? newlocs.get("${field.name}") : oldlocs.get("${field.name}"));
-      #end
-    #end
-            return new ${class.name}(
-    #if ( $class == $root )
-                namespaceUri != null ? namespaceUri : (base != null ? base.namespaceUri : ""),
-                modelEncoding != null ? modelEncoding : (base != null ? base.modelEncoding : "UTF-8"),
-    #end
-    #foreach ( $field in $allFields )
-      #set ( $sep = "#if(${locationTracking}||$field!=${allFields[${allFields.size()} - 1]}),#end" )
-      #if ( $field.type == "boolean" || $field.type == "int" )
-                ${field.name} != null ? ${field.name} : (base != null ? base.${field.name} : ${field.defaultValue})${sep}
-      #else
-                ${field.name} != null ? ${field.name} : (base != null ? base.${field.name} : null)${sep}
-      #end
-    #end
-    #if ( $locationTracking )
-                locations,
-                importedFrom
-    #end
-            );
+            return new ${class.name}(this);
         }
     }