Skip to content
57 changes: 42 additions & 15 deletions src/Database/Adapter/SQL.php
Original file line number Diff line number Diff line change
Expand Up @@ -350,6 +350,10 @@ public function getDocument(string $collection, string $id, array $queries = [],
$sql .= " {$forUpdate}";
}

if (!empty($selections)){
echo $sql.PHP_EOL;
}

Comment thread
coderabbitai[bot] marked this conversation as resolved.
Outdated
$stmt = $this->getPDO()->prepare($sql);

$stmt->bindValue(':_uid', $id);
Expand Down Expand Up @@ -393,6 +397,12 @@ public function getDocument(string $collection, string $id, array $queries = [],
unset($document['_permissions']);
}

$document['main::$permissions'] = json_decode($document['main::$permissions'], true);

if ($this->sharedTables) {
$document['main::$tenant'] = $document['main::$tenant'] === null ? null : (int)$document['main::$tenant'];
}

Comment on lines +396 to +401
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

⚠️ Potential issue

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.

Suggested change
$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.

return new Document($document);
}

Expand Down Expand Up @@ -1674,6 +1684,34 @@ public function getTenantQuery(
return "{$condition} ({$alias}{$dot}_tenant IN ({$bindings}) {$orIsNull})";
}

/**
* 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);
}
Comment on lines +1683 to +1707
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

addHiddenAttribute() – dead parameter, duplicated data, leading comma

  • array $selects is 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.


/**
* Get the SQL projection given the selected attributes
*
Expand All @@ -1685,28 +1723,17 @@ public function getTenantQuery(
protected function getAttributeProjection(array $selections, string $prefix): mixed
{
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);
}
Comment on lines 1719 to 1731
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🛠️ 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:

  1. Bloats result sets.
  2. Wastes bandwidth & memory.
  3. Requires extra CPU to hydrate arrays with duplicate data.

Refactor to:

  1. Select either * or the explicit list of user-requested columns plus hidden projection – never both.
  2. 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.


protected function getInternalKeyForAttribute(string $attribute): string
Expand Down
103 changes: 88 additions & 15 deletions src/Database/Database.php
Original file line number Diff line number Diff line change
Expand Up @@ -526,6 +526,25 @@ public function silent(callable $callback, ?array $listeners = null): mixed
}
}

/**
* Check if attribute is internal
*
* @param string $attribute
* @return bool
*/
public static function isInternalAttribute(string $attribute): bool
{
if (str_contains($attribute, '$')) {
foreach (Database::INTERNAL_ATTRIBUTES as $attr) {
if (str_contains($attribute, $attr['$id'])) {
return true;
}
}
}

return false;
}

/**
* Get getConnection Id
*
Expand Down Expand Up @@ -3277,7 +3296,7 @@ public function getDocument(string $collection, string $id, array $queries = [],
if ($collection->getId() !== self::METADATA) {
if (!$validator->isValid([
...$collection->getRead(),
...($documentSecurity ? $document->getRead() : [])
...($documentSecurity ? $document->getRead('main::$permissions') : [])
])) {
return new Document();
}
Expand All @@ -3294,17 +3313,17 @@ public function getDocument(string $collection, string $id, array $queries = [],
$queries,
$forUpdate
);

if ($document->isEmpty()) {
return $document;
}

$document->setAttribute('$collection', $collection->getId());
$document->setAttribute('main::$collection', $collection->getId());

if ($collection->getId() !== self::METADATA) {
if (!$validator->isValid([
...$collection->getRead(),
...($documentSecurity ? $document->getRead() : [])
...($documentSecurity ? $document->getRead('main::$permissions') : [])
])) {
return new Document();
}
Expand Down Expand Up @@ -3335,6 +3354,9 @@ public function getDocument(string $collection, string $id, array $queries = [],

$this->trigger(self::EVENT_DOCUMENT_READ, $document);

$document->setAttribute('main::$createdAt', DateTime::formatTz($document->getAttribute('main::$createdAt')));
$document->setAttribute('main::$updatedAt', DateTime::formatTz($document->getAttribute('main::$updatedAt')));

return $document;
}

Expand Down Expand Up @@ -3667,6 +3689,14 @@ public function createDocument(string $collection, Document $document): Document

$this->trigger(self::EVENT_DOCUMENT_CREATE, $document);

$document->setAttribute('main::$id', $document->getId());
$document->setAttribute('main::$sequence', $document->getSequence());
$document->setAttribute('main::$permissions', $document->getPermissions());
$document->setAttribute('main::$createdAt', $document->getCreatedAt());
$document->setAttribute('main::$updatedAt', $document->getUpdatedAt());
$document->setAttribute('main::$tenant', $document->getTenant());
$document->setAttribute('main::$collection', $document->getCollection());

return $document;
}

Expand Down Expand Up @@ -3989,7 +4019,7 @@ private function relateDocuments(

if ($related->isEmpty()) {
// If the related document doesn't exist, create it, inheriting permissions if none are set
if (!isset($relation['$permissions'])) {
if (!isset($relation['$permissions'])) { // todo: Should this be main::$permissions
$relation->setAttribute('$permissions', $document->getPermissions());
}

Expand Down Expand Up @@ -4114,6 +4144,10 @@ public function updateDocument(string $collection, string $id, Document $documen

$document = $this->withTransaction(function () use ($collection, $id, $document) {
$time = DateTime::now();

/**
* @var $old Document
*/
$old = Authorization::skip(fn () => $this->silent(
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue

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.

Suggested change
/**
* @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.

fn () => $this->getDocument($collection->getId(), $id, forUpdate: true)
));
Expand Down Expand Up @@ -4143,8 +4177,14 @@ public function updateDocument(string $collection, string $id, Document $documen
$relationships[$relationship->getAttribute('key')] = $relationship;
}

$alias = Query::DEFAULT_ALIAS;

// Compare if the document has any changes
foreach ($document as $key => $value) {
if (str_starts_with($key, $alias.'::') && Database::isInternalAttribute($key)){
continue;
}

// Skip the nested documents as they will be checked later in recursions.
if (\array_key_exists($key, $relationships)) {
// No need to compare nested documents more than max depth.
Expand Down Expand Up @@ -4288,6 +4328,14 @@ public function updateDocument(string $collection, string $id, Document $documen

$this->trigger(self::EVENT_DOCUMENT_UPDATE, $document);

$document->setAttribute('main::$id', $document->getId());
$document->setAttribute('main::$sequence', $document->getSequence());
$document->setAttribute('main::$permissions', $document->getPermissions());
$document->setAttribute('main::$createdAt', $document->getCreatedAt());
$document->setAttribute('main::$updatedAt', $document->getUpdatedAt());
$document->setAttribute('main::$tenant', $document->getTenant());
$document->setAttribute('main::$collection', $document->getCollection());

return $document;
}

Expand Down Expand Up @@ -4565,7 +4613,7 @@ private function updateDocumentRelationships(Document $collection, Document $old
$related = $this->skipRelationships(
fn () => $this->getDocument($relatedCollection->getId(), $value, [Query::select(['$id'])])
);

var_dump($related);
if ($related->isEmpty()) {
// If no such document exists in related collection
// For one-one we need to update the related key to null if no relation exists
Expand All @@ -4588,6 +4636,13 @@ private function updateDocumentRelationships(Document $collection, Document $old
$related->getId(),
$related->setAttribute($twoWayKey, $document->getId())
));
var_dump($relatedCollection->getId());
var_dump($value);
var_dump(\gettype($value));
var_dump($twoWay);
var_dump($twoWayKey);
var_dump($related);
var_dump($document->getId());
break;
case 'object':
if ($value instanceof Document) {
Expand All @@ -4606,7 +4661,7 @@ private function updateDocumentRelationships(Document $collection, Document $old

$this->relationshipWriteStack[] = $relatedCollection->getId();
if ($related->isEmpty()) {
if (!isset($value['$permissions'])) {
if (!isset($value['$permissions'])) {// todo check if should be main::$permissions
$value->setAttribute('$permissions', $document->getAttribute('$permissions'));
}
$related = $this->createDocument(
Expand Down Expand Up @@ -4695,7 +4750,7 @@ private function updateDocumentRelationships(Document $collection, Document $old
);

if ($related->isEmpty()) {
if (!isset($relation['$permissions'])) {
if (!isset($relation['$permissions'])) { // todo check if should be main::$permissions
$relation->setAttribute('$permissions', $document->getAttribute('$permissions'));
}
$this->createDocument(
Expand Down Expand Up @@ -4735,7 +4790,7 @@ private function updateDocumentRelationships(Document $collection, Document $old
);

if ($related->isEmpty()) {
if (!isset($value['$permissions'])) {
if (!isset($value['$permissions'])) { // todo check if should be main::$permissions
$value->setAttribute('$permissions', $document->getAttribute('$permissions'));
}
$this->createDocument(
Expand Down Expand Up @@ -4808,7 +4863,7 @@ private function updateDocumentRelationships(Document $collection, Document $old
$related = $this->getDocument($relatedCollection->getId(), $relation->getId(), [Query::select(['$id'])]);

if ($related->isEmpty()) {
if (!isset($value['$permissions'])) {
if (!isset($value['$permissions'])) {// todo check if should be main::$permissions
$relation->setAttribute('$permissions', $document->getAttribute('$permissions'));
}
$related = $this->createDocument(
Expand Down Expand Up @@ -6085,6 +6140,9 @@ public function find(string $collection, array $queries = [], string $forPermiss

if (!$node->isEmpty()) {
$node->setAttribute('$collection', $collection->getId());
$node->setAttribute('main::$collection', DateTime::formatTz($node->getAttribute('main::$collection')));
$node->setAttribute('main::$createdAt', DateTime::formatTz($node->getAttribute('main::$createdAt')));
$node->setAttribute('main::$updatedAt', DateTime::formatTz($node->getAttribute('main::$updatedAt')));
}
Comment thread
coderabbitai[bot] marked this conversation as resolved.
Outdated
}

Expand Down Expand Up @@ -6298,6 +6356,21 @@ public static function addFilter(string $name, callable $encode, callable $decod
*/
public function encode(Document $collection, Document $document): Document
{
/**
* When iterating over an ArrayObject, foreach uses an internal iterator (like Iterator), and modifying the object during iteration doesn’t affect the iterator immediately.
*/
$keysToRemove = [];

foreach ($document as $key => $value) {
if (strpos($key, '::$') !== false) {
$keysToRemove[] = $key;
}
}

foreach ($keysToRemove as $key) {
unset($document[$key]);
}

$attributes = $collection->getAttribute('attributes', []);

$internalAttributes = \array_filter(Database::INTERNAL_ATTRIBUTES, function ($attribute) {
Expand Down Expand Up @@ -6586,12 +6659,12 @@ private function validateSelections(Document $collection, array $queries): array

$selections = \array_merge($selections, $relationshipSelections);

$selections[] = '$id';
$selections[] = '$sequence';
$selections[] = '$collection';
$selections[] = '$createdAt';
$selections[] = '$updatedAt';
$selections[] = '$permissions';
// $selections[] = '$id';
// $selections[] = '$sequence';
// $selections[] = '$collection';
// $selections[] = '$createdAt';
// $selections[] = '$updatedAt';
// $selections[] = '$permissions';

return \array_values(\array_unique($selections));
}
Expand Down
19 changes: 7 additions & 12 deletions src/Database/Document.php
Original file line number Diff line number Diff line change
Expand Up @@ -80,17 +80,17 @@ public function getCollection(): string
/**
* @return array<string>
*/
public function getPermissions(): array
public function getPermissions(string $attribute = '$permissions'): array
{
return \array_values(\array_unique($this->getAttribute('$permissions', [])));
return \array_values(\array_unique($this->getAttribute($attribute, [])));
}
Comment on lines +83 to 86
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue

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.

Suggested change
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.


/**
* @return array<string>
*/
public function getRead(): array
public function getRead(string $attribute = '$permissions'): array
{
return $this->getPermissionsByType(Database::PERMISSION_READ);
return $this->getPermissionsByType(Database::PERMISSION_READ, $attribute);
}
Comment on lines +91 to 94
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🛠️ 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.


/**
Expand Down Expand Up @@ -132,11 +132,11 @@ public function getWrite(): array
/**
* @return array<string>
*/
public function getPermissionsByType(string $type): array
public function getPermissionsByType(string $type, string $attribute = '$permissions'): array
{
$typePermissions = [];

foreach ($this->getPermissions() as $permission) {
foreach ($this->getPermissions($attribute) as $permission) {
if (!\str_starts_with($permission, $type)) {
continue;
}
Expand Down Expand Up @@ -183,13 +183,8 @@ public function getAttributes(): array
{
$attributes = [];

$internalKeys = \array_map(
fn ($attr) => $attr['$id'],
Database::INTERNAL_ATTRIBUTES
);

foreach ($this as $attribute => $value) {
if (\in_array($attribute, $internalKeys)) {
if (Database::isInternalAttribute($attribute)){
continue;
}

Expand Down
Loading
Loading