Aliases for Dotted attribute not getting removed during decode in sql

[ArnabChatterjee20k]

'a.b' getting stored as 'ab' in sql
during decode we are setitng the 'a.b' but not removing the 'ab' -> structure validation error during update

Summary by CodeRabbit

• Bug Fixes

  • Fixed document attribute resolution to correctly handle null-valued attributes and ensure only user-visible attribute keys are exposed in document retrieval results, preventing unintended internal storage references.

• Tests

  • Added comprehensive end-to-end test coverage validating attribute filtering behavior and proper key exposure across document creation, retrieval, and multiple update operations.

[coderabbitai]

📝 Walkthrough

Walkthrough

The Database::decode() method now resolves null-valued attributes using the adapter-filtered key first, and removes filtered-key attributes from decoded documents when they differ from original keys. A new e2e test validates that dotted attribute IDs expose only schema keys without SQL storage aliases.

Changes

Cohort / File(s)	Summary
Database Decoding Logic src/Database/Database.php	Modified decode() method to resolve null-valued attributes using adapter-filtered key first; removes filtered-key attributes if they differ from original keys to eliminate SQL alias keys from decoded documents.
E2E Test for Attribute Filtering tests/e2e/Adapter/Scopes/DocumentTests.php	Added testDottedAttributeKeyGetDocumentExposesOnlySchemaKeys() test validating that dotted attribute IDs are exposed only with schema keys, not SQL storage aliases, across create and update operations.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Possibly related PRs

• fix relationship top level filter #715 — Both PRs modify how adapter-filtered keys and values are resolved and applied in Database::decode(), affecting attribute key exposure in decoded documents.
• Fix encoding decoding #639 — Both PRs alter Database::decode() behavior with implications for attribute handling and document filtering.
• Fix update documents #646 — Related changes to Database::decode() method regarding attribute filtering and removal during decoding.

Suggested reviewers

• abnegate

Poem

🐰 With fuzzy paws on keys so keen,
We filter out what's not been seen,
No aliases shall lurk about,
Just schema keys—no SQL clout! ✨

🚥 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 directly addresses the main change: fixing alias removal for dotted attributes during SQL decode operations, which matches the core objective of the PR.

_{✏️ 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 dot-attribute-update-fix

---

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

• X
• Mastodon
• Reddit
• LinkedIn

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

[greptile-apps]

Greptile Summary

This PR fixes a bug in the SQL adapter's decode() path where dotted attribute keys (e.g. a.b) were being stored in the DB under a filtered alias (e.g. ab). During decode, when the optional attribute's column was NULL, the alias key was never removed from the in-memory document, leaving both ab (null) and a.b (null) present — triggering a structure-validation error on subsequent updates. The fix adds an elseif branch that uses offsetExists() (which behaves like array_key_exists, returning true even for null values) to detect and remove the alias key regardless of whether the stored value is null or non-null.

Confidence Score: 5/5

Safe to merge — the fix is surgical, logically correct, and covered by a new e2e test.

The one tricky invariant (that ArrayObject::offsetExists returns true for null-valued keys, unlike isset) is exactly what makes the elseif work correctly, and the accompanying test exercises the null-column path directly. No other call sites are affected.

No files require special attention.

Important Files Changed

Filename	Overview
src/Database/Database.php	Adds an elseif to the decode loop that removes the SQL-filtered alias key (e.g. ab) when the attribute value is null, preventing spurious stale keys from breaking structure validation.
tests/e2e/Adapter/Scopes/DocumentTests.php	Adds a focused e2e test that verifies getDocument and updateDocument never expose the filtered storage alias for a dotted attribute, covering both null and non-null value scenarios.

_{Reviews (1): Last reviewed commit: "refactor: streamline attribute filtering..." | Re-trigger Greptile}

[coderabbitai]

🧹 Nitpick comments (1)

tests/e2e/Adapter/Scopes/DocumentTests.php (1)

6690-6743: Make teardown unconditional with try/finally

If any assertion fails before the last line, the collection is left behind. Wrapping the body in try/finally keeps test isolation deterministic.

♻️ Proposed refactor

 $database->createCollection($collectionId);
- $this->assertTrue($database->createAttribute($collectionId, $attrId, Database::VAR_STRING, 128, false));-- // Optional attribute omitted: DB column is NULL — decode must not leave the SQL column name as a key.- $database->createDocument($collectionId, new Document([- '$id' => 'dev1',- '$permissions' => [- Permission::read(Role::any()),- Permission::update(Role::any()),- ],- ]));-- $doc = $database->getDocument($collectionId, 'dev1');- $this->assertSame('dev1', $doc->getId());- $this->assertNull($doc->getAttribute($attrId));- $this->assertArrayNotHasKey($filteredStorageKey, $doc->getAttributes());- $userKeys = array_keys($doc->getAttributes());- sort($userKeys);- $this->assertSame([$attrId], $userKeys);+ try {+ $this->assertTrue($database->createAttribute($collectionId, $attrId, Database::VAR_STRING, 128, false));++ // Optional attribute omitted: DB column is NULL — decode must not leave the SQL column name as a key.+ $database->createDocument($collectionId, new Document([+ '$id' => 'dev1',+ '$permissions' => [+ Permission::read(Role::any()),+ Permission::update(Role::any()),+ ],+ ]));++ $doc = $database->getDocument($collectionId, 'dev1');+ $this->assertSame('dev1', $doc->getId());+ $this->assertNull($doc->getAttribute($attrId));+ $this->assertArrayNotHasKey($filteredStorageKey, $doc->getAttributes());+ $userKeys = array_keys($doc->getAttributes());+ sort($userKeys);+ $this->assertSame([$attrId], $userKeys);- $updated = $database->updateDocument($collectionId, 'dev1', new Document([- $attrId => '1.0.0',- ]));- $this->assertSame('1.0.0', $updated->getAttribute($attrId));- $this->assertArrayNotHasKey($filteredStorageKey, $updated->getAttributes());- $userKeys = array_keys($updated->getAttributes());- sort($userKeys);- $this->assertSame([$attrId], $userKeys);+ $updated = $database->updateDocument($collectionId, 'dev1', new Document([+ $attrId => '1.0.0',+ ]));+ $this->assertSame('1.0.0', $updated->getAttribute($attrId));+ $this->assertArrayNotHasKey($filteredStorageKey, $updated->getAttributes());+ $userKeys = array_keys($updated->getAttributes());+ sort($userKeys);+ $this->assertSame([$attrId], $userKeys);- $doc = $database->getDocument($collectionId, 'dev1');- $this->assertSame('1.0.0', $doc->getAttribute($attrId));- $this->assertArrayNotHasKey($filteredStorageKey, $doc->getAttributes());- $userKeys = array_keys($doc->getAttributes());- sort($userKeys);- $this->assertSame([$attrId], $userKeys);+ $doc = $database->getDocument($collectionId, 'dev1');+ $this->assertSame('1.0.0', $doc->getAttribute($attrId));+ $this->assertArrayNotHasKey($filteredStorageKey, $doc->getAttributes());+ $userKeys = array_keys($doc->getAttributes());+ sort($userKeys);+ $this->assertSame([$attrId], $userKeys);- $updated = $database->updateDocument($collectionId, 'dev1', new Document([- $attrId => '2.0.0',- ]));- $this->assertSame('2.0.0', $updated->getAttribute($attrId));- $this->assertArrayNotHasKey($filteredStorageKey, $updated->getAttributes());- $userKeys = array_keys($updated->getAttributes());- sort($userKeys);- $this->assertSame([$attrId], $userKeys);+ $updated = $database->updateDocument($collectionId, 'dev1', new Document([+ $attrId => '2.0.0',+ ]));+ $this->assertSame('2.0.0', $updated->getAttribute($attrId));+ $this->assertArrayNotHasKey($filteredStorageKey, $updated->getAttributes());+ $userKeys = array_keys($updated->getAttributes());+ sort($userKeys);+ $this->assertSame([$attrId], $userKeys);- $doc = $database->getDocument($collectionId, 'dev1');- $this->assertSame('2.0.0', $doc->getAttribute($attrId));- $this->assertArrayNotHasKey($filteredStorageKey, $doc->getAttributes());- $userKeys = array_keys($doc->getAttributes());- sort($userKeys);- $this->assertSame([$attrId], $userKeys);-- $database->deleteCollection($collectionId);+ $doc = $database->getDocument($collectionId, 'dev1');+ $this->assertSame('2.0.0', $doc->getAttribute($attrId));+ $this->assertArrayNotHasKey($filteredStorageKey, $doc->getAttributes());+ $userKeys = array_keys($doc->getAttributes());+ sort($userKeys);+ $this->assertSame([$attrId], $userKeys);+ } finally {+ $database->deleteCollection($collectionId);+ }

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.
In `@tests/e2e/Adapter/Scopes/DocumentTests.php` around lines 6690 - 6743, Test
teardown currently runs only at the end so if an assertion fails the created
collection remains; wrap the test body that runs between
$database->createCollection(...) and $database->deleteCollection($collectionId)
in a try/finally and move the $database->deleteCollection($collectionId) call
into the finally block to guarantee cleanup; locate the sequence using the calls
to $database->createCollection, $database->createAttribute, multiple
$database->getDocument/$database->updateDocument, and
$database->deleteCollection and ensure the finally still runs even if earlier
assertions fail (optionally check that $collectionId is set before calling
deleteCollection).

🤖 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/DocumentTests.php`:
- Around line 6690-6743: Test teardown currently runs only at the end so if an
assertion fails the created collection remains; wrap the test body that runs
between $database->createCollection(...) and
$database->deleteCollection($collectionId) in a try/finally and move the
$database->deleteCollection($collectionId) call into the finally block to
guarantee cleanup; locate the sequence using the calls to
$database->createCollection, $database->createAttribute, multiple
$database->getDocument/$database->updateDocument, and
$database->deleteCollection and ensure the finally still runs even if earlier
assertions fail (optionally check that $collectionId is set before calling
deleteCollection).

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: cff49971-6174-46ba-bebf-9e05ce68c9b3

📥 Commits

Reviewing files that changed from the base of the PR and between 72ee161 and 3564deb.

📒 Files selected for processing (2)

• src/Database/Database.php
• tests/e2e/Adapter/Scopes/DocumentTests.php