Skip to content

#73 Implementing Immutable Constructors #847

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
cliedeman wants to merge 1 commit into from
Closed

#73 Implementing Immutable Constructors #847

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
34 changes: 34 additions & 0 deletions core-common/src/main/java/org/mapstruct/Constructor.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,34 @@
/**
* Copyright 2012-2016 Gunnar Morling (http://www.gunnarmorling.de/)
* and/or other contributors as indicated by the @authors tag. See the
* copyright.txt file in the distribution for a full listing of all
* contributors.
*
* Licensed 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.mapstruct;

import java.lang.annotation.ElementType;
import java.lang.annotation.Retention;
import java.lang.annotation.RetentionPolicy;
import java.lang.annotation.Target;

/**
* @author Ciaran Liedeman
*/
@Target(ElementType.CONSTRUCTOR)
@Retention(RetentionPolicy.CLASS)
public @interface Constructor {

String[] value();
}
155 changes: 129 additions & 26 deletions processor/src/main/java/org/mapstruct/ap/internal/model/BeanMappingMethod.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,47 +18,49 @@
*/
package org.mapstruct.ap.internal.model;

import java.text.MessageFormat;
import java.util.ArrayList;
import java.util.Collection;
import java.util.Collections;
import java.util.Comparator;
import java.util.HashMap;
import java.util.HashSet;
import java.util.Iterator;
import java.util.LinkedHashMap;
import java.util.List;
import java.util.Map;
import java.util.Map.Entry;
import java.util.Set;

import javax.lang.model.element.ExecutableElement;
import javax.lang.model.type.DeclaredType;
import javax.tools.Diagnostic;

import org.mapstruct.ap.internal.model.PropertyMapping.ConstantMappingBuilder;
import org.mapstruct.ap.internal.model.PropertyMapping.JavaExpressionMappingBuilder;
import org.mapstruct.ap.internal.model.PropertyMapping.PropertyMappingBuilder;
import org.mapstruct.ap.internal.model.assignment.Assignment;
import org.mapstruct.ap.internal.model.common.Parameter;
import org.mapstruct.ap.internal.model.common.Type;
import org.mapstruct.ap.internal.model.common.Constructor;
import org.mapstruct.ap.internal.model.dependency.GraphAnalyzer;
import org.mapstruct.ap.internal.model.dependency.GraphAnalyzer.GraphAnalyzerBuilder;
import org.mapstruct.ap.internal.model.source.Mapping;
import org.mapstruct.ap.internal.model.source.PropertyEntry;
import org.mapstruct.ap.internal.model.source.SelectionParameters;
import org.mapstruct.ap.internal.model.source.SourceMethod;
import org.mapstruct.ap.internal.model.source.TargetReference;
import org.mapstruct.ap.internal.model.source.SourceReference;
import org.mapstruct.ap.internal.model.source.TargetReference;
import org.mapstruct.ap.internal.option.ReportingPolicy;
import org.mapstruct.ap.internal.prism.BeanMappingPrism;
import org.mapstruct.ap.internal.prism.CollectionMappingStrategyPrism;
import org.mapstruct.ap.internal.prism.NullValueMappingStrategyPrism;
import static org.mapstruct.ap.internal.util.Collections.first;
import static org.mapstruct.ap.internal.util.Collections.last;
import org.mapstruct.ap.internal.util.MapperConfiguration;
import org.mapstruct.ap.internal.util.Message;
import org.mapstruct.ap.internal.util.Strings;

import javax.lang.model.element.ExecutableElement;
import javax.lang.model.type.DeclaredType;
import javax.lang.model.type.TypeKind;
import javax.tools.Diagnostic;
import java.text.MessageFormat;
import java.util.ArrayList;
import java.util.Collection;
import java.util.Collections;
import java.util.Comparator;
import java.util.HashMap;
import java.util.HashSet;
import java.util.Iterator;
import java.util.LinkedHashMap;
import java.util.List;
import java.util.Map;
import java.util.Map.Entry;
import java.util.Set;

import static org.mapstruct.ap.internal.util.Collections.first;
import static org.mapstruct.ap.internal.util.Collections.last;
import static org.mapstruct.ap.internal.util.Strings.getSaveVariableName;

/**
Expand All @@ -70,6 +72,7 @@
public class BeanMappingMethod extends MappingMethod {

private final List<PropertyMapping> propertyMappings;
private final ConstructorMapping constructorMapping;
private final Map<String, List<PropertyMapping>> mappingsByParameter;
private final List<PropertyMapping> constantMappings;
private final MethodReference factoryMethod;
Expand All @@ -84,6 +87,8 @@ public static class Builder {
private Map<String, ExecutableElement> unprocessedTargetProperties;
private Set<String> targetProperties;
private final List<PropertyMapping> propertyMappings = new ArrayList<PropertyMapping>();
private final Map<String, PropertyMapping> constructorPropertyMappings = new HashMap<String, PropertyMapping>();
private ConstructorMapping constructorMapping;
private final Set<Parameter> unprocessedSourceParameters = new HashSet<Parameter>();
private NullValueMappingStrategyPrism nullValueMappingStrategy;
private SelectionParameters selectionParameters;
Expand Down Expand Up @@ -112,6 +117,14 @@ public Builder souceMethod(SourceMethod sourceMethod) {
for ( Parameter sourceParameter : method.getSourceParameters() ) {
unprocessedSourceParameters.add( sourceParameter );
}

if ( method.getResultType().getConstructor() != null ) {

@cliedeman cliedeman Aug 7, 2016

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Add the extra mapping targets

for (String property : method.getResultType().getConstructor().getPropertyNames()) {
unprocessedTargetProperties.put( property,
method.getResultType().getConstructor().getExecutableElement() );
}
}

existingVariableNames.addAll( method.getParameterNames() );
return this;
}
Expand Down Expand Up @@ -139,6 +152,8 @@ public BeanMappingMethod build() {
// map parameters without a mapping
applyParameterNameBasedMapping();

constructorMapping = createConstructorMapping();

// report errors on unmapped properties
reportErrorForUnmappedTargetPropertiesIfRequired();

Expand Down Expand Up @@ -181,6 +196,7 @@ public BeanMappingMethod build() {
return new BeanMappingMethod(
method,
propertyMappings,
constructorMapping,
factoryMethod,
mapNullToDefault,
resultType,
Expand Down Expand Up @@ -367,7 +383,12 @@ else if ( mapping.getJavaExpression() != null ) {
// remaining are the mappings without a 'source' so, 'only' a date format or qualifiers

if ( propertyMapping != null ) {
propertyMappings.add( propertyMapping );
if ( propertyMapping.isConstructorMapping() ) {

@cliedeman cliedeman Aug 7, 2016

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I want to remove this and isConstructorMapping

constructorPropertyMappings.put( propertyMapping.getName(), propertyMapping );
}
else {
propertyMappings.add( propertyMapping );
}
}
}
}
Expand Down Expand Up @@ -432,9 +453,9 @@ private void applyPropertyNameBasedMapping() {
newPropertyMapping = new PropertyMappingBuilder()
.mappingContext( ctx )
.sourceMethod( method )
.targetPropertyName( targetPropertyName )

@cliedeman cliedeman Aug 7, 2016

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some of the builder code introduced a set order dependency... Not sure if that is ok

.targetWriteAccessor( targetProperty.getValue() )
.targetReadAccessor( getTargetPropertyReadAccessor( targetPropertyName ) )
.targetPropertyName( targetPropertyName )
.sourceReference( sourceRef )
.formattingParameters( mapping != null ? mapping.getFormattingParameters() : null )
.selectionParameters( mapping != null ? mapping.getSelectionParameters() : null )
Expand Down Expand Up @@ -462,7 +483,13 @@ else if ( newPropertyMapping != null ) {
}

if ( propertyMapping != null ) {
propertyMappings.add( propertyMapping );
if ( propertyMapping.isConstructorMapping() ) {

@cliedeman cliedeman Aug 7, 2016

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Needs to be refactored

constructorPropertyMappings.put( propertyMapping.getName(), propertyMapping );
}
else {
propertyMappings.add( propertyMapping );
}

targetPropertyEntriesIterator.remove();
}
}
Expand Down Expand Up @@ -493,9 +520,9 @@ private void applyParameterNameBasedMapping() {
PropertyMapping propertyMapping = new PropertyMappingBuilder()
.mappingContext( ctx )
.sourceMethod( method )
.targetPropertyName( targetProperty.getKey() )

@cliedeman cliedeman Aug 7, 2016

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Builder invocation order dependency again

.targetWriteAccessor( targetProperty.getValue() )
.targetReadAccessor( getTargetPropertyReadAccessor( targetProperty.getKey() ) )
.targetPropertyName( targetProperty.getKey() )
.sourceReference( sourceRef )
.formattingParameters( mapping != null ? mapping.getFormattingParameters() : null )
.selectionParameters( mapping != null ? mapping.getSelectionParameters() : null )
Expand All @@ -511,6 +538,76 @@ private void applyParameterNameBasedMapping() {
}
}


private ConstructorMapping createConstructorMapping() {
Constructor constructor = method.getResultType().getConstructor();

if (constructor != null) {
LinkedHashMap<String, PropertyMapping> mappings = new LinkedHashMap<String, PropertyMapping>( );

for (String propertyName : constructor.getPropertyNames()) {
if ( constructorPropertyMappings.containsKey( propertyName ) ) {
mappings.put( propertyName, constructorPropertyMappings.get( propertyName ) );
}
else {
Parameter parameter = constructor.getParameter( propertyName );

@cliedeman cliedeman Aug 7, 2016

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Replace missing mappings with either a null assignment or the relevant primitive constant

PropertyMapping nullMapping = new ConstantMappingBuilder()
.mappingContext( ctx )
.sourceMethod( method )
.constantExpression( "\"" + getConstructorConstantExpression( parameter ) + "\"" )
.targetProperty( PropertyEntry.forTargetReference(
propertyName.split( "\\." ),
null,
constructor.getExecutableElement(),
parameter.getType() ) )
.targetPropertyName( propertyName )
.existingVariableNames( existingVariableNames )
.build();

mappings.put( propertyName, nullMapping );
}
}

return new ConstructorMapping( constructor, mappings );
}

return null;
}

private String getConstructorConstantExpression(Parameter parameter) {
if ( parameter.getType().isPrimitive() ) {
TypeKind kind = parameter.getType().getTypeMirror().getKind();

@cliedeman cliedeman Aug 7, 2016

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure if similar code exists somewhere in the codebase

if ( kind == TypeKind.BOOLEAN ) {

@sjaakd sjaakd Aug 16, 2016

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm. I just did a search on 0.0. Seems that we have similar code already in Type and IterableMappingMethod.. Perhaps already a refactoring point 😄

return "false";
}
else if ( kind == TypeKind.BYTE ) {
return "(byte) 0";
}
else if ( kind == TypeKind.SHORT ) {
return "(short) 0";
}
else if ( kind == TypeKind.INT ) {
return "0";
}
else if ( kind == TypeKind.LONG ) {
return "0L";
}
else if ( kind == TypeKind.FLOAT ) {
return "0.0";
}
else if ( kind == TypeKind.DOUBLE ) {
return "0.0";
}
else {
// This should not be possible ?
return "null";
}
}
else {
return "null";
}
}

private ExecutableElement getTargetPropertyReadAccessor( String propertyName ) {
return method.getResultType().getPropertyReadAccessors().get( propertyName );
}
Expand Down Expand Up @@ -562,6 +659,7 @@ private void reportErrorForUnmappedTargetPropertiesIfRequired() {

private BeanMappingMethod(SourceMethod method,
List<PropertyMapping> propertyMappings,
ConstructorMapping constructorMapping,
MethodReference factoryMethod,
boolean mapNullToDefault,
Type resultType,
Expand All @@ -571,6 +669,7 @@ private BeanMappingMethod(SourceMethod method,
NestedTargetObjects nestedTargetObjects ) {
super( method, existingVariableNames, beforeMappingReferences, afterMappingReferences );
this.propertyMappings = propertyMappings;
this.constructorMapping = constructorMapping;

// intialize constant mappings as all mappings, but take out the ones that can be contributed to a
// parameter mapping.
Expand All @@ -596,6 +695,10 @@ public List<PropertyMapping> getPropertyMappings() {
return propertyMappings;
}

public ConstructorMapping getConstructorMapping() {
return constructorMapping;
}

public List<PropertyMapping> getConstantMappings() {
return constantMappings;
}
Expand Down Expand Up @@ -774,7 +877,7 @@ private NestedTargetObjects(Set<LocalVariable> localVariables, Map<String, Strin
/**
* returns a local vaRriable name when relevant (so when not the 'parameter' targetBean should be used)
*
* @param targefetRef
* @param targetRef
* @return generated local variable name
*/
private String getLocalVariableName(TargetReference targetRef) {
Expand Down
53 changes: 53 additions & 0 deletions processor/src/main/java/org/mapstruct/ap/internal/model/ConstructorMapping.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,53 @@
/**
* Copyright 2012-2016 Gunnar Morling (http://www.gunnarmorling.de/)
* and/or other contributors as indicated by the @authors tag. See the
* copyright.txt file in the distribution for a full listing of all
* contributors.
*
* Licensed 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.mapstruct.ap.internal.model;

import org.mapstruct.ap.internal.model.common.Constructor;
import org.mapstruct.ap.internal.model.common.ModelElement;
import org.mapstruct.ap.internal.model.common.Type;

import java.util.Collection;
import java.util.Collections;
import java.util.LinkedHashMap;
import java.util.Set;

public class ConstructorMapping extends ModelElement {

private final Constructor constructor;
private final LinkedHashMap<String, PropertyMapping> propertyMappings;

public ConstructorMapping(Constructor constructor, LinkedHashMap<String, PropertyMapping> propertyMappings) {
this.constructor = constructor;
this.propertyMappings = propertyMappings;
}

public Constructor getConstructor() {
return constructor;
}

public Collection<PropertyMapping> getProperties() {
return propertyMappings.values();
}

@Override
public Set<Type> getImportTypes() {
// TODO
return Collections.emptySet();
}
}
Loading