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

Async fix for "Generate values for PK properties that are also self-referencing FK properties" #29134

Merged
merged 1 commit into from
Sep 19, 2022

Conversation

ajcvickers
Copy link
Member

@ajcvickers ajcvickers commented Sep 18, 2022

See #22585, #22573, #26448

Async code pat was not fixed in original PR. However, this was not exposed until additional bug fix for #26448 in EF7 RC2

Description

Using the async overload of Add fails to generate a key value for TPT mappings where the primary keys of sub-tables are also foreign keys to the main table.

Customer impact

Exception when using AddAsync in many TPT scenarios.

How found

Found while writing samples for EF7.

Regression

Yes; worked in EF Core 6.0.

Testing

Added more use of AddAsync in tests.

Risk

Low; async code path was missing a check that existed in the sync code path.

…eferencing FK properties"

See #22585, #22573, #26448

Async code pat was not fixed in original PR. However, this was not exposed until additional bug fix for #26448 in EF7 RC2
@ajcvickers ajcvickers requested a review from a team September 18, 2022 21:11
@ajcvickers ajcvickers changed the base branch from main to release/7.0 September 18, 2022 21:12
@ajcvickers ajcvickers added this to the 7.0.0-rc2 milestone Sep 18, 2022
@ajcvickers
Copy link
Member Author

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@ajcvickers
Copy link
Member Author

ajcvickers commented Sep 19, 2022

@roji If you are around, could I get a review of this? It has been approved by Tactics for RC2.

@@ -59,9 +59,9 @@ public async Task Should_not_throw_if_specified_region_is_right()
using var context = new CustomerContext(options);
context.Database.EnsureCreated();

context.Add(customer);
await context.AddAsync(customer);
Copy link
Member

Choose a reason for hiding this comment

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

Does this remove the coverage from the sync path, i.e. should these tests be theories on async to test both paths?

Copy link
Member Author

Choose a reason for hiding this comment

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

There is in general a huge amount of coverage for the sync path. That being said, it would be good to be more aware of testing both going forward.

@ajcvickers ajcvickers merged commit b02da05 into release/7.0 Sep 19, 2022
@ajcvickers ajcvickers deleted the FullMagazine0918 branch September 19, 2022 09:31
ajcvickers added a commit that referenced this pull request Sep 19, 2022
@ajcvickers ajcvickers removed this from the 7.0.0-rc2 milestone Sep 19, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants