Repository navigation
Relationship field validation does not respect the current transaction #4078
Description
Link to reproduction
https://github.com/diesieben07/payload/tree/repro/relationship-validation-transactions
Describe the Bug
The validation code for filterOptions on relationship fields does not respect the current transaction. As a result, you cannot create an object which has a relationship to an object that was created in the same transaction, because the validation code will not find the previously created object, because it is checking without a transaction.
The reason for this is that validateFilterOptions does not use (or receive) the PayloadRequest and does not pass it to the find operation it does.
There may be other validation functions that have the same issue.
To Reproduce
- Create a collection X and a collection Y, where Y has a relationship field to X with
filterOptionsset (can be an empty object). - Start a transaction.
- Create object A of collection X.
- Create object B of collection Y with a relationship to A
Step 4 will fail with a validation error, claiming that A is not a valid option.
It will work fine when not using transactions.
Given the above reproduction link, this can be reproduced as follows:
docker compose up -d, this starts a MongoDB ReplicaSetexport PAYLOAD_TEST_MONGO_URL='mongodb://127.0.0.1/payloadtests?replicaSet=payload'pnpm run test:int _community
Payload Version
2.1.0
Adapters and Plugins
db-mongodb, db-postgres
Activity
diesieben07 commentedon Nov 9, 2023
I am currently working on a pull request to fix this issue.
BrianJM commentedon Feb 22, 2024
@diesieben07 I ran into this issue when trying to debug #4350. With transactions enabled, the error still occurs.
diesieben07 commentedon Feb 22, 2024
@BrianJM that is quite likely. I discovered while working on #4087 (and other PRs I did regarding transactions) that in my honest opinion the way transactions were implemented into the Payload API is fundamentally flawed. I spent a lot of time trying to work around it and fix it up, but ultimately could not dedicate any more time to it a) because it is no longer relevant to my work and b) I just got frustrated by it.
DanRibbens commentedon Feb 22, 2024
Sorry for the frustrations, I've been working on other things but I am making some breakthroughs.
BrianJM commentedon Feb 22, 2024
@diesieben07 I am looking into this more. If you have anything else to share, I'd love to hear more about the struggles you ran into. I'm on the discord server as well.
diesieben07 commentedon Feb 23, 2024
@BrianJM Here is how transactions work in Payload at the moment:
Every operation (
find,findByID,create,update, etc.) does its own transaction management. To do this, the code for every operation includes the following boilerplate:Now this on its own would not be very useful, because you've essentially invented autocommit mode. Therefor every operation1 also has a
reqparameter of typePayloadRequest, which stores the transaction ID. Therefor if multiple operations use the samePayloadRequest, they can coordinate - right? Well, you run into a bunch of race conditions if you do this with the naive implementation above:transactionIDwas removed from thePayloadRequestby committing it). They might even fail, and now part of their work has been committed, even though they failed.I think most of this was also fixed by fix: fixes more race conditions with transactions #4164. Now operations only commit transactions that they themself have started. If you run operations in parallel, this still requires the foresight to start a transaction before you run them in parallel, otherwise you could still have the situation where the operation who "wins" at starting the transaction is also the first to finish - committing the transaction before the others are done.
PayloadRequestobject no longer has atransactionID, meaning everything else that is still running will not be rolled back, but instead just start a new transaction and commit that. This will either fail with integrity constraints (because part of the work was rolled back) or cause inconsistent data in the database (again, because part of the work was rolled back).Even if point 3 is fixed, in my opinion the whole design is still brittle and error-prone and unlike anything I have ever seen in other systems that use database transactions. If you forget to pass along the
PayloadRequestat every step of the way things go wrong (see #4087 for an example in Payload and it happend constantly in my own application code). If you forget to adhere to the exact calling requirements (see point 2 above, you must start a transaction yourself if you are going to call multiple things in parallel), things go wrong. And the way things go wrong is not at all fail-fast. If things go wrong, basically anything can happen:reqor the transaction got killed out from under you.And if you try again, it most likely worked, because everything is full of race conditions.
I wish you the best of luck in your endeavors, but I am no longer interested in trying to make this system work.
Footnotes
A bunch were missing this parameter, fixed in fix: make sure all local operations accept a
PayloadRequestparameter #3651. ↩xHomu commentedon Feb 24, 2024
Thanks for the detailed breakdown, @diesieben07, that was very informative!
BrianJM commentedon Feb 25, 2024
This defect also exists in 2.11.1. #5079 may fix this.
wkentdag commentedon Jun 17, 2024
I think the root cause of this bug might be related to #6800 (pending fix in #6821)
github-actions commentedon Sep 7, 2024
This issue has been automatically locked.
Please open a new issue if this issue persists with any additional detail.