SQL Trouble Creating/Writing Stored Procedure

Question

I am having trouble creating a stored procedure to insert an item into my database, the stored procedure also checks that the items exist and if not create them.

Here is my database diagram

and here is my SQL code used to create the tables (with the exception that a foreign key was added from table MenuItem.MenuGroupID to MenuGroup.MenuGroupId) this will show data types, etc.

use MenuDatabase
GO

CREATE TABLE Menu
(
MenuID int primary key identity (1,1),
MenuTitle varchar(200) null,
MenuDescriptionText varchar(200) null,
)

create table Menugroup
(
MenuGroupID int primary key identity (1,1),
MenuID int,
MenuGroupText varchar(200) null,
MenuGroupDescriptionText varchar(200) null,
)
 create table MenuItem
 (
 MenuItemID int primary key identity (1,1),
 MenuID int,
 MenuGroupID varchar(200) null,
 MenuItemCallNumber varchar(200) null,
 MenuItemTitle varchar(200) null,
 MenuItemDescriptionText varchar(200) null,
 MenuItemNutritionText varchar(200) null,
 MenuItemIngredientsText varchar(200) null,
 MenuItemQuantity varchar(200) null,
 MenuItemPreparationTime varchar(200) null,
 MenuItmeCost varchar(200) null,
 MenuItemTypeCode varchar(200) null,
 MenuItemEthnicTypeCode varchar(200) null,
 MenuItemAvailabilityBit varchar(200) null,
 )

Here is the SQL script I'm working on for the stored procedure.

SET ANSI_NULLS ON
GO

SET QUOTED_IDENTIFIER ON
GO



ALTER PROCEDURE [dbo].[sp_InsertNewMenuItem] (
 @MenuItemTitle VARCHAR(200),
 @MenuGroupText VARCHAR(200),
 @MenuTitle VARCHAR(200)
 )

AS
BEGIN

DECLARE @MenuCount INT
SET @MenuCount = (SELECT COUNT(*) FROM Menu WHERE MenuTitle = @MenuTitle)

DECLARE @MenuID INT
IF @MenuCount= 1 
BEGIN
SET @MenuID = (SELECT MenuID FROM Menu WHERE MenuTitle = @MenuTitle)
END
ELSE
BEGIN
INSERT INTO Menu (MenuTitle) VALUES (@MenuTitle)
SET @MenuID = @@IDENTITY
END

DECLARE @MenuGroupCount INT
SET @MenuGroupCount = (SELECT COUNT(*) FROM MenuGroup WHERE MenuGroupText = @MenuGroupText)

DECLARE @MenuGroupID INT

IF @MenuGroupCount = 1 
BEGIN
 SET @MenuGroupID = (SELECT MenuGroupID FROM MenuGroup WHERE MenuGroupText = @MenuGroupText)
END
ELSE
BEGIN
INSERT INTO MenuGroup (MenuGroupText, MenuGroupID) VALUES (@MenuGroupText, @MenuID)
SET @MenuGroupID = @@IDENTITY
END

INSERT INTO MenuItem (MenuItemTitle)
     VALUES (@MenuItemTitle)

INSERT INTO MenuGroup (MenuGroupText)
     VALUES (@MenuGroupText)

INSERT INTO Menu (MenuTitle)
     VALUES (@MenuTitle)

     SET IDENTITY_INSERT MenuGroup OFF


END


GO

I'm currently getting the following errors in management studio when executing the procedure

(1 row(s) affected)
Msg 544, Level 16, State 1, Procedure sp_InsertNewMenuItem, Line 36
Cannot insert explicit value for identity column in table 'MenuGroup' when IDENTITY_INSERT is set to OFF.

I need to try and fix these errors ( I don't understand why they are occurring), as well as make sure the function will do what is required that is to check the following:

Stored Procedure Variable/Field| Table Field| If Exists| If Not Exists

MenuTitle varchar| Menu.MenuTitle| Get Menu.MenuID| Insert, Get Identity

Menu.MenuID| MenuGroupText varchar| MenuGroup.MenuGroupText|Get MenuGroup.MenuGroupID| Insert, Get Identity MenuGroup.MenuGroupID

MenuItem varchar| MenuItem.MenuItemTitle| Do Not Insert| Insert

An example;

MenuTitle - Lunch Menu

MenuGroupText - Sandwiches

MenuItem - California Chicken Sandwich

The first two errors are because you are not calling DECLARE on @MenuGroupCount — Minijack
You are missing an opening bracket on Line 36 after VALUES — Minijack
new errors: the first one is because by default you cannot insert into the primary key column of the table, this is easy to fix with SET INDENTITY_INSERT ON the second one is because of some Foreign key rule that you have, probably because you are duplicating the value — Minijack

Zohar Peled Zohar Peled · Accepted Answer · 2018-02-12T06:02:56

Even if you solve the error messages, your current stored procedure does have some problems that can be easily fixed. For instance, you don't need to count the records to decide if they exists in the table.
Also,you shouldn't use @@Identity, you should use scope_identity().
The reason for this is that @@Identity will return the last identity value inserted in the database, so you might get values inserted into a table you don't even relate to in your procedure. scope_identity(), however, will return the last identity value inserted in the current scope - meaning inside your stored procedure.

so this part:

DECLARE @MenuCount INT
SET @MenuCount = (SELECT COUNT(*) FROM Menu WHERE MenuTitle = @MenuTitle)

DECLARE @MenuID INT
IF @MenuCount= 1 
BEGIN
    SET @MenuID = (SELECT MenuID FROM Menu WHERE MenuTitle = @MenuTitle)
END
ELSE
BEGIN
    INSERT INTO Menu (MenuTitle) VALUES (@MenuTitle)
    SET @MenuID = @@IDENTITY
END

can be written like this:

DECLARE @MenuID INT
SELECT @MenuID = MenuID FROM Menu WHERE MenuTitle = @MenuTitle;

IF @MenuID IS NULL
BEGIN
    INSERT INTO Menu(MenuTitle) VALUES (@MenuTitle);
    SET @MenuID = SCOPE_IDENTITY()
END

Note that my version have only a single select statement instead of two in your version.

Also, this entire procedure should be inside a transaction, so that if one part fails, all the changes until the failure point will be reverted.
So here is how I would suggest writing your procedure (note I've change it's name from sp_InsertNewMenuItem to stp_InsertNewMenuItem to address marc_s's comment:

DROP PROCEDURE [dbo].[sp_InsertNewMenuItem] 
GO

CREATE PROCEDURE [dbo].[stp_InsertNewMenuItem] 
(
    @MenuItemTitle VARCHAR(200),
    @MenuGroupText VARCHAR(200),
    @MenuTitle VARCHAR(200)
)

AS
BEGIN

    BEGIN TRANSACTION

    BEGIN TRY

    DECLARE @MenuID INT
    SELECT @MenuID = MenuID 
    FROM Menu 
    WHERE MenuTitle = @MenuTitle

    IF @MenuID IS NULL
    BEGIN
        INSERT INTO Menu(MenuTitle) 
        VALUES (@MenuTitle)
        SET @MenuID = SCOPE_IDENTITY()
    END

    DECLARE @MenuGroupID INT            
    SELECT @MenuGroupID = MenuGroupID 
    FROM MenuGroup 
    WHERE MenuGroupText = @MenuGroupText

    IF @MenuGroupID IS NULL
    BEGIN
        INSERT INTO MenuGroup(MenuID, MenuGroupText) 
        VALUES (@MenuID, @MenuGroupText)
        SET @MenuGroupID = SCOPE_IDENTITY()
    END

    INSERT INTO MenuItem (MenuID, MenuGroupID, MenuItemTitle)
    VALUES (@MenuID, @MenuGroupID, @MenuItemTitle)

    COMMIT TRANSACTION

    END TRY
    BEGIN CATCH
        IF @@TRANCOUNT > 0
            ROLLBACK TRANSACTION
    END CATCH
END
GO

One more thing - if you multiple users might edit the menu simultaneously, you might get the wrong results (because of race conditions). To avoid that, read Dan Guzman's blog post on the subject.

SQL Trouble Creating/Writing Stored Procedure

1 Answers