[fix] Builders check primitives are initialized

[iamdanfox]

Before this PR

Conjure POJOs containing primitives (e.g. int, boolean) would be coerced from JSON in which the fields were actually missing. This was not compliant with the wire spec and could lead to nasty bugs, where a field was unintentionally initialized to 0 or false. For example, consider this type:

UpdateSalaryRequest:
  salary: integer
  shares: integer

The following request would result in dfox's salary being set to 0, because the the 'salary' field was missing and is initialized to 0!!

curl -XPOST https://some-api/update-salary/dfox --data '{"shares":20}'

After this PR

Trying to deserialize a type with booleans, integers or doubles from {} will now fail.

Concerns

People may have been relying on this behaviour, so there's a chance this might break people's servers... In theory, we could put this behaviour behind a feature flag, and then phase it in slowly?? EDIT not gonna do this as it's not actually a wire-break and people's servers can be trivially fixed by updating their conjure defs:

 UpdateSalaryRequest:
-  salary: integer
+  salary: optional<integer>
   shares: integer

Future work: unify this validatePrimitivesHaveBeenInitialized method with the other validateFields method

fixes #14

[bkrieger]

It's a little late now, but I think this should have gone into Conjure 2.0.0. While it's technically not a wire break as you say, an unknowing server could accidentally make a wire break if it didn't notice this change.

[iamdanfox]

@bkrieger yeah this was definitely a subtle change, but I'm not sure under what criteria we'd class this as a break?

• old JSON can still be deserialized by new servers
• JSON serialized by new servers can still be deserialized by old clients

The only case this wouldn't work is if a new client was trying to talk to an old server, but we don't claim to support that workflow anyway.

[bkrieger]

If you upgrade the server to a version with this change, and you don't notice that you have to change boolean to optional<boolean>, then old JSON cannot be deserialized by new servers.

[iamdanfox]

@bkrieger if the JSON came from a remoting client then you'll never actually hit that problem because ObjectMappers.newObjectMapper() will always serialize the value as a boolean:

class Foo {
 boolean hello;
}

System.out.println(ObjectMappers.newClientObjectMapper().writeValueAsString(new Foo()));
// {"hello": false}

[bkrieger]

Sure, but we have non-Java consumers. In addition, a client using an old version on the Java API might not include the Boolean if it was previously not included in the API. We used the fact the booleans default to false as a way to add fields to the a request object without a breaking change.

…

On Tue, Sep 18, 2018 at 12:36 PM iamdanfox ***@***.***> wrote: @bkrieger <https://github.com/bkrieger> if the JSON came from a remoting client then you'll never actually hit that problem because ObjectMappers.newObjectMapper() will always serialize the value as a boolean: class Foo { boolean hello; } System.out.println(ObjectMappers.newClientObjectMapper().writeValueAsString(new Foo()));// {"hello": false} — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#36 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ABl4V3USUzgLUckOmLB2mjMVCIBzHwq6ks5ucSEXgaJpZM4VLbGz> .

[iamdanfox]

So the requirement now is to add optional<boolean> if you want to add a field to a request object in a backwards compatible way (still tolerating requests from older clients).

[bkrieger]

Yep, and that's fine going forward. But if I used boolean with an old conjure version, and then upgraded conjure without carefully checking the release notes (because this was marked as a fix, not a break), I've lost backcompat.

…

On Wed, Sep 19, 2018 at 5:48 AM iamdanfox ***@***.***> wrote: So the requirement now is to add optional<boolean> if you want to add a field to a request object in a backwards compatible way (still tolerating requests from older clients). — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#36 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ABl4VxRfdfZHhvGq3RZpCq4Z3QDE5Otaks5uchLMgaJpZM4VLbGz> .