Acquiring an ID from a newly inserted entry in an MSSQL database

Question

In an MSSQL database, I have automatically increasing IDs for every entry. Is it safe to acquire the ID of the newly inserted entry like this? I am worried that, if at the exact same time another entry gets inserted, I could end up with the wrong ID.

string query = "INSERT INTO subscriptions (polygon,...) VALUES (@polygon,...)";
                    SqlDataAdapter cmd = new SqlDataAdapter();  
                    using (SqlCommand querySaveSubscription = new SqlCommand(query))
                    {
                        querySaveSubscription.Connection = openCon;
                        // Create Poly out of given geodata values
                        var point = SqlGeography.Point(Double.Parse(lat), Double.Parse(lon), 4326);
                        SqlGeography poly = point.BufferWithTolerance(Double.Parse(radius) * 1000, 0.01, true);



                        var param = new SqlParameter(@"polygon", poly);
                        param.UdtTypeName = "Geography";

                        querySaveSubscription.Parameters.Add(param);
        ...



                        openCon.Open();
                        querySaveSubscription.ExecuteNonQuery();

I have the following c# code directly after my INSERT INTO - query getting executed:

                    query = "SELECT ID FROM subscriptions ORDER BY ID DESC";
                    SqlCommand command = new SqlCommand(query, openCon);
                    SqlDataReader reader = command.ExecuteReader();
                    reader.Read();

                    int id = reader.GetInt32(0);

                    result.Add("success", "true");

                    result.Add("id", id.ToString());

Answer 1

You can use scope_identity as @RobH hinted in a comment, but the sexiest way IMO is to use the OUTPUT clause of the INSERT syntax:

DECLARE @result TABLE (Id INT NOT NULL);

INSERT INTO subscriptions (polygon,...) 
VALUES (@polygon,...)
OUTPUT INSERTED.Id INTO @result;

SELECT Id FROM @result;

Now, having a whole multi-statement CommandText like this, even if it's parameterized, isn't desirable at all, and makes for pretty ugly C# code - why not put that T-SQL in a stored procedure, and call that instead?

Speaking of parameters, your code snippet, as small as it is, is showing us that your method is breaking SRP (Single Responsibility Principle):

   // Create Poly out of given geodata values
    var point = SqlGeography.Point(Double.Parse(lat), Double.Parse(lon), 4326);
    SqlGeography poly = point.BufferWithTolerance(Double.Parse(radius) * 1000, 0.01, true);

That logic doesn't belong there at all: you need a SqlGeography-type parameter, fine. That doesn't mean the logic for translating lat and lon values into one belongs in that method.

Side note, I get what lat and lon stand for in the context of creating a SqlGeography value, but these are bad variable names nonetheless: there's no reason to shorten the names like this - not even "it's faster to type"; with IntelliSense you type "lat" and it autocompletes "latitude" for you.

---

I'm going to make a little assumption here. This makes me think your method is returning something like a Dictionary<string,string>:

result.Add("success", "true");
result.Add("id", id.ToString());

That's a very error-prone way of working. The "success" value is useless, because if you have a value, you have a successful insert. If you don't have a value, you're dealing with an exception - and I see no try/catch block in your code, so I'm hoping the caller is handling such an exception...

You could have a class somewhere, with properties for each column of a database table / entity you're working with, and you should return that instead... but the simplest return type to use would be a Nullable<int>, so that the method either succeeded and returns the Id for the inserted record, or failed and returns null - for that to happen reliably, you'll need to wrap your logic in a try/catch block, and handle any SqlException that might be thrown in the process.

---

Another thing is that you're not properly disposing of your connection there. SqlConnection implements IDisposable, and the instance should be wrapped in a using block, too. Also I would try to open the connection before I start creating the command, so that I could pass the connection into the command's constructor (and not even bother instantiating it if the connection can't be opened). Keeps things cleaner I find:

try
{
    using (var connection = new SqlConnection(...))
    {
        connection.Open();
        using (var command = new SqlCommand(sql, connection))
        {
            //...
        }
    }
}
catch (SqlException)
{
    return null;
}

---

Lastly, I find that you're using confusing identifiers: in the top snippet your adapter is called cmd, and your command is called querySaveSubscription, as if it were a string containing the SQL statement (and the SQL string in question is called query... but doesn't query anything). ...but then in the bottom snippet a command is called a command and a reader is called a reader, and a connection that might never have successfully opened is called openCon. Would you ever work with a closedCon? Why not just call it connection then?