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

ID not returned for INSERT #206

Closed
acullather opened this issue Apr 17, 2014 · 3 comments
Closed

ID not returned for INSERT #206

acullather opened this issue Apr 17, 2014 · 3 comments

Comments

@acullather
Copy link

ID not returned for Insert because the SCOPE_IDENTITY is out of context of the INSERT

public virtual dynamic Insert(object o) {
    var ex = o.ToExpando();
    if (!IsValid(ex)) {
        throw new InvalidOperationException("Can't insert: " + String.Join("; ", Errors.ToArray()));
    }
    if (BeforeSave(ex)) {
        using (dynamic conn = OpenConnection()) {
            var cmd = CreateInsertCommand(ex);
            cmd.Connection = conn;
            cmd.ExecuteNonQuery();
            cmd.CommandText = "SELECT SCOPE_IDENTITY() as newID";
            ex.ID = cmd.ExecuteScalar();
            Inserted(ex);
        }
        return ex;
    } else {
        return null;
    }
}

I humbly suggest replacing

cmd.ExecuteNonQuery();
cmd.CommandText = "SELECT SCOPE_IDENTITY() as newID";

with

cmd.CommandText += "; SELECT SCOPE_IDENTITY()";

leaving the ExecuteScalar() statement to execute both the INSERT and the SCOPE_IDENTITY() statements. This combination statement puts the identity request within the context of the insert statement instead of a separate, non-contextual identity request.

@robconery
Copy link
Contributor

Thanks so much for the bug report! I need to be clear on a few things, however...

First - which DB are we talking about here? SQL Server? Next: the issue you're reporting is pretty serious: insert's aren't returning a new id across the board - if so that's a pretty big problem :). Is that what you're saying here?

If you had a moment to attach this to a PR (with some tests would be lovely!) then you could get credit for the fix :). If not - I'll have a poke when I can.

@acullather
Copy link
Author

Hi Rob,

First, thank you for your efforts in creating Massive. You've done an outstanding job and I should've started with that but I was in a hurry to post :)

So, to your questions:
I am using SQL Servier 2012 Dev Edition on a VM to test this with, so yes, the DB = SQL. Second part of the question: I haven't used Massive against any other database so I'm not sure it's across the board.

2nd Q: I'm not sure what a PR is. If you can clarify, I will do my best to work with you on whatever the PR is and, in the meantime, I will get some tests written which exemplify the issue.

@acullather
Copy link
Author

I posted a Pull Request (forgive my question, I'm a nub at Git) but it appears I've posted it as a separate issue. Sorry about that. The request is #209

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

No branches or pull requests

2 participants