Skip to content

Fix:one-way relationship twoWayKey handling in createRelationship#842

Open
HarshMN2345 wants to merge 5 commits intomainfrom
fix-one-way-relationship-twowaykey
Open

Fix:one-way relationship twoWayKey handling in createRelationship#842
HarshMN2345 wants to merge 5 commits intomainfrom
fix-one-way-relationship-twowaykey

Conversation

@HarshMN2345
Copy link
Member

@HarshMN2345 HarshMN2345 commented Mar 19, 2026

Summary by CodeRabbit

  • Bug Fixes

    • One-way relationships no longer enforce or store a public inverse key; provided inverse keys are ignored. Two-way relationships continue to validate duplicate inverse keys. Relationship operations now use a stable internal key for consistent traversal and metadata updates.
  • Tests

    • Added and updated end-to-end tests to cover one-way vs two-way behaviors, ignored provided inverse keys, and duplicate-key validation for two-way relationships.

@coderabbitai
Copy link
Contributor

coderabbitai bot commented Mar 19, 2026

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: e971d7bf-4266-4980-ab73-85e38905460e

📥 Commits

Reviewing files that changed from the base of the PR and between af610c0 and 3844d74.

📒 Files selected for processing (1)
  • src/Database/Database.php
✅ Files skipped from review due to trivial changes (1)
  • src/Database/Database.php

📝 Walkthrough

Walkthrough

Adds an internal internalTwoWayKey separate from the public twoWayKey; updates relationship create/update/delete and traversal/population paths to use the internal key for stable identity while keeping twoWayKey only for two-way relationships. Tests updated to expect twoWayKey === null for one-way relationships.

Changes

Cohort / File(s) Summary
Core Database Logic
src/Database/Database.php
Introduce internalTwoWayKey derivation/accessor; prefer internalTwoWayKey for lookups and traversal; persist both internalTwoWayKey and twoWayKey (public only for two-way); adjust duplicate validation to apply only to two-way relationships; propagate internal key through metadata updates.
Many-to-One Tests
tests/e2e/Adapter/Scopes/Relationships/ManyToOneTests.php
Update testManyToOneOneWayRelationship to expect twoWayKey === null; add tests verifying one-way relationships ignore/allow duplicate twoWayKey values and two-way relationships still enforce duplicates.
One-to-One Tests
tests/e2e/Adapter/Scopes/Relationships/OneToOneTests.php
Adjust one-way relationship assertions to expect twoWayKey === null; remove redundant duplicate-conflict assertion.
Many-to-Many Tests
tests/e2e/Adapter/Scopes/Relationships/ManyToManyTests.php
Update one-way many-to-many test to expect twoWayKey === null; add test ensuring provided twoWayKey is ignored for one-way relationships.
One-to-Many Tests
tests/e2e/Adapter/Scopes/Relationships/OneToManyTests.php
Update one-way one-to-many test to expect twoWayKey === null; add test verifying provided twoWayKey is ignored when twoWay: false.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

Possibly related PRs

Suggested reviewers

  • abnegate
  • fogelito

Poem

"🐇 I hopped through code both bright and meek,
Found keys inside where names grew weak,
One-way keeps its secret tucked,
Two-way shows the name unstuck,
Hoppity-hop — metadata hide-and-seek!"

🚥 Pre-merge checks | ✅ 2 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 50.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (2 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title specifically addresses the main focus of the changes: fixing how one-way relationships handle the twoWayKey parameter in the createRelationship method, which aligns with the core implementation changes in Database.php and corresponding test updates.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch fix-one-way-relationship-twowaykey

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@greptile-apps
Copy link

greptile-apps bot commented Mar 19, 2026

Greptile Summary

This PR fixes a bug where creating a one-way (twoWay: false) relationship would incorrectly throw DuplicateException: Related attribute already exists when another one-way relationship to the same collection already existed, because the twoWayKey duplicate check was not gated on twoWay. The fix moves $twoWayKey ??= $collection->getId() to after the validation loop and adds a $twoWay guard, correctly restricting the twoWayKey conflict check to two-way relationships only.

Changes:

  • src/Database/Database.php: Defers the default $twoWayKey assignment until after the duplicate-detection loop, and adds $twoWay && + isset(...) guards to the twoWayKey conflict check.
  • tests/e2e/Adapter/Scopes/Relationships/ManyToOneTests.php: Removes an explicit twoWayKey: 'reviews' workaround that was required under the old behaviour; adds testManyToOneOneWayRelationshipDoesNotValidateTwoWayKeyDuplicates and testManyToOneTwoWayRelationshipStillValidatesTwoWayKeyDuplicates to cover both paths.
  • tests/e2e/Adapter/Scopes/Relationships/OneToOneTests.php: Removes the now-incorrect expectation that a one-way relationship with a default twoWayKey would throw DuplicateException, and updates asserted twoWayKey values accordingly.

Issue: Because $twoWayKey is no longer defaulted before the loop, it can be null when the loop runs. When $twoWay: true and $twoWayKey is omitted, \strtolower($twoWayKey) is called with null, which triggers a PHP 8.1+ deprecation warning (the project requires PHP ≥ 8.4). Additionally, strtolower(null) returns "", which will never match any stored key, so the twoWayKey duplicate guard is silently bypassed for two-way relationships that rely on the default key — a regression from the previous behaviour. Adding a $twoWayKey !== null guard to the condition would fix both issues.

Confidence Score: 3/5

  • Safe to merge only after addressing the strtolower(null) deprecation and the bypassed twoWayKey duplicate check for two-way relationships with a default key.
  • The core one-way relationship fix is correct and well-tested. However, deferring the $twoWayKey default assignment without guarding for null in the comparison introduces a PHP 8.4 deprecation warning and silently weakens duplicate detection for two-way relationships that rely on the default twoWayKey — an unintended regression.
  • src/Database/Database.php — the moved $twoWayKey ??= assignment and the null-unsafe strtolower call at line 3577.

Important Files Changed

Filename Overview
src/Database/Database.php Moves $twoWayKey ??= $collection->getId() after the duplicate-check loop and gates the twoWayKey conflict check on $twoWay. Correctly fixes false-positive duplicate errors for one-way relationships, but introduces a strtolower(null) deprecation warning and silently bypasses twoWayKey duplicate detection when $twoWay: true and $twoWayKey is not explicitly provided.
tests/e2e/Adapter/Scopes/Relationships/ManyToOneTests.php Removes explicit twoWayKey: 'reviews' and updates expected value to the collection ID default; adds two new tests verifying that one-way relationships allow duplicate twoWayKeys and two-way relationships still reject them.
tests/e2e/Adapter/Scopes/Relationships/OneToOneTests.php Removes the now-incorrect expectation that a one-way relationship with a default twoWayKey throws DuplicateException, and updates the asserted twoWayKey value from a custom key to the collection ID default.

Last reviewed commit: "Merge branch 'main' ..."

@@ -3577,8 +3577,6 @@ public function createRelationship(

Copy link

Choose a reason for hiding this comment

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

P1 strtolower(null) deprecation in PHP 8.4

Since $twoWayKey ??= $collection->getId() was moved to after the loop, $twoWayKey can now be null at line 3577 when a caller passes twoWay: true but omits twoWayKey. Calling \strtolower(null) triggers a deprecation warning in PHP 8.1+ and the project requires PHP >= 8.4. This also silently breaks the duplicate twoWayKey guard for two-way relationships when the default key is used: strtolower(null) === "" will never match any stored twoWayKey string, so the loop won't detect a conflict, and two two-way relationships pointing at the same collection with the same default key could be created without throwing DuplicateException.

Adding a $twoWayKey !== null guard preserves the intended duplicate check and avoids the deprecation notice:

Suggested change
if (
$twoWay
&& $twoWayKey !== null
&& $attribute->getAttribute('type') === self::VAR_RELATIONSHIP
&& isset($attribute->getAttribute('options')['twoWayKey'])
&& \strtolower($attribute->getAttribute('options')['twoWayKey']) === \strtolower($twoWayKey)
&& $attribute->getAttribute('options')['relatedCollection'] === $relatedCollection->getId()
) {

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 2

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)
tests/e2e/Adapter/Scopes/Relationships/OneToOneTests.php (1)

1027-1042: ⚠️ Potential issue | 🟡 Minor

The new default makes the later duplicate-update check inconsistent.

With children now defaulting to parent, the later updateRelationship(... newTwoWayKey: 'parent') call is a no-op, not a conflicting change. This either makes the test fail once idempotent updates are allowed, or it locks in the wrong behavior. Please change that later assertion to use a genuinely conflicting key, or assert that reapplying the same twoWayKey succeeds.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@tests/e2e/Adapter/Scopes/Relationships/OneToOneTests.php` around lines 1027 -
1042, The test becomes inconsistent because the collection 'children' now
defaults its twoWayKey to 'parent', making the later call updateRelationship(...
newTwoWayKey: 'parent') a no-op; update the test in OneToOneTests.php to either
(a) change the later updateRelationship call to use a genuinely conflicting key
(e.g. 'otherParent') and assert the conflict/changed value, or (b) if the intent
is to allow idempotent updates, change the assertion to verify that reapplying
newTwoWayKey: 'parent' succeeds (no-op) — locate the createRelationship call and
the later updateRelationship(...) invocation and adjust the assertion for the
'children' attribute accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@src/Database/Database.php`:
- Around line 3588-3592: The condition uses strtolower($twoWayKey) while
$twoWayKey can be null, causing deprecation/TypeError; ensure $twoWayKey is
initialized to an empty string (or safe default) before the loop that checks
$attribute and $attribute->getAttribute('options')['twoWayKey'] so strtolower is
never passed null. Update the code that sets/accepts $twoWayKey (the variable
used in the two-way duplicate scan condition involving $twoWay,
self::VAR_RELATIONSHIP, $attribute->getAttribute('options')['twoWayKey'], and
$relatedCollection->getId()) to default $twoWayKey to '' when null or missing so
the existing comparison logic remains unchanged and avoids strtolower(null).

In `@tests/e2e/Adapter/Scopes/Relationships/ManyToOneTests.php`:
- Around line 53-64: The test currently asserts absence of the old backreference
key 'reviews' on the movie payload but the related-side metadata was renamed to
'review', so update the hidden-backreference assertion to check the new key
name: replace the assertArrayNotHasKey('reviews', $movie) check with
assertArrayNotHasKey('review', $movie) (look for usages of assertArrayNotHasKey
and the $movie variable to locate and update the assertion).

---

Outside diff comments:
In `@tests/e2e/Adapter/Scopes/Relationships/OneToOneTests.php`:
- Around line 1027-1042: The test becomes inconsistent because the collection
'children' now defaults its twoWayKey to 'parent', making the later call
updateRelationship(... newTwoWayKey: 'parent') a no-op; update the test in
OneToOneTests.php to either (a) change the later updateRelationship call to use
a genuinely conflicting key (e.g. 'otherParent') and assert the conflict/changed
value, or (b) if the intent is to allow idempotent updates, change the assertion
to verify that reapplying newTwoWayKey: 'parent' succeeds (no-op) — locate the
createRelationship call and the later updateRelationship(...) invocation and
adjust the assertion for the 'children' attribute accordingly.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: 3a0b6d17-3486-481d-bec7-1c647e6fcff1

📥 Commits

Reviewing files that changed from the base of the PR and between f121418 and 504c45e.

📒 Files selected for processing (3)
  • src/Database/Database.php
  • tests/e2e/Adapter/Scopes/Relationships/ManyToOneTests.php
  • tests/e2e/Adapter/Scopes/Relationships/OneToOneTests.php

Comment on lines +53 to +64
$this->assertEquals('review', $attribute['options']['twoWayKey']);
}
}

// Check metadata for related collection
$collection = $database->getCollection('movie');
$attributes = $collection->getAttribute('attributes', []);
foreach ($attributes as $attribute) {
if ($attribute['key'] === 'reviews') {
if ($attribute['key'] === 'review') {
$this->assertEquals('relationship', $attribute['type']);
$this->assertEquals('reviews', $attribute['$id']);
$this->assertEquals('reviews', $attribute['key']);
$this->assertEquals('review', $attribute['$id']);
$this->assertEquals('review', $attribute['key']);
Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟡 Minor

Update the hidden-backreference assertions to the renamed key.

The related-side metadata now uses review, but the later assertArrayNotHasKey('reviews', $movie) checks still verify the old key. That means this test no longer catches regressions that accidentally expose the one-way backreference under its new name.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@tests/e2e/Adapter/Scopes/Relationships/ManyToOneTests.php` around lines 53 -
64, The test currently asserts absence of the old backreference key 'reviews' on
the movie payload but the related-side metadata was renamed to 'review', so
update the hidden-backreference assertion to check the new key name: replace the
assertArrayNotHasKey('reviews', $movie) check with
assertArrayNotHasKey('review', $movie) (look for usages of assertArrayNotHasKey
and the $movie variable to locate and update the assertion).

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 2

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)
src/Database/Database.php (1)

3991-4014: ⚠️ Potential issue | 🔴 Critical

Keep one-to-one reverse indexes in sync when twoWay flips.

These metadata updates can rename the reverse attribute between the generated internal key and the public key, but the one-to-one index path later only handles rename-in-place. createRelationship() only creates the reverse unique index when twoWay=true, so false -> true needs a createIndex() path, and true -> false needs a deleteIndex() path. Otherwise the toggle either fails on a missing reverse index or leaves stale reverse-index metadata behind.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@src/Database/Database.php` around lines 3991 - 4014, The metadata update
handles renames but doesn't create or remove the one-to-one reverse index when
twoWay flips; update the logic around updateAttributeMeta (the blocks that
modify the attribute and $twoWay reverse attribute) to detect a change from
false->true and call the code path that creates the reverse unique index
(reuse/createIndex behavior from createRelationship) and detect true->false to
remove the reverse unique index (call deleteIndex path), ensuring you reference
the same attribute IDs/keys ($actualNewKey, $actualNewTwoWayKey, $oldTwoWayKey)
when creating or deleting the index so the physical index state stays in sync
with the updated options.
🧹 Nitpick comments (2)
tests/e2e/Adapter/Scopes/Relationships/OneToManyTests.php (1)

416-418: Harden relationship lookup to avoid undefined-offset failures.

Line 416 directly indexes [0] after filtering. If setup regresses, this can fail before a useful assertion. Add an explicit count/assert first for clearer diagnostics.

Proposed test-hardening diff
-        $relationship = \array_values(\array_filter($attributes, fn (Document $attribute) => $attribute->getId() === 'books'))[0];
-
-        $this->assertEquals(null, $relationship['options']['twoWayKey']);
+        $matches = \array_values(\array_filter($attributes, fn (Document $attribute) => $attribute->getId() === 'books'));
+        $this->assertCount(1, $matches, 'Expected exactly one relationship attribute with id "books"');
+        $relationship = $matches[0];
+        $this->assertEquals(null, $relationship['options']['twoWayKey']);
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@tests/e2e/Adapter/Scopes/Relationships/OneToManyTests.php` around lines 416 -
418, The test currently assumes the filtered array contains an element by doing
array_values(...)[0], which can produce an undefined-offset error; modify the
lookup in the test around $relationship and $attributes (the filtering closure
using Document::getId and the resulting $relationship variable) to first capture
the filtered/array_values result into a variable, assert that it is not empty
(e.g., assertNotEmpty or assertCount > 0) and only then access the [0] element,
then proceed to assertEquals(null, $relationship['options']['twoWayKey']); this
ensures a clear diagnostic if the relationship is missing.
tests/e2e/Adapter/Scopes/Relationships/ManyToManyTests.php (1)

353-356: Avoid unguarded [0] access after filtering.

Line [355] can fail with a less-informative undefined offset if no tags attribute is found. Add an explicit assertion before dereferencing.

Suggested test-hardening diff
-        $relationship = \array_values(\array_filter($attributes, fn (Document $attribute) => $attribute->getId() === 'tags'))[0];
+        $matches = \array_values(\array_filter(
+            $attributes,
+            fn (Document $attribute) => $attribute->getId() === 'tags'
+        ));
+        $this->assertNotEmpty($matches, 'Relationship attribute "tags" was not created on course.');
+        $relationship = $matches[0];
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@tests/e2e/Adapter/Scopes/Relationships/ManyToManyTests.php` around lines 353
- 356, The code uses array_values(array_filter(...))[0] to find the 'tags'
attribute which will throw an undefined offset if not found; update the test
around $collection, $attributes, and the $relationship extraction to assert that
the filtered result is not empty before dereferencing (e.g., check
count(array_filter(...)) > 0 or assertNotEmpty on the filtered array / use
current() safely) so the test fails with a clear assertion message instead of an
undefined offset error.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@src/Database/Database.php`:
- Around line 3939-3942: The duplicate-check currently treats the existing
reverse attribute as a duplicate because actualPublicNewTwoWayKey can equal the
current related attribute key; update the condition so you first exclude the
attribute whose 'key' equals the existing counterpart from
relatedCollection->getAttribute('attributes', []) (e.g. filter out
attribute['key'] === $actualPublicNewTwoWayKey or the existing counterpart key
variable) and then check if actualPublicNewTwoWayKey exists among the remaining
attributes; this prevents metadata-only updates (onDelete, etc.) from raising a
DuplicateException while keeping the duplicate detection for other attributes.
- Around line 3550-3556: The method getRelationshipTwoWayKey currently accepts
array|Document but only Document is used; change its signature to accept
Document $relationship (remove the array union) and simplify the body to
retrieve options via $relationship->getAttribute('options', []) and return
$options['internalTwoWayKey'] ?? $options['twoWayKey'] ?? ''; update any
docblock/type hints referencing array|Document to Document to satisfy PHPStan
and avoid the dead-code is_array branch.

---

Outside diff comments:
In `@src/Database/Database.php`:
- Around line 3991-4014: The metadata update handles renames but doesn't create
or remove the one-to-one reverse index when twoWay flips; update the logic
around updateAttributeMeta (the blocks that modify the attribute and $twoWay
reverse attribute) to detect a change from false->true and call the code path
that creates the reverse unique index (reuse/createIndex behavior from
createRelationship) and detect true->false to remove the reverse unique index
(call deleteIndex path), ensuring you reference the same attribute IDs/keys
($actualNewKey, $actualNewTwoWayKey, $oldTwoWayKey) when creating or deleting
the index so the physical index state stays in sync with the updated options.

---

Nitpick comments:
In `@tests/e2e/Adapter/Scopes/Relationships/ManyToManyTests.php`:
- Around line 353-356: The code uses array_values(array_filter(...))[0] to find
the 'tags' attribute which will throw an undefined offset if not found; update
the test around $collection, $attributes, and the $relationship extraction to
assert that the filtered result is not empty before dereferencing (e.g., check
count(array_filter(...)) > 0 or assertNotEmpty on the filtered array / use
current() safely) so the test fails with a clear assertion message instead of an
undefined offset error.

In `@tests/e2e/Adapter/Scopes/Relationships/OneToManyTests.php`:
- Around line 416-418: The test currently assumes the filtered array contains an
element by doing array_values(...)[0], which can produce an undefined-offset
error; modify the lookup in the test around $relationship and $attributes (the
filtering closure using Document::getId and the resulting $relationship
variable) to first capture the filtered/array_values result into a variable,
assert that it is not empty (e.g., assertNotEmpty or assertCount > 0) and only
then access the [0] element, then proceed to assertEquals(null,
$relationship['options']['twoWayKey']); this ensures a clear diagnostic if the
relationship is missing.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: a33b325d-1393-4bfd-bc1d-43fe4593bce0

📥 Commits

Reviewing files that changed from the base of the PR and between 504c45e and af610c0.

📒 Files selected for processing (5)
  • src/Database/Database.php
  • tests/e2e/Adapter/Scopes/Relationships/ManyToManyTests.php
  • tests/e2e/Adapter/Scopes/Relationships/ManyToOneTests.php
  • tests/e2e/Adapter/Scopes/Relationships/OneToManyTests.php
  • tests/e2e/Adapter/Scopes/Relationships/OneToOneTests.php
🚧 Files skipped from review as they are similar to previous changes (1)
  • tests/e2e/Adapter/Scopes/Relationships/ManyToOneTests.php

Comment on lines +3550 to +3556
private function getRelationshipTwoWayKey(array|Document $relationship): string
{
$options = \is_array($relationship)
? ($relationship['options'] ?? [])
: $relationship->getAttribute('options', []);

return $options['internalTwoWayKey'] ?? $options['twoWayKey'] ?? '';
Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟠 Major

🧩 Analysis chain

🏁 Script executed:

# First, let's locate and examine the method definition
fd 'Database.php' | head -20

Repository: utopia-php/database

Length of output: 88


🏁 Script executed:

# Search for the getRelationshipTwoWayKey method definition
rg 'getRelationshipTwoWayKey' -A 8 -B 2

Repository: utopia-php/database

Length of output: 11891


🏁 Script executed:

# Check all call sites of getRelationshipTwoWayKey
rg 'getRelationshipTwoWayKey\(' -B 2 -A 2

Repository: utopia-php/database

Length of output: 5764


🏁 Script executed:

# Find the updateRelationship method to see what type $attributes has
rg 'public function updateRelationship' -A 30 | head -50

Repository: utopia-php/database

Length of output: 2859


🏁 Script executed:

# Look for where $oldAttribute is being used and its type context
rg '\$oldAttribute = \$attributes' -B 10 -A 5

Repository: utopia-php/database

Length of output: 1262


🏁 Script executed:

# Check the type hint for $attributes parameter in updateRelationship
rg 'updateRelationship\(' -B 3 -A 15 | grep -A 15 'function updateRelationship'

Repository: utopia-php/database

Length of output: 7528


🏁 Script executed:

# Check what Collection::getAttribute returns for 'attributes'
rg 'class Collection' -A 30 | head -60

Repository: utopia-php/database

Length of output: 45


🏁 Script executed:

# Look at a few more call sites to understand what types are actually passed
rg 'getRelationshipTwoWayKey\(' -B 5 -A 2 | grep -A 2 -B 5 'processedRelationship\|relationship\[' | head -40

Repository: utopia-php/database

Length of output: 2626


🏁 Script executed:

# Let me check what the actual attributes look like in the collection
# Search for where attributes are set on Collection
rg "setAttribute.*'attributes'" -B 2 -A 2 | head -40

Repository: utopia-php/database

Length of output: 2079


🏁 Script executed:

# Look at the specific call site in updateRelationship more carefully
rg 'updateRelationship\(' -A 50 | head -80

Repository: utopia-php/database

Length of output: 5621


🏁 Script executed:

# Check if there's a phpstan config and any reported errors
fd -e 'phpstan.neon' -o -e 'phpstan.neon.dist'

Repository: utopia-php/database

Length of output: 179


🏁 Script executed:

# Look for Document class definition to understand its structure
rg 'class Document' -A 10 | head -30

Repository: utopia-php/database

Length of output: 1771


🏁 Script executed:

# Get the exact context around the updateRelationship call to getRelationshipTwoWayKey
sed -n '3900,3970p' src/Database/Database.php

Repository: utopia-php/database

Length of output: 3126


🏁 Script executed:

# Check if getAttribute returns Document objects or arrays for attributes
rg 'getAttribute.*attributes' -A 1 -B 1 | grep -A 3 -B 3 'getAttribute.*attributes.*\[\]' | head -30

Repository: utopia-php/database

Length of output: 2432


🏁 Script executed:

# Look for the Document class and its ArrayAccess implementation
rg 'class Document' -A 50 | grep -A 50 'class Document'

Repository: utopia-php/database

Length of output: 19878


🏁 Script executed:

# Search for ArrayAccess or magic methods in Document
rg 'ArrayAccess|__get|__set|offsetGet|offsetSet' src/

Repository: utopia-php/database

Length of output: 45


🏁 Script executed:

# Look at the actual line where getAttribute is called and what it returns
rg 'getAttribute\(.attributes' -B 2 -A 5 | head -80

Repository: utopia-php/database

Length of output: 5424


🏁 Script executed:

# Check if attributes are stored as Document objects or plain arrays
rg 'setAttribute.*attributes.*Document\|append' -B 2 -A 2 | head -50

Repository: utopia-php/database

Length of output: 45


🏁 Script executed:

# Look for where attributes are set and what type they are
rg 'setAttribute.*attributes.*APPEND' -B 3 -A 3

Repository: utopia-php/database

Length of output: 2346


🏁 Script executed:

# Check how attributes are constructed and what type is stored
rg 'attributes\[\].*=' -A 2 -B 2 | head -40

Repository: utopia-php/database

Length of output: 2265


🏁 Script executed:

# Look at one complete flow - examine lines around the updateRelationship call to understand types better
sed -n '3895,3945p' src/Database/Database.php

Repository: utopia-php/database

Length of output: 2341


🏁 Script executed:

# Check what type getAttribute returns for 'attributes'
rg 'function getAttribute' src/Database/Document.php -A 10

Repository: utopia-php/database

Length of output: 601


🏁 Script executed:

# Verify whether Document objects are what's actually stored in the attributes array
rg 'relationships as \$relationship' -B 5 -A 5

Repository: utopia-php/database

Length of output: 5322


🏁 Script executed:

# Final check: confirm the attributes array actually contains Document objects
rg "setAttribute\('attributes'" -A 1 -B 1 | grep -i document | head -20

Repository: utopia-php/database

Length of output: 1250


Tighten getRelationshipTwoWayKey() parameter type for PHPStan compliance.

This method is private and all call sites pass Document objects exclusively. The array|Document union type is dead code that triggers PHPStan's iterable-value-type error and can be safely removed.

Suggested fix
-    private function getRelationshipTwoWayKey(array|Document $relationship): string
+    private function getRelationshipTwoWayKey(Document $relationship): string
     {
-        $options = \is_array($relationship)
-            ? ($relationship['options'] ?? [])
-            : $relationship->getAttribute('options', []);
+        $options = $relationship->getAttribute('options', []);
 
         return $options['internalTwoWayKey'] ?? $options['twoWayKey'] ?? '';
     }
🧰 Tools
🪛 GitHub Actions: CodeQL

[error] 3550-3550: PHPStan (level 7) error: Method Utopia\Database\Database::getRelationshipTwoWayKey() has parameter $relationship with no value type specified in iterable type array.


[error] 3550-3550: PHPStan (level 7) error: Method Utopia\Database\Database::getRelationshipTwoWayKey() has parameter $relationship with no value type specified in iterable type array|Utopia\Database\Document.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@src/Database/Database.php` around lines 3550 - 3556, The method
getRelationshipTwoWayKey currently accepts array|Document but only Document is
used; change its signature to accept Document $relationship (remove the array
union) and simplify the body to retrieve options via
$relationship->getAttribute('options', []) and return
$options['internalTwoWayKey'] ?? $options['twoWayKey'] ?? ''; update any
docblock/type hints referencing array|Document to Document to satisfy PHPStan
and avoid the dead-code is_array branch.

Comment on lines 3939 to +3942
if (
!\is_null($newTwoWayKey)
&& \in_array($newTwoWayKey, \array_map(fn ($attribute) => $attribute['key'], $relatedCollection->getAttribute('attributes', [])))
$actualTwoWay
&& !\is_null($actualPublicNewTwoWayKey)
&& \in_array($actualPublicNewTwoWayKey, \array_map(fn ($attribute) => $attribute['key'], $relatedCollection->getAttribute('attributes', [])))
Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🔴 Critical

Exclude the existing counterpart from this duplicate check.

When the reverse key is unchanged, actualPublicNewTwoWayKey resolves to the current related attribute, so this in_array() turns metadata-only updates like onDelete changes into a false DuplicateException.

🩹 Suggested fix
-        if (
-            $actualTwoWay
-            && !\is_null($actualPublicNewTwoWayKey)
-            && \in_array($actualPublicNewTwoWayKey, \array_map(fn ($attribute) => $attribute['key'], $relatedCollection->getAttribute('attributes', [])))
-        ) {
-            throw new DuplicateException('Related attribute already exists');
-        }
+        if ($actualTwoWay && !\is_null($actualPublicNewTwoWayKey)) {
+            foreach ($relatedCollection->getAttribute('attributes', []) as $relatedAttribute) {
+                if (
+                    $relatedAttribute->getAttribute('key') === $actualPublicNewTwoWayKey
+                    && $relatedAttribute->getId() !== $oldTwoWayKey
+                ) {
+                    throw new DuplicateException('Related attribute already exists');
+                }
+            }
+        }
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@src/Database/Database.php` around lines 3939 - 3942, The duplicate-check
currently treats the existing reverse attribute as a duplicate because
actualPublicNewTwoWayKey can equal the current related attribute key; update the
condition so you first exclude the attribute whose 'key' equals the existing
counterpart from relatedCollection->getAttribute('attributes', []) (e.g. filter
out attribute['key'] === $actualPublicNewTwoWayKey or the existing counterpart
key variable) and then check if actualPublicNewTwoWayKey exists among the
remaining attributes; this prevents metadata-only updates (onDelete, etc.) from
raising a DuplicateException while keeping the duplicate detection for other
attributes.

Sign up for free to 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.

1 participant