Conjure enums are no longer case sensitive

The Conjure specification requires upper case string values for enum constants.
palantir · Jan 10, 2019 · 7327148 · 7327148
1 parent 76b90c7
commit 7327148
Show file tree

Hide file tree

Showing 8 changed files with 103 additions and 32 deletions.
diff --git a/conjure-java-client-verifier/src/test/resources/ignored-test-cases.yml b/conjure-java-client-verifier/src/test/resources/ignored-test-cases.yml
@@ -72,6 +72,10 @@ client:
       - '{"10": true, "10e0": false}'
       - '{"10": true, "10.0": false}'
 
+    # 'BAD VARIANT' is not allowed, this test should use 'BAD_VARIANT' which is a valid unknown enum string.
+    receiveMapEnumExampleAlias:
+      - '{"ONE": "", "TWO": "", "BAD VARIANT": ""}'
+
   singleHeaderService:
     headerOptionalString:
       - "null"

diff --git a/conjure-java-core/src/integrationInput/java/com/palantir/product/EnumExample.java b/conjure-java-core/src/integrationInput/java/com/palantir/product/EnumExample.java
@@ -2,7 +2,7 @@
 
 import com.fasterxml.jackson.annotation.JsonCreator;
 import com.fasterxml.jackson.annotation.JsonValue;
-import java.util.Locale;
+import com.palantir.conjure.java.lib.internal.ConjureEnums;
 import java.util.Objects;
 import javax.annotation.Generated;
 
@@ -62,16 +62,16 @@ public int hashCode() {
     @JsonCreator
     public static EnumExample valueOf(String value) {
         Objects.requireNonNull(value, "value cannot be null");
-        String upperCasedValue = value.toUpperCase(Locale.ROOT);
-        switch (upperCasedValue) {
+        switch (value) {
             case "ONE":
                 return ONE;
             case "TWO":
                 return TWO;
             case "ONE_HUNDRED":
                 return ONE_HUNDRED;
             default:
-                return new EnumExample(Value.UNKNOWN, upperCasedValue);
+                ConjureEnums.validate(value);
+                return new EnumExample(Value.UNKNOWN, value);
         }
     }
 

diff --git a/conjure-java-core/src/integrationInput/java/com/palantir/product/SimpleEnum.java b/conjure-java-core/src/integrationInput/java/com/palantir/product/SimpleEnum.java
@@ -2,7 +2,7 @@
 
 import com.fasterxml.jackson.annotation.JsonCreator;
 import com.fasterxml.jackson.annotation.JsonValue;
-import java.util.Locale;
+import com.palantir.conjure.java.lib.internal.ConjureEnums;
 import java.util.Objects;
 import javax.annotation.Generated;
 
@@ -54,12 +54,12 @@ public int hashCode() {
     @JsonCreator
     public static SimpleEnum valueOf(String value) {
         Objects.requireNonNull(value, "value cannot be null");
-        String upperCasedValue = value.toUpperCase(Locale.ROOT);
-        switch (upperCasedValue) {
+        switch (value) {
             case "VALUE":
                 return VALUE;
             default:
-                return new SimpleEnum(Value.UNKNOWN, upperCasedValue);
+                ConjureEnums.validate(value);
+                return new SimpleEnum(Value.UNKNOWN, value);
         }
     }
 

diff --git a/conjure-java-core/src/main/java/com/palantir/conjure/java/types/EnumGenerator.java b/conjure-java-core/src/main/java/com/palantir/conjure/java/types/EnumGenerator.java
@@ -22,6 +22,7 @@
 import com.google.common.collect.ImmutableList;
 import com.google.common.collect.Iterables;
 import com.palantir.conjure.java.ConjureAnnotations;
+import com.palantir.conjure.java.lib.internal.ConjureEnums;
 import com.palantir.conjure.spec.EnumDefinition;
 import com.palantir.conjure.spec.EnumValueDefinition;
 import com.squareup.javapoet.ClassName;
@@ -35,7 +36,6 @@
 import com.squareup.javapoet.TypeSpec;
 import com.squareup.javapoet.TypeVariableName;
 import java.util.List;
-import java.util.Locale;
 import javax.lang.model.element.Modifier;
 import org.apache.commons.lang3.StringUtils;
 
@@ -66,7 +66,7 @@ private static TypeSpec createSafeEnum(
         TypeSpec.Builder wrapper = TypeSpec.classBuilder(typeDef.getTypeName().getName())
                 .addAnnotation(ConjureAnnotations.getConjureGeneratedAnnotation(EnumGenerator.class))
                 .addModifiers(Modifier.PUBLIC, Modifier.FINAL)
-                .addType(createEnum(enumClass, typeDef.getValues(), true))
+                .addType(createEnum(enumClass, typeDef.getValues()))
                 .addType(createVisitor(visitorClass, typeDef.getValues()))
                 .addField(enumClass, VALUE_PARAMETER, Modifier.PRIVATE, Modifier.FINAL)
                 .addField(ClassName.get(String.class), STRING_PARAMETER, Modifier.PRIVATE, Modifier.FINAL)
@@ -123,7 +123,7 @@ private static Iterable<FieldSpec> createConstants(Iterable<EnumValueDefinition>
                 });
     }
 
-    private static TypeSpec createEnum(ClassName enumClass, Iterable<EnumValueDefinition> values, boolean withUnknown) {
+    private static TypeSpec createEnum(ClassName enumClass, Iterable<EnumValueDefinition> values) {
         TypeSpec.Builder enumBuilder = TypeSpec.enumBuilder(enumClass.simpleName())
                 .addAnnotation(ConjureAnnotations.getConjureGeneratedAnnotation(EnumGenerator.class))
                 .addModifiers(Modifier.PUBLIC);
@@ -133,19 +133,7 @@ private static TypeSpec createEnum(ClassName enumClass, Iterable<EnumValueDefini
                     anonymousClassBuilder.addJavadoc("$L", StringUtils.appendIfMissing(docs.get(), "\n")));
             enumBuilder.addEnumConstant(value.getValue(), anonymousClassBuilder.build());
         }
-        if (withUnknown) {
-            enumBuilder.addEnumConstant("UNKNOWN");
-        } else {
-            enumBuilder.addMethod(MethodSpec.methodBuilder("fromString")
-                    .addJavadoc("$L", "Preferred, case-insensitive constructor for string-to-enum conversion.\n")
-                    .addAnnotation(JsonCreator.class)
-                    .addModifiers(Modifier.PUBLIC, Modifier.STATIC)
-                    .addParameter(ClassName.get(String.class), "value")
-                    .addStatement("return $T.valueOf(value.toUpperCase($T.ROOT))", enumClass, Locale.class)
-                    .returns(enumClass)
-                    .build());
-        }
-        return enumBuilder.build();
+        return enumBuilder.addEnumConstant("UNKNOWN").build();
     }
 
     private static TypeSpec createVisitor(ClassName visitorClass, Iterable<EnumValueDefinition> values) {
@@ -217,7 +205,7 @@ private static MethodSpec createValueOf(ClassName thisClass, Iterable<EnumValueD
         ParameterSpec param = ParameterSpec.builder(ClassName.get(String.class), "value").build();
 
         CodeBlock.Builder parser = CodeBlock.builder()
-                .beginControlFlow("switch (upperCasedValue)");
+                .beginControlFlow("switch ($N)", param);
         for (EnumValueDefinition value : values) {
             parser.add("case $S:\n", value.getValue())
                     .indent()
@@ -226,7 +214,9 @@ private static MethodSpec createValueOf(ClassName thisClass, Iterable<EnumValueD
         }
         parser.add("default:\n")
                 .indent()
-                .addStatement("return new $T(Value.UNKNOWN, upperCasedValue)", thisClass)
+                // Only validate unknown values, matches are validated at build time.
+                .addStatement("$T.validate($N)", ConjureEnums.class, param)
+                .addStatement("return new $T(Value.UNKNOWN, $N)", thisClass, param)
                 .unindent()
                 .endControlFlow();
 
@@ -236,8 +226,6 @@ private static MethodSpec createValueOf(ClassName thisClass, Iterable<EnumValueD
                 .addAnnotation(JsonCreator.class)
                 .addParameter(param)
                 .addStatement("$L", Expressions.requireNonNull(param.name, param.name + " cannot be null"))
-                // uppercase param for backwards compatibility
-                .addStatement("String upperCasedValue = $N.toUpperCase($T.ROOT)", param, Locale.class)
                 .addCode(parser.build())
                 .build();
     }

diff --git a/conjure-java-core/src/test/java/com/palantir/conjure/java/types/WireFormatTests.java b/conjure-java-core/src/test/java/com/palantir/conjure/java/types/WireFormatTests.java
@@ -4,12 +4,18 @@
 
 package com.palantir.conjure.java.types;
 
+import static com.palantir.logsafe.testing.Assertions.assertThatLoggableExceptionThrownBy;
 import static org.assertj.core.api.Assertions.assertThat;
 
 import com.fasterxml.jackson.databind.JsonMappingException;
 import com.fasterxml.jackson.databind.ObjectMapper;
+import com.google.common.base.Throwables;
 import com.google.common.collect.ImmutableSet;
 import com.palantir.conjure.java.serialization.ObjectMappers;
+import com.palantir.logsafe.SafeLoggable;
+import com.palantir.logsafe.UnsafeArg;
+import com.palantir.logsafe.exceptions.SafeIllegalArgumentException;
+import com.palantir.logsafe.testing.LoggableExceptionAssert;
 import com.palantir.product.BinaryAliasExample;
 import com.palantir.product.BinaryExample;
 import com.palantir.product.DateTimeExample;
@@ -32,6 +38,7 @@
 import java.time.ZoneOffset;
 import java.util.Set;
 import java.util.UUID;
+import org.assertj.core.api.ThrowableAssert;
 import org.junit.Rule;
 import org.junit.Test;
 import org.junit.rules.ExpectedException;
@@ -154,9 +161,30 @@ public void testEmptyObjectsDeserialize() throws Exception {
     @Test
     public void testEnumCasingDeserializationInvariantToInputCase() throws Exception {
         assertThat(mapper.readValue("\"ONE\"", EnumExample.class)).isEqualTo(EnumExample.ONE);
-        assertThat(mapper.readValue("\"one\"", EnumExample.class)).isEqualTo(EnumExample.ONE);
-        assertThat(mapper.readValue("\"onE\"", EnumExample.class)).isEqualTo(EnumExample.ONE);
-        assertThat(mapper.readValue("\"oNE\"", EnumExample.class)).isEqualTo(EnumExample.ONE);
+        assertThat(mapper.readValue("\"ONE_HUNDRED\"", EnumExample.class)).isEqualTo(EnumExample.ONE_HUNDRED);
+        assertThatExceptionThrownRootCause(() -> mapper.readValue("\"one\"", EnumExample.class))
+                .isInstanceOf(SafeIllegalArgumentException.class)
+                .hasLogMessage("Enum values must use SCREAMING_SNAKE_CASE")
+                .hasExactlyArgs(UnsafeArg.of("value", "one"));
+        assertThatExceptionThrownRootCause(() -> mapper.readValue("\"onE\"", EnumExample.class))
+                .isInstanceOf(SafeIllegalArgumentException.class)
+                .hasLogMessage("Enum values must use SCREAMING_SNAKE_CASE")
+                .hasExactlyArgs(UnsafeArg.of("value", "onE"));
+        assertThatExceptionThrownRootCause(() -> mapper.readValue("\"oNE\"", EnumExample.class))
+                .isInstanceOf(SafeIllegalArgumentException.class)
+                .hasLogMessage("Enum values must use SCREAMING_SNAKE_CASE")
+                .hasExactlyArgs(UnsafeArg.of("value", "oNE"));
+    }
+
+    private <T extends Throwable & SafeLoggable> LoggableExceptionAssert<T> assertThatExceptionThrownRootCause(
+            ThrowableAssert.ThrowingCallable task) {
+        return assertThatLoggableExceptionThrownBy(() -> {
+            try {
+                task.call();
+            } catch (Throwable t) {
+                throw Throwables.getRootCause(t);
+            }
+        });
     }
 
     @Test

diff --git a/conjure-java-server-verifier/src/test/resources/ignored-test-cases.yml b/conjure-java-server-verifier/src/test/resources/ignored-test-cases.yml
@@ -3,6 +3,9 @@ server:
     getMapBinaryAliasExample:
       - '{}'
       - '{"SGVsbG8sIFdvcmxk": true}'
+    # 'BAD VARIANT' is not allowed, this test should use 'BAD_VARIANT' which is a valid unknown enum string.
+    getMapEnumExampleAlias:
+      - '{"ONE": "", "TWO": "", "BAD VARIANT": ""}'
     getRawOptionalExample:
       - 'null'
     getOptionalBearerTokenAliasExample:

diff --git a/conjure-lib/src/main/java/com/palantir/conjure/java/lib/internal/ConjureEnums.java b/conjure-lib/src/main/java/com/palantir/conjure/java/lib/internal/ConjureEnums.java
@@ -0,0 +1,48 @@
+/*
+ * (c) Copyright 2019 Palantir Technologies Inc. All rights reserved.
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ *     http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package com.palantir.conjure.java.lib.internal;
+
+import com.palantir.logsafe.UnsafeArg;
+import com.palantir.logsafe.exceptions.SafeIllegalArgumentException;
+
+/**
+ * Internal utility functions for conjure enum types.
+ */
+public final class ConjureEnums {
+
+    private ConjureEnums() {
+        // cannot instantiate
+    }
+
+    public static void validate(String value) {
+        if (value.isEmpty()) {
+            throw new SafeIllegalArgumentException("Enum values must not be empty");
+        }
+
+        int length = value.length();
+        for (int index = 0; index < length; index++) {
+            if (!isAllowedCharacter(value.charAt(index))) {
+                throw new SafeIllegalArgumentException("Enum values must use SCREAMING_SNAKE_CASE",
+                        UnsafeArg.of("value", value));
+            }
+        }
+    }
+
+    private static boolean isAllowedCharacter(char character) {
+        return (character >= 'A' && character <= 'Z') || character == '_';
+    }
+}
diff --git a/versions.props b/versions.props
@@ -10,7 +10,7 @@ com.palantir.conjure.java.runtime:* = 4.12.0
 # Regression introduced by https://github.com/palantir/conjure-java-runtime/pull/865
 com.palantir.conjure.java.runtime:conjure-java-jaxrs-client = 4.6.0
 # dependency-upgrader:ON
-com.palantir.conjure.verification:* = 0.16.2
+com.palantir.conjure.verification:* = 0.16.5
 com.palantir.conjure:* = 4.2.2
 com.palantir.ri:resource-identifier = 1.0.1
 com.palantir.syntactic-paths:syntactic-paths = 0.6.1