Skip to content

Fix four defects in the table-create and query paths - #2403

Open
jh-RLI wants to merge 5 commits into
developfrom
fix-api-table-create-and-query-defects
Open

Fix four defects in the table-create and query paths#2403
jh-RLI wants to merge 5 commits into
developfrom
fix-api-table-create-and-query-defects

Conversation

@jh-RLI

@jh-RLI jh-RLI commented Aug 5, 2026

Copy link
Copy Markdown
Contributor

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

  1. A field alias was parsed and discarded. {"type": "column", "column": "name", "as": "who"} in an advanced search came back named name; a
    labelled function came back as upper_1. col.label(...) was called without
    binding the result. Any client keying results by column name broke silently.
  2. Unsupported constraint types were dropped without a word. The create path
    branched on PRIMARY KEY and UNIQUE and fell off the end of the chain for
    everything else, so a FOREIGN KEY or CHECK in the payload vanished and
    the request returned 201.
  3. Two paths swallowed the database's reason. A failed create answered
    Could not create table <name>; a failed spatial query answered Invalid 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 the
    report stayed open: the message gave nobody anything to act on.
  4. The geometry type modifier was parsed by accident. geometry(Point,4326)
    put the whole modifier into GeoAlchemy2's geometry_type and never set
    srid, rendering geometry(POINT,4326,-1). Postgres reads the first two
    values 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

  • The labelled expression is bound. An invalid alias now fails with the existing
    identifier error instead of falling back to a generated name.
  • The create path rejects constraint types it cannot apply, naming the supported
    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.
  • Both failure paths 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 XX000, which also covers real server
    faults, 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.
  • The geometry modifier is split into subtype and SRID. A non-numeric SRID is
    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_req helper — no new test seam, and
no 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_index default now fails a
test 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 oekg SPARQL
test needs the fuseki hostname from the container network).

Deliberately not in this PR

  • Foreign key support at creation time. It needs decisions this PR has no basis
    to make — cross-topic references, what permission the referenced table
    requires (the alter path currently checks none, with a FIXME on that line),
    and what a foreign key means for rows sitting in a Journal Table before Apply.
  • Check constraint support, at creation or afterwards.
  • Indexes on non-geometry columns. Still a maintainer action on request.
  • Type checking the Advanced Query API's function arguments. The API forwards
    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

Reviewer

  • 🐙 Follow the
    Reviewer Guidelines
  • 🐙 Provided feedback and show sufficient appreciation for the work done

jh-RLI and others added 5 commits August 5, 2026 00:22
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>
@jh-RLI jh-RLI self-assigned this Aug 5, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant