Skip to content
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.

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

Explores support for concurrency tokens using PostgreSQL, continued #1119

Draft
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

bart-degreed
Copy link
Contributor

@bart-degreed bart-degreed commented Nov 30, 2021

This PR revisits the topic of supporting optimistic concurrency in JsonApiDotNetCore. An earlier design was proposed in #1004 and partially implemented in #917.

What is it?

When multiple clients update the same record, they potentially overwrite each others' changes, which goes unnoticed. If using optimistic concurrency, each record gets an extra concurrency token column (which we'll call "version" from now on), whose value automatically changes on each update. SQL Server users may know this as rowversion.

Now when the client updates a record, it passes the version which it obtained at the time the record was retrieved. The update fails if the incoming version differs from the stored one. When that happens, the client needs to retry the operation: fetch the latest version of the record, adapt it, and send a new update request.

Why?

Using optimistic concurrency prevents records, whose column values depend on other column values, to become inconsistent. For example, running totals. This is often used in financial systems, where the total amount of money must remain constant. Another case is dealing with existing models that require concurrency tokens, such as ASP.NET Authentication. When this level of consistency is required, it scales better compared to using high database transaction levels, which require either pessimistic locking or tracking version history.

An implementation journey...

As mentioned, an earlier proposal (#1004) explored ways to implement this. To avoid breaking changes, option 4 (Encode the version in the resource ID) seemed like the best approach at the time. But now that we're working on v5, where we can take breaking changes, I explored option 3 (Send the version in the request body and URL parameter) instead.

I started out by adding 'version' to request/response bodies and routes. Followed by passing a version parameter through the entire pipeline, but found this resulted in a lot of duplication: controllers, resource services, resource repositories, resource definitions, and a few more all needed to be split up into a versioned and a non-versioned variant. So instead I decided to make it a light-up feature and update existing components to take IVersionedIdentifiable into account, as well as IJsonApiRequest.PrimaryVersion.

This worked well, but while writing tests it occurred to me that sometimes incoming versions are ignored. For example, when updating a relationship, the client sends the version of the left resource and the right resource/resources. Now depending on where the foreign key is defined, some of those records don't get updated, so no version check occurs. First of all, the client has no way to know which versions are needed. But more importantly, it gives a false sense of correctness, because when a client sends: "attach customer A to order B", it would fail when the order has changed, but not if the customer has changed in-between.

Therefore I added an extra column to versioned resources, whose value is always overwritten on relationship updates. The value itself is irrelevant, but it triggers an update on the record, which results in a version check. This solved the problem: the system fails when any incoming version has become stale. Either way, for atomic:operations requests, we'll need to track versions internally because the client cannot always know them. For example, when creating two resources and linking them together. While writing tests for that, I stumbled upon the next problem: deleting a resource resulted in setting the foreign key value on a related entity to null, thus changing the version of that related resource. Version tracking did not detect that, which made me realize there's a broader problem here (explained in #1118): Writes may affect related resources that were never in the incoming request, so the client has no way to pass versions for them. Another issue with version tracking is that relationship endpoints never return data, so the version tracker (but also the API client!) cannot know what versions have become stale.

So there's no way in JSON:API for the client to send all the versions we may need, and there's no way to return all affected versions, in cases where related resources were involved that the request did not target.

Possible future directions:

  1. Minimal support, solely to unblock using existing models which contain concurrency tokens.
    Do not expose versions to clients, just fetch-latest before updating. This defeats the whole mechanism and may still fail on high load.
  2. Use versions, without an extra value column.
    Expose versions to clients, but be agnostic in request validation. Clients can send versions anywhere and they must know when to do that, depending on where the foreign key resides. Some stale versions don't result in a conflict.
  3. Strong validation of versions in incoming requests
    Fail when a version is missing and use the extra value column to ensure both sides get updated, which produces a conflict when any incoming version is stale. Make versions optional in atomic:operations requests. This results in more version changes than strictly needed, so the client needs to re-fetch more often.

For 2 and 3, the version tracking problem of #1118 remains. Assuming we can solve that, should we apply the strategy from 1 in such cases? Or extend the JSON:API protocol to allow sending in additional versions? Even if we do that, how would the client know what to pass?

@codecov
Copy link

codecov bot commented Nov 30, 2021

Codecov Report

Merging #1119 (0422d51) into master (ea3ab72) will decrease coverage by 0.68%.
The diff coverage is 75.27%.

❗ Current head 0422d51 differs from pull request most recent head c33677a. Consider uploading reports for the commit c33677a to get more accurate results

@@            Coverage Diff             @@
##           master    #1119      +/-   ##
==========================================
- Coverage   92.61%   91.92%   -0.69%     
==========================================
  Files         242      243       +1     
  Lines        7701     7939     +238     
==========================================
+ Hits         7132     7298     +166     
- Misses        569      641      +72     
Impacted Files Coverage Δ
...rc/JsonApiDotNetCore.Annotations/TypeExtensions.cs 91.66% <ø> (ø)
...JsonApiDotNetCore/Controllers/JsonApiController.cs 100.00% <ø> (ø)
...iDotNetCore/Queries/Internal/QueryLayerComposer.cs 92.78% <0.00%> (-6.49%) ⬇️
...e/Serialization/Response/ResourceObjectTreeNode.cs 75.26% <0.00%> (ø)
...onApiDotNetCore/AtomicOperations/VersionTracker.cs 30.30% <30.30%> (ø)
...DotNetCore/AtomicOperations/OperationsProcessor.cs 86.41% <50.00%> (-10.46%) ⬇️
...onApiDotNetCore/Services/JsonApiResourceService.cs 93.68% <76.92%> (-3.59%) ⬇️
...nApiDotNetCore/Resources/IdentifiableExtensions.cs 75.60% <82.14%> (+14.07%) ⬆️
...ontextExample/Repositories/DbContextARepository.cs 100.00% <100.00%> (ø)
...ontextExample/Repositories/DbContextBRepository.cs 100.00% <100.00%> (ø)
... and 41 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@bkoelman
Copy link
Member

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

2 participants