Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Mappers in protobuf-support-(core | standard | lite) should **never return null**. #176

Open
zartc opened this issue Mar 18, 2022 · 0 comments · May be fixed by #177
Open

Mappers in protobuf-support-(core | standard | lite) should **never return null**. #176

zartc opened this issue Mar 18, 2022 · 0 comments · May be fixed by #177

Comments

@zartc
Copy link

zartc commented Mar 18, 2022

quoted from protocolbuffers/protobuf#5450

Protobuf Java uses a null safe approach, at no point in protobuf-Java null is ever used.
Unset fields don't return null when you call its getters, they return a "default" value (empty, zero, EPOCH, etc...),
setters will always throw NPE if you try to pass them null.

I think it's a safer practice to always return an object instead of null from a mapper method:

  • map domain to proto => returning an object is clearly the right thing to do: because albeit it is completely acceptable for the fields of the domain object to be null at the application level, but this should not result in NPE when sending those objects over the network using protobuf. The non-null policy enforced by protobuf should be respected by mappers (hand written or generated), but it is not an application concern and it should not leak outside the application's mapper package.
  • map proto to domain => the best course of action is to pass the value returned by the proto verbatim to the domain object. IMHO, If a generated mapper takes on him to convert protobuf default values to null it clearly steps on the application prerogatives. The application is the only one knowing if and when to convert "default" values to null in a post-mapping sanitization step using all the knowledge it have of the data. Applying a default-to-null policy is an application concern, not a mapstruct concern.
    Beside the risk is that the end result would be different between an object mapped via a mapstruct generated mapper and the same type of object mapped by a similar hand written mapper (breaking the least-astonishment-principle).

Regards.

@zartc zartc linked a pull request Mar 20, 2022 that will close this issue
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

1 participant