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

Implement stored procedure update mapping #28553

Merged
2 commits merged into from
Aug 13, 2022
Merged

Conversation

roji
Copy link
Member

@roji roji commented Jul 31, 2022

Here's a WIP for some early feedback to make sure I'm in the right direction.

Some comments:

  • ResultSetMapping is now a flags enum, with a bit that specifies whether the command has output parameters (I've also redone the "positional mapping" optimization for SQL Server as another bit).
  • Rows affected is unimplemented (waiting for metadata), same with fancier mapping scenarios (need to know which table is being updated by the sproc).
  • Positional vs. named argument style
    • All databases support positional arguments to stored procedures, but not all support named (see comparison below); we should do positional by default.
    • Supporting named seems like it needs some metadata changes... StoredProcedureParameterBuilder has HasName (StoreStoredProcedureParameter extends IColumnBase which has Name). From a user perspective this looks like it should set the parameter name in the sproc call (e.g. EXEC @id = 3, @name = 'foo'). However, this is all non-nullable, so we have no way to make the named/positional distinction. Making IColumnBase.Name nullable seems like it would have a huge impact; another option would be to use some other property specific to StoreStoredProcedureParameter, but that would make Name dead for that type. We can also scope this out and say that named parameters aren't supported (and remove the HasName API for now).
  • Some assumptions in this implementation:
    • We call sprocs from regular commands (CommandType.Text with EXEC), even with output parameters. The alterative is to use CommandType.StoredProcedure, but these can't be batched - one call per command (until the new ADO.NET batching API is implemented). This works well for SQL Server, but I'm not 100% sure of the portability here.
    • We consume output parameters as we're traversing the DbCommand's result set. If some database only populates output parameters at the end (e.g. when the reader is disposed), this will fail.
  • No seeding support right now. I think this would require propagating the relational model through migrations - probably not trivial. Consider scoping out.
  • Sprocs don't have to return rows affected (but they can). This means that the concurrency check is optional - not possible without sprocs at the moment (see Allow opt-out of optimistic concurrency/rows affected check in the model #10443).

Some cross-database comparison:

Database Syntax Named/Positional Docs
SQL Server EXEC MySproc 'foo', 'bar' Both docs
PostgreSQL CALL MySproc('foo', 'bar') Both docs
MySQL CALL MySproc('foo', 'bar') Positional only docs
MariaDB CALL MySproc('foo', 'bar') Positional only docs
Oracle CALL MySproc('foo', 'bar') Positional only (in SQL) docs
SQLite No sproc support

Closes #245
Closes #28435

@roji roji requested a review from AndriySvyryd July 31, 2022 14:01
@roji
Copy link
Member Author

roji commented Aug 1, 2022

Crap, seems like on SQL Server output parameters aren't populated until NextResult is called. Will change the logic to take this into account.

@ajcvickers
Copy link
Contributor

@roji One for the book? 🤣

@roji
Copy link
Member Author

roji commented Aug 1, 2022

Oh I dunno, sprocs are such a quirky area that I'm hardly even surprised here...

@ajcvickers
Copy link
Contributor

Yeah, I don't suppose you have any lack of material. No need to find filler.

@roji
Copy link
Member Author

roji commented Aug 1, 2022

Pretty much 🤣

@ErikEJ
Copy link
Contributor

ErikEJ commented Aug 1, 2022

@roji works fine for me without next result. Will dig out a sample..

@roji
Copy link
Member Author

roji commented Aug 1, 2022

@ErikEJ this is specifically for a call that also returns a resultset (so both resultset and output parameter). If there's only an output parameter without a resultset, the previous NextResult already "went past" the row and processed its output parameter.

Here's a code sample:

Code sample
await using var conn = new SqlConnection("Server=localhost;Database=test;User=SA;Password=Abcd5678;Connect Timeout=60;ConnectRetryCount=0;Encrypt=false");
await conn.OpenAsync();

var cmd = conn.CreateCommand();
cmd.CommandText = @"
DROP TABLE IF EXISTS Blogs;
CREATE TABLE Blogs
(
    Id int IDENTITY PRIMARY KEY,
    Title VARCHAR(MAX)
)";
cmd.ExecuteNonQuery();

cmd.CommandText = @"
CREATE OR ALTER PROCEDURE Blogs_Insert(@id int OUT, @title varchar(max))
AS BEGIN
    INSERT INTO [Blogs] ([Title]) VALUES (@title);
    SELECT @id = SCOPE_IDENTITY();
    SELECT 8;
END;";
cmd.ExecuteNonQuery();

cmd.CommandText = "EXEC Blogs_Insert @Id OUTPUT, 'foo'";
var idParam = new SqlParameter("Id", SqlDbType.Int)
{
    Direction = ParameterDirection.Output
};
cmd.Parameters.Add(idParam);

cmd.Parameters.AddWithValue("title", "foo");
using var reader = cmd.ExecuteReader();
reader.Read();
Console.WriteLine("Result column: " + reader[0]);

Console.WriteLine("ID: " + idParam.Value);

@roji
Copy link
Member Author

roji commented Aug 1, 2022

Pushed a logic change to handle this, @AndriySvyryd this is probably a good time to give this a 1st review.

@roji
Copy link
Member Author

roji commented Aug 1, 2022

@ErikEJ note that I'm also supporting multiple sproc calls in a single command (batching), by concatenating multiple EXECs into the same CommandText. If you have any info/insights/possible pitfalls, they'd be very welcome 😬

@roji roji force-pushed the SprocUpdatePipeline branch from 29094d2 to 1735680 Compare August 2, 2022 07:06
@ErikEJ

This comment was marked as resolved.

@roji

This comment was marked as resolved.

@ErikEJ

This comment was marked as resolved.

@roji

This comment was marked as resolved.

@AndriySvyryd
Copy link
Member

Supporting named seems like it needs some metadata changes... StoredProcedureParameterBuilder has HasName (StoreStoredProcedureParameter extends IColumnBase which has Name). From a user perspective this looks like it should set the parameter name in the sproc call (e.g. EXEC @id = 3, @name = 'foo'). However, this is all non-nullable, so we have no way to make the named/positional distinction. Making IColumnBase.Name nullable seems like it would have a huge impact; another option would be to use some other property specific to StoreStoredProcedureParameter, but that would make Name dead for that type. We can also scope this out and say that named parameters aren't supported (and remove the HasName API for now).

We can use IColumnBase.Name, but use named argument style for all parameters or add a flag or some other configuration which would determine when a particular argument should be passed by name (tracked in #28439)

@AndriySvyryd
Copy link
Member

No seeding support right now. I think this would require propagating the relational model through migrations - probably not trivial. Consider scoping out.

We currently get IColumnMapping when available and pass through the IProperty.

name, originalValue: null, value, propertyMapping?.Property, columnType, typeMapping,

It shouldn't be too different for sprocs. If sprocs are to be used and the model is not available we should throw. But this can be done in a separate PR or post 7.0

@roji roji force-pushed the SprocUpdatePipeline branch from 1735680 to 9d706b5 Compare August 3, 2022 12:31
@roji

This comment was marked as resolved.

@roji

This comment was marked as resolved.

@ErikEJ

This comment was marked as resolved.

@roji

This comment was marked as resolved.

@roji roji force-pushed the SprocUpdatePipeline branch from 9d706b5 to 8c6658c Compare August 3, 2022 23:29
@roji roji changed the base branch from main to Issue28435 August 3, 2022 23:30

// For in/out parameters, both UseCurrentValueParameter and UseOriginalValueParameter are true, but we only want to add a single
// parameter. This will happen below.
if (columnModification.UseCurrentValueParameter && direction != ParameterDirection.InputOutput)
Copy link
Member

Choose a reason for hiding this comment

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

How is the DbParameter added for CurrentValue InputOutput parameters?

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's currently an assumption that if there's an InputOutput parameter, the in side is the original value and the out is the current value (e.g. store-generated concurrency token with original as condition and current as read). That's why we skip creating a current value parameter and only create the original one below (otherwise we'd get two parameters).

Do we have a scenario where that's not sufficient?

Copy link
Member

Choose a reason for hiding this comment

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

A column with a default value configured

Copy link
Member Author

Choose a reason for hiding this comment

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

How can an InputOutput parameter be used there? Either the property is set in EF and we just write it (no need to read it back), or it's not set in EF and we just read it (nothing to write), no?

Copy link
Member

Choose a reason for hiding this comment

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

For a parameter to be used for both reading and writing (even if not at the same time) it needs to be InputOutput in the metadata.

Copy link
Member Author

Choose a reason for hiding this comment

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

What's the incantation for that? :)

Copy link
Contributor

Choose a reason for hiding this comment

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

modelBuilder
    .Entity<Blog>()
    .Property(e => e.Title)
    .HasComputedColumnSql("'X'")
    .Metadata.SetAfterSaveBehavior(PropertySaveBehavior.Save);

Take a look at StoreGeneratedTestBase. For example, Computed_property_on_Modified_entity_is_included_in_update_when_modified.

Copy link
Contributor

Choose a reason for hiding this comment

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

However, it looks like the update pipeline doesn't generate code to read the computed value if it has been set:

      SET IMPLICIT_TRANSACTIONS OFF;
      SET NOCOUNT ON;
      UPDATE [Blogs] SET [Title] = @p0
      OUTPUT 1
      WHERE [Id] = @p1;

Copy link
Member Author

Choose a reason for hiding this comment

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

OK, so let's split out to make sure this works across the board?

Copy link
Member Author

Choose a reason for hiding this comment

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

Opened #28704

@roji roji force-pushed the SprocUpdatePipeline branch from 1e15aa4 to f3e95a9 Compare August 11, 2022 14:53
Copy link
Member Author

@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.

@AndriySvyryd pushed another batch of work, including support for original/current values for non-concurrency-tokens, inheritance tests and other stuff. There seem to be some minor stuff needed from the metadata side (see comments), but I think we should be close to done here.

BTW if you need to do metadata fixes, feel free to push directly into my branch here.

foreach (var mapping in mappings)
var foundMapping = false;

foreach (var tableMapping in entry.EntityType.GetTableMappings())
Copy link
Member

Choose a reason for hiding this comment

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

You also need to handle the case where there are no table mappings .ToTable((string?)null)

Copy link
Member Author

Choose a reason for hiding this comment

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

Can there be sproc mappings without table mappings? If so, I'll need a bit more info...

If you mean no table mappings or sproc mappings, isn't that what foundMapping does (and we throw down below, like we used to)?

Copy link
Member

Choose a reason for hiding this comment

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

You can map to sprocs without mapping to a table

Copy link
Member Author

Choose a reason for hiding this comment

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

So I'm again not sure how that works with the topological sort, where we depend on always having table (the original problem we discussed a while ago). Maybe we should jump on a call.

Copy link
Member

Choose a reason for hiding this comment

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

We discussed this already. In this case we'd need to do c-space topological sort as EF has no information on the constraints in the database, so you'd need to add back the logic removed in 246cc86#diff-d8e43ad90b6d31311a60648979a88137fac5880b21a502d6f42c5db73f5541fb
It's fine to do this in a separate PR

Copy link
Member Author

Choose a reason for hiding this comment

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

So this is about supporting the user configuring .ToTable((string?)null), and then mapping with sprocs? Is there a reason we need to support that rather than require a table mapping when using sprocs (am looking for a real-world scenario again) - I'd certainly hate to introduce all the c-space stuff just to support this if there isn't a good reason.

In any case, let's split out to another issue?

Copy link
Member

@AndriySvyryd AndriySvyryd Aug 12, 2022

Choose a reason for hiding this comment

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

The user can use views/TVFs + sprocs to completely abstract their schema from EF, I think this is a very powerful tool that can be used as an escape hatch for advanced mappings that we don't plan to support.

Copy link
Member Author

Choose a reason for hiding this comment

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

Sure, I agree - but we can still require them to specify a table for that, even if we never update (or query) it directly, as a place to hold the data we need for topological sort etc. It's a bit odd (virtual table never getting used), but it does solve the problem without extract complexity. Or am I missing something?

Copy link
Member Author

Choose a reason for hiding this comment

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

Opened #28703 to track this.

@julielerman
Copy link

Hey I don't want to bug you if this isn't ready for primetime yet but thought I'd check. I've got 7.0.0-rc.1.22410.9. Intellisense helps me build the Insert/Update/Delete mappings. Compiler is happy. But procedures are not being used. Just localdb on same machine as VS. It's enough for me to write my article knowing that it WILL work, though sorry I can't say "and here's the SQL ...look it's calling the sprocs!". However if this SHOULD work with this build, I can share my sln in separate issue for you to see if a) I'm doing something wrong (?) or b) I'm doing something differently (super simple class/dbcontext) . Thanks

@roji roji requested a review from AndriySvyryd August 12, 2022 15:41
@roji
Copy link
Member Author

roji commented Aug 12, 2022

@julielerman

It's enough for me to write my article knowing that it WILL work

Never trust us untrustworthy devs!

Unfortunately, AFAIK our CI doesn't produce daily builds of PRs which haven't been merged yet, so that version you're using only contains the metadata APIs (this PR does the actual implementation).

You can still build this PR yourself and run against that - it shouldn't be too hard. Or if you have a couple of days to wait, this should be merged and you'll have a daily build nupkg.

In the meantime, here's a console program which works for me when executed against the PR, it outputs:

EXEC [Blog_Insert] @p0, @p1 OUTPUT;
Code sample
await using var ctx = new BlogContext();
await ctx.Database.EnsureDeletedAsync();
await ctx.Database.EnsureCreatedAsync();

ctx.Database.ExecuteSqlInterpolated(
$"""
CREATE PROCEDURE Blog_Insert(@Name varchar(max), @Id int OUT)
AS BEGIN
    INSERT INTO [Blogs] ([Name]) VALUES (@Name);
    SET @Id = SCOPE_IDENTITY();
END;
""");

ctx.Database.ExecuteSqlInterpolated(
$"""
CREATE PROCEDURE Blog_Update(@Id int, @Name varchar(max))
AS UPDATE [Blogs] SET [Name] = @Name WHERE [Id] = @id;
""");

ctx.Database.ExecuteSqlInterpolated(
$"""
CREATE PROCEDURE Blog_Delete(@Id int)
AS DELETE FROM [Blogs] WHERE [Id] = @Id;
""");

var blog = new Blog { Name = "Foo" };
ctx.Blogs.Add(blog);
await ctx.SaveChangesAsync();

public class BlogContext : DbContext
{
    public DbSet<Blog> Blogs { get; set; }

    protected override void OnConfiguring(DbContextOptionsBuilder optionsBuilder)
        => optionsBuilder
            .UseSqlServer(@"Server=localhost;Database=test;User=SA;Password=Abcd5678;Connect Timeout=60;ConnectRetryCount=0;Encrypt=false")
            .LogTo(Console.WriteLine, LogLevel.Information)
            .EnableSensitiveDataLogging();

    protected override void OnModelCreating(ModelBuilder modelBuilder)
    {
        modelBuilder.Entity<Blog>()
            .InsertUsingStoredProcedure(
                "Blog_Insert",
                spb => spb
                    .HasParameter(w => w.Name)
                    .HasParameter(w => w.Id, pb => pb.IsOutput()))
            .UpdateUsingStoredProcedure(
                "Blog_Update",
                spb => spb
                    .HasParameter(w => w.Id)
                    .HasParameter(w => w.Name))
            .DeleteUsingStoredProcedure(
                "Blog_Delete",
                spb => spb.HasParameter(w => w.Id));
    }
}

public class Blog
{
    public int Id { get; set; }
    public string Name { get; set; }
}

@julielerman
Copy link

thanks @roji ...i will wait! HOpe this wasn't the absolutely wrong place to ask. Carry on! :)

@roji roji force-pushed the SprocUpdatePipeline branch from 53a717b to 58cca4f Compare August 12, 2022 21:30
@ghost
Copy link

ghost commented Aug 12, 2022

Hello @roji!

Because this pull request has the auto-merge label, I will be glad to assist with helping to merge this pull request once all check-in policies pass.

p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (@msftbot) and give me an instruction to get started! Learn more here.

@ghost
Copy link

ghost commented Aug 12, 2022

Apologies, while this PR appears ready to be merged, I've been configured to only merge when all checks have explicitly passed. The following integrations have not reported any progress on their checks and are blocking auto-merge:

  1. Azure Pipelines

These integrations are possibly never going to report a check, and unblocking auto-merge likely requires a human being to update my configuration to exempt these integrations from requiring a passing check.

Give feedback on this
From the bot dev team

We've tried to tune the bot such that it posts a comment like this only when auto-merge is blocked for exceptional, non-intuitive reasons. When the bot's auto-merge capability is properly configured, auto-merge should operate as you would intuitively expect and you should not see any spurious comments.

Please reach out to us at fabricbotservices@microsoft.com to provide feedback if you believe you're seeing this comment appear spuriously. Please note that we usually are unable to update your bot configuration on your team's behalf, but we're happy to help you identify your bot admin.

Closes dotnet#245
Closes dotnet#28435

Co-authored-by: Andriy Svyryd <AndriySvyryd@users.noreply.github.com>
@roji roji force-pushed the SprocUpdatePipeline branch from 58cca4f to a1478eb Compare August 13, 2022 07:43
@ghost
Copy link

ghost commented Aug 13, 2022

Apologies, while this PR appears ready to be merged, I've been configured to only merge when all checks have explicitly passed. The following integrations have not reported any progress on their checks and are blocking auto-merge:

  1. Azure Pipelines

These integrations are possibly never going to report a check, and unblocking auto-merge likely requires a human being to update my configuration to exempt these integrations from requiring a passing check.

Give feedback on this
From the bot dev team

We've tried to tune the bot such that it posts a comment like this only when auto-merge is blocked for exceptional, non-intuitive reasons. When the bot's auto-merge capability is properly configured, auto-merge should operate as you would intuitively expect and you should not see any spurious comments.

Please reach out to us at fabricbotservices@microsoft.com to provide feedback if you believe you're seeing this comment appear spuriously. Please note that we usually are unable to update your bot configuration on your team's behalf, but we're happy to help you identify your bot admin.

This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants