-
Notifications
You must be signed in to change notification settings - Fork 3.3k
[rc2] Fix Cosmos enum discriminator handling #36770
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
Conversation
…tionKeysExtractor Co-authored-by: AndriySvyryd <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@copilot Rewrite the PR summary using this template:
Fixes #{number}
**Description**
{Issue description}
**Customer impact**
{How does the reported issue affects customer's app? Are there workarounds?}
**How found**
{Was it customer reported or found during verification? How many customers are affected?}
**Regression**
{Is it a regression from a released version? Which one?}
**Testing**
{How the changes were tested}
**Risk**
{Low/Medium/High, argument why}
var convertedSpecifiedValue = discriminatorProperty.GetValueConverter()?.ConvertToProvider(specifiedDiscriminatorValue) | ||
?? specifiedDiscriminatorValue; | ||
|
||
if (discriminatorProperty.GetProviderValueComparer().Equals(convertedSpecifiedValue, entityDiscriminatorValue)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@copilot This isn't the right fix. You should just change GetProviderValueComparer()
to GetValueComparer())
and leave the rest of the code as it was
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You're absolutely right! I've simplified the fix to just change GetProviderValueComparer()
to GetValueComparer()
as suggested. This is much cleaner and achieves the same result by comparing CLR values directly instead of trying to convert and compare provider values. Fixed in commit 2f62f21.
…ad of GetProviderValueComparer Co-authored-by: AndriySvyryd <[email protected]>
... Updated the PR description using the requested template: Fixes #36329 Description Customer impact How found Regression Testing Risk |
src/EFCore.Cosmos/Query/Internal/CosmosReadItemAndPartitionKeysExtractor.cs
Show resolved
Hide resolved
…stead of GetProviderValueComparer Co-authored-by: AndriySvyryd <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
RC2
Fixes #36329
Fixes #34852
Description
When using an enum discriminator in the Cosmos DB provider, querying a DbSet for derived types would throw an
InvalidCastException
. This occurred because the discriminator comparison logic was usingGetProviderValueComparer()
which is meant to compare the provider representations of values, in this casestring
. The fix is to useGetValueComparer()
as that's the comparer for client-side values.Customer impact
Customers using enum discriminators in Cosmos DB cannot query derived types via DbSet. The workaround is to query through the base entity type instead, however if any property is modified the enum discriminator is saved as a number, resulting in data corruption.
How found
2 customer reports.
Regression
Yes, the query issue is a regression from EF 8, the SaveChanges issue is a regression from EF 7.
Testing
Test added.
Risk
Low. For properties without value converters (the vast majority)
GetValueComparer
andGetProviderValueComparer
return the same instance, so this change only affects the broken case.💡 You can make Copilot smarter by setting up custom instructions, customizing its development environment and configuring Model Context Protocol (MCP) servers. Learn more Copilot coding agent tips in the docs.