Conversation
# Conflicts: # composer.lock # src/Database/Database.php # tests/e2e/Adapter/Base.php
| if ($options['relationType'] === Database::RELATION_ONE_TO_ONE && $options['twoWay'] === false && $options['side'] === Database::RELATION_SIDE_CHILD) { | ||
| throw new \Exception('Cannot query on virtual relationship attribute'); | ||
| } | ||
|
|
||
| if ($options['relationType'] === Database::RELATION_ONE_TO_MANY && $options['side'] === Database::RELATION_SIDE_PARENT) { | ||
| throw new \Exception('Cannot query on virtual relationship attribute'); | ||
| } | ||
|
|
||
| if ($options['relationType'] === Database::RELATION_MANY_TO_ONE && $options['side'] === Database::RELATION_SIDE_CHILD) { | ||
| throw new \Exception('Cannot query on virtual relationship attribute'); | ||
| } | ||
|
|
||
| if ($options['relationType'] === Database::RELATION_MANY_TO_MANY) { | ||
| throw new \Exception('Cannot query on virtual relationship attribute'); | ||
| } |
There was a problem hiding this comment.
They are still in the main branch in Filter.php class 🤔
# Conflicts: # src/Database/Database.php
# Conflicts: # src/Database/Database.php
# Conflicts: # composer.lock # src/Database/Adapter/Mongo.php
Greptile SummaryThis PR adds SQL JOIN support (inner, left, right) to Key issues found:
Confidence Score: 3/5Not safe to merge yet — two P1 runtime bugs (null key insertion in MongoDB, potential SQL syntax error from empty projection) must be fixed first The join architecture is well-designed and has good test coverage (JoinsTests.php). However, two concrete runtime bugs were identified: the commented-out removeNullKeys in the Mongo adapter will silently corrupt document storage semantics for all new inserts, and getAttributeProjection can produce invalid SQL for a valid user input. Additionally, casting() now silently drops unrecognised attributes which is a breaking behavioral change that needs explicit documentation or a fix. src/Database/Adapter/Mongo.php (line 1277: commented-out removeNullKeys), src/Database/Adapter/SQL.php (getAttributeProjection empty string fallback missing), src/Database/Database.php (casting() attribute dropping, filterDocumentsBySelections change) Important Files Changed
|
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| //$record = $this->removeNullKeys($record); | ||
| $result = $this->insertDocument($name, $record, $options); |
There was a problem hiding this comment.
removeNullKeys left in production code
The removeNullKeys call was commented out, meaning MongoDB documents are now inserted with null-value keys stored explicitly:
//$record = $this->removeNullKeys($record);
$result = $this->insertDocument($name, $record, $options);This is a significant behavioral change:
isNull()/isNotNull()queries may now match (or stop matching) documents that previously behaved differently — in MongoDB a storednullkey is distinct from a missing key.$exists/$notExistssemantics are affected for the same reason.- Document size grows for every null-valued attribute in every newly inserted document.
This looks like debug code accidentally left in. If the intent is to stop stripping nulls, the line should be deleted (not commented out) with an explanatory comment. If it was accidentally removed, restore it:
| //$record = $this->removeNullKeys($record); | |
| $result = $this->insertDocument($name, $record, $options); | |
| $record = $this->removeNullKeys($record); | |
| $result = $this->insertDocument($name, $record, $options); |
| $string = ''; | ||
| foreach ($selects as $select) { | ||
| if ($select->getAttribute() === '$collection') { | ||
| continue; | ||
| } | ||
|
|
||
| $selections = \array_diff($selections, [...$internalKeys, '$collection']); | ||
| $alias = $select->getAlias(); | ||
| $alias = $this->filter($alias); | ||
|
|
||
| foreach ($internalKeys as $internalKey) { | ||
| $selections[] = $this->getInternalKeyForAttribute($internalKey); | ||
| } | ||
| $attribute = $this->getInternalKeyForAttribute($select->getAttribute()); | ||
|
|
||
|
|
||
| if ($attribute !== '*') { | ||
| $attribute = $this->filter($attribute); | ||
| $attribute = $this->quote($attribute); | ||
| } | ||
|
|
||
| $as = $select->getAs(); | ||
|
|
||
| if (!empty($as)) { | ||
| $as = ' as '.$this->quote($this->filter($as)); | ||
| } | ||
|
|
||
| $projections = []; | ||
| foreach ($selections as $selection) { | ||
| $filteredSelection = $this->filter($selection); | ||
| $quotedSelection = $this->quote($filteredSelection); | ||
| $projections[] = "{$this->quote($prefix)}.{$quotedSelection}"; | ||
| if (!empty($string)) { | ||
| $string .= ', '; | ||
| } | ||
|
|
||
| $string .= "{$this->quote($alias)}.{$attribute}{$as}"; | ||
| } | ||
|
|
||
| return \implode(',', $projections); | ||
| return $string; |
There was a problem hiding this comment.
getAttributeProjection() can return an empty string, producing invalid SQL
The method skips $collection selects but will return '' if all provided selects resolve to $collection:
$string = '';
foreach ($selects as $select) {
if ($select->getAttribute() === '$collection') {
continue; // all entries skipped
}
$string .= ...;
}
return $string; // '' when all selects were '$collection'When called with [Query::select('$collection')], the generated SQL becomes SELECT FROM \table` AS `main` …— a syntax error that throws an unhandledPDOException. The validator permits select('$collection')via thevalidateAttributeExist` early-return, so this path is reachable by callers.
Add a fallback after the loop:
| $string = ''; | |
| foreach ($selects as $select) { | |
| if ($select->getAttribute() === '$collection') { | |
| continue; | |
| } | |
| $selections = \array_diff($selections, [...$internalKeys, '$collection']); | |
| $alias = $select->getAlias(); | |
| $alias = $this->filter($alias); | |
| foreach ($internalKeys as $internalKey) { | |
| $selections[] = $this->getInternalKeyForAttribute($internalKey); | |
| } | |
| $attribute = $this->getInternalKeyForAttribute($select->getAttribute()); | |
| if ($attribute !== '*') { | |
| $attribute = $this->filter($attribute); | |
| $attribute = $this->quote($attribute); | |
| } | |
| $as = $select->getAs(); | |
| if (!empty($as)) { | |
| $as = ' as '.$this->quote($this->filter($as)); | |
| } | |
| $projections = []; | |
| foreach ($selections as $selection) { | |
| $filteredSelection = $this->filter($selection); | |
| $quotedSelection = $this->quote($filteredSelection); | |
| $projections[] = "{$this->quote($prefix)}.{$quotedSelection}"; | |
| if (!empty($string)) { | |
| $string .= ', '; | |
| } | |
| $string .= "{$this->quote($alias)}.{$attribute}{$as}"; | |
| } | |
| return \implode(',', $projections); | |
| return $string; | |
| if (empty($string)) { | |
| return $this->quote(Query::DEFAULT_ALIAS).'.*'; | |
| } | |
| return $string; |
|
|
||
| $attributes = $collection->getAttribute('attributes', []); | ||
| $internals = []; | ||
| $schema = []; | ||
|
|
||
| foreach ($this->getInternalAttributes() as $attribute) { | ||
| $attributes[] = $attribute; | ||
| foreach (Database::INTERNAL_ATTRIBUTES as $attribute) { |
There was a problem hiding this comment.
casting() silently drops all attributes not found in any collection schema
The rewritten casting() creates a brand-new Document and skips any key it cannot resolve against $internals or $schema:
$attribute = $internals[$key] ?? null;
if (is_null($attribute)) {
$attribute = $schema[$collection->getId()][$this->adapter->filter($key)] ?? null;
}
if (is_null($attribute)) {
continue; // silently dropped from the new document
}The old implementation mutated the original document, preserving unknown keys. Because getSupportForCasting() is true for all SQL adapters, any runtime-injected attribute or join-sourced column that cannot be resolved through the select-alias lookup is now silently lost before decode() runs.
If this is intentional (all join columns are guaranteed to be matched via the $selects loop), please add a comment making that contract explicit and add a test that covers a column not present in any schema. Otherwise, consider preserving unresolved attributes similar to how decode() handles schemaless databases.
|
|
||
| if (!empty($cursor)) { | ||
| foreach ($orderAttributes as $order) { | ||
| if ($cursor->getAttribute($order) === null) { | ||
| foreach ($orders as $order) { |
There was a problem hiding this comment.
$orders[] = Query::orderAsc(); // In joins we should not add a default order, we should validate when using a cursor we should have a unique orderThe comment says joins should not get a default order, but Query::orderAsc() (which defaults to $sequence) is unconditionally added for all queries, including joins. Either:
- Remove the comment if a default
$sequenceorder is intentional for joins too, or - Guard with
if (empty($joins))so join queries behave differently, and document what cursor users must supply instead.
| <?php | ||
|
|
||
| namespace Utopia\Database\Validator; | ||
|
|
||
| use Exception; | ||
| use Utopia\Database\Database; | ||
| use Utopia\Database\Document; | ||
| use Utopia\Database\Query; | ||
| use Utopia\Database\Validator\Query\Base; | ||
|
|
||
| class IndexedQueries extends Queries | ||
| { | ||
| /** | ||
| * @var array<Document> | ||
| */ | ||
| protected array $attributes = []; | ||
|
|
||
| /** | ||
| * @var array<Document> | ||
| */ | ||
| protected array $indexes = []; | ||
|
|
||
| /** | ||
| * Expression constructor | ||
| * | ||
| * This Queries Validator filters indexes for only available indexes | ||
| * | ||
| * @param array<Document> $attributes | ||
| * @param array<Document> $indexes | ||
| * @param array<Base> $validators | ||
| * @throws Exception | ||
| */ | ||
| public function __construct(array $attributes = [], array $indexes = [], array $validators = []) | ||
| { | ||
| $this->attributes = $attributes; | ||
|
|
||
| $this->indexes[] = new Document([ | ||
| 'type' => Database::INDEX_UNIQUE, | ||
| 'attributes' => ['$id'] | ||
| ]); | ||
|
|
||
| $this->indexes[] = new Document([ | ||
| 'type' => Database::INDEX_KEY, | ||
| 'attributes' => ['$createdAt'] | ||
| ]); | ||
|
|
||
| $this->indexes[] = new Document([ | ||
| 'type' => Database::INDEX_KEY, | ||
| 'attributes' => ['$updatedAt'] | ||
| ]); | ||
|
|
||
| foreach ($indexes as $index) { | ||
| $this->indexes[] = $index; | ||
| } | ||
|
|
||
| parent::__construct($validators); | ||
| } | ||
|
|
||
| /** | ||
| * Count vector queries across entire query tree | ||
| * | ||
| * @param array<Query> $queries | ||
| * @return int | ||
| */ | ||
| private function countVectorQueries(array $queries): int | ||
| { | ||
| $count = 0; | ||
|
|
||
| foreach ($queries as $query) { | ||
| if (in_array($query->getMethod(), Query::VECTOR_TYPES)) { | ||
| $count++; | ||
| } | ||
|
|
||
| if ($query->isNested()) { | ||
| $count += $this->countVectorQueries($query->getValues()); | ||
| } | ||
| } | ||
|
|
||
| return $count; | ||
| } | ||
|
|
||
| /** | ||
| * @param mixed $value | ||
| * @return bool | ||
| * @throws Exception | ||
| */ | ||
| public function isValid($value): bool | ||
| { | ||
| if (!parent::isValid($value)) { | ||
| return false; | ||
| } | ||
| $queries = []; | ||
| foreach ($value as $query) { | ||
| if (! $query instanceof Query) { | ||
| try { | ||
| $query = Query::parse($query); | ||
| } catch (\Throwable $e) { | ||
| $this->message = 'Invalid query: '.$e->getMessage(); | ||
|
|
||
| return false; | ||
| } | ||
| } | ||
|
|
||
| if ($query->isNested()) { | ||
| if (! self::isValid($query->getValues())) { | ||
| return false; | ||
| } | ||
| } | ||
|
|
||
| $queries[] = $query; | ||
| } | ||
|
|
||
| $vectorQueryCount = $this->countVectorQueries($queries); | ||
| if ($vectorQueryCount > 1) { | ||
| $this->message = 'Cannot use multiple vector queries in a single request'; | ||
| return false; | ||
| } | ||
|
|
||
| $grouped = Query::groupByType($queries); | ||
| $filters = $grouped['filters']; | ||
|
|
||
| foreach ($filters as $filter) { | ||
| if ( | ||
| $filter->getMethod() === Query::TYPE_SEARCH || | ||
| $filter->getMethod() === Query::TYPE_NOT_SEARCH | ||
| ) { | ||
| $matched = false; | ||
|
|
||
| foreach ($this->indexes as $index) { | ||
| if ( | ||
| $index->getAttribute('type') === Database::INDEX_FULLTEXT | ||
| && $index->getAttribute('attributes') === [$filter->getAttribute()] | ||
| ) { | ||
| $matched = true; | ||
| } | ||
| } | ||
|
|
||
| if (!$matched) { | ||
| $this->message = "Searching by attribute \"{$filter->getAttribute()}\" requires a fulltext index."; | ||
| return false; | ||
| } | ||
| } | ||
| } | ||
|
|
||
| return true; | ||
| } | ||
| } | ||
| // | ||
| //namespace Utopia\Database\Validator; | ||
| // | ||
| //use Exception; | ||
| //use Utopia\Database\Database; | ||
| //use Utopia\Database\Document; | ||
| //use Utopia\Database\Query; | ||
| //use Utopia\Database\Validator\Query\Base; |
There was a problem hiding this comment.
Both IndexedQueries.php and Queries/Documents.php now consist entirely of commented-out class bodies (148 and ~80 lines respectively). This creates confusion about whether these classes are still partially active, temporarily disabled, or permanently superseded.
If Queries/Queries.php is the permanent replacement, please delete these files so git history cleanly records the removal. If they need to stay temporarily, add a @deprecated docblock at the top of each file pointing to the replacement class.
2 participants
Summary by CodeRabbit
New Features
Refactor