SqlException: procedure or function has too many arguments specified

Question

I have very little experience in SQL, I have a method that is accessing the database and adding data to a few different tables. I think my stored procedure is correct. It's the method I am having issues with.

I am creating a Web API endpoint that is a PUT, it's supposed to be able to add multiple holidays and dates to multiple sessions and update the database.

This is what I have currently, what do I need to do so I do not get the exception?

private void SaveGlobalOrderDays(List<SessionInfoList> sessionList, List<Holiday> selectedOrderHolidays, List<Holiday> selectedHolidays, List<string> orderDays, List<string> noOrderDays)
{
    try
    {
        using (SqlCommand cmd = new SqlCommand())
        using (SqlConnection connection = new SqlConnection(ConnectionString))
        {
            cmd.CommandTimeout = 600;
            cmd.CommandText = "[dbo].[SaveGlobalOrderDays]"; 
            cmd.CommandType = CommandType.StoredProcedure;
            cmd.Connection = connection;

            foreach (SessionInfoList session in sessionList)
            {
                cmd.Parameters.Add("@SessionId", SqlDbType.Int).Value = session.SessionID;

                if (selectedOrderHolidays.Count > 0)
                {
                    foreach (Holiday holiday in selectedOrderHolidays)
                    {
                        string orderHolidays = string.Join(",", holiday.Name).Trim();
                        cmd.Parameters.Add("@HolidayName", SqlDbType.NVarChar).Value = orderHolidays; 
                    }                                
                }

                if (selectedHolidays.Count > 0)
                {
                    foreach (Holiday holiday in selectedHolidays)
                    {
                        string noOrderHolidays = string.Join(",", holiday.Name).Trim();
                        cmd.Parameters.Add("@SelectedHolidayName", SqlDbType.NVarChar).Value = noOrderHolidays; 
                    }                                
                }

                if (orderDays.Count > 0)
                {
                    foreach (string order in orderDays)
                    {
                        string od = string.Join(",",order).Trim();
                        cmd.Parameters.Add("@OrderDays", SqlDbType.NVarChar).Value = od; 
                    }                                
                }

                if (noOrderDays.Count > 0)
                {
                    foreach (string noOrder in noOrderDays)
                    {
                        string nod = string.Join(",",noOrder).Trim();
                        cmd.Parameters.Add("@NoOrderDays", SqlDbType.NVarChar).Value = nod; 
                    }                                
                }                                             
            }

            foreach (SqlParameter parameter in cmd.Parameters)
            {
                if (parameter.Value == null)
                    parameter.Value = DBNull.Value;
            }

            connection.Open();

            cmd.ExecuteNonQuery();

            cmd.Dispose();
        }
    }
    catch (Exception ex)
    {
        string message = "Failed to save global order info";
        // Logger.Error(LogSource, "SaveGlobalOrderDays", string.Empty, message, string.Empty, ex);
        throw new Exception(message, ex);
    }
}

Stored procedure:

IF OBJECT_ID('[dbo].[SaveGlobalOrderDays]') IS NOT NULL
     DROP PROCEDURE [dbo].[SaveGlobalOrderDays]
GO

CREATE PROCEDURE [dbo].[SaveGlobalOrderDays]
    @SessionId INT,
    @HolidayName NVARCHAR(MAX),
    @SelectedHolidayName NVARCHAR(MAX),
    @OrderDays NVARCHAR(MAX),
    @NoOrderDays NVARCHAR(MAX)
WITH ENCRYPTION
AS
BEGIN
    SET NOCOUNT ON;

    UPDATE [cfgSchedule]
    SET [cfgSchedule].[SessionId] = @SessionId,
        [OrderDays] = @OrderDays,
        [NoOrderDays] = @NoOrderDays
    FROM [cfgSchedule],[SessionHolidayMapping],[SessionOrderHolidayMapping]
    WHERE [cfgSchedule].[SessionId] = [SessionHolidayMapping].[SessionId]
      AND [cfgSchedule].[SessionId] = [SessionOrderHolidayMapping].[SessionId]
      AND [cfgSchedule].[SessionId] = @SessionId

    UPDATE [SessionHolidayMapping]
    SET [SessionHolidayMapping].[HolidayName] = @SelectedHolidayName
    FROM [cfgSchedule],[SessionHolidayMapping],[SessionOrderHolidayMapping]
    WHERE [cfgSchedule].[SessionId] = [SessionHolidayMapping].[SessionId]
      AND [cfgSchedule].[SessionId] = [SessionOrderHolidayMapping].[SessionId]
      AND [cfgSchedule].[SessionId] = @SessionId

    UPDATE [SessionOrderHolidayMapping]
    SET [SessionOrderHolidayMapping].[HolidayName] = @HolidayName
    FROM [cfgSchedule],[SessionHolidayMapping],[SessionOrderHolidayMapping]
    WHERE [cfgSchedule].[SessionId] = [SessionHolidayMapping].[SessionId]
      AND [cfgSchedule].[SessionId] = [SessionOrderHolidayMapping].[SessionId]
      AND [cfgSchedule].[SessionId] = @SessionId
END
GO  

Answer 1

We have this loop:

foreach (SessionInfoList session in sessionList)
{

And within the loop, we have this parameter:

cmd.Parameters.Add("@SessionId", SqlDbType.Int).Value = session.SessionID;

and conditionally these others, which are also inside loops:

cmd.Parameters.Add("@NoOrderDays", SqlDbType.NVarChar).Value = nod; 
cmd.Parameters.Add("@HolidayName", SqlDbType.NVarChar).Value = orderHolidays; 
cmd.Parameters.Add("@SelectedHolidayName", SqlDbType.NVarChar).Value = noOrderHolidays; 
cmd.Parameters.Add("@OrderDays", SqlDbType.NVarChar).Value = od; 

These loops attempt to re-add new items to the Parameters collection on each iteration, meaning you'll have the wrong number of parameters starting the first time any of those reaches it's second iteration.

Additionally, we do this at the bottom of the loop:

connection.Open();
cmd.ExecuteNonQuery();
cmd.Dispose();

A tight loop like this is the one case where we don't really have to re-create and re-open the connection each time. Let's do this:

private void SaveGlobalOrderDays(IEnumerable<SessionInfoList> sessionList, IEnumerable<Holiday> selectedOrderHolidays, IEnumerable<Holiday> selectedHolidays, IEnumerable<string> orderDays, IEnumerable<string> noOrderDays)
{
    try
    {
        using (SqlConnection connection = new SqlConnection(ConnectionString))
        using (SqlCommand cmd = new SqlCommand("[dbo].[SaveGlobalOrderDays]", connection))
        {
            cmd.CommandTimeout = 600;
            cmd.CommandType = CommandType.StoredProcedure;

            //Set up parameter placeholders once, before any looping starts.
            // From here on out we'll only ever set their .Value properties.
            cmd.Parameters.Add("@SessionId", SqlDbType.Int);
            //Add a Length to these!
            // If the target columns really are nvarchar(max), use -1
            cmd.Parameters.Add("@HolidayName", SqlDbType.NVarChar, -1); 
            cmd.Parameters.Add("@SelectedHolidayName", SqlDbType.NVarChar, -1);
            cmd.Parameters.Add("@OrderDays", SqlDbType.NVarChar, -1);
            cmd.Parameters.Add("@NoOrderDays", SqlDbType.NVarChar, -1);


            //Open the connection just once, immediately before beginning of the loop.
            // The using block will ensure it is closed later.
            connection.Open(); 
            foreach (SessionInfoList session in sessionList)
            {
                cmd.Parameters["@SessionId"].Value = session.SessionID;

                cmd.Paramters["@HolidayName"].Value = DBNull.Value;
                string joinedNames = string.Join(",", selectedOrderHolidays.Select(h => h.Name.Trim()));
                if (!string.IsNullOrEmpty(joinedNames))
                    cmd.Paramters["@HolidayName"].Value = joinedNames;

                cmd.Paramters["@SelectedHolidayName"].Value = DBNull.Value;
                joinedNames = string.Join(",", selectedHolidays.Select(h => h.Name.Trim()));
                if (!string.IsNullOrEmpty(orderHolidays))
                    cmd.Paramters["@SelectedHolidayName"].Value = joinedNames;

                cmd.Paramters["@OrderDays"].Value = DBNull.Value;
                joinedNames = string.Join(",", orderDays);
                if (!string.IsNullOrEmpty(joinedNames))
                    cmd.Paramters["@OrderDays"].Value = joinedNames;

                cmd.Paramters["@NoOrderDays"].Value = DBNull.Value;
                joinedNames = string.Join(",", noOrderDays);
                if (!string.IsNullOrEmpty(joinedNames))
                    cmd.Paramters["@NoOrderDays"].Value = joinedNames;

                //Now there's no need to Open() or Dispose() anything at this point.
                cmd.ExecuteNonQuery();
            }
        }
        // Consider using a separate Try/Catch inside the loop.
        // ... unless you want one failure to abort everything part way through, with some updates completed and some not.
        catch (Exception ex)
        {
            string message = "Failed to save global order info";
            // Logger.Error(LogSource, "SaveGlobalOrderDays", string.Empty, message, string.Empty, ex);
            throw new Exception(message, ex);
        }
    }

Finally, this looks like you're setting comma-separated values within individual columns. This is really poor schema design! Never put comma-separated data into a single column. You'll likely do so much better to put these columns into additional tables.