Protobuf example #25
Protobuf example #25
Conversation
|
You can use |
|
Thx. I added the NullValueCheckStrategy. Null values now map to the protobuf defaults. I added the testcase to illustrate the behaviour. |
| String methodName = super.getPropertyName(adderMethod); | ||
| Element receiver = adderMethod.getEnclosingElement(); | ||
| if (receiver != null) { | ||
| TypeElement type = (TypeElement) ((DeclaredType) receiver).asElement(); |
There was a problem hiding this comment.
I would rewrite the routine a little bit. Don't think its required go from a element to a DeclaredType to a TypeElement if you already know its a class. So like this:
@Override
public String getElementName(ExecutableElement adderMethod) {
String methodName = getPropertyName( adderMethod );
Element receiver = adderMethod.getEnclosingElement();
if ( receiver != null && receiver.getKind() == ElementKind.CLASS ) {
TypeElement type = (TypeElement) receiver;
TypeMirror superType = type.getSuperclass();
if ( superType != null && PROTOBUF_MESSAGE_V3.equals( superType.toString() ) ) {
methodName += LIST_SUFFIX;
}
}
return methodName;
}(I also would recommend using constants).
There was a problem hiding this comment.
That looks nicer. I was glad to make it work, have never used this part of api before.
mapstruct-protobuf3/usage/pom.xml
Outdated
| <plugin> | ||
| <groupId>org.xolstice.maven.plugins</groupId> | ||
| <artifactId>protobuf-maven-plugin</artifactId> | ||
| <version>0.5.0</version> |
There was a problem hiding this comment.
can you add plugin versions to properties defined at protbuf parent level? That makes them easy to maintain.
| <build> | ||
| <extensions> | ||
| <extension> | ||
| <groupId>kr.motd.maven</groupId> |
There was a problem hiding this comment.
Is this extension needed for protobuf?
There was a problem hiding this comment.
It allows platform detection so that maven can pull the correct binaries for protoc
${os.detected.classifier}
| user.setEmail("test"); | ||
| user.getPermissions().add(Permission.ADMIN); | ||
|
|
||
| UserDTO.Builder dto = UserMapper.INSTANCE.map(user); |
There was a problem hiding this comment.
ah.. the good old round-trip.. 😄 I was thinking.. Would it be good to call the serializing functions in protobuf and deserialize in a fresh object? Just to show its working end-to-end..
There was a problem hiding this comment.
Never have done that before :) But it's good to see that it works.
|
See also #14 |
|
I just pushed the changes. Thx for the feedback. |
| @Override | ||
| public String getElementName(ExecutableElement adderMethod) { | ||
|
|
||
| String methodName = getPropertyName(adderMethod); |
There was a problem hiding this comment.
I didn't notice this before., but you are calling getPropertyName(adderMethod) instead of super.getElementName(adderMethod). It currently works, because in getPropertyName() we always strip the first 3 characters. However, I think that it is more correct if you just call super.getElementName(adderMethod)
| */ | ||
| public class ProtobufAccessorNamingStrategy extends DefaultAccessorNamingStrategy { | ||
|
|
||
| public static final String PROTOBUF_GENERATED_MESSAGE_V3 = "com.google.protobuf.GeneratedMessageV3"; |
There was a problem hiding this comment.
Again something I read on the protbuf website (I have no real experience). It says that GeneratedMessage is an implementation detail and that the builders are implementing Message.Builder or MessageLite.Builder depending on something you pick in the .proto
There was a problem hiding this comment.
I have no clue how to check that with that part of the java api, sorry.
|
@eiswind Thanks a lot for the example. I think that it is a really nice addition. Regarding my #25 (comment). If you think that it is worth it and it is OK for you, I can add that to your PR, or create a new one, that will do that. Additionally you had some questions in one of the issues, about not needing to use Finally, will it be OK for you if I squash all the commits into one before merging, or you would prefer to have them as is merged into the examples? |
|
Feel free to make your changes as you prefer! |
|
Thanks for the PR @eiswind, sorry for taking so long to merge it. |
3 participants
Null values don't work, as this is not supported in the Builder.