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.

Sign up for GitHub

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

Jump to bottom

Allow to deserialize an empty string to null #630

Merged
merged 2 commits into from
Nov 2, 2023
Merged

Allow to deserialize an empty string to null #630

merged 2 commits into from
Nov 2, 2023

Conversation

dstepanov
Copy link
Contributor

Not sure if this is the best solution. Maybe it should be configurable?

Fixes #619

sdelamo
sdelamo reviewed Nov 1, 2023
View reviewed changes
serde-jackson/src/main/java/io/micronaut/serde/jackson/JacksonDecoder.java
@@ -292,6 +292,17 @@ private Boolean decodeBooleanSlow() throws IOException {
return null;
}
case START_OBJECT, END_OBJECT, END_ARRAY, FIELD_NAME -> throw unexpectedToken(JsonToken.VALUE_TRUE, t);
case VALUE_STRING -> {
String string = parser.getText();
if (string.isEmpty()) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we use?

Suggested change
if (string.isEmpty()) {
if (StringUtils.isEmpty(string)) {

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why?

serde-jackson/src/main/java/io/micronaut/serde/jackson/JacksonDecoder.java
@@ -312,6 +323,17 @@ public byte decodeByte() throws IOException {
public Byte decodeByteNullable() throws IOException {
JsonToken t = nextToken();
switch (t) {
case VALUE_STRING -> {
String string = parser.getText();
if (string.isEmpty()) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
if (string.isEmpty()) {
if (StringUtils.isEmpty(string)) {

serde-jackson/src/main/java/io/micronaut/serde/jackson/JacksonDecoder.java
@@ -353,6 +375,17 @@ public short decodeShort() throws IOException {
public Short decodeShortNullable() throws IOException {
JsonToken t = nextToken();
switch (t) {
case VALUE_STRING -> {
String string = parser.getText();
if (string.isEmpty()) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
if (string.isEmpty()) {
if (StringUtils.isEmpty(string)) {

serde-jackson/src/main/java/io/micronaut/serde/jackson/JacksonDecoder.java
@@ -407,6 +440,9 @@ public Character decodeCharNullable() throws IOException {
}
case VALUE_STRING -> {
String string = parser.getText();
if (string.isEmpty()) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
if (string.isEmpty()) {
if (StringUtils.isEmpty(string)) {

serde-jackson/src/main/java/io/micronaut/serde/jackson/JacksonDecoder.java
@@ -460,6 +496,9 @@ private Integer decodeIntSlow() throws IOException {
}
case VALUE_STRING -> {
String string = parser.getText();
if (string.isEmpty()) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
if (string.isEmpty()) {
if (StringUtils.isEmpty(string)) {

serde-jackson/src/main/java/io/micronaut/serde/jackson/JacksonDecoder.java
@@ -579,13 +619,14 @@ public Float decodeFloatNullable() throws IOException {
switch (t) {
case VALUE_STRING -> {
String string = parser.getText();
float value;
if (string.isEmpty()) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
if (string.isEmpty()) {
if (StringUtils.isEmpty(string)) {

serde-jackson/src/main/java/io/micronaut/serde/jackson/JacksonDecoder.java
@@ -641,6 +682,9 @@ private Double decodeDoubleSlow(JsonToken t) throws IOException {
}
case VALUE_STRING -> {
String string = parser.getText();
if (string.isEmpty()) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
if (string.isEmpty()) {
if (StringUtils.isEmpty(string)) {

serde-jackson/src/main/java/io/micronaut/serde/jackson/JacksonDecoder.java
@@ -691,6 +735,9 @@ public BigInteger decodeBigIntegerNullable() throws IOException {
switch (t) {
case VALUE_STRING -> {
String string = parser.getText();
if (string.isEmpty()) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
if (string.isEmpty()) {
if (StringUtils.isEmpty(string)) {

serde-jackson/src/main/java/io/micronaut/serde/jackson/JacksonDecoder.java
@@ -741,6 +788,9 @@ public BigDecimal decodeBigDecimalNullable() throws IOException {
switch (t) {
case VALUE_STRING -> {
String string = parser.getText();
if (string.isEmpty()) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
if (string.isEmpty()) {
if (StringUtils.isEmpty(string)) {

serde-jackson/src/main/java/io/micronaut/serde/jackson/JacksonDecoder.java Outdated
@@ -793,7 +843,8 @@ private Number decodeNumber() throws IOException {

@Override
public boolean decodeNull() throws IOException {
if (peekToken() == JsonToken.VALUE_NULL) {
JsonToken jsonToken = peekToken();
if (jsonToken == JsonToken.VALUE_NULL || jsonToken == JsonToken.VALUE_STRING && parser.getText().isBlank()) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
if (jsonToken == JsonToken.VALUE_NULL || jsonToken == JsonToken.VALUE_STRING && parser.getText().isBlank()) {
if (jsonToken == JsonToken.VALUE_NULL || jsonToken == JsonToken.VALUE_STRING && StringUtils.isEmpty(parser.getText())) {

@dstepanov
Copy link
Contributor Author

I think that utility method is needed

@yawkat
Copy link
Member

yawkat commented Nov 2, 2023

why on earth does JacksonDecoder matter for forms?

@dstepanov
Copy link
Contributor Author

To convert forms into a bean, we convert a map into the JSON node model in JsonBeanPropertyBinder and deserialize it into a bean. And we get all kind of problems like this and #616

@sonarqubecloud SonarQubeCloud
Copy link

sonarqubecloud bot commented Nov 2, 2023

SonarCloud Quality Gate failed.    Quality Gate failed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 1 Code Smell

79.6% 79.6% Coverage
0.0% 0.0% Duplication

idea Catch issues before they fail your Quality Gate with our IDE extension sonarlint SonarLint

@dstepanov dstepanov merged commit 017c03e into master Nov 2, 2023
10 of 11 checks passed
@dstepanov dstepanov deleted the form branch November 2, 2023 13:43
dstepanov added a commit that referenced this pull request Nov 6, 2023
@dstepanov
Revert "Allow to deserialize an empty string to null (#630)"
32020ac 
This reverts commit 017c03e.
dstepanov added a commit that referenced this pull request Nov 6, 2023
@dstepanov
Revert "Allow to deserialize an empty string to null (#630)" (#645)
9d48c72 
* Revert "Allow to deserialize an empty string to null (#630)"

This reverts commit 017c03e.

* Keep TCK updated

* Added tests
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sdelamo sdelamo sdelamo left review comments

@graemerocher graemerocher graemerocher approved these changes

@yawkat yawkat Awaiting requested review from yawkat

Assignees
No one assigned
Labels
None yet
Projects
No open projects
4.1.7 Release
Status: Done
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Form submission of optional number input fails
4 participants
@dstepanov @yawkat @graemerocher @sdelamo