-
Notifications
You must be signed in to change notification settings - Fork 1.2k
[FEATURE] Elastic 8 Client - Stores Enums as Strings #8194
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Comments
Hi @DR9885, most of the times you want enums to be stored as As we are using public class JsonOrdinalEnumConverter :
JsonConverter<Enum>
{
public override Enum? Read(ref Utf8JsonReader reader, Type typeToConvert, JsonSerializerOptions options) =>
(Enum)Enum.ToObject(typeToConvert, reader.GetUInt64());
public override void Write(Utf8JsonWriter writer, Enum value, JsonSerializerOptions options) =>
writer.WriteNumberValue(Convert.ToUInt64(value));
} It's important to use the converter attribute on the property like this: public class Movie
{
[JsonConverter(typeof(JsonOrdinalEnumConverter))]
public Rating Rating { get; init; }
} and not on the type (sadly, the default serializer always takes precedence): [JsonConverter(typeof(JsonOrdinalEnumConverter))]
public enum Rating
{
...
} Does that solve your issue? |
Good to know, we can leverage the System.Text features. However, these changes have other impacts:
Example of some Enum Values we use, that would be too wordy to store as strings.
I think it would make more sense to have compiled languages like C# and Java store it as an integer. And javascript store it as a string. |
The new default behavior was decided before I joined the team and there probably are as well good arguments in favor of this change. Maybe @Mpdreamz @stevejgordon can remember these?
Range queries on enum fields is a nieche use-case and probably even indicates a wrong type. Enums are not intended to cover a continuous range of values.
Your point is completely valid. This is a breaking change and could mess with existing data, if not carefully handled (like e.g. using the workaround I mentioned above). I have to check, if this point is part of the migration guide.. The other arguments do all boil down to some advances/disadvantages of the underlaying data type. I still think the current default is good as it minimizes surprises (especially for rather inexperienced developers or new users who don't think about the underlaying types). For the more advances users there still is the option to customize this behavior like using a custom converter like mentioned above. Maybe I could make the default configurable to make it a little bit easier to switch back to ordinal values. |
I remember we did this because we got a lot of bug reports where this caught people off guard. For sure With https://github.com/elastic/elastic-ingest-dotnet we don't default to writing enums as strings because The one argument for sending them as integers is to allow people to rename enums values provided they maintain the enum ordering. We should align https://github.com/elastic/elastic-ingest-dotnet with the client behaviour though. |
@Mpdreamz there are also performance benefits to using int over string.
I have a few objects where we save with over 100 different enums in each object, with batches of 10k models. The change to will impact performance heavily. Also updating each and every field that uses an enum would take a lot of developer time. Can we just make this configurable by the framework as a whole? |
Is your feature request related to a problem? Please describe.
Before enums were stored as integers, which allowed us to do range queries.
Ex: we have a Rating Enum, and sometimes we like to search above a certain rating.
Describe the solution you'd like
Options:
The text was updated successfully, but these errors were encountered: