Lombok: Make Getters and Setters on Date (or other mutable objects) set/return new copy.

[ikocev]

Let's consider the following class:

@Getter
@Setter
public class SomeClassWithDates {
    private Date start;
    private Date end;
}

This will result in normal getters and setters. Then somewhere else in the code:

SomeClassWithDates obj = new SomeClassWithDates();
Date now = new Date();
obj.setStart(now);
.....
now.setYear(...);
//now the obj.getStart() will be also modified

Proper implementation will be instead of returning get and set directly on the object, call new Date on the passed argument in the setter:

public void setStart(Date date) {
   this.start = new Date(date.getTime());
}

public Date getStart() {
   return new Date(this.start.getTime());
}

My best guess is that this should at least work for Java Classes like Date (since it needs to know which constructor to call and with which data), but maybe some other annotation can be added. An idea will be also to have an argument in @Getter and @Setter for example:
@Setter(new = true)
And possible a method reference to the constructor:
@Setter(new = true, with = Date::getTime)

I am not sure if there was already an issue similar to this one. What do you guys think?

[randakar]

Date being mutable like that is a bug. That class should be treated as
deprecated, and replaced with ZonedDateTime or LocalDateTime instances
wherever possible.
This is not a lombok problem, imho.

On Wed, Aug 1, 2018 at 11:47 AM, Ivica [e-mail] wrote:

Let's consider the following class:

@Getter
@Setter
public class SomeClassWithDates {
private Date start;
private Date end;
}

This will result in normal getters and setters. Then somewhere else in the
code:

SomeClassWithDates obj = new SomeClassWithDates();
Date now = new Date();
obj.setStart(now);
.....
now.setYear(...);
//now the obj.getStart() will be also modified

Proper implementation will be instead of returning get and set directly on
the object, call new Date on the passed argument in the setter:

public void setStart(Date date) {
this.start = new Date(date.getTime());
}

public Date getStart() {
return new Date(this.start.getTime());
}

My best guess is that this should at least work for Java Classes like Date
(since it needs to know which constructor to call and with which data), but
maybe some other annotation can be added. An idea will be also to have an
argument in @getter https://github.com/getter and @setter
https://github.com/setter for example:
@Setter(new = true)
And possible a method reference to the constructor:
@Setter(new = true, with = Date::getTime)

I am not sure if there was already an issue similar to this one. What do
you guys think?

—
You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub
https://github.com/rzwitserloot/lombok/issues/1799, or mute the thread
https://github.com/notifications/unsubscribe-auth/AAKCRZIo0K7Ngn0cSZItksEEOCCBHaf_ks5uMXlEgaJpZM4VqL2q
.

--
"Don't only practice your art, but force your way into it's secrets, for it
and knowledge can raise men to the divine."
-- Ludwig von Beethoven

[victorwss]

Ok, let's suppose that your field, getter and setter is StringBuilder. This means that it is inherently mutable. Making a copy of it before returning will probably not work as intended.

Maybe something like @Getter(makeCopy = "clone") could do it? The copy would be created by calling the clone() method (after checking out for nulls) and casting the result into the same return type of the getter. If clone() method is not available or declares to throws the nasty CloneNotSupportedException, just allow javac complain with a compile error.

Alternatively you could add @Getter(makeCopy = "yourOwnCopyMethod") and it would call value.yourOwnCopyMethod.

If you can't modify the target class and it does not provide a method for cloning/copying, maybe a @Getter(makeCopy = "yourOwnCopyMethod", staticCopy = true) to make it call a static yourOwnCopyMethod(value); method instead?

[Maaartinus]

This is not a lombok problem, imho.

@randakar Right, but there could be a Lombok solution. I surely agree that Date is obsolete and shouldn't be user anymore, but there are things like byte[] having no replacement.

---

If clone() method is not available or declares to throws the nasty CloneNotSupportedException

@victorwss org.bouncycastle is full of Cloneable classes declaring for whatever stupid reason to throw. In org.apache.http and quite a few more packages, there's more of it.

So it'd be better to catch and wrap.... but this would fail for methods not declared to throw. I doubt if Lombok can tell which is which without resolution so maybe a dirty hack like

try {
    if (Boolean.FALSE.booleanValue()) throw new CloneNotSupportedException();
    return (MyFieldType) myField.clone();
} catch (CloneNotSupportedException e) {
     throw new RuntimeException(e);
}

or an additional argument like wrapCloneNotSupportedException=boolean may be needed. Or just catch (Exception e), which is considered dirty for reason I find mostly wrong.

---

I'd personally be happy with something like @Getter(cloner=MyCloner.class), where it'd be the user's responsibility to write their own cloner class and ensuring it has a matching static method for everything it gets used for. Writing the class should be pretty trivial

public class MyCloner {
    public static Date clone(Date input) {
        return (Date) input.clone();
    }
    public static byte[] clone(byte[] input) {
        return input.clone();
    }
    // overload with all needed variants
}

It's hacky, but very simple and practical. It could lead to compile-time errors, but this is no big deal. Actually, it's no deal at all, as you'd get a message like "The method clone(....) in the type MyCloner is not applicable for the arguments (byte[])".

The annotation would accept any class and Lombok would just handle null and otherwise do a trivial static method call, relying on overload resolution. For Lombok itself, there's no way to enforce that a proper class gets used. This sounds worrying, but it's no different from what happens when writing a plain java statement.

It handles deep cloning, cloning with bells and whistles and cloning with whatever, simply by using @Getter(cloner=MyDeepCloner.class) (sure, the user has to implement it, but that's a matter of minutes for simple cases and some googling otherwise; anyway, not something, Lombok could perfectly solve itself).

Actually, I'd call it transform instead, as it could be used also like @Setter(transform=MyNotEmptyStringChecker.class), partially solving a different issue.

[randakar]

Fair, Lombok could do something like that.

There is of course the Clonable interface as well. Classes implementing
that should be easy to support - but if won't cover Date, let alone byte
arrays.

I'm thinking the generic version, where you hook a Transformer into setters
and getters sounds interesting. It will have limitations - don't think we
should try to change the method signature with that, for instance - but I
can see uses for this.

Nonetheless, I have doubts. I often see cases where people write code that
calls the same getter multiple times for no good reason. Getters are
supposed to be cheap, if they start involving deep clones of potentially
complex objects, they won't be.

Op za 15 sep. 2018 05:40 schreef Maaartinus [e-mail]:

This is not a lombok problem, imho.

@randakar https://github.com/randakar Right, but there could be a
Lombok solution. I surely agree that Date is obsolete and shouldn't be

user anymore, but there are things like byte[] having no replacement.

If clone() method is not available or declares to throws the nasty
CloneNotSupportedException

@victorwss https://github.com/victorwss org.bouncycastle is full of
Cloneable classes declaring for whatever stupid reason to throw. In
org.apache.http and quite a few more packages, there's more of it.

So it'd be better to catch and wrap.... but this would fail for methods
not declared to throw. I doubt if Lombok can tell which is which without
resolution so maybe a dirty hack like

try {
if (Boolean.FALSE.booleanValue()) throw new CloneNotSupportedException();
return (MyFieldType) myField.clone();
} catch (CloneNotSupportedException e) {
throw new RuntimeException(e);
}

or an additional argument like wrapCloneNotSupportedException=boolean may
be needed. Or just catch (Exception e), which is considered dirty for

reason I find mostly wrong.

I'd personally be happy with something like @Getter(cloner=MyCloner.class),
where it'd be the user's responsibility to write their own cloner class
and ensuring it has a matching static method for everything it gets
used for. Writing the class should be pretty trivial

public class MyCloner {
public static Date clone(Date input) {
return (Date) input.clone();
}
public static byte[] clone(byte[] input) {
return input.clone();
}
// overload with all needed variants
}

It's hacky, but very simple and practical. It could lead to compile-time
errors, but this is no big deal. Actually, it's no deal at all, as you'd
get a message like "The method clone(....) in the type MyCloner is not
applicable for the arguments (byte[])".

The annotation would accept any class and Lombok would just handle null
and otherwise do a trivial static method call, relying on overload
resolution. For Lombok itself, there's no way to enforce that a proper
class gets used. This sounds worrying, but it's no different from what
happens when writing a plain java statement.

It handles deep cloning, cloning with bells and whistles and cloning with
whatever, simply by using @Getter(cloner=MyDeepCloner.class) (sure, the
user has to implement it, but that's a matter of minutes for simple cases
and some googling otherwise; anyway, not something, Lombok could perfectly
solve itself).

Actually, I'd call it transform instead, as it could be used also like
@Setter(transform=MyNotEmptyStringChecker.class), partially solving a
different issue.

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
https://github.com/rzwitserloot/lombok/issues/1799#issuecomment-421527770,
or mute the thread
https://github.com/notifications/unsubscribe-auth/AAKCRV7cPynr_tkj_PvU0-xskYgSYXJGks5ubHamgaJpZM4VqL2q
.

[ikocev]

Back to my own issue, I agree with your first response that, having a mutable objects in such a cases is deviant. Although Lombok indeed can do something about it (already explained by others above), I think the focus should be not supporting non-standard/buggy approaches. Users should seek for what might be the real problem, because having a mutable object could lead to many other issues.

In the current cases (the one that I explained in the beginning), custom implementation will be better approach in my opinion now, in cases where immutable object cannot be provided.

[Jiehong]

What about the cases of List and Map that suffer the same issue? (And it also impacts the @AllArgsConstructor as well)