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

A union type even in invalid format should deserialize to Unknown #564

Open
gm2211 opened this issue Sep 18, 2019 · 1 comment
Open

A union type even in invalid format should deserialize to Unknown #564

gm2211 opened this issue Sep 18, 2019 · 1 comment

Comments

@gm2211
Copy link

gm2211 commented Sep 18, 2019

What happened?

Given:

    objects:
      Foo:
        fields:
          foo: string
      Bar:
        fields:
          bar: string
      FooBar:
        union:
          foo: Foo
          bar: Bar

the following fails:

        ObjectMapper mapper = ObjectMappers.createDefaultJsonObjectMapper(false);
        String good = "{\n"
                + "  \"type\" : \"bar\",\n"
                + "  \"bar\" : {\n"
                + "    \"bar\" : \"123\"\n"
                + "  }\n"
                + "}";
        String bad = "{\n"
                + "  \"type\" : \"new\",\n"
                + "  \"some-string\""
                + "}";

        assertThat(mapper.readValue(good, FooBar.class))
                .extracting("value")
                .extracting("value")
                .isInstanceOf(Bar.class);
        assertThat(mapper.readValue(bad, FooBar.class))
                .extracting("value")
                .extracting(Object::getClass)
                .extracting(Class::getSimpleName)
                .asString()
                .contains("UnknownWrapper");

What did you want to happen?

The test should pass.

We can achieve this by modifying the generated code for the unknown wrapper to look something like this:

image

this is obviously not completely correct since if type is missing or there exists no sub-object, we'd be setting the raw invalid json as type, so we'd probably have to do a little more work (like making type optional or nullable for the unknown wrapper)

@iamdanfox
Copy link
Contributor

I dunno - the whole 'unknown' thing is supposed to make conjure enums forward compatible, so that old code can be left running and it shouldn't break if a server starts sending new responses.

Your bad example has a random floating "some-string", which can't plausibly be emitted by anything complying with the current conjure spec.

I think changing this behaviour would be net negative, because it would make it harder to catch malformed inputs, without actually improving our forwards-comaptibility posture.

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

No branches or pull requests

2 participants