Skip to content
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

Fix to #31100 - Switch to storing enums as ints in JSON instead of strings #31317

Merged
merged 1 commit into from
Aug 10, 2023

Conversation

maumar
Copy link
Contributor

@maumar maumar commented Jul 20, 2023

Problem was that in 7.0 we stored enums inside JSON as strings by default (by applying EnumToStringConverter by convention), but in 8.0 we are changing this to int. This is a breaking change and it's extra problematic for databases that used EF JSON functionality in 7.0. This can easily create a scenario where there is a mix of string and int representation for an enum value within the same document. (some data was added in 7.0, and then some in 8.0 before customer realized that the breaking change has been made). To mitigate this we are adding a fallback mechanism when reading enum data that is part of JSON entity. We try to read as int and if that fails we try to read again as string. This way should minimize the disruption, moreover any data saved back to the database will be saved in the new format, so over time everything should normalize. We will still throw when projecting individual enum properties of a JSON entity (as opposed to the entire entity), because materialization for this goes through different path, indistinguishable from normal enum value read from column in relational table.

Fixes #31100

@maumar maumar requested review from ajcvickers and roji July 20, 2023 09:07
@maumar maumar force-pushed the enum_breaking_change branch from f2cda7f to cf27c6d Compare July 20, 2023 09:20
@maumar maumar force-pushed the enum_breaking_change branch 2 times, most recently from d374b81 to 48ae4b4 Compare July 20, 2023 17:41
@ajcvickers ajcvickers force-pushed the enum_breaking_change branch from 48ae4b4 to 75770ed Compare August 7, 2023 15:45
@ajcvickers ajcvickers changed the base branch from main to 230720Facetbook August 7, 2023 15:46
@ajcvickers ajcvickers requested a review from a team August 7, 2023 15:46
@ajcvickers
Copy link
Contributor

@AndriySvyryd @roji Took this over and updated to warn like we discussed.

@ajcvickers ajcvickers force-pushed the enum_breaking_change branch from 75770ed to 6638412 Compare August 7, 2023 19:50
"""
@p0='[{"Date":"2101-01-01T00:00:00","Enum":"Two","Enums":[0,-1,1],"Fraction":10.1,"NullableEnum":"One","NullableEnums":[null,-1,1],"OwnedCollectionLeaf":[{"SomethingSomething":"e1_r_c1_c1"},{"SomethingSomething":"e1_r_c1_c2"}],"OwnedReferenceLeaf":{"SomethingSomething":"e1_r_c1_r"}},{"Date":"2102-01-01T00:00:00","Enum":"Three","Enums":[0,-1,1],"Fraction":10.2,"NullableEnum":"Two","NullableEnums":[null,-1,1],"OwnedCollectionLeaf":[{"SomethingSomething":"e1_r_c2_c1"},{"SomethingSomething":"e1_r_c2_c2"}],"OwnedReferenceLeaf":{"SomethingSomething":"e1_r_c2_r"}},{"Date":"2010-10-10T00:00:00","Enum":"Three","Enums":null,"Fraction":42.42,"NullableEnum":null,"NullableEnums":null,"OwnedCollectionLeaf":[{"SomethingSomething":"ss1"},{"SomethingSomething":"ss2"}],"OwnedReferenceLeaf":{"SomethingSomething":"ss3"}}]' (Nullable = false) (Size = 808)
"""
@p0='[{"Date":"2101-01-01T00:00:00","Enum":1,"Enums":[0,-1,1],"Fraction":10.1,"NullableEnum":0,"NullableEnums":[null,-1,1],"OwnedCollectionLeaf":[{"SomethingSomething":"e1_r_c1_c1"},{"SomethingSomething":"e1_r_c1_c2"}],"OwnedReferenceLeaf":{"SomethingSomething":"e1_r_c1_r"}},{"Date":"2102-01-01T00:00:00","Enum":2,"Enums":[0,-1,1],"Fraction":10.2,"NullableEnum":1,"NullableEnums":[null,-1,1],"OwnedCollectionLeaf":[{"SomethingSomething":"e1_r_c2_c1"},{"SomethingSomething":"e1_r_c2_c2"}],"OwnedReferenceLeaf":{"SomethingSomething":"e1_r_c2_r"}},{"Date":"2010-10-10T00:00:00","Enum":2,"Enums":null,"Fraction":42.42,"NullableEnum":null,"NullableEnums":null,"OwnedCollectionLeaf":[{"SomethingSomething":"ss1"},{"SomethingSomething":"ss2"}],"OwnedReferenceLeaf":{"SomethingSomething":"ss3"}}]' (Nullable = false) (Size = 784)
Copy link
Member

Choose a reason for hiding this comment

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

Do we have an issue tracking setting this size to something that doesn't pollute the query cache?

Copy link
Contributor

Choose a reason for hiding this comment

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

@ajcvickers ajcvickers force-pushed the 230720Facetbook branch 2 times, most recently from 423bc0b to 9d0a4e1 Compare August 10, 2023 14:52
@ajcvickers ajcvickers force-pushed the enum_breaking_change branch from 6638412 to fbb0fab Compare August 10, 2023 15:16
Base automatically changed from 230720Facetbook to main August 10, 2023 15:54
…rings

Problem was that in 7.0 we stored enums inside JSON as strings by default (by applying EnumToStringConverter by convention), but in 8.0 we are changing this to int.
This is a breaking change and it's extra problematic for databases that used EF JSON functionality in 7.0. This can easily create a scenario where there is a mix of string and int representation for an enum value within the same document.
(some data was added in 7.0, and then some in 8.0 before customer realized that the breaking change has been made). To mitigate this we are adding a fallback mechanism when reading enum data that is part of JSON entity. We try to read as int and if that fails we try to read again as string. This way should minimize the disruption, moreover any data saved back to the database will be saved in the new format, so over time everything should normalize.
We will still throw when projecting individual enum properties of a JSON entity (as opposed to the entire entity), because materialization for this goes through different path, indistinguishable from normal enum value read from column in relational table.

Fixes #31100

Taking over the PR

Tweaks from review
@ajcvickers ajcvickers force-pushed the enum_breaking_change branch from fbb0fab to f07d70e Compare August 10, 2023 15:57
@ajcvickers ajcvickers merged commit 078c98f into main Aug 10, 2023
@ajcvickers ajcvickers deleted the enum_breaking_change branch August 10, 2023 18:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Switch to storing enums as ints in JSON instead of strings
3 participants