Shifting a sort order in a table causes IIS to crash

Question

Current project:

• ASP.NET 4.5.2
• MVC 5

I have the following update code for an entry:

[HttpPost]
[ValidateAntiForgeryToken]
public async Task<ActionResult> EditClient(ModifyClientViewModel model) {
  Clients clients = await db.Clients.FindAsync(new Guid(model.ClientId));
  if(clients == null) { return HttpNotFound(); }
  try {
    if(ModelState.IsValid) {
      TextInfo ti = CultureInfo.CurrentCulture.TextInfo;
      clients.ClientName = ti.ToTitleCase(model.ClientName.Trim());
      clients.CityId = new Guid(model.CityId);
      clients.SortOrder = MakeRoom(model.SortOrder, model.ClientId);
      clients.Active = model.Active;
      clients.Modified = DateTime.UtcNow;
      clients.TouchedBy = User.GetClaimValue("UserGuid");
      db.Entry(clients).State = EntityState.Modified;
      await db.SaveChangesAsync();
      return RedirectToAction("Index");
    }
  } catch(DbUpdateConcurrencyException ex) {
// removed
  }
  return View(model);
}

The important line is Line 11, which calls MakeRoom(), where I need to be able to shift the sort order of the entry in question, assuming it is changed.

Now, a word about the sort order: It is a short (smallint) column in SQL Server 2012, because there will probably never be more than a few hundred entries anyhow. These are sequential numbers from 1 on up. There will be no gaps in the number sequence (deletions will pull everyone else down), so pulling the max value from that column will also describe the number of rows there are. While two rows can have the same SortOrder (the column allows duplicates for the sorting code to function), this is not supposed to be a persistent state -- it will exist only as long as the actual sort code is running. Once the sorting is done, no duplicate should exist.

If anyone remembers their first-year programming classes, this recursion should be analogous to a “bubble sort” where an element bubbles up to where it is supposed to be. Except here we are “bubbling” by actually changing the row’s SortOrder value.

I have some code that is supposed to be a recursive algorithm that takes the desired position, and the ID of the current position, and brings the current position close to the desired position by swapping with the next closer item before looping back onto itself. Once current = desired, the recursion is supposed to end, punting the desired value back to the code above.

Here is what I have made:

public short MakeRoom(short newSort, string id) {
  var currentClient = db.Clients.Find(new Guid(id));
  short currentSort = currentClient.SortOrder;
  if(currentSort != newSort) { // need to shift the current value
    short shiftSort = (currentSort < newSort ? currentSort++ : currentSort--); //are we shifting up or down?
    var shiftClient = db.Clients.Where(x => x.SortOrder == shiftSort).First();
    Clients clients = db.Clients.Find(shiftClient.ClientId);
    clients.SortOrder = currentSort; // give the next row the current client's sort number
    db.Entry(clients).State = EntityState.Modified;
    db.SaveChanges();
    currentClient.SortOrder = shiftSort; // give the current row the next row's sort number
    db.Entry(currentClient).State = EntityState.Modified;
    db.SaveChanges();
    MakeRoom(newSort, id); //One swap done, proceed to the next swap.
  }
  return newSort;
}

As you can see, my “base condition” is whether current = desired, and if it does match, all the code is supposed to be ignored in favour of the return statement. If it doesn’t match, the code executes one shift, and then calls itself to conduct the next shift (re-evaluating the current sort value via the ID of the current client because the current sort number is now different due to the just-executed prior shift). Once all shifts are done and current = desired, the code exits with the return statement.

I was hoping someone could examine my logic and see if it is where my problem lies. I seem to be having an infinite loop that doesn’t actually touch the DB, because none of the values in the DB actually get altered -- IIS just ends up crashing.

---

Edit 1: Found the stack overflow. Turns out the problem is with

    var shiftClient = db.Clients.Where(x => x.SortOrder == shiftSort).First();

Problem is, I am not sure why. In the prior line, I had set shiftSort to be one off (depending on the direction) from the current sort. I then want to grab the ClientID via this shiftSort value (which is the SortOrder). Since there should be only one such SortOrder value in the column, I should be able to do a search for it using the line above. But apparently it throws a stack overflow.

So, to be specific: Let's say I went after a client with a SortOrder of 53. I want him to end up with a SortOrder of 50. The prior line takes that 50 (the newSort) and discovers that it is less than the currentSort, so it assigns the shiftSort a value of currentSort-- (53 - 1 = 52). The line above is supposed to take that value of 52, and return a row where that 52 exists, so that on the following rows the ClientID of 52 can be used to modify that line to be 53 (the swap).

Suggestions? I am not understanding why I am experiencing a stack overflow here.

---

Edit 2: Revamped by MakeRoom method, but I am still experiencing a Stack Overflow on the affected line:

public short MakeRoom(short newSort, string id) {
  Clients currentClient = db.Clients.Find(new Guid(id));
  short currentSort = currentClient.SortOrder;
  if(currentSort != newSort) { // need to shift the current sort
    short shiftSort = (currentSort < newSort ? currentSort++ : currentSort--);
    Clients shiftClient = db.Clients.Where(x => x.SortOrder == shiftSort).FirstOrDefault(); //Stack Overflow is here -- why??
    shiftClient.SortOrder = currentSort;
    db.Entry(shiftClient).State = EntityState.Modified;
    currentClient.SortOrder = shiftSort;
    db.Entry(currentClient).State = EntityState.Modified;
    db.SaveChanges();
    MakeRoom(newSort, id);
  }
  return newSort;
}

---

Edit 3: I have altered my MakeRoom method again:

public void MakeRoom(short newSort, string id) {
  var currentClient = db.Clients.Find(new Guid(id));
  short currentSort = currentClient.SortOrder;
  if(currentSort < newSort) {
    var set = db.Clients.Where(x => x.SortOrder > currentSort && x.SortOrder <= newSort).OrderBy(c => c.SortOrder).ToList();
    short i = set.First().SortOrder;
    set.ForEach(c => {
      c.SortOrder = i--;
      c.Modified = DateTime.UtcNow;
      c.TouchedBy = User.GetClaimValue("UserGuid");
      db.Entry(c).State = EntityState.Modified;
    });
    db.SaveChanges();
  } else if(currentSort > newSort) {
    var set = db.Clients.Where(x => x.SortOrder >= newSort && x.SortOrder < currentSort).OrderBy(c => c.SortOrder).ToList();
    short i = set.First().SortOrder;
    set.ForEach(c => {
      c.SortOrder = i++;
      c.Modified = DateTime.UtcNow;
      c.TouchedBy = User.GetClaimValue("UserGuid");
      db.Entry(c).State = EntityState.Modified;
    });
    db.SaveChanges();
  }
}

But even though the debugger clearly walks through the code and in the right way, the actual DB values do not get changed.

Answer 1

Let the database sort this for you.

var first = db.Clients.FirstOrDefault(c => c.Guid == guid);

var set = db.Clients
    .Where(c => c.SortOrder >= first.SortOrder)
    .OrderBy(c => c.SortOrder).ToList();

int i = set.First().SortOrder;  // or 1

set.ForEach(c => {
    c.SortOrder = i++;
    db.Entry(c).State = EntityState.Modified;
});
db.SaveChanges();

Answer 2

Oh. My. Goodness. Talk about overlooking the single screw that caused the house to collapse.

My problem was not with the line that was causing the stack overflow -- my problem was with the line immediately prior:

short shiftSort = (currentSort < newSort ? currentSort++ : currentSort--);

Can you see the problem? I didn’t. Until now. While attempting to implement the solution provided by Jasen (thank you for your efforts, Good Sir, they were greatly appreciated), I was forced to go through my code a bit more carefully, and I noticed something strange: the increments/decrements. Those are supposed to be for altering the actual value of the item being incremented/decremented, even if it is part of an assignment. So no wonder my script was f**cking up -- I was changing the value of currentSort at the same time I was assigning it to shiftSort.

What I did was two-fold: since there wasn’t any real need to pass a value back, I altered the original HttpPost method as such:

[HttpPost]
[ValidateAntiForgeryToken]
public async Task<ActionResult> EditClient(ModifyClientViewModel model) {
  Clients clients = await db.Clients.FindAsync(new Guid(model.ClientId));
  if(clients == null) { return HttpNotFound(); }
  try {
    if(ModelState.IsValid) {
      MakeRoom(model.SortOrder, clients.SortOrder);
      TextInfo ti = CultureInfo.CurrentCulture.TextInfo;
      clients.ClientName = ti.ToTitleCase(model.ClientName.Trim());
      clients.CityId = new Guid(model.CityId);
      clients.SortOrder = model.SortOrder;
      clients.Active = model.Active;
      clients.Modified = DateTime.UtcNow;
      clients.TouchedBy = User.GetClaimValue("UserGuid");
      db.Entry(clients).State = EntityState.Modified;
      await db.SaveChangesAsync();
      return RedirectToAction("Index");
    }
  } catch(DbUpdateConcurrencyException ex) {
    // Ignore
  }
  return View(model);
}

Notice how the MakeRoom() is moved to the front, and given only the Source and Destination values? No more need to pass an ID.

Then for the actual method:

public void MakeRoom(short newSort, short oldSort) {
  if(oldSort != newSort) { // need to shift the current sort
    short shiftSort = (oldSort < newSort ? (short)(oldSort + 1) : (short)(oldSort - 1));
    Clients currentClient = db.Clients.Where(x => x.SortOrder == oldSort).FirstOrDefault();
    currentClient.SortOrder = shiftSort;
    db.Entry(currentClient).State = EntityState.Modified;
    Clients shiftClient = db.Clients.Where(x => x.SortOrder == shiftSort).FirstOrDefault();
    shiftClient.SortOrder = oldSort;
    db.Entry(shiftClient).State = EntityState.Modified;
    db.SaveChanges();
    MakeRoom(newSort, shiftSort);
  }
}

Now look at the assignment to shiftSort - I assign it the value of oldSort modified by a single digit. This, I am shamed to say, has made all the difference.

My code now works perfectly, and instantly even over many dozens of intermediary items. I can take an item that had a SortOrder of 3, and move it to a SortOrder of 53, and everything from 53 down to 4 gets shifted down one perfectly to make room (at SortOrder 53) for the item that formerly had a SortOrder of 3.

The nice thing about this method is that it can be used equally well for deletions (to prevent gaps in the numbering) and additions (when you want the new item somewhere other than the end). For deletions you just shift everything after the deleted item (say, SortOrder 33 to Max SortOrder) down one, and for additions you shift everything from the insertion point to the Max SortOrder up one, then insert your value.

I hope this helps anyone who comes after me, and the fates help them with Google -- they will get reams of results that talk about sorting and paging output, and precious little about changing the value of the sort order for each entry between a source and a destination value.