sul-dlss / dor-services-app

A Rails application exposing Digital Object Registry functions as a RESTful HTTP API
https://sul-dlss.github.io/dor-services-app/
Other
3 stars 2 forks source link

[SPIKE] Optimistic locking #3568

Closed justinlittman closed 2 years ago

justinlittman commented 2 years ago

Cocina objects are immutable (unlike Fedora objects), which leads to the following bug:

  1. Service A loads cocina object 1.
  2. Service A invokes Service B and passes cocina object 1.
  3. Service B changes cocina object 1 by creating cocina object 1a with change X.
  4. Service B persists cocina object 1a.
  5. Service B completes and Service A resumes.
  6. Service A change cocina object 1 by creating cocina object 1b with change Y (that is to an unrelated part of cocina object to change X).
  7. Service A persists cocina object 1b <--- This overwrites change X.

One possible fix to this is optimistic locking:

  1. Service A loads cocina object 1 at v1.
  2. Service A invokes Service B and passes cocina object 1.
  3. Service B changes cocina object 1 by creating cocina object 1a with change X.
  4. Service B persists cocina object 1a. CocinaObjectStore checks that cocina object 1a was loaded from current version (v1) and when persisting changes to v2.
  5. Service B completes and Service A resumes.
  6. Service A change cocina object 1 by creating cocina object 1b with change Y (that is to an unrelated part of cocina object to change X).
  7. Service A persists cocina object 1b. CocinaObjectStore checks that cocina object 1b was loaded from current version (v2). It wasn't, so raises an error.

Note that versions for the purpose of optimistic locking shouldn't be conflated with any of the other notions of versions. While a locking version can be an integer, it doesn't have to be.

justinlittman commented 2 years ago

Though less concerning, this also begs the question about whether there should also optimistic locking at the API level to prevent different applications from overwriting changes.

jcoyne commented 2 years ago

This seems like a good use for ETag and If-Match headers: https://fideloper.com/etags-and-optimistic-concurrency-control

justinlittman commented 2 years ago

@jcoyne Just to avoid confusion: There are 2 use cases for this: (1) within DSA and (2) services updating via DSA. #3664 addresses 1, but not 2 so there is still additional work to be done.

jcoyne commented 2 years ago

@justinlittman can you give an example of how one would use optimistic locking within DSA? I'm not sure I understand the mechanism for that. As far as I can tell the description of this ticket (Service A, Service B, etc) only describes part 2.

justinlittman commented 2 years ago

Read Service as DSA service class, not service exposed by DSA API.

Recall that this is what motivated this work -- surfacing confusion over / coding errors related to the immutability of cocina objects rather than silently overwriting.

jcoyne commented 2 years ago

So, in rails you can add a lock_version (https://api.rubyonrails.org/classes/ActiveRecord/Locking/Optimistic.html) to your table and that takes care of it, but I have no idea how one would do this while supporting Fedora too.

We'd also need to switch to passing around AR records rather than cocina objects.

justinlittman commented 2 years ago

Here's my proposal:

Add additional schemas to cocina models, something like:

DROWithMetadata:
      type: object
      additionalProperties: false
      allOf:
        - $ref: "#/components/schemas/DRO"
        - type: object
          additionalProperties: false
          properties:
            lock:
              type: string
           modified:
              type: string
              format: date-time
           created:
              type: string
              format: date-time
          required:
            - lock
            - created
            - modified

This would be in a schema section that would NOT be copied to other openapi.yml.

The resulting object could be used interchangeably with a plain-old DRO; that is, it should not require modifying any code outside the CocinaObjectStore.

Within the CocinaObjectStore, lock, modified, and created will be set based on which datastore the object is retrieved from. (Passing around the modified and created will eliminate the need of the separate *_with_timestamp methods, but that is just a bonus.) When retrieved from Fedora, the modified date will be used as the lock. When retrieved from PG, the lock_version will be used as the lock (assuming that this can be made to work with our AR updates; otherwise, modified date can be used).

When updating and saving to PG, ideally AR magic can be used to check the lock; otherwise within a transaction, the modified date from the db will be checked against the lock. When not saving to PG (i.e., saving to Fedora only), the modified date from Fedora will be checked against the lock.

jcoyne commented 2 years ago

If this is internal to DSA, does it need to be in Cocina models?

justinlittman commented 2 years ago

No, but having it is Cocina models allows us to use the code generator to create a class that is compatible with Cocina::Models::DRO. This should minimize the code changes in DSA.

I actually suspect that the same might be useful in consumers of dor-services-client, as there needs to be a means of keeping track of the Etag lock.