[fix] bring back old enum behaviour

[iamdanfox]

Before this PR

Pre-2.8.0, conjure-java generated very lenient code: an enum defined as [BLUE, GREEN] would accept values blue, bLuE and bLUE as BLUE. It would also tolerate unknown values, such as "RED", "", "!@%^£".

When looking into tightening up the Conjure spec, we decided that all conjure languages should not be required to implement this behaviour. In 2.8.0, we tightened up the validation of conjure-generated enums, I expected this to be safe because conjure-typescript only ever emitted uppercase values. Unfortunately, this caused P0s at some products where bad values had already been persisted into databases, from non-typescript origins.

To alleviate some of this pain, we added a --useInsensitiveEnums flag to bring back the old behaviour. Unfortunately, this would still throw when deserializing strange values like "", which have already been persisted to databases. Also, existing products do not know whether they have nonsensical values in their database already, and realistically do not have serialization tests that exercise every enum edge case, so they didn't know whether they needed to opt-in.

After this PR

To avoid causing lots more P0s and keeping feature flags forever, this PR reverts conjure-java's codegen to the old pre-2.8.0 behaviour. The Conjure spec still says only deserialize and serialize UPPERCASE values, but this PR intentionally diverges from the spec in order to avoid causing pain for existing products.

We think this will be safe for inter-language communication because a string "bLuE" is deserialized as MyEnum.BLUE and re-serialized as "BLUE" so typescript clients keep working.

In the future, we may want to implement a feature where servers can reject all 'unknown' values in incoming requests.

cc @diogoholanda @markelliot @cakofony

[carterkozak]

I’m fine with this, though based on previous painful breaks caused by new validation it would be nice to have clarity on when a revert is allowable. The most recent example that comes to mind is the break in string->long coercion in conjure-java-runtime, causing clients to throw at runtime for certain APIs.

[iamdanfox]

@cakofony the feedback we've got has told us that in order for people to trust Conjure (especially for people using it for database persistence) we can't really just make our implementations stricter without really good comms (cutover period, major rev etc). With that principle in mind, I think we shouldn't have made that string -> long change in a minor version either (although with that one at least it's just an objectmapper flag to get back the old behaviour).

Ideally I'd like people to be able to trust Conjure enough to use it as the persistence layer, which is a very high bar and would require really careful staging of any change that affects deserialization or wire formats.

[dansanduleac]

👍

[carterkozak]

Thanks Dan

…

-ck

On Feb 18, 2019, at 10:14 AM, iamdanfox ***@***.***> wrote: @cakofony the feedback we've got has told us that in order for people to trust Conjure (especially for people using it for database persistence) we can't really just make our implementations stricter. With that principle in mind, I think we shouldn't have made that string -> long change in a minor version either (although with that one at least it's just an objectmapper flag to get back the old behaviour). — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or mute the thread.