C# replacing foreach

Question

I'm using a Webservice GET request to get some info about customers. I want to insert a record to the DB for each customer returned from the request .

I have this code for now :

            var container = JsonConvert.DeserializeObject<MarkedCampaigns>(json);
            string insertDB = "";
            foreach (var item in container.items)
            {
                insertDB += "INSERT INTO TABLE (CampaignId,CookieId,Url) values(" + item.CampaignId + "," + item.VisitorExternalId + "," + item.Url + ");";
            }
            //EXECUTE STRING .

Container is the response from the Get request. It contains an Item property which each item represent a customer.

My question is, is this the correct way to insert the records to my DB? Or is there a simpler way using the container and some methods I'm not familiar with ?

Answer 1

I would strongly suggest to use a prepared statement. This will on the one hand remove some unnecessary overhead for parsing the query every time and on the other hand it will force you to use parameterized queries, which will prevent issues with type conversion -- which I see will happen in your code because url is most likely some character type and you didn't add quotes -- and SQL injection.

string query = "INSERT INTO table(CampaignId, CookieId, Url) VALUES (@campaignid, @cookieid, @url)";

using (SqlConnection c = new SqlConnection(connectstring)) {
    c.Open();
    SqlCommand cmd = new SqlCommand(query, c);
    cmd.Parameters.Add(new SqlParameter("@campaignid", SqlDbType.Int, 0)); //use appropriate type/size here
    cmd.Parameters.Add(new SqlParameter("@cookieid", SqlDbType.Int, 0)); //use appropriate type/size here
    cmd.Parameters.Add(new SqlParameter("@url", SqlDbType.NVarChar, 500)); //use appropriate type/size here

    cmd.Prepare();

    foreach (var item in container.items) {
        cmd.Parameters[0].value = item.CampaignId;
        cmd.Parameters[1].value = item.VisitorExternalId;
        cmd.Parameters[2].value = item.url;
        cmd.ExecuteNonQuery();
    }
}

Answer 2

If you only want to do one call to the db to do all inserts then one option is to use a stored proc that takes a list:

The sql to create the type that will store our list:

create type [dbo].CampaignList as table (CampaignId int,  CookieId int, [Url] varchar(255))

The stored proc to do the inserts

create procedure [dbo].[spSaveCampaigns]
    @CampaignList CampaignList readonly
as
    insert into tblCampaigns (CampaignId, CookieId, [Url])
    select CampaignId, CookieId, [Url] from @CampaignList;

The C# to call it:

public async Task InsertCampigns()
        {
            var campaigns = new List<Campaign> {new Campaign(1, 1, "bar"), new Campaign(2, 2, "foo") };
            using (var sqlConnection = new SqlConnection(_connectionString))
            {
                using (var cmd = new SqlCommand("exec [dbo].[spSaveCampaigns] @CampaignList", sqlConnection))
                {
                    await sqlConnection.OpenAsync().ConfigureAwait(false);

                    using (var table = new DataTable())
                    {
                        table.Columns.Add("CampaignId", typeof(int));
                        table.Columns.Add("CookieId", typeof(int));
                        table.Columns.Add("Url", typeof(string));

                        foreach (var campaign in campaigns)
                            table.Rows.Add(campaign.CampaignId, campaign.CookieId, $"{campaign.Url}");

                        var parameters = new SqlParameter("@CampaignList", SqlDbType.Structured)
                                         {
                                             TypeName = "dbo.CampaignList",
                                             Value = table
                                         };

                        cmd.Parameters.Add(parameters);
                        await cmd.ExecuteNonQueryAsync().ConfigureAwait(false);
                    }
                }
            }
        }

You could pull the code that creates the data table from your type out to a helper to make this smaller.

Advantages: It's parameterised properly. I prefer to call stored procs rather than run sql against the db (but you might have a different opinion on this.)

Result calling

await InsertCampigns();

CampaignId | CookieId | Url

1 | 1 | bar

2 | 2 | foo

To do this without the stored proc, see this link from @Magnus in comment

https://learn.microsoft.com/en-us/dotnet/framework/data/adonet/sql/table-valued-parameters#passing-a-table-valued-parameter-to-a-parameterized-sql-statement

Answer 3

A part from the great problem of Sql Injection you are facing a decision to take. If you have few elements to insert then probably you could use a simple loop as explained in the answer from @derpisher, but that answer requires a call to the database engine for each element of your query and you need to define yourself all the parameters.

A single command text with many inserts is preferable because you make a single call to the database engine and in case of many records to insert the difference is noticeable. But this last approach is difficult if you want to use a parameterized query because you will need a parameter for each single value you want to insert.

Instead I suggest you to try using Dapper.

With this simple ORM library you could write this

using(IDbConnection cnn = GetSqlConnection())
{
    string cmdText = @"INSERT INTO TABLE (CampaignId,CookieId,Url)
                       VALUES(@CampaignId, @VisitorExternalId, @Url)";
    cnn.Execute(cmdText, container.items);
}

Here the GetSqlConnection is a method that returns an instance of your connection already opened. The cmdText is the command to execute as if you have just one record to insert. The trick is the Execute extension command added by Dapper where you pass directly your list of items to insert and the commandtext. You only need to have the names of your parameters matching the names of the properties inside the list.