Skip to content

Sproc fixes#28942

Merged
roji merged 2 commits intodotnet:release/7.0from
roji:MoreSprocBugs
Sep 1, 2022
Merged

Sproc fixes#28942
roji merged 2 commits intodotnet:release/7.0from
roji:MoreSprocBugs

Conversation

@roji
Copy link
Copy Markdown
Member

@roji roji commented Aug 31, 2022

Fixes to stored procedure update mapping

  • Don't attempt to propagate rows affected result columns.
  • Warn if an unexpected trailing result set is found.
  • Throw an informative message if a result set is missing.

Fixes #28803

* Don't attempt to propagate rows affected result columns.
* Warn if an unexpected trailing result set is found.
* Throw an informative message if a result set is missing.

Fixes dotnet#28803
@roji roji marked this pull request as ready for review September 1, 2022 11:11
@roji roji requested a review from AndriySvyryd September 1, 2022 11:12
}

Debug.Assert(onResultSet != true, "Unexpected result set found at end");
if (onResultSet == true)
Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Note that this is best-effort. Specifically, if there were no (expected) result sets, then a trailing one wouldn't be detected (since onResultSet is null here). We could add logic to check for that (calling reader.Read and NextResult), but that would potentially slow things down only for the purpose of this check.

if (onResultSet == false)
{
Check.DebugFail("Missing a result set");
throw new InvalidOperationException(RelationalStrings.MissingResultSetWhenSaving);
Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Dependencies.UpdateLogger.UnexpectedTrailingResultSetWhenSaving();
}

reader.Close();
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Wouldn't you need to consume all result sets before closing the reader to populate output parameters?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Add a test for this

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Wouldn't you need to consume all result sets before closing the reader to populate output parameters?

Not AFAIK - all output parameters should be made available after Close is called, regardless of whether NextResult was called before that (until it returned false). From the SqlClient docs:

The Close method populates the values for output parameters, return values and RecordsAffected on the SqlDataReader by consuming any pending results.

In any case, we've definitely consumed all known result sets - the only unconsumed ones here would be for resultsets the user hasn't configured in metadata, so I'm not sure what we'd do with output parameters from those commands...

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

In any case, we've definitely consumed all known result sets - the only unconsumed ones here would be for resultsets the user hasn't configured in metadata, so I'm not sure what we'd do with output parameters from those commands...

The extra result sets might be unrelated and safe to ignore

@roji roji merged commit d7d6d91 into dotnet:release/7.0 Sep 1, 2022
@roji roji deleted the MoreSprocBugs branch September 1, 2022 23:20
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.

Exception: "Unable to cast object of type" in many stored procs mapping cases

2 participants