Skip to content
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

[FEATURE] Provide option to use getters for @Wither #3128

Open
soberich opened this issue Mar 2, 2022 · 1 comment
Open

[FEATURE] Provide option to use getters for @Wither #3128

soberich opened this issue Mar 2, 2022 · 1 comment
Labels
discuss Feature proposal that needs to be debated by stakeholders first before PRs are accepted.

Comments

@soberich
Copy link

soberich commented Mar 2, 2022

Describe the feature
There is a case where frameworks like, for example, Micronaut, trying using wither methods to create a copy of near-immutable instance to, for example, set DB assigned value (like id) to property. Unfortunately in case of "lazy" getter ("lazy" getter in common sense, not relevant to Lombok's feature), which ever only executed once and subsequent invocation simply check if field non-null. So, initially, backing field is null until getter first called.

Unlike @EqualsAndHashCode and @ToString - @With uses fields, which makes "lazy" getter not being executed, so copied state is incomplete from what is supposed to be.

Hope/seems it's easy enough to create maybe config an option @EqualsAndHashCode and @ToString alike to force using getters.

Describe the target audience
Anyone with something like this in pojo

@With
@Value
public class Pojo {
    Long id;

    Long timestamp;

    public Long getTimestamp() {
        return timestamp != null ? timestamp : System.currentTimeMillis();
    }

    // produces
    // public Pojo withId(Long id) {                      ↓↓↓ HERE!! ↓↓↓
    //     return this.id == id ? this : new Pojo(id, this.timestamp/*oops, `null` will be copied*/);
    // }
}
@rzwitserloot
Copy link
Collaborator

I get the need. I've been thinking about this one for a few days.

There are 2 big problems with what you want:

  • ToString, EqualsAndHashCode and friends default to the opposite behaviour of wither: They by default invoke getters and have a setting to tell them explicitly not to do that, whereas @With and @WithBy by default do direct field access and you're asking for a feature to explicitly ask them to generate calls to the getter instead. That's ugly (ugly doesn't mean: "We will not do this", just - increases the cost side of the cost/benefit analysis). We can't change how either feature works (at least, not without extreme caution) because all of this stuff is in the core package (not in experimental, where the bar to break backwards compat is much lower).
  • More crucially though, I do not think calling the getter is correct except in a truly tiny number of situations, in sharp contrast to ToString and EqualsAndHashCode!

Your example doesn't make sense to me

Just looking at your example without further context, I think what you want is just flat out wrong, and wrong in a nasty way (that is: A way that is really hard to test or observe, but is wrong and will therefore eventually lead to an expensive bug, as in, hits production, costs hours of dev time, literally more expensive than fixing 100 more normal bugs or so).

The behaviour of your timestamp in this example is that it is either some specific value, or it is a magic sentinel value which implies that the timestamp is 'now', whenever you care to ask about it.

These are all 'values'. 1233455L is a value. so is null, which in this case means: "Magic sentinel value that means: Always right now whenever you ask".

However, in your code snippet, if I use withId to make a modified clone of this Pojo, as a totally sneaky surprise, I get not just a modified id, I also get a modified timestamp property! It modifies it from the magic voodoo value that means 'always right now, whenever you ask' to 'when it was cloned'. There's nothing in the name withId that suggests this, hence why it strikes me as subtly but nastily wrong.

The point isn't: This code is buggy. Who knows - it's clearly an example, not real code written for real dollars or eyeballs. The point is instead: It is equally, if not more, imaginable that the reverse behaviour is desired: That I have some POJO where I want that null to be copied verbatim, and not whatever the getter returns, which is presumably some dummy value, or live-calculated.

Hence, I can only interpret this request as: "Whilst lomboks default choice in this regard is usually irrelevant, and in the rare cases where it is relevant, the current choice of doing direct field access is almost always correct, but in the cornercase of the cornercase, lombok's choice is wrong (it IS functionallity different, and I want to invoke the getter)". I want a way to tell lombok explicitly that it needs to make the other choice.

If your request instead is: Can this work exactly like @ToString(doNotUseGetters = true), then the answer is obviously: No, that would be wrong.

The choice is fundamentally different

In particular for this example it feels like the 'choice' of using getter or direct field access needs to be made for every property. I can easily imagine a scenario where you want e.g. timestamp to invoke the getter, but for e.g. id it needs to copy verbatim, and calling the getter would be wrong.

One trivial way to solve that problem is to make the 'argument' to the annotation parameter callGetters a string array, where you type field names, instead of a boolean. But we strongly dislike this ("stringly based typing"): You can typo those field names, and your IDE won't give autocomplete assistance. It just feels ugly to stick field names in string literals. It's why we introduced @ToString.Include, for example - to move away from having to type @ToString(of = {"field1", "field2"}).

Feature proposal

Introduce annotation @With.CallGetter. This annotation does nothing at all on its own, it merely modifies how other features (specifically, @With and @WithBy) operate. We should probably emit a warning on that annotation if it ends up modifying nothing at all.

@With and @WithBy will call the getter both for the identity equality check (this.id == id in your example would become this.getId() == id) and for the cloning operation (the param to the constructor). There is no way to choose on a per-wither basis, and that is intentional.

Your snippet

To get the desired behaviour, you'd write:

@With
@Value
public class Pojo {
  Long id;
  @With.CallGetter Long timestamp;

  public long getTimestamp() {
    return timestamp != null ? timestamp : System.currentTimeMillis();
  }
}

I'm introducing a new tag "discuss", as I'm not sure this is worth adding just yet. But if the answer is yes, the above proposal seems like the right way to do it.

Feedback welcome!

@rzwitserloot rzwitserloot added the discuss Feature proposal that needs to be debated by stakeholders first before PRs are accepted. label Mar 8, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
discuss Feature proposal that needs to be debated by stakeholders first before PRs are accepted.
Projects
None yet
Development

No branches or pull requests

2 participants