Remove spatial attributes injection #719
Conversation
WalkthroughRemoved the attributes array parameter from SQL condition builders and moved attribute-type awareness into Query; adapters (MariaDB/Postgres) now use Query::isSpatialAttribute and Query::getAttributeType for spatial handling. Database::convertQuery sets Query attributeType. One test switched an existence check to a try-catch delete. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Client
participant Database
participant Query
participant Adapter as Adapter (MariaDB/Postgres)
participant Spatial as Spatial Handler
Client->>Database: request conversion / build Query
Database->>Query: setOnArray(...), setAttributeType(attribute.type)
Client->>Adapter: getSQLCondition(Query, binds)
Adapter->>Query: isSpatialAttribute()
alt spatial attribute
Adapter->>Query: getAttributeType()
Adapter->>Spatial: handleSpatialQueries(query, binds, attribute, type, alias, placeholder)
Spatial-->>Adapter: spatial SQL fragment
Adapter-->>Client: composed SQL fragment
else non-spatial attribute
Adapter-->>Client: standard SQL fragment
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests
📜 Recent review detailsConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (10)
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. Comment |
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/Database/Database.php (1)
2041-2051: Logic bug: spatial defaults always trigger “Unknown attribute type”.Spatial branch lacks a break and falls into default, throwing erroneously whenever a spatial attribute defines a default. Add a break after validating array defaults.
case self::VAR_POINT: case self::VAR_LINESTRING: case self::VAR_POLYGON: // Spatial types expect arrays as default values if ($defaultType !== 'array') { throw new DatabaseException('Default value for spatial type ' . $type . ' must be an array'); } - // no break - default: - throw new DatabaseException('Unknown attribute type: ' . $type . '. Must be one of ' . self::VAR_STRING . ', ' . self::VAR_INTEGER . ', ' . self::VAR_FLOAT . ', ' . self::VAR_BOOLEAN . ', ' . self::VAR_DATETIME . ', ' . self::VAR_RELATIONSHIP . ', ' . self::VAR_POINT . ', ' . self::VAR_LINESTRING . ', ' . self::VAR_POLYGON); + break; + default: + throw new DatabaseException( + 'Unknown attribute type: ' . $type . '. Must be one of ' . + self::VAR_STRING . ', ' . self::VAR_INTEGER . ', ' . self::VAR_FLOAT . ', ' . + self::VAR_BOOLEAN . ', ' . self::VAR_DATETIME . ', ' . self::VAR_RELATIONSHIP . ', ' . + self::VAR_POINT . ', ' . self::VAR_LINESTRING . ', ' . self::VAR_POLYGON + );Based on learnings
🧹 Nitpick comments (3)
tests/e2e/Adapter/Scopes/GeneralTests.php (1)
440-447: Avoid swallowing real setup errors; narrow the catch or drop the unused variable.Catching Throwable without handling hides genuine failures (permissions, connectivity). If the intent is “best‑effort cleanup”, consider catching only the expected not‑found exception type, or at minimum remove the unused variable.
Apply to drop the unused variable:
- } catch (\Throwable $e) { + } catch (\Throwable) { }If you prefer to narrow, replace Throwable with the specific adapter exception class you expect here.
src/Database/Query.php (1)
939-962: Harden spatial-type checkUse strict comparison in in_array to avoid edge cases if non-string values flow in.
- public function isSpatialAttribute(): bool - { - return in_array($this->attributeType, Database::SPATIAL_TYPES); - } + public function isSpatialAttribute(): bool + { + return in_array($this->attributeType, Database::SPATIAL_TYPES, true); + }src/Database/Adapter/Postgres.php (1)
1609-1615: Prefix alias in full-text SEARCH/NOT_SEARCHThese branches reference only the quoted column name without the table alias. For consistency with the rest of the method and to avoid ambiguity, prefix with {$alias}.
- return "to_tsvector(regexp_replace({$attribute}, '[^\\w]+',' ','g')) @@ websearch_to_tsquery(:{$placeholder}_0)"; + return "to_tsvector(regexp_replace({$alias}.{$attribute}, '[^\\w]+',' ','g')) @@ websearch_to_tsquery(:{$placeholder}_0)";- return "NOT (to_tsvector(regexp_replace({$attribute}, '[^\\w]+',' ','g')) @@ websearch_to_tsquery(:{$placeholder}_0))"; + return "NOT (to_tsvector(regexp_replace({$alias}.{$attribute}, '[^\\w]+',' ','g')) @@ websearch_to_tsquery(:{$placeholder}_0))";Also applies to: 1612-1615
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
src/Database/Adapter/MariaDB.php(3 hunks)src/Database/Adapter/Postgres.php(3 hunks)src/Database/Adapter/SQL.php(5 hunks)src/Database/Database.php(1 hunks)src/Database/Query.php(2 hunks)tests/e2e/Adapter/Scopes/GeneralTests.php(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (6)
tests/e2e/Adapter/Scopes/GeneralTests.php (4)
src/Database/Database.php (2)
getDatabase(779-782)delete(1300-1314)src/Database/Adapter.php (2)
getDatabase(160-163)delete(515-515)tests/e2e/Adapter/Base.php (1)
getDatabase(36-36)src/Database/Mirror.php (1)
delete(192-195)
src/Database/Adapter/SQL.php (3)
src/Database/Adapter/MariaDB.php (1)
getSQLCondition(1485-1578)src/Database/Adapter/Postgres.php (1)
getSQLCondition(1581-1674)src/Database/Query.php (3)
Query(8-1116)getValues(167-170)getMethod(151-154)
src/Database/Adapter/Postgres.php (3)
src/Database/Adapter/MariaDB.php (1)
getSQLCondition(1485-1578)src/Database/Adapter/SQL.php (1)
getSQLCondition(1799-1799)src/Database/Query.php (2)
Query(8-1116)isSpatialAttribute(958-961)
src/Database/Database.php (2)
src/Database/Query.php (3)
getAttribute(159-162)setOnArray(934-937)setAttributeType(943-946)src/Database/Document.php (1)
getAttribute(224-231)
src/Database/Query.php (1)
src/Database/Database.php (1)
Database(37-7359)
src/Database/Adapter/MariaDB.php (3)
src/Database/Adapter/Postgres.php (2)
getSQLCondition(1581-1674)handleSpatialQueries(1510-1571)src/Database/Adapter/SQL.php (1)
getSQLCondition(1799-1799)src/Database/Query.php (3)
Query(8-1116)isSpatialAttribute(958-961)getAttributeType(951-954)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Setup & Build Docker Image
🔇 Additional comments (7)
tests/e2e/Adapter/Scopes/GeneralTests.php (1)
427-429: Docblock addition looks good.Local Database typing improves IDE/static analysis in this test.
src/Database/Database.php (1)
7114-7130: Confirm Postgres spatial handler consumes attributeType
MariaDB adapter passes attributeType; please verify Postgres’s handleSpatialQueries readsQuery::getAttributeType()internally:rg -nP -C3 'getAttributeType\(' src/Database/Adapter/Postgres.phpsrc/Database/Query.php (1)
113-115: New attributeType state looks goodProperty addition is straightforward and aligns with adapter usage.
src/Database/Adapter/Postgres.php (1)
1581-1594: Signature update and spatial gate: LGTMRemoving the attributes param and switching to $query->isSpatialAttribute() is consistent with the new Query API and SQL base class.
src/Database/Adapter/SQL.php (2)
1799-1800: Abstract and helper signatures aligned: LGTMRemoving the attributes parameter tightens the API; recursive/nested calls updated correctly.
Also applies to: 1808-1820
2419-2421: Call-site updates: LGTM; optional sanity checkfind/count/sum now call getSQLConditions without attributes as expected. If you want extra safety, run the repo-wide check above to ensure no missed call-sites.
Also applies to: 2543-2546, 2619-2622
src/Database/Adapter/MariaDB.php (1)
1485-1497: LGTM – getSQLCondition signature and spatial branch align with Query changes, and no outdated call-sites remain.
Summary by CodeRabbit
New Features
Refactor
Tests