-
-
Notifications
You must be signed in to change notification settings - Fork 159
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
bart-degreed
wants to merge
5
commits into
master
Choose a base branch
from
optimistic-concurrency
base: master
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Codecov Report
@@ 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
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. |
This was referenced Dec 1, 2021
bart-degreed
force-pushed
the
optimistic-concurrency
branch
from
December 1, 2021 11:17
ecfec2d
to
cee13f6
Compare
bkoelman
force-pushed
the
optimistic-concurrency
branch
from
April 11, 2022 08:56
cee13f6
to
934f96a
Compare
…ut IVersionedIdentifiable<TId, TVersion>
…h checked during update
bkoelman
force-pushed
the
optimistic-concurrency
branch
from
October 5, 2022 22:13
934f96a
to
c33677a
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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 asIJsonApiRequest.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:
Do not expose versions to clients, just fetch-latest before updating. This defeats the whole mechanism and may still fail on high load.
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.
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?