Make @Singular create mutable collections

[Master-Killer]

For mutable collections (the java.util ones supported by Singular), let @Singular create mutable collections instead of wrapping them in unmodifiable* or using empty* or singleton*. It could be an option disabled by default, but accessible like this @Singluar(mutable = true). It feels to me that being forced to go from mutable to immutable just by adding a convenience @Singular to a field is counterintuitive.

[Master-Killer]

Or we can have the current behavior of creating immutable collections applied when the class is also marked as @Value, but have mutable collections otherwise a default.

[schmidti159]

Another take at this: Currently it is not possible to mix @Singular with @Builder.Default which (I assume) is based on the fact that all Collections generated with @Singular are immutable, in which case an initialization with another Collection is not possible.

However the combination of @Singular with @Builder.Default could be used to allow arbitrary collection-types to be used with @Singular allowing even initializing the collection with default values:

Lombok

@Builder
class ExampleWithSingularAndDefault<T> {
  @Singular @Default private List<T> children = new ArrayList<T>();
  @Singular @Default private List<T> childrenWithDefaults = 
          ExampleWithSingularAndDefault.generateDefaultChildren();
  @Singular @Default private Set<T> childrenAsTreeSet = new TreeSet<T>();
  @Singular private List<T> childrenWithImmutableList;
}

Vanilla

class ExampleWithSingularAndDefault<T> {
  private List<T> children = new ArrayList<T>();
  private List<T> childrenWithDefaults = 
        ExampleWithSingularAndDefault.generateDefaultChildren();
  private Set<T> childrenAsTreeSet = new TreeSet<T>();
  private List<T> childrenWithImmutableList;

  /*
   * Builder code generated without any change
         * (in the end the AllArgsConstructor is called with immutable collections)
   */

  public BuilderSingularLists(
      List<T> children, List<T> childrenWithDefaults, 
      Set<T> childrenAsTreeSet, List<T> childrenWithImmutableList) {
    if(this.children == null) {
      // Default value is not available → act like the available implementation
      this.children = children;
    } else {
      // Add the values from the builder to the generated defaults
      this.children.addAll(children);
    }
    // handle the other collections identically
    if(this.childrenWithDefaults == null) {
      this.childrenWithDefaults = childrenWithDefaults;
    } else {
      this.childrenWithDefaults.addAll(childrenWithDefaults);
    }
    if(this.childrenAsTreeSet == null) {
      this.childrenAsTreeSet = childrenAsTreeSet;
    } else {
      this.childrenAsTreeSet.addAll(childrenAsTreeSet);
    }
    // handle like before if @Default is not defined
    this.childrenWithImmutableList = childrenWithImmutableList;
  }
}

As I am quite new to this project: Has a solution like this already been discussed? Have I missed some major pitfalls with @Singular and @Default?

[zachofalltrades]

Or we can have the current behavior of creating immutable collections applied when the class is also marked as @Value, but have mutable collections otherwise a default.

I REALLY like the first suggestion of adding an optional (mutable = true) parameter to the Singular annotation. But changing default behavior in a widely used library is not something I would ever encourage.

[bappajubcse05]

Any update on this please! I believe this is crucial feature that Lombok team should consider.

[AdrianRomanski]

Thank You guys for letting me know that im not the only one that wish to have this kind of feature with Lombok. Creating mutable collection at the moment results in bad looking code. I spend hours on trying to figure it out how to do this better with Lombok and found this thread.

[peterlgh7]

We are in dire need of this feature right now. I understand the advantages of immutable collections in certain scenarios, but the practical advantages of mutable collections are often more important in real world projects.

[rzwitserloot]

There are a ton of complications if these things become mutable. We'd have to explain all this, add a sizable maintenance burden, which makes this feature fall under overgrowth of annotation parameters.

Sorry folks. We're not going to do this, and would not accept a PR.

[FrancoisDevemy5]

Thank you rzwitserloot for your reply. I understand that I don't get all the implications of adding this feature. Good luck continuing work on this beautiful project.

Cheers

[anenviousguest]

For those who comes across this issue, it seems to be possible to work around it by overriding the constructor which otherwise would have been generated by Lombok - the all-arg one - and setting the list field there to be a mutable collection.

@Data
@Builder
class Container {

    @Singular
    private List<Element> elements;

    private Container(List<Element> elements) {
        this.elements = new ArrayList(elements);
    }
}

[Laures]

I found this because I'm using singular on JPA Entities and Hibernate tries to merge entity changes with add(..).

The workaround above requires me to overwrite the entire allArgsConstructor, so instead i fell back on Lombok.Default and added the Singular-Convenience Methods myself:

@Builder(toBuilder = true)
public class Example {

    @Builder.Default
    private Set<Other> preferences = new HashSet<>();

    public static class ExampleBuilder {

        public ExampleBuilder preference(Other other) {
            if (!preferences$set) {
                preferences$value = Example.$default$preferences();
                preferences$set = true;
            }
            preferences$value.add(other);

            return this;
        }

        public ExampleBuilder clearPreferences() {
            preferences$value = Example.$default$preferences();
            preferences$value.clear();
            preferences$set = true;

            return this;
        }

        // overwrite the generated method to mimic singular-behaviour
        public ExampleBuilder preferences(Set<Other> others) {
            if (!preferences$set) {
                preferences$value = Example.$default$preferences();
                preferences$set = true;
            }
            preferences$value.addAll(others);

            return this;
        }
    }
}

Depending on your IDE you may see a warning when accessing $default$preferences() (your properties default value) but the compiler will accept it since the method does exist (intelliJ did not like the line). If that bothers you, replace the call with with the actual default value.

[rzwitserloot]

@Master-Killer wrote:

Or we can have the current behavior of creating immutable collections applied when the class is also marked as @Value, but have mutable collections otherwise a default.

That would not be backwards compatible. Folks update their lombok and their collections silently become mutable. Possibly introducing a security leak and doing untold amounts of damage. No can do.

[rzwitserloot]

@schmidti159 wrote:

However the combination of @Singular with @Builder.Default could be used to allow arbitrary collection-types to be used with @Singular allowing even initializing the collection with default values

This is not a good idea. Basic semantics: Why would setting the default all of a sudden mean that lombok will call .addAll and pass the immutable list it made, instead of just assigning the immutable list it made? This plan will technically result in what you want, but it makes as much sense as saying that you get this behaviour using the @Banana annotation. Worse actually, banana at least is a non-sequitur, the text @Default would make people think it has something to do with defaults. Surely you see why one would think that. Discoverability and readability of code is very important, and this proposal fails the mark on that so badly, it's not going to happen.

[rzwitserloot]

@Laures wrote:

so instead i fell back on Lombok.Default and added the Singular-Convenience Methods myself

We reserve the right to change anything related to $ fields in minor point releases. In other words, don't do this.

The most direct route to a fix seems to be to introduce a feature whereby you can have lombok inject an all-args constructor as normal, but allow you to write a 'mutator' method; its argument(s) take the place of where lombok would have ordinarily put the parameter, and its return value takes the place. Then you can add a mutator to the all-args constructor so that you can wrap it in your own whatever-list-you-please, without the burden of having to write out a parameter for all the fields, write all the this.x = x; assignments, and maintain all that when you add/remove/change fields.

[Laures]

@rzwitserloot so instead of my workaround above i should do something like this (?):

@Builder(toBuilder = true)
public class Example {

    private Set<Other> preferences;

    public static class ExampleBuilder {

        private Set<Other> preferences = new HashSet<>();

        public ExampleBuilder preference(Other other) {
            preferences.add(other);

            return this;
        }

        public ExampleBuilder clearPreferences() {
            preferences.clear();

            return this;
        }

        // overwrite the generated method to mimic singular-behaviour
        public ExampleBuilder preferences(Set<Other> others) {
            preferences.addAll(others);

            return this;
        }
    }
}

[rzwitserloot]

Sort of, except now you don't get the benefits of all the smart stuff lombok does under the hood for singular, and this breaks some rules (for example, our builder() guarantees that you can call build() more than once without all hell breaking loose. With your example, all hell would break loose.

I get the need for something like this, but there are a billion-and-one requirements and most of them get into hairy questions that don't come up except in exotic scenarios. Which means it seems like an easy feature, but isn't.

The point stands: This isn't happening anytime soon; we don't run into this often but the proposals so far seem to be written by folks who don't seem to be on the same page as we are as far as problems are concerned. That leaves us at an impasse, and that's why the issue remains closed.

A more thorough proposal for the mutator idea, plus some documentation updates for @Singular to explain how you can use a mutator to create your own mutable list variant, seems like the easy answer.

[mando23]

I found this also looking for a solution on mutable collections...

I have another workaround, kind of ugly if you have a lot of collections but helpful in the end, and you don't really have to override anything:

@Builder(toBuilder = true)
public class Example {

    @Builder.ObtainVia(method = "copyPhones")
    private List<Phone> phones;

    private List<Phone> copyPhones()
  {
    return new ArrayList<Phone>(phones != null ? phones : new ArrayList<Phone>());
  }
}

The only downside I find is that you would need to create a "copy" method for each collection.

[DarkFisk]

Hi guys, I've just run into an exception since .build() created an object with a singleton collection inside. Who decided that it is normal behavior? what if it is @Entity with a collection of some child entities, and now I cannot add more childs after building.