Fix four defects in the table-create and query paths - #2403
Open
jh-RLI wants to merge 5 commits into
Open
Conversation
The field list of a select parsed the alias and called label() without keeping the returned expression, so the alias was discarded: a labelled function came back as "upper_1" and a labelled column as its own name. Any client keying results by name broke, and nothing in the response said why. Bind the labelled expression. An invalid alias now fails with the existing identifier error instead of silently falling back to a generated name. The explicit label expression type is unchanged and stays the documented form; the field-level alias is now an equivalent shorthand for it. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The constraint loop recognised PRIMARY KEY and UNIQUE and fell off the end of its branch chain for everything else. A FOREIGN KEY or CHECK in the payload was discarded, the create returned 201, and nothing told the caller that half their request had been dropped. Add a terminal branch. FOREIGN KEY names the supported route — create first, then add the constraint through the table endpoint, which the alter path already implements. CHECK reports as unsupported, matching what that same alter path already answers. Anything else names the value that was sent and lists what is supported. The rejection happens while parsing the payload, before a Main Table exists, so the existing all-or-nothing behaviour covers cleanup. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Two paths dropped the cause of a failure and left the caller guessing. A failed table create answered "Could not create table <name>" and nothing else, so an unusable column definition - numeric(1001), say - was undebuggable without reproducing the request against a database. A spatial query whose reference system arrived as a JSON string answered "Invalid request". A string makes postgres resolve ST_Transform's (geometry, to_proj text) overload, which then tries to read "4326" as a projection definition. PostGIS said exactly that; the API swallowed it. This is the open report in openego/ding0#405, and the reason it stayed open is that the message gave nobody anything to act on. Both now consult one policy, reflectable_cause in api.error, which answers whether a database cause may be disclosed. The query path's inline pgcode allowlist and its single hard-coded PostGIS message move there unchanged; the projection-parse message is added. PostGIS raises everything under the generic internal-error code, which also covers real server faults, so recognition has to be by message text - deliberately narrow, not "all internal errors". Behaviour is otherwise unchanged: causes outside the policy still report the generic message, and the full exception still goes to the log. Note the reflected PostGIS text now comes from the exception's string form on both paths, as the query path already did for the unknown-SRID case, so that message is unchanged. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
geometry(Point,4326) put the whole parenthesised modifier into GeoAlchemy2's geometry_type and never set srid, so the rendered type carried three values - geometry(POINT,4326,-1). Postgres reads the first two and ignores the third, which is why the declared SRID landed correctly all along: by accident, not by intent. Split subtype from SRID and pass them separately, so the rendered type carries two. Verified against PostGIS 3.4 to register the same subtype and SRID as before. A non-numeric SRID now fails with a named error instead of reaching postgres as part of the type. The point of this commit is the accompanying test. The automatic GiST index on a geometry column comes from GeoAlchemy2's spatial_index default, not from any code of ours, and nothing asserted it - so a dependency upgrade could have quietly stopped indexing every new geo table. Four of the five new tests pass on the unfixed parser, which is the finding: the contract was undefended, not broken. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
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
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.
Summary of the discussion
Four defects where the API accepted a request, did something other than what was
asked, and reported success — plus one test-only change that defends a contract
nobody had asserted. Found while answering a user support question about
bounding-box queries; one of them is the standing report in
[openego/ding0#405](openego/ding0#405).
What was wrong
{"type": "column", "column": "name", "as": "who"}in an advanced search came back namedname; alabelled function came back as
upper_1.col.label(...)was called withoutbinding the result. Any client keying results by column name broke silently.
branched on
PRIMARY KEYandUNIQUEand fell off the end of the chain foreverything else, so a
FOREIGN KEYorCHECKin the payload vanished andthe request returned
201.Could not create table <name>; a failed spatial query answeredInvalid request. In the ding0 case PostGIS had said exactly what was wrong —could not parse proj string '4326'— and the API dropped it. That is why thereport stayed open: the message gave nobody anything to act on.
geometry(Point,4326)put the whole modifier into GeoAlchemy2's
geometry_typeand never setsrid, renderinggeometry(POINT,4326,-1). Postgres reads the first twovalues and ignores the third, so the declared SRID landed correctly — by
accident. Nothing asserted either that or the automatic GiST index, which
comes from a library default rather than from our code.
What changed
identifier error instead of falling back to a generated name.
route: foreign keys after creation through the table endpoint (which the alter
path already implements), check constraints unsupported (which that same alter
path already answers). Rejection happens while parsing, before any Main Table
exists, so the existing all-or-nothing behaviour covers cleanup.
reflectable_causeinapi.error,which answers whether a database cause may be disclosed. The query path's
inline pgcode allowlist and its single hard-coded PostGIS message move there
unchanged; the projection-parse message is added. PostGIS raises everything
under the generic internal-error code
XX000, which also covers real serverfaults, so recognition has to be by message text — deliberately narrow, not
"all internal errors". Causes outside the policy still report the generic
message, and the full exception still goes to the log either way.
rejected with a named error instead of reaching postgres as part of the type.
Testing
19 new tests across four regression modules, all driven through the HTTP
endpoints via the existing
APITestCase.api_reqhelper — no new test seam, andno assertions on generated DDL. Every test was verified red before green.
Notable: four of the five geometry tests pass on the unfixed parser. That is the
finding — the contract was undefended, not broken. Their value is that a
dependency upgrade changing GeoAlchemy2's
spatial_indexdefault now fails atest instead of silently stopping the indexing of every new geo table.
The geometry tests need PostGIS and will fail against a plain postgres; there is
a note to that effect in the module.
Full suite: 404 tests, one pre-existing environment failure (the
oekgSPARQLtest needs the
fusekihostname from the container network).Deliberately not in this PR
to make — cross-topic references, what permission the referenced table
requires (the alter path currently checks none, with a
FIXMEon that line),and what a foreign key means for rows sitting in a Journal Table before Apply.
calls without knowing their signatures; coercing numeric-looking strings is
rejected outright, since it would change the meaning of requests that pass
numeric strings on purpose.
Workflow checklist
Automation
Closes #
PR-Assignee
CONTRIBUTING.md
CHANGELOG.md
mkdocs
Reviewer
Reviewer Guidelines