Skip to content
This repository has been archived by the owner on Feb 12, 2022. It is now read-only.

Nil union values not handled properly by jackson. #39

Open
jpbelang opened this issue Dec 1, 2019 · 11 comments
Open

Nil union values not handled properly by jackson. #39

jpbelang opened this issue Dec 1, 2019 · 11 comments

Comments

@jpbelang
Copy link
Contributor

jpbelang commented Dec 1, 2019

mulesoft-labs/raml-for-jax-rs#414

@Kaiser1989
Copy link

I found the place where to fix. I'm on it. I will send you a pull request when i'm finished.

@Kaiser1989
Copy link

Kaiser1989 commented Dec 5, 2019

OK, i'm done. Just need to write some test cases, to show that everyhing is working correctly.

All currently setup tests are running without any problems. My changes are more extensive then i thought in the beginning.

Following things needed to be changed:

  • Changed complete behaviour of Jackson-Deserialization
    • Don't use deserialize to Map anymore, use JsonNode instead
    • Added deserialization strategy for every TypeDeclaration
    • Unions, Arrays and Any Types are not supported
    • Sorting of types for better deserialization (integer before number)
  • All Unions are working fine:
    • Objects, nil and primitive types
    • Inline declaration and type declaration
  • Throwing an Ambiguous Exception if ambiguous types are used
    • This needs to be done as two String types cannot be distinguished when in Json Format anymore
  • Changed ObjectCreation for Unions to create correct inline types
  • Changed structure of Unions:
    • Changed union name (string | integer | nil => StringIntegerNilUnion)
    • Instead of one "any-field" all types have their own field (anyfield was removed)
    • This is needed, as validation cannot be done on a single field (imagine a Email (string with pattern) and nil union)
    • Created enum of types in unions to allow Switch statements, not asking every field
  • Added field creation to JSR303 to add validation for union fields
  • Changed naming of union fields to match the given type (Email (type String)) is now used as Email and not as String anymore (isEmail, getEmail)
  • Added simple Jackson2 test

TODO:

  • Add these changes to jackson (currently on in jackson2)
  • Create more complex test cases for jackson2

I hope this will help you and it will find its way into the next release as soon as possible. There is already one more feature i've seen that needs to be done. It's the validation of url parameters (i will do that in the next week, requiring jsr349 standard).

Best regards
Philipp

@jpbelang
Copy link
Contributor Author

jpbelang commented Dec 6, 2019

I'm going to be reviewing this Sunday (work has me busy....)

jpbelang added a commit that referenced this issue Dec 8, 2019
Fix for #39 (Nil union values not handled properly by jackson)
@jpbelang
Copy link
Contributor Author

jpbelang commented Dec 8, 2019

I've merged and added a couple of tests.

       NilUnionTypeImpl nilUnionType = new NilUnionTypeImpl();
        ObjectMapper mapper = new ObjectMapper();
        StringWriter out = new StringWriter();

        mapper.writeValue(out, nilUnionType);

        NilUnionType b = mapper.readValue(new StringReader(out.toString()), NilUnionType.class);

        assertTrue(b.isNil());
    }

This seems to fail. I'm working on something to fix it from .getNullValue(ctxt) from the Deserializer class. Would you agree ?

@jpbelang
Copy link
Contributor Author

jpbelang commented Dec 8, 2019

Also, not sure if this generated code is necessary for nil types.

      if (node.isNull()) {
        return new NilUnionTypeImpl(null);
      }

@jpbelang
Copy link
Contributor Author

jpbelang commented Dec 8, 2019

In #42. (I just don't want to break or go against your very helpful pull request).

@Kaiser1989
Copy link

Oh yeah, that's the bug. Null types should be created by the emtpy constructor, otherwise the value construcot is used and thhe wrong enum is set. Therfore we should also add a not null test to value constructors. I can do this tomorrow if you want. I'll send you a new request for this.

@jpbelang
Copy link
Contributor Author

jpbelang commented Dec 8, 2019

I'll merge the other PR. Give it a look then.

@jpbelang
Copy link
Contributor Author

jpbelang commented Dec 12, 2019

Damn, that's a big class full of if statements. That parser gets on my nerves :-)

There is one thing I would ask you to do: look at this class org.raml.ramltopojo.extensions.jackson1.JacksonScalarTypeSerialization#fieldBuilt: dates need to be formatted according to RAML rules. Could you add appropriate logic please and one or two tests ?

If you don't have the time, I'll merge on the weekend and do the work. (Last stretch before christmas at work, I don't have tons of time during the week).

@Kaiser1989
Copy link

I'm on it

@Kaiser1989
Copy link

I fixed the date stuff and added some tests. The commit was added to the pull request (Fix/union #43).

The serialization needed to be handled seperately. JacksonScalarTypeSerialization can't be used for this, as unions are not serialized as objects, but only single entries of it. I added a date formatter for writing and reading objects with patterns defined in JacksonScalarTypeSerialization.

Should be fine so far.

Best regards

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants