Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Sign up
Appearance settings

Explores support for concurrency tokens using PostgreSQL #917

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

Closed
bart-degreed wants to merge 1 commit into master from concurrency-tokens

Conversation

Copy link
Contributor

@bart-degreed bart-degreed commented Jan 10, 2021
edited
Loading

Related to #350.

Update (2021年11月08日): Rebased on latest changes in master branch

Because we fetch the row before update and apply changes on that, a concurrency violation is only reported when two concurrent requests update the same row in parallel. Instead, we want to produce an error if the token sent by the user does not match the stored token. To do that, we need to convince EF Core to use that as original version. That's not too hard.

Now the problem is that there is no way to send the token for relationships or deleting a resource. Skipped tests have been added to demonstrate this.

We could fetch such related rows upfront to work around that, but that kinda defeats the purpose of using concurrency tokens in the first place. It may be more correct to fail when a user is trying to add a related resource that has changed since it was fetched. This reasoning may be a bit too puristic and impractical, but at least that's how EF Core seems to handle it.

Solutions considerations:

  • Add 'version' to resource identifier object, so the client can send it. The spec does not explicitly forbid adding custom fields, however 'meta' would probably be the recommended approach. Instead of extending the definition, we could encode it in the StringId.
  • Once we have access to that token value, we need to somehow map that to 'the' resource property. What if there are multiple concurrency token properties on a resource? And depending on the database used, this could be typed as numeric, guid, timestamp, binary or something else.
  • Given that PostgreSQL uses a number (uint xmin), should we obfuscate or even encrypt that? If the latter, we need to add an option for api developers to set the encryption key.

See also:
json-api/json-api#600
json-api/json-api#824

Copy link
Contributor Author

bart-degreed commented Jan 29, 2021
edited
Loading

To solve the second and third bullet, we can leave it up to the resource class itself to choose the matching property and apply the conversion. Something along the lines of:

// JADNC
public interface IVersionedIdentifiable : IIdentifiable
{
 string Version { get; set; }
}
public abstract class VersionedIdentifiable<TId> : Identifiable<TId>, IVersionedIdentifiable
{
 public abstract string Version { get; set; }
}
// API Project
public sealed class Disk : VersionedIdentifiable<long>
{
 [Attr]
 public string Manufacturer { get; set; }
 [Attr]
 public string SerialCode { get; set; }
 [ConcurrencyCheck]
 [Timestamp]
 public uint xmin { get; set; }
 public override string Version
 {
 get => xmin.ToString();
 set => xmin = uint.Parse(value);
 }
}

Copy link
Contributor Author

bart-degreed commented Feb 4, 2021
edited
Loading

Add 'version' to resource identifier object

This won't work for DELETE resource requests, because no body is being sent. The same applies for updating a relationship via relationship endpoints. We could decide on a separator that is not allowed to occur in ID or version. For example, choosing '@' would allow:

DELETE /photos/123@789 HTTP/1.1

which we can then interpret as: resource with ID 123 at version 789. Perhaps '@' is not the best choice, in case someone uses email addresses for IDs.

Update (May 2021):

The section on path component in RFC for URLs contains:

For example, one URI producer might use a segment such as "name;v=1.1" to indicate a reference to version 1.1 of "name".

Splitting on ";v=" sounds safe enough, IDs are unlikely to ever contain that value.

@bart-degreed bart-degreed force-pushed the concurrency-tokens branch 3 times, most recently from 19110ee to 5d0ecc6 Compare November 9, 2021 17:37
Copy link

codecov bot commented Nov 9, 2021
edited
Loading

Codecov Report

Merging #917 (eacfc83) into master (15f5923) will increase coverage by 0.02%.
The diff coverage is 100.00%.

Impacted file tree graph

@@ Coverage Diff @@
## master #917 +/- ##
==========================================
+ Coverage 89.00% 89.03% +0.02% 
==========================================
 Files 249 250 +1 
 Lines 7123 7141 +18 
==========================================
+ Hits 6340 6358 +18 
 Misses 783 783 
Impacted Files Coverage Δ
...onApiDotNetCore/Errors/DataConcurrencyException.cs 100.00% <100.00%> (ø)
...Core/Repositories/EntityFrameworkCoreRepository.cs 99.28% <100.00%> (+0.03%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 15f5923...eacfc83. Read the comment docs.

*Update (2021年11月08日): Rebased on latest changes in master branch*
Because we fetch the row before update and apply changes on that, a concurrency violation is only reported when two concurrent requests update the same row in parallel. Instead, we want to produce an error if the token sent by the user does not match the stored token. To do that, we need to convince EF Core to use that as original version. That's not too hard.
Now the problem is that there is no way to send the token for relationships or deleting a resource. Skipped tests have been added to demonstrate this.
We could fetch such related rows upfront to work around that, but that kinda defeats the purpose of using concurrency tokens in the first place. It may be more correct to fail when a user is trying to add a related resource that has changed since it was fetched. This reasoning may be a bit too puristic and impractical, but at least that's how EF Core seems to handle it.
Solutions considerations:
- Add 'version' to resource identifier object, so the client can send it. The spec does not explicitly forbid adding custom fields, however 'meta' would probably be the recommended approach. Instead of extending the definition, we could encode it in the StringId.
- Once we have access to that token value, we need to somehow map that to 'the' resource property. What if there are multiple concurrency token properties on a resource? And depending on the database used, this could be typed as numeric, guid, timestamp, binary or something else.
- Given that PostgreSQL uses a number (uint xmin), should we obfuscate or even encrypt that? If the latter, we need to add an option for api developers to set the encryption key.
See also:
json-api/json-api#600
json-api/json-api#824 
Copy link
Contributor Author

Superseded by #1119.

@bart-degreed bart-degreed deleted the concurrency-tokens branch December 1, 2021 11:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

1 participant

AltStyle によって変換されたページ (->オリジナル) /