-
Notifications
You must be signed in to change notification settings - Fork 13
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
IBX-8426: Fixed duplicate relations #390
Conversation
@alongosz
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm missing here integration test coverage (not a unit one) reproducing the use case from the JIRA ticket on API.
That would help me determining if the fix is done in a proper way and will make it future-proof when other changes gets applied.
thanks @alongosz - will try to get this done - and ping you when I have questions. |
@alongosz : integration test has been added |
781b113
to
d8fdddb
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the test case, after debugging it I see what's the root cause here. Seems we either:
- have a missing feature when loading a
$content
item byinternalLoadContentById
- RelationList field should never load on API level an item which user has no permissions to access - might be difficult to implement on the relevant layer level though, - are updating relations based on too heavy objects - this can negatively impact performance - if we don't touch relation field in an update, it should not be updated at all
both of these things however are too complex to mitigate them via this issue. I have another suggestion as a quick fix for our faulty architecture.
// Side note: please rebase, there are some PHPStan issues that were solved on 4.6 yesterday.
tests/integration/Core/Repository/ContentService/UpdateContentTest.php
Outdated
Show resolved
Hide resolved
tests/integration/Core/Repository/ContentService/UpdateContentTest.php
Outdated
Show resolved
Hide resolved
7ea59f6
to
7737be3
Compare
@@ -1421,7 +1421,7 @@ protected function internalUpdateContent( | |||
)->id, | |||
] | |||
); | |||
$existingRelations = $this->internalLoadRelations($versionInfo); | |||
$existingRelations = $this->repository->sudo(fn (): array => $this->internalLoadRelations($versionInfo)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Review note: it's done like that because there are some architecture issues that are not easy to fix, see #390 (review)
tests/integration/Core/Repository/ContentService/UpdateContentTest.php
Outdated
Show resolved
Hide resolved
Co-authored-by: Paweł Niedzielski <[email protected]>
Quality Gate passedIssues Measures |
Description:
PR adds a "permission switch" tointernalLoadRelations()
.maintainer update: changed to use sudo as all of them are needed for the update process, see review notes.
Background:
When a user changes Content with related Content without having read permission to the related Content, existing relations are duplicated in
ezcontentobject_link
table.https://github.com/ibexa/core/blob/4.6/src/lib/Repository/ContentService.php#L1432
processFieldRelations()
adds all relations from$inputRelations
that do not alreaday exist in$existingRelations
.$existingRelations
are load frominternalLoadRelations()
- skipping non-readable Content