Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
Original file line number Diff line number Diff line change
Expand Up @@ -190,6 +190,14 @@ else if ( !resultType.isAssignableTo( method.getResultType() ) ) {
Message.BEANMAPPING_NOT_ASSIGNABLE, resultType, method.getResultType()
);
}
else if ( !resultType.hasEmptyAccessibleContructor() ) {
ctx.getMessager().printMessage(
method.getExecutable(),
beanMappingPrism.mirror,
Message.GENERAL_NO_SUITABLE_CONSTRUCTOR,
resultType
);
}
}
else if ( !method.isUpdateMethod() && method.getReturnType().isAbstract() ) {
ctx.getMessager().printMessage(
Expand All @@ -198,6 +206,13 @@ else if ( !method.isUpdateMethod() && method.getReturnType().isAbstract() ) {
method.getReturnType()
);
}
else if ( !method.isUpdateMethod() && !method.getReturnType().hasEmptyAccessibleContructor() ) {
ctx.getMessager().printMessage(
method.getExecutable(),
Message.GENERAL_NO_SUITABLE_CONSTRUCTOR,
method.getReturnType()
);
}
}

sortPropertyMappingsByDependencies();
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -863,8 +863,7 @@ public boolean hasEmptyAccessibleContructor() {
hasEmptyAccessibleContructor = false;
List<ExecutableElement> constructors = ElementFilter.constructorsIn( typeElement.getEnclosedElements() );
for ( ExecutableElement constructor : constructors ) {
if ( (constructor.getModifiers().contains( Modifier.PUBLIC )
|| constructor.getModifiers().contains( Modifier.PROTECTED ) )
if ( !constructor.getModifiers().contains( Modifier.PRIVATE )
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

How do we know here that the constructor is actually accessible from the package of the mapper?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

That is the case, we don't really know. And I think it is difficult to check this because the test is later on. Actually one of our test have package protected classes and package protected static classes. The modifiers are then empty (and that is why it started failing).

I am not sure if it is worth it to add more complex checks? It will create a compile problem in the generated file in any case.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Sure, that’s okay as well.

&& constructor.getParameters().isEmpty() ) {
hasEmptyAccessibleContructor = true;
break;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -386,17 +386,6 @@ public boolean isEnumMapping() {
return isEnumMapping;
}

public boolean isBeanMapping() {
if ( isBeanMapping == null ) {
isBeanMapping = !isIterableMapping()
&& !isMapMapping()
&& !isEnumMapping()
&& !isValueMapping()
&& !isStreamMapping();
}
return isBeanMapping;
}

/**
* The default enum mapping (no mappings specified) will from now on be handled as a value mapping. If there
* are any @Mapping / @Mappings defined on the method, then the deprecated enum behavior should be executed.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -23,16 +23,17 @@
import java.util.List;
import java.util.Set;

import javax.lang.model.element.AnnotationMirror;
import javax.lang.model.type.DeclaredType;

import org.mapstruct.ap.internal.model.common.Parameter;
import org.mapstruct.ap.internal.model.common.Type;
import org.mapstruct.ap.internal.model.common.TypeFactory;
import org.mapstruct.ap.internal.prism.CollectionMappingStrategyPrism;
import org.mapstruct.ap.internal.prism.InheritInverseConfigurationPrism;
import org.mapstruct.ap.internal.util.Executables;
import org.mapstruct.ap.internal.util.FormattingMessager;
import org.mapstruct.ap.internal.util.Message;

import org.mapstruct.ap.internal.util.Strings;
import org.mapstruct.ap.internal.util.accessor.Accessor;

Expand Down Expand Up @@ -161,7 +162,7 @@ && matchesSourceOrTargetParameter( segments[0], parameter, reverseSourceParamete

if ( !foundEntryMatch && errorMessage != null) {
// This is called only for reporting errors
errorMessage.report();
errorMessage.report( isReverse );
}

// foundEntryMatch = isValid, errors are handled here, and the BeanMapping uses that to ignore
Expand Down Expand Up @@ -364,14 +365,21 @@ private MappingErrorMessage(Mapping mapping, SourceMethod method, FormattingMess
this.messager = messager;
}

abstract void report();
abstract void report(boolean isReverse);

protected void printErrorMessage(Message message, Object... args) {
protected void printErrorMessage(Message message, boolean isReverse, Object... args) {
Object[] errorArgs = new Object[args.length + 2];
errorArgs[0] = mapping.getTargetName();
errorArgs[1] = method.getResultType();
System.arraycopy( args, 0, errorArgs, 2, args.length );
messager.printMessage( method.getExecutable(), mapping.getMirror(), mapping.getSourceAnnotationValue(),
AnnotationMirror annotationMirror = mapping.getMirror();
if ( isReverse ) {
InheritInverseConfigurationPrism reversePrism = InheritInverseConfigurationPrism.getInstanceOn(
method.getExecutable() );

annotationMirror = reversePrism == null ? annotationMirror : reversePrism.mirror;
}
messager.printMessage( method.getExecutable(), annotationMirror, mapping.getSourceAnnotationValue(),
message, errorArgs
);
}
Expand All @@ -384,8 +392,8 @@ private NoWriteAccessorErrorMessage(Mapping mapping, SourceMethod method, Format
}

@Override
public void report() {
printErrorMessage( Message.BEANMAPPING_PROPERTY_HAS_NO_WRITE_ACCESSOR_IN_RESULTTYPE );
public void report(boolean isReverse) {
printErrorMessage( Message.BEANMAPPING_PROPERTY_HAS_NO_WRITE_ACCESSOR_IN_RESULTTYPE, isReverse );
}
}

Expand All @@ -404,7 +412,7 @@ private NoPropertyErrorMessage(Mapping mapping, SourceMethod method, FormattingM
}

@Override
public void report() {
public void report(boolean isReverse) {

Set<String> readAccessors = nextType.getPropertyReadAccessors().keySet();
String mostSimilarProperty = Strings.getMostSimilarWord(
Expand All @@ -415,7 +423,11 @@ public void report() {
List<String> elements = new ArrayList<String>( Arrays.asList( entryNames ).subList( 0, index ) );
elements.add( mostSimilarProperty );

printErrorMessage( Message.BEANMAPPING_UNKNOWN_PROPERTY_IN_RESULTTYPE, Strings.join( elements, "." ) );
printErrorMessage(
Message.BEANMAPPING_UNKNOWN_PROPERTY_IN_RESULTTYPE,
isReverse,
Strings.join( elements, "." )
);
}
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -517,15 +517,6 @@ private MappingOptions getInverseMappingOptions(List<SourceMethod> rawMethods, S

if ( reversePrism != null ) {

// is there a suitable constructor
if ( method.isBeanMapping()
&& !method.isUpdateMethod()
&& !method.getReturnType().isCollectionOrMapType()
&& !method.getReturnType().hasEmptyAccessibleContructor() ) {
reportErrorWhenNoSuitableConstrutor( method, reversePrism );
return null;
}

// method is configured as being reverse method, collect candidates
List<SourceMethod> candidates = new ArrayList<SourceMethod>();
for ( SourceMethod oneMethod : rawMethods ) {
Expand Down Expand Up @@ -689,17 +680,6 @@ private void reportErrorWhenAmbigousReverseMapping(List<SourceMethod> candidates
}
}

private void reportErrorWhenNoSuitableConstrutor( SourceMethod method,
InheritInverseConfigurationPrism reversePrism ) {

messager.printMessage( method.getExecutable(),
reversePrism.mirror,
Message.INHERITINVERSECONFIGURATION_NO_SUITABLE_CONSTRUCTOR,
reversePrism.name()

);
}

private void reportErrorWhenSeveralNamesMatch(List<SourceMethod> candidates, SourceMethod method,
InheritInverseConfigurationPrism reversePrism) {

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -89,6 +89,7 @@ public enum Message {
GENERAL_VALID_DATE( "Given date format \"%s\" is valid.", Diagnostic.Kind.NOTE ),
GENERAL_INVALID_DATE( "Given date format \"%s\" is invalid. Message: \"%s\"." ),
GENERAL_NOT_ALL_FORGED_CREATED( "Internal Error in creation of Forged Methods, it was expected all Forged Methods to finished with creation, but %s did not" ),
GENERAL_NO_SUITABLE_CONSTRUCTOR( "%s does not have an accessible empty constructor." ),

RETRIEVAL_NO_INPUT_ARGS( "Can't generate mapping method with no input arguments." ),
RETRIEVAL_DUPLICATE_MAPPING_TARGETS( "Can't generate mapping method with more than one @MappingTarget parameter." ),
Expand All @@ -111,7 +112,6 @@ public enum Message {
INHERITINVERSECONFIGURATION_DUPLICATES( "Several matching inverse methods exist: %s(). Specify a name explicitly." ),
INHERITINVERSECONFIGURATION_INVALID_NAME( "None of the candidates %s() matches given name: \"%s\"." ),
INHERITINVERSECONFIGURATION_DUPLICATE_MATCHES( "Given name \"%s\" matches several candidate methods: %s." ),
INHERITINVERSECONFIGURATION_NO_SUITABLE_CONSTRUCTOR( "There is no suitable result type constructor for reversing this method." ),
INHERITINVERSECONFIGURATION_NO_NAME_MATCH( "Given name \"%s\" does not match the only candidate. Did you mean: \"%s\"." ),
INHERITCONFIGURATION_DUPLICATES( "Several matching methods exist: %s(). Specify a name explicitly." ),
INHERITCONFIGURATION_INVALIDNAME( "None of the candidates %s() matches given name: \"%s\"." ),
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -76,7 +76,11 @@ public void factoryMethodWithSourceParamIsChosen() {
+ " .*TargetB .*TargetFactories\\.createTargetB\\(.*SourceB source,"
+ " @TargetType .*Class<.*TargetB> clazz\\),"
+ " .*TargetB .*TargetFactories\\.createTargetB\\(@TargetType java.lang.Class<.*TargetB> clazz\\),"
+ " .*TargetB .*TargetFactories\\.createTargetB\\(\\).")
+ " .*TargetB .*TargetFactories\\.createTargetB\\(\\)."),
@Diagnostic(type = ErroneousIssue1242MapperMultipleSources.class,
kind = javax.tools.Diagnostic.Kind.ERROR,
line = 33,
messageRegExp = ".*TargetB does not have an accessible empty constructor\\.")
})
public void ambiguousMethodErrorForTwoFactoryMethodsWithSourceParam() {
}
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,36 @@
/**
* Copyright 2012-2017 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.test.bugs._1283;

import org.mapstruct.InheritInverseConfiguration;
import org.mapstruct.Mapper;
import org.mapstruct.Mapping;

/**
* @author Filip Hrisafov
*/
@Mapper
public interface ErroneousInverseTargetHasNoSuitableConstructorMapper {

@Mapping(target = "target", source = "source")
Target fromSource(Source source);

@InheritInverseConfiguration
Source fromTarget(Target target);
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,32 @@
/**
* Copyright 2012-2017 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.test.bugs._1283;

import org.mapstruct.Mapper;
import org.mapstruct.Mapping;

/**
* @author Filip Hrisafov
*/
@Mapper
public interface ErroneousTargetHasNoSuitableConstructorMapper {

@Mapping(target = "source", source = "target")
Source fromTarget(Target target);
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,70 @@
/**
* Copyright 2012-2017 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.test.bugs._1283;

import org.junit.Test;
import org.junit.runner.RunWith;
import org.mapstruct.ap.testutil.IssueKey;
import org.mapstruct.ap.testutil.WithClasses;
import org.mapstruct.ap.testutil.compilation.annotation.CompilationResult;
import org.mapstruct.ap.testutil.compilation.annotation.Diagnostic;
import org.mapstruct.ap.testutil.compilation.annotation.ExpectedCompilationOutcome;
import org.mapstruct.ap.testutil.runner.AnnotationProcessorTestRunner;

/**
* @author Filip Hrisafov
*/
@IssueKey("1283")
@RunWith(AnnotationProcessorTestRunner.class)
@WithClasses({
Source.class,
Target.class
})
public class Issue1283Test {

@Test
@WithClasses(ErroneousInverseTargetHasNoSuitableConstructorMapper.class)
@ExpectedCompilationOutcome(
value = CompilationResult.FAILED,
diagnostics = {
@Diagnostic(type = ErroneousInverseTargetHasNoSuitableConstructorMapper.class,
kind = javax.tools.Diagnostic.Kind.ERROR,
line = 35L,
messageRegExp = ".*\\._1283\\.Source does not have an accessible empty constructor"
)
}
)
public void inheritInverseConfigurationReturnTypeHasNoSuitableConstructor() {
}

@Test
@WithClasses(ErroneousTargetHasNoSuitableConstructorMapper.class)
@ExpectedCompilationOutcome(
value = CompilationResult.FAILED,
diagnostics = {
@Diagnostic(type = ErroneousTargetHasNoSuitableConstructorMapper.class,
kind = javax.tools.Diagnostic.Kind.ERROR,
line = 31L,
messageRegExp = ".*\\._1283\\.Source does not have an accessible empty constructor"
)
}
)
public void returnTypeHasNoSuitableConstructor() {
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,39 @@
/**
* Copyright 2012-2017 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.test.bugs._1283;

/**
* @author Filip Hrisafov
*/
public class Source {

private String source;

public Source(String source) {
this.source = source;
}

public String getSource() {
return source;
}

public void setSource(String source) {
this.source = source;
}
}
Loading