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

Deserializing JSON with missing field fails using @JsonbCreator #237

Open
emattheis opened this issue Feb 13, 2019 · 12 comments · May be fixed by #239
Open

Deserializing JSON with missing field fails using @JsonbCreator #237

emattheis opened this issue Feb 13, 2019 · 12 comments · May be fixed by #239
Assignees
@Verdent @aguibert
Labels
enhancement

Comments

@emattheis
Copy link
Contributor

When I attempt to deserialize a JSON document with missing fields as a Java type using @JsonbCreator I get the following exception:

javax.json.bind.JsonbException: JsonbCreator parameter PARAM_NAME is missing in json document.

Looking into the source, I found this which indicates that the error condition is mandated by the spec. However, I can't find anything in the spec that requires all parameters in a creator to be present. Am I missing it?

Here's a test class that illustrates the issue:

import static org.junit.jupiter.api.Assertions.assertEquals;
import static org.junit.jupiter.api.Assertions.assertNull;

import javax.json.bind.Jsonb;
import javax.json.bind.annotation.JsonbCreator;
import javax.json.bind.annotation.JsonbProperty;

import org.eclipse.yasson.internal.JsonBindingBuilder;
import org.junit.jupiter.api.BeforeEach;
import org.junit.jupiter.api.Test;

public class JsonbCreatorTest {
    public static class Mutable {
        private String test = "default";

        public String getTest() {
            return test;
        }

        public void setTest(String test) {
            this.test = test;
        }
    }

    public static class Immutable {
        private final String test;

        @JsonbCreator
        public Immutable(@JsonbProperty("test") String test) {
            this.test = test;
        }

        public String getTest() {
            return test;
        }
    }

    private static final String OBJECT_WITH_FIELD = "{\"test\":\"test\"}";

    private static final String EMPTY_OBJECT = "{}";

    private Jsonb jsonb;

    @BeforeEach
    public void createJsonBuilder() {
        jsonb = new JsonBindingBuilder().build();
    }

    @Test
    public void testDeserializeMutableFromObjectWithField() {
    	Mutable mutable = jsonb.fromJson(OBJECT_WITH_FIELD, Mutable.class);
    	assertEquals("test", mutable.getTest());
    }

    @Test
    public void testDeserializeImmutableFromObjectWithField() {
    	Immutable immutable = jsonb.fromJson(OBJECT_WITH_FIELD, Immutable.class);
    	assertEquals("test", immutable.getTest());
    }

    @Test
    public void testDeserializeMutableFromEmptyOBject() {
    	Mutable mutable = jsonb.fromJson(EMPTY_OBJECT, Mutable.class);
    	// spec says the setter MUST NOT be called when the property is absent
    	assertEquals("default", mutable.getTest());
    }

    @Test
    public void testDeserializeImmutableFromEmptyOBject() {
    	Immutable immutable = jsonb.fromJson(EMPTY_OBJECT, Immutable.class);
    	// spec does not seem to mention how to handle this - null seems like the only sane choice
    	assertNull(immutable.getTest());
    }
}

Ultimately, I think this needs to be clarified in the spec, so I will raise it there as well, but I would like a way to get null passed in to creators when fields are missing instead of exceptions. I don't want to have to resort to mutable POJOs just to support this use case.
@bravehorsie
Copy link
Contributor

Its section 4.5

In case a field required for a parameter mapping doesn’t exist in JSON document, JsonbException MUST be thrown.

The complicated thing with this example is that Yasson will never know the field is missing intentionally or not.
When parsing "{"test":null}", JsonParser encounters a test field for a constructor or setter and Yasson propagates the null value.
However parsing "{}" there is no callback into Yasson for property to be set. In case of @JsonbCreator Yasson can be "smart" and use null because it has a constructor to call but there are no values for it, but what if properties are missed in JSON unintentionally?

@emattheis
Copy link
Contributor Author

I followed the link from the JSON-B website which leads here. Now I realize that is the public review draft. 🙄 Anyway, I found the final spec and I see the reference.

This is disappointing since it basically means you cannot deserialize to a truly immutable type if your JSON documents have optional fields which is quite common in my experience, but I understand Yasson's position on spec-compliance, so I'll raise the issue in the json-b api project.

This was referenced Feb 14, 2019
Closed
Open
@misl misl mentioned this issue Jun 24, 2019
Open
@misl
Copy link

misl commented Jun 25, 2019

Wouldn't it still be compliant with JSON-B specification if only Optional<> JsonbCreator arguments were allowed to be missing? In this case missing none Optional<> arguments would still throw the MessageKeys.JSONB_CREATOR_MISSING_PROPERTY error.

I did something similar in #276. I only noticed how similar both solutions were after I was done.

@m0mus
Copy link
Member

m0mus commented Jun 26, 2019

@aguibert @Verdent Can you folks review it. I am also interested in is TCK will pass with this PR merged.

@aguibert aguibert mentioned this issue Jun 27, 2019
Open
@aguibert
Copy link
Member

@m0mus before we merge this PR or the similar PR that @misl proposed, we need to address this at the JSON-B spec level over at jakartaee/jsonb-api#121
Lets focus our efforts on that JSON-B spec issue and then we can do the appropriate impl in Yasson.

@m0mus
Copy link
Member

m0mus commented Jun 28, 2019

We discussed it with @Verdent today. We both agree that the issue exists and it makes sense to fix it. The problem is that it goes against the spec which explicitly restricts this behavior. Thanks @aguibert for creating a task in JSONB spec issues tracker. We will address it when we will start working on a new version of the spec.
It doesn't mean that it cannot be fixed in Yasson now. We can add a Yasson specific configuration property called for example ALLOW_JSONB_CREATOR_OPTIONAL_PARAMS. By default this property is not set which makes Yasson 100% spec compliant. If this property is set JsonbCreator behavior is switched to what's been suggested above. Setting configuration property is easy:

JsonbConfig config = new JsonbConfig()
    .setProperty("ALLOW_JSONB_CREATOR_OPTIONAL_PARAMS", true);

@aguibert @emattheis what do you think? If you agree please change a PR.

@emattheis
Copy link
Contributor Author

emattheis commented Jun 28, 2019 •
edited
Loading

Personally, my interest in Yasson is limited to its role as the JSON-B reference implementation, so I am happy to wait until this becomes specified behavior. If I can't write portable code, then I'm going to use a more mature JSON framework anyway.

I don't necessarily object to making this a configurable behavior of Yasson, and I imagine it won't be difficult to add, but I think it dilutes the value of the JSON-B standard when all the implementations support a common feature in a non-standard way. It would be interesting to know if the TCK specifically enforces spec-compliance in this case. If it doesn't, perhaps a minor change to the spec could be fast-tracked somehow?

@aguibert
Copy link
Member

@m0mus I hesitate to add a yasson-specific switch to enable non-spec compliant behavior for the reasons that @emattheis outlined and also because it creates more long-term code maintenance for Yasson. Especially if JSON-B decides to solve this in a completely different way, then we would have to support both ways in Yasson and hope they don't conflict.

We can't fast-track a change to the JSON-B spec unfortuantely. I think this would be a must-have feature for the next version of JSON-B though.
Dmitry, do you know if we are allowed to start working on JSON-B v1.1 now? Or do we have to wait until JakartaEE 8 finalizes before we can do any work at all?

@m0mus
Copy link
Member

m0mus commented Jun 29, 2019

@emattheis in this case we should close this issue. Andy already added a task to JSONB issues tracker. There is no need to keep it open.

@aguibert Sure, we can start working on JSONB 1.1 now in master branch. It's fine to define a scope and feature set and start working on updating APIs. We cannot release a final version without going through the spec approval process (but we can release non-final versions such as betas, RCs, etc.). It's also unclear about javax->jakarta namespace conversion at the moment. I guess we can change all packages to jakarta.* now and start thinking about how we implement it in Yasson.

@m0mus m0mus mentioned this issue Jul 26, 2019
Closed
@misl misl mentioned this issue Oct 2, 2019
Open
@oliviercailloux
Copy link

The corresponding issue is marked as solved. I am sure that many developers are eager to use this feature. Is Yasson going to implement this soon?

@aguibert wrote, on 29 July 2019: “IMO optional constructor properties are one of the top 3 most needed features for JSON-B (…), my preference is to wait until this is defined at the spec level (which will hopefully be soon).”

@Verdent
Copy link
Member

Verdent commented Feb 3, 2022

@oliviercailloux sure thing. I am already having working implementation of this feature in my local branch. We will try to get this branch to the master soon. This feature will be released as soon as the new JSONB is released.

@mkarg
Copy link
Member

mkarg commented Mar 28, 2024

What is the current status of this?

oliviercailloux pushed a commit to oliviercailloux/JGradLib that referenced this issue Jun 17, 2024
Workaround eclipse-ee4j/yasson#237 by converting record into class
d73d95a
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@Verdent Verdent

@aguibert aguibert

Labels
enhancement
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Add support for optional JsonbCreator parameters emattheis/yasson
8 participants
@oliviercailloux @mkarg @misl @Verdent @aguibert @m0mus @bravehorsie @emattheis