Skip to content

Implement optimistic locking #8 #42

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
+350 −95
Open

Implement optimistic locking #8 #42

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions Sources/Linq2DynamoDb.DataContext.Tests/Entities/Book.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,9 @@ public class Book : EntityBase
[DynamoDBProperty(typeof(StringTimeSpanDictionaryConverter))]
public IDictionary<string, TimeSpan> FilmsBasedOnBook { get; set; }

[DynamoDBVersion]
public int? VersionNumber { get; set; }
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

indentation is off a bit


public enum Popularity
{
Low,
Expand Down
3 changes: 3 additions & 0 deletions Sources/Linq2DynamoDb.DataContext.Tests/Entities/BookPoco.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,9 @@ public class BookPoco {
[DynamoDBProperty(typeof(StringTimeSpanDictionaryConverter))]
public IDictionary<string, TimeSpan> FilmsBasedOnBook { get; set; }

[DynamoDBVersion]
public int? VersionNumber { get; set; }

public enum Popularity {
Low,

Expand Down
11 changes: 6 additions & 5 deletions Sources/Linq2DynamoDb.DataContext.Tests/EntityManagementTests/EntityCreationTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,17 +33,18 @@ public void DataContext_EntityCreation_PersistsRecordToDynamoDb()
Assert.IsNotNull(storedBook);
}

[Ignore("This behavior is currently expected. SubmitChanges() uses DocumentBatchWrite, which only supports PUT operations, which by default replaces existing entities")]
[Test]
[ExpectedException(typeof(InvalidOperationException), ExpectedMessage = "cannot be added, because entity with that key already exists", MatchType = MessageMatch.Contains)]
[ExpectedException(typeof(AggregateException))]
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hmmm i'd probably want to catch+flatten any aggregate exceptions that get thrown internally.
unless a method returns a Task (is Async), a user should not expect that method to throw an AggregateException.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ah just saw your comment in the PR description:

If a CUD operation fails we'll get an AggregateException with a ConditionalCheckFailedException but no detail about the entity for which the check failed. We should probably create our own exception for this.

yes. definitely want to catch and re-throw. AFAIK, the Dynamo API doesn't really tell you which expectation failed either, but we are only using conditions for document versioning, so we can assume it's a version mismatch, right?

public void DataContext_EntityCreation_ThrowsExceptionWhenEntityAlreadyExistsInDynamoDbButWasNeverQueriedInCurrentContext()
{
var book = BooksHelper.CreateBook(popularityRating: Book.Popularity.Average);
var persistedBook = BooksHelper.CreateBook(popularityRating: Book.Popularity.Average);

book.PopularityRating = Book.Popularity.High;
var bookCopy = BooksHelper.CreateBook(name: persistedBook.Name, publishYear: persistedBook.PublishYear, persistToDynamoDb: false);

bookCopy.PopularityRating = Book.Popularity.High;

var booksTable = this.Context.GetTable<Book>();
booksTable.InsertOnSubmit(book);
booksTable.InsertOnSubmit(bookCopy);
this.Context.SubmitChanges();
}

Expand Down
25 changes: 3 additions & 22 deletions Sources/Linq2DynamoDb.DataContext.Tests/EntityManagementTests/EntityModificationTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,9 @@
using Linq2DynamoDb.DataContext.Tests.Entities;
using Linq2DynamoDb.DataContext.Tests.Helpers;
using NUnit.Framework;
using System;
using System.Collections.Generic;
using System.Linq;

namespace Linq2DynamoDb.DataContext.Tests.EntityManagementTests
{
Expand Down Expand Up @@ -50,28 +52,7 @@ public void DataContext_EntityModification_UpdateRecordWithNewArray()

CollectionAssert.AreEquivalent(storedBook.RentingHistory, storedBookAfterModification.RentingHistory);
}

[Ignore("This behavior is currently expected. SubmitChanges() uses DocumentBatchWrite, which only supports PUT operations with default 'replace' behavior")]
[Test]
public void DataContext_EntityModification_UpdateShouldNotAffectFieldsModifiedFromOutside()
{
var book = BooksHelper.CreateBook(popularityRating: Book.Popularity.Average, persistToDynamoDb: false);

var booksTable = this.Context.GetTable<Book>();
booksTable.InsertOnSubmit(book);
this.Context.SubmitChanges();

// Update record from outside of DataTable
BooksHelper.CreateBook(book.Name, book.PublishYear, numPages: 15);

book.PopularityRating = Book.Popularity.High;
this.Context.SubmitChanges();

var storedBook = booksTable.Find(book.Name, book.PublishYear);
Assert.AreEqual(book.PopularityRating, storedBook.PopularityRating, "Record was not updated");
Assert.AreEqual(book.NumPages, 15, "Update has erased changes from outside");
}


[Test]
public void DataContext_UpdateEntity_UpdatesRecordWhenOldRecordIsNull()
{
Expand Down
14 changes: 0 additions & 14 deletions Sources/Linq2DynamoDb.DataContext.Tests/EntityManagementTests/Poco/PocoCreationTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,20 +33,6 @@ public void DataContext_EntityCreation_PersistsRecordToDynamoDb()
Assert.IsNotNull(storedBookPoco);
}

[Ignore("This behavior is currently expected. SubmitChanges() uses DocumentBatchWrite, which only supports PUT operations, which by default replaces existing entities")]
[Test]
[ExpectedException(typeof(InvalidOperationException), ExpectedMessage = "cannot be added, because entity with that key already exists", MatchType = MessageMatch.Contains)]
public void DataContext_EntityCreation_ThrowsExceptionWhenEntityAlreadyExistsInDynamoDbButWasNeverQueriedInCurrentContext()
{
var book = BookPocosHelper.CreateBookPoco(popularityRating: BookPoco.Popularity.Average);

book.PopularityRating = BookPoco.Popularity.High;

var booksTable = this.Context.GetTable<BookPoco>();
booksTable.InsertOnSubmit(book);
this.Context.SubmitChanges();
}

[Test]
[ExpectedException(typeof(InvalidOperationException), ExpectedMessage = "cannot be added, because entity with that key already exists", MatchType = MessageMatch.Contains)]
public void DataContext_EntityCreation_ThrowsExceptionWhenTryingToAddSameEntityTwice()
Expand Down
21 changes: 0 additions & 21 deletions Sources/Linq2DynamoDb.DataContext.Tests/EntityManagementTests/Poco/PocoModificationTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -51,27 +51,6 @@ public void DataContext_EntityModification_UpdateRecordWithNewArray()
CollectionAssert.AreEquivalent(storedBookPoco.RentingHistory, storedBookPocoAfterModification.RentingHistory);
}

[Ignore("This behavior is currently expected. SubmitChanges() uses DocumentBatchWrite, which only supports PUT operations with default 'replace' behavior")]
[Test]
public void DataContext_EntityModification_UpdateShouldNotAffectFieldsModifiedFromOutside()
{
var book = BookPocosHelper.CreateBookPoco(popularityRating: BookPoco.Popularity.Average, persistToDynamoDb: false);

var booksTable = this.Context.GetTable<BookPoco>();
booksTable.InsertOnSubmit(book);
this.Context.SubmitChanges();

// Update record from outside of DataTable
BookPocosHelper.CreateBookPoco(book.Name, book.PublishYear, numPages: 15);

book.PopularityRating = BookPoco.Popularity.High;
this.Context.SubmitChanges();

var storedBookPoco = booksTable.Find(book.Name, book.PublishYear);
Assert.AreEqual(book.PopularityRating, storedBookPoco.PopularityRating, "Record was not updated");
Assert.AreEqual(book.NumPages, 15, "Update has erased changes from outside");
}

[Test]
public void DataContext_UpdateEntity_UpdatesRecordWhenOldRecordIsNull()
{
Expand Down
91 changes: 91 additions & 0 deletions ...Linq2DynamoDb.DataContext.Tests/EntityManagementTests/Versioning/EntityVersioningTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,91 @@
using Linq2DynamoDb.DataContext.Tests.Entities;
using Linq2DynamoDb.DataContext.Tests.Helpers;
using NUnit.Framework;
using System;
using System.Collections.Generic;

namespace Linq2DynamoDb.DataContext.Tests.EntityManagementTests.Versioning
{
[TestFixture]
class EntityVersioningTests : DataContextTestBase
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

maybe add a test case for the scenario where you submit and THEN read and modify the entity again?
(in other words, the non-exceptional case. verify you can make 2 updates on a single entity and verify both changes as you make them).

{
public override void SetUp()
{
}

public override void TearDown()
{
}

[Test]
[ExpectedException(typeof(AggregateException))]
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is there any way we can catch expected exceptions on the line where we actually expect them?
I'm not a huge fan of catching this at the [Test] level.

Especially in this case, where we want to make sure we throw from the contextB.SubmitChanges() and not the contextA call.

public void DataContext_UpdateEntity_Does_OptimisticLocking()
{
var contextA = TestConfiguration.GetDataContext();
var contextB = TestConfiguration.GetDataContext();

var originalBook = BooksHelper.CreateBook(popularityRating: Book.Popularity.Low, rentingHistory: null);
var booksTableA = contextA.GetTable<Book>();
var booksTableB = contextB.GetTable<Book>();

// Read the same entry from the database into two contexts
var retrievedBookA = booksTableA.Find(originalBook.Name, originalBook.PublishYear);
var retrievedBookB = booksTableB.Find(originalBook.Name, originalBook.PublishYear);

// Mutate a property on instance A and persist
retrievedBookA.PopularityRating = Book.Popularity.Average;
contextA.SubmitChanges();

// Mutate a property on instance B (unaware of changes to A)
retrievedBookB.RentingHistory = new List<string> { "history element" };
contextB.SubmitChanges();
}

[Test]
[ExpectedException(typeof(AggregateException))]
public void DataContext_AddEntity_DoesNotOverwrite_ExistingVersionedEntity()
{
var contextA = TestConfiguration.GetDataContext();
var contextB = TestConfiguration.GetDataContext();

var tableA = contextA.GetTable<Book>();
var tableB = contextB.GetTable<Book>();

var bookA = BooksHelper.CreateBook(name: "A Tale of Two Books", publishYear: 0, persistToDynamoDb: false);
var bookB = BooksHelper.CreateBook(name: "A Tale of Two Books", publishYear: 0, persistToDynamoDb: false);


tableA.InsertOnSubmit(bookA);
contextA.SubmitChanges();

tableB.InsertOnSubmit(bookB);
contextB.SubmitChanges();
}

[Test]
[ExpectedException(typeof(AggregateException))]
public void DataContext_RemoveEntity_RespectsVersionConstraint()
{
var book = BooksHelper.CreateBook(numPages: 5, persistToDynamoDb: false);

var contextA = TestConfiguration.GetDataContext();
var contextB = TestConfiguration.GetDataContext();

var tableA = contextA.GetTable<Book>();
var tableB = contextB.GetTable<Book>();

// Insert the book in Context A
tableA.InsertOnSubmit(book);
contextA.SubmitChanges();

// Find and modify the book in Context B
var retrievedBook = tableB.Find(book.Name, book.PublishYear);
retrievedBook.NumPages = 10;
contextB.SubmitChanges();

// Try to delete the book from Context A
tableA.RemoveOnSubmit(book);
contextA.SubmitChanges();
}
}
}
2 changes: 1 addition & 1 deletion Sources/Linq2DynamoDb.DataContext.Tests/Helpers/BooksHelper.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,7 @@ public static void CleanSession()
{
Logger.DebugFormat("Removing {0} records from DynamoDb", _recordsForCleanup.Count);

Parallel.ForEach(_recordsForCleanup, book => PersistenceContext.Delete(book));
Parallel.ForEach(_recordsForCleanup, book => PersistenceContext.Delete(book, new DynamoDBOperationConfig { SkipVersionCheck = true, ConsistentRead = true }));

_recordsForCleanup = new ConcurrentQueue<Book>();
}
Expand Down
5 changes: 4 additions & 1 deletion Sources/Linq2DynamoDb.DataContext.Tests/Linq2DynamoDb.DataContext.Tests.csproj
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -90,6 +90,7 @@
<Compile Include="EntityManagementTests\Poco\PocoCreationTests.cs" />
<Compile Include="EntityManagementTests\Poco\PocoModificationTests.cs" />
<Compile Include="EntityManagementTests\Poco\PocoRemovalTests.cs" />
<Compile Include="EntityManagementTests\Versioning\EntityVersioningTests.cs" />
<Compile Include="Helpers\BookPocoHelper.cs" />
<Compile Include="Helpers\BooksHelper.cs" />
<Compile Include="Helpers\MemcachedController.cs" />
Expand Down Expand Up @@ -140,7 +141,9 @@
<SubType>Designer</SubType>
</None>
</ItemGroup>
<ItemGroup />
<ItemGroup>
<Service Include="{82A7F48D-3B50-4B1E-B82E-3ADA8210C358}" />
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is this an intentional change?

</ItemGroup>
<Import Project="$(MSBuildToolsPath)\Microsoft.CSharp.targets" />
<!-- To modify your build process, add your task inside one of the targets below and uncomment it.
Other similar extension points exist, see Microsoft.Common.targets.
Expand Down
2 changes: 2 additions & 0 deletions Sources/Linq2DynamoDb.DataContext/EntityProxy.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -131,6 +131,8 @@ public Document GetDocumentIfDirty()

public object Entity { get { return this.GetTransparentProxy(); } }

public Document AsDocument() { return _document; }

public void Commit() {}
}
}
Expand Down
50 changes: 49 additions & 1 deletion Sources/Linq2DynamoDb.DataContext/EntityWrapper.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,8 @@
using System.Linq;
using Amazon.DynamoDBv2.DocumentModel;
using Linq2DynamoDb.DataContext.Utils;
using System.Reflection;
using Amazon.DynamoDBv2.DataModel;

namespace Linq2DynamoDb.DataContext
{
Expand All @@ -14,6 +16,31 @@ internal class EntityWrapper : IEntityWrapper
private readonly IEntityKeyGetter _keyGetter;
private Document _doc, _newDoc;

private PropertyInfo _entityVersionNumberProperty;
private bool _hasResolvedEntityVersionNumberProperty;
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this could be a Lazy<bool>

Copy link
Owner

@scale-tone scale-tone Apr 2, 2017 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm afraid it wouldn't help either.
Technically, what is happening here now - is that the value for _entityVersionNumberProperty is calculated per each EntityWrapper instance. In effect, it's recalculated all the time (because EntityWrapper instances have very short lifes). Which is inefficient, and is exactly what we try to avoid throughout the library.

A typical approach to workaround this is demonstrated by TableDefinitionWrapper.ToDocumentConversionFunctor property. It contains a functor, that does a straightforward conversion from an entity instance to a Document. And that functor is a compiled Expression, so it works as fast, as if you manually wrote an entity-specific conversion code like

doc["Field1"] = entity.Field1;
doc["Field2"] = entity.Field2;
...

Another, less complicated, approach is used in EntityKeyGetter, where PropertyInfos are cached per TableDefinitionWrapper instance.

I would propose to use one of these two approaches, or just Memoize a version value getter per entity type.


/// <summary>
/// Gets PropertyInfo for the entity's property that has [DynamoDBVersion]
/// attribute or returns null if there is none.
/// </summary>
private PropertyInfo EntityVersionNumberProperty {
get {
if (!_hasResolvedEntityVersionNumberProperty) {
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

not sure about the project's braces style, but looks like you have mixed foo { and foo\n{ in this file's changes.

_entityVersionNumberProperty = Entity
.GetType()
.GetProperties(BindingFlags.Public | BindingFlags.Instance)
.Where(property =>
property
.GetCustomAttributes(typeof(DynamoDBVersionAttribute), true)
.SingleOrDefault() != null
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

.Any()?

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

or maybe you WANT to throw an exception if an entity contains more than one Version property.
is that even legal in dynamo's object model?

either way.... if your intention is to validate Single versioned property, maybe that should be done somewhere else or at least throw a useful exception if entities violate that constraint.

).SingleOrDefault();
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you can collapse that GetProperties(...).Where(predicate).SingleOrDefault() into GetProperties(...)SingleOrDefault(predicate)

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

also just realized there are 2 levels of validation you might want for an entity.

  1. should not have more than one [DynamoDbVersion] attribute on a single property (the inner SingleOrDefault). This actually should be enforced by the AttributeUsage of the attribute itself:
[AttributeUsage(AttributeTargets.Field | AttributeTargets.Property, Inherited = true, AllowMultiple = false)]
    public sealed class DynamoDBVersionAttribute : DynamoDBRenamableAttribute
    {
    }

so i don't think you need this inner check and can just use Any.

  1. should not have more than one property decorated with [DynamoDbVersion]. this is the outer SingleOrDefault and we should throw a nice exception if it's violated.


_hasResolvedEntityVersionNumberProperty = true;
}
return _entityVersionNumberProperty;
}
}

internal EntityWrapper(object entity, Func<object, Document> conversionFunctor, IEntityKeyGetter keyGetter)
{
this.Entity = entity;
Expand All @@ -38,14 +65,16 @@ internal EntityWrapper(Document doc, Type entityType, Func<object, Document> con
/// </summary>
internal bool IsCommited { get; private set; }

public Document AsDocument() { return this._conversionFunctor(this.Entity); }

/// <summary>
/// Returns a new document, if the entity was modified since the last call to this method.
/// Otherwise returns null.
/// </summary>
/// <returns></returns>
public Document GetDocumentIfDirty()
{
this._newDoc = this._conversionFunctor(this.Entity);
this._newDoc = AsDocument();

if (this._doc == null)
{
Expand Down Expand Up @@ -101,9 +130,28 @@ public void Commit()
{
this._doc = this._newDoc;
this._newDoc = null;
this.UpdateEntityVersionNumber();
this.IsCommited = true;
}

/// <summary>
/// Sets the value of the Entity's propety that has the DynamoDBVersionAttribute to
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

propety -> property

/// the value in _doc. When adding/updating the document the version number will
/// change in the _doc on the way to DynamoDB
/// </summary>
private void UpdateEntityVersionNumber()
{
if (EntityVersionNumberProperty == default(PropertyInfo))
{
return;
}

EntityVersionNumberProperty.SetValue(
Entity,
this._doc[EntityVersionNumberProperty.Name].ToObject(EntityVersionNumberProperty.PropertyType)
);
}

#region Redirecting Equals() and GetHashCode() to the underlying entity

public override bool Equals(object obj)
Expand Down
1 change: 1 addition & 0 deletions Sources/Linq2DynamoDb.DataContext/IEntityWrapper.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,5 +7,6 @@ public interface IEntityWrapper
object Entity { get; }
Document GetDocumentIfDirty();
void Commit();
Document AsDocument();
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No need in this method.

We only need to convert an entity into a Document, if that entity was modified or newly added. Simply because we only need to save modified Documents to storage, unchanged (and deleted) documents do not need to be saved. The GetDocumentIfDirty() method was specifically designed to underscore this.

If at some place of your code you found yourself wanting AsDocument() method, that rather signals a design flaw. And that's what we see in TableDefinitionWrapper, 230: no need to convert deleted entities into Documents, keys are enough for doing a delete.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For optimistic delete you also would need only keys and version field value, so no need in converting the whole entity even in that case.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed. We only need the value of the version property (if optimistically locking) instead of the whole document.

}
}
Loading