[JAVA] Enum mapping make wrong value as null instead of throwing an exception

[TehBakker]

Description

Short : If I've a value that does not match my enum, the JsonCreator of the enum make it null, where I would want it to throw a validation exception.
This is following #1919 changes

Swagger-codegen version

2.2.3-SNAPSHOT

Swagger declaration file content or url

 MyBean:
      type: object
      required:
        - enumList
      description: Enum list attribute
      properties:
        enumList:
          type: array
          minItems: 1
          items:
            $ref: '#/definitions/MyEnum'

 MyEnum:
      type: string
      description: blabla my enum
      enum:
        - VALUE1
        - VALUE2

The generated code is

public enum MyEnum {
  
  VALUE1("VALUE1"),
  
  VALUE2("VALUE1");

  private String value;

  MyEnum(String value) {
    this.value = value;
  }

  @Override
  @JsonValue
  public String toString() {
    return String.valueOf(value);
  }

  @JsonCreator
  public static MyEnum fromValue(String text) {
    for (MyEnum b : MyEnum.values()) {
      if (String.valueOf(b.value).equals(text)) {
        return b;
      }
    }
    return null;
  }
}

Related issues

#1919

Suggest a Fix

What I would like is to be able to say if in case of unknown value, the @JsonCreator should return null like today or throw an exception.
Since my Enum is in a list attribut, I cannot just use @NotNull (and the exception message wouldn't be correct with what is really going on anyway).

Also is there a way to tell a list attribut that it should not contains any null value ?
Such as Jackson READ_UNKNOWN_ENUM_VALUES_AS_NULL property allow to declare ?

Please let me know if I'm not clear enough.

Regards

[wing328]

@TehBakker so basically you want to throw an exception instead of returning null, right? I think it's reasonable and to achieve that without breaking backward-compatibility, the only way seems to be adding another option (e.g. invalidEnumThrowsException)

cc @cbornet

[wing328]

cc @ePaul as well

[TehBakker]

Yes that's exactly that, an extra option would be good yeah.
Although I feel that should have been the default behavior maybe ?

[cbornet]

I think you should be able to configure Jackson READ_UNKNOWN_ENUM_VALUES_AS_NULL from the client without the need to add another option.

[TehBakker]

@cbornet I thought so too, I might have messed up, but I tried setting spring.jackson.deserialization.READ_UNKNOWN_ENUM_VALUES_AS_NULL=false in my application.properties and it didn't seems to have effect.

I think it's because the @JsonCreator generated take precedant over Jackson conf no ?

@JsonCreator
  public static MyEnum fromValue(String text) {
    for (MyEnum b : MyEnum.values()) {
      if (String.valueOf(b.value).equals(text)) {
        return b;
      }
    }
    return null;
  }

[cbornet]

I think the default here should be to throw an exception. Also I'm not sure why we need to use JsonCreator here whereas I think Jackson could handle enum mapping directly using READ_ENUMS_USING_TO_STRING and WRITE_ENUMS_USING_TO_STRING.

[cbornet]

I've checked and it seems all jackson-based java clients set READ_ENUMS_USING_TO_STRING and WRITE_ENUMS_USING_TO_STRING. So I don't understand why we also have @JsonValue/@JsonCreator...
@TehBakker can you check what it gives if you remove the annotations ?

[TehBakker]

Sure, as I said in the original post, it's tied to that ticket #1919
It was added to handle it case insensitively, might make sense sometime but not in all case and returning null is just wrong imo.
If I remove the @JsonCreator it works as expected and throws an InvalidFormatException:

Caused by: com.fasterxml.jackson.databind.exc.InvalidFormatException: Can not deserialize value of type com.mypackage.MyEnum from String "test": value not one of declared Enum instance names: [VALUE1, VALUE2, VALUE2]

[cbornet]

@wing328 do you know if we can remove the annotations and the fromValue() method or is there a good reason not to do it ?

[TehBakker]

Any news on the fix making it into the next release ?

[wing328]

do you know if we can remove the annotations and the fromValue() method or is there a good reason not to do it ?

Seems like it's related to enum marshalling: #4059

@TehBakker what about using the okhttp-gson or retrofit2 Java client instead? You can also use customized templates with the -t option.

[cbornet]

@wing328 actually, I think we can keep the fromValue method. But I would prefer configuring Jackson and removing @JsonCreator

[cbornet]

@TehBakker did you test removing the annotations ?

[stevecookform3]

I've also raised #6286 which is intended to add an option in the other direction (ie. make acceptance of enums broader, by generating string constants rather than enums).

I think these two options could complement each other, so codegen users would have a choice of either

• Enum throwing exceptions (for strongly typed APIs), or
• String capturing data (for permissive apis).

@wing328 - thoughts?

[cbornet]

Revising what I wrote here . I think enums should return null on unknown values as per Postel's law. I'm not sure I see the interest of returning a String : what would you do with it ?

[kevinmic]

The reason why a String would be useful instead of an enum is for handling future unknown values. If I generate a client based on a given swagger definition with enums, and then the owner of the api changes the enum by adding more values to that enum set, my client will not gracefully handle these values.

So for example, I GET an object from their api, make a few changes and then PUT it back. If they add a new value to the enum, when I GET their object I will lose the value of that enum field, then when I PUT it back I will attempt to update that value to null. I have no other options other than to quickly regenerate my client every time they add a single enum value. I can't even code around the problem. Allowing me to generate Strings instead of java enums would allow me a way to code around the problem.

[TehBakker]

Well using an ENUM means you think the values will be fixed.
And will only change on "major" version.
If you think the values might change then String should be used imo.

Right now I end up with a null value in my code, unable to know if it was a null or a wrong value inputed.
So might as well use String as enum won't do validation and I lose information.

[cbornet]

@TehBakker agreed. As per JSON schema spec, enum is a validation keyword and should be used as such

[kevinmic]

If you think the values might change then String should be used imo.

With the way swagger codegen currently works I can't make that choice. I am forced to use enums if the api specifies enums.

As per JSON schema spec, enum is a validation keyword and should be used as such

@cbornet That is great from an API Generation stand point. It communicates valid values for this field. It may not be so great as the consumer of an api though. I may not care what the field contains. I may have to code around mistakes made by the owner of the api, but with enum validation I am unable to code around those mistakes.

So right now if an api specifies "enum" then I have two options. 1. My java client will contain enums, or 2. Preprocess the swagger api file and strip out all enum lists so that codegen will generate the client the way I prefer to have it (with Strings instead of enums).

It would be nice if swagger codegen would help me with option 2 instead of fighting me on it.

[TehBakker]

I understand your point if view Kevin, however I'm on the other side, as API server and I can't benefit from validation offered by the Enum keyword and have to add manual validation to see if the enum was null or a wrong value.
Kinda same issue as you describe on the server side.

As someone designing my server, I'm using enum because it's not supposed to change. If I was to add other value, I would make sure to communicate to people using the API or make another version of my API.

However best of both world would be to leave an option to use String instead of enum for client who, like you said, don't even care for the value.

[kevinmic]

Sorry @TehBakker for attempting to hijack your original issue. I followed #6286 to here and posted on both. The problems seem related, we want to control how the client handles enums. Whether it be more validation or less.

[stevecookform3]

@TehBakker

as API server and I can't benefit from validation offered by the Enum keyword

The proposal in #6286 would still use the enum keyword. If you use swagger codegen to build your server, many languages (including Java/spring, which I use) will automatically generate the validation code on the server for you, so that shouldn't be an issue. (And if validation is not generated, you shouldn't be relying on client validation for your API, or you risk exposing issues in your API to clients that dont choose to use swagger to generate their API calls).

The only difference would be there would be a client generation option which would interpret enums as string fields with a list of constants, rather than strictly enforcing an enum.

[TehBakker]

Hum ?

I'm not relying on client validation.
I'm on the server side and getting a null value for an enum field right now means I do not know if the user inputed a wrong value or if he did not input any value.

Obviously on server side if the client sent a wrong value I want to throw a validation error.
And this would be handled by the deserialization process in most framework.

[stevecookform3]

Ah ok. I understand. I think then a good option would be:

1. For server side, enum with UNKNOWN (as you suggest)
2. For client side, either enum or optionally generation of string/constants as proposed in [Java] Option to generate Enum values as public static final constants? #6286

[TehBakker]

I'm not suggesting UNKNOWN (although that would be better than what I've now), since that would force me to not have UNKNOWN value in my Enum (which in fact I already have in some of them).

I'm hoping to have my server, or at least an option to do, behave like the default Enum validation pattern, throwing a validation exception if the value does not match a value of my Enum during the derserialization process (like javax.validation annotations)

[cbornet]

I tend to agree with Zalando's guidelines on the subject : http://zalando.github.io/restful-api-guidelines/#112 http://zalando.github.io/restful-api-guidelines/#125 .
Enum should only be used for closed enumerations (eg. "MONDAY", "TUESDAY", ...). Now I also understand that all swagger spec publishers don't respect this rule...

[yinan-liu]

any news on this one, will it be fixed or not please ?

[slarti-b]

I am using it on the server, and agree with @TehBakker I think it should throw a validation error. I can see the possible desire to be more relaxed when generating a client, but if we make it a config option you have that. I would say that I think the default should be to be strict, both because that's what the swagger spec says (en7ums are validation) and since this is at one level a regression introduced by #1919. At least, as I read this discussion it would have been a validation error if just left to jackson, but the introduction of @JsonCreator broke the validation.

I'd be happy to submit a PR if there is some general agreement on what the goal is.

I think #6286 is a separate issue, although I have an opinion there too, but have commented on that thread.

[batwad]

Any word on this being fixed? I've just wasted two hours figuring out why READ_UNKNOWN_ENUM_VALUES_AS_NULL isn't working and it turns out it's because of the @JsonCreator annotation in the generated code.

[slarti-b]

I had said i'd try and fix it, but we're now evaluating moving to the OpenAPI tools for code generation, and it's already fixed there, so I'm not sure if I will or not. The fix there is fromhttps://github.com/OpenAPITools/openapi-generator/issues/625 so should be relatively easy to port I guess

[batwad]

I may have to do the same; I wasn't aware of the fork and progress on this tool seems to have died since.

[vlavasa]

Hallo,
will it be repaired by the end of the universe?

[lyubomyr-shaydariv]

I'm really interested in bringing this to the life either, before the universe and the project I work on all end.

I'm currently working on migrating a legacy project to use the codegen and the current behavior returning null, instead of throwing a validation exception, makes some bad things happen. E.g., If I have a day of weak enum (say, trivial MON...SUN), how should I recognize whether the API user is passing a null or they are suggesting a new day of week? Now it results in a 500 Internal Server Error, because the previous implementation didn't use the codegen and was NPE-resistant, and because I cannot patch every single piece of code to double check whether it's null to throw as a 400 Bad Request (and that's not always possible at that level). Now I'm going to tell my team: "hey, when we migrate to the codegen, we lose the stability for some cases, you know"...

Any chances to have it fixed in the v2 soon? Would be greatly appreciated.

[lyubomyr-shaydariv]

For those who can't wait and those who want outrace the day the Universe ends, here is a Jackson module to override the problematic use case:

public final class SwaggerCodegenQuirksModule
		extends SimpleModule {

	private final Predicate<? super Class<?>> fixQuirks;

	private SwaggerCodegenQuirksModule(final Predicate<? super Class<?>> fixQuirks) {
		this.fixQuirks = fixQuirks;
	}

	public static Module create(final Predicate<? super Class<?>> fixQuirks) {
		return new SwaggerCodegenQuirksModule(fixQuirks);
	}

	@Override
	public void setupModule(final SetupContext setupContext) {
		setupContext.addDeserializers(DeserializerRegistry.create(fixQuirks));
	}

}

final class DeserializerRegistry
		extends SimpleDeserializers {

	private final Predicate<? super Class<?>> fixQuirks;

	private DeserializerRegistry(final Predicate<? super Class<?>> fixQuirks) {
		this.fixQuirks = fixQuirks;
	}

	static Deserializers create(final Predicate<? super Class<?>> fixQuirks) {
		return new DeserializerRegistry(fixQuirks);
	}

	@Override
	@Nullable
	public JsonDeserializer<?> findEnumDeserializer(final Class<?> enumClass, final DeserializationConfig config, final BeanDescription description) {
		if ( !fixQuirks.test(enumClass) ) {
			return null;
		}
		@SuppressWarnings("unchecked")
		final Class<Enum<?>> castEnumClass = (Class<Enum<?>>) enumClass;
		return EnumDeserializer.create(castEnumClass);
	}

}

final class EnumDeserializer<E extends Enum<?>>
		extends StdDeserializer<E> {

	private final Map<? super String, ? extends E> enumIndex;

	private EnumDeserializer(final Class<E> enumClass, final Map<? super String, ? extends E> enumIndex) {
		super(enumClass);
		this.enumIndex = enumIndex;
	}

	static <E extends Enum<?>> JsonDeserializer<E> create(final Class<E> enumClass) {
		try {
			final Field valueField = enumClass.getDeclaredField("value");
			valueField.setAccessible(true);
			final Map<String, E> enumIndex = createEnumIndex(enumClass, valueField);
			return new EnumDeserializer<>(enumClass, enumIndex);
		} catch ( final NoSuchFieldException ex ) {
			throw new RuntimeException(ex);
		}
	}

	@Override
	@Nullable
	public E deserialize(final JsonParser parser, final DeserializationContext context)
			throws IOException {
		final String value = parser.getValueAsString();
		if ( value == null ) {
			return null;
		}
		@Nullable
		final E enumValue = enumIndex.get(value);
		if ( enumValue == null ) {
			throw new JsonMappingException(parser, value + " does not match any of " + enumIndex.keySet());
		}
		return enumValue;
	}

	private static <E extends Enum<?>> Map<String, E> createEnumIndex(final Class<E> enumClass, final Field valueField) {
		return Stream.of(enumClass.getEnumConstants())
				// can be optimized returning specialized maps
				.collect(Collectors.toMap(
						e -> {
							try {
								return (String) valueField.get(e);
							} catch ( final IllegalAccessException ex ) {
								throw new AssertionError(ex);
							}
						},
						Function.identity()
				));
	}

}

Example of use:

public final class SwaggerCodegenQuirksModuleTest {

	private enum NonGeneratedDayOfWeek {

		MON, TUE, WED, THU, FRI, SAT, SUN

	}

	// moved from the generated class and cleaned up for testing purposes
	@Validated
	@javax.annotation.Generated(value = "io.swagger.codegen.languages.SpringCodegen", date = "2020-06-26T13:14:37.578+03:00")
	public static final class Generated {

		public enum DayOfWeek {

			MON("@@Monday@@"), // of course, no @@ in the production code, just for testing
			TUE("@@Tuesday@@"),
			WED("@@Wednesday@@"),
			THU("@@Thursday@@"),
			FRI("@@Friday@@"),
			SAT("@@Saturday@@"),
			SUN("@@Sunday@@");

			private String value;

			DayOfWeek(String value) {
				this.value = value;
			}

			@JsonCreator
			public static DayOfWeek fromValue(String text) {
				for ( DayOfWeek b : DayOfWeek.values() ) {
					if ( String.valueOf(b.value).equals(text) ) {
						return b;
					}
				}
				return null;
			}
		}

	}

	private static final ObjectMapper defaultObjectMapper = new ObjectMapper();

	private static final ObjectMapper quirksSupportObjectMapper = new ObjectMapper()
			.registerModule(SwaggerCodegenQuirksModule.create(clazz -> {
				// simple check for tests only, I prefer to check for the package
				return clazz == Generated.DayOfWeek.class;
			}));

	private static Stream<Arguments> testEnumForEquality() {
		return Stream.of(
				Arguments.of(defaultObjectMapper, Generated.DayOfWeek.class, "\"@@Monday@@\"", Generated.DayOfWeek.MON),
				Arguments.of(defaultObjectMapper, NonGeneratedDayOfWeek.class, "\"SUN\"", NonGeneratedDayOfWeek.SUN),
				Arguments.of(quirksSupportObjectMapper, Generated.DayOfWeek.class, "\"@@Tuesday@@\"", Generated.DayOfWeek.TUE),
				Arguments.of(quirksSupportObjectMapper, NonGeneratedDayOfWeek.class, "\"WED\"", NonGeneratedDayOfWeek.WED)
		);
	}

	@ParameterizedTest
	@MethodSource
	public <E extends Enum<?>> void testEnumForEquality(final ObjectMapper objectMapper, final Class<E> enumClass, final String json,
			final E expectedEnumValue)
			throws IOException {
		Assertions.assertSame(expectedEnumValue, objectMapper.readValue(json, enumClass));
	}

	private static Stream<Arguments> testEnumForExpectedNull() {
		return Stream.of(
				Arguments.of(defaultObjectMapper, Generated.DayOfWeek.class, "null"),
				Arguments.of(defaultObjectMapper, NonGeneratedDayOfWeek.class, "null"),
				Arguments.of(quirksSupportObjectMapper, Generated.DayOfWeek.class, "null"),
				Arguments.of(quirksSupportObjectMapper, NonGeneratedDayOfWeek.class, "null")
		);
	}

	@ParameterizedTest
	@MethodSource
	public <E extends Enum<?>> void testEnumForExpectedNull(final ObjectMapper objectMapper, final Class<E> enumClass, final String json)
			throws IOException {
		Assertions.assertNull(objectMapper.readValue(json, enumClass));
	}

	private static Stream<Arguments> testEnumForExpectedErrorOnIllegalValue() {
		return Stream.of(
				Arguments.of(defaultObjectMapper, NonGeneratedDayOfWeek.class, "\"w.h.a.t.e.v.e.r\""),
				Arguments.of(quirksSupportObjectMapper, Generated.DayOfWeek.class, "\"w.h.a.t.e.v.e.r\""),
				Arguments.of(quirksSupportObjectMapper, NonGeneratedDayOfWeek.class, "\"w.h.a.t.e.v.e.r\"")
		);
	}

	@ParameterizedTest
	@MethodSource
	public <E extends Enum<?>> void testEnumForExpectedErrorOnIllegalValue(final ObjectMapper objectMapper, final Class<E> enumClass, final String json) {
		Assertions.assertThrows(JsonMappingException.class, () -> objectMapper.readValue(json, enumClass));
	}

	private static Stream<Arguments> testEnumForUnexpectedNullOnIllegalValue() {
		return Stream.of(
				Arguments.of(defaultObjectMapper, Generated.DayOfWeek.class, "\"w.h.a.t.e.v.e.r\"")
		);
	}

	@ParameterizedTest
	@MethodSource
	public <E extends Enum<?>> void testEnumForUnexpectedNullOnIllegalValue(final ObjectMapper objectMapper, final Class<E> enumClass, final String json)
			throws IOException {
		Assertions.assertNull(objectMapper.readValue(json, enumClass));
	}

}

Hope it helps.