Skip to content

Conversation

roimenashe
Copy link
Member

  • Add enum tests to both reactive and non-reactive flows.

+ Add enum tests to both reactive and non-reactive flows.
@roimenashe roimenashe added the enhancement New feature or request label Jun 9, 2021
@roimenashe roimenashe requested review from tfaulkes and reugn June 9, 2021 14:43
@roimenashe roimenashe self-assigned this Jun 9, 2021
@roimenashe roimenashe linked an issue Jun 9, 2021 that may be closed by this pull request
Copy link
Contributor

@tfaulkes tfaulkes left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey Roi. A couple of comments on this:

In the EnumMapper, your toAerospikeFormat does:

Field enumRequestedField = clazz.getDeclaredField(enumField);
enumRequestedField.setAccessible(true);
return enumRequestedField.get(value).toString();

and fromAerospikeFormat does the converse. This means that 3 introspection calls are done every time we convert the value. The general principle I've tried to adhere to is minimizing introspection calls, and the first 2 are able to be done once when we first see the class which we need to introspect. So I would be inclined to do these 2 in the constructor, but then have addToMap = false in TypeUtils.getMapper so that we don't cache these and hence let different classes uses different short strings. Then you can reduce the number of introspection calls in toAerospikeFormat/fromAerospikeFormat to 1 per call.

The other thought is: I like what you've done here, allowing a lot of flexibility. If there are 2 short names for an enum (eg 2-character country codes and 3-character country codes), the end user can pick and choose. But I would think in most cases the enum itself would know that it wants to use the short name. Can we give the user the option to mark the default Aerospike mapping on the Enum proper as well? So if there are 10 places the same enum is used, we don't have to remember to mark each of them as using the same short field.

@roimenashe roimenashe changed the title Map Enums by short value fields instead of Enum constant names. Map Enums by value fields instead of Enum constant names. Jun 10, 2021
@roimenashe
Copy link
Member Author

@tfaulkes
I committed a fix to avoid introspection calls each time we convert (instead I used the constructor as you suggested).

addToMap was already false since I wanted to avoid a situation where caching enum converters of the same enum type but with different @AerospikeEnum annotation field (points to another field) or 1 with annotation and 1 without, in this case it cached the EnumMapper of the first Enum field and use it for the rest of the same enum fields which was a bug (addToMap = false fixed it).

I think giving the user the option to set default enum field to a specific enum (for example, every Country enum usage will be converted to 2 char country code by default) is an advanced feature that I don't see much value in, since we don't want to cache these converters and the annotation is used at the field level, default is to store the constant name and if you want to store a specific field just specify it at the @AerospikeEnum annotation field. it is kind of similar situation as the @AerospikeExclude annotation, let say the user always wants to exclude an email field - it will have to specify @AerospikeExclude at each email field of any object.

@roimenashe roimenashe requested a review from tfaulkes June 10, 2021 07:53
@roimenashe roimenashe merged commit e202cc7 into main Jul 1, 2021
@roimenashe roimenashe deleted the enum-value-mapper branch July 1, 2021 14:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Map Enums by short value fields instead of Enum constant names.
3 participants