Use Enum.name() in EnumAttributeConverter instead of Enum.toString()

[rkennedy-mode]

Describe the bug

EnumAttributeConverter uses Enum.toString() to build an identity map of type String -> the corresponding enum value: https://github.com/aws/aws-sdk-java-v2/blob/master/services-custom/dynamodb-enhanced/src/main/java/software/amazon/awssdk/enhanced/dynamodb/internal/converter/attribute/EnumAttributeConverter.java#L49

Enum.toString(), however, is not final. It can be overloaded by implementation classes. This makes it inherently unsafe to use when serializing an Enum into a DynamoDB attribute. If the toString() implementation of an Enum were to change after having been used previously to write values to DynamoDB, it's likely that future versions of the code wouldn't be able to deserialize values from DynamoDB because the strings would no longer match.

Enum.name(), however, is final. It can't be changed and the value returned by it can be fed to Enum.valueOf(String name) to turn the name back into the corresponding Enum value. As a result, this is a far safer mechanism to use.

Expected Behavior

I expect Java enums written to DynamoDB via EnumAttributeConverter to use the string returned via name() instead of the string returned via toString().

Current Behavior

EnumAttributeConverter uses the string returned via toString().

Reproduction Steps

None needed. Just look at the code.

Possible Solution

No response

Additional Information/Context

In addition, changing to name() and valueOf(String name) would allow the enumValueMap class field (https://github.com/aws/aws-sdk-java-v2/blob/master/services-custom/dynamodb-enhanced/src/main/java/software/amazon/awssdk/enhanced/dynamodb/internal/converter/attribute/EnumAttributeConverter.java#L42) to be removed as well as the corresponding code that initializes the map (https://github.com/aws/aws-sdk-java-v2/blob/master/services-custom/dynamodb-enhanced/src/main/java/software/amazon/awssdk/enhanced/dynamodb/internal/converter/attribute/EnumAttributeConverter.java#L47-L51).

AWS Java SDK version used

dynamodb-enhanced-2.20.51

JDK version used

Any

Operating System and version

Any

[martinKindall]

I agree with your reasoning. I took a look at the Enum docs here, regarding Enum::name() https://docs.oracle.com/javase/8/docs/api/java/lang/Enum.html#name-- and the following makes sense in this context:

This method is designed primarily for use in specialized situations where correctness depends on getting the exact name, which will not vary from release to release.

Which is just what we need here. I will push a PR with the necessary changes.

[rkennedy-mode]

Thanks!

I will add one caveat, which is that if anyone has been using this to write enum data to DynamoDB with an enum that has a customized toString() implementation, changing to name() may break their application in such a way that they can no longer deserialize their records from DynamoDB.

That won't affect me (I'm not using this class just yet and I don't overload toString() in any of my enums). But it could affect others using the SDK.

You could potentially add some backward compatibility code that uses name() when writing new values and attempts to use name() when reading values, falling back to toString() if the value in DynamoDB doesn't match any name(). It adds complexity to the code, but maybe provides a saving throw for anyone that might have fallen into the gap described above.

[martinKindall]

I was already giving a though to this, because changing enumConstant.toString() straight to enumConstant.name() is a breaking change indeed.

I would leave the default behaviour as it is, and add an optional way to create the EnumAttributeConverter based on names.

[martinKindall]

I noticed there's another bigger obstacle though, the .toString() method is also used here, which is part of an already defined interface. I could easily change the implementation, but that is a breaking change. And extending that interface to allow an optional configuration would be too messy imo.

I think this change can be introduced as a breaking change with proper documentation and versioning.

[martinKindall]

Finally I found a way to keep backward compatibility and also make it possible to use Enum::name instead of Enum::toString for those who need it. Changes are here #3971

[debora-ito]

We'll take a look at the PR.

Changing this to feature request, feels more like an enhancement than a bug.

[rkennedy-mode]

Changing this to feature request, feels more like an enhancement than a bug.

Anyone who creates and later changes the implementation of toString() on their enum(s) will disagree. But that's probably infrequent enough that I suspect you won't hear from them.

Unless, of course, if a future version of the Java language specification or JVM implementation changes what the default toString() implementation for enums returns.

[debora-ito]

Change released in SDK version 2.20.87 via #3971.

[github-actions]

⚠️COMMENT VISIBILITY WARNING⚠️

Comments on closed issues are hard for our team to see.
If you need more assistance, please open a new issue that references this one.
If you wish to keep having a conversation with other community members under this issue feel free to do so.