search

square / wire

gRPC and protocol buffers for Android, Kotlin, Swift and Java.
https://square.github.io/wire/
Apache License 2.0
4.23k stars 571 forks source link

Generate static factory method for generated builders in Java #3073

Open rmschots opened 4 weeks ago

rmschots commented 4 weeks ago

When mapping to the generated Java classes for the messages with MapStruct, it is complaining about ambiguous constructors. It would be able to use the builder if there is a static factory method for it. I know that this is not a issue with Wire, but rather with mapstruct.. ..but it's not really a bad practice to have this design pattern in the generated code, so I'd request it to be added (maybe as an option?) to generate something like this for each message:

public static final Builder builder() {
  return new Builder();
}

This issue prevents me from using Wire in my project.

oldergod commented 3 weeks ago

Is there something we can improve on MapStruct? I don't really mind adding the Builder#builder method (or maybe rename it newBuilder() in any case, and it'd be nice to see in the PR that you can actually achieve what this PR is trying to fix.

rmschots commented 3 weeks ago

@oldergod Mapstruct constructor usage is documented here: https://mapstruct.org/documentation/stable/reference/html/#mapping-with-constructors MapStruct also supports mapping of immutable types via builders: https://mapstruct.org/documentation/stable/reference/html/#mapping-with-builders

So, currently mapstruct does following steps when trying to generate a mapping:

  1. no builder found
  2. There's no constructor with a custom '@Default' annotation
  3. There's no parameterless constructor
  4. There's multiple eligible constructors (one with, and one without ByteString unknownFields)

So, first it tries to find a public static builder creation method, which would be the generated method I'm introducing in my PR. As public Builder newBuilder() is already generated by Wire (which creates a new builder using the message data), I wouldn't be able to use the public static final Builder newBuilder() signature, as newBuilder() is already defined in a non-static way. Hence, I'm using builder() in my PR. Feel free to suggest alternative solutions.

I tested the solution to work correctly in another project already by manually editing the generated messages. But it's unlikely a good idea to write a test for mapstruct integration this project, as it's not really of importance to Wire.

oldergod commented 1 week ago

Got it, thank you. I didn't know what MapStruct is. I don't think it's such a good idea for Wire to do that, specially since the Builder has a public constructor. Could you reach out to MapStruct and ask for them a hint? Maybe there's a way for you to point out which constructor to use or they could add a new place where they'd be searching for builders as an inner class called Builder with a unique constructor?