From 402c8c2bc084a7a3dec25d3d5588eb01db950d37 Mon Sep 17 00:00:00 2001 From: Konrad Windszus Date: Fri, 16 Aug 2024 18:15:11 +0200 Subject: [PATCH 1/3] [MNG-8214] Improve model velocity template to support subclasses Make the constructors protected and take the Builder as argument --- api/maven-api-plugin/pom.xml | 5 ++ .../another/ExtendedPluginDescriptorTest.java | 77 ++++++++++++++++ src/mdo/model.vm | 87 +++++-------------- 3 files changed, 105 insertions(+), 64 deletions(-) create mode 100644 api/maven-api-plugin/src/test/java/org/apache/maven/api/plugin/descriptor/another/ExtendedPluginDescriptorTest.java diff --git a/api/maven-api-plugin/pom.xml b/api/maven-api-plugin/pom.xml index 733c1158e2e7..9a9a876c99b2 100644 --- a/api/maven-api-plugin/pom.xml +++ b/api/maven-api-plugin/pom.xml @@ -39,6 +39,11 @@ under the License. org.apache.maven maven-api-xml + + org.junit.jupiter + junit-jupiter-api + test + diff --git a/api/maven-api-plugin/src/test/java/org/apache/maven/api/plugin/descriptor/another/ExtendedPluginDescriptorTest.java b/api/maven-api-plugin/src/test/java/org/apache/maven/api/plugin/descriptor/another/ExtendedPluginDescriptorTest.java new file mode 100644 index 000000000000..3b6fb037aa06 --- /dev/null +++ b/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()); + } +} diff --git a/src/mdo/model.vm b/src/mdo/model.vm index 4658eb1cee8c..2dbf95b2a1c9 100644 --- a/src/mdo/model.vm +++ b/src/mdo/model.vm @@ -145,55 +145,39 @@ public class ${class.name} #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 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 newlocs = builder.locations != null ? builder.locations : Collections.emptyMap(); + Map oldlocs = builder.base != null && builder.base.locations != null ? builder.base.locations : Collections.emptyMap(); #if ( ! $class.superClass ) - this.locations = ImmutableCollections.copy(locations); - this.importedFrom = importedFrom; + this.locations = new HashMap<>(); + this.importedFrom = builder.importedFrom; + this.locations.put("", newlocs.containsKey("") ? newlocs.get("") : oldlocs.get("")); #end + #foreach ( $field in $class.getFields($version) ) + this.locations.put("${field.name}", newlocs.containsKey("${field.name}") ? newlocs.get("${field.name}") : oldlocs.get("${field.name}")); + #end #end } @@ -406,7 +390,7 @@ public class ${class.name} InputLocation importedFrom; #end - Builder(boolean withDefaults) { + protected Builder(boolean withDefaults) { #if ( $class.superClass ) super(withDefaults); #end @@ -424,7 +408,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,6 +477,7 @@ public class ${class.name} #end @Nonnull public ${class.name} build() { + // this method should not contain any logic other than creating the object in order to ease subclassing if (base != null #foreach ( $field in $allFields ) && (${field.name} == null || ${field.name} == base.${field.name}) @@ -500,33 +485,7 @@ public class ${class.name} ) { return base; } - #if ( $locationTracking ) - Map newlocs = this.locations != null ? this.locations : Collections.emptyMap(); - Map oldlocs = this.base != null && this.base.locations != null ? this.base.locations : Collections.emptyMap(); - Map 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); } } From 8ce4cdedd2040f9a97b2cc6a586b67b4aae749e6 Mon Sep 17 00:00:00 2001 From: Konrad Windszus Date: Mon, 26 Aug 2024 09:24:42 +0200 Subject: [PATCH 2/3] Make locations map immutable --- src/mdo/model.vm | 17 +++++++++++------ 1 file changed, 11 insertions(+), 6 deletions(-) diff --git a/src/mdo/model.vm b/src/mdo/model.vm index 2dbf95b2a1c9..1ba49ca1922f 100644 --- a/src/mdo/model.vm +++ b/src/mdo/model.vm @@ -136,9 +136,9 @@ 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 locations; + #if ( ! $class.superClass ) /** Location tracking */ final InputLocation importedFrom; #end @@ -171,13 +171,16 @@ public class ${class.name} Map newlocs = builder.locations != null ? builder.locations : Collections.emptyMap(); Map oldlocs = builder.base != null && builder.base.locations != null ? builder.base.locations : Collections.emptyMap(); #if ( ! $class.superClass ) - this.locations = new HashMap<>(); + Map mutableLocations = new HashMap<>(); this.importedFrom = builder.importedFrom; - this.locations.put("", newlocs.containsKey("") ? newlocs.get("") : oldlocs.get("")); + mutableLocations.put("", newlocs.containsKey("") ? newlocs.get("") : oldlocs.get("")); + #else + Map mutableLocations = new HashMap<>(super.locations); #end #foreach ( $field in $class.getFields($version) ) - this.locations.put("${field.name}", newlocs.containsKey("${field.name}") ? newlocs.get("${field.name}") : oldlocs.get("${field.name}")); + mutableLocations.put("${field.name}", newlocs.containsKey("${field.name}") ? newlocs.get("${field.name}") : oldlocs.get("${field.name}")); #end + this.locations = Collections.unmodifiableMap(mutableLocations); #end } @@ -234,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. */ @@ -249,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. */ @@ -257,6 +261,7 @@ public class ${class.name} return importedFrom; } + #end #end /** * Creates a new builder with this object as the basis. From effa73709e4b49a9509f0891ee3c6225b0b9a02c Mon Sep 17 00:00:00 2001 From: Konrad Windszus Date: Tue, 27 Aug 2024 09:01:15 +0200 Subject: [PATCH 3/3] clarify comment --- src/mdo/model.vm | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/mdo/model.vm b/src/mdo/model.vm index 1ba49ca1922f..008142064086 100644 --- a/src/mdo/model.vm +++ b/src/mdo/model.vm @@ -482,7 +482,7 @@ public class ${class.name} #end @Nonnull public ${class.name} build() { - // this method should not contain any logic other than creating the object in order to ease subclassing + // 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})