projectlombok / lombok

Very spicy additions to the Java programming language.
https://projectlombok.org/
Other
12.9k stars 2.39k forks source link

Feature request: @ToString allows skipping null-value fields #1752

Open chriswilty opened 6 years ago

chriswilty commented 6 years ago

It would be nice if I could skip null-valued fields in the generated toString(), e.g. with

@ToString(excludeNulls = true)
class MyClass { ... }

As Optionals are not supported by lombok, I may have one or more fields that are nullable in my DTO, and which do not need to be set during construction.

This is a nice-to-have; it's not the end of the world to need to copy the generated toString and add in my null-exclusion logic, nor indeed to see "fieldName=null" in the toString output, as I only use it for logging. If it's not an easy win, don't worry about it :grin:

mohasaid commented 6 years ago

I could work on it, let's see if someone confirms me.

Maaartinus commented 6 years ago

@chriswilty This issue is a duplicate of quite a few -- IMHO erroneously -- closed issues, as stated in this comment.

@mohasaid The idea has been discussed since ages and implementing it without helper methods is said to be a lot of work. And it's getting more complicated as new features get added, like using methods rather than fields only.

It'd make sense to add skipNulls as an option to the nested annotation @ToString.Include, too.

Anyway, you'd better await an OK by the project owners.


PS: There are related feature requests like skipping empty fields, but it's unclear what "empty" means in general. For a field like private SomeType amIEmpty, it's unclear, how to test it. SomeType could be a descendant of Collection to be tested with amIEmpty.isEmpty() (or amIEmpty.size() == 0) or an implementation of CharSequence to be tested with amIEmpty.length() == 0. Actually, it could be both.

The only good news is that it can't be an array. This can of worms should probable be left closed.

chriswilty commented 6 years ago

@Maaartinus Thanks for the input, and the link to #1297, which I'd managed to miss.

I feel that issue has exploded a bit with all the "oh, and how about if it did this..." issues linked. In contrast, the concept of nulls being a special case is already part of lombok (@NonNull and the @-ArgsConstructors) which is why I felt it would be a natural addition.

I deliberately didn't include a request for @ToString.(In|Ex)clude because I foresaw that it could be a springboard for all sorts of other requests to expand the functionality of those, however, for the sake of completion and consistency it might make sense to augment those also. I'm not sure that it makes sense to have skipNulls as an option to @ToString.Include as skipping is an exclusion; probably something like @ToString.Include(onlyNonNull = true).

Anyhow, as stated it's not a big problem to need to write my own toString, so I won't be upset if this issue is closed also :)

Maaartinus commented 6 years ago

@chriswilty

I'm not sure that it makes sense to have skipNulls as an option to @ToString.Include

My only problem with this is that it mayn't be very useful. Quite often, you want to include (or exclude) some fields, quite often you want to exclude all nulls, and probably only rarely you want to include some field unconditionally and others only when not null.

as skipping is an exclusion; probably something like @ToString.Include(onlyNonNull = true).

IMHO the name should be the same as @Whatever.Include is (a sort of) the per-field variant of @Whatever. It just has double duty: Ensure that the field gets considered AND optionally override options. While some options make no sense in one place (e.g., callSuper or rank), others do. Don't let English grammar details take over logic (see SQL or Javabeans for where it leads to).

I'd read @*.Include(skipNull = true) like "include this field, but skip it if null". It's still an inclusion, it's just conditional. ;) More importantly: It's an override for the equally-named option on the master annotation.

IMHO @*.Exclude should never get any options. In this case, conditional exclusion is tempting, but conditional exclusion is the same thing as conditional inclusion, just with the inverse condition. This would lead to needless ambiguities.

thorvx commented 5 years ago

any news about this request?

remigiusv commented 5 years ago

we expect this feature to be prioritised

randakar commented 5 years ago

Based on what?

On Fri, Jul 19, 2019 at 7:49 AM remigiusv notifications@github.com wrote:

we expect this feature to be prioritised

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/rzwitserloot/lombok/issues/1752?email_source=notifications&email_token=AABIERLIY2I7ONEEMNIRF4DQAFIYLA5CNFSM4FICT4TKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOD2KUS6A#issuecomment-513100152, or mute the thread https://github.com/notifications/unsubscribe-auth/AABIEROKSQ75GCM7TAHGLDLQAFIYLANCNFSM4FICT4TA .

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

remigiusv commented 5 years ago

toString implementation needs an option of printing only non null members of the class we annotate. Badly its needed for better logging.

randakar commented 5 years ago

If you have so many fields that you desperately need filters in your toString implementation your class is doing too much at once, imho.

On Tue, Jul 23, 2019 at 6:57 AM remigiusv notifications@github.com wrote:

toString implementation needs an option of printing only non null members of the class we annotate. Badly its needed for better logging.

— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/rzwitserloot/lombok/issues/1752?email_source=notifications&email_token=AABIERNAPGW56UOPAYNBVVLQA2FVBA5CNFSM4FICT4TKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOD2R5P4I#issuecomment-514054129, or mute the thread https://github.com/notifications/unsubscribe-auth/AABIERPWHWFCYLUEFC4I57LQA2FVBANCNFSM4FICT4TA .

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

valentinbrasov commented 4 years ago

Any progress on this? And by "this" I mean allowing something like @ToString(excludeNulls = true) at the top level (class level), not something on each individual field. I am working on a project and we are using lombok, except the @ToString for this very reason, because it would pollute the logs by listing null fields. So as it is (without the capability to skip null fields), the @ToString is useless to us.

rzwitserloot commented 4 years ago

telling an open source dev 'this needs a higher priority' is a great way to cause demotivation. Not fair to the great contributors we do have, but, @remigiusv – you need to work on your manners. I don't work for you! If I could write this feature such that it wouldn't work for just you, I'd do it. Fortunately, open source doesn't work that way, but, cripes. Way to be a downer.

Not a high priority right now. The problem with adding 'just this feature' without considering the greater scope is that if we DO eventually get around to rethinking how to make ToString more configurable, perhaps we then realize that we have to deprecate this feature, or we now have two ways to accomplish the same thing which is a needless raising of learning curve and opening the door to style debates.

rmannibucau commented 3 years ago

+1 for this feature, I can see some cases it is undesired but for all DTO (in particular in microservice erea) it is really a code and time saver to be able to skip nulls (and lombok is literally about it right?).

rspilker commented 3 years ago

What about other default values? Like 0, 0.0, false, '\0'? What about the empty string? Or even skipping every value that has the same value as the field initializer expression, or even take constructors into account?

What makes null so special that it deserves explicit and special handling?

rmannibucau commented 3 years ago

@rspilker the fact it can be not initialized - which is not true for primitives. For example in a DTO, adding a "integer" (functionally) you can choose to make it appearing or not by using Integer or int so a natural toString implementation would be enabled to follow that. That said if you go the @ToString.SkipIfEquals("...") path I'm more than happy with it too while null can have a "on class" toggle (= good default) since it is still the most common in most application (even if I agree some only use primitive but it is not mainstream at all). Finally, empty string and initializer cases don't make sense for me since it is about representing the values. Only specifity is: do you keep nulls or not in the reprensentation.

I guess we can take inspiration from most common JSON setups there, generally null are ignored and you can force them to be shown. All other settings are rarely used (for "tostring" case) so I guess only supporting null is a huge step forward and that if can be refined for primitives later if needed but i'm not sure primitive really pollute the output as much as strings/objects in practise.

YaRuliY commented 3 years ago

What about other default values? Like 0, 0.0, false, '\0'? What about the empty string? Or even skipping every value that has the same value as the field initializer expression, or even take constructors into account?

What makes null so special that it deserves explicit and special handling?

what you listed are value and null is absence of value

dstango commented 2 years ago

Didn't we recently have in some issue the idea of having a configurable toString-implementation? I think someone was asking for JSon-output (or even xml?) ...

If we could specify something e.g. like: "toString.defaultImpl=my.special.package.MyPrettyFormatter.objectToWhateverILike", and have the an option for individual @ToString-annotations to override the default everyone could simply plug in their own make-me-happy-toString-implementation to their likings without bothering the lombok-team any further about individual preferences ...

aleksej-turko-dgn commented 1 year ago

sometimes i need other value if the field is null.. if user.email is null, i could have user.name and user.surname :) what about user.somefield with @ToString(converter=MyValueToStringConverter.class) (with Object Referenz)

rzwitserloot commented 1 year ago

@aleksej-turko-dgn That's not a good idea. We already support such a mechanism, via annotating a method with @ToString.Include which has the same name as your field (or, just specify the field name in the @Include argument) where you can write this logic. The problem with your approach is that for your idea, there are 50 other slightly exotic situations, thus, it turns the annotation parameters into a really crappy Domain Specific Language (crappy because annotation params are very limited indeed).

rzwitserloot commented 1 year ago

@dstango Maybe. There are a ton of requests for more customization of toString's outputs, but so many of them strike me as 'well, if lombok grows enough warts to cater to them all, the sheer amount of crud you have to shove into annotation parameters to configure it all / the additional code you need to write in 'converters' and the like feel like they add up to just as much code as just writing the toString yourself).

There's:

The problem is, some of those seem veto-ably silly to me, some seem great but I do not see a route as to how to deliver it without breaking rules (such as the isEmpty() thing - we aren't going to go with structural typing. Java isn't a structurally typed language. And there isn't an Emptyable annotation like there's an AutoClosable either) - which leaves a few that are useful and are doable, such as this, but just ignoring the other stuff for now and delivering the simplest thing that could work on the few things that are possible just feels like being in a hole and digging even deeper: Without considering the wide range of effects, any changes we make now just cramps our style or forces us to break compatibility later if ever we do find neat ways to deliver on as much as is requested in a decent way.

So far our general point is: These are mildly useful things for debugging purposes primarily, not things that should be expertly crafted to look juuuust right. In other words, if you:

Then our advice is not to use @ToString at all. (Also, our advice is _not to use toString() for these purposes!) - and the toString() docs agree with us, at least on the first 2 items in that list.

All these requests have in common that they are either distant nice-to-haves for the 'use for casual debugging' purpose, or even that it has no effect on that usecase at all and it's solely to cater to the above 3 items. So, so far, the easy solution is: ToString just isn't meant for that. We cater only to the 'nicer debugging output' thing.

We aren't opposed to making @ToString do more. But we are opposed to one-off fire-and-forget actions. We want something that feels particularly well-thought-through.

A few ground rules to keep in mind:

This issue remains in that weird in-between space where it's a decent enough proposal but we see no way to design a way to deliver on it, except things that feel incomplete.

dstango commented 1 year ago

@rzwitserloot you summarize all sorts of requests that have been made, and I agree Lombok shouldn't grow a @ToString-annotation-with-a-multitude-of-parameters-to-rule-them-all.

Yet: where's the trouble in providing a config property that could override Lombok's default implementation easily by referencing a static method that would do whatever a user requests. If the implementation is sub-optimal to whatever security-, nullability-, Optional-, emtpy-, or other concerns that's not the responsibility of Lombok, but the user's. Sounds right to me: more freedom for the user comes with more responsibility ... Thus: all the valid points you mention become trade-offs to be dealt with by the user.

Of could one could do a simple toString()-implementation for every single class, that calls that exact static method mentioned. Thus one would replace @ToString with an individual toString()-method. No big deal.

Yet it becomes really handy if I use @Data, that has an implicit @ToString. It keeps the code nice, clean, concise. And if that uses the configured static @ToString-method we can keep the class clutter-free ...

Just my 2 ct ...

rzwitserloot commented 1 year ago

@dstango How do you configure where lombok should call out to? The simplest thing I can think of, is that you stick a fully qualified class name in some lombok.config key and lombok then generates toString implementations that refer to that class or method you named in the config key.

But that is extremely nebulous as a definition. What does that actually mean? It sounds like you want:

> cat lombok.config
lombok.toString.customProvider = com.dstango.util.ToStringImpl

> java -jar lombok.jar delombok TrivialExample.java

class TrivialExample {
  private final int a;
  private final String b;

  @Override public String toString() {
    return com.dstango.util.ToStringImpl.toString(this);
  }
}

But that sucks! The only way ToStringImpl can do anything meaningful here is to go on a reflection spree, and every version of OpenJDK makes that more and more complicated. Surely that cannot be the right answer. Instead of 2 cents, work something specific out. I'm going to veto this entire debate as just an utter waste of time until someone comes up with actual code examples. So far I'm mostly listening to a lot of 'I really really want this thing' (notably including having to wipe out a whole boatload of pointless '+1' comments) with virtually zero '... and this is how we can assuage some concerns'. Oh I get it, you really really want this thing. So do I. I also want world peace, and a pony. It needs to be somewhat practical and attainable or it's just frustrating / time wasting.

dstango commented 1 year ago

@rzwitserloot thanks for your quick response, and: yes, you got me right and your example fits. (I thought I had sketched it already in my earlier message, but that was not fleshed out -- sorry.)

And yes: the way I see a realization is mostly via reflection. If you want e.g. some json you can use gson or whatever library you like to produce some serialization. This is something happening all the time in real world projects, and I don't see what's speaking against it, and I can't imagine reflection going away as it's so widely used. You might say 'performance is worse', but we're talking primarily about a debug output here, so that doesn't count.

You're saying reflection is getting more and more complicated. I'm not sure what you're exactly referring to -- maybe the stricter encapsulation of internal classes, and consequences of applying the module system more strictly? I'd argue: these things don't really matter, as the classes I annotate with @ToString are in my own code, so I have full access to them, and should be able to use reflection on them. But maybe you have something else in mind.

Anyhow: it might be an option to have several overloaded static toString()-methods with different types to handle different cases. While that might be useful sometimes (e.g. for certain child classes from common parent classes that should be handled in a similar way), a degenerated variant would be to have such a static toString()-method for every class with the @ToString-annotation, which would be an implementation from the category: "how to write unmaintainable code".

Regarding the pony etc.: I have no strong desire for this feature (I'm usually happy with the current implementation), but I saw so many different feature requests around @ToString-special-features, that I thought: "why not provide some extension point, that opens a practical way to handle such situations, while at the same time frees you and the Lombok team from putting any energy into such specific request like "please hide my password fields", "convert null to an empty string", "leave out field if vale == null", "produce json/xml/yaml/..."

But if the most obvious implementation idea for such a toString()-implementation is reflection, and you don't want to provide a way that often would result in reflection usage -- then I don't have any idea how to realize it. If you want to force reflection-less toString()-implementations, then you'd either have to provide a multitude of annotation-parameters (meh...), or come up with a lomboqueske template-language to generate highly flexible source code for each individual toString()-implementation (I guess that's out of scope for Lombok, and would be a maintenance burden anyhow) ...

So: sorry, not more than another 2 ct from my side -- I'm out of magical ideas, if reflection in the user's implementation is out of the question ...

Tobias4git commented 1 year ago

May I point back to feature request https://github.com/projectlombok/lombok/issues/1752? If a user could iterate at runtime over all fields and get their values, she or he could easly exclude null values or hide passwords based on the specific field or even based on the type.

rmannibucau commented 1 year ago

Think it does not make much sense to plug a custom "converter"/mapper cause at the end it means the user should just override the underlying tostring method. Handling null (or not) makes sense cause it is generic but agree other cases can be too much.

rzwitserloot commented 1 year ago

If you want e.g. some json you can use gson or whatever library you like to produce some serialization.

Well, there we go. From a java design perspective, having your toString() emit JSON is something I would instaflag as a knockout bug. That's not what it is for. If you want to write some java code that does do that, more power to you. Far be it from me to complain about hackery and using things for purposes it wasn't really intended to :) - but where I draw the line is: You can't then call that 'boilerplate'. And if it's not boilerplate, lombok has no business catering for the use case.

If we add this feature and it requires reflection to use it, the next thing that is going to happen is about 50 bugs a day filed about WTF that's about. Gathering up all the requests and takes on what folks expect a 'ToString is now more flexible' feature would look like, that caters to a tiny slice of them and annoys the other slices.

No can do.

rzwitserloot commented 1 year ago

@Tobias4git wrote:

If a user could iterate at runtime over all fields and get their values

Just write toString() if you want to do that. I'm going to have to yell at you too. I will summarize your comment:

You're wasting everybody's time.

New rule: If you propose a way forward, either it includes code that shows how to write it with lombok and what lombok delomboks that to, or I delete it.

Tobias4git commented 1 year ago

I'm sorry, I put the wrong link in the comment. I meant https://github.com/projectlombok/lombok/issues/3328 and there is the example code. In my example was the code for the password - not for a Null check.

@Data
@FieldNameConstants(asEnum = true, innerTypeName = "jobProps")
public class LombokTest {

private String name;
private String password;

public String toString() {
    StringBuilder result = new StringBuilder();
    for (jobProps value : jobProps.values()) {
        if (value.getValue(this) != null) {
            result.append(value.name() + "=" + value.getValue(this));
        }
    }
    return result.toString();
}

I thought that "rule" already exists - sorry for the wrong link.

rzwitserloot commented 1 year ago

@Tobias4git Ah now your comment makes a lot more sense :)

I've had to shoot down #3328 unfortunately. The same problem it has (namely, you have to write loads of unchecked casts (as in, casts that are just wrong if you fatfinger it and are rather error prone, not that you get the warning)) - applies to trying to use that fix here, too. Well, not specifically here - but string concat is pretty much the one and only place in all of java where just shoving objects in is fine. Other than toString, letting @FieldNameConstants have a getValue() methods is useless unless you accept tons of unchecked casts. Hence, why we're not doing #3328.

Tobias4git commented 1 year ago

I see your point - although I think there are other generic "containers" where it would be useful without the need of casts, for example java collections/arrays or database statement wrappers. We could make the logic the other way round, instead of a getValue somthing like putValue, the casting or type switch would be done on the receiving side. I dont know if the interface should be generated or a predefined abstract class to be overwritten at customer needs.

public class LombokTest {

    // vvvvvvvvvvvvvvv generated by lombok vvvvvvvvvvvvvvv
    public interface Container {
        void putValue(String string) throws Exception;
        void putValue(java.util.Date date) throws Exception;
        void putValue(Integer integer) throws Exception;
    }

    static enum jobProps  {
        name, password, dateOfBirth, numberOfKids;

        public Object getValue(LombokTest instance) {
            if (this == jobProps.name) {
                return instance.name;
            } else if (this == jobProps.password) {
                return instance.password;
            } else if (this == jobProps.dateOfBirth) {
                return instance.dateOfBirth;
            } else if (this == jobProps.numberOfKids) {
                return instance.numberOfKids;
            }
            return null;
        }

        public void putValue(LombokTest instance, Container container) throws Exception {
            if (this == jobProps.name) {
                container.putValue(instance.name);
            } else if (this == jobProps.password) {
                container.putValue(instance.password);
            } else if (this == jobProps.dateOfBirth) {
                container.putValue(instance.dateOfBirth);
            } else if (this == jobProps.numberOfKids) {
                container.putValue(instance.numberOfKids);
            }
        }
    }
    // ^^^^^^^^^^ generated by lombok ^^^^^^^^^^

    // example fields
    private String name;
    private String password;
    private java.util.Date dateOfBirth;
    private Integer numberOfKids;

    // example usage of getValue
    public String toString() {
        StringBuilder result = new StringBuilder();
        for (jobProps value : jobProps.values()) {
            if (value != jobProps.password) {
                result.append(value.name() + "=" + value.getValue(this));
                result.append(",");
            }
        }        
        return result.toString();
    }

    // example usage of putValue
    void serializeToDB(java.sql.CallableStatement callableStatement) throws Exception {
        for (jobProps value : jobProps.values()) {
            value.putValue(this, new Container(){

                @Override
                public void putValue(String string) throws Exception {
                    callableStatement.setString(value.name(), string);
                }

                @Override
                public void putValue(java.util.Date date) throws Exception {
                    callableStatement.setDate(value.name(), new java.sql.Date(date.getTime()));
                }

                @Override
                public void putValue(Integer integer) throws Exception {
                    callableStatement.setInt(value.name(), integer);
                }
            });
        }        
    }
}
rzwitserloot commented 1 year ago

Not even in the same ballpark as something I'd find acceptable. I'm not sure how I can explain why that is so very much not at all what we are looking for though :(

dstango commented 1 year ago

@rzwitserloot:

But that sucks! The only way ToStringImpl can do anything meaningful here is to go on a reflection spree, and every version of OpenJDK makes that more and more complicated. Surely that cannot be the right answer. Instead of 2 cents, work something specific out. I'm going to veto this entire debate as just an utter waste of time until someone comes up with actual code examples. So far I'm mostly listening to a lot of 'I really really want this thing' (notably including having to wipe out a whole boatload of pointless '+1' comments) with virtually zero '... and this is how we can assuage some concerns'. Oh I get it, you really really want this thing. So do I. I also want world peace, and a pony. It needs to be somewhat practical and attainable or it's just frustrating / time wasting.

I've been pondering about a solution without reflection -- and here you go with a first approximation for a suggestion (I'm picking up your trivial example and expand on that to explain):

> cat lombok.config
lombok.toString.customProvider = com.dstango.util.ToStringImpl.trivialToString

> java -jar lombok.jar delombok TrivialExample.java

class TrivialExample {
  private final int a;
  private final String b;

  @Override public String toString() {
    LinkedHashMap<String, Object> propertyMap = new LinkedHashMap<>();
    propertyMap.put("a", a);
    propertyMap.put("b", b);
    return com.dstango.util.ToStringImpl.toString.trivialToString(this.getClass, propertyMap);
  }

A sample implementation of com.dstango.util.ToStringImpl, that would produce something similar to the regular toString()-result of lombok could then look like this (not taking proper care of Arrays.deepToString here, and having curly braces instead of brackets):

public class ToStringImpl {
    public static String trivialToString(Class<?> clazz, LinkedHashMap propertyMap) {
        return clazz.getSimpleName() + propertyMap.toString();
    }
}

As demonstrated here the provider would take two arguments:

Some things to consider:

For the Implementation I'd guess the major difference to lombok's regular toString()-implementation is to create a LinkedHashMap and fill it with key-value-pairs, instead of concatenating these key-value-pairs to a single string.

I think such an approach would offer much flexibility, and keep most of @ToString's existing contract regarding various options, and wouldn't rely on reflection.

So what do you think about something like that?

rzwitserloot commented 1 year ago

@dstango Ah, actual code as part of a proposal. That's great! Thank you.

As a feature request this is vetoed. The problem is something I already mentioned earlier in this issue: The only obvious implementations possible for such a thing (given that it will have to accept a Map<String, Object> is to go on a reflection spree. Or possibly on a a pattern matching spree.

Even if that is acceptable (it isn't), all implementations I can think of will just casually .toString() all the things except those specific types that this 'explicit toString implementation' actually cares about. Which sounds simple and obvious but is incorrect in that you now lose lombok's smarts about e.g. what to do with arrays.

"Slap all fields together into some flavour of java.util.Map and pass it to a single static do-it-all method" is a dead end, I think.