CodeSOD: Uniquely Unique

Giles's company has a hard time with doing things in the database.

In today's example, they attempt the very challenging task of generating unique IDs in a SQL Server database. Now, what you're about to see follows the basic pattern of "generate a random number and see if it's already been used", which is a fairly common anti-pattern, but it's managed to do this in some of the worst ways I've ever seen. And it can't even hide behind the defense of being written a long time ago- it's a handful of years old.

Comments to this C# code have been added by Giles, and no, there were no comments.

protected void AddBlankRowToDatabase(){ //This - in effect - calls a stored procedure which has been carefully designed to work around the issue with SPs and //SQL injection - i.e. in theory using a stored procedure could help prevent it from happening, which is obviously //a problem, so this SP allows it once again. //also note that this actually fetches *all* machines for the currently selected customer, //so is basically calling "select * from ..." and creating a list of objects to represent them. List<Machine> allMachine = MachineManager.GetByCriteria("CustomerId=" + Convert.ToString(Session["ActiveCustomerId"]), ""); //having gone to all that trouble, let us see how many machines there are, and increment this since we are going to //add one int machineCount = allMachine.Count + 1; //Create a new object representing a machine. Machine machine = new Machine(); //we will definitely want random numbers, as that is the correct way to ensure uniqueness. Random _rng = new Random(); //new numbers must start with 999 for no reason that is documented or known to anyone. string paddedString, padding = "999"; //OK, random number please. We want it to be 3 chars, but instead of all that zero-padding nonsense, let's just //ensure this by starting from 100. int RandomId = _rng.Next(100, 999); //append our random number to our "999" paddedString = padding + RandomId; RandomId = Convert.ToInt32(paddedString); //now here's the big brain part; we don't know if the machine ID already exists, so we check if it does! bool machineIdNotExists = true; while (machineIdNotExists) { //Get any machine with that ID, again via this garbage SP system. List<Machine> machineExists = MachineManager.GetByCriteria("MachineId=" + RandomId, ""); if (machineExists.Count > 0) { //ah well, let's try *another* random 'number' RandomId = _rng.Next(100, 999); paddedString = padding + RandomId; RandomId = Convert.ToInt32(paddedString); machineIdNotExists = true; } else { //ok, so the machine does not not Exist, we have a new number! machineIdNotExists = false; } } //Good stuff, ensure we use the ID we found machine.MachineId = RandomId; //let's use the count of machines we arrived at earlier to set a temporary title for the machine.... //yes, this is the *only* use of the count, which we worked out by selecting an entire list //of DB rows, constructing a bunch of objects and then *counting* them. machine.Title = "New Machine " + machineCount; //////// //Snip a bunch of other boring property setting ///////// //Now we save the created machine. //This internally takes the Machine Object and (manually - no EF or similar) //pulls its properties into params for another stored proc, which adds the entry to the DB. MachineManager.InsertMachine(machine); //Now run through the entire grid of all machines, and save them all individually. //Even though you haven't amended them. SaveAllRecords(); //Reload the page, which will refresh the grid to show our new machine! Response.Redirect("MachinesAdmin.aspx?RecordSaved=Yes");}

SQL Injection by stored procedure, fetching entire sets from the database when you need a count. Forcing entire front-end page refreshes via redirect. Mysterious and very not random padding.

Every choice made here was a bad choice.

[Advertisement] Utilize BuildMaster to release your software with confidence, at the pace your business demands. Download today!