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

176 mappers should never return null #177

Open
wants to merge 4 commits into
base: develop
Choose a base branch
from

Conversation

zartc
Copy link

@zartc zartc commented Mar 20, 2022

Here's the code that implements the ban of null return values from mappers.

  • null return values from domain->proto mapper will end up with a NPE thrown by Protobuff.
  • null return values from proto->domain is less dangerous but it is best to let the application decide what to do in a post-mapper step.

The changes consist of systematically return a "default' value: empty string or arrays, EPOCH, zero, or protobuff's getDefaultInstance.

Close #176

- add protobuf-java-util dependency to the pom.xml, required by the new unit-test

add a unit-test to make sure an hypothesis is true
The mapper are not allowed to return null anymore.
It is the responsibility of the application to perform
domain value check and defaultToNull post processing when appropriate.
… null

replace one unit-test to conform with the new implementation that ban null
@seime
Copy link
Contributor

seime commented Sep 8, 2022

@zartc Sorry for the late response,

I agree with your reasoning. On the proto->domain side I would like to see 2 different implementations, yours (map to empty domain) as well as the existing one (map to null).

If we split the existing mapper class into 3 variants

  • DomainToProto
  • ProtoToEmptyDomain
  • ProtoToNullDomain

the developer can select the appropriate implementation based on his/her needs. It also avoid breaking (runtime) any existing use of this library (mapper uses attribute must be updated, but this is detected compile time)

@wirekang
Copy link

Any news?

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 this pull request may close these issues.

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