Internal attributes defaults #608
Conversation
…ttributes-defaults # Conflicts: # src/Database/Adapter/SQL.php
WalkthroughThese changes refactor how internal metadata attributes are handled and exposed in the database layer. Internal attributes are now consistently prefixed with Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Database
participant SQLAdapter
Client->>Database: getDocument(id, selections)
Database->>SQLAdapter: getDocument(id, selections)
SQLAdapter->>SQLAdapter: getAttributeProjection(selections, prefix)
SQLAdapter->>SQLAdapter: addHiddenAttribute(selections)
SQLAdapter->>SQLAdapter: Build SELECT with user and hidden attributes
SQLAdapter->>Database: Return document with main::$ attributes
Database->>Database: Format timestamps, set main::$collection, etc.
Database->>Client: Return document with main::$ attributes
Possibly related PRs
Poem
📜 Recent review detailsConfiguration used: .coderabbit.yaml 📒 Files selected for processing (4)
🚧 Files skipped from review as they are similar to previous changes (4)
⏰ Context from checks skipped due to timeout of 90000ms (11)
✨ Finishing Touches
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 9
🔭 Outside diff range comments (1)
src/Database/Database.php (1)
4616-4646: Remove leftovervar_dump/ debug statementsRaw debugging output in production path will break JSON responses and logs.
- var_dump($related); - // ... several var_dump lines ...Delete all
var_dump(...)calls or wrap behind a logger flag.Also applies to: 4750-4760, 4860-4870
🧹 Nitpick comments (7)
src/Database/Document.php (1)
186-190:getAttributes()– spacing nit & style error flagged by PintThe CI linter fails with a PSR-12 brace-position warning.
Adding a space after the closing parenthesis resolves both readability and Pint:- if (Database::isInternalAttribute($attribute)){ + if (Database::isInternalAttribute($attribute)) {tests/e2e/Adapter/Scopes/RelationshipTests.php (1)
902-904: Remove commented-out debugging artefact
//$species = $database->getDocument('species', $species->getId());slipped into the committed code.
Keeping dead code in tests clutters the file history and may confuse future contributors.- //$species = $database->getDocument('species', $species->getId());src/Database/Database.php (5)
529-546:isInternalAttribute()– accept but watch for false-positivesLogic is OK, yet
str_contains($attribute, $attr['$id'])may mark user-defined keys like"user_$id_copy"as internal.
Consider stricter matching (e.g.preg_match('/(^|::)\$id$/')).
3357-3359: Redundant date formatting
main::$createdAt/updatedAtwere already written inDateTime::formatTzform; calling it again is harmless but unnecessary.
4180-4187: Skip-internal check usesmain::prefixPrefix comparison is
"{$alias}::"; internal keys are"main::$". Works now but will skip keys likemain::foothat aren’t internal. Harmless, yet maybe compare::\$to stay precise.
4331-4338: Redundant attribute mirroring duplicated with createDocumentConsider extracting to helper (
applyInternalMirrors($document)) to DRY.
6359-6373: Encoding strip logic OK but micro-optimisableLoop + temp array is fine; using
foreach ($document as $k=>$_) if (strpos($k,'::$')!==false) unset($document[$k]);avoids second pass.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
src/Database/Adapter/SQL.php(4 hunks)src/Database/Database.php(18 hunks)src/Database/Document.php(3 hunks)tests/e2e/Adapter/Scopes/DocumentTests.php(9 hunks)tests/e2e/Adapter/Scopes/RelationshipTests.php(9 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (2)
tests/e2e/Adapter/Scopes/RelationshipTests.php (1)
src/Database/Query.php (2)
Query(8-730)select(463-466)
src/Database/Database.php (2)
src/Database/Document.php (3)
getRead(91-94)getAttribute(207-214)getPermissions(83-86)src/Database/Query.php (2)
getAttribute(120-123)Query(8-730)
🪛 GitHub Actions: Linter
src/Database/Document.php
[error] 1-1: PHP CS Fixer (Pint) PSR-12 style issue: 'braces_position' violation.
src/Database/Adapter/SQL.php
[error] 1-1: PHP CS Fixer (Pint) PSR-12 style issue: 'braces_position' violation.
tests/e2e/Adapter/Scopes/DocumentTests.php
[error] 1-1: PHP CS Fixer (Pint) PSR-12 style issue: 'statement_indentation' violation.
src/Database/Database.php
[error] 1-1: PHP CS Fixer (Pint) PSR-12 style issues: 'braces_position' and 'statement_indentation' violations.
🪛 GitHub Actions: CodeQL
src/Database/Database.php
[error] 4151-4151: PHPDoc tag @var has invalid value ($old Document): Unexpected token "$old", expected type at offset 24
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Setup & Build Docker Image
🔇 Additional comments (16)
src/Database/Document.php (1)
135-146: Tiny optimisation + type safety ingetPermissionsByType()
- Skip the
str_replacewhen$permissionclearly does not start with$type.- Cast the return value of
str_replacetostringto silence Psalm/phpstan about mixed.- foreach ($this->getPermissions($attribute) as $permission) { - if (!\str_starts_with($permission, $type)) { - continue; - } - $typePermissions[] = \str_replace([$type . '(', ')', '"', ' '], '', $permission); - } + foreach ($this->getPermissions($attribute) as $permission) { + if (!\is_string($permission) || !\str_starts_with($permission, $type)) { + continue; + } + $typePermissions[] = (string) \str_replace([$type . '(', ')', '"', ' '], '', $permission); + }[ suggest_optional_refactor ]
src/Database/Adapter/SQL.php (1)
1729-1733: Filter ordering may break aliases containing$
getInternalKeyForAttribute()is invoked before$this->filter().
filter()strips any character not matching[A-Za-z0-9_], thus turning'main::$permissions'into'main__permissions'.
Downstream MySQL ends up with “Unknown column”.Ensure alias names with
::and$bypass the generic sanitiser, or move the sanitising to the original attribute (before aliasing).- $selection = $this->getInternalKeyForAttribute($selection); - $selection = "{$this->quote($prefix)}.{$this->quote($this->filter($selection))}"; + $internal = $this->getInternalKeyForAttribute($selection); + $column = $this->filter($internal); + $selection = "{$this->quote($prefix)}.{$this->quote($column)}";Ensure
filter()allows internal attributes or adjust accordingly.tests/e2e/Adapter/Scopes/DocumentTests.php (10)
744-746: Verify the new expectation (1 vs 2 affected documents)The assertion was changed from
2to1but the surrounding logic is unchanged except for setting thelastattribute tonullon$existingDocument.
Please confirm the adapter really returns1in this scenario; otherwise the test will mask a regression.No code change suggested – just double-check the behaviour.
1132-1144: Internal-attribute assertions look goodThe new checks correctly enforce the
main::$*prefix and ensure legacy keys are absent.
👍 Nothing to add.
1151-1164: Consistent prefix assertionsSame comment as above – assertions are accurate and valuable.
2821-2833: Select-query prefix assertionsGood coverage for prefixed metadata keys in
testFindSelect.
2846-2857: Select with$idkeeps both key sets in syncLGTM.
2869-2881: Select with$sequence– assertions validNo issues spotted.
2893-2904: Select with$collection– assertions validNo concerns.
2916-2927: Select with$createdAt– assertions validLooks correct.
2939-2950: Select with$updatedAt– assertions validAll good.
2963-2973: Select with$permissions– assertions validChecks are thorough and consistent.
src/Database/Database.php (4)
3297-3301: Permission check key updated correctly ✅
No issues spotted; call now usesmain::$permissions.
3321-3330: Duplicate collection id & permissions formatting
$document->setAttribute('$collection', …)ANDmain::$collection. Duplicates are unavoidable, but laterDateTime::formatTz()is applied to collection id (see 6143-6144) — that’s a bug, not here but downstream.
3692-3699: Post-creation mirror attributes – OK
Attributes mirrored undermain::$look consistent with convention.
6662-6668: Commented-out auto-selectsDisabling automatic inclusion of internal attributes changes public API; confirm downstream callers don’t rely on them.
| public function getPermissions(string $attribute = '$permissions'): array | ||
| { | ||
| return \array_values(\array_unique($this->getAttribute('$permissions', []))); | ||
| return \array_values(\array_unique($this->getAttribute($attribute, []))); | ||
| } |
There was a problem hiding this comment.
Guard against non-array permission payloads
getPermissions() assumes the resolved attribute is always an array.
If the DB adapter ever returns malformed data (e.g. a string‐encoded JSON, null, etc.), array_unique() will raise a warning and the method will explode.
- public function getPermissions(string $attribute = '$permissions'): array
- {
- return \array_values(\array_unique($this->getAttribute($attribute, [])));
- }
+ public function getPermissions(string $attribute = '$permissions'): array
+ {
+ $permissions = $this->getAttribute($attribute, []);
+
+ if (!\is_array($permissions)) {
+ // Fail-safe – return an empty list rather than trigger a PHP warning
+ return [];
+ }
+
+ return \array_values(\array_unique($permissions));
+ }Same defensive check is advisable in the other helpers that delegate to this method.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| public function getPermissions(string $attribute = '$permissions'): array | |
| { | |
| return \array_values(\array_unique($this->getAttribute('$permissions', []))); | |
| return \array_values(\array_unique($this->getAttribute($attribute, []))); | |
| } | |
| public function getPermissions(string $attribute = '$permissions'): array | |
| { | |
| $permissions = $this->getAttribute($attribute, []); | |
| if (!\is_array($permissions)) { | |
| // Fail-safe – return an empty list rather than trigger a PHP warning | |
| return []; | |
| } | |
| return \array_values(\array_unique($permissions)); | |
| } |
🤖 Prompt for AI Agents
In src/Database/Document.php around lines 83 to 86, the getPermissions() method
assumes the attribute is always an array, which can cause warnings if the data
is malformed. Add a defensive check to verify the attribute is an array before
calling array_unique and array_values; if not, return an empty array. Apply
similar checks in other helper methods that call getPermissions() to ensure
robustness against unexpected data types.
| public function getRead(string $attribute = '$permissions'): array | ||
| { | ||
| return $this->getPermissionsByType(Database::PERMISSION_READ); | ||
| return $this->getPermissionsByType(Database::PERMISSION_READ, $attribute); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Propagate the attribute parameter to the convenience helpers
getRead() (and, by extension, getCreate(), getUpdate(), getDelete()) now call the 2-parameter getPermissionsByType() but still omit the second argument.
If callers pass a custom attribute key to getRead() they’ll silently fall back to '$permissions', which defeats the purpose of the new API.
- public function getRead(string $attribute = '$permissions'): array
- {
- return $this->getPermissionsByType(Database::PERMISSION_READ, $attribute);
- }
+ public function getRead(string $attribute = '$permissions'): array
+ {
+ return $this->getPermissionsByType(Database::PERMISSION_READ, $attribute);
+ }Replicate the additional $attribute parameter on getCreate(), getUpdate(), and getDelete() for a consistent public surface.
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In src/Database/Document.php around lines 91 to 94, the getRead() method accepts
an $attribute parameter but does not pass it to getPermissionsByType(), causing
it to always use the default '$permissions'. To fix this, update getRead() to
pass the $attribute parameter to getPermissionsByType(). Also, add the
$attribute parameter to the signatures of getCreate(), getUpdate(), and
getDelete() methods and ensure they pass it to getPermissionsByType() as well,
maintaining a consistent API.
| if (empty($selections) || \in_array('*', $selections)) { | ||
| return "{$this->quote($prefix)}.*"; | ||
| return "{$this->quote($prefix)}.* {$this->addHiddenAttribute($selections)}"; | ||
| } | ||
|
|
||
| $internalKeys = [ | ||
| '$id', | ||
| '$sequence', | ||
| '$permissions', | ||
| '$createdAt', | ||
| '$updatedAt', | ||
| ]; | ||
|
|
||
| $selections = \array_diff($selections, [...$internalKeys, '$collection']); | ||
|
|
||
| foreach ($internalKeys as $internalKey) { | ||
| $selections[] = $this->getInternalKeyForAttribute($internalKey); | ||
| } | ||
| $selections = array_diff($selections, ['$collection']); | ||
|
|
||
| foreach ($selections as &$selection) { | ||
| $selection = $this->getInternalKeyForAttribute($selection); | ||
| $selection = "{$this->quote($prefix)}.{$this->quote($this->filter($selection))}"; | ||
| } | ||
|
|
||
| return \implode(',', $selections); | ||
| return \implode(',', $selections).$this->addHiddenAttribute($selections); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
getAttributeProjection() builds SQL with accidental duplicate columns
{$this->quote($prefix)}.* already contains _uid, _permissions, etc., and addHiddenAttribute() appends them again with aliases. This:
- Bloats result sets.
- Wastes bandwidth & memory.
- Requires extra CPU to hydrate arrays with duplicate data.
Refactor to:
- Select either
*or the explicit list of user-requested columns plus hidden projection – never both. - Rely solely on aliased hidden columns to sidestep collisions with the raw ones.
Example patch (after adopting comment above):
-if (empty($selections) || \in_array('*', $selections)) {
- return "{$this->quote($prefix)}.* {$this->addHiddenAttribute($selections)}";
-}
+if (empty($selections) || \in_array('*', $selections, true)) {
+ return "{$this->quote($prefix)}.*";
+}
...
-return \implode(',', $selections).$this->addHiddenAttribute($selections);
+return \implode(',', $selections).', '.$this->getHiddenProjection($prefix);Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In src/Database/Adapter/SQL.php around lines 1725 to 1737, the method
getAttributeProjection() currently returns both a wildcard selection with all
columns and separately appends hidden attributes, causing duplicate columns in
the SQL result. To fix this, refactor the code so that if the selections contain
'*' or are empty, return only the wildcard selection without appending hidden
attributes. Otherwise, return the explicitly quoted user-requested columns
combined with the aliased hidden attributes, ensuring no duplicates. This will
prevent redundant columns, reduce data bloat, and improve performance.
| /** | ||
| * Get the SQL projection given the selected attributes | ||
| * | ||
| * @param array<string> $selects | ||
| * @return string | ||
| * @throws Exception | ||
| */ | ||
| protected function addHiddenAttribute(array $selects): string | ||
| { | ||
| //return ''; | ||
|
|
||
| $alias = Query::DEFAULT_ALIAS; | ||
|
|
||
| $strings = []; | ||
|
|
||
| $strings[] = $alias.'._uid as '.$this->quote($alias.'::$id'); | ||
| $strings[] = $alias.'._id as '.$this->quote($alias.'::$sequence'); | ||
| $strings[] = $alias.'._permissions as '.$this->quote($alias.'::$permissions'); | ||
| $strings[] = $alias.'._createdAt as '.$this->quote($alias.'::$createdAt'); | ||
| $strings[] = $alias.'._updatedAt as '.$this->quote($alias.'::$updatedAt'); | ||
|
|
||
| if ($this->sharedTables) { | ||
| $strings[] = $alias.'._tenant as '.$this->quote($alias.'::$tenant'); | ||
| } | ||
|
|
||
| return ', '.implode(', ', $strings); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
addHiddenAttribute() – dead parameter, duplicated data, leading comma
array $selectsis never referenced → dead argument.- The function always prefixes the string with
', '. When the caller already built an empty projection (rare but possible), the SQL becomes syntactically invalid:'SELECT , main._uid AS …'. - It re-selects columns already included by
{$prefix}.*, doubling network payload.
Consider:
-protected function addHiddenAttribute(array $selects): string
+protected function getHiddenProjection(string $alias): string
{
- //return '';
- $alias = Query::DEFAULT_ALIAS;
-
- $strings = [];
+ $columns = [];
- $strings[] = $alias.'._uid as '.$this->quote($alias.'::$id');
+ $columns[] = "{$alias}._uid AS ".$this->quote($alias.'::$id');
...
- return ', '.implode(', ', $strings);
+ return implode(', ', $columns);
}Then compose the projection once in getAttributeProjection() without the leading comma.
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In src/Database/Adapter/SQL.php around lines 1687 to 1713, the
addHiddenAttribute() method has an unused parameter $selects, always prepends a
leading comma causing invalid SQL if called on empty projections, and duplicates
columns already selected by {$prefix}.*. Remove the unused $selects parameter,
eliminate the leading comma from the returned string, and refactor the method to
avoid selecting columns already included by the wildcard. Instead, move the
hidden attribute projection logic into getAttributeProjection() to compose the
full projection string once without redundant prefixes or duplicates.
| $document['main::$permissions'] = json_decode($document['main::$permissions'], true); | ||
|
|
||
| if ($this->sharedTables) { | ||
| $document['main::$tenant'] = $document['main::$tenant'] === null ? null : (int)$document['main::$tenant']; | ||
| } | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Undefined-index & double-decode risk on main::$permissions / main::$tenant
$document['main::$permissions'] and $document['main::$tenant'] are accessed unconditionally.
When the caller selects a subset of columns (e.g. ['name']) these aliases are not present, producing PHP 8.2 “Undefined array key” notices.
Additionally, _permissions was already decoded into $permissions a few lines above, so this is a redundant second decode that can diverge from the canonical $permissions key.
- $document['main::$permissions'] = json_decode($document['main::$permissions'], true);
- if ($this->sharedTables) {
- $document['main::$tenant'] = $document['main::$tenant'] === null ? null : (int)$document['main::$tenant'];
- }
+ if (\array_key_exists('main::$permissions', $document)) {
+ $document['$permissions'] ??= json_decode($document['main::$permissions'] ?? '[]', true);
+ unset($document['main::$permissions']);
+ }
+
+ if ($this->sharedTables && \array_key_exists('main::$tenant', $document)) {
+ $document['$tenant'] ??= ($document['main::$tenant'] === null ? null : (int)$document['main::$tenant']);
+ unset($document['main::$tenant']);
+ }This keeps one canonical key ($permissions, $tenant) and avoids runtime notices.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| $document['main::$permissions'] = json_decode($document['main::$permissions'], true); | |
| if ($this->sharedTables) { | |
| $document['main::$tenant'] = $document['main::$tenant'] === null ? null : (int)$document['main::$tenant']; | |
| } | |
| if (\array_key_exists('main::$permissions', $document)) { | |
| $document['$permissions'] ??= json_decode($document['main::$permissions'] ?? '[]', true); | |
| unset($document['main::$permissions']); | |
| } | |
| if ($this->sharedTables && \array_key_exists('main::$tenant', $document)) { | |
| $document['$tenant'] ??= ($document['main::$tenant'] === null ? null : (int)$document['main::$tenant']); | |
| unset($document['main::$tenant']); | |
| } |
🤖 Prompt for AI Agents
In src/Database/Adapter/SQL.php around lines 400 to 405, avoid accessing
$document['main::$permissions'] and $document['main::$tenant'] directly without
checking if these keys exist to prevent PHP 8.2 undefined array key notices.
Remove the redundant json_decode on $document['main::$permissions'] since
$permissions is already decoded earlier. Instead, ensure you use the canonical
keys $permissions and $tenant consistently, and only set $tenant after verifying
its existence in $document to avoid notices.
| ->setAttribute('last', 'last') | ||
| ]); | ||
|
|
||
| var_dump($docs); | ||
| $this->assertEquals(1, $docs); | ||
|
|
||
| $this->assertEquals('first', $existingDocument->getAttribute('first')); |
There was a problem hiding this comment.
Remove leftover debug statement and fix style violation
var_dump($docs); is a debugging artefact that should not live in committed test code.
Besides polluting CI output, it is the reason for the current Pint statement_indentation failure.
Proposed fix:
- var_dump($docs);
+ // $docs is the number of modified documents – keep it for the assertion below.Or simply delete the line altogether.
While touching this, consider renaming $docs to $count (it's an integer) and adding $this->assertIsInt($docs); for clarity.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| ->setAttribute('last', 'last') | |
| ]); | |
| var_dump($docs); | |
| $this->assertEquals(1, $docs); | |
| $this->assertEquals('first', $existingDocument->getAttribute('first')); | |
| ->setAttribute('last', 'last') | |
| ]); | |
| // $docs is the number of modified documents – keep it for the assertion below. | |
| $this->assertEquals(1, $docs); | |
| $this->assertEquals('first', $existingDocument->getAttribute('first')); |
🤖 Prompt for AI Agents
In tests/e2e/Adapter/Scopes/DocumentTests.php around lines 742 to 747, remove
the var_dump($docs) debug statement to fix the style violation and clean up the
test output. Rename the variable $docs to $count to better reflect that it holds
an integer, and add an assertion $this->assertIsInt($count) before asserting its
value to improve test clarity.
src/Database/Database.php
Outdated
| /** | ||
| * @var $old Document | ||
| */ | ||
| $old = Authorization::skip(fn () => $this->silent( |
There was a problem hiding this comment.
Fix invalid PHPDoc
@var $old Document triggers CodeQL error. Should be:
- * @var $old Document
+ * @var Document $old📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| /** | |
| * @var $old Document | |
| */ | |
| $old = Authorization::skip(fn () => $this->silent( | |
| /** | |
| * @var Document $old | |
| */ | |
| $old = Authorization::skip(fn () => $this->silent( |
🧰 Tools
🪛 GitHub Actions: CodeQL
[error] 4151-4151: PHPDoc tag @var has invalid value ($old Document): Unexpected token "$old", expected type at offset 24
🤖 Prompt for AI Agents
In src/Database/Database.php around lines 4148 to 4151, the PHPDoc annotation
`@var $old Document` is invalid and triggers a CodeQL error. Remove the dollar
sign from the variable name in the PHPDoc so it reads `@var old Document` to
correctly document the variable type without syntax errors.
…ttributes-defaults
…ttributes-defaults
1 participant
Summary by CodeRabbit
Bug Fixes
Tests