From 84dc040198d08c7c6cf981c728d4c4d8a94c0c05 Mon Sep 17 00:00:00 2001 From: Jay Lorch Date: Wed, 24 Apr 2019 21:11:04 -0700 Subject: [PATCH] Avoid raising SQL exceptions in the common case of updating an annotation (#412) * Update list of off-limits buildings in PH20 rules * Avoid annotation exceptions * Use FirstOrDefaultAsync instead of SingleOrDefaultAsync, per Morgan's suggestion --- ServerCore/SyncController.cs | 160 +++++++++++++++++++++-------------- 1 file changed, 95 insertions(+), 65 deletions(-) diff --git a/ServerCore/SyncController.cs b/ServerCore/SyncController.cs index 10ddc3f0..e7688388 100755 --- a/ServerCore/SyncController.cs +++ b/ServerCore/SyncController.cs @@ -289,87 +289,117 @@ private async Task HandleSyncAspectsRequiringListOfSolvedPuzzles(DecodedSyncRequ } /// - /// This routine stores in the database any annotations the requester has uploaded. + /// This routine updates a single existing annotation. As we do so, we increment + /// its version number and update its timestamp. + /// + /// This routine is only meant to be called when we know an annotation exists + /// with the given puzzle ID, team ID, and key. In other words, this routine + /// is called when we need to update that annotation. /// - private async Task StoreAnnotations(DecodedSyncRequest request, SyncResponse response, int puzzleId, int teamId) + private async Task UpdateOneAnnotation(SyncResponse response, int puzzleId, int teamId, int key, string contents) { - if (request.AnnotationRequests == null) { - return; - } + // You may wonder why we're using ExecuteSqlCommandAsync instead of "normal" + // Entity Framework database functions. The answer is that we need to atomically + // update the Version field of the record, and Entity Framework has no way of + // expressing that directly. + // + // The reason we want to update the version number atomically is that we rely + // on the version number being a unique identifier of an annotation. We don't + // want the following scenario: + // + // Alice tries to set the annotation for key 17 to A, and simultaneously Bob + // tries to set it to B. Each reads the current version number, finds it to be + // 3, and updates the annotation to have version 4. Both of these updates may + // succeed, but one will overwrite the other; let's say Bob's write happens last + // and "wins". So Alice may believe that version 4 is A when actually version 4 + // is B. When Alice asks for the current version, she'll be told it's version 4, + // and Alice will believe this means it's A. So Alice will believe that A is + // what's stored in the database even though it's not. Alice and Bob's computers + // will display different annotations for the same key, indefinitely. + // + // Note that we need a version number because the timestamp isn't guaranteed to + // be unique. So in the example above Alice and Bob might wind up updating with + // the same timestamp. + // + // You may also wonder why we use DateTime.Now instead of letting the database + // assign the timestamp itself. The reason is that the database might be running + // on a different machine than the puzzle server, and it might be using a different + // time zone. + + try { + var sqlCommand = "UPDATE Annotations SET Version = Version + 1, Contents = @Contents, Timestamp = @Timestamp WHERE PuzzleID = @PuzzleID AND TeamID = @TeamID AND [Key] = @Key"; + int result = await context.Database.ExecuteSqlCommandAsync(sqlCommand, + new SqlParameter("@Contents", contents), + new SqlParameter("@Timestamp", DateTime.Now), + new SqlParameter("@PuzzleID", puzzleId), + new SqlParameter("@TeamID", teamId), + new SqlParameter("@Key", key)); + if (result != 1) { + response.AddError("Annotation update failed."); + } + } + catch (DbUpdateException) { + response.AddError("Encountered error while trying to update annotation."); + } + catch (Exception) { + response.AddError("Miscellaneous error while trying to update annotation."); + } + } - foreach (var annotationRequest in request.AnnotationRequests) { - // Try to generate this as a new annotation, with version 1. + /// + /// This routine takes a single annotation that the requester has uploaded, and inserts + /// it if it doesn't exist and updates it otherwise. + /// + private async Task InsertOrUpdateOneAnnotation(SyncResponse response, int puzzleId, int teamId, int key, string contents) + { + // Check to see if the annotation already exists. If so, update it and we're done. - Annotation annotation = new Annotation(); - annotation.PuzzleID = puzzleId; - annotation.TeamID = teamId; - annotation.Key = annotationRequest.key; - annotation.Version = 1; - annotation.Contents = annotationRequest.contents; - annotation.Timestamp = DateTime.Now; + Annotation existingAnnotation = + await context.Annotations.FirstOrDefaultAsync(a => a.PuzzleID == puzzleId && a.TeamID == teamId && a.Key == key); + if (existingAnnotation != null) + { + context.Entry(existingAnnotation).State = EntityState.Detached; + UpdateOneAnnotation(response, puzzleId, teamId, key, contents); + } + else + { + // The annotation doesn't exist yet. So, try to generate a new annotation, with version 1. + Annotation annotation = new Annotation { PuzzleID = puzzleId, TeamID = teamId, Key = key, + Version = 1, Contents = contents, Timestamp = DateTime.Now }; try { context.Annotations.Add(annotation); await context.SaveChangesAsync(); } catch (DbUpdateException) { // If the insert fails, there must already be an annotation there with the - // same puzzle ID, team ID, and key. So we need to update the existing one. - // As we do so, we increment its version number and update its timestamp. - // - // You may wonder why we're using ExecuteSqlCommandAsync instead of "normal" - // Entity Framework database functions. The answer is that we need to atomically - // update the Version field of the record, and Entity Framework has no way of - // expressing that directly. - // - // The reason we want to update the version number atomically is that we rely - // on the version number being a unique identifier of an annotation. We don't - // want the following scenario: - // - // Alice tries to set the annotation for key 17 to A, and simultaneously Bob - // tries to set it to B. Each reads the current version number, finds it to be - // 3, and updates the annotation to have version 4. Both of these updates may - // succeed, but one will overwrite the other; let's say Bob's write happens last - // and "wins". So Alice may believe that version 4 is A when actually version 4 - // is B. When Alice asks for the current version, she'll be told it's version 4, - // and Alice will believe this means it's A. So Alice will believe that A is - // what's stored in the database even though it's not. Alice and Bob's computers - // will display different annotations for the same key, indefinitely. - // - // Note that we need a version number because the timestamp isn't guaranteed to - // be unique. So in the example above Alice and Bob might wind up updating with - // the same timestamp. - // - // You may also wonder why we use DateTime.Now instead of letting the database - // assign the timestamp itself. The reason is that the database might be running - // on a different machine than the puzzle server, and it might be using a different - // time zone. - - // First, detach the annotation from the context so the context doesn't think the annotation is in the database. + // same puzzle ID, team ID, and key. (This means there was a race condition: + // between the time we checked for the existence of a matching annotation and + // now, there was another insert.) So, we need to update the existing one. + + // But first, we need to detach the annotation from the context so the context + // doesn't think the annotation is in the database. + context.Entry(annotation).State = EntityState.Detached; - - try { - var sqlCommand = "UPDATE Annotations SET Version = Version + 1, Contents = @Contents, Timestamp = @Timestamp WHERE PuzzleID = @PuzzleID AND TeamID = @TeamID AND [Key] = @Key"; - int result = await context.Database.ExecuteSqlCommandAsync(sqlCommand, - new SqlParameter("@Contents", annotationRequest.contents), - new SqlParameter("@Timestamp", DateTime.Now), - new SqlParameter("@PuzzleID", puzzleId), - new SqlParameter("@TeamID", teamId), - new SqlParameter("@Key", annotationRequest.key)); - if (result != 1) { - response.AddError("Annotation update failed."); - } - } - catch (DbUpdateException) { - response.AddError("Encountered error while trying to update annotation."); - } - catch (Exception) { - response.AddError("Miscellaneous error while trying to update annotation."); - } + UpdateOneAnnotation(response, puzzleId, teamId, key, contents); } } } + /// + /// This routine stores in the database any annotations the requester has uploaded. + /// + private async Task StoreAnnotations(DecodedSyncRequest request, SyncResponse response, int puzzleId, int teamId) + { + if (request.AnnotationRequests == null) { + return; + } + + foreach (var annotationRequest in request.AnnotationRequests) { + await InsertOrUpdateOneAnnotation(response, puzzleId, teamId, annotationRequest.key, annotationRequest.contents); + } + } + /// /// This routine fetches the list of annotations that the requester's team has made since the last /// time the requester got a list of annotations.