Skip to content

Commit 6f2fc7f

Browse files
github-actions[bot]claude
authored and
claude-bot
committed
(fix): address review findings — critical & warning
Query.php: - LOGICAL_TYPES redeclared as `public const array` (matching parent's typed declaration); widened from `private` so Queries.php can reuse it without redeclaring. The prior `private const` narrowed the base class's protected visibility — a pre-existing fatal error that prevented the class from loading at all. - parse()/parseQuery() signatures aligned with the base library's current `bool $allowRaw = false` parameter — otherwise the child class was incompatible with the parent and could not load. - toArray() now uses self::LOGICAL_TYPES (strict `in_array`) and throws a typed QueryException when a logical-query child is not a Query instance, instead of crashing with a generic Error on method call. - groupForDatabase() rebuilds orderAttributes/orderTypes locally from the incoming query list. The base library's ParsedQuery no longer exposes those properties, so the prior code emitted undefined-property access and passed null to adapter `find()` (cascading into 29 unit test errors). Validator/Query/Filter.php: - Constructor uses ColumnType::tryFrom(...) ?? raw-string to avoid throwing \ValueError on unknown type strings; unknown types now fall through to the existing "Unknown Data type" recoverable error path. - Added missing ColumnType cases to the per-value validator switch: Float, BigInteger, Timestamp, Uuid7, Enum, Json, Binary, Serial, BigSerial, SmallSerial. Without these, any filter query on those columns failed with "Unknown Data type". - Dropped unreachable duplicate schema-lookup block (lines 138-142); the same condition is already short-circuited earlier in the method. Validator/Query/Order.php: - Aggregation aliases now live in a separate $aggregationAliases map rather than mutating $this->schema. Added resetAggregationAliases() so callers can clear per-pass. Prior code leaked aliases across requests in pooled / long-lived processes (Swoole), letting an unrelated query order by a stale alias from a previous request. Validator/Queries.php: - Calls Order::resetAggregationAliases() at the start of isValid() so each pass starts from a clean slate. - Alias collection and method-type dispatch both route through `$method->isAggregate()` — the base enum's source of truth. Prior code hand-listed only 8 of the 15 aggregate methods, so stddevPop, varPop, bitAnd/Or/Xor etc. were silently rejected as "Invalid query method". - Recursive nested-validation is now gated on Query::LOGICAL_TYPES only, not Method::isNested(). Union/UnionAll wrap sub-SELECTs whose children are not filters against the current collection's schema and must not be recursed into here. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
1 parent 56e3342 commit 6f2fc7f

4 files changed

Lines changed: 178 additions & 99 deletions

File tree

‎src/Database/Query.php‎

Lines changed: 42 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -26,11 +26,13 @@ class Query extends BaseQuery
2626

2727
/**
2828
* Methods that compose child queries and contribute their inner
29-
* structure to a shape/fingerprint.
29+
* structure to a shape/fingerprint. Widened from parent's protected
30+
* declaration so external validators (Queries.php) can reuse it
31+
* without redeclaring the list.
3032
*
31-
* @var array<Method>
33+
* @var list<Method>
3234
*/
33-
private const LOGICAL_TYPES = [Method::And, Method::Or, Method::ElemMatch];
35+
public const array LOGICAL_TYPES = [Method::And, Method::Or, Method::ElemMatch];
3436

3537
public const TYPE_ELEM_MATCH = 'elemMatch';
3638

@@ -51,10 +53,10 @@ public function __construct(Method|string $method, string $attribute = '', array
5153
/**
5254
* @throws QueryException
5355
*/
54-
public static function parse(string $query): static
56+
public static function parse(string $query, bool $allowRaw = false): static
5557
{
5658
try {
57-
$parsed = parent::parse($query);
59+
$parsed = parent::parse($query, $allowRaw);
5860

5961
return new static($parsed->getMethod(), $parsed->getAttribute(), $parsed->getValues());
6062
} catch (BaseQueryException $e) {
@@ -67,10 +69,10 @@ public static function parse(string $query): static
6769
*
6870
* @throws QueryException
6971
*/
70-
public static function parseQuery(array $query): static
72+
public static function parseQuery(array $query, bool $allowRaw = false): static
7173
{
7274
try {
73-
$parsed = parent::parseQuery($query);
75+
$parsed = parent::parseQuery($query, $allowRaw);
7476

7577
return new static($parsed->getMethod(), $parsed->getAttribute(), $parsed->getValues());
7678
} catch (BaseQueryException $e) {
@@ -213,9 +215,13 @@ public function toArray(): array
213215
$array['attribute'] = $this->attribute;
214216
}
215217

216-
if (\in_array($this->method, [Method::And, Method::Or, Method::ElemMatch])) {
218+
if (\in_array($this->method, self::LOGICAL_TYPES, true)) {
217219
foreach ($this->values as $index => $value) {
218-
/** @var Query $value */
220+
if (! $value instanceof self) {
221+
throw new QueryException(
222+
'Invalid child query in '.$this->method->value.' at index '.$index.': expected Query, got '.\get_debug_type($value)
223+
);
224+
}
219225
$array['values'][$index] = $value->toArray();
220226
}
221227
} else {
@@ -270,18 +276,42 @@ public static function groupForDatabase(array $queries): array
270276
/** @var Document|null $cursor */
271277
$cursor = $grouped->cursor;
272278

279+
// The base library's groupByType no longer tracks order attributes on
280+
// ParsedQuery — order clauses are consumed directly by the compiler.
281+
// Database adapters still take orderAttributes/orderTypes, so rebuild
282+
// them here from the pending query list.
283+
$orderAttributes = [];
284+
$orderTypes = [];
285+
foreach ($queries as $query) {
286+
$direction = match ($query->getMethod()) {
287+
Method::OrderAsc => OrderDirection::Asc,
288+
Method::OrderDesc => OrderDirection::Desc,
289+
Method::OrderRandom => OrderDirection::Random,
290+
default => null,
291+
};
292+
293+
if ($direction === null) {
294+
continue;
295+
}
296+
297+
$orderAttributes[] = $query->getAttribute();
298+
$orderTypes[] = $direction;
299+
}
300+
/** @var list<string> $groupBy */
301+
$groupBy = $grouped->groupBy;
302+
273303
return [
274304
'filters' => $filters,
275305
'selections' => $selections,
276306
'aggregations' => $aggregations,
277-
'groupBy' => $grouped->groupBy,
307+
'groupBy' => $groupBy,
278308
'having' => $having,
279309
'joins' => $joins,
280310
'distinct' => $grouped->distinct,
281311
'limit' => $grouped->limit,
282312
'offset' => $grouped->offset,
283-
'orderAttributes' => $grouped->orderAttributes,
284-
'orderTypes' => $grouped->orderTypes,
313+
'orderAttributes' => $orderAttributes,
314+
'orderTypes' => $orderTypes,
285315
'cursor' => $cursor,
286316
'cursorDirection' => $grouped->cursorDirection,
287317
];

‎src/Database/Validator/Queries.php‎

Lines changed: 90 additions & 78 deletions
Original file line numberDiff line numberDiff line change
@@ -63,6 +63,16 @@ public function isValid($value): bool
6363
return false;
6464
}
6565

66+
// Clear any aliases left over from a previous pass before collecting
67+
// this call's set. Order validators persist across requests in pooled
68+
// / long-lived processes; letting aliases accumulate leaks state and
69+
// lets an unrelated query order by a stale alias.
70+
foreach ($this->validators as $validator) {
71+
if ($validator instanceof Order) {
72+
$validator->resetAggregationAliases();
73+
}
74+
}
75+
6676
/** @var array<string> $aggregationAliases */
6777
$aggregationAliases = [];
6878
foreach ($value as $q) {
@@ -73,10 +83,7 @@ public function isValid($value): bool
7383
continue;
7484
}
7585
}
76-
if (\in_array($q->getMethod(), [
77-
Method::Count, Method::CountDistinct, Method::Sum, Method::Avg,
78-
Method::Min, Method::Max, Method::Stddev, Method::Variance,
79-
], true)) {
86+
if ($q->getMethod()->isAggregate()) {
8087
$alias = $q->getValue('');
8188
if (\is_string($alias) && $alias !== '') {
8289
$aggregationAliases[] = $alias;
@@ -102,7 +109,11 @@ public function isValid($value): bool
102109
}
103110
}
104111

105-
if ($query->isNested() && $query->getMethod() !== Method::Having) {
112+
// Only logical filter wrappers carry a list of sibling filters to
113+
// re-validate. Having has its own handling; Union/UnionAll wrap
114+
// sub-SELECTs whose children are not filters for this collection
115+
// and must not be recursed into here.
116+
if (\in_array($query->getMethod(), Query::LOGICAL_TYPES, true)) {
106117
/** @var array<Query|string> $nestedValues */
107118
$nestedValues = $query->getValues();
108119
if (! self::isValid($nestedValues)) {
@@ -111,79 +122,80 @@ public function isValid($value): bool
111122
}
112123

113124
$method = $query->getMethod();
114-
$methodType = match ($method) {
115-
Method::Select => Base::METHOD_TYPE_SELECT,
116-
Method::Limit => Base::METHOD_TYPE_LIMIT,
117-
Method::Offset => Base::METHOD_TYPE_OFFSET,
118-
Method::CursorAfter,
119-
Method::CursorBefore => Base::METHOD_TYPE_CURSOR,
120-
Method::OrderAsc,
121-
Method::OrderDesc,
122-
Method::OrderRandom => Base::METHOD_TYPE_ORDER,
123-
Method::Equal,
124-
Method::NotEqual,
125-
Method::LessThan,
126-
Method::LessThanEqual,
127-
Method::GreaterThan,
128-
Method::GreaterThanEqual,
129-
Method::Search,
130-
Method::NotSearch,
131-
Method::IsNull,
132-
Method::IsNotNull,
133-
Method::Between,
134-
Method::NotBetween,
135-
Method::StartsWith,
136-
Method::NotStartsWith,
137-
Method::EndsWith,
138-
Method::NotEndsWith,
139-
Method::Contains,
140-
Method::ContainsAny,
141-
Method::NotContains,
142-
Method::And,
143-
Method::Or,
144-
Method::ContainsAll,
145-
Method::ElemMatch,
146-
Method::Crosses,
147-
Method::NotCrosses,
148-
Method::DistanceEqual,
149-
Method::DistanceNotEqual,
150-
Method::DistanceGreaterThan,
151-
Method::DistanceLessThan,
152-
Method::Intersects,
153-
Method::NotIntersects,
154-
Method::Overlaps,
155-
Method::NotOverlaps,
156-
Method::Touches,
157-
Method::NotTouches,
158-
Method::Covers,
159-
Method::NotCovers,
160-
Method::SpatialEquals,
161-
Method::NotSpatialEquals,
162-
Method::VectorDot,
163-
Method::VectorCosine,
164-
Method::VectorEuclidean,
165-
Method::Regex,
166-
Method::Exists,
167-
Method::NotExists => Base::METHOD_TYPE_FILTER,
168-
Method::Count,
169-
Method::CountDistinct,
170-
Method::Sum,
171-
Method::Avg,
172-
Method::Min,
173-
Method::Max,
174-
Method::Stddev,
175-
Method::Variance => Base::METHOD_TYPE_AGGREGATE,
176-
Method::Distinct => Base::METHOD_TYPE_DISTINCT,
177-
Method::GroupBy => Base::METHOD_TYPE_GROUP_BY,
178-
Method::Having => Base::METHOD_TYPE_HAVING,
179-
Method::Join,
180-
Method::LeftJoin,
181-
Method::RightJoin,
182-
Method::CrossJoin,
183-
Method::FullOuterJoin,
184-
Method::NaturalJoin => Base::METHOD_TYPE_JOIN,
185-
default => '',
186-
};
125+
126+
// Route every aggregate method through the single source of truth
127+
// on the base enum. Previously this match hand-listed only half
128+
// of the aggregate methods, silently rejecting stddevPop, varPop,
129+
// bitAnd, etc. with "Invalid query method".
130+
if ($method->isAggregate()) {
131+
$methodType = Base::METHOD_TYPE_AGGREGATE;
132+
} else {
133+
$methodType = match ($method) {
134+
Method::Select => Base::METHOD_TYPE_SELECT,
135+
Method::Limit => Base::METHOD_TYPE_LIMIT,
136+
Method::Offset => Base::METHOD_TYPE_OFFSET,
137+
Method::CursorAfter,
138+
Method::CursorBefore => Base::METHOD_TYPE_CURSOR,
139+
Method::OrderAsc,
140+
Method::OrderDesc,
141+
Method::OrderRandom => Base::METHOD_TYPE_ORDER,
142+
Method::Equal,
143+
Method::NotEqual,
144+
Method::LessThan,
145+
Method::LessThanEqual,
146+
Method::GreaterThan,
147+
Method::GreaterThanEqual,
148+
Method::Search,
149+
Method::NotSearch,
150+
Method::IsNull,
151+
Method::IsNotNull,
152+
Method::Between,
153+
Method::NotBetween,
154+
Method::StartsWith,
155+
Method::NotStartsWith,
156+
Method::EndsWith,
157+
Method::NotEndsWith,
158+
Method::Contains,
159+
Method::ContainsAny,
160+
Method::NotContains,
161+
Method::And,
162+
Method::Or,
163+
Method::ContainsAll,
164+
Method::ElemMatch,
165+
Method::Crosses,
166+
Method::NotCrosses,
167+
Method::DistanceEqual,
168+
Method::DistanceNotEqual,
169+
Method::DistanceGreaterThan,
170+
Method::DistanceLessThan,
171+
Method::Intersects,
172+
Method::NotIntersects,
173+
Method::Overlaps,
174+
Method::NotOverlaps,
175+
Method::Touches,
176+
Method::NotTouches,
177+
Method::Covers,
178+
Method::NotCovers,
179+
Method::SpatialEquals,
180+
Method::NotSpatialEquals,
181+
Method::VectorDot,
182+
Method::VectorCosine,
183+
Method::VectorEuclidean,
184+
Method::Regex,
185+
Method::Exists,
186+
Method::NotExists => Base::METHOD_TYPE_FILTER,
187+
Method::Distinct => Base::METHOD_TYPE_DISTINCT,
188+
Method::GroupBy => Base::METHOD_TYPE_GROUP_BY,
189+
Method::Having => Base::METHOD_TYPE_HAVING,
190+
Method::Join,
191+
Method::LeftJoin,
192+
Method::RightJoin,
193+
Method::CrossJoin,
194+
Method::FullOuterJoin,
195+
Method::NaturalJoin => Base::METHOD_TYPE_JOIN,
196+
default => '',
197+
};
198+
}
187199

188200
$methodIsValid = false;
189201
foreach ($this->validators as $validator) {

‎src/Database/Validator/Query/Filter.php‎

Lines changed: 17 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -41,9 +41,11 @@ public function __construct(
4141
/** @var string $attrKey */
4242
$attrKey = $attribute->getAttribute('key', $attribute->getId());
4343
$copy = $attribute->getArrayCopy();
44-
// Convert type string to ColumnType enum for typed comparisons
44+
// Convert known type strings to ColumnType enum for typed comparisons.
45+
// Unknown strings are preserved as-is so the downstream switch can
46+
// emit a recoverable "Unknown Data type" error instead of throwing.
4547
if (isset($copy['type']) && \is_string($copy['type'])) {
46-
$copy['type'] = ColumnType::from($copy['type']);
48+
$copy['type'] = ColumnType::tryFrom($copy['type']) ?? $copy['type'];
4749
}
4850
$this->schema[$attrKey] = $copy;
4951
}
@@ -135,12 +137,6 @@ protected function isValidAttributeAndValues(string $attribute, array $values, M
135137
return false;
136138
}
137139

138-
if (! $this->supportForAttributes && ! isset($this->schema[$attribute])) {
139-
return true;
140-
}
141-
/** @var array<string, mixed> $attributeSchema */
142-
$attributeSchema = $this->schema[$attribute];
143-
144140
/** @var ColumnType|null $attributeType */
145141
$attributeType = $attributeSchema['type'] ?? null;
146142

@@ -167,10 +163,16 @@ protected function isValidAttributeAndValues(string $attribute, array $values, M
167163
case ColumnType::Text:
168164
case ColumnType::MediumText:
169165
case ColumnType::LongText:
166+
case ColumnType::Uuid7:
167+
case ColumnType::Enum:
168+
case ColumnType::Json:
169+
case ColumnType::Binary:
170170
$validator = new Text(0, 0);
171171
break;
172172

173173
case ColumnType::Integer:
174+
case ColumnType::Serial:
175+
case ColumnType::SmallSerial:
174176
/** @var int $size */
175177
$size = $attributeSchema['size'] ?? 4;
176178
/** @var bool $signed */
@@ -181,6 +183,12 @@ protected function isValidAttributeAndValues(string $attribute, array $values, M
181183
$validator = new Integer(false, $bits, $unsigned);
182184
break;
183185

186+
case ColumnType::BigInteger:
187+
case ColumnType::BigSerial:
188+
$validator = new Integer(false, 64, false);
189+
break;
190+
191+
case ColumnType::Float:
184192
case ColumnType::Double:
185193
$validator = new FloatValidator();
186194
break;
@@ -190,6 +198,7 @@ protected function isValidAttributeAndValues(string $attribute, array $values, M
190198
break;
191199

192200
case ColumnType::Datetime:
201+
case ColumnType::Timestamp:
193202
$validator = new DatetimeValidator(
194203
min: $this->minAllowedDate,
195204
max: $this->maxAllowedDate

0 commit comments

Comments
 (0)