Skip to content

Commit

Permalink
[fix] bring back old enum behaviour (#238)
Browse files Browse the repository at this point in the history 
## 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](palantir/conjure#193), 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`](#232) 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
  • Loading branch information
@iamdanfox @bulldozer-bot
iamdanfox authored and bulldozer-bot[bot] committed Feb 18, 2019
1 parent 8bf6a57 commit ee749ae
Show file tree
Hide file tree
Showing 17 changed files with 56 additions and 270 deletions.
117 changes: 0 additions & 117 deletions conjure-java-core/src/integrationInput/java/com/palantir/insensitive/InsensitiveEnum.java
View file

This file was deleted.

8 changes: 4 additions & 4 deletions conjure-java-core/src/integrationInput/java/com/palantir/product/EnumExample.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2,8 +2,8 @@

import com.fasterxml.jackson.annotation.JsonCreator;
import com.fasterxml.jackson.annotation.JsonValue;
import com.palantir.conjure.java.lib.internal.ConjureEnums;
import com.palantir.logsafe.Preconditions;
import java.util.Locale;
import javax.annotation.Generated;

/**
Expand Down Expand Up @@ -62,16 +62,16 @@ public int hashCode() {
@JsonCreator
public static EnumExample valueOf(String value) {
Preconditions.checkNotNull(value, "value cannot be null");
switch (value) {
String upperCasedValue = value.toUpperCase(Locale.ROOT);
switch (upperCasedValue) {
case "ONE":
return ONE;
case "TWO":
return TWO;
case "ONE_HUNDRED":
return ONE_HUNDRED;
default:
ConjureEnums.validate(value);
return new EnumExample(Value.UNKNOWN, value);
return new EnumExample(Value.UNKNOWN, upperCasedValue);
}
}

Expand Down
18 changes: 4 additions & 14 deletions conjure-java-core/src/integrationInput/java/com/palantir/product/SimpleEnum.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2,8 +2,8 @@

import com.fasterxml.jackson.annotation.JsonCreator;
import com.fasterxml.jackson.annotation.JsonValue;
import com.palantir.conjure.java.lib.internal.ConjureEnums;
import com.palantir.logsafe.Preconditions;
import java.util.Locale;
import javax.annotation.Generated;

/**
Expand All @@ -21,8 +21,6 @@
public final class SimpleEnum {
public static final SimpleEnum VALUE = new SimpleEnum(Value.VALUE, "VALUE");

public static final SimpleEnum VALUE2 = new SimpleEnum(Value.VALUE2, "VALUE2");

private final Value value;

private final String string;
Expand Down Expand Up @@ -56,23 +54,19 @@ public int hashCode() {
@JsonCreator
public static SimpleEnum valueOf(String value) {
Preconditions.checkNotNull(value, "value cannot be null");
switch (value) {
String upperCasedValue = value.toUpperCase(Locale.ROOT);
switch (upperCasedValue) {
case "VALUE":
return VALUE;
case "VALUE2":
return VALUE2;
default:
ConjureEnums.validate(value);
return new SimpleEnum(Value.UNKNOWN, value);
return new SimpleEnum(Value.UNKNOWN, upperCasedValue);
}
}

public <T> T accept(Visitor<T> visitor) {
switch (value) {
case VALUE:
return visitor.visitValue();
case VALUE2:
return visitor.visitValue2();
default:
return visitor.visitUnknown(string);
}
Expand All @@ -82,17 +76,13 @@ public <T> T accept(Visitor<T> visitor) {
public enum Value {
VALUE,

VALUE2,

UNKNOWN
}

@Generated("com.palantir.conjure.java.types.EnumGenerator")
public interface Visitor<T> {
T visitValue();

T visitValue2();

T visitUnknown(String unknownValue);
}
}
6 changes: 0 additions & 6 deletions conjure-java-core/src/main/java/com/palantir/conjure/java/FeatureFlags.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -59,10 +59,4 @@ public enum FeatureFlags {
* Use the conjure immutable "Bytes" class over ByteBuffer.
*/
UseImmutableBytes,

/**
* Enums valueOf function will use a case-insensitive lookup. Note that this is not allowed by the conjure
* specification, however may be enabled for backwards compatibility.
*/
CaseInsensitiveEnums,
}
57 changes: 28 additions & 29 deletions conjure-java-core/src/main/java/com/palantir/conjure/java/types/EnumGenerator.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -22,8 +22,6 @@
import com.google.common.collect.ImmutableList;
import com.google.common.collect.Iterables;
import com.palantir.conjure.java.ConjureAnnotations;
import com.palantir.conjure.java.FeatureFlags;
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;
Expand All @@ -38,7 +36,6 @@
import com.squareup.javapoet.TypeVariableName;
import java.util.List;
import java.util.Locale;
import java.util.Set;
import javax.lang.model.element.Modifier;
import org.apache.commons.lang3.StringUtils;

Expand All @@ -52,28 +49,24 @@ public final class EnumGenerator {

private EnumGenerator() {}

public static JavaFile generateEnumType(EnumDefinition typeDef, Set<FeatureFlags> featureFlags) {
public static JavaFile generateEnumType(EnumDefinition typeDef) {
String typePackage = typeDef.getTypeName().getPackage();
ClassName thisClass = ClassName.get(typePackage, typeDef.getTypeName().getName());
ClassName enumClass = ClassName.get(typePackage, typeDef.getTypeName().getName(), "Value");
ClassName visitorClass = ClassName.get(typePackage, typeDef.getTypeName().getName(), "Visitor");

return JavaFile.builder(typePackage, createSafeEnum(typeDef, thisClass, enumClass, visitorClass, featureFlags))
return JavaFile.builder(typePackage, createSafeEnum(typeDef, thisClass, enumClass, visitorClass))
.skipJavaLangImports(true)
.indent(" ")
.build();
}

private static TypeSpec createSafeEnum(
EnumDefinition typeDef,
ClassName thisClass,
ClassName enumClass,
ClassName visitorClass,
Set<FeatureFlags> featureFlags) {
EnumDefinition typeDef, ClassName thisClass, ClassName enumClass, ClassName visitorClass) {
TypeSpec.Builder wrapper = TypeSpec.classBuilder(typeDef.getTypeName().getName())
.addAnnotation(ConjureAnnotations.getConjureGeneratedAnnotation(EnumGenerator.class))
.addModifiers(Modifier.PUBLIC, Modifier.FINAL)
.addType(createEnum(enumClass, typeDef.getValues()))
.addType(createEnum(enumClass, typeDef.getValues(), true))
.addType(createVisitor(visitorClass, typeDef.getValues()))
.addField(enumClass, VALUE_PARAMETER, Modifier.PRIVATE, Modifier.FINAL)
.addField(ClassName.get(String.class), STRING_PARAMETER, Modifier.PRIVATE, Modifier.FINAL)
Expand All @@ -93,7 +86,7 @@ private static TypeSpec createSafeEnum(
.build())
.addMethod(createEquals(thisClass))
.addMethod(createHashCode())
.addMethod(createValueOf(thisClass, typeDef.getValues(), featureFlags))
.addMethod(createValueOf(thisClass, typeDef.getValues()))
.addMethod(generateAcceptVisitMethod(visitorClass, typeDef.getValues()));

typeDef.getDocs().ifPresent(
Expand Down Expand Up @@ -130,7 +123,7 @@ private static Iterable<FieldSpec> createConstants(Iterable<EnumValueDefinition>
});
}

private static TypeSpec createEnum(ClassName enumClass, Iterable<EnumValueDefinition> values) {
private static TypeSpec createEnum(ClassName enumClass, Iterable<EnumValueDefinition> values, boolean withUnknown) {
TypeSpec.Builder enumBuilder = TypeSpec.enumBuilder(enumClass.simpleName())
.addAnnotation(ConjureAnnotations.getConjureGeneratedAnnotation(EnumGenerator.class))
.addModifiers(Modifier.PUBLIC);
Expand All @@ -140,7 +133,19 @@ private static TypeSpec createEnum(ClassName enumClass, Iterable<EnumValueDefini
anonymousClassBuilder.addJavadoc("$L", StringUtils.appendIfMissing(docs.get(), "\n")));
enumBuilder.addEnumConstant(value.getValue(), anonymousClassBuilder.build());
}
return enumBuilder.addEnumConstant("UNKNOWN").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();
}

private static TypeSpec createVisitor(ClassName visitorClass, Iterable<EnumValueDefinition> values) {
Expand Down Expand Up @@ -208,28 +213,20 @@ private static MethodSpec createConstructor(ClassName enumClass) {
.build();
}

private static MethodSpec createValueOf(
ClassName thisClass,
Iterable<EnumValueDefinition> values,
Set<FeatureFlags> featureFlags) {
private static MethodSpec createValueOf(ClassName thisClass, Iterable<EnumValueDefinition> values) {
ParameterSpec param = ParameterSpec.builder(ClassName.get(String.class), "value").build();
CodeBlock.Builder parser = CodeBlock.builder();
if (featureFlags.contains(FeatureFlags.CaseInsensitiveEnums)) {
parser.addStatement("value = value.toUpperCase($T.ENGLISH)", Locale.class);
}
parser.beginControlFlow("switch ($N)", param);

CodeBlock.Builder parser = CodeBlock.builder()
.beginControlFlow("switch (upperCasedValue)");
for (EnumValueDefinition value : values) {
parser.add("case $S:\n", value.getValue())
.indent()
.addStatement("return $L", value.getValue())
.unindent();
}
parser.add("default:\n").indent();
if (!featureFlags.contains(FeatureFlags.CaseInsensitiveEnums)) {
// Only validate unknown values, matches are validated at build time.
parser.addStatement("$T.validate($N)", ConjureEnums.class, param);
}
parser.addStatement("return new $T(Value.UNKNOWN, $N)", thisClass, param)
parser.add("default:\n")
.indent()
.addStatement("return new $T(Value.UNKNOWN, upperCasedValue)", thisClass)
.unindent()
.endControlFlow();

Expand All @@ -239,6 +236,8 @@ private static MethodSpec createValueOf(
.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();
}
Expand Down
3 changes: 2 additions & 1 deletion conjure-java-core/src/main/java/com/palantir/conjure/java/types/ObjectGenerator.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -46,7 +46,8 @@ public Set<JavaFile> generateTypes(List<TypeDefinition> types) {
return UnionGenerator.generateUnionType(
typeMapper, typeDef.accept(TypeDefinitionVisitor.UNION));
} else if (typeDef.accept(TypeDefinitionVisitor.IS_ENUM)) {
return EnumGenerator.generateEnumType(typeDef.accept(TypeDefinitionVisitor.ENUM), featureFlags);
return EnumGenerator.generateEnumType(
typeDef.accept(TypeDefinitionVisitor.ENUM));
} else if (typeDef.accept(TypeDefinitionVisitor.IS_ALIAS)) {
return AliasGenerator.generateAliasType(
typeMapper, typeDef.accept(TypeDefinitionVisitor.ALIAS));
Expand Down
Loading

0 comments on commit ee749ae

Please sign in to comment.