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

validation: Fix @JsonProperty(required = true) annotation on primitives #97

Open
BrunoRosendo opened this issue Feb 5, 2023 · 0 comments
Labels
bug Something isn't working
Milestone

Comments

@BrunoRosendo
Copy link
Member

BrunoRosendo commented Feb 5, 2023

Ideally, Kotlin's nullable type system would be enough to dictate which fields are required and which are optional. However, this logic doesn't work on primitives (Boolean, Int, Long, etc.) because Jackson (our json mapper) transforms a missing value into the default one (false for boolean, 0 for numbers, etc.). That's why we're currently using @JsonProperty(required = true) on those fields. In a sad try to be consistent, we even added the annotation to all required fields, even though most of them weren't even necessary.

There are 2 problems with this approach:

  • It clutters the code because we're adding this annotation to dozens of fields that don't need it. If we remove them, then it passes the message that the fields might not be required (in a newcomer's eyes, this boolean is the only required field in the class!).
  • This annotation, when used on non-nullable primitives, throws a null pointer exception (see RoleDto example below from Feature/generations #88). This exception is too broad and shouldn't be treated for their specific case.

My suggestion is to create a custom ConstraintValidator annotation (e.g. @RequiredPrimitive) that checks if the converted value is missing and sends a readable message error. Note, however, that this implementation might not be trivial since it might have to process some custom mapping, so consider other options. There might also be some existing annotation that I'm not aware of, so make sure to research this matter.

In worst-case scenario, we could at least create a wrapper annotation of @JsonProperty(required = true) with a different name

@BrunoRosendo BrunoRosendo added the bug Something isn't working label Feb 5, 2023
@BrunoRosendo BrunoRosendo added this to the Website MVP milestone Feb 8, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

1 participant