Skip to content

fix: implementing optimistic concurrency control#2331

Merged
nitrosx merged 11 commits intomasterfrom
if-unmodified-interceptor
Nov 26, 2025
Merged

fix: implementing optimistic concurrency control#2331
nitrosx merged 11 commits intomasterfrom
if-unmodified-interceptor

Conversation

@FloKo2669
Copy link
Contributor

@FloKo2669 FloKo2669 commented Nov 7, 2025

Description

This is a direct followup PR for #2138, addressing the change requests. Mainly introducing a helper function to reduce the code duplication in each controller function. The following Endpoints are modified:

  • Sample
  • Attachments
  • Datasets
  • datasets.v4
  • instruments
  • origdatablocks
  • origdatablocks.v4
  • proposals

The tests were left as they were in PR #2138, except for minor adjustments.

Changes compared to original PR:

  • introduced helper function to reduce code duplication
  • removed the check in pusblished dataset
  • uniform header access using request.headers
  • removed findByIdAndUpdateInternal in publish datasets

@FloKo2669 FloKo2669 requested a review from a team as a code owner November 7, 2025 09:39
@FloKo2669 FloKo2669 changed the title Follow-up PR for PR #2138: implement optimistic concurrency control fix: Follow-up PR for PR #2138: implement optimistic concurrency control Nov 7, 2025
@FloKo2669 FloKo2669 force-pushed the if-unmodified-interceptor branch from 7431c99 to 5b1c8b4 Compare November 7, 2025 13:17
@FloKo2669 FloKo2669 changed the title fix: Follow-up PR for PR #2138: implement optimistic concurrency control fix: implementing optimistic concurrency control Nov 10, 2025
@FloKo2669 FloKo2669 force-pushed the if-unmodified-interceptor branch from 5b1c8b4 to fc191d8 Compare November 10, 2025 09:24
@FloKo2669 FloKo2669 changed the title fix: implementing optimistic concurrency control fix: implementing optimistic concurrency control Nov 10, 2025
@FloKo2669 FloKo2669 force-pushed the if-unmodified-interceptor branch from fc191d8 to 3df7b19 Compare November 12, 2025 12:39
@FloKo2669 FloKo2669 force-pushed the if-unmodified-interceptor branch from 04a01fb to 557f31c Compare November 12, 2025 16:10
@FloKo2669 FloKo2669 force-pushed the if-unmodified-interceptor branch from bb58538 to 58266bb Compare November 13, 2025 09:06
@joe-baudisch joe-baudisch mentioned this pull request Nov 14, 2025
Closed
nitrosx
nitrosx reviewed Nov 17, 2025
View reviewed changes
Copy link
Member

@nitrosx nitrosx left a comment

Choose a reason for hiding this comment

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

Do we need a migration script for the db given that instruments are now inheriting from ownable?
Other than that, it is good to go for me.

@FloKo2669
Copy link
Contributor Author

FloKo2669 commented Nov 20, 2025

@nitrosx Valid point. However, I am not entirely sure that a migration script is needed, because the two newly added required fields are createdBy and updatedBy. createdBy is already set for Instrument (at least in the test data of SciCatLive). Both fields are set automatically during creation/update in the service, and to my understanding, they are only validated during these operations. However, I can see how it could lead to problems or unexpected behavior when updatedBy is not set.

I also found the migration script that sets updatedBy for other types (Attachment, Dataset, Proposal, etc.). So I added a migration script with the same logic for Instrument to be safe.
However, I am not an expert in MongoDB migrations, so I am happy for anyone to check if it works.

@FloKo2669 FloKo2669 linked an issue Nov 26, 2025 that may be closed by this pull request
Support for conditional requests/Strategy for not missing updates? #1969
Closed
@nitrosx nitrosx merged commit 99b32b1 into master Nov 26, 2025
13 checks passed
@nitrosx nitrosx deleted the if-unmodified-interceptor branch November 26, 2025 14:32
@cfelder cfelder mentioned this pull request Dec 22, 2025
Merged
nitrosx added a commit that referenced this pull request Feb 17, 2026
@nitrosx
fix: fixup optimistic concurrency control (#2431)
83402ea 
## Description

If the time send by the client is equal to the ressource on the server,
the clients updated version is based on servers version with that time.

or in other words:

the client has taken into account the ressource on the server b/c the
client should use the timestamp received with the ressource from the
server.

from the RFC:

> The origin server MUST NOT perform the requested method
> if the selected representation's last modification date is more
> recent than the date provided in the field-value

Client receives t0
Client sends update w/ t0 -> OK

Server is on t1
Client received t0 (previously)
Client sends update w/ t0 -> FAIL

## Motivation

The current implementation does not accept timestamps equal to the
server ressource as described above.

## Fixes

* Fixup for helper function in "fix: implementing optimistic concurrency
control (#2331)"
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@nitrosx nitrosx nitrosx approved these changes

@bpedersen2 bpedersen2 bpedersen2 approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Support for conditional requests/Strategy for not missing updates?

3 participants

@FloKo2669 @nitrosx @bpedersen2