asp.net mvc / Sql double register

Question

I made an new MVC project and I made insert function into db. My problem is that when i enter a model into db it records twice. Tried debugging, but unfortunately couldn't find the mistake. Is there any miss in the code below?

 public class DbCode
    {
        protected SqlConnection conn;

        public bool Open(string Connection = "MyDb")
        {
            conn = new SqlConnection(@WebConfigurationManager.ConnectionStrings[Connection].ToString());
            try
            {
                var b = true;
                if (conn.State.ToString() != "Open")
                {
                    conn.Open();
                }
                return b;
            }
            catch (SqlException ex)
            {
                return false;
            }
        }

        public bool Close()
        {
            try
            {
                conn.Close();
                return true;
            }
            catch (Exception ex)
            {
                return false;
            }
        }

        public int ToInt(object s)
        {
            try
            {
                return Int32.Parse(s.ToString());
            }
            catch (Exception)
            {
                return 0;
            }

        }

        public int DataInsert(string sql)
        {
            int LastID = 0;
            string query = sql + ";SELECT @@Identity;";
            try
            {
                if (conn.State.ToString() == "Open")
                {
                    SqlCommand cmd = new SqlCommand(query, conn);
                    cmd.ExecuteNonQuery();
                    LastID = this.ToInt(cmd.ExecuteScalar());
                }
                return this.ToInt(LastID);
            }
            catch
            {
                return 0;
            }
        }
    }


 public class StudentModel
    {
        [Required]
        [StringLength(5)]
        public string productname { get; set; }

        [Required]
        [StringLength(5)]
        public string quantity { get; set; }

        [Required]
        [StringLength(5)]
        public string price { get; set; }
    }

Controller

 public class StudentController : Controller
    {
        protected DbCode DB = new DbCode();
        public ActionResult Index()
        {
            return View();
        }
        [HttpPost]
        [ValidateAntiForgeryToken]
        public ActionResult SaveDataStudent(StudentModel student)
        {
            if (ModelState.IsValid)
            {
                DB.Open();
                int i = DB.DataInsert("INSERT INTO tblproducts(productname,quantity,price) VALUES('" + student.productname + "','" + student.quantity + "','" + student.price + "')");
                if (i > 0)
                {
                    ModelState.AddModelError("Success", "Save Success");
                }
                else
                {
                    ModelState.AddModelError("Error", "Save Error");
                }
                DB.Close();
            }
            return RedirectToAction("Index", "Student");
        }
    }

Student View

@model MVC5.Models.StudentModel

@{
    ViewBag.Title = "Index";
}


@using (Html.BeginForm("SaveDataStudent", "Student", new { @id = "Form" }, FormMethod.Post))
{
    @Html.ValidationSummary();
    @Html.AntiForgeryToken();
    @Html.LabelFor(m => m.productname);
    @Html.TextBoxFor(m => m.productname);<br/>

    @Html.LabelFor(m => m.quantity);
    @Html.TextBoxFor(m => m.quantity);<br />

    @Html.LabelFor(m => m.price);
    @Html.TextBoxFor(m => m.price);<br />

    <input type="submit" value="Save" name="Save" />
}

Connection string in webconfig is added

score 1 · Accepted Answer · edited May 23 '17 at 12:30

The problem lays in these lines:

cmd.ExecuteNonQuery();
LastID = this.ToInt(cmd.ExecuteScalar());

You execute the same command twice so 2 records are inserted into a database.

In order to read the last identity value I would define a stored procedure which will insert data into a table and then return the last identity value. This approach is described here.

By the way, there is also another serious problems with your code. You define an sql command by concatenating strings. Some of these strings come from a user side. It means that your code is vulnerable to SQL Injection attack. Instead you should use parameters (see this or this).

I also suggest not to use magic constants like in this line conn.State.ToString() == "Open" The better approach is to use an enumeration member: conn.State == ConnectionState.Open.

asp.net mvc / Sql double register

1 Answers1