Native PostGIS Support for Prisma#5797
Open
lh0x00 wants to merge 7 commits into
Open
Conversation
|
|
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughThis pull request adds comprehensive PostGIS geometry support to Prisma, extending the schema language with ChangesPostGIS Geometry Feature
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes ✨ Finishing Touches🧪 Generate unit tests (beta)
✨ Simplify code
|
This was referenced Mar 20, 2026
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 14
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@psl/parser-database/src/types.rs`:
- Around line 253-275: The postgres_sql_type method on GeometrySpec currently
collapses omitted SRID and SRID 0 by using self.srid.unwrap_or(0); change
postgres_sql_type so it preserves None by emitting "geometry(subtype)" or
"geography(subtype)" when srid is None and only appends ",{srid}" when self.srid
is Some(value); locate the srid handling in GeometrySpec::postgres_sql_type
(references: GeometrySpec, postgres_sql_type, srid, unwrap_or(0)) and update the
formatting logic to conditionally include the ",{srid}" piece instead of
unwrapping to 0.
In `@psl/psl-core/src/builtin_connectors/postgres_datamodel_connector.rs`:
- Around line 86-88: The helper geometry_sql_column_type simply forwards to
spec.postgres_sql_type() and should be inlined: remove or stop using
geometry_sql_column_type and replace its callers with direct calls to
spec.postgres_sql_type() (i.e., call the GeometrySpec method directly wherever
geometry_sql_column_type(...) is used), ensuring imports/signatures remain
correct and no other logic is lost.
In `@psl/schema-ast/src/parser/parse_types.rs`:
- Around line 78-86: The SRID parsing currently accepts any i32 (via
raw.parse::<i32>()), allowing negative values; after successful parse in the
match arm for Ok(v) add a range check that returns a
DatamodelError::new_validation_error (using the same (file_id,
srid_pair.as_span()).into() span) if v is negative or > 999_999, and only return
Some(v) when 0 <= v <= 999_999; update the error message to indicate an
out-of-range SRID such as "Invalid SRID: expected a value between 0 and 999999."
In `@quaint/.github/workflows/test.yml`:
- Around line 58-63: The workflow currently contains hardcoded DSNs for
TEST_MYSQL, TEST_MYSQL8, TEST_MYSQL_MARIADB, TEST_PSQL, TEST_MSSQL, and
TEST_CRDB; replace these inline credentials with references to CI secrets or
repo/organization vars (e.g. use ${{ secrets.TEST_MYSQL }} / ${{ vars.TEST_MYSQL
}}), or if these values are intentionally test-only and non-sensitive, add a
scoped secret-scan/Checkov skip annotation with a brief justification tied to
the specific env keys to suppress the finding; update the env entries for
TEST_MYSQL, TEST_MYSQL8, TEST_MYSQL_MARIADB, TEST_PSQL, TEST_MSSQL, and
TEST_CRDB accordingly.
- Around line 16-18: The workflow currently uses the deprecated action reference
"actions-rs/toolchain@v1"; replace it with the maintained
"dtolnay/rust-toolchain@stable" (or alternatively
"actions-rust-lang/setup-rust-toolchain@v1") and update the inputs accordingly:
remove or adapt unsupported keys like components and override, keep toolchain:
stable (or use the new input name if different), and ensure clippy is installed
via the new action's documented inputs or a separate step (rustup component add
clippy) so CI behavior remains the same.
In `@query-compiler/core-tests/tests/geometry_find_many_graph_builds.rs`:
- Around line 13-16: The test's generator block declares previewFeatures =
["relationJoins"] which is unrelated to geometry query compilation; remove
"relationJoins" from the previewFeatures array in the generator client
declaration (the `generator client { ... previewFeatures = [...] }` block) to
keep the test focused unless some other part of this test explicitly depends on
relationJoins.
In `@query-compiler/core/src/query_document/parser.rs`:
- Around line 421-423: The code accepts PrismaValue::Json for
ScalarType::Geometry in parser.rs (the match arm that converts Json to Bytes)
but prisma_value_type_matches_scalar_type() still rejects PrismaValueType::Json
for Geometry, creating inconsistent behavior; update
prisma_value_type_matches_scalar_type() to treat PrismaValueType::Json as valid
for ScalarType::Geometry (alongside Bytes and String) so placeholder and inlined
paths agree, or alternatively remove the Json-to-Bytes coercion in the parser to
preserve the original policy—pick one consistent approach and apply it to the
function prisma_value_type_matches_scalar_type() and the parser.rs match arms
(PrismaValue::Json handling or its removal) so both places use the same Geometry
JSON policy.
In `@query-compiler/dmmf/src/ast_builders/datamodel_ast_builder.rs`:
- Around line 13-15: The helper function geometry_dmmf_field_type merely
forwards to spec.postgres_sql_type(); inline it by replacing all calls to
geometry_dmmf_field_type(spec) with spec.postgres_sql_type() and then remove the
geometry_dmmf_field_type function definition to avoid unnecessary indirection
(look for calls to geometry_dmmf_field_type and the GeometrySpec type/method to
update).
In `@query-compiler/dmmf/src/tests/tests.rs`:
- Around line 44-51: Add test coverage for the optional "path" field similar to
the existing "position" check: locate the field via the same
fields.iter().find(...) pattern (use a new variable path_field), extract its
outputType into out_path, and assert that out_path.get("type").and_then(|t|
t.as_str()) equals Some("geometry(LineString,0)"); this mirrors the existing
pos_field/out_pos assertions and ensures optional geometry fields are validated.
In
`@query-compiler/schema/src/build/input_types/fields/data_input_mapper/update.rs`:
- Around line 48-50: The update uses the wrong envelope identifier for geometry:
in the match arm for TypeIdentifier::Geometry(...) you call
InputType::object(update_operations_object_type(ctx, "Bytes", sf.clone(),
false)) which reuses the "Bytes" prefix and collides with bytes field update
types; change the prefix to a geometry-specific name (e.g., "Geometry") so the
call becomes update_operations_object_type(ctx, "Geometry", sf.clone(), false)
to produce a distinct GeometryFieldUpdateOperationsInput; ensure this aligns
with map_scalar_input_type() which maps geometry to ScalarType::Geometry so the
update operations identifier is unique.
In `@schema-engine/sql-introspection-tests/tests/postgres/postgis_geometry.rs`:
- Line 25: Replace the weak compound assertion in the postgis_geometry.rs test
that uses schema.contains("path") && schema.contains("Geometry(LineString)")
with a single assertion that verifies the field name and its type appear
together; for example, use a Regex to assert that the string "path" is followed
by "Geometry(LineString)" (and nullable marker if applicable) in the same
declaration — i.e., remove the current assert!(schema.contains(... && ...)) and
instead create a Regex (via regex::Regex::new) that matches something like
r"path\s+Geometry\(LineString\)\??" and assert that re.is_match(&schema),
referencing the existing schema variable.
In
`@schema-engine/sql-migration-tests/tests/migrations/postgres/postgis_geometry.rs`:
- Around line 86-87: The two brittle string assertions using the introspected
variable (assert!(introspected.contains("Geometry(Point, 4326)")) and
assert!(introspected.contains("Geometry(LineString, 4326)"))) should be replaced
with more robust checks: either parse the introspected schema into a structured
representation and assert the geometry type and SRID for the relevant field(s),
or use flexible pattern matching (e.g., regex) that tolerates whitespace/format
variations (e.g., match /Geometry\s*\(\s*Point\s*,\s*4326\s*\)/). Update the
test to locate the Geometry definitions by name in the parsed output (or by
applying the regex) and assert the type and SRID rather than relying on exact
string spacing.
In `@schema-engine/sql-schema-describer/src/postgres.rs`:
- Around line 1566-1574: The current SRID parsing in the RE_TWO capture block
silently defaults to 0 on parse failure; change it so that if caps.get(3) exists
but parse fails you log a warning and do not silently return 0. Concretely,
inside the RE_TWO handling (the block using trimmed, caps, map_geometry_subtype
and constructing GeometrySpec), replace the single .and_then(...).unwrap_or(0)
with logic that checks caps.get(3): if None -> leave srid as None; if Some(text)
-> attempt to parse to i32 and on Ok(v) set srid = Some(v), on Err(e) emit a
warning (using the crate’s logging/tracing facility) and set srid = None (or
otherwise avoid using 0). Ensure GeometrySpec is constructed with that
Option<i32> srid and keep subtype mapping via map_geometry_subtype unchanged.
In `@schema-engine/sql-schema-describer/src/postgres/default.rs`:
- Around line 104-106: Consolidate the match arms that route to
parse_unsupported by adding ColumnTypeFamily::Geometry(_) into the existing
pattern so all unsupported families share a single arm; update the match over
ColumnTypeFamily (where parse_unsupported is referenced) to use a combined
pattern like ColumnTypeFamily::Udt(_) | ColumnTypeFamily::Unsupported(_) |
ColumnTypeFamily::Uuid | ColumnTypeFamily::Geometry(_) => &parse_unsupported so
the code is cleaner and consistent.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 94f599c0-c079-4b00-ae47-62a3a7d854f7
⛔ Files ignored due to path filters (2)
Cargo.lockis excluded by!**/*.lockquery-compiler/query-compiler/tests/snapshots/queries__queries@geometry-find-many.json.snapis excluded by!**/*.snap
📒 Files selected for processing (53)
psl/parser-database/Cargo.tomlpsl/parser-database/src/attributes/default.rspsl/parser-database/src/lib.rspsl/parser-database/src/types.rspsl/psl-core/src/builtin_connectors/capabilities_support.rspsl/psl-core/src/builtin_connectors/postgres_datamodel_connector.rspsl/psl-core/src/datamodel_connector/capabilities.rspsl/psl-core/src/validate/validation_pipeline/validations.rspsl/psl-core/src/validate/validation_pipeline/validations/fields.rspsl/schema-ast/src/ast.rspsl/schema-ast/src/ast/field.rspsl/schema-ast/src/parser/datamodel.pestpsl/schema-ast/src/parser/parse_types.rsquaint/.github/workflows/test.ymlquery-compiler/core-tests/Cargo.tomlquery-compiler/core-tests/tests/geometry_find_many_graph_builds.rsquery-compiler/core/src/constants.rsquery-compiler/core/src/query_document/parser.rsquery-compiler/dmmf/Cargo.tomlquery-compiler/dmmf/src/ast_builders/datamodel_ast_builder.rsquery-compiler/dmmf/src/ast_builders/schema_ast_builder/type_renderer.rsquery-compiler/dmmf/src/tests/tests.rsquery-compiler/query-builders/sql-query-builder/src/convert.rsquery-compiler/query-builders/sql-query-builder/src/model_extensions/scalar_field.rsquery-compiler/query-compiler/src/data_mapper.rsquery-compiler/query-compiler/tests/data/geometry-find-many.jsonquery-compiler/query-compiler/tests/data/schema.prismaquery-compiler/query-structure/src/field/mod.rsquery-compiler/query-structure/src/field/scalar.rsquery-compiler/query-structure/src/prisma_value_ext.rsquery-compiler/request-handlers/src/protocols/json/protocol_adapter.rsquery-compiler/schema/src/build/input_types/fields/data_input_mapper/update.rsquery-compiler/schema/src/build/input_types/fields/field_filter_types.rsquery-compiler/schema/src/build/input_types/mod.rsquery-compiler/schema/src/build/output_types/field.rsquery-compiler/schema/src/output_types.rsquery-compiler/schema/src/query_schema.rsschema-engine/connectors/sql-schema-connector/src/flavour/postgres/renderer.rsschema-engine/connectors/sql-schema-connector/src/flavour/postgres/schema_differ.rsschema-engine/connectors/sql-schema-connector/src/flavour/sqlite/renderer.rsschema-engine/connectors/sql-schema-connector/src/introspection/introspection_pair/scalar_field.rsschema-engine/connectors/sql-schema-connector/src/sql_schema_calculator.rsschema-engine/sql-introspection-tests/tests/postgres/mod.rsschema-engine/sql-introspection-tests/tests/postgres/postgis_geometry.rsschema-engine/sql-migration-tests/tests/migrations/postgres.rsschema-engine/sql-migration-tests/tests/migrations/postgres/postgis_geometry.rsschema-engine/sql-schema-describer/src/lib.rsschema-engine/sql-schema-describer/src/mssql.rsschema-engine/sql-schema-describer/src/mysql.rsschema-engine/sql-schema-describer/src/postgres.rsschema-engine/sql-schema-describer/src/postgres/default.rsschema-engine/sql-schema-describer/src/postgres/default/c_style_scalar_lists.rsschema-engine/sql-schema-describer/src/sqlite.rs
There was a problem hiding this comment.
Actionable comments posted: 10
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@query-compiler/core/src/query_graph_builder/extractors/filters/scalar.rs`:
- Around line 190-202: The geometry filters (parse_geometry_near,
parse_geometry_within, parse_geometry_intersects) are created unconditionally as
Filter::Geometry and do not respect the self.reverse() flag; update the branches
handling filters::NEAR, filters::WITHIN, and filters::INTERSECTS to check
self.reverse() and either (a) construct the appropriate negated filter/field
operation when reverse() is true or (b) return a validation error if negation is
unsupported for geometry filters—choose one approach and implement it
consistently for all three parsers so NOT semantics are handled correctly.
In `@query-compiler/core/src/query_graph_builder/extractors/query_arguments.rs`:
- Around line 403-427: extract_float_from_pv and extract_int_from_pv duplicate
logic already implemented as extract_float and extract_int in the scalar filter
helpers; extract the shared parsing logic into a new utility (e.g., pv_parsers
or value_conversions) and move the common code for parsing PrismaValue->f64 and
PrismaValue->i32 there, then update extract_float_from_pv and
extract_int_from_pv to call the new shared functions (and similarly update
extract_float/extract_int to use them) so all modules reuse the single
implementation.
In `@query-compiler/query-builders/sql-query-builder/src/cursor_condition.rs`:
- Line 445: Replace the panic in the match arm for OrderBy::Geometry in
cursor_condition.rs with a structured error return: update the OrderBy::Geometry
arm to return a proper Err variant (use the module's existing query/error type,
e.g., an UnsupportedOperation or InvalidCursor error with a descriptive message
like "cursor-based pagination with geometry orderBy is not supported"), and if
the enclosing function (e.g., the cursor-building function that matches on
OrderBy) does not already return Result<..., YourErrorType>, change its
signature to return Result and propagate errors accordingly so callers can
handle this gracefully instead of panicking.
In `@query-compiler/query-builders/sql-query-builder/src/filter/visitor.rs`:
- Around line 620-623: In visit_geometry_filter, the generated field_ref
currently wraps field_column.name in quotes but doesn't escape internal double
quotes; update the construction of field_ref to double any internal '"'
characters (e.g., replace '"' with '""') or call the existing utility
(Quoted::postgres_ident) used elsewhere to render PostgreSQL identifiers, so
field_ref becomes a properly escaped PostgreSQL identifier for
field_column.name.
In `@query-compiler/query-builders/sql-query-builder/src/ordering.rs`:
- Around line 299-309: The SQL builder currently wraps field_column.name in
double quotes without escaping internal quote characters, which can break
Postgres identifiers; update the code that constructs field_ref (used when
building the ST_Distance SQL and producing distance_expr) to properly escape
internal double quotes by doubling them (replace each `"` with `""`) before
surrounding with outer quotes, and apply the same change to the other
occurrences noted in filter/visitor.rs so all identifier interpolations use this
escaping (or switch to a shared identifier-escaping helper function to
centralize the logic).
In `@query-compiler/query-structure/src/filter/geometry.rs`:
- Around line 26-54: The implementation violates Eq/Hash because f64 NaN values
make derived PartialEq/Eq unsafe and hashing Intersects.geometry via to_string()
is inefficient; fix by writing a manual PartialEq that compares f64 fields using
to_bits() (for Near: compare point.0.to_bits(), point.1.to_bits(),
max_distance.to_bits(), srid; for Within: compare each polygon coordinate via
to_bits(); for Intersects: compare a canonical byte representation), keep or add
impl Eq only after ensuring PartialEq is total, and change the Hash impl to hash
the same bitwise f64 values and, for Intersects.geometry, hash a stable
binary/serialized representation (e.g., serde_json::to_vec or a canonical binary
form) instead of geometry.to_string() to avoid performance and nondeterminism
issues; update GeometryFilterCondition matching in both PartialEq and Hash to
use these exact comparisons so equality and hashing are consistent.
In `@query-compiler/query-structure/src/order_by.rs`:
- Around line 234-266: The auto-derived PartialEq can be violated by NaN in the
point f64s while you implement Eq and a hash based on to_bits(); replace the
derive PartialEq with a manual impl of PartialEq for OrderByGeometry that
compares field, path, sort_order, srid, and compares point coordinates via their
to_bits() (e.g., self.point.0.to_bits() == other.point.0.to_bits() && same for
.1) so equality semantics match the Hash impl and Eq is sound; update/remove the
derive(PartialEq) and add the explicit impl to keep behavior consistent.
In `@query-compiler/schema/src/build/input_types/objects/order_by_objects.rs`:
- Around line 281-294: The point field in geometry_distance_from_input currently
uses InputType::list(InputType::float()) (constants::filters::POINT) which
allows arbitrary-length arrays; either enforce a fixed two-element coordinate
tuple at the schema level (replace the open list with a fixed-length/tuple type
if available) or, if the schema system lacks fixed-length lists, add explicit
validation where the point is consumed (the extractor that reads
constants::filters::POINT before calling ST_MakePoint) to check the vec length
== 2 and return a clear validation error for malformed input; update
geometry_distance_from_input or the consuming function accordingly so callers
get a descriptive error instead of passing invalid arrays to PostGIS.
In `@query-compiler/schema/src/identifier_type.rs`:
- Around line 322-325: Add a new enum variant
IdentifierType::GeometryOrderByInput to ensure consistency with the raw string
used in order_by_objects.rs (Identifier::new_prisma("GeometryOrderByInput"));
update the Display implementation in identifier_type.rs to return
"GeometryOrderByInput" for that variant, and search for any
conversions/serializations (e.g., From/Into implementations or parsing logic
that map identifier names to IdentifierType) to handle the new variant so usages
expecting the enum variant compile and behave the same as the existing
GeometryDistanceFromInput mapping.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: ae37045c-aa91-431d-98f8-c5b9b20c764a
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (15)
query-compiler/core/src/query_graph_builder/extractors/filters/scalar.rsquery-compiler/core/src/query_graph_builder/extractors/query_arguments.rsquery-compiler/query-builders/sql-query-builder/src/cursor_condition.rsquery-compiler/query-builders/sql-query-builder/src/filter/visitor.rsquery-compiler/query-builders/sql-query-builder/src/ordering.rsquery-compiler/query-builders/sql-query-builder/src/select/mod.rsquery-compiler/query-structure/Cargo.tomlquery-compiler/query-structure/src/filter/geometry.rsquery-compiler/query-structure/src/filter/mod.rsquery-compiler/query-structure/src/order_by.rsquery-compiler/query-structure/src/record.rsquery-compiler/schema/src/build/input_types/fields/field_filter_types.rsquery-compiler/schema/src/build/input_types/objects/order_by_objects.rsquery-compiler/schema/src/constants.rsquery-compiler/schema/src/identifier_type.rs
lh0x00
changed the title
This was referenced Mar 21, 2026
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
query-compiler/query-structure/src/order_by.rs (1)
26-33: 🧹 Nitpick | 🔵 TrivialAdd explicit
OrderBy::Geometry(_)handling to match expressions for consistency.The new
Geometryvariant is handled through wildcard patterns in several locations, while other files use explicit arms:
query_arguments.rs:218(contains_null_cursor) and line 301 (has_unbatchable_ordering) use wildcardsdistinct.rs:30uses_ => falserecord.rs:83explicitly handles withunimplemented!()For consistency and to catch accidental omissions when new variants are added, add explicit
OrderBy::Geometry(_)arms alongside the other aggregation types in the wildcard locations.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@query-compiler/query-structure/src/order_by.rs` around lines 26 - 33, Replace wildcard match arms that currently absorb the Geometry variant with an explicit OrderBy::Geometry(_) arm in the same places other variants are handled: update the matches in contains_null_cursor and has_unbatchable_ordering (both in query_arguments.rs) and the match in distinct.rs so they include OrderBy::Geometry(_) alongside ScalarAggregation, ToManyAggregation and Relevance; make the Geometry arm return the same boolean/result as the analogous aggregation arms (or false where `_ => false` was used) so behavior is unchanged and matches remain exhaustive and explicit.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@query-compiler/core-tests/tests/geometry-filters-graph-builds.rs`:
- Around line 239-292: The test currently expects successful graph construction
but the QueryGraphBuilder::build path rejects geometry filters under NOT; change
the final assertion to expect an error instead of success by calling
QueryGraphBuilder::new(&query_schema).build(query).expect_err(...) and then
assert the returned error message contains the expected validation text (lock
the exact message fragment produced by the extractor in
query_graph_builder::extractors::filters::scalar.rs) so the test verifies the
rejection rather than successful graph construction.
In `@query-compiler/core/src/query_graph_builder/extractors/filters/scalar.rs`:
- Around line 766-774: The extract_int function currently uses unchecked "as
i32" casts for PrismaValue::Int and PrismaValue::BigInt which silently truncate
out-of-range values; update extract_int (matching on PrismaValue::Int and
PrismaValue::BigInt) to perform checked conversions (e.g., use i32::try_from or
TryFrom) and return a QueryGraphBuilderError::InputError when the conversion
fails/overflows, keeping the same error message shape but indicating the value
is out of range so invalid SRIDs are rejected instead of wrapped/truncated.
In `@query-compiler/query-builders/sql-query-builder/src/filter/visitor.rs`:
- Around line 620-623: visit_geometry_filter constructs field_ref from the bare
column name (filter.field.as_column(ctx) -> field_column.name) and omits the
current alias, causing ambiguous or wrong table binding in subqueries; update
visit_geometry_filter to prepend the active alias (use self.parent_alias() or
the equivalent alias accessor) when building field_ref so it references
"alias"."column" (or alias-qualified form used elsewhere) instead of just the
column name, ensuring geometry predicates inside relation/self-relation
subqueries bind to the correct table.
- Around line 624-630: The code collapses a missing SRID to 4326: change the
srid binding to preserve the Option (don't unwrap_or(4326)) so that
GeometryFilterCondition::Near/Within/Intersects yields the original Option<SRID>
(e.g., let srid = match &filter.condition { ... => srid.clone() }); then compute
use_geography by checking the explicit srid when Some(s) (s == 4326 || s == 4269
|| s == 4167) and otherwise consult the geometry field's declared SRID (do not
assume 4326 when srid is None). Update the use_geography computation (and any
downstream casts) to branch on the Option instead of treating None as 4326.
In `@query-compiler/query-compiler/tests/data/geometry-count-with-filter.json`:
- Around line 1-26: The fixture file name implies a count test but the JSON uses
"modelName": "Location" with "action": "findMany" and no count selection; either
rename the fixture to reflect a findMany/within test (e.g., include "findMany"
or "within" in the filename) or change the payload to perform a count (set
"action" to "count" and include the appropriate count selection/arguments for
the within polygon filter) so the filename and the contents match; update
whichever you choose and ensure the "position" -> "within" -> "polygon" filter
remains intact.
---
Outside diff comments:
In `@query-compiler/query-structure/src/order_by.rs`:
- Around line 26-33: Replace wildcard match arms that currently absorb the
Geometry variant with an explicit OrderBy::Geometry(_) arm in the same places
other variants are handled: update the matches in contains_null_cursor and
has_unbatchable_ordering (both in query_arguments.rs) and the match in
distinct.rs so they include OrderBy::Geometry(_) alongside ScalarAggregation,
ToManyAggregation and Relevance; make the Geometry arm return the same
boolean/result as the analogous aggregation arms (or false where `_ => false`
was used) so behavior is unchanged and matches remain exhaustive and explicit.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 9f037221-a4da-4f78-911d-e2a1cbf62bae
⛔ Files ignored due to path filters (15)
query-compiler/query-compiler/tests/snapshots/queries__queries@geometry-combined-scalar-spatial.json.snapis excluded by!**/*.snapquery-compiler/query-compiler/tests/snapshots/queries__queries@geometry-count-with-filter.json.snapis excluded by!**/*.snapquery-compiler/query-compiler/tests/snapshots/queries__queries@geometry-delete-with-filter.json.snapis excluded by!**/*.snapquery-compiler/query-compiler/tests/snapshots/queries__queries@geometry-filter-and-orderby.json.snapis excluded by!**/*.snapquery-compiler/query-compiler/tests/snapshots/queries__queries@geometry-filter-and-scalar-filter.json.snapis excluded by!**/*.snapquery-compiler/query-compiler/tests/snapshots/queries__queries@geometry-filter-custom-srid.json.snapis excluded by!**/*.snapquery-compiler/query-compiler/tests/snapshots/queries__queries@geometry-filter-near.json.snapis excluded by!**/*.snapquery-compiler/query-compiler/tests/snapshots/queries__queries@geometry-filter-not-near.json.snapis excluded by!**/*.snapquery-compiler/query-compiler/tests/snapshots/queries__queries@geometry-filter-or-multiple.json.snapis excluded by!**/*.snapquery-compiler/query-compiler/tests/snapshots/queries__queries@geometry-filter-within.json.snapis excluded by!**/*.snapquery-compiler/query-compiler/tests/snapshots/queries__queries@geometry-find-many.json.snapis excluded by!**/*.snapquery-compiler/query-compiler/tests/snapshots/queries__queries@geometry-multiple-orderby.json.snapis excluded by!**/*.snapquery-compiler/query-compiler/tests/snapshots/queries__queries@geometry-orderby-distance-asc.json.snapis excluded by!**/*.snapquery-compiler/query-compiler/tests/snapshots/queries__queries@geometry-orderby-distance-desc.json.snapis excluded by!**/*.snapquery-compiler/query-compiler/tests/snapshots/queries__queries@geometry-orderby-with-limit.json.snapis excluded by!**/*.snap
📒 Files selected for processing (24)
psl/parser-database/src/types.rspsl/schema-ast/src/parser/parse_types.rsquery-compiler/core-tests/tests/geometry-filters-graph-builds.rsquery-compiler/core/src/query_graph_builder/extractors/filters/scalar.rsquery-compiler/query-builders/sql-query-builder/src/filter/visitor.rsquery-compiler/query-compiler/tests/data/geometry-combined-scalar-spatial.jsonquery-compiler/query-compiler/tests/data/geometry-count-with-filter.jsonquery-compiler/query-compiler/tests/data/geometry-delete-with-filter.jsonquery-compiler/query-compiler/tests/data/geometry-filter-and-orderby.jsonquery-compiler/query-compiler/tests/data/geometry-filter-and-scalar-filter.jsonquery-compiler/query-compiler/tests/data/geometry-filter-custom-srid.jsonquery-compiler/query-compiler/tests/data/geometry-filter-intersects.json.skipquery-compiler/query-compiler/tests/data/geometry-filter-near.jsonquery-compiler/query-compiler/tests/data/geometry-filter-not-near.jsonquery-compiler/query-compiler/tests/data/geometry-filter-or-multiple.jsonquery-compiler/query-compiler/tests/data/geometry-filter-within.jsonquery-compiler/query-compiler/tests/data/geometry-multiple-orderby.jsonquery-compiler/query-compiler/tests/data/geometry-orderby-distance-asc.jsonquery-compiler/query-compiler/tests/data/geometry-orderby-distance-desc.jsonquery-compiler/query-compiler/tests/data/geometry-orderby-with-limit.jsonquery-compiler/query-compiler/tests/data/schema.prismaquery-compiler/query-structure/src/filter/geometry.rsquery-compiler/query-structure/src/order_by.rsquery-compiler/schema/src/build/input_types/fields/data_input_mapper/update.rs
|
Hey @aidankmcalister |
|
@elrhourha is right the docs is displaying wrong information, quite confusing until I found this PR wasn't merged yet.. when it is expected to be merged? |
That explain everything. I spend 2 days trying to make it work. When is it expected to merge? |
Author
|
Hmm, maybe the PR for this feature was overlooked by the team? |
|
CodeRabbit chat interactions are restricted to organization members for this repository. Ask an organization member to interact with CodeRabbit, or set |
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (3)
query-compiler/query-builders/sql-query-builder/src/filter/visitor.rs (2)
622-623:⚠️ Potential issue | 🟠 Major | ⚡ Quick win
field_refstill dropsparent_alias().
visit_geometry_filterbuildsfield_reffrom the bare column name, so geometry predicates inside relation/self-relation subqueries will bind to the wrong table or hit ambiguous-column errors once joins are introduced. Every other path in this file (e.g.visit_scalar_filter,visit_scalar_list_filter) qualifies columns viaaliased_col(self.parent_alias(), ctx). This needs the same qualification, e.g. quoting"<alias>"."<column>"whenself.parent_alias()isSome.🛠️ Suggested change
- let field_column = filter.field.as_column(ctx); - let field_ref = format!("\"{}\"", field_column.name); + let field_column = filter.field.as_column(ctx); + let field_ref = match self.parent_alias() { + Some(alias) => format!("\"{}\".\"{}\"", alias, field_column.name), + None => format!("\"{}\"", field_column.name), + };🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@query-compiler/query-builders/sql-query-builder/src/filter/visitor.rs` around lines 622 - 623, visit_geometry_filter builds field_ref from the bare column name (using filter.field.as_column(ctx)) which drops self.parent_alias() and causes wrong/ambiguous bindings; change it to qualify the column the same way other visitors do by using aliased_col(self.parent_alias(), ctx) or otherwise prepend the quoted parent alias when present so field_ref becomes "\"<alias>\".\"<column>\"" when self.parent_alias() is Some, ensuring geometry predicates inside relation/self-relation subqueries reference the correct table.
625-631:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy lift
srid.unwrap_or(4326)still loses the field's declared SRID.
parse_geometry_*()usesNoneto mean "client did not override SRID", but this branch still rewrites that to4326for every geometry field. Non‑WGS84 columns will therefore emit mixed‑SRID SQL (e.g.ST_DWithin(col_in_3857, ST_SetSRID(..., 4326)::geography)) or an invalid::geographycast unless every query redundantly passessrid. Preserve theOption<i32>and fall back to the column's declared SRID before defaulting;use_geographyshould likewise branch on the resolved SRID.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@query-compiler/query-builders/sql-query-builder/src/filter/visitor.rs` around lines 625 - 631, The code currently forces srid = srid.unwrap_or(4326) losing the field's declared SRID; instead keep srid as Option<i32> (the value coming from filter.condition matching GeometryFilterCondition::Near/Within/Intersects) and only resolve it by first checking the explicit srid Option, then the geometry column's declared SRID, and finally defaulting to 4326; update the subsequent use_geography computation to branch on that resolved SRID (not on the original forced 4326) and ensure parse_geometry_*() callers receive the Option<i32> when they expect "client did not override" semantics so you don't emit mixed-SRID SQL or invalid ::geography casts.query-compiler/core/src/query_document/parser.rs (1)
421-423:⚠️ Potential issue | 🟠 Major | ⚡ Quick winJson/Geometry coercion still inconsistent with
prisma_value_type_matches_scalar_type.Line 423 still accepts
PrismaValue::JsonforScalarType::Geometry(_)(coercing to bytes vias.into_bytes()), but the updated helper at line 995 only mapsPrismaValueType::Json | PrismaValueType::ObjecttoScalarType::Json. Geometry is missing from theJson/Objectarm, so an inlined JSON geometry value goes through the coercion path while the same value arriving as a typedPlaceholder(or generator return) is rejected. The two paths still disagree.Additionally, the
s.into_bytes()conversion just gives the UTF‑8 bytes of the JSON text; that is neither WKT nor (E)WKB. If the intent is to support GeoJSON input for geometry columns, a proper GeoJSON→WKT/WKB step (or routing through a string codepath that downstream code parses as GeoJSON) is needed; otherwise these bytes will fail to bind on the database side. Either drop theJson → Geometryarm or fix both the helper and the conversion to a representation PostGIS will accept.🛠️ Sketch of one consistent option (drop the Json→Geometry arm)
- (pv @ PrismaValue::Bytes(_), &ScalarType::Geometry(_)) => Ok(pv), - (pv @ PrismaValue::String(_), &ScalarType::Geometry(_)) => Ok(pv), - (PrismaValue::Json(s), &ScalarType::Geometry(_)) => Ok(PrismaValue::Bytes(s.into_bytes())), + (pv @ PrismaValue::Bytes(_), &ScalarType::Geometry(_)) => Ok(pv), + (pv @ PrismaValue::String(_), &ScalarType::Geometry(_)) => Ok(pv),Also applies to: 979-1002
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@query-compiler/core/src/query_document/parser.rs` around lines 421 - 423, The PrismaValue::Json -> ScalarType::Geometry coercion (the match arm converting Json via s.into_bytes()) is inconsistent with prisma_value_type_matches_scalar_type and produces invalid UTF-8 bytes for PostGIS; remove the PrismaValue::Json => ScalarType::Geometry arm (or replace it with a real GeoJSON→WKB/WKT conversion if you intend to support GeoJSON) and make the change symmetric by ensuring prisma_value_type_matches_scalar_type does not map PrismaValueType::Json/ Object to Geometry (or does if you implement full GeoJSON conversion); update/align both the match in parser.rs (the PrismaValue match arms) and the helper (prisma_value_type_matches_scalar_type) so Json/Object are either both allowed for Geometry with proper conversion or both rejected.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@query-compiler/query-builders/sql-query-builder/src/filter/visitor.rs`:
- Around line 660-731: The Intersects branch in GeometryFilterCondition (in
visitor.rs) currently uses panic! for missing/invalid GeoJSON fields and
unsupported types; replace those panics with fallible handling: validate "type"
and "coordinates" and return a safe failure (e.g., propagate a ValidationError
or return ConditionTree::NegativeCondition/Err) from the function instead of
panicking, updating the code paths in the GeometryFilterCondition::Intersects
arm (and the helper used by Within, e.g., format_polygon_wkt callers) to produce
structured errors or negative conditions on bad input; ensure you escape WKT as
before but do not abort the process on malformed GeoJSON — return the
error/negative condition up the visitor so the caller can surface a proper
validation response.
- Around line 696-722: In the Polygon WKT generation (the format_polygon_wkt
function and the Intersects -> "Polygon" branch in visitor.rs), ensure each
linear ring is topologically closed by checking whether the first coordinate
equals the last and, if not, appending the first vertex to the end of the ring
before emitting the "(x y, ...)" sequence; additionally validate coordinates by
using f64::is_finite() for each numeric value instead of unconditionally using
as_f64().unwrap_or(0.0) so that NaN/inf are rejected (or return an error) early
rather than being formatted into WKT—update the ring-building logic in
format_polygon_wkt and the ring/point extraction in the Intersects "Polygon"
branch to perform these checks and fail fast on invalid coordinates.
---
Duplicate comments:
In `@query-compiler/core/src/query_document/parser.rs`:
- Around line 421-423: The PrismaValue::Json -> ScalarType::Geometry coercion
(the match arm converting Json via s.into_bytes()) is inconsistent with
prisma_value_type_matches_scalar_type and produces invalid UTF-8 bytes for
PostGIS; remove the PrismaValue::Json => ScalarType::Geometry arm (or replace it
with a real GeoJSON→WKB/WKT conversion if you intend to support GeoJSON) and
make the change symmetric by ensuring prisma_value_type_matches_scalar_type does
not map PrismaValueType::Json/ Object to Geometry (or does if you implement full
GeoJSON conversion); update/align both the match in parser.rs (the PrismaValue
match arms) and the helper (prisma_value_type_matches_scalar_type) so
Json/Object are either both allowed for Geometry with proper conversion or both
rejected.
In `@query-compiler/query-builders/sql-query-builder/src/filter/visitor.rs`:
- Around line 622-623: visit_geometry_filter builds field_ref from the bare
column name (using filter.field.as_column(ctx)) which drops self.parent_alias()
and causes wrong/ambiguous bindings; change it to qualify the column the same
way other visitors do by using aliased_col(self.parent_alias(), ctx) or
otherwise prepend the quoted parent alias when present so field_ref becomes
"\"<alias>\".\"<column>\"" when self.parent_alias() is Some, ensuring geometry
predicates inside relation/self-relation subqueries reference the correct table.
- Around line 625-631: The code currently forces srid = srid.unwrap_or(4326)
losing the field's declared SRID; instead keep srid as Option<i32> (the value
coming from filter.condition matching
GeometryFilterCondition::Near/Within/Intersects) and only resolve it by first
checking the explicit srid Option, then the geometry column's declared SRID, and
finally defaulting to 4326; update the subsequent use_geography computation to
branch on that resolved SRID (not on the original forced 4326) and ensure
parse_geometry_*() callers receive the Option<i32> when they expect "client did
not override" semantics so you don't emit mixed-SRID SQL or invalid ::geography
casts.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: bb79731e-41da-4a6f-ad13-eb7e93319750
📒 Files selected for processing (4)
query-compiler/core/src/query_document/parser.rsquery-compiler/query-builders/sql-query-builder/src/filter/visitor.rsschema-engine/sql-migration-tests/tests/migrations/postgres.rsschema-engine/sql-schema-describer/src/postgres/default.rs
SevInf
requested changes
May 21, 2026
Contributor
SevInf
left a comment
SevInf
left a comment
There was a problem hiding this comment.
HI @lh0x00
Thank you for the PR. I think few changes would need to happen before we can proceed. 3 of the biggest ones:
- Can we split
GeometryandGeographytypes? - Would it be possible to use
point Geometry @db.Geometry(Point, 4032)to be consistent with existing PSL parametrized types - User input needs to be properly escaped within spatial filtering code.
|
|
||
| /// PostGIS base type for a [`GeometrySpec`] (`geometry` vs `geography` in PostgreSQL). | ||
| #[derive(Debug, Clone, Copy, PartialEq, Eq, Hash, Serialize, Deserialize, Default)] | ||
| pub enum PostgisSpatialKind { |
Contributor
There was a problem hiding this comment.
That would make more sense as separate Geometry/Geography types like they are in PostGIS
| pub enum FieldType { | ||
| Supported(Identifier), | ||
| /// `Geometry(Point, 4326)` or `Geometry(LineString)` (SRID optional). | ||
| Geometry { |
Contributor
There was a problem hiding this comment.
Geometry being a third type alongsideSupported and Unsupported does not seem right to me.
I think this is because you try introduce new syntax for parametrized types that did not exist in PSL before. point Geometry @db.Geometry("Point", 4236) would be consistent with Prisma 7 syntax and would avoid you this kind of problem
| unsupported_type = { "Unsupported(" ~ string_literal ~ ")" } | ||
| base_type = { unsupported_type | identifier } // Called base type to not conflict with type rust keyword | ||
|
|
||
| geometry_subtype = { |
Contributor
There was a problem hiding this comment.
I don't think we need entire grammar for this - this will not produce the best syntax error. I'd rather keep it as identifier and validate it after Pest
| runs-on: ubuntu-latest | ||
| env: | ||
| RUSTFLAGS: "-Dwarnings" | ||
| RUSTFLAGS: '-Dwarnings' |
Contributor
There was a problem hiding this comment.
Please don't reformat unrelated files
| ScalarFieldType::CompositeType(_) => "object", | ||
| ScalarFieldType::Enum(_) => "enum", | ||
| ScalarFieldType::BuiltInScalar(_) => "scalar", | ||
| ScalarFieldType::BuiltInScalar(_) | ScalarFieldType::Geometry(_) => "scalar", |
Contributor
There was a problem hiding this comment.
ScalarFiledType and kind on dmmf should match. Either Geometry should be subtype of BuiltInScalar or kind should be geomatery
| bigdecimal.workspace = true | ||
| itertools.workspace = true | ||
| psl.workspace = true | ||
| psl = { workspace = true, features = ["all"] } |
Contributor
There was a problem hiding this comment.
This is not right — it will include PSL handlers for all databases in the bundle even if only one of them is used.
Merging this PR will not alter performance
Comparing 
Footnotes
|
Contributor
|
Also, the code does not compile on CI |
…ile preserving native-attribute parity with VarChar/Decimal
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
query-compiler/core/src/query_graph_builder/extractors/query_arguments.rs (1)
409-420:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winInconsistent coordinate handling: raw tuple vs
GeoCoord.The filter parsing in
scalar.rsusesGeoCoord::new(lon, lat)which validates finite coordinates, butextract_geometry_distance_frompasses raw(lon, lat)floats directly toOrderBy::geometry. This skips the finite-coordinate validation that filter parsing performs, allowingNaN/Infinityto reach SQL generation.🛠️ Suggested fix to validate coordinates
let lon = extract_float_from_pv(&point_list[0])?; let lat = extract_float_from_pv(&point_list[1])?; + // Validate coordinates are finite (extract_float_from_pv already checks this, + // but GeoCoord::new provides consistent validation with filter parsing) + use query_structure::GeoCoord; + let _ = GeoCoord::new(lon, lat).map_err(|e| QueryGraphBuilderError::InputError(e.to_string()))?; let sort_order = pv_to_sort_order(direction_value.try_into()?)?;Note:
extract_float_from_pvalready validates finite values (lines 439-444), so this is more about consistency. Ifextract_float_from_pvis trusted, this is optional.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@query-compiler/core/src/query_graph_builder/extractors/query_arguments.rs` around lines 409 - 420, The code constructs an OrderBy::geometry with a raw (lon, lat) tuple which bypasses GeoCoord validation; change this to build a GeoCoord via GeoCoord::new(lon, lat) (or equivalent constructor) and pass that GeoCoord to OrderBy::geometry to enforce finite coordinate checks and match scalar.rs behavior (refer to extract_float_from_pv, OrderBy::geometry, and GeoCoord::new to locate the spots to update).
♻️ Duplicate comments (2)
schema-engine/sql-introspection-tests/tests/postgres/postgis_geometry.rs (1)
56-63:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winStrengthen declaration-level assertions for geography fields.
These split
contains()checks can pass on unrelated lines. Assert the whole field declaration pattern so name, scalar type, and native type stay linked.Proposed test tightening
+use regex::Regex; ... - assert!( - schema.contains("location Geography?") && schema.contains("`@db.Geography`(Point, 4326)"), - "expected `location Geography? ... `@db.Geography`(Point, 4326)`, got:\n{schema}", - ); - assert!( - schema.contains("region Geography") && schema.contains("`@db.Geography`(Polygon, 4326)"), - "expected `region Geography ... `@db.Geography`(Polygon, 4326)`, got:\n{schema}", - ); + let location_re = Regex::new(r"(?m)^\s*location\s+Geography\?\s+@db\.Geography\(Point,\s*4326\)\s*$").unwrap(); + let region_re = Regex::new(r"(?m)^\s*region\s+Geography\s+@db\.Geography\(Polygon,\s*4326\)\s*$").unwrap(); + assert!(location_re.is_match(&schema), "expected location geography declaration, got:\n{schema}"); + assert!(region_re.is_match(&schema), "expected region geography declaration, got:\n{schema}");🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@schema-engine/sql-introspection-tests/tests/postgres/postgis_geometry.rs` around lines 56 - 63, Replace the split contains() checks with assertions that match the entire field declaration so the field name, scalar type/nullability and native type directive are contiguous; specifically update the two asserts that reference "location" and "region" to assert that schema contains the full declaration strings (e.g. a single contains() check per field that includes "location Geography? `@db.Geography`(Point, 4326)" and "region Geography `@db.Geography`(Polygon, 4326)" respectively) so the name, scalar type, and `@db.Geography`(...) stay linked.query-compiler/query-builders/sql-query-builder/src/filter/visitor.rs (1)
698-701:⚠️ Potential issue | 🟠 Major | ⚡ Quick winPolygon WKT rings are not guaranteed to be closed.
PostGIS requires polygon rings to be topologically closed (first vertex == last vertex). This function formats the coordinates as-is without checking or enforcing closure. If the input
positionsslice doesn't have matching first and last points, the generated WKT will fail with "geometry contains non-closed rings" at query execution time.Additionally, if any coordinate contains NaN or infinity (from upstream float parsing),
format!("{} {}", c.x, c.y)will produce literalNaN/inftokens in the WKT string, which are invalid and will cause opaque SQL errors.🐛 Proposed fix to ensure ring closure
fn format_polygon_ring_wkt(positions: &[GeoCoord]) -> String { + if positions.is_empty() { + return "POLYGON(())".to_string(); + } + // Ensure ring is closed per OGC/PostGIS requirement + let mut pts = positions.to_vec(); + if let (Some(first), Some(last)) = (pts.first(), pts.last()) { + if (first.x - last.x).abs() > f64::EPSILON || (first.y - last.y).abs() > f64::EPSILON { + pts.push(*first); + } + } - let parts: Vec<_> = positions.iter().map(|c| format!("{} {}", c.x, c.y)).collect(); + let parts: Vec<_> = pts.iter().map(|c| format!("{} {}", c.x, c.y)).collect(); format!("POLYGON(({}))", parts.join(", ")) }For NaN/inf validation, consider adding a check in the upstream extractor or here to reject non-finite coordinates early.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@query-compiler/query-builders/sql-query-builder/src/filter/visitor.rs` around lines 698 - 701, The function format_polygon_ring_wkt currently emits the ring "as-is", which can produce non-closed rings and embed NaN/inf tokens; update format_polygon_ring_wkt(positions: &[GeoCoord]) to (1) validate every coordinate is finite (reject or return an error if c.x or c.y is NaN/inf) before formatting, and (2) ensure the ring is closed by checking if the first and last GeoCoord are equal and, if not, append the first coordinate to the end of the sequence before joining; keep using the same "{} {}" value order when building the WKT string so output remains "POLYGON((...))" but only after these validations and closure enforcement.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@psl/parser-database/src/attributes/default.rs`:
- Around line 194-199: The error message in the validation branch that checks
scalar_type against ScalarType::Geometry | ScalarType::Geography is misleading
because it hardcodes "Geometry"; update the call to
ctx.push_attribute_validation_error (the branch that matches scalar_type, and
the similar branch later around the other check) so the message refers to both
"Geometry and Geography" (or dynamically includes the actual variant name)
instead of only "Geometry" to produce correct diagnostics for Geography fields.
In `@psl/psl-core/src/validate/validation_pipeline/validations/fields.rs`:
- Around line 345-347: The diagnostic message built in the helper (variable msg)
is too specific to "Geometry" even though the helper handles both Geometry and
Geography; update the message to use neutral wording like "PostGIS spatial type"
(e.g., "uses a PostGIS spatial type, which is only supported on PostgreSQL with
PostGIS") and ensure references to field_name, container, and container_name
remain intact so the message works for both Geometry and Geography paths.
In `@query-compiler/query-structure/src/filter/geojson.rs`:
- Around line 88-93: The coord helper in to_wkt currently uses format!("{} {}",
c.x, c.y) which can emit scientific notation and break PostGIS WKT parsing;
change coord (and any uses in to_wkt) to produce decimal strings that never use
exponential notation and preserve f64 precision (use a decimal serializer like
ryu or dtoa to format each GeoCoord component with ~17 significant digits into
non-exponential form) so the generated WKT is always PostGIS-compatible; update
coord/GeoCoord formatting to use that serializer for both c.x and c.y.
---
Outside diff comments:
In `@query-compiler/core/src/query_graph_builder/extractors/query_arguments.rs`:
- Around line 409-420: The code constructs an OrderBy::geometry with a raw (lon,
lat) tuple which bypasses GeoCoord validation; change this to build a GeoCoord
via GeoCoord::new(lon, lat) (or equivalent constructor) and pass that GeoCoord
to OrderBy::geometry to enforce finite coordinate checks and match scalar.rs
behavior (refer to extract_float_from_pv, OrderBy::geometry, and GeoCoord::new
to locate the spots to update).
---
Duplicate comments:
In `@query-compiler/query-builders/sql-query-builder/src/filter/visitor.rs`:
- Around line 698-701: The function format_polygon_ring_wkt currently emits the
ring "as-is", which can produce non-closed rings and embed NaN/inf tokens;
update format_polygon_ring_wkt(positions: &[GeoCoord]) to (1) validate every
coordinate is finite (reject or return an error if c.x or c.y is NaN/inf) before
formatting, and (2) ensure the ring is closed by checking if the first and last
GeoCoord are equal and, if not, append the first coordinate to the end of the
sequence before joining; keep using the same "{} {}" value order when building
the WKT string so output remains "POLYGON((...))" but only after these
validations and closure enforcement.
In `@schema-engine/sql-introspection-tests/tests/postgres/postgis_geometry.rs`:
- Around line 56-63: Replace the split contains() checks with assertions that
match the entire field declaration so the field name, scalar type/nullability
and native type directive are contiguous; specifically update the two asserts
that reference "location" and "region" to assert that schema contains the full
declaration strings (e.g. a single contains() check per field that includes
"location Geography? `@db.Geography`(Point, 4326)" and "region Geography
`@db.Geography`(Polygon, 4326)" respectively) so the name, scalar type, and
`@db.Geography`(...) stay linked.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 0244588f-98a5-4716-b370-e4cfa7a21dd5
⛔ Files ignored due to path filters (14)
query-compiler/query-compiler/tests/snapshots/queries__queries@geometry-combined-scalar-spatial.json.snapis excluded by!**/*.snapquery-compiler/query-compiler/tests/snapshots/queries__queries@geometry-count-with-filter.json.snapis excluded by!**/*.snapquery-compiler/query-compiler/tests/snapshots/queries__queries@geometry-delete-with-filter.json.snapis excluded by!**/*.snapquery-compiler/query-compiler/tests/snapshots/queries__queries@geometry-filter-and-orderby.json.snapis excluded by!**/*.snapquery-compiler/query-compiler/tests/snapshots/queries__queries@geometry-filter-and-scalar-filter.json.snapis excluded by!**/*.snapquery-compiler/query-compiler/tests/snapshots/queries__queries@geometry-filter-custom-srid.json.snapis excluded by!**/*.snapquery-compiler/query-compiler/tests/snapshots/queries__queries@geometry-filter-near.json.snapis excluded by!**/*.snapquery-compiler/query-compiler/tests/snapshots/queries__queries@geometry-filter-not-near.json.snapis excluded by!**/*.snapquery-compiler/query-compiler/tests/snapshots/queries__queries@geometry-filter-or-multiple.json.snapis excluded by!**/*.snapquery-compiler/query-compiler/tests/snapshots/queries__queries@geometry-filter-within.json.snapis excluded by!**/*.snapquery-compiler/query-compiler/tests/snapshots/queries__queries@geometry-multiple-orderby.json.snapis excluded by!**/*.snapquery-compiler/query-compiler/tests/snapshots/queries__queries@geometry-orderby-distance-asc.json.snapis excluded by!**/*.snapquery-compiler/query-compiler/tests/snapshots/queries__queries@geometry-orderby-distance-desc.json.snapis excluded by!**/*.snapquery-compiler/query-compiler/tests/snapshots/queries__queries@geometry-orderby-with-limit.json.snapis excluded by!**/*.snap
📒 Files selected for processing (50)
psl/parser-database/src/attributes.rspsl/parser-database/src/attributes/default.rspsl/parser-database/src/types.rspsl/psl-core/src/builtin_connectors/mod.rspsl/psl-core/src/builtin_connectors/postgres_datamodel_connector.rspsl/psl-core/src/builtin_connectors/postgres_datamodel_connector/native_types.rspsl/psl-core/src/datamodel_connector.rspsl/psl-core/src/validate/validation_pipeline/validations/fields.rspsl/psl/tests/validation/postgres/postgis_geometry_keyword_valid.prismapsl/psl/tests/validation/postgres/postgis_native_type_keyword_mismatch.prismapsl/schema-ast/src/ast/field.rspsl/schema-ast/src/parser/datamodel.pestpsl/schema-ast/src/parser/parse_types.rsquaint/src/ast/function.rsquaint/src/ast/function/postgis.rsquaint/src/visitor.rsquery-compiler/core-tests/tests/geometry-filters-graph-builds.rsquery-compiler/core-tests/tests/geometry_find_many_graph_builds.rsquery-compiler/core/src/query_document/parser.rsquery-compiler/core/src/query_graph_builder/extractors/filters/scalar.rsquery-compiler/core/src/query_graph_builder/extractors/query_arguments.rsquery-compiler/dmmf/src/ast_builders/datamodel_ast_builder.rsquery-compiler/dmmf/src/ast_builders/schema_ast_builder/type_renderer.rsquery-compiler/dmmf/src/tests/tests.rsquery-compiler/query-builders/sql-query-builder/src/cursor_condition.rsquery-compiler/query-builders/sql-query-builder/src/filter/visitor.rsquery-compiler/query-builders/sql-query-builder/src/ordering.rsquery-compiler/query-compiler/src/data_mapper.rsquery-compiler/query-compiler/tests/data/schema.prismaquery-compiler/query-structure/src/field/mod.rsquery-compiler/query-structure/src/field/scalar.rsquery-compiler/query-structure/src/filter/geojson.rsquery-compiler/query-structure/src/filter/geometry.rsquery-compiler/query-structure/src/filter/mod.rsquery-compiler/query-structure/src/prisma_value_ext.rsquery-compiler/schema/src/build/input_types/fields/field_filter_types.rsquery-compiler/schema/src/build/input_types/mod.rsquery-compiler/schema/src/build/input_types/objects/order_by_objects.rsquery-compiler/schema/src/build/output_types/field.rsquery-compiler/schema/src/output_types.rsquery-compiler/schema/src/query_schema.rsquery-engine/connectors/mongodb-query-connector/src/filter.rsschema-engine/connectors/sql-schema-connector/src/flavour/postgres/renderer.rsschema-engine/connectors/sql-schema-connector/src/flavour/postgres/schema_differ.rsschema-engine/connectors/sql-schema-connector/src/introspection/introspection_pair/scalar_field.rsschema-engine/connectors/sql-schema-connector/src/sql_doc_parser.rsschema-engine/connectors/sql-schema-connector/src/sql_schema_calculator.rsschema-engine/sql-introspection-tests/tests/postgres/postgis_geometry.rsschema-engine/sql-migration-tests/tests/migrations/postgres/postgis_geometry.rsschema-engine/sql-schema-describer/src/postgres.rs
💤 Files with no reviewable changes (2)
- query-compiler/core/src/query_document/parser.rs
- psl/schema-ast/src/parser/parse_types.rs
…d tolerate column-alignment padding in introspection checks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
6 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
See full features at prisma/prisma#29365.