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.

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

Emit warnings for precision loss #5

Open
gunnarmorling opened this Issue Mar 2, 2013 · 15 comments

Comments

Projects
None yet
5 participants
@gunnarmorling
Member

gunnarmorling commented Mar 2, 2013

This follows up on #1; If built-in conversion causes a possible precision loss (e.g. from long to int), a warning or error should be created. If the concerned attribute is mapped via @Mapping the marker should be at the annotation, otherwise at the mapping method(s).

It might be a good idea to make it configurable either globally and or via an attribute in @Mapper whether the error kind should be WARNING or ERROR.

@lesaint

This comment has been minimized.

Show comment
Hide comment
@lesaint

lesaint Nov 5, 2014

Hi @gunnarmorling,

I am wondering here, what are the options for the developer once an error or a warning was raised?

Either the developer is ok with the precision-loss, then how does she get rid of the warning/error?
Either the developer is not ok with it, then how does she fix it?

I don't know MapStruct all that well so I might be missing an obvious answer.

lesaint commented Nov 5, 2014

Hi @gunnarmorling,

I am wondering here, what are the options for the developer once an error or a warning was raised?

Either the developer is ok with the precision-loss, then how does she get rid of the warning/error?
Either the developer is not ok with it, then how does she fix it?

I don't know MapStruct all that well so I might be missing an obvious answer.

@sjaakd

This comment has been minimized.

Show comment
Hide comment
@sjaakd

sjaakd Nov 5, 2014

Contributor

@Lasaint, mapping methods overrule type conversions and builtinmehods . So if you detect a precision loss, you can decide to use a handwritten mapping method.

Contributor

sjaakd commented Nov 5, 2014

@Lasaint, mapping methods overrule type conversions and builtinmehods . So if you detect a precision loss, you can decide to use a handwritten mapping method.

@lesaint

This comment has been minimized.

Show comment
Hide comment
@lesaint

lesaint Nov 5, 2014

ok, thanks for the reply @sjaakd

lesaint commented Nov 5, 2014

ok, thanks for the reply @sjaakd

@gunnarmorling

This comment has been minimized.

Show comment
Hide comment
@gunnarmorling

gunnarmorling Nov 5, 2014

Member

Either the developer is ok with the precision-loss, then how does she get rid of the warning/error?

Hi @lesaint, we haven't thought about it in detail, but I could envision the following

  • By default, raise a build time error (or warning; there'd probably be a global setting for this) in case the type of a target property may not carry all possible values from the source property
  • For the (likely rare cases) where you are ok with a possible value loss, there could be a new attribute on @Mapping such as acceptPotentialValueLoss()=true. This would allow to suppress the error on a per-case basis (which I think is what makes most sense as the situation almost always is problematic). Alternatively, you could define a custom method such as byte intToByte(int i) which then would be applied as @sjaakd is saying, but that'd be of more global nature.

Either the developer is not ok with it, then how does she fix it?

They'd have to change the type of the target property to accept the source property without loss.

Member

gunnarmorling commented Nov 5, 2014

Either the developer is ok with the precision-loss, then how does she get rid of the warning/error?

Hi @lesaint, we haven't thought about it in detail, but I could envision the following

  • By default, raise a build time error (or warning; there'd probably be a global setting for this) in case the type of a target property may not carry all possible values from the source property
  • For the (likely rare cases) where you are ok with a possible value loss, there could be a new attribute on @Mapping such as acceptPotentialValueLoss()=true. This would allow to suppress the error on a per-case basis (which I think is what makes most sense as the situation almost always is problematic). Alternatively, you could define a custom method such as byte intToByte(int i) which then would be applied as @sjaakd is saying, but that'd be of more global nature.

Either the developer is not ok with it, then how does she fix it?

They'd have to change the type of the target property to accept the source property without loss.

@gunnarmorling gunnarmorling modified the milestones: 1.0.0.RC1, 1.0.0.Beta3 Nov 19, 2014

@gunnarmorling gunnarmorling modified the milestones: 1.0.0.RC1, 1.0.0.Beta4 Mar 1, 2015

@gunnarmorling gunnarmorling modified the milestones: 1.1-next, 1.0.0.CR1 May 29, 2015

@GuiSim

This comment has been minimized.

Show comment
Hide comment
@GuiSim

GuiSim Jan 17, 2017

Any progress on this feature? I think it would be a great addition to MapStruct.

GuiSim commented Jan 17, 2017

Any progress on this feature? I think it would be a great addition to MapStruct.

@agudian

This comment has been minimized.

Show comment
Hide comment
@agudian

agudian Jan 19, 2017

Member

No one worked on this, yet. Would you maybe like to give it a try?

Member

agudian commented Jan 19, 2017

No one worked on this, yet. Would you maybe like to give it a try?

@gunnarmorling

This comment has been minimized.

Show comment
Hide comment
@gunnarmorling

gunnarmorling Jan 19, 2017

Member

As @agudian is saying, any help with this would be welcome. I'd be very glad to see it in :) We can give directions to get started if needed.

Member

gunnarmorling commented Jan 19, 2017

As @agudian is saying, any help with this would be welcome. I'd be very glad to see it in :) We can give directions to get started if needed.

@GuiSim

This comment has been minimized.

Show comment
Hide comment
@GuiSim

GuiSim Jan 19, 2017

Thanks for the proposition. We're still unsure if we're going to go ahead with MapStruct but if we do, I'd love to contribute back to the project as I think we'll need this feature.

I'll update if we choose MapStruct!

GuiSim commented Jan 19, 2017

Thanks for the proposition. We're still unsure if we're going to go ahead with MapStruct but if we do, I'd love to contribute back to the project as I think we'll need this feature.

I'll update if we choose MapStruct!

@gunnarmorling

This comment has been minimized.

Show comment
Hide comment
@gunnarmorling

gunnarmorling Jan 19, 2017

Member

Sure; if you have any questions during your decision process, let us know, we'll be glad to help as good as we can.

Member

gunnarmorling commented Jan 19, 2017

Sure; if you have any questions during your decision process, let us know, we'll be glad to help as good as we can.

@gunnarmorling

This comment has been minimized.

Show comment
Hide comment
@gunnarmorling

gunnarmorling Feb 17, 2017

Member

Hey @GuiSim, curious which route you did end up following? If you didn't go for MapStruct in the end, do you mind me asking what spoke against it? Always eager to learn from people's thinking so we can improve the stuff. Thanks!

Member

gunnarmorling commented Feb 17, 2017

Hey @GuiSim, curious which route you did end up following? If you didn't go for MapStruct in the end, do you mind me asking what spoke against it? Always eager to learn from people's thinking so we can improve the stuff. Thanks!

@GuiSim

This comment has been minimized.

Show comment
Hide comment
@GuiSim

GuiSim Feb 17, 2017

We didn't go with any mapping technology for now.. we're still writing what we call "translators" to convert from one type to another.

It's still a tedious process but we're slowly removing intermediary objects. We realize that this increases coupling but we feel like the pros outweigh the cons.

GuiSim commented Feb 17, 2017

We didn't go with any mapping technology for now.. we're still writing what we call "translators" to convert from one type to another.

It's still a tedious process but we're slowly removing intermediary objects. We realize that this increases coupling but we feel like the pros outweigh the cons.

spoerri pushed a commit to spoerri/mapstruct that referenced this issue Nov 26, 2017

# This is a combination of 9 commits.
# This is the 1st commit message:
#1269 use update methods for different sources for nested targets

# This is the commit message #2:

[maven-release-plugin] prepare release 1.2.0.CR2

# This is the commit message #3:

[maven-release-plugin] prepare for next development iteration

# This is the commit message #4:

Switch to OpenJDK 7 on Travis (Oracle Java 7 is no longer present)

# This is the commit message #5:

Run build in VM (fixes issues when build is killed by Travis containers)

# This is the commit message #6:

Fix a few errors in reference guide and readme

# This is the commit message #7:

#1283 Handle no suitable empty constructor during BeanMappingMethod creation

# This is the commit message #8:

#1281 Do not depend on deprecated sonatype parent

# This is the commit message #9:

#1273 Fix for NullValueMappingStrategy#RETURN_DEFAULT when using collections

When mapping a collection using NullValueMappingStrategy#RETURN_DEFAULT and the source is null, the target will be an empty collection.

# This is the commit message #10:

Adding Kevin to copyright.txt

# This is the commit message #1:

#744 Improve support for Java 9

When compilig with Java 9 and and source version 1.8 Elements#getTypeElement(CharSequence) returns the types from all modules (such as java.xml.bind or java.xml.datatype).
However if the required modules are not added the classes cannot be used. Therefore, apart from using the Elements we are also checking if the class is also there.

If source version 9 is used then Elements#getTypeElement(CharSequence) works correctly and does not return the types if the modules are not there

# This is the commit message #2:

#1304 Add thrown exceptions to the generated nested mapping methods

# This is the commit message #3:

Add Darren to copyright.txt

# This is the commit message #4:

[maven-release-plugin] prepare release 1.2.0.Final

# This is the commit message #5:

[maven-release-plugin] prepare for next development iteration

# This is the commit message #6:

Update latest stable badge with 1.2.0.Final

# This is the commit message #7:

#1297 Add IntelliJ Formatter to CONTRIBUTING.md

# This is the commit message #8:

#571 Add Constructor Injection for Annotation Based Component Model

- Allow configuring of injection strategy (Constructor / Field)
- Default injection strategy is Field

# This is the commit message #9:

#571 Add test for default injection strategy

# This is the commit message #10:

#1312 Change MapStruct Version in README to latest 1.2.0.Final

# This is the commit message #1:

#610 Add support for unmappedSourcePolicy

# This is the commit message #2:

#610 remove unmapped source policy processor option

# This is the commit message #3:

610 unmappedSourcePolicy with tests

# This is the commit message #4:

#610 test MapperConfig

# This is the commit message #5:

initial attempt at support for ignoring an unmapped source field

# This is the commit message #6:

revert what pull -Xtheirs didn't

# This is the commit message #7:

revert what pull -Xtheirs didn't

# This is the commit message #8:

revert what pull -Xtheirs didn't

# This is the commit message #9:

revert what pull -Xtheirs didn't

# This is the commit message #10:

revert what pull -Xtheirs didn't

# This is the commit message #1:

revert what pull -Xtheirs didn't

# This is the commit message #2:

revert what pull -Xtheirs didn't

# This is the commit message #3:

revert what pull -Xtheirs didn't

# This is the commit message #4:

Update BeanMappingMethod.java

# This is the commit message #5:

Update IgnoreMapper.java

# This is the commit message #6:

Update UnmappedSourceTest.java

# This is the commit message #7:

Update Mapping.java

# This is the commit message #8:

Update Mapping.java

# This is the commit message #9:

Update Mapping.java
@sjaakd

This comment has been minimized.

Show comment
Hide comment
@sjaakd

sjaakd Apr 14, 2018

Contributor

i'll pick this one up. I think we have to have a strategy as well (just like for unmapped target properties):

  • fail
  • warn
    (as is stated in the initial descriptions 😄
Contributor

sjaakd commented Apr 14, 2018

i'll pick this one up. I think we have to have a strategy as well (just like for unmapped target properties):

  • fail
  • warn
    (as is stated in the initial descriptions 😄

@sjaakd sjaakd self-assigned this Apr 14, 2018

@sjaakd

This comment has been minimized.

Show comment
Hide comment
@sjaakd

sjaakd Apr 16, 2018

Contributor

There are actually 2 interesting things, which are not the same.

  1. narrowing. In general, loss of precision (the stuff after the dot when converting from float to int) or loss of range (trying to fit a int in a short).
  2. widening.. In general not a problem , but could lead to precision loss. In case of integer or long to float conversion, or long to double.

I think we need to distinguish these cases.
So:

  • ReportingPolicy conversionNarrowingPolicy;
  • ReportingPolicy conversionPrecisionLossPolicy;

Note: although not orthogonal, I would propose to only apply the conversionPrecisionLossPolicy to widening cases.

Contributor

sjaakd commented Apr 16, 2018

There are actually 2 interesting things, which are not the same.

  1. narrowing. In general, loss of precision (the stuff after the dot when converting from float to int) or loss of range (trying to fit a int in a short).
  2. widening.. In general not a problem , but could lead to precision loss. In case of integer or long to float conversion, or long to double.

I think we need to distinguish these cases.
So:

  • ReportingPolicy conversionNarrowingPolicy;
  • ReportingPolicy conversionPrecisionLossPolicy;

Note: although not orthogonal, I would propose to only apply the conversionPrecisionLossPolicy to widening cases.

@sjaakd

This comment has been minimized.

Show comment
Hide comment
@sjaakd

This comment has been minimized.

Show comment
Hide comment
@sjaakd

sjaakd Apr 21, 2018

Contributor

The term JDK uses for this error is "possibly lossy conversion from t1 to t2" .. for what the JLS calls narrowing.
For the widening case + precision loss there is no warning.. Just as the JLS states.

Contributor

sjaakd commented Apr 21, 2018

The term JDK uses for this error is "possibly lossy conversion from t1 to t2" .. for what the JLS calls narrowing.
For the widening case + precision loss there is no warning.. Just as the JLS states.

sjaakd added a commit to sjaakd/mapstruct that referenced this issue May 21, 2018

sjaakd added a commit to sjaakd/mapstruct that referenced this issue May 21, 2018

sjaakd added a commit to sjaakd/mapstruct that referenced this issue May 21, 2018

sjaakd added a commit to sjaakd/mapstruct that referenced this issue May 22, 2018

sjaakd added a commit to sjaakd/mapstruct that referenced this issue May 27, 2018

sjaakd added a commit to sjaakd/mapstruct that referenced this issue May 27, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment