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

Sample for stored procedure mapping #4007

Merged
merged 3 commits into from
Sep 18, 2022
Merged

Sample for stored procedure mapping #4007

merged 3 commits into from
Sep 18, 2022

Conversation

ajcvickers
Copy link
Member

No docs yet.

Copy link
Member

@roji roji left a comment

Choose a reason for hiding this comment

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

@ajcvickers I found some mapping issues here which explain most of the errors in #2487 - take a look below (but this also uncovered issues in my code!). We can iterate on this together tomorrow if you want.

});

entityTypeBuilder.DeleteUsingStoredProcedure(
"Person_Delete",
Copy link
Member

Choose a reason for hiding this comment

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

This mapping - and many others - is missing a rows affected result column definition; the sproc definition does return a result set with it (OUTPUT 1). This means that result sets in the batch can get misaligned as EF thinks this sproc doesn't return anything but it does.

@ajcvickers
Copy link
Member Author

@roji Pushed some updates. Updating data is still failing for TPH. Optimistic concurrency test 1 is still failing in all cases.

@roji
Copy link
Member

roji commented Sep 2, 2022

Found two more bugs while investigating this:

  1. When updating, if a column hasn't changed (e.g. the discriminator), we don't create a parameter for it, and everything gets mismatched. This was raised by Andriy in Support sproc input/output parameters on non-concurrency-token properties efcore#28908 (comment), and I'm working on it there. It basically means that partial updates are currently broken.
  2. We don't properly detect concurrency exceptions when the rows affected is in the result set, and there are other result set values to propagate.
    a. In regular SQL, when there's nothing to propagate, we expect a single value with 1 or 0 (from OUTPUT 1 SQL).
    b. But when there's something to propagate, we interpret the existence of a row as a sign that the update succeeded.
    c. While this works for regular SQL, with sprocs we always get back a row. So we need to add logic to check for 1 or 0 even when there's propagation.

    This was actually #28997.

I'll be back to full work on Tuesday (will be partially doing stuff on Monday too) and will concentrate on fixing all this (not feeling great about the work I've done here...)

@roji
Copy link
Member

roji commented Sep 12, 2022

@ajcvickers just making sure that everything works for you and you're not blocked on me by anything.

@ajcvickers ajcvickers merged commit 439d3fa into main Sep 18, 2022
@ajcvickers ajcvickers deleted the Sprocenfest0831 branch September 18, 2022 12:38
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.

2 participants