projectlombok / lombok

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

[FEATURE] implements in Builder/SuperBuilder #2224

Open maroziza opened 5 years ago

maroziza commented 5 years ago

It would be great to add implements argument to [Super]Builder annotation to implement general builder interface. With this feature

@Builder(implements=custom.Builder.class) class Bean {
   String a, b, c;
{ BeanBuilder.class.isAssignableFrom(custom.Builder.class) => **true**
}

without this feature we can't implement SuperBuilder at all, and need to use boilerplate for regular Builder:

@Builder(implements=custom.Builder.class) class Bean {
public static abstract class BeanBuilder implements custom.Builder<Bean> {}
   String a, b, c;
{ BeanBuilder.class.isAssignableFrom(custom.Builder.class) => **true**
}

as an option default interface LombokBuilder can be provided

janrieke commented 5 years ago

What's the use cases here? The interface to be implemented by the builder can only contain the build() method (and only if it's parameterized, and Lombok cannot know that). If it would contain other methods, you'd have to customize the builder anyway, and then there's no use in that parameter.

maroziza commented 5 years ago

I have an object storage and this builder is used in generics. so my framework only needs one method - build();

public class ObjectStorage<
        T extends Mutable<K>,
        K extends Builder<T>> {

where Mutable<> describes toBuilder method and Builder only build() method. Storage manages saving and loading from/to Json and provide interface of changing beans through new builder updating json. I want to call build() inside service to make necessary decoration and checks so there is no object creation will occur in user's code. so we can write code with autocompletion like

Person stored = personStorage.create((k) -> {
            k.name("Bob").surname("Jenkins");
        }); 

also really can implement not only build in builder because builder setter can implement Interface methods; like in

interface NamedBuilder<T> {
Object name(String name); <-- this will be implemented by Builder
T build(); <-- and this also
}

another usecase is using builder as a default object provider. like java.util.func.Supplier to create new instances

janrieke commented 5 years ago

I think this is a good example of the customization possibilities: It's not common enough to become an annotation parameter, and it is really simple to do just by adding the builder header and an implements.

What I don't understand is why it should not be possible with @SuperBuilder.

Disclaimer: I'm not a Lombok team member, just a contributer.

PS: If you only need the build() and toBuilder() method, why don't you use method references?

maroziza commented 5 years ago

i already made this by using methods reference, but i got back to boilerplate code because i need to write this monster in each bean class.

     MutabilityManager<NeImage, NeImage.NeImageBuilder> imagePatcher = new MutabilityManager<>(
            NeImage::toBuilder,
            NeImage.NeImageBuilder::build,
            NeImage::builder
    );

so do i with builder implements. also Builder does not have common on* args. so we can't add Jackson annotation to builder without creating class, it's another problem, but it's very common in practice. Though we can create some global annotation like @Implements() just to implement different staff with lombok, and then do something like @Builder(onClass={@JsonPOJOBuilder, @Implements(Builder.class, {Bean.class})}). Oh i guess i take wrong language to do such things...

rspilker commented 4 years ago

@janrieke, @maroziza Do I understand correctly that I can close this issue, or is there still significant value that we can add in the foreseeable future?

janrieke commented 4 years ago

I think @maroziza and me disagree on whether there is significant value.

I regard this use case as rather uncommon, and a case which can be easily mitigated by customizing the builder classes. I agree that this leads to a little boilerplate code in each class, but Lombok cannot support every use case.

savinovalex commented 4 years ago

Sorry for spamming in this non-related thread, but is there some estimations about releasing out SuperBuilder from the experimental package?

janrieke commented 4 years ago

@savinovalex: Please use the search function, it has been discussed recently.

briantopping commented 4 years ago

@janrieke As a very happy Lombok user, I don't want to rock the boat unnecessarily, but I have to agree with @maroziza in this case. My use case is converting a project built with compiled protobufs, which have extensive builder typing and and builder materialization options. One of the methods I was depending on for a class mapper that doesn't use extensive reflection was com.google.protobuf.Message.Builder#setField, which takes a string and can set a field based on the field name alone. In my use case, this is essential when getting a row back from a database. I can iterate over the returned row metadata and call setField with the field name and the value returned in the row.

Without a marker interface on the builder, the client code explodes with reflective checks on the various objects and creates a really painful developer experience. The solution I came here looking for was that they generated types implement (at least) a marker interface. But I think this feature request does a much better job for those that need this functionality, without interrupting the experience of the de-lomboked code for those that do not.

janrieke commented 4 years ago

I do believe you that there is a usecase. I just don't think that it is common enough to justify the effort the Lombok team has to make to get this working. Especially as there is a workaround (inserting the builder header) which is rather easy to do.

I'm not a Lombok team member, but I contributed @SuperBuilder and do some maintenance for it. It is up to the maintainers to decide here, but I vote against it (and, to be honest, most likely would do other things before maintaining such a feature in @SuperBuilder).

briantopping commented 4 years ago

I do believe you that there is a usecase. I just don't think that it is common enough to justify the effort the Lombok team has to make to get this working.

Maybe you are inferring "diminishing returns", which is arguable. But I think you are completely overestimating the effort required for this simple change versus the power it affords the community through its availability. And I may fully misunderstand how difficult this is since I've never looked at the Lombok source. Presumably it's using ASM for direct bytecode generation (EDIT: Eclipse JDT), but even that is not rocket science. (I caveat this with the fact I do not have a PR, but it doesn't diminish this request or imply that it ought to be closed.)

Especially as there is a workaround (inserting the builder header) which is rather easy to do.

I honestly did not see this reference or understand what that means after I look through your comments. What is a "builder header"? Neither the @SuperBuilder source nor the documentation includes the word "header". You might be correct, but we could use some additional information to sniff test it against our respective use cases.

In any event, I only noticed your comment from another laptop that doesn't get Github notifications because I came back to include that the implements ought to also include extends. As I thought about my use case somewhat, extending a generic base class provided by a community mapper library would be a very powerful abstraction for the facilities Lombok provides. Enabling such orthogonality is a hallmark of great libraries, and while I'm sure the Lombok team is very busy, the world marches forward. There's always room to make a great library better and I'm sure nobody would advocate that the library is finished evolving because people don't have endless time to work on it.

janrieke commented 4 years ago

As with @Builder, you can customize @SuperBuilder by simply putting the builder class header into the annotated class. You are correct that this is only briefly described in the @SuperBuilder docs. However, the @Builder docs cover that topic with an example.

rzwitserloot commented 4 years ago

@briantopping The problem with an overture to the 'power it affords the community' is that lombok has something around a million users (due to maven central, hard to have exact numbers), and it is obviously not feasible to reach them all for some sort of vote to get an accurate read on how useful this is.

As a consequence we are primarily motivated with arguments along the lines of 'this common idiom (reference to 'proof' for example with a github search) would be easier if lombok feature X is changed like so', or 'this commonly used library (easy enough to show a library is commonly used; show your work here) requires that things look like X, so why not add a lombok feature?'. We are not motivated by the say-so of an individual, or even the say-so of a 100 upvotes. I wish I could introduce a system whereby we can submit a feature proposal to the lombok audience and see what people think, but every time I bring up popup boxes in IDEs, people immediately start shipping for pitchforks. It's just not an option.

Thus, we have only one way to do this: Taking comments and our admittedly limited and no doubt biased view of what the greater java community wants, mixed in with what motivates us, we make decisions, and try our best.

This one isn't making the cut. Yet.

The best way to change our minds is to make specific references to libraries or idioms that would be better; this thread already has a few of these. I'm particularly interested in the protobuf angle. Some more commentary on how this interface stuff would help there would be useful. Protobuf is commonly used; that needs no further debate :) – but I haven't personally used it so I'm having a hard time following how it helps. Can you perhaps link to some github gists to show what you're trying to do?

NB: We work on a 'default answer is no' model; the problem with saying yes to anything that seems easy enough to write, is that it leads to a builder annotation with 75 parameters, and whilst each individual parameter is easy enough to explain and test, having a feature page to explain 75 parameters is not good for lombok's learning curve, and trying to test the combinatorial explosion of 75 parameters takes a few thousand years, so it also hurts the stability of lombok: Hence, the burden of proof for a feature request is high, and will remain so.

briantopping commented 4 years ago

@rzwitserloot Thanks for the details here. As I explained some of this to the client this morning in a meeting prior to reading this just now, I said a lot of the same things. There are far too many critical uses of Lombok that would be endangered over time by weak curation of features. I don't think there's anything wrong with the path we're on, and it's up to external entities with the domain expertise to prove out the use case. Nobody reads minds! ☺️

In general, the use case here is the polymorphic dynamic access to classes decorated with @Builder, including to the builder classes themselves, without using reflection. It would be better to illustrate this as a complete project and I'll try to put that together, but for now, I'll have to suffice with a narrative.

Start by considering an object-relational database mapper (such as a JPA implementation) that leveraged @Builder. Typical JPA implementations scan for @Entity annotations and build a cache of accessors via reflection (along with other optimizations that likely include building synthetic interfaces for the accessors using ASM and casting objects to them). These accessors are suitable for both reading from and writing to the underlying mapped objects. The accessors themselves are agnostic to the type of object being mapped, they have a consistent interface for fields of all objects. Iteratively processing a results row is simply an instantiation of the object and an iteration to call the accessor for each column with the column data from the results.

Could @Builder make this a simpler process? Probably not as it stands today as it does not have any sort of abstracted setField(FieldDescriptor, Object) method – we would still be reflectively discovering the builder method for each field and invoking on the Method object we got back. No real gain.

Even if we had such a setField() method that could set a field based on some dynamic key (whether it is a FieldDescriptor, String, etc), we still don't have polymorphic access (from an interface implementation) to this dynamic setter across objects decorated with @Builder. Without that, one is still relegated to reflectively discovering the setField() method baked into the implementation (and discoverable via delombok), then calling it as an invocation, which is orders of magnitude slower than a legal cast to an interface that includes the setField() signature and calling the method via that cast. Reflective invocations add up quite considerably when called iteratively in this manner (such as for thousands of rows with dozens of fields being mapped per row).

In the client use case, they are using the R2DBC database connector and manually mapping rows to object fields with static code. This is static code so it's relatively fast, but very fragile as the schema of the database or objects change. I built them a primordial mapper in 50 lines of code that gets around this using the facilities provided by the Protobuf compiler, which also implements it's own very robust version of Builder objects. A major difference is the generated objects (both the DTO and it's Builder) is they conform to interfaces that exist in the Java Protobuf library (see above link). There's no reflection required to access these methods, only a cast in rare situations.

If I were reading this, I'd still be asking myself "okay, where's the ultra compelling use case here?", and that's fair. OR mappers aren't standard engineering fare, we mostly take them off-the-shelf and R2DBC just isn't quite there yet. Should Lombok be concerned? My rhetorical answer would be "well, what are the use cases for Protobuf?" Of course there are innumerable use cases, of them is gRPC, which most will agree is a very elegant insofar its leverage of Protobuf. Will someone write a RPC platform using Lombok? Probably not if they don't have polymorphic abstractions to the objects being sent over the wire. Does it matter? If you ask me, I think Lombok is great, so of course I want to see compliments to it!

There's usually some "tipping point" where paradigms are powerful enough for someone to have an epiphany and write something that catches fire. The client POC I referenced was already written with Protobuf and as I thought about it, a mapper was an obvious weave between R2DBC and the classes generated by the Java Protobuf compiler. The client is using Lombok and not Protobuf though, my landing on this issue was a result of trying to accomplish that port. And what I had to tell them this morning was it isn't possible without so much reflection it would be a pretty unusable solution.

What can I do to help here?

rzwitserloot commented 4 years ago

That does sound cool, but I see some issues with it; lombok currently ships essentially without any runtime dependencies. That would change if we introduce, either by default for all @Builder-generated builders, or as a new feature, the notion of a 'dynamic builder', where we implement this interface:

public interface DynamicBuilder<T> {
    DynamicBuilder<T> setProperty(String pName, Object v);
    T build();
    // possibly: List<String> getPropertyNames();
    // possibly: Map<String, Class<?>> getPropertyTypes();
    // possibly: List<String> getMandatoryPropertyNames();
    // this list of fun methods we could have is probably rather long :P
}

... and, of course, ship that interface. And therein lies the rub. Currently lombok doesn't ship any runtime deps; we'd have to create it just for this. I'm game, but it raises the bar for using it. In practice, IF people are already doing something like this, presumably they are already relying on an existing mapper system, such as protobuf, and they have an interface there that they'd want us to implement.

Except each such interface is no doubt a little different.

There's a way out: If protobuf has such a 'dynamicbuilder'-esque interface with setField or setProperty or whatnot, we could make a lombok annotation which means: Make me an impl of that one, please! – Lombok does already ship with the concept of 'a feature that is targeted at some not-in-core-java library': That's what @lombok.extern.slf4j.Slf4j is all about, for example. We could have a @lombok.extern.protobuf.ProtobufDynamicBuilder annotation.

The slam dunk use cases for lombok are where we encode an existing and already widely applied boilerplate pattern as a lombok feature (example: builders, @Value), but sometimes there's an elegant enough paradigm which we could turn into a lombok feature, but where for whatever reason the java community at large isn't doing it yet. Chicken and egg problem: What if the community isn't employing it because it's too much effort? Effort which lombok can eliminate for you? Should lombok introduce new idioms?

We're trying this experiment right now with @WithBy, which is targeted at the next release, so we're certainly intrigued by it. But if we're going to experiment, having to worry about shipping an interface adds complications I'd rather not have.

This would all be a lot simpler if 'dynamic builder' as a concept was already employed, with existing interfaces (even if not in java.*) we can use.

As a side-benefit, the learning curve and support argument for @lombok.extern.protobuf.ProtobufDynamicBuilder are way, way better: All the extern stuff can be documented on a package level: "Stuff in this package is useful only if you use the protobuf library. If you don't, skip this entire section, and if you do, please note the docs don't make sense unless you know what protobuf does, so go figure that out first" – that's pithy and to the point, and by making it a separate feature, the combinatorial explosion nature can be managed better.

So, I guess... the next step'd be to, uh, launch a new (or preferably, find an existing) library that is somewhat widely used with a DynamicBuilder-esque interface :)

briantopping commented 4 years ago

Great stuff, thanks for the consideration! I did forget about the lack of runtime dependencies. This is a huge oversight on my part and I think it's a "core elegance" of Lombok – it kind of "just disappears"...

Which is ironically a great plug for implements / extends... which leaves it up to the developer who uses it to decide how to meet the SPI contract created by Lombok. Delombok in the IDE is a superpower, and I think it's a very reasonable developer experience to depend on it in a case like this. I don't believe the SPI is a problem, if I want this functionality, I'm happy to comply.

It's also kind of a "gateway drug" to the @lombox.extern, which I hadn't heard of at all. An ecosystem of Lombok plugins would be really cool, but in a world were some developers have to convene a UN council to get a new project added or change builds, a lighter weight entry to the fore is likely to be a lifesaver. I think implements / extends is such a gateway.

What if Lombok @Builder generated interfaces internally by default, and this FR was really just a way to say "use my class/interface instead of generating them"? I would imagine how three segments of users might react to this:

I think there is a fourth group, which is the one that Delomboks the code and then says "wow, this code is complex, muh rights are being violated and I'm going to complain anonymously on Reddit!" Though I think this is a fleetingly small group. Most people who stumble upon Delombok and don't understand what just happened quickly select "Undo" or git reset --hard. Those that Delombok with intent are power users, and they need the power. implements / extends provides that power, with a middle step for those that just need some way to abstractly access a builder if such an interface were automatically generated. Community plugins make a fourth level of extension power where anything is possible.

In the local use case I have here of generating a "best practice POC" for the client, I'm not sure how I feel about generating a Maven / Gradle subproject just for the Lombok extension if that were my only way out. Imagining that the extension would have code that looked like this, I think the client would be like "#wtf we can't maintain this" and my engagement manager would be like "#wtf why did you spend so much time on this?" Because most folks are looking for simple patterns that are powerful, not subsystems that are powerful. If that makes sense...

Lastly, to your point about javax.*, I think java.util.function.* can describe every signature in terms of these primitives. Such expressions are awkward and require higher level mojo to understand, but they are super flexible and I'm surprised of late how well IntelliJ's simplify intentions use them. Almost 100% probability such signatures would not directly match something provided by a target client such as Protobuf, but the ability to craft a proxy that does map them would be simple.

Maaartinus commented 4 years ago

@briantopping

One of the methods I was depending on for a class mapper that doesn't use extensive reflection was com.google.protobuf.Message.Builder#setField, which takes a string and can set a field based on the field name alone. In my use case, this is essential when getting a row back from a database. I can iterate over the returned row metadata and call setField with the field name and the value returned in the row.

I may be completely mistaken concerning your goals, but it looks like you wanted the most efficient conversion from database rows to entities and the most efficient way includes no setField. There are two reasons why reflections is slow: One of them are the access checks (which were optimized a lot) and the other is the dynamic lookup. This dynamic lookup has to be done by protobuf's setField as well.

The most efficient way includes code generation. AFAIK what Hibernate does is to generate an SQL returning all columns and to generate a method populating your fields like

entity.setId(resultSet.getInt(1));
entity.setName(resultSet.getObject(2));
...

Hibernate uses reflection in order to get the field list (both for the SQL and for populating the entity) and it generates the method using bytecode generation (gclib, bytebuddy or whatever). This is just the initialization; everything else uses the generated code which is as fast as manually written one. Instead of reflection + bytecode generation, you could use annotation processing (assuming all entities are available at compile time).


That all said, I'm not opposed to this feature. I may also be completely misunderstanding your needs.

briantopping commented 4 years ago

Thanks @Maaartinus, I should have been clearer about when I was optimizing for efficiency and to what degree. There are cache-free implementations, implementations that cache invocation proxies, and bytecode generation. A setField implementation could use the first two, but even then, what about developer efficiency? There's a lot of details that could have avoided inference with better editing on my part. I agree with you that any of the bytecode generators you reference would be an optimal choice from the common finished code perspective.

In the case of the mapper I was working on, I was focused on as few lines of code as possible so the core pattern would stand out. The Protobuf compiler I was using didn't actually use dynamic access for setField with a string key, but passed a FieldDescriptor generated from the string key, which could have been cached if it was not. It's similar to a method invocation proxy but it's really just an accessor to protected data, so it's fast.

In the end, these were just use case examples for how Lombok could benefit from tunable code generation. Apologies again for the misunderstanding.

janrieke commented 4 years ago

I also see the benefits of having such an dynamic builder set method. It would be really great if there was something like a common interface for this. Imagine lombok would generate builders that implement it; then frameworks like Jackson could simply make an isAssignableFrom check and instantiate POJOs without any reflection or similar stuff at near-maximum performance (still one additional function call, a switch-case and a cast needed compared to a regular set method call). lombok has all information required to generate a setField method without any reflective code. A lombok.extern. annotation could trigger that generation.

I know the Spring Data folks like lombok, they specifically recommend it in their docs: "Use Lombok to avoid boilerplate code". If we could get them, the Jackson people and maybe someone from JPA/Hibernate onboard, this could be the start of something. Is anyone in contact with someone in that projects? Am I thinking too large?

Maaartinus commented 4 years ago

Apologies for sounding negative... I'd really love to see Lombok nicely integrating with other tools and vice versa. I'm just unsure whether setField is really worth the effort.

@janrieke

additional function call, a switch-case and a cast needed compared to a regular set method call

Unlike when processing SQL result set, there's no fixed field order, so a regular set method can't be used (assuming you get an incoming JSON object and process its fields as they come rather than reassembling somehow, which IMHO is inefficient). So with Lombok, you'd probably get the maximum speed possible.

OTOH the only gain is saving the reflective access. I tried to find something about the reflection overhead, but the SO answers are not only obsolete, but also a crap (no warmup and OSR in the accepted answer). When performance is the goal, then we should have a JMH benchmark.

janrieke commented 4 years ago

DZone has an article on that topic, using JMH and Java 8. It looks quite solid to me. Takeaway is that reflection still has a performance impact of factor 2; if you want it fast, use a) JavaCompiler (but with extremely large bootstrap costs), or b) LambdaMetafactory (33% slower). Maybe the numbers changed for more recent Java versions.

Besides the performance argument, such an interface would make writing frameworks that instantiate POJOs much easier: Just require the POJOs to have a builder conforming to that standard interface, and you do not have to deal with reflection, code generation or similar stuff at all.

PS: @Maaartinus I do not understand your point on "fixed field order". How is the order relevant when only using statements like builder.setField("fieldName1", "value1")?

Maaartinus commented 4 years ago

@janrieke Concerning "fixed field order": With SQL, you make the query SELECT id, name, .... and then call e.setId(rs.getInt(1)); e.setName(rs.getObject(2)); .... because of the fixed field order. This is much faster than any setField, no matter how optimized and this is what Hibernate does (at runtime, using bytecode generation). With JSON, you can't do this because of the field order being arbitrary.

DZone has an article on that topic, using JMH and Java 8. It looks quite solid to me.

I guess, it's fine. What I'm missing there is bytecode generation, which is sort of standard and surely has much lower bootstrap cost than the JavaCompiler. I strongly disagree with the refusal of StaticMethodHandle as IMHO it's the proper way (though without putting all statics into a single class).

Another thing: The benmark compares the performance of Person.getName() against reflection, and we won't have Person.getName(), but a switch statement instead.


Actually, setField is something I always wanted and never had. In the end, I always found some workaround and that's what makes me skeptical. Maybe I should reconsider.

Given the above idea like

public interface DynamicBuilder<T> {
    DynamicBuilder<T> setProperty(String pName, Object v);
    T build();
    // possibly: List<String> getPropertyNames();
    // possibly: Map<String, Class<?>> getPropertyTypes();
    // possibly: List<String> getMandatoryPropertyNames();
    // this list of fun methods we could have is probably rather long :P
}

I wonder, whether builder.asMap().put(pName, v) could be the way to go. We'd use the fluency, but we'd get a common interface and getPropertyNames() (as keySet()) for free. OTOH we'd also get get() which could be a problem as the builder field types differ (see @Singular). And it wouldn't support the metadata like getPropertyTypes() and getMandatoryPropertyNames(), etc.