Enum values are case sensitive, values are validated #143
Conversation
| @@ -152,11 +152,11 @@ public void testEmptyObjectsDeserialize() throws Exception { | |||
| } | |||
|
|
|||
| @Test | |||
| public void testEnumCasingDeserializationInvariantToInputCase() throws Exception { | |||
| public void testEnumCasingDeserialization() throws Exception { | |||
There was a problem hiding this comment.
is there no corresponding test in conjure-verifier? (I think there should be)
|
I think it'd be preferable to finish the discussion on #134 prior to merging this. As noted, this might be a significant break since early adopters requested this leniency during decoding. |
|
ack |
| assertThat(mapper.readValue("\"ONE\"", EnumExample.class)).isEqualTo(EnumExample.ONE); | ||
| assertThat(mapper.readValue("\"one\"", EnumExample.class)).isEqualTo(EnumExample.ONE); |
There was a problem hiding this comment.
I think this is a pretty dangerous behaviour change in the case where users might have already persisted lowercase enum values to a database. Would prefer this to throw.
There was a problem hiding this comment.
That's fair, I will add validation matching our schema regex to the valueOf(String) statement
There was a problem hiding this comment.
For now I'll just validate chars A-Z_ without dealing with leading/trailing/duplicate underscores, we can add that later.
carterkozak
force-pushed
the
ckozak/case_sensitive_enums
branch
2 times, most recently
from
cc534b7 to
14488b0
Compare
carterkozak
changed the title
carterkozak
force-pushed
the
ckozak/case_sensitive_enums
branch
from
1309385 to
7327148
Compare
carterkozak
changed the title
carterkozak
force-pushed
the
ckozak/case_sensitive_enums
branch
from
7327148 to
1ac4319
Compare
The Conjure specification requires upper case string values for enum constants.
carterkozak
force-pushed
the
ckozak/case_sensitive_enums
branch
from
1ac4319 to
5276c58
Compare
| .hasExactlyArgs(UnsafeArg.of("value", "onE")); | ||
| assertThatExceptionThrownRootCause(() -> mapper.readValue("\"oNE\"", EnumExample.class)) | ||
| .isInstanceOf(SafeIllegalArgumentException.class) | ||
| .hasLogMessage("Enum values must use SCREAMING_SNAKE_CASE") |
There was a problem hiding this comment.
Could we tweak this error message to just say UPPER_SNAKE_CASE? I think the 'screaming' bit might be a bit more humour than java programmers can handle.
Edit I'm just gonna push this fix and merge I think.
There was a problem hiding this comment.
Sounds good, thanks Dan!
There was a problem hiding this comment.
ah whoops, can I have some perms?
This reverts commit 4d39bdd.
The Conjure specification requires upper case string values for
enum constants.