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

Generate hasXXX() method #2669

Open
lex-em opened this issue Dec 7, 2020 · 61 comments
Open

Generate hasXXX() method #2669

lex-em opened this issue Dec 7, 2020 · 61 comments
Labels
parked Without further feedback this bug cannot be processed. If no feedback is provided, we close these.

Comments

@lex-em
Copy link

lex-em commented Dec 7, 2020

Hello!

Some frameworks have support of hasXXX methods, for example mapstruct: if field data have additional method boolean hasData() then generated code will call this method and do check around setting target field value.

Same facilities have protobuf.

Possible realization is create annotated with @Transient (json generation for example or jpa) method hasData() with backing transient boolean field __hasData, set __hasData to true(default false) when data setter called, or field data initialized via constructor (builder)

@rzwitserloot rzwitserloot added the parked Without further feedback this bug cannot be processed. If no feedback is provided, we close these. label Dec 7, 2020
@rzwitserloot
Copy link
Collaborator

I like this feature request because it gives falsifiable context about adoption of this concept: mapstruct and protobuf appear to use this. It would have been even better if you link to the relevant sections. I can't find it for mapstruct, but here is java's protobuf implementation's hasMethod feature.

What's less clear to me is what we want with this. A full technical outlay: Show me the code with lombok, and what it boils down to without.

I can see a few routes, but I'm not a frequent user of these frameworks so I'll wait for proposals. Without them this isn't detailed enough to judge.

@lex-em
Copy link
Author

lex-em commented Dec 8, 2020

This is my mistake, I am not focusing on the link to the mapstruct documentation, this is section Source presence checking. Here describing example:
Classes for mapping:

@Data
public class SomeEntity {

    private Long id;

    private String data;
}

@Data
public class SomeDto {

    private Long id;

    private String data;

    public boolean hasId() {
        return id != null;
    }

    public boolean hasData() {
        return data != null;
    }
}

Mapper interface and implementation:

@Mapper
public interface SomeMapper {

    void toEntity(@MappingTarget SomeEntity entity, SomeDto dto);

    SomeDto toDto(SomeEntity someEntity);
}

public class SomeMapperImpl implements SomeMapper {

    @Override
    public void toEntity(SomeEntity someEntity, SomeDto dto) {
        if ( dto == null ) {
            return;
        }

        // using hasField methods
        if ( dto.hasId() ) {
            someEntity.setId( dto.getId() );
        }
        if ( dto.hasData() ) {
            someEntity.setData( dto.getData() );
        }

        return someEntity;
    }

    @Override
    public SomeDto toDto(SomeEntity someEntity) {
        if ( someEntity == null ) {
            return null;
        }

        SomeDto someDto = new SomeDto();

        // no hasField methods
        someDto.setId( someEntity.getId() );
        someDto.setData( someEntity.getData() );

        return someDto;
    }
}

My feature suggestion:
@Setter and additional @Has annotation

@Has
@Setter
@Getter
@NoArgsConstructor
@AllArgsConstructor
class Sample {

  private String data;
}

// generated
class Sample {

  private String data;
  @Transient
  private boolean __hasData = false;

  public Sample() {} // __hasData = false

  public Sample(String data) {
    this.data = data; // or call setter
    this.__hasData = true;
  }

  public String getData() {
    return data;
  }

  public void setData(String data) {
    this.data = data;
    this.__hasData = true;
  }

  @Transient
  public boolean hasData() {
    return __hasData;
  }
}

@Builder and additional @Has annotation

@Has
@Setter
@Getter
@Builder
@AllArgsConstructor
@NoArgsConstructor
class Sample {

  private String data;
}

// generated
class Sample {

  private String data;
  @Transient
  private boolean __hasData = false;

  public Sample() {} // __hasData = false

  public Sample(String data) {
    this.data = data; // or call setter
    this.__hasData = true;
  }

  //private for builder only
  private Sample(String data, boolean __hasData) {
    this.data = data;
    this.__hasData = __hasData;
  }

  public String getData() {
    return data;
  }

  public void setData(String data) {
    this.data = data;
    this.__hasData = true;
  }

  @Transient
  public boolean hasData() {
    return __hasData;
  }

  class SampleBuilder {

    private String data;
    private boolean __hasData = false;

    public SampleBuilder data(String data) {
      this.data = data;
      this.__hasData = true;
      return this;
    }

    public Sample build() {
      return new Sample(data, __hasData);
    }
  }

  // .. builder() method
}

As we can see - the idea is to mark the field as "touched"

@lex-em
Copy link
Author

lex-em commented Dec 8, 2020

Use case for this feature request:
Deserialization of partial json (some fields is omitted), jackson (for example) call setters and mark fields "touched", that means fields was present in json, after that mapper will set to entity only "touched" fields.
Data for SomeDto:

{
  "data": "123123"
}

Logic:

public void update(SomeDto dto, Long id) {
  SomeEntity entity = repository.find(id);
  someMapper.toEntity(entity, dto);
  repository.save(entity);
}

@rzwitserloot
Copy link
Collaborator

Let's forget, for a moment, about mapstruct and the like. Just look at the feature request.

It's completely mystifying and makes no sense whatsoever.

The fact that the has field is transient means that if you do serialize and then de-serialize it, then the object used to return hasX() = true, but afterwards, it now returns hasX = false. Also, instead of using the transient keyword, this is conveyed by way of an annotation from a non-standard library (which makes this feature a non-starter unless it goes in a lombok.extern subpackage).

There is no way to sanely document any of this whatsoever.

Unless, of course, you say: Actually, this feature makes no sense unless you realize it's about making JPA/mapstruct/protobuf work (and I'm pretty sure all 3 really want entirely different things, they just all so happen to share one small aspect of it all, which is a method named hasX), and then document in terms of that.

Which means as written this feature cannot be implemented (lack of sense). But there is an avenue to make e.g. the annotation lombok.extern.jpa.Has, which has this exact behaviour (including using jpa's @Transient).

This then also solves the problem that the proposed implementation doesn't seem like it's what you'd want for e.g. protobuf.
We could then also introduce lombok.extern.protobuf.Has which adds the support for what protobuf wants.

The downside is, all lombok feature requests are judged based on 'value for the money'.

This modification moves this feature request in the wrong direction on both fronts: The value goes down because it's just for one of the 3 targeted libraries instead of for all of them, and the cost goes up because we're now much more beholden to idiomatic JPA which is likely to change (that's the downside of speccing a feature in terms of 'it does thing X which is handy when using library Y: You're now beholden to the whims of Y'.

With that in mind I doubt this will end up being a good idea, but, maybe.

Focussing on just this JPA thing (which, at this point, has nothing whatsoever to do with mapstruct anymore, right, or do I misunderstand your example?)

Looking specifically at your proposal for this feature: If builder is involved, that explodes the complexity (builder is already very complicated, piling on even more, that's possible but tricky). The proposal also interacts with @AllArgsConstructor, which must now generate those hasX = true statements. It also interacts with the very definition of @Builder, which says that builder assumes the all-args constructor is there, and will assume @AllArgsConstructor unless you either write a constructor yourself or hav ean explicit XArgsConstructor annotation.

Now if @Has is involved, all of a sudden that constructor disappears, or, builder generates that one for no reason (it will never call it), but will call another, private constructor that looks bizsarre with a ton of booleans.

Builder in general seems lost here: Builder is primarily for immutables. This is not immutable, so, just.. call setters. That's what they're for. Forget builder, this feature proposal is DOA if that's involved. What if we just say that @Builder + @Has is disallowed? We can always add support later if that ends up being a really useful thing to do.

Even then this is more complicated than I had hoped.

@xak2000
Copy link

xak2000 commented Dec 8, 2020

I like the idea but I'm unsure how it is related to JPA.

Looks like it's more related to deserialization of data into objects. Useful for merge-PATCH-requests for example to determine what fields were present in the JSON.

With that in mind I don't see how this feature is related to any concrete framework/library. The only thing that a library must do is to use setters. And has* methods could be called by user code then. It's not necessary that a library must be primary client for has* methods (but if some libraries already follow this contract, it's good).

The manual code to implement common HTTP PATCH request could look like this:

Order order = orderRepository.findOrder(orderId);
if (orderDto.hasName()) {
  orderService.updateOrderName(order, orderDto.getName());
}
if (orderDto.hasApproved()) {
  if (orderDto.isApproved()) {
    orderService.approveOrder(order);
  } else {
    orderService.withdrawOrderApproval(order);
  }
}

This example shows code that support JSON MERGE PATCH requests with just 2 possible fields: name (string) and approved (boolean).

To me, the main point of this feature request is to make Lombok to generate boilerplate that allows to check if some field was set at least once or never been set at all.

The challenge here is to make generated boolean __has* fields to be ignored on serialization stage. Each library has own approach to do this, I think. E.g. for Jackson it's @JsonIgnore, for JPA (I'm not sure why this feature can be useful with JPA, but still...) it's @javax.persistence.Transient etc.

@rzwitserloot
Copy link
Collaborator

how it is related to JPA.

Okay, then what is that @Transient in your snippet? I thought you were talking about @javax.persistence.Transient.

@rzwitserloot
Copy link
Collaborator

With that in mind I don't see how this feature is related to any concrete framework/library.

That means hasFoo() is to be read as: "Since deserializing or otherwise reading this from a persistence store, has Foo been changed"?

Maybe my grasp of english is weird, but how you get from "hasFoo" to "Foo has been changed" is based on wishes and voodoo magic only. Hence why this doesn't make sense unless you sign on to the fact that JPA and such came up with this turd.

@rzwitserloot
Copy link
Collaborator

rzwitserloot commented Dec 8, 2020

Here is a completely different take on has, to highlight why I don't see a route forward without tying specifically to a library:

with lombok:

@Data @AllArgsConstructor
class Example {
    @Has String foo;
}

delomboked:

class Example {
    private String foo;
    
    // all the normal stuff, unchanged; equals, hashCode, etc.
    // all args constructor too. Unchanged from current lombok. and also:

   public boolean hasFoo() {
      return this.foo != null;
   }
}

Why doesn't it generate that? I find it hard to explain why not without saying something like: "Well, because the point of @Has is to interop with serialization and persistence frameworks, or anything else that wants to track 'has this been changed' state".

And if that's the goal, and we don't tie it to a framework, 'has', as an english word, doesn't suggest 'change management' whatsoever. I can see @lombok.TrackChanges that would generate boolean fields to track changes and generates methods called hasFooChanged() or perhaps isChanged(Fields.FOO). Now the feature self-documents... but doesn't actually do what you want when interacting with commonly used persistence and serialization frameworks, so now the feature makes sense, but isn't useful.

That is the dilemma here: Without tying it to a specific framework, it's either too hard to explain without mentioning those frameworks, or it's useless.

@xak2000
Copy link

xak2000 commented Dec 8, 2020

Okay, then what is that @Transient in your snippet?

What do you mean "mine" snippet? I'm not the author of this issue. In my code there is no @Transient.

Maybe my grasp of english is weird, but how you get from "hasFoo" to "Foo has been changed" is based on wishes and voodoo magic only.

I'm not a native English speaker. Maybe that is why I don't see a big problem here. To me it sounds good. Not "Foo has been changed" though, but rather "Foo has been set (at least once) and now the object has this property (set) as opposed to a previous state when this property was never set and this object hadn't this property then". But I agree it is somewhat contrived. isFooSet sounds better.

It's not me who offered this has* variant. But, if 3 different libraries already use this pattern, who I am to question it..

For example, in the link you mentioned earlier I found this:

There are also has getters for each singular field which return true if that field has been set.

So, the semantic is basically the same I have in mind. hasFoo() method should return true if foo was set.

I think the logic here is the same as with is* methods. Sometime they totally have no sense if you try interpret them in literature english (e.g. boolean show; field would generate isShow() (let's pretend you can't replace show with visible for some reason)), but they are still named this way to follow the JavaBean contract.

Hence why this doesn't make sense unless you sign on to the fact that JPA and such came up with this turd.

I still don't see how does this relate to JPA. If you give me an example it would be good.

Also, your delomboked example makes no sense to me. Why hasFoo() would test foo for null? I could just use getter for this and test it myself. The whole reason to have hasFoo() method is to know if foo was really been set (maybe it was set to null and in this case getFoo() != null doesn't help) or not.

But, I'll repeat - I am not the original author of this request and maybe my vision is very different from author's. I just wanted to add my 5 cents to maybe let others a broader vision to this request than associate it with some concrete library.

Now the feature self-documents... but doesn't actually do what you want when interacting with commonly used persistence and serialization frameworks

Sorry, I'm not sure who you are talking to. If to the author of the issue (@lex-em), then I agree (he wants it for concrete library mapstruct that checks methods presence by concrete pattern, if I understand correctly). If to me, then I disagree because my example is hand-written code and to me it doesn't matter how methods are named.

What does matter to me is to have some way to mark generated boolean fields as ignorable for Jackson (yes, my primary use-case is bound to a library too :) ). But I think it's possible with some configuration of objectmapper (e.g. configure it to use only getters and ignore all fields altogether or generate inner class as a wrapper for boolean properties and then manually put @JsonIgnoreProperties("__hasChangedHelper") on a DTO class, or maybe there is some way to filter out properties based on their name by regexp, I don't know). But I would try to abstract from this if possible. If I would not, then this feature must be bound to Jackson and this would obviously not suitable to include it in the Lombok. And it would definitely be not a feature that author of this issue requested.

@rzwitserloot
Copy link
Collaborator

@xak2000

What do you mean "mine" snippet?

I meant lex-em's snippet.

"Foo has been set (at least once) and now the object has this property (set) as opposed to a previous state when this property was never set and this object hadn't this property then".

But lex-em's snippet doesn't do this, and you haven't proposed anything yet, just showed some nebulous usage. Perhaps you have a different idea about what this feature ought to do than lex-em. Can you post a 'with lombok' and a 'this is what lombok would delombok it to' example side-by-side so I can follow along with what you're thinking about?

I still don't see how does this relate to JPA. If you give me an example it would be good.

Lex-em's proposal, as linked above.

If to me, then I disagree because my example is hand-written code and to me it doesn't matter how methods are named.

That's another use case: There is:

  • I want this for mapstruct
  • I want this for protobuf
  • I want this for JPA
  • I want this for some other library
  • I just like having some hasX method in my own API and don't want to hand-write it

I'm pretty sure that 5th case is a few orders of magnitude too rare to be called boiler-plate though. This feature stands very little chance unless it picks up enough ground to qualify as boilerplate, so I'm pretty sure it needs to cover all or most of that list.

My prediction that these are 5 separate feature requests that are mostly unrelated, except they share the name 'hasX', seems to be coming true, and if true, that's... not helping this proposal at all; confusion increases the 'cost' factor in the 'value/cost' calculation considerably.

@xak2000
Copy link

xak2000 commented Dec 8, 2020

But lex-em's snippet doesn't do this

Actually his example does it, but only for Sample class for some reason.

Can you post a 'with lombok' and a 'this is what lombok would delombok it to' example side-by-side so I can follow along with what you're thinking about?

Of course!

@Data @AllArgsConstructor
class Example {
    @Has String foo;
}

Delomboked:

class Example {
    private String foo;
    private boolean __hasFoo; // or maybe wrapped in inner class
    
    // all the normal stuff, unchanged; equals, hashCode, etc.
    // all args constructor too. Unchanged from current lombok. and also:

   public void setFoo(String foo) {
      this.foo = foo;
      this.__hasFoo = true; // setter must update the corresponding boolean field
   }

   public boolean hasFoo() {
      return this.__hasFoo; // true only if setter was called at least once, even it `null` was it's argument
   }
}

Basically this is the same as HashMap.containsKey(Object key); that can return true even for key, that is mapped to null value.

To me it could be usable for implementing PATCH-requests in REST API.

If DTO class has 50 fields it's pretty large boilerplate to add all these boolean has* fields and methods to check them. This also prevents to use lombok for setters, so they must be written manually too.

But let's listen what @lex-em think about it. Maybe I turned the discussion in the wrong way and my "feature request" is actually not directly related. :)

@rzwitserloot
Copy link
Collaborator

rzwitserloot commented Dec 8, 2020

I'm not sure I follow your example, so let me check first:

If lombok worked as your delomboked example shows, then this is what would happen:

Example ex = new Example("Hello");
System.out.println(ex.getFoo());
System.out.println(ex.hasFoo());
ex.setFoo("World");
System.out.println(ex.getFoo());
System.out.println(ex.hasFoo());
> Hello
> false
> world
> true

Is that really what's supposed to happen here?

If true, I'm vetoing the request (specifically, the 'the feature stands on its own, and not in relationship to any specific java framework/library') - It makes zero sense that hasFoo() returns false when getFoo() returns something here. Why would this Example, initialized with a value for the foo field, report that it 'has no foo'? That makes no sense. What you want is a method whose javadoc would read:

/** @return {@code true} if this property has been changed since this object's creation. */

hasFoo sounds about as appropriate a name for that concept as SupercalifragilisticexpialidociousFoo(). Actually, SupercalifragilisticexpialidociousFoo is better, because unlike hasFoo, it doesn't actively mislead.

If the above 4 prints is not what's supposed to happen, can you correct the delomboked example / post a new take?

@Gaibhne
Copy link

Gaibhne commented Dec 9, 2020

I have no worthwhile input on the various frameworks, but at my company, we too follow the idiom of having 'has'-methods to simply provide a DSL of sorts around null/0 checks (we also have them for CollectionS, such as class Server { private List<Client> clients = new ArrayList<>(); public boolean hasClients() { return clients.size() > 0); }} ... if (server.hasClients()) { ...

I would love to see this as a basic feature because I feel that the readability is much nicer, but I understand that we too may be an edge case.

@lex-em
Copy link
Author

lex-em commented Dec 9, 2020

@rzwitserloot
yes, my wording with @Transactional was a little bit incorrect, it is not jpa or jackson (or any other) oriented solution. it is "partial updates", as mentioned @xak2000. solving this moment - what about using mechanism onMethod? I.e. @Has(onMethod_={@Transient}) with needed import. it will remove the contradictions

what about serialization - temporary fields __has* is not important for serialization at all. when the serialized data is deserialized the __has* fields will be restored according to the same principle. you can say - null fields will be written explicit with null values during serialization, this can be bypassed by not serializing the fields with has*() == false. it is all about serialization of __has* fields not mater

@xak2000 says and I totally agree

To me, the main point of this feature request is to make Lombok to generate boilerplate that allows to check if some field was set at least once or never been set at all.

Why doesn't it generate that? I find it hard to explain why not without saying something like: "Well, because the point of @Has is to interop with serialization and persistence frameworks, or anything else that wants to track 'has this been changed' state".

@rzwitserloot It is wrong, because the point is that "does foo field was changed ever", that means does field foo, initialized by default, was setted after parent object construct, for example default value is null, during deserialization field setted with some value and if value is null then hasFoo() should be true as when setted value is not null (because setter was called)

But I agree it is somewhat contrived. isFooSet sounds better.

@xak2000 I agree, but it is not competable with other libraries

If to the author of the issue (@lex-em), then I agree (he wants it for concrete library mapstruct that checks methods presence by concrete pattern, if I understand correctly

Yes, that's right

But lex-em's snippet doesn't do this

@rzwitserloot why? The part with generated code for proposal does exactly that, setting __hasData to true only when value setted to field

That's another use case: There is:

I want this for mapstruct
I want this for protobuf
I want this for JPA
I want this for some other library
I just like having some hasX method in my own API and don't want to hand-write it

No, I meant only mapstruct part. Other libraries only for sample

@lex-em
Copy link
Author

lex-em commented Dec 9, 2020

If DTO class has 50 fields it's pretty large boilerplate to add all these boolean has* fields and methods to check them. This also prevents to use lombok for setters, so they must be written manually too.

Yes, that right

Is that really what's supposed to happen here?

No, that is wrong, because foo was initialized in constructor and hasFoo() should be true

@rzwitserloot
Copy link
Collaborator

@Gaibhne

I have no worthwhile input on the various frameworks, but at my company, we too follow the idiom of having 'has'-methods to simply provide a DSL of sorts around null/0 checks (we also have them for CollectionS, such as class Server { private List<Client> clients = new ArrayList<>(); public boolean hasClients() { return clients.size() > 0); }} ... if (server.hasClients()) { ...

I would love to see this as a basic feature because I feel that the readability is much nicer, but I understand that we too may be an edge case.

No, this is quite useful! It suggests my idea that a method named hasX returns false if the X property is some sort of take on 'not a value' or 'empty', be it null, an empty list, some sentinel, etcetera. That's in sharp contrast to this idea of separate boolean fields, which do not mesh with this take on what hasX means.

@rzwitserloot
Copy link
Collaborator

@lex-em

what about serialization - temporary fields __has* is not important for serialization at all.

Before we go any further on this, I still have absolutely no idea why this boolean is needed. I'm trying to fully grok what you're trying to do. Forget lombok, forget technical descriptions, what is your API supposed to do? So far, I've gathered conflicting information. Something about protobuf, something about mapstruct. From what I was able to gather from those, the point of it all is for mapstruct and protobuf to know what needs updating when sending/persisting/converting explicitly vs. what needs to be not sent/not named in the sql UPDATE statement/not converted, thus relying on the default in the target. Which is a fine feature, but the name hasX is in a word idiotic, and yet it seems like mapstruct and protobuf are idiots, so, clearly I'm missing something there.

If I understand what you want correctly, you want this behaviour. Given @BunchOfConstructors @Value class Person { @Has String name; } you'd want:

Person p = new Person();
print(p.hasName());
> false
p.setName("Joe");
print(p.hasName());
> true
p.saveToDisk(PATH);
p = Person.loadFromDisk(PATH);
print(p.getName());
> joe
print(p.hasName());
> false [RED: This is bizarre. You really want this? See *1]
p = new Person("Jane");
print(p.hasName());
> true
p.setName(null);
print(p.hasName());
> true [red: also a bizarre answer] *2

*1) You say you wanted the boolean fields to be transient. Then this nuttiness is what would ensue. I can see the point if the idea is 'track changes', but the name hasX is a bizarre method name for the method that returns if a thing has been changed or not. That's why I keep harping on about: "Are you asking for a track changes method"? Because you keep talking about booleans and transient. What am I missing?

*2) Given that you want booleans, this bizarre answer (a person with null for a name 'has a name') is evidently what you'd want, or, if not, what am I missing?

when the serialized data is deserialized the __has* fields will be restored according to the same principle.

If it is possible to determine the state of those boolean fields based solely on the other fields, then why the heck are these fields in the first place? Why bother storing information that can be cheaply derived at all?

during deserialization field setted with some value and if value is null then hasFoo() should be true as when setted value is not null (because setter was called)

Okay, so you want this:

Person p = new Person();
print(p.getName());
> null
print(p.hasName());
> false
p.setName(null);
print(p.getName());
> null
print(p.hasName());
> true

In other words, differentiate between 'the value is null, and that is because nobody set it yet' and 'the value is null, and that is because somebody explicitly set it to that value'.

Why do you want that? That's what I need to understand. It sounds like rather exotic bit of terrible code design to care about this (heck, in other threads I'm fighting the good fight about folks who want to eliminate null, and in this thread you're evidently making multiple flavours of it!), but that's probably because I'm not facing the same use case you are. I'd like to understand the point of wanting this API first. If this is what you want, I get why you need a boolean.

@xak2000
Copy link

xak2000 commented Dec 9, 2020

In other words, differentiate between 'the value is null, and that is because nobody set it yet' and 'the value is null, and that is because somebody explicitly set it to that value'.

Why do you want that?

@rzwitserloot I'll try to describe my use-case. But I should warn you that it could be not the same as lex-em's. But 2 use-cases are better than one, right? :)

Let's imagine, I want to implement a REST API to manage marketplace orders. Let's left alone GET/PUT/DELETE methods implementation and focus on PATCH method (to be clear: HTTP method PATCH).

For simplicity my update request will have 2 fields. In real app it would be much more.

@Data
class UpdateOrderRequestDto {
  private String shipmentAddressId;
  private Boolean active;
}

It's very important to understand, that this class will be used exclusively as a container to deserialized representation of JSON body of HTTP requests! So if I would call hasShipmentAddressId() it doesn't mean that if order doesn't have a shipment address, this method should return false. It's not a method of order class. It's a method of request representation's class. So the meaning of hasShipmentAddressId is totally different. It does mean that request (as opposed to order) has shipmentAddressId.

Now, I want my users to be able to send any combination of fields in JSON body of HTTP requests.

Update shipment address and deactivate the order:

{
  "shipmentAddressId": "XXX",
  "active": false
}

Update shipment address:

{
  "shipmentAddressId": "YYY",
}

Activate the order:

{
  "active": true
}

All these requests are valid.

It's also important for this API to support removing of shipmentAddressId from order. So client should be able to pass a JSON like this:

{
  "shipmentAddressId": null,
}

and server must then remove a shipment address from this order, if it's currently set and order is not complete yet.

Now, in my business logic I want to check what fields were really present and what were not. I need this because of mentioned early requirement. I can't just check for null. It's possible that client passed null for shipmentAddressId field or it didn't pass this field at all. And business logic should be allowed to differentiate these cases from each other. If client didn't pass shipmentAddressId at all, it does mean, that maybe client passed some other fields, but shipment address of the order should not be changed. If client did pass "shipmentAddressId": null, it does mean, that client intends to remove a shipment address from the order.

Currently there is a way to achieve what I described using HashMap instead of UpdateOrderRequestDto. This is possible, but working with string keys instead of fields of java objects is not a good experience in statically-typed language. It's more common in JS or PHP, but not Java.

So, that's why I liked this idea of has methods.


if the above 4 prints is not what's supposed to happen, can you correct the delomboked example / post a new take?

It's hard to say. At first I want to say "no". Because constructor also must use setter and so __has field will be updated. But then... what about hand-written constructors? What about initializing fields in-place? With my use-case in mind, __has fields must be updated only by JSON deserialization. In other contexts they do not have any sense. But as far as I use a dedicated class for representation of deserialized request, it is ok to me.

So, answering your question - looks like it doesn't matter for my use-case. My class must be constructed using default constructor only and it must not have any other costructors.

@xak2000
Copy link

xak2000 commented Dec 9, 2020

More thoughts. If the same class UpdateOrderRequestDto will be used in client (to represent a request that will be sent), the serializing library could use has-ers to decide what fields are need to be serialized. So, if shipmentAddressId was set (even to null), it must be serialized, but if it was never set, it must not be present in resulting request's JSON (or else server will clear shipment address of an order).

Currently I don't know if, say, Jackson could be configured to do this.

By the way, for request side it would be good to have another set of clear* methods. This way the code that builds the request could actually "remove" fields from marked as to be serialized. As I see, protobuf has these methods too (from link you mentioned earlier). But I never worked with protobuf, so it's hard to say if they have the same purpose.

@rzwitserloot
Copy link
Collaborator

rzwitserloot commented Dec 9, 2020

@xak2000

what about hand-written constructors? What about initializing fields in-place? With my use-case in mind, __has fields must be updated only by JSON deserialization.

You nailed it. That's the problem.

I'll take credit for my haphazard floundering in an attempt to figure out how this is useful: My @TrackChanges thing is looking pretty good now 😛

The problem is, we have the following issues now, assuming we want your JSON use-case to work out, and we want this 'the booleans arent persisted, so upon loading from persistence, always init to false' thing from @lex-em:

  • hasX as a name doesn't really get the job done; it would return 'false' when a property has a value but that value is not set since initialization / load-from-persistence. That's a dumb name.
  • hasXChanged as a name does get the job done. But it doesn't integrate with the very libraries/frameworks you'd want to use this feature for.

Your second use case of having java be on the other side and send a cutdown JSON dump listing only the changed fields also seems to match exactly with wanting the notion of 'hasXChanged', running headfirst into the giant bar of 'why the heck is the standardized name for this method hasX? Who named this mess?'

This is heading rapidly to the unfortunate conclusion of: Because the authors of jackson and protobuf screwed up, lombok must sign on to the same mess, and that's severely hampering the chances of this proposal. Unfortunately, I highly doubt jackson and protobuf are willing to acknowledge they done goofed on the naming, let alone fix it.

If somehow these parties are on board for a name change, I'm game. Proposed annotation name: @TrackChanges.

@Rawi01
Copy link
Collaborator

Rawi01 commented Dec 9, 2020

This change detection mechanism seems to be similar to @BoundSetter. Wouldn't it be more useful to implement it like this:

Before

@Data
class Example {
    @TrackChanges String foo;
}

After

class Example {
	String foo;
	
	private PropertyChangeSupport propertySupport = new PropertyChangeSupport(this);
	{
		addPropertyChangeListener(new PropertyChangeListener() {
			void propertyChange(PropertyChangeEvent evt) {
				changed.put(evt.getPropertyName(), true);
			}
		})
	}
	private Map<String, Boolean> $changed = new HashMap<>();
	
	public void addPropertyChangeListener(PropertyChangeListener listener) {
		this.propertySupport.addPropertyChangeListener(listener);
	}
	
	public void removePropertyChangeListener(PropertyChangeListener listener) {
		this.propertySupport.removePropertyChangeListener(listener);
	}
	
	public void setFoo(String foo) {
		String old = foo;
		this.foo = foo;
		this.propertySupport.firePropertyChange("foo", old, foo);
	}
	
	public boolean hasFoo() {
		return this.$changed.get("foo");
	}
}

@rzwitserloot
Copy link
Collaborator

rzwitserloot commented Dec 10, 2020

@Rawi01 Going back to the underlying problem that this feature is ostensibly trying to solve: If generating all that jazz will magically make protobuf and mapstruct just do the right thing, maybe.

However, your proposal doesn't solve one of the dilemmas that I really, really want solved first:

It makes a method named hasFoo, which is a lying liar which lies. Because what it returns is'has foo been changed since creation or deserialization', and not whether we can characterize the current value of 'foo' as having presence or not (presence is like javascript falsy: null has no presence. Something like "Foo" has presence. Whether "" has presence or not is up for debate, which is why truthy/falsy and presence systems aren't fantastic, but it is commonly what hasFoo is understood to mean, right?)

@Rawi01
Copy link
Collaborator

Rawi01 commented Dec 10, 2020

I agree with you that the name is misleading in general but it doesn't feel that wrong if you are in a deserialization context. It seems like that protobouf already generates these fields for a Message and that there is no need to generate it using lombok. I couldn't figure out how mapstruct handles these hasXxx methods, the documentation of this feature is a bit too short.

About my proposal: I would expect to get code that enables object change tracking using something like a listener if I use @TrackChanges. Thats why I added this example.

@xak2000
Copy link

xak2000 commented Dec 10, 2020

It makes a method named hasFoo, which is a lying liar which lies. Because what it returns is'has foo been changed since creation or deserialization', and not whether we can characterize the current value of 'foo' as having presence or not

@rzwitserloot

I'll repeat myself:

It's very important to understand, that this class will be used exclusively as a container to deserialized representation of JSON body of HTTP requests! So if I would call hasShipmentAddressId() it doesn't mean that if order doesn't have a shipment address, this method should return false. It's not a method of order class. It's a method of request representation's class. So the meaning of hasShipmentAddressId is totally different. It does mean that request (as opposed to order) has shipmentAddressId.

If you look to has methods in this context (serizliation/deserialization context), they are named totally sane. And these methods don't have any value in other contexts anyway. Why do I need hasFoo() method that checks if foo is null or ""? I could just do a.getFoo() == null || "".equals(a.getFoo()) if I need to. Encapsulation? Okay, but then I'll better write my has method manually, because for each domain the definition of "emptiness" is different.


Proposition: can the new annotation have prefix and suffix properties (with some default values)? Is it possible/easy to implement? This would solve the naming problem.

Like this:

  • @TrackChanges(prefix = "has", suffix = "") will generate hasFoo() for property foo.
  • @TrackChanges(prefix = "has", suffix = "Changed") will generate hasFooChanged() for property foo.

@rzwitserloot
Copy link
Collaborator

@Rawi01 wrote:

I would expect to get code that enables object change tracking using something like a listener if I use @TrackChanges. Thats why I added this example.

That is an excellent point.

@rzwitserloot
Copy link
Collaborator

@xak2000 wrote:

Why do I need hasFoo() method that checks if foo is null or ""? I could just do a.getFoo() == null || "".equals(a.getFoo()) if I need to.

Sure. It's not about what you need. It's about what one would expect a method named hasFoo() does.

It does mean that request (as opposed to order) has shipmentAddressId.

I see now. I think I'm too unfamiliar with the problem domain (or I'm having a dumb moment) as I didn't really get it first time I read this, but I think I do now.

I'm not sure that mental model actually 'works'. If I understand you correctly, you have a domain model, let's say it represents a Course at a university. The Course itself has the property of 'title'. And what you're suggesting is that e.g. hasTitle() here can return false even if the course has a title just fine, because the java object does not, in fact, represent that Course at all. It represents the notion of the change request to be sent to the document system that stores Course objects.

Therefore, hasTitle() returning false implies that the Course Change Request doesn't currently have a value for the concept of its 'change the title to this' property. Even if this Course Change Request is understood to be about the Course object in the data store representing the Econ 101 course, and whose current title is "Economics 101" - so the title is Economics 101, but, 'hasTitle' is false.

Did I get that right?

However, if that's the actual mental model, then .getTitle() should return null and not "Economics 101" for the economics 101 course, because you're calling getTitle not on the course, but on the course-change-request, and you haven't filed any new title in your change request yet. Or, instead of null, it should throw something, or possibly, builder-style, there is no get method, instead there is a 'craft me a JSON string that I can ship off to the endpoint' method, similar to how builders don't have get methods either, only 'setters' and a build() method, because the purpose of this object isn't to be a vehicle for passing data along within java apps; it's a vehicle for crafting an API call, and it's best not to create the wrong impression that you can also use it for a completely different purpose (namely, to convey the info about the underlying course).

Returning the title of the underlying domain object (Course itself) if you call getTitle(), but returning the status of presence of the Course-change-request concept if you call hasTitle() means that the API uses one word (Title) to mean 2 completely different concepts which are easily confused, and therefore, I'm unmoved: This comes across to me as terrible API design, and I mostly wish mapstruct and friends (if they really work this way) hadn't done this. Lombok has no problems catering to a popular library even if that popular library has terrible API, but only if we then make a lombok.extern. feature and specify the feature in terms of the library it is meant to support.

Proposition: can the new annotation have prefix and suffix properties (with some default values)? Is it possible/easy to implement? This would solve the naming problem.

I'm not sure it solves much. If the default is Changed but you can modify it using @TrackChanges(suffix = ""), that is bad for readability (a casual reader who sees that is supposed to just clue into the notion that this is happening because the naming conventions required by e.g. mapstruct are silly - I don't think so). Vs @MapStructTrackChanges, terrible as that name may be, which at least does have the property that a casual peruser of the source code is likely to draw the correct conclusion. It's also boilerplate-in-the-boilerplate in that a mapstruct user is now forced to write (suffix = "") everywhere. We can fix that in turn with a lombok.config key but there's pretty much no way to use this feature unless you read and fully understand the implications of 2+ pagefuls of docs, and that means the value of this feature is in the toilet, and it'll never cross the value/money bar we've set for lombok features.

@lex-em
Copy link
Author

lex-em commented Dec 11, 2020

I fully agree with @xak2000 in message #2669 (comment)

It is mostly json deserialization feature for partial changes (patching), my example with the builder could be superfluous at this stage, but in the future it would be nice to have such functionality in view of the fact that we can make such requests from Java code (it builds with builders for some reasons, but not necessary). there we can still tweak the serialization and send only the necessary (through has* methods of course)

Let's forget about builders for now, understanding of how to do better may come later.

@rzwitserloot can I ask a question? why you are catch on jpa? may be I was incorrect, but I didn't meant to use it with jpa, only DTOs from json

hasX as a name doesn't really get the job done; it would return 'false' when a property has a value but that value is not set since initialization / load-from-persistence. That's a dumb name.

why it should return false? I said that it should return true, because of it is initialized (setters or constructor is called after object creation during deserialization). I've said this more than once here

This is heading rapidly to the unfortunate conclusion of: Because the authors of jackson and protobuf screwed up, lombok must sign on to the same mess, and that's severely hampering the chances of this proposal. Unfortunately, I highly doubt jackson and protobuf are willing to acknowledge they done goofed on the naming, let alone fix it.

@rzwitserloot okay, I see you are picking on the name has*because your opinion is different, did you agree to does it with @Has(prefix = "has", suffix="") with defaults prefix=does and suffix=Changed?

@lex-em
Copy link
Author

lex-em commented Dec 11, 2020

i see it was already proposed)))

Like this:

@TrackChanges(prefix = "has", suffix = "") will generate hasFoo() for property foo.
@TrackChanges(prefix = "has", suffix = "Changed") will generate hasFooChanged() for property foo.

@lex-em
Copy link
Author

lex-em commented Dec 11, 2020

@rzwitserloot what is my initial point:

any data format (json, xml, any other) have three states:

  1. no data (field is absent)
    2.a. have data: null
    2.b. have data: non null

unfortunately java does not present the first state, unlike javascript for example. we will newer know does input data have value and if it have null - we need to handle it as set null. this featire intended to solve this situation.

how it cleary can be solve? I think just generate setting this.__hasX = true in setters, because of setters will be called during deserialization. No constructors needed. Do you agree?

@rzwitserloot
Copy link
Collaborator

(@lex-em) I said earlier, that it is impossible, if field has value that means setter was called, than the flag switches to true.

transient, in most contexts (or, at least, the context of both JPA and serialization), means that it doesn't survive persistence or transfer and will revert to a default value and/or is reconstructed based on the other fields.

So, going back all the way to the first few posts in this issue: Can we consider the mention of @Transient as a mistake, or am I missing something about what transient means in this context?

@lex-em
Copy link
Author

lex-em commented Dec 14, 2020

@rzwitserloot

transient, in most contexts (or, at least, the context of both JPA and serialization), means that it doesn't survive persistence or transfer and will revert to a default value and/or is reconstructed based on the other fields.

we are only looking over deserialization from three-state sources with partial structure (json from HTTP PATCH), we do not looking over persisting via jpa or other orm, or serializing. in that cases no three states in data and there no need to this feature. serializing and persisting of boolean flags are not necessary at all

So, going back all the way to the first few posts in this issue: Can we consider the mention of @transient as a mistake, or am I missing something about what transient means in this context?

Are you looked at my last propersal #2669 (comment)?

@Has(suffix="", onMethod=@__(@JsonIgnore)) // default prefix="has", suffix="Changed"

@rzwitserloot
Copy link
Collaborator

@lex-em yes; as @filiphr from mapstruct mentioned, mapstruct is not the best fit for this, and they're also just giving in to established naming conventions - in their case, inspired by protobuf.

That Transient thing is still a mystery to me. What happened to it? Your proposal no longer includes them, but your original request did, and I still have no idea what you're trying to accomplish with that @Transient, or even which @Transient annotation that's trying to refer to. Surely if you're attempting to ensure that hasFoo itself isn't included in e.g. the JSON generated, and the library "knows" what the existence of a hasFoo method means, you don't also have to tell said library that the hasFoo thing itself isn't a property in the first place?

@almson
Copy link

almson commented Dec 14, 2020

@rzwitserloot Looks like onMethod=@__(@JsonIgnore) replaced @Transient

@xak2000
Copy link

xak2000 commented Dec 14, 2020

@rzwitserloot Forget about @Transient. I think it was a bad example. @lex-em meant "marking a field in some way to let serializing/deserializing framework to ignore it". This is hypothetical @Transient :). For example, for Jackson it would be @JsonIgnore.

However, if that's the actual mental model, then .getTitle() should return null and not "Economics 101" for the economics 101 course, because you're calling getTitle not on the course, but on the course-change-request, and you haven't filed any new title in your change request yet. Or, instead of null, it should throw something, or possibly, builder-style, there is no get method, instead there is a 'craft me a JSON string that I can ship off to the endpoint' method, similar to how builders don't have get methods either

No, the class that represents JSON request must have getters because request can be seen from two sides:

  1. Sending side
  2. Receiving side

Both sides working with request. The side that sends it doesn't require getters. But the side receiving it requires them to get the data from this request, obviously.

Unlike builder, that is used only as temporal intermediate storage.

I'm not sure that mental model actually 'works'. If I understand you correctly, you have a domain model, let's say it represents a Course at a university. The Course itself has the property of 'title'. And what you're suggesting is that e.g. hasTitle() here can return false even if the course has a title just fine, because the java object does not, in fact, represent that Course at all. It represents the notion of the change request to be sent to the document system that stores Course objects.

Yes, and most important here is that it's hasTitle() of CourseUpdateRequest that would return false. It's not Course's method! It's important!

Therefore, hasTitle() returning false implies that the Course Change Request doesn't currently have a value for the concept of its 'change the title to this' property. Even if this Course Change Request is understood to be about the Course object in the data store representing the Econ 101 course, and whose current title is "Economics 101" - so the title is Economics 101, but, 'hasTitle' is false.

Did I get that right?

No, hasTitle() returning false implies only that JSON representation of CourseUpdateRequest didn't contain the title field in serialized form of the request, nothing more (so, the name hasTitle() has much sense in this context). So, the title field of this CourseUpdateRequest object will be null (or any other default value). It has nothing to do with the title of the Course object. These are 2 different objects with different meanings. When we analyzing the title of CourseUpdateRequest object we don't know if Course (domain object) has title (and what title) or not. We could load it from DB etc, but this is a different story.

Returning the title of the underlying domain object (Course itself) if you call getTitle(), but returning the status of presence of the Course-change-request concept if you call hasTitle() means that the API uses one word (Title) to mean 2 completely different concepts which are easily confused

But you are talking about 2 different classes. You are talking here about Course.getTitle() vs CourseUpdateRequest.hasTitle(). Why should they represent single concept? One is domain object and other is representation of JSON request.

If you use a boolean to track this info and consider it transient information, that means upon conversion to some other system and back to java (so, you save it to DB and back, or you toss it to some document API via JSON and then restore it, yadayada), the boolean is now FALSE, and more generally you need to explain the model because with that boolean we now have 4 states, not 3!

The transient nature of these __has* fields works because in serialized representation (JSON) the information about field presence or absence is encoded naturally (by including or excluding field from JSON representation of object). Protobuf shares this property, AFAIK.

In contrary, SQL doesn't! In SQL there are columns and if you set any value to a column you can't then know if the value was set or not (when SELECT-ing the data). So, if by "serialization" you mean "saving to SQL DB", then I agree, __has* fields must not be transient. But it doesn't matter because these fields don't have any sense for object that represents DB table. I repeat again - they are useful only in context of serialization/deserialization, and only when serialized representation naturally supports absence of some fields (like JSON or Protobuf).

I already compared the CourseUpdateRequest class with HashMap. In fact, CourseUpdateRequest is virtually HashMap with restricted set of keys (and keys are also typed).


@lex-em, are you sure it would work? I think Jackson will ignore hasSomeValue() method anyway, because it is not getter nor setter. So the annotation here is not required. But it is required on __hasSomeValue field! Because by default Jackson will serialize/deserialize not only fields that is accessible through setters/getters, but also any other private fields.

  @JsonIgnore
  public boolean hasSomeValue() {
    return __hasSomeValue;
  }

@rzwitserloot I'll repeat that has methods proposed here have nothing to do with JPA. JPA is ORM. It's not serializing/deserializing framework. It's unable to save fields partially. It always saves all fields of entity. So has methods are not useful in JPA context at all. They could potentially be used to generate raw UPDATE requests (including only fields for which has method returns true), but only theoretically and without JPA. I'm not aware of any framework that supports this.

What lex-em did mean is that JPA could be used together with MapStruct mapper together with Jackson deserialization together with JSON PATCH request together with has methods (as you do see in his last examples). But it could be any persistence technology, not only JPA. Or not persistence at all, as in my examples. As @filiphr mentioned, my examples looks very similar to what MapStruct generates. But there is a big difference. MapStruct just maps fields of one object to other object. My example calls business method.

My

if (orderDto.hasName()) {
    orderService.updateOrderName(order, orderDto.getName());
}

calls service method, that could do business logic of any complexity. It could be just orderEntity.setName(newName), or it could be a big BPMN workflow that touches 50 microservices.

What is similar in my example and MapStruct's generated mapper (or manually written mapper) is that they both need to know which fields were included in the received JSON.

So, I agree that this feature request is indeed very tied to serialization/deserialization, but don't agree that it is tied to MapStruct or, especially, JPA. If you ask me to select one framework that is tied to this feature, I would say Jackson (or any other JSON-serialization framework). Even Protobuf is not related here, because it already implements has methods in classes, that it generates. So, Lombok is not required for Protobuf models.

Also, I agree with @almson about OOP. I like his Holder solution. If I would be able to configure Jackson to properly serialize/deserialize these holders and all popular libraries, like MapStruct, will properly handle them, this feature will not be required. Unfortunately, I don't think this would happen. We even don't have such standard Holder class in JDK. If each library would use it's own, it wouldn't be interop.. It also could have a bad effect on performance (each field will be required to be wrapped with Holder), but maybe it's contrived problem.


I found an interesting example from Google Classroom API to update Course entity (@rzwitserloot it's funny that your example uses courses too). This API uses explicit updateMask query parameter because their server is unable to determine which fields on the course to update (they lack has-methods :) ). I think, they also use it to allow calling side to pass null-valued fields which still must be ignored (e.g., to allow calling side to not configure Jackson to not serialize unset fields, so if request was {name: null} or {name: John}, but didn't include name in updateMask query parameter, then name field of request would still be ignored).

If we would try to implement this feature to support server-side of this API, it would be implemented exactly the same way as earlier described, but deserialization code must not call setters of CourseUpdateRequest for fields which were not included in updateMask query parameter. And to implement client-side for this API, we would need to call all has methods of CourseUpdateRequest and include in updateMask only these fields, for which has method returns true.

So, this API uses totally different way of marking fields to "patch" (query parameter instead of excluding fields from JSON), but still fits well on this feature request. The only changing part is serialization/deserialization.

@lex-em
Copy link
Author

lex-em commented Dec 15, 2020

@lex-em, are you sure it would work? I think Jackson will ignore hasSomeValue() method anyway, because it is not getter nor setter. So the annotation here is not required. But it is required on __hasSomeValue field! Because by default Jackson will serialize/deserialize not only fields that is accessible through setters/getters, but also any other private fields.

@xak2000 yes, you are right - annotation is not needed on has method, but it is not needed on field too, because of jackson use accessors by default and has* is not part of java beans convention
but on the other hand, it would be useful to be able to add annotation to the field, not only to the has* method

@lex-em
Copy link
Author

lex-em commented Dec 15, 2020

@rzwitserloot

public @interface Has {
  String prefix() default "has";
  String suffix() default "Changed";
  AnyAnnotation[] onMethod() default {};
  AnyAnnotation[] onField() default {};
}

@lex-em
Copy link
Author

lex-em commented Dec 17, 2020

@rzwitserloot what do you think about implementation? you will do this?

@projectlombok projectlombok deleted a comment from randakar Dec 17, 2020
@rzwitserloot
Copy link
Collaborator

rzwitserloot commented Dec 17, 2020

I've had to go on a deletion spree due to some proseletyzing of optional, which is entirely inappropriate here. Optional is shot down for this, for extremely obvious reasons, and any comments that suggest it will be deleted. There are plenty of issues around where such discussion is begrudgingly allowed; this is not one of them.

EDIT: I now see github has 'hide' functionality. That's fantastic, but I saw that too late. I'll use that next time, though.

NB: To all except @randakar - your comments were fine, I just deleted them because they were in response to comments that I deleted. Your opinions are noted and agreed on :)

@projectlombok projectlombok deleted a comment from xak2000 Dec 17, 2020
@projectlombok projectlombok deleted a comment from randakar Dec 17, 2020
@projectlombok projectlombok deleted a comment from xak2000 Dec 17, 2020
@projectlombok projectlombok deleted a comment from lex-em Dec 17, 2020
@projectlombok projectlombok deleted a comment from randakar Dec 17, 2020
@projectlombok projectlombok deleted a comment from lex-em Dec 17, 2020
@rzwitserloot
Copy link
Collaborator

rzwitserloot commented Dec 17, 2020

We've spent a million comments on this. A lot more is clear, and yet, we are no further to a fully fleshed out proposal (@lex-em - your 'fully fleshed out proposal' is very far removed from what is needed! It doesn't explain how e.g. @AllArgsConstructor and @Has interacts, or @Builder and @Has, and makes no effort to try to ensure that invalid states (such as the __hasX field being false whilst the x field is non-null) are avoided).

Let's lay down what we do know, and what it missing. Please double-check my work.

Agreed on:

  • The mental model of this feature is to track two separate null-like states: "This property has not been set" vs. "This property has been set, to null".
  • At the core of this feature is the generation of a separate boolean field, named to indicate that no code (other than what lombok generates) should ever miss with this field (so, $ in the name).
  • It is never supposed to happen that the boolean is false (indicating "unset"), whilst the property field itself is non-null/0/false/'\0'.
  • The primary purpose of this feature is not protobuf; protobuf generates code with has methods and doesn't have any particular need for you to provide it classes with has methods.
  • The two core libraries/frameworks that benefit from this is mapstruct, and jackson.
  • The generated API is not really meant for consumption by humans writing code - these methods are generated solely to make jackson and mapstruct do what you want. That's because this purpose of conveying a tri-state scenario is better done with a Holder object.
  • Lombok is supposed to make things work right for setters, withs, and builders: if you do CourseChangeRequest.builder().name(null).build(), the produced CCR object should have .hasName() be true, but without that .name(null) in between, hasName() should return false. someCCR.withName("foo") should generate a new CCR object whose hasName() returns true. The setter will always set the boolean field to true, even if the parameter is null/0/false/'\0'.
  • The generated method will be called hasX using the same naming rules that turn a field named x into the setX method name. It will not be possible to mess with the naming scheme.

Open problems:

  • If the primary point of this feature is to make your lombok-assisted POJOs work better with mapstruct and jackson, then we have a problem: If the feature is generalized (so, @lombok.Has), then lombok cannot generate @JsonIgnore because it has no idea if you even have the jackson dep in your classpath. However, without marking the boolean field, jackson (and, presumably, mapstruct) will do the wrong thing, which means the simplest thing imaginable, where you annotate a field with @Has and do nothing else (no __onHelperFieldeld=@JsonIgnore or whatnot), the produced code is not useful for any meaningful purpose. That would be a showstopper. Fixable by presumably either making it @lombok.extern.jackson.Has, but then we also need @lombok.extern.mapstruct.Has, or by building into the feature that just @Has, on its own, is an error, as it must be accompanied by @Jacksonized or a to be created @Mapstructized? I don't like any of these options.
  • Lombok cannot guarantee that the boolean field is properly tracking. Imagine this:
class CourseChangeRequest {
    @Setter @Getter @Has String name;

    public void copyNameFrom(Course other) {
        this.name = other.getName();
    }
}

This is a bug - now your CCR is broken, because it should have written setName(other.getName(); instead. One way out is for lombok to mutilate your fieldname (turn it into something with a $ in it, to clue the programmer into awareness that they need to understand how this feature works before messing with the field).

  • Lombok's interaction model with constructors is unclear. How would @Has and @AllArgsConstructor interact? Given that @Builder must do the right thing, and @Builder on a class is just syntax sugar for 'make the all args constructor and act as if builder is on it', this is a very important question that needs resolving: How would a builder even instantiate an object such that the boolean field is false, or true, as required?

  • IS that boolean the right way to go about it? Had there been some actual Holder class, lombok could turn:

@Builder @Value
class CourseChangeRequest {
    @Has @Getter private String name;
}

into:

class CourseChangeRequest {
   private final Holder<String> name;

   public String getName() {
       return name.get();
    }

    public boolean hasName() {
        return name.isPresent();
    }

    private CourseChangeRequest(@NonNull Holder<String> name) { this.name = name; }

    public static CourseChangeRequestBuilder builder() { return new CCRB(); }

    public static class CourseChangeRequestBuilder {
        private Holder<String> name = Holder.unset();
        CourseChangeRequestBuilder() {}

        public CourseChangeRequestBuilder name(String name) {
            this.name = Holder.of(name);
        }

        public CourseChangeRequestBuilder unsetName() {
            this.name = Holder.unset();
        }

        public CourseChangeRequest build() { return new CourseChangeRequest(name); }
    }
}

Where, presumably, jackson automatically gets it right because it realizes that the getName() and hasName() methods are to be used to serialize the name property (so, ignore the field entirely), and to deserialize, use the builder, and don't even invoke name if it isn't in the source JSON string that is being deserialized. Is that how jackson works?

Holder would be something like:

@AllArgsConstructor(access = AccessLevel.PRIVATE)
public final class Holder<T> {
    T value;
    boolean set;

   // appropriate suppresswarnings magic here
    private static final Holder UNSET_SINGLETON = new Holder(null, false);
    public static <T> Holder<T> of(T value) { return new Holder<T>(value, true); }
    public static <T> Holder<T> unset() { return (Holder<T>) UNSET_SINGLETON; }

    public boolean isPresent() { return set; }
    public T get() { return value; }
}

Lombok cannot, however, contribute any types here, because lombok is not present at runtime, and there is no suitable Holder anywhere in the JDK, so that's quite a big issue that needs addressing first, then.


In all the traffic I may have missed answers to some of my open issues, but without a really good answer to all of them I'm closing this one. Too complicated for not enough gain.

@xak2000
Copy link

xak2000 commented Dec 18, 2020

I'll try to answer some of the questions, but not all.

However, without marking the boolean field, jackson (and, presumably, mapstruct) will do the wrong thing, which means the simplest thing imaginable, where you annotate a field with @Has and do nothing else (no __onHelperFieldeld=@JsonIgnore or whatnot), the produced code is not useful for any meaningful purpose.

This is actually not a problem. @lex-em is right, that Jackson by default will not serialize/deserialize any private fields. It could be configured to do it, but by default Jackson uses presence of getter as an indicator if field must be considered as a property (so, participate in ser/deser) of just be ignored.

I tried it on this class:

    @ToString
    @EqualsAndHashCode
    private static class RequestWithPrivateField {
        private String foo; // no getter/setter - will be serialized/deserialized? No.

        @Getter @Setter
        private String bar;

        public RequestWithPrivateField() {
        }

        public RequestWithPrivateField(String bar) {
            this.bar = bar;
        }
    }

With this code:

ObjectMapper objectMapper = new ObjectMapper();
String json = "{\"bar\": \"bar-value\"}";
RequestWithPrivateField request = objectMapper.readValue(json, RequestWithPrivateField.class);
String jsonSerialized = objectMapper.writeValueAsString(request);
log.info("Request deseriazlied from {} into {} and serialized back into {}", json, request, jsonSerialized);

And the output is:

Request deseriazlied from {"bar": "bar-value"} into App.RequestWithPrivateField(foo=null, bar=bar-value) and serialized back into {"bar":"bar-value"}

Fixable by presumably either making it @lombok.extern.jackson.Has, but then we also need @lombok.extern.mapstruct.Has

I don't think this feature is directly related to Mapstruct. Yes, mapstruct can use has* methods if they are present, but that's all. Mapstruct doesn't require them and doesn't require any annotations on them. So, @lombok.extern.mapstruct.Has will be wrong way to go.

Where, presumably, jackson automatically gets it right because it realizes that the getName() and hasName() methods are to be used to serialize the name property (so, ignore the field entirely), and to deserialize, use the builder, and don't even invoke name if it isn't in the source JSON string that is being deserialized. Is that how jackson works?

For serialization stage, unfortunately, no. Jackson don't use has* methods to determine if field should be serialized or not. Jackson has an annotation where you can set some strategies to determine if field should be serialized or not (like: only when non-nul, only when collection is not empty etc), but it is unable to use has* methods for this purpose. Maybe if could be configured to do it using @JsonInclude(value = CUSTOM, valueFilter = MyCustomHolderFilter.class) (but this could be problematic, as getters will return unwrapped value instead of Holder and a filter implementation will unable to do holder.isPresent()). Another possibility is to use @JsonInclude(value = NON_DEFAULT) which works by creating an instance of POJO using zero-argument constructor, and accessing property values: value is used as the default value by using equals() method, except for the case where property has null value in which case straight null check is used. I didn't test any of these methods, though. This also could be problematic as Jackson will use getters and not Holders..

On deserialization, Jackson just calls setters for any field, that is present in source JSON. This is fine. You can also configure it to use a builder, but this is not required.


You raised a good questions. I don't have answers to all of them. :(

I like you idea with Holder, as it would be encapsulated state that getters not expose. But with all other unanswered questions (and with the fact that Lombok is not present in runtime, so it can't add it's own Holder implementation) I give up on this feature. Good in theory, but too many quirks to consider and solve.

@rzwitserloot
Copy link
Collaborator

This is actually not a problem. @lex-em is right, that Jackson by default will not serialize/deserialize any private fields. It could be configured to do it, but by default Jackson uses presence of getter as an indicator if field must be considered as a property (so, participate in ser/deser) of just be ignored.

Wow! Sweet. Yeah that definitely helps. That's one of my open issues eliminated.

For serialization stage, unfortunately, no. Jackson don't use has* methods to determine if field should be serialized or not. Jackson has an annotation where you can set some strategies to determine if field should be serialized or not (like: only when non-nul, only when collection is not empty etc), but it is unable to use has* methods for this purpose.

Let's go back to the very first post - the original request:

@lex-em Some frameworks have support of hasXXX methods, for example mapstruct: if field data have additional method boolean hasData() then generated code will call this method and do check around setting target field value.

What am I missing? These two statements seem like they contradict each other.

I've also solved my own holder problem, sort of: We can do it! With an AtomicReference. Turn:

class Example {
    @Has private String name;
}

into:

class Example {
    private final AtomicReference<Object> $lombok$name = new AtomicReference<Object>();

    public void setName(String name) {
        // if the atomicref refs itself, that means 'it is set; to null'.
        // if the atomicref refs null, that means 'it is unset'.
        $lombok$name.set(name == null ? $lombok$name : name);
    }

    public void unsetName() {
        $lombok$name.set(null);
    }

    public String getName() {
        Object v = $lombok$name.get();
        return v == $lombok$name ? null : (String) v;
    }

    public boolean hasName() {
        return v.get() != null;
    }
}

We can use a lot of the code from @Getter(lazy=true) for it. It's quite possibly still far too complicated compared to what is gained, though. AtomicReference's equals/hc impl don't exist (they fall back to Object's, and that is intentional - an AtomicRef is equal to itself, and not equal to another AR that so happens to be referencing the same object), so lombok's own equals/hashCode impl needs to take this into account, but that's not too difficult. A bigger issue is how to make an immutable take on this (with e.g. @Builder Value public class Example { @Has String name; }) - Presumably we should then make a constructor that takes AtomicReference<Object>, which is bizarre, and we can't stick a dollar in the constructor's name. Quite problematic: Will lombok users realize and accept that the constructor, whilst visible in e.g. their outline view and auto-complete dialogs within the same source file, makes no sense and shouldn't be called?

We could, presumably, demand that the field is non-final, and have @Has generate the whole bevy (getter, setter, hasMethod, possibly unSetter), and still allow a builder which will just invoke no-args and then a bunch of setters, but that's a lot of work.

So, current open questions:

  • How does mapstruct relate to hasMethods. Is there any interaction at all? Are mapstruct-specific annotations required?

  • Is this mangling of your field actually understandable? It's nice that we've talked through the issues and that this seemingly crazy plan actually solves many problems, but if the intended audience for this feature just doesn't grok what is going on...

@filiphr
Copy link

filiphr commented Dec 19, 2020

I'll try to answer some of the questions related to MapStruct and maybe chime in on some of the other points.


How does mapstruct relate to hasMethods. Is there any interaction at all? Are mapstruct-specific annotations required?

The place where it makes the most sense for users to use hasXXX methods (we call them presence check methods) for MapStruct is for Updating existing bean instances.

The users write something like:

@Mapper(nullValuePropertyMappingStrategy = NullValuePropertyMappingStrategy.IGNORE)
public CourseUpdateMapper {

    void updateCourse(@MappingTarget Course course, CourseChangeRequest request);

}

The code that MapStruct generates looks like:

public class CourseUpdateMapperImpl {

    public void updateCourse(Course course, CourseChangeRequest request) {
        //...

        if (request.getName() != null) {
            course.setName(request.getName());
        }
        //...
    }

}

When hasXXX (presence) methods are there then MapStruct generates the following code

public class CourseUpdateMapperImpl {

    public void updateCourse(Course course, CourseChangeRequest request) {
        //...

        if (request.hasName()) {
            course.setName(request.getName());
        }
        //...
    }

}

Basically extremely similar to what @xak2000 explained in #2669 (comment).

Long story short, in my opinion there is nothing MapStruct specific needed in Lombok regarding the @Has annotation. We will either use the method (if it is available in the abstract syntax tree 😉 or not).


Regarding the @AllArgsContstructor and @Builder. There are 2 scenarios here:

1. @AllArgsConstructor without @Builder

In this case the only possible solutions is all those @Has fields to be set to true as there is no other way of knowing whether the passed value in the constructor parameter means missing value or not. In addition to that, when having an all args constructor then the only way to create the object is by explicitly giving some value for all the properties.

2. @AllArgsConstructor with @Builder

Implicitly from the first point having @AllArgsConstructor will always set the has fields to true.
However, having the @Builder means that you can actually track whether the properties have been set or not.
From what I can see in the generated byte code the builder uses the all args constructor to create the object itself. This makes it a bit more complicated.

One option would be to create the object and then set the hasXXX fields in the object.

e.g.

public CourseChangeRequest build() {
    CourseChangeRequest courseChangeRequest = new CourseChangeRequest( title );
    courseChangeRequest.$lombok$hasTitle = this.$lombokHasTitle;
    return courseChangeRequest;
 }

This of course makes it complicated if you mix the @SuperBuilder or inheritance as the hasXXX fields are most likely private and not accessible for the parent classes.

What you could do instead is to have a protected (for the @SuperBuilder) constructor that takes the builder object as the input parameter. If you do that you can then set the hasXXX fields without a problem in the constructor and you won't have the inheritance problem. This most likely is a bigger change as it changes how the constructors are created in Lombok (I have no knowledge about this)

@janrieke
Copy link
Contributor

What you could do instead is to have a protected (for the @SuperBuilder ) constructor that takes the builder object as the input parameter.

That's what @SuperBuilder already does. So I also think this is solvable for @Builder and @SuperBuilder.
@With could use a similar approach as @Builder, where every with method sets the internal @Has fields.

However, the maintenance burden for this would probably be very high, because of all these interactions with other features.

@xak2000
Copy link

xak2000 commented Dec 19, 2020

For serialization stage, unfortunately, no. Jackson don't use has* methods to determine if field should be serialized or not. Jackson has an annotation where you can set some strategies to determine if field should be serialized or not (like: only when non-nul, only when collection is not empty etc), but it is unable to use has* methods for this purpose.

Let's go back to the very first post - the original request:

@lex-em Some frameworks have support of hasXXX methods, for example mapstruct: if field data have additional method boolean hasData() then generated code will call this method and do check around setting target field value.

What am I missing? These two statements seem like they contradict each other.

I described serialization stage and behavior of Jackson (when you already have an object with has methods and want to serialize it to JSON, and you also want that JSON to not contain fields, for which has returns false). This case is not solved, because Jackson doesn't check has methods at all.

@lex-em described deserialization stage and behavior of Jackson + Mapstruct (that operates with already deserialized data (java object)). So, this is the case, when you already have partially filled JSON and want to:

  1. Deserialize it into a java object. This is solved by Jackson already, as it doesn't call setters for properties, which do not exist in JSON. By "solved" I mean the fact that Jackson just calls setters and that's all. These setters still must track what fields has been set, and what fields are not, if we want to use this information later from business-logic.
  2. Do business logic. In @lex-em's case, business logic is simple: just update fields of domain object (but only fields, which was present in JSON). It's very common "business-logic" in many CRUD applications. This is solved by Mapstruct, which understands has methods.

So, there is no contradiction here. They are 2 different use cases: serialization vs deserializtion. Deserialization case will be solved with presence of has methods. Serialization case will not, unfortunately. Maybe someone more familiar with Jackson, than me, tells how to configure Jackson to use custom object methods to determine if field should be serialized or not... I'm not aware of such possibility.

@janrieke
Copy link
Contributor

janrieke commented Dec 19, 2020

Serialization case will not, unfortunately. Maybe someone more familiar with Jackson, than me, tells how to configure Jackson to use custom object methods to determine if field should be serialized or not... I'm not aware of such possibility.

There is @JsonFilter, but that requires programmatically adding a filter to the ObjectMapper (so nothing Lombok can generate automatically).

@filiphr
Copy link

filiphr commented Dec 19, 2020

Regarding the Jackson deserialization stage, I would advise someone to open an issue in the Jackson tracker to support this kind of hasXXX methods. Jackson has something called JsonInclude that can be used to control when content needs to be included in the serialization or not. Having an option to use such presence methods is something that might be worthwhile for the Jackson team as well.

@lex-em
Copy link
Author

lex-em commented Dec 23, 2020

Regarding the Jackson deserialization stage, I would advise someone to open an issue in the Jackson tracker to support this kind of hasXXX methods. Jackson has something called JsonInclude that can be used to control when content needs to be included in the serialization or not. Having an option to use such presence methods is something that might be worthwhile for the Jackson team as well.

I think this will be a good new feature in jackson!

But now we are considering a case of deserialization at first. @rzwitserloot I think enough to use @Has only with setters to start
What about @*Contructor and @Builder... I agree with @filiphr #2669 (comment).
I think if anyone need tri-state fieds in dtos, they should just use a plain pojo with @Getter, @Setter (maybe @Data) and @Has.
Maybe builders and constructors can be designed later, when we get a clearer understanding.

what opened issues do we have now?

@rzwitserloot
Copy link
Collaborator

Since my last comment this discussion seems to be heading back to the setup where @Has fields are exploded from @Has String name; into:

String name;
boolean __hasName;

vs my idea of changing that to:

AtomicReference<String> $lombok$name;

The problem with the first 2 is: Given that the field is left intact, the author of the code is likely going to write, for example, name = "x";, which isn't actually allowed, because this doesn't update the presence info (the setter must be used): It asks the question: What does __hasName = false and name is not null even mean? Presumably, it means: A bug has occurred, but that means a programmer wrote bad code, and the correct move is that this code preferably does not even compile in the first place / causes warnings, and second-best is that an exception is thrown right as the bug occurs. This third option (the object is in an invalid state) is not acceptable. I cannot accept the feature request as is without some serious arguments as to why this gigantic hole in the design is allowed to stand.

So, one major unanswered question now is remains: What should lombok actually do in the face of this @Has annotation? It's still not at all obvious. Adding that __hasName field feels like a big mistake, in that it makes it very easy for a user of the @Has annotation to mess up, but my idea of taking control of the field entirely doesn't seem to be liked.

Separate from that, given that interactions required with the already very complicated @Builder, I'm still skeptical this is a good idea.

What is really working out right now, though, is the consensus on what this does and what it should be called: 'presence' (I'm even thinking the annotation might need to be @Presence and not @Has), and as a concept it is clean enough that we can e.g. ask jackson to take these 'presence' methods into consideration for serialization. So that helps. We can move away from lombok.extern: This is about a concept useful to programming as a general idea, and not about supporting specific libraries.

@janrieke
Copy link
Contributor

janrieke commented Dec 23, 2020

No, I think we were all talking about the AtomicReference idea (at least I was). I fully agree that leaving the field intact is a bad idea.
What's making it complicated is that if @Presence is present, @(Super)Builder and @With have to generate different code, because they cannot simply use the all-args constructor, but have to modify the AtomicReference afterwards to distinguish between null and "not set". That means quite an explosion of use cases and test cases.

@kokorin
Copy link

kokorin commented Dec 28, 2020

Recently I published to maven central lombok extension to do exactly what OP is talking about: lombok-presence-checker

It does the following:

// Original code
@PresenceChecker
@Getter
@Setter
public class UserUpdateDto {
    private String name;
}

//Generated code
public class UserUpdateDto {
    private boolean hasName;
    private String name;

    public String getName() {
        return this.name;
    }

    public void setName(String name) {
        this.name = name;
        this.hasName = true;
    }

    public boolean hasName() {
        return this.hasName;
    }
}

The annotation works on class and field levels. It works well with MapStruct. Check an example

@rzwitserloot
Copy link
Collaborator

@kokorin Nice to have a PoC to play with, but what do you think about the problems I raised? For example, someone could write this code:

@Presence @Getter @Setter
public class UserUpdateDao {
    private String name;

    public void setNameToAnonymous() {
        this.name = "(anonymous)";
    }
}

That code seems fine, but it is broken because it forgot to update the hidden hasName field. I don't like that. It also plays havoc with reflective systems which see that hasName field and may do unexpected and undesired things because of it.

@kokorin
Copy link

kokorin commented Dec 28, 2020

@rzwitserloot Actually lombok-presence-checker doesn't generate a setter itself, but it requires a setter to be either generated with lombok, or to be written manually. During code generation it alternates a setter and adds this.hasName = true as a last statement.

The main use case for the extension is DTO partial update.

Personally I don't want this extension to be included into core lombok. If anyone uses it he/she should understand that problems may appear. I posted a link to my project here only to point that it's not required to bother core lombok authors to support hasXXX() methods (and to promote my project of course)

P.S. lombok-presence-checker is really an extension, not a fork.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
parked Without further feedback this bug cannot be processed. If no feedback is provided, we close these.
Projects
None yet
Development

No branches or pull requests

10 participants