Fix:one-way relationship twoWayKey handling in createRelationship#842
Fix:one-way relationship twoWayKey handling in createRelationship#842HarshMN2345 wants to merge 5 commits intomainfrom
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
✅ Files skipped from review due to trivial changes (1)
📝 WalkthroughWalkthroughAdds an internal Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
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 docstrings
🧪 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 bug where creating a one-way ( Changes:
Issue: Because Confidence Score: 3/5
Important Files Changed
Last reviewed commit: "Merge branch 'main' ..." |
| @@ -3577,8 +3577,6 @@ public function createRelationship( | |||
|
|
|||
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() | |
| ) { |
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
| 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.
| 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.
Summary by CodeRabbit
Bug Fixes
Tests