Skip to content

add checking of nested docs id in comparison check when updating document - #304

Merged
abnegate merged 5 commits into
mainfrom
fix-5404-check-related-doc-id
Aug 10, 2023
Merged

add checking of nested docs id in comparison check when updating document#304
abnegate merged 5 commits into
mainfrom
fix-5404-check-related-doc-id

Conversation

@fanatic75

Copy link
Copy Markdown
Contributor

This PR checks the nested docs id to the old doc nested docs.
For example:

If document A is related to document B but we do not have permission to access it, we can still establish a relationship between document A and a different version of document B (let's call it B1). This is because we compare B to its previous version, and B1 to its previous version as well. If there are no changes in B1, then we do not need permission to relate it to document A.

@abnegateabnegate left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Let's see if we can simplify some of the conditions to make it a bit easier to read

Comment threadsrc/Database/Database.php Outdated
Comment threadsrc/Database/Database.php Outdated
$relationType = (string) $relationships[$key]['options']['relationType'];
$side = (string) $relationships[$key]['options']['side'];
switch($relationType) {
case Database::RELATION_ONE_TO_ONE: {

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

We don't need the curly braces around the switch cases

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

can we keep them? It allows me to minimize the whole case at once without curly brackets, it's not possible.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Let's remove them, they will be inconsistent with other switches, and we shouldn't change code style just for IDE features.

If you're using VS Code, you could change the folding strategy to allow minimizing the whole case without changing code

Comment threadsrc/Database/Database.php Outdated
Comment threadsrc/Database/Database.php Outdated
Comment threadsrc/Database/Database.php Outdated

@abnegateabnegate left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Nice looks much better, just a few more style changes 👍

Comment threadsrc/Database/Database.php Outdated
Comment on lines +2936 to +2937
(\is_null($oldValue) && !\is_null($value)) ||
(!is_null($oldValue) && \is_null($value)) ||

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

These two can just be (\is_null($value) !== \is_null($oldValue))

Comment threadsrc/Database/Database.php Outdated
$shouldUpdate = true;
break;
}
} else {

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Let's break from the if instead of nesting in an else

Comment threadsrc/Database/Database.php Outdated
$side = (string) $relationships[$key]['options']['side'];
switch($relationType) {
case Database::RELATION_ONE_TO_ONE: {
$oldValue = $old->getAttribute($key) instanceof Document ? $old->getAttribute($key)->getId() : $old->getAttribute($key);

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Let's line break to make ternarys easier to read:

Suggested change
$oldValue = $old->getAttribute($key) instanceof Document ? $old->getAttribute($key)->getId() : $old->getAttribute($key);
$oldValue = $old->getAttribute($key) instanceof Document
? $old->getAttribute($key)->getId()
: $old->getAttribute($key);

Comment threadsrc/Database/Database.php Outdated
break;
}
foreach ($value as $index=>$relation) {
$oldValue = $old->getAttribute($key)[$index] instanceof Document ? $old->getAttribute($key)[$index]->getId() : $old->getAttribute($key)[$index];

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Let's line break this too

Comment threadsrc/Database/Database.php Outdated
foreach ($value as $index=>$relation) {
$oldValue = $old->getAttribute($key)[$index] instanceof Document ? $old->getAttribute($key)[$index]->getId() : $old->getAttribute($key)[$index];
if (
(\is_string($relation) && $oldValue!==$relation) ||

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Suggested change
(\is_string($relation) && $oldValue!==$relation) ||
(\is_string($relation) && $oldValue!==$relation) ||

Comment threadsrc/Database/Database.php Outdated
Comment on lines +2955 to +2956
(\is_null($oldValue) && !\is_null($value)) ||
(!is_null($oldValue) && \is_null($value)) ||

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

These two can just be (\is_null($value) !== \is_null($oldValue))

Comment threadsrc/Database/Database.php Outdated
Comment on lines +2965 to +2966
(\is_null($old->getAttribute($key)) && !\is_null($value)) ||
(!is_null($old->getAttribute($key)) && \is_null($value)) ||

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

These two can just be (\is_null($value) !== \is_null($oldValue))

Comment threadsrc/Database/Database.php Outdated
foreach ($document as $key=>$value) {
// Skip the nested documents as they will be checked later in recursions.
if (array_key_exists($key, $relationshipKeys)) {
if (array_key_exists($key, $relationships)) {

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Suggested change
if (array_key_exists($key, $relationships)) {
if (\array_key_exists($key, $relationships)) {

Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants

@fanatic75@abnegate