[BUG]/[FEATURE] Solve toString() infinite recursion

Describe the feature When using Spring Data JPA and define a bidirectional relationship (this tutorial example (scroll to: Bidirectional @OneToMany chapter) is absolutely valid enough ) Such bidirectional relationship producing infinity recustion problem, which in the case of JSON serialisation can be solved by: @JsonManagedReference and @JsonBackReference however toString() still suffers from infinite recursion and produce java.lang.StackOverflowError exception. Cannot evaluate <your_class>.toString()

Would be nice if this can be also solved in the same way as JSON annotations

Describe the target audience All of those who defined Entity bidirectional relationship and have infinity recursion

Why is @ToString.Exclude not sufficient in this case?

Well... Excluding functionality completely is one of the solutions, but having toString() working corectly is better :)

Yeah, but unfortunately that's impossible because it requires resolution: Lombok would have to look into the type of the references, but that information is not available when Lombok runs.

Excluding functionality completely is one of the solutions

Another one is using @ToString.Include on a method, replacing the infinite thing by whatever you want.

... requires resolution ...

For an automatic compile-time solution, yes. It could also be solved at runtime using a ThreadLocal for detecting the recursion (this has already been discussed), but it's probably not worth the effort and runtime-cost. The user could also provide the information using something like @JsonBackReference, but it's unclear what exactly should happen.

@Bryksin Is there something what can't be achieved easily using @ToString.Include and how should the output look like?

I agree that Exclude, and Include on a method, are okay-ish workarounds.

On the other hand, we already have some treatment for other Jackson annotations.

We could by default (possibly have a config key) exclude fields annotated with @JsonBackReference, both from the ToString and from EqualsAndHashCode.

At the very least we could catch StackOverflowError in toString() and abort. If only as a temporary meassure. Example:

Stringbuilder output = new StringBuilder(getClass().getName()).append("[");
 try {
    output.eppend("fieldname1=").append(field1.toString());
    output.eppend("fieldname2=").append(field2.toString());
   return output.toString()
 } catch (StackOverflowError  x) {
   return null; // mo method calls
}

(Not tested)

At the very least we could catch StackOverflowError in toString() and abort.

I'd rather have this error bubble up. I can imagine this causing serious issues in a production application if the stack is just allowed to overflow, and I'm handling thousands of requests per minute. I'd expect this would be caught during development 9/10 times, but it'd be an easy mistake to log something and not notice the value is null.

I've ran into this several times in the exact scenario. What I'd like to see, which I think is possible, is something like this:

@ToString
class User {

  String name;

  @ToString.Exclude(field = "user")
  Permission permission; 
}

@ToString
class Permission {

  String permissionName;

   @ToString.Exclude(field = "permission")
   User user;
}

Maybe even the same functionality on a ToString.Include where I can specify that, in User for instance, I only want the field named permissionName. So ToString.Include(field = "permissionName"). I find that I'm not always interested in all the content of an object. In the above example, if Permission and User were more complex, I may only be interested in name and permissionName.

If it were possible to implement, I'd also suggest considering to add some static analysis, so that recursive ToString references are detected and the developer is notified, as it seems this may be a very serious issue in a production, e.g. if a logger causes a business logic method to fail. Otherwise this will probably always require additional specialized tests for all entities (possibly iterating over their package and using reflection to test all combinations), and every single developer on the team will always need to be taught this special exception and remember to explicitly check for it during all code reviews. And even worse, this strongly contributes towards steep learning curve (as a beginner I've spent majority of the total time of using Lombok actually just looking for workarounds for issues like this, incorrectly assuming that it's obviously compatible with frameworks like Spring).

It also implies this should be explicitly documented as a common issue (or better, as a prerequisite) in the ToString documentation, which currently doesn't contain strings like "bi" or "recur", just barely a reference to arrays containing themselves:

Arrays are printed via Arrays.deepToString, which means that arrays that contain themselves will result in StackOverflowErrors. However, this behaviour is no different from e.g. ArrayList.

The ToString.Exclude solution seems like an overkill at a first glance, because that completely defeats the purpose of this feature if there's always some random field missing. You can never predict which side of the bi-directional relationship will you need to debug.

There's also one specific suggestion I'll make: instead of forcing us to Exclude a field and Include a method that prints the field, would it be feasible to support something like ToString.Of for a field level? Specifically, there would be at least two separate use-cases; using a field name in a POJO reference, and using a field name within a Collection (List). This could provide a complete end-to-end solution documented as a general best practice for frameworks like JPA, for example by always using only "id" fields in recursive Lists. (Maybe a proxy collection wrapper with a modified toString could solve this.) Either way, if this weren't feasible, a dynamic runtime solution as mentioned in comments above could be a better option, directly replacing a recursive reference by something like "this". By making this optional via configuration, a slower alternative would be probably still preferred by many.

If it were possible to implement, I'd also suggest considering to add some static analysis, so that recursive ToString references are detected and the developer is notified, as it seems this may be a very serious issue in a production, e.g. if a logger causes a business logic method to fail.

IMHO it's not lombok's job to make sure you will never shoot yourself in the foot. Just because it's easy to put a @ToString on a class does not mean it should be done without thinking. It's the job of the developers to think. IMHO if you don't know what a lombok annotation exactly does, you should either not use it, or inspect the delomboked code to learn what it does.

Even if the lombok maintainers wanted to add such an analysis, it would be impossible: The kind of information required to do such a static analysis is not available to annotation processors. However, there are tools for such analyses: Linters like FindBugs or Sonar. It should be possible to write a rule for this type of bug, and such a rule would also find similar non-lombok bugs.

It also implies this should be explicitly documented as a common issue (or better, as a prerequisite) in the ToString documentation

Yes, that would help, although I guess not too much. Who reads the docs? ;)

There's also one specific suggestion I'll make: instead of forcing us to Exclude a field and Include a method that prints the field, would it be feasible to support something like ToString.Of for a field level?

I don't exactly understand what that annotation should mean. Do you want it to be put on the back-reference, and it should mean "when converting this field to a string, just use a specific getter method of the referenced value"? So just a shortcut for @ToString.Exclude on the field and @ToString.Include on a method that calls, let's say, value.getId()? Or is there something more?

if you don't know what a lombok annotation exactly does, you should (either) not use it

Who reads the docs? ;)

Aren't you contradicting yourself there a little bit? ;) I don't see how the process of "learning what is the intention of an annotation" is in any way deterministic, and I believe such an important prerequisite should be very visible within the basic description, possibly even in annotation's JavaDocs, and not just in a "Small print" - where it's also missing. And that's even worse when the developer gets used to the simpler @Data annotation. Let's just try to guess - what % of developers who get used to using @Data won't realize (before getting StackOverflow) that it's not feasible to use once they work with JPA entities? :) I bet it's a dangerously high number, and the UX learnability should be prioritized for a project like Lombok that's still rapidly growing its userbase.

Linters like FindBugs or Sonar

I agree, in this case it should be probably reported as a suggestion for them. (Personally I'd prefer if this were also explicitly advised in the Lombok docs - such that users will know which static analyzers fully implement the required specs, and which don't.)

There are also other similar JPA-related issues with @EqualsAndHashCode, e.g. when used with a Set<> described in an article from thoughts-on-java.org. which could be possibly also related to this, as Lombok also doesn't document them. If there is no plan to extend Lombok docs for general issues with well-known frameworks, then it would be nice if it "being incompatible" could be at least briefly mentioned, hopefully also motivating Spring docs to provide an official Lombok guideline (there are various third-party sites/blogs that currently fill this role (like this), which feels very dangerous). I don't have enough experience judging the severity of similar issues, so if you have any insight, please share it :)

something like ToString.Of for a field level? Specifically, there would be at least two separate use-cases; using a field name in a POJO reference, and using a field name within a Collection (List).

So just a shortcut for @ToString.Exclude on the field and @ToString.Include on a method that calls, let's say, value.getId()?

Exactly, it would be most relevant for the sake of simplicity in the case of Collections, so that when you want to keep the code clean (for simple entities), you would be able to print just IDs, thus avoiding the need to either completely remove printing a List or remove a mapping. I think the "POJO field" and "Collection entry field" would solve most basic entity use-cases, and a declarative approach is always safer than an ad-hoc hard-coded print logic in every entity. For example, @ToString.Replace(strategy, value) where strategy is Field, CollectionField, String (default), or Pattern (which could replace all of them if we were okay with a custom syntax). After all, any standard approach enables compiler optimization in the future, along with better issue tracking.

Maybe someone should inject a little bit of reality into this discussion.

Here's the thing. This is an Open Source framework. Reinier and Roel, our intrepid maintainers, invest the time and effort they are investing for basically free, and they are swamped with work. Lombok is not an easy project to build or maintain. Writing documentation is not necessarily their expertise, and this is exactly the kind of thing where talk is cheap but code is gold. An outside person like you could easily jump in to fill this gap without needing much in the way of knowledge of the internals.

In other words: If you want more documentation, consider writing it yourself.

Don't demand that people who are providing a free service go above and beyond what they can do on their own time to satisfy your own pet peeve - it just doesn't work that way.

On Fri, Apr 10, 2020 at 2:27 PM scscgit notifications@github.com wrote:

if you don't know what a lombok annotation exactly does, you should (either) not use it

Who reads the docs? ;)

Aren't you contradicting yourself there a little bit? ;) I don't see how the process of "learning what is the intention of an annotation" is in any way deterministic, and I believe such an important prerequisite should be very visible within the basic description, possibly even in annotation's JavaDocs, and not just in a "Small print" - where it's also missing. And that's even worse when the developer gets used to the simpler @Data annotation. Let's just try to guess - how many % of developers who get used to using @Data will realize (before getting StackOverflow) that it's not feasible to use once they work with JPA entities? :) I bet it's a dangerously high number, and the UX learnability should be prioritized for a project like Lombok that's still rapidly growing its userbase.

Linters like FindBugs or Sonar

I agree, in this case it should be probably reported as a suggestion for them. (Personally I'd prefer if this were also explicitly advised in the Lombok docs - such that users will know which static analyzers fully implement the required specs, and which don't.)

There are also other similar JPA-related issues with @EqualsAndHashCode, e.g. when used with a Set<> described in an article from thoughts-on-java.org https://thoughts-on-java.org/ultimate-guide-to-implementing-equals-and-hashcode-with-hibernate/. which could be possibly also related to this, as Lombok also doesn't document them. If there is no plan to extend Lombok docs for general issues with well-known frameworks, then it would be nice if it "being incompatible" could be at least briefly mentioned, hopefully also motivating Spring docs to provide an official Lombok guideline (there are various third-party sites/blogs that currently fill this role, which feels very dangerous). I don't have enough experience judging the severity of similar issues, so if you have any insight, please share it :)

something like ToString.Of for a field level? Specifically, there would be at least two separate use-cases; using a field name in a POJO reference, and using a field name within a Collection (List).

So just a shortcut for @ToString.Exclude on the field and @ToString.Include on a method that calls, let's say, value.getId()?

Exactly, it would be most relevant for the sake of simplicity in the case of Collections, so that when you want to keep the code clean (for simple entities), you would be able to print just IDs, thus avoiding the need to either completely remove printing a List or remove a mapping. I think the "POJO field" and "Collection entry field" would solve most basic entity use-cases, and a declarative approach is always safer than an ad-hoc hard-coded print logic in every entity. For example, @ToString.Replace(strategy, value) where strategy is Field, CollectionField, String (default), or Pattern (which could replace all of them if we were okay with a custom syntax). After all, any standard approach enables compiler optimization in the future, along with better issue tracking.

— 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/2255#issuecomment-612007823, or unsubscribe https://github.com/notifications/unsubscribe-auth/AABIERIEVWOVUMZO34KIGULRL4GD5ANCNFSM4I7ZEF3A .

-- "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

Of course, this is just a suggestion, and I am interested in the feedback relating to whether other people who use the most popular frameworks have the same issues. When it comes to documentation, I believe that it's not about writing long essays, but just about the strategic decision of where to add a very brief mention of this issue, so I'm doing my best to analyze the available options and their possible impact. I have no problem sending a PR that adds a JavaDoc if you think that is the most important problem here.

However, what I'm also pointing out is a UX flaw of learnability, which I believe to be very important to address from a long-term perspective, and my intention is to point out the root of the problem, including a solution that I see. Obviously, I would prefer for the project maintainers to spend the least amount of effort, making the decisions and just specifying the implementation, and then letting other contributors like me implement their choices. On the other hand, I can't just go around re-writing the docs along with the code at my discretion without having my opinion validated by community. If you are so critical of my feedback, please point out what better course of action should I take.

Just pointing out the elephant in the room here.

Once Reinier gets a chance to chime in I'm sure he will have an opinion on the matter. Just realise he may say no simply based on the time issue.

On Fri, Apr 10, 2020, 18:13 scscgit notifications@github.com wrote:

Of course, this is just a suggestion, and I am interested in the feedback relating to whether other people who use the most popular frameworks have the same issues. When it comes to documentation, I believe that it's not about writing long essays, but just about the strategic decision of where to add a very brief mention of this issue, so I'm doing my best to analyze the available options and their possible impact. I have no problem sending a PR that adds a JavaDoc if you think that is the most important problem here.

However, what I'm also pointing out is a UX flaw of learnability, which I believe to be very important to address from a long-term perspective, and my intention is to point out the root of the problem, including a solution that I see. Obviously, I would prefer for the project maintainers to spend the least amount of effort, making the decisions and just specifying the implementation, and then letting other contributors like me implement their choices. On the other hand, I can't just go around re-writing the docs along with the code at my discretion without having my opinion validated by community. If you are so critical of my feedback, please point out what better course of action should I take.

— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/rzwitserloot/lombok/issues/2255#issuecomment-612101225, or unsubscribe https://github.com/notifications/unsubscribe-auth/AABIERNWKNE6FZMJ37UETTLRL5ARRANCNFSM4I7ZEF3A .

I've been thinking more about this issue, and it certainly is a challenge to introduce a universal solution without making the project depend on new external classes (in order to support delombok). The simplest solutions to translate objects within Collections or Arrays would require either Streams, turning Java 8 into dependency, or some libraries with a similar functionality. In order to prevent additional performance hit, I've come up with one solution, which is pretty straightforward and can be in-lined inside the toString() method, though it's quite verbose, but after all, this is an optional feature and the code won't be maintained by hand.

It allocates three objects, one of which being effectively just a lambda, other two (AbstractCollection along with an Iterator) providing a forward compatibility by re-using the native toString() implementation and just wrapping the object for that single purpose. (We could save one allocation if we implemented Iterator in the same type, which anonymous classes don't support, though we could define a class at the end of the file.) Arrays don't provide any way to wrap around the index accessor (to be called via deepToString()), so they are iterated using an index. Iterable objects use their iterator() instead. This implementation has an obvious downside of depending on a hard-coded AbstractCollection's toString(), so that the documentation needs to be explicit about overriding any custom toString() implementation. (If only Java supported wrapping collection items directly, this could have been so easy.)

// Replacing Array: @ToString.Include(onlyFields = "id") private Employee[] employees;

new AbstractCollection<Object>() {
    @Override
    public Iterator<Object> iterator() {
        return new Iterator<Object>() {
            int position = 0;

            @Override
            public boolean hasNext() {
                return employees.length > position;
            }

            @Override
            public Object next() {
                final Employee next = employees[position++];
                return new Object() {
                    @Override
                    public String toString() {
                        return "Employee(id=" + next.getId() + ")";
                    }
                };
            }
        };
    }

    @Override
    public int size() {
        throw new UnsupportedOperationException("This Iterable isn't a Collection");
    }
}.toString()

// Replacing Iterable: @ToString.Include(onlyFields = "id") private List employees;

new AbstractCollection<Object>() {
    @Override
    public Iterator<Object> iterator() {
        final Iterator<Employee> iterator = employees.iterator();
        return new Iterator<Object>() {
            @Override
            public boolean hasNext() {
                return iterator.hasNext();
            }

            @Override
            public Object next() {
                final Employee next = iterator.next();
                return new Object() {
                    @Override
                    public String toString() {
                        return "Employee(id=" + next.getId() + ")";
                    }
                };
            }
        };
    }

    @Override
    public int size() {
        throw new UnsupportedOperationException("This Iterable isn't a Collection");
    }
}.toString()

// Replacing any other POJO: // Straightforward, just access the public fields

Instead of using a dedicated annotation as suggested previously, I think that @ToString.Include would be less intrusive as there's only one primary use-case. Here is an example documentation for inspiration:

public @interface ToString {
public @interface Include {
/**
 * Fields of the annotated object to be selected exclusively. Accessor can be a field or a getter, the first
 * available will be used. The child element is used for Arrays and Iterable types like Collections instead.
 * Nested collections may be supported in the future. Don't use this to annotate primitive types. The main
 * intended use-case of this annotation is a prevention of stack overflow when iterating bi-directional
 * references - by explicitly selecting only safe fields, e.g. the id columns of entities. Please consider
 * using some third party static analysis tool if you need to detect and prevent any risk of such cycles.
 *
 * @return The names of all of the target object's fields to be included in its {@code toString()} section,
 * referring to child elements in the case of Arrays and Iterable types like Collections.
 */
String[] onlyFields();
}
}

Furthermore, as previously discussed, this shortcut could be replaced by simply adding a @ToString.Exclude on the field and @ToString.Include on a method, which includes any similar iteration algorithm. This may be thus replaced by two rounds of annotation processing instead if it were preferable.

Considering this process shouldn't affect the other class, I think this feature will be limited to selecting public fields unlike toString() generated on a class directly, which can also include private ones. I'll note for a reference that there's also an option of generating some kind of toStringOnlyFields1() on a target class, which would make the alternative possible, also making the code a bit cleaner. There's also the issue of choosing between a field and getter method, but I believe this can be easily solved using reflection during preprocessing. Also, there's a very interesting potential to create a solution even for extremely complex scenarios of nested objects, e.g. traversing even the path of collection-class-collection-field by choosing onlyFields = "employees.company.owners.id". This would make any complex nested print statements very performance efficient by ignoring all of the omitted fields. Of course, this shouldn't be prioritized, I'm just looking for a feedback. All I'm asking for is a possibility to add this feature by a community later on.

Either way, I've been unsuccessful with trying to implement this myself, as I've spent several days trying to make lombok properly compile in various IDEs without relying on ant dist, but I still haven't managed to run it under a debugger, or to extract all core files into another project. I also couldn't accomplish this by running mvn compile on my project via debugger, because the ShadowClassLoader hides .lombok files from the IDE. I'd suggest updating the page for contributors to reflect some use-cases that beginners will have to tackle, as there are countless of unintuitive obstacles. I'm aware that Lombok is trying to prevent overgrowth, but that's one more reason why there could be some initiative in providing the community with efficient guidelines related to extending the project, possibly creating compatible alternatives.... If there are any issues with my example other than the effort needed to write the abstract syntax tree algorithm, then I'm looking forward to receiving a feedback.

The obvious solution to me is to detect 'backlinkage', but without using resolution. For example, if a field is annotated with some JPA 'this is a backref' annotation, we can see that, and skip it.

Categorically denied:

Catching StackOverflowError. This is insane - toString is called in production environments and just wasting the time until SOErr occurs is obviously an unacceptable act, this isn't so much a workaround as a bacteria-infested plaster for the wound. Seems to work, but good gosh is the cure ever so much worse than the wound.
Adding a loop detection scheme to every toString we generate. I can expand on why this is denied if needed, but I'm hoping it's obvious enough why.
Anything involving resolution (having to know information about types that cannot be gleaned from the source file we are in, such as which types of fields some other class has).

Denied, but, we could be talked into allowing it:

An option on @ToString to enable it explicitly.

Reasonable options:

Add try/catch SOEx, but only to expand on the message. We'll just toss that exception right on through, but we will add: Hey, maybe you should consider excluding a field maybe. I don't think it's particularly worthwhile trying to add a whole bunch of code to attempt to analyse which field, and as we can't use deps, we'd have to generate that 'analyse the trace' code every time, so that's not a good idea (@ToString is very common, generating a ton of code there every time is not an option).

projectlombok / lombok

[BUG]/[FEATURE] Solve toString() infinite recursion #2255