Default alias getDocument #606
Conversation
WalkthroughThe changes update method signatures for Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Pool
participant SQLAdapter
Client->>Pool: getAttributeProjection(selections, prefix)
Pool->>SQLAdapter: getAttributeProjection(selections, prefix)
SQLAdapter->>SQLAdapter: Build projection with prefix (alias)
SQLAdapter-->>Pool: Projected attributes
Pool-->>Client: Projected attributes
Poem
✨ Finishing Touches
🪧 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: 0
🔭 Outside diff range comments (1)
src/Database/Adapter/SQL.php (1)
1685-1707:⚠️ Potential issueGuard against empty
$prefixto avoid invalid ````.*`` SQLThe new logic always prefixes attributes:
return "{$this->quote($prefix)}.*";If a caller accidentally passes an empty string, the projection becomes ````.*`` which MySQL rejects.
- if (empty($selections) || \in_array('*', $selections)) { - return "{$this->quote($prefix)}.*"; + if (empty($prefix)) { + throw new DatabaseException('Attribute projection requires a non-empty table alias/prefix'); + } + + if (empty($selections) || \in_array('*', $selections)) { + return "{$this->quote($prefix)}.*"; }This explicit check fails fast and protects against subtle runtime SQL errors.
🧹 Nitpick comments (1)
src/Database/Adapter/Pool.php (1)
458-461: Docblock missing for the newly-typed method
getAttributeProjection()inPoolnow mirrors the stricter signature but has no PHPDoc.
Adding it keeps IDEs & static analysers happy and documents the delegated behaviour.+ /** + * @inheritDoc + */ protected function getAttributeProjection(array $selections, string $prefix): mixed
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/Database/Adapter.php(1 hunks)src/Database/Adapter/Pool.php(1 hunks)src/Database/Adapter/SQL.php(3 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (2)
src/Database/Adapter/Pool.php (2)
src/Database/Adapter.php (1)
getAttributeProjection(1073-1073)src/Database/Adapter/SQL.php (1)
getAttributeProjection(1685-1710)
src/Database/Adapter.php (2)
src/Database/Adapter/Pool.php (1)
getAttributeProjection(458-461)src/Database/Adapter/SQL.php (1)
getAttributeProjection(1685-1710)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Setup & Build Docker Image
- GitHub Check: CodeQL
🔇 Additional comments (2)
src/Database/Adapter.php (1)
1067-1074:❓ Verification inconclusive
BC break –
$prefixis now mandatory, check every subclass & call-siteRemoving the default value forces all implementers (and any direct call sites) to always pass a prefix.
External adapters or older code extendingAdapterwill now fatally error until updated.This should return zero matches.
Consider adding an explicit runtime check inside the abstract method (or in concrete ones) that throws when an empty prefix is provided to avoid silent generation of ````.*```.
🏁 Script executed:
#!/bin/bash # Find all concrete implementations still using the old signature (missing the 2nd param) ast-grep --pattern $'function getAttributeProjection($_selections) {$$$}'Length of output: 74
🏁 Script executed:
#!/bin/bash # 1) List all definitions of getAttributeProjection and show their full signature (to verify parameter count) rg --type=php -n 'function\s+getAttributeProjection\(' --color never # 2) Find all call-sites passing only one argument (no comma inside the parens) rg --type=php -n "getAttributeProjection\(" --color never | grep -P 'getAttributeProjection\([^,]+\)'Length of output: 546
Internal Implementations Updated – External BC Risk Remains
All internal subclasses and call-sites now include the new
$prefixparameter:
- Definitions found in:
src/Database/Adapter.php(line 1073)src/Database/Adapter/SQL.php(line 1685)src/Database/Adapter/Pool.php(line 458)- No direct calls to
getAttributeProjection()with only one argument detected.However, any external adapters or legacy code extending
Adapterwill still break at runtime. To safeguard against silent failures (e.g. generating.*), consider adding a runtime guard in the abstract method:abstract protected function getAttributeProjection(array $selections, string $prefix): mixed { if ($prefix === '') { throw new \InvalidArgumentException('`$prefix` cannot be empty'); } // …rest of implementation }Also update the upgrading guide/docs to note the now-required
$prefix.src/Database/Adapter/SQL.php (1)
340-347:✅ Verification successful
Verify
Query::DEFAULT_ALIASexists and is unique per statementThe query now hard-codes an alias:
$alias = Query::DEFAULT_ALIAS;If the constant isn’t defined or collides with other aliases in complex queries, SQL will fail.
🏁 Script executed:
#!/bin/bash # 1. Ensure the constant exists rg --line-number $'const DEFAULT_ALIAS' --context 2 | head # 2. Scan for SELECT statements that already set their own alias rg -n $' AS [`"]?'+$(php -r 'require "vendor/autoload.php"; echo \Utopia\Database\Query::DEFAULT_ALIAS;') $'src/Database/Adapter' | headLength of output: 1927
#!/bin/bash # Extract DEFAULT_ALIAS value without loading vendor files alias=$(sed -En "s/.*public const DEFAULT_ALIAS = '([^']+)'.*/\1/p" src/Database/Query.php) echo "Default alias = $alias" # Search for literal uses of this alias in all adapter classes rg -n " AS \`$alias\`" src/Database/Adapter | head
DEFAULT_ALIAS is defined and used consistently
We’ve confirmed that:
Query::DEFAULT_ALIASis declared on line 41 ofsrc/Database/Query.phpwith the value'main'.- All SQL adapters (
SQL.php,Postgres.php,SQLite.php,MariaDB.php, etc.) alias their primary table asmainand do not introduce any conflicting aliases within the same statement.No alias collisions were found—no changes necessary.
Summary by CodeRabbit