Fix:one-way relationship twoWayKey handling in createRelationship#842
Fix:one-way relationship twoWayKey handling in createRelationship#842HarshMN2345 wants to merge 3 commits intomainfrom
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThe duplicate-relationship check in Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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. Comment |
Greptile SummaryThis PR fixes a regression in Key concerns found:
Confidence Score: 2/5
Important Files Changed
|
![greptile-apps[bot]](https://avatars.gh.lumenfield.work/in/867647?s=60&v=4){kind=small}
| if ($relatedCollection->isEmpty()) { | ||
| throw new NotFoundException('Related collection not found'); | ||
| } | ||
|
|
There was a problem hiding this comment.
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:
| 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() | |
| ) { |
![coderabbitai[bot]](https://avatars.gh.lumenfield.work/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
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 | 🟡 MinorThe new default makes the later duplicate-update check inconsistent.
With
childrennow defaulting toparent, the laterupdateRelationship(... 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 sametwoWayKeysucceeds.🤖 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
📒 Files selected for processing (3)
src/Database/Database.phptests/e2e/Adapter/Scopes/Relationships/ManyToOneTests.phptests/e2e/Adapter/Scopes/Relationships/OneToOneTests.php
| $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']); |
There was a problem hiding this comment.
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).
There was a problem hiding this comment.
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 | 🔴 CriticalKeep one-to-one reverse indexes in sync when
twoWayflips.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 whentwoWay=true, sofalse -> trueneeds acreateIndex()path, andtrue -> falseneeds adeleteIndex()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
tagsattribute 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
📒 Files selected for processing (5)
src/Database/Database.phptests/e2e/Adapter/Scopes/Relationships/ManyToManyTests.phptests/e2e/Adapter/Scopes/Relationships/ManyToOneTests.phptests/e2e/Adapter/Scopes/Relationships/OneToManyTests.phptests/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
src/Database/Database.php
Outdated
| private function getRelationshipTwoWayKey(array|Document $relationship): string | ||
| { | ||
| $options = \is_array($relationship) | ||
| ? ($relationship['options'] ?? []) | ||
| : $relationship->getAttribute('options', []); | ||
|
|
||
| return $options['internalTwoWayKey'] ?? $options['twoWayKey'] ?? ''; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's locate and examine the method definition
fd 'Database.php' | head -20Repository: utopia-php/database
Length of output: 88
🏁 Script executed:
# Search for the getRelationshipTwoWayKey method definition
rg 'getRelationshipTwoWayKey' -A 8 -B 2Repository: utopia-php/database
Length of output: 11891
🏁 Script executed:
# Check all call sites of getRelationshipTwoWayKey
rg 'getRelationshipTwoWayKey\(' -B 2 -A 2Repository: 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 -50Repository: 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 5Repository: 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 -60Repository: 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 -40Repository: 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 -40Repository: utopia-php/database
Length of output: 2079
🏁 Script executed:
# Look at the specific call site in updateRelationship more carefully
rg 'updateRelationship\(' -A 50 | head -80Repository: 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 -30Repository: 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.phpRepository: 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 -30Repository: 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 -80Repository: 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 -50Repository: 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 3Repository: 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 -40Repository: 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.phpRepository: utopia-php/database
Length of output: 2341
🏁 Script executed:
# Check what type getAttribute returns for 'attributes'
rg 'function getAttribute' src/Database/Document.php -A 10Repository: 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 5Repository: 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 -20Repository: 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.
src/Database/Database.php
Outdated
| 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', []))) |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
tests/e2e/Adapter/Scopes/RelationshipTests.php (1)
673-687: Widened exception assertion is appropriate but consider a more descriptive assertion.The change correctly handles that one-way relationships (with
twoWay: false) no longer create atwoWayKeyattribute on the related collection. Setting a value for this non-existent attribute can be caught at different validation layers, resulting in either exception type.For improved test clarity and failure messages, consider using
assertThatwith a custom constraint or extracting a helper:💡 Optional: Improve assertion readability
} catch (Exception $e) { - $this->assertTrue($e instanceof RelationshipException || $e instanceof StructureException); + $this->assertTrue( + $e instanceof RelationshipException || $e instanceof StructureException, + 'Expected RelationshipException or StructureException, got ' . get_class($e) . ': ' . $e->getMessage() + ); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/e2e/Adapter/Scopes/RelationshipTests.php` around lines 673 - 687, Replace the terse assertTrue checks that an exception is either RelationshipException or StructureException with a more descriptive assertion message or helper to improve failure output: update the two occurrences where you currently call $this->assertTrue($e instanceof RelationshipException || $e instanceof StructureException) (inside the try/catch blocks around createDocument calls) to either call a helper like $this->assertRelationshipOrStructureException($e) or use $this->assertTrue(..., "Expected RelationshipException or StructureException, got ".get_class($e)) so failures show the actual exception class; add the helper method assertRelationshipOrStructureException(Exception $e) that asserts the instance types and includes the descriptive message.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@tests/e2e/Adapter/Scopes/RelationshipTests.php`:
- Around line 673-687: Replace the terse assertTrue checks that an exception is
either RelationshipException or StructureException with a more descriptive
assertion message or helper to improve failure output: update the two
occurrences where you currently call $this->assertTrue($e instanceof
RelationshipException || $e instanceof StructureException) (inside the try/catch
blocks around createDocument calls) to either call a helper like
$this->assertRelationshipOrStructureException($e) or use $this->assertTrue(...,
"Expected RelationshipException or StructureException, got ".get_class($e)) so
failures show the actual exception class; add the helper method
assertRelationshipOrStructureException(Exception $e) that asserts the instance
types and includes the descriptive message.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 609174b3-b857-4f49-88ad-b6a3f192d8ab
📒 Files selected for processing (2)
tests/e2e/Adapter/Scopes/RelationshipTests.phptests/e2e/Adapter/Scopes/Relationships/OneToOneTests.php
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/e2e/Adapter/Scopes/Relationships/OneToOneTests.php
HarshMN2345
force-pushed
the
fix-one-way-relationship-twowaykey
branch
from
7799a19 to
cff2b6e
Compare
HarshMN2345
force-pushed
the
fix-one-way-relationship-twowaykey
branch
from
630eb82 to
84ed291
Compare
|
@greptile |
| try { | ||
| $database->createRelationship( | ||
| collection: 'parent', | ||
| relatedCollection: 'child', | ||
| type: Database::RELATION_ONE_TO_MANY, | ||
| id: 'children', | ||
| twoWay: true, | ||
| id: 'twoWayChildren', | ||
| twoWayKey: 'parent_id' | ||
| ); | ||
| $this->fail('Failed to throw Exception'); | ||
| } catch (Exception $e) { | ||
| $this->assertEquals('Related attribute already exists', $e->getMessage()); | ||
| } |
There was a problem hiding this comment.
The DuplicateException is thrown here because childrenById (a one-way relationship) happens to carry the same twoWayKey value in its stored options. Since one-way relationships do not create a real reverse attribute in the related collection, the error message 'Related attribute already exists' is misleading — the attribute does not actually exist at the schema level.
More importantly, this test does not exercise the core guard: a genuine two-way vs two-way reverse-key collision, where the reverse attribute was actually created in the related collection. A stronger coverage would first create a twoWay: true relationship, then attempt to create a second twoWay: true relationship pointing at the same relatedCollection with an identical reverse key, and assert DuplicateException is thrown. Without that scenario, a future change could accidentally weaken the guard for real two-way key conflicts and the test suite would not catch the regression.
1 participant
Summary by CodeRabbit