Skip to content

admin_ops — GeoPackage import and map-module composition over HTTP - #713

Open
nofurtherinformation wants to merge 2 commits into
cutover/pr3-comment-scopingfrom
cutover/pr4-admin-ops
Open

admin_ops — GeoPackage import and map-module composition over HTTP#713
nofurtherinformation wants to merge 2 commits into
cutover/pr3-comment-scopingfrom
cutover/pr4-admin-ops

Conversation

@nofurtherinformation

Copy link
Copy Markdown
Collaborator

Description

Stacked on cutover/pr3-comment-scoping. Inert on deploy: nothing calls these until the CMS exists.

  • Two scope-gated endpoints so an admin UI can run data operations that were CLI-only: POST /api/admin/gerrydb/import and POST /api/admin/districtr-map/compose.
  • Compose chains the same steps as the CLI commands (shatterable view when a child layer is given, create map, extent, parent-child edges, group membership, overlay links) as a background task, after validating cheap preconditions in-request so callers get 404/409 instead of a silent background failure.
  • Modules are always composed hidden (visible=false) — flip them on after review.

Reviewers

  • Primary:
  • Secondary:

Checklist

  • Added/Updated related documentation (if applicable). — endpoint docstrings describe the CLI equivalence.
  • Added/Updated related unit tests (if applicable). — 24 tests over the scope gate, slug/SQL-identifier validation, step ordering, and overlay attachment. 410 passed locally.

Screenshots (if applicable):

N/A — no UI.

Review required

  • A non-UUID string in overlay_ids currently surfaces as a 500 from the existence check rather than a 422 (no injection risk — queries are parameterized and layer/slug fields are regex-validated).
  • Confirm the scope choice (create:districtr_maps) is the boundary you want for composing modules.

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 88c0751a4b

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment thread backend/app/admin_ops/main.py
Comment thread backend/app/admin_ops/main.py Outdated
Comment thread backend/app/admin_ops/main.py
Comment thread backend/app/admin_ops/main.py

@peterrrock2 peterrrock2 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Responses to reviewer required section:

  • A non-UUID string in overlay_ids currently surfaces as a 500 from the existence check rather than a 422 (no injection risk — queries are parameterized and layer/slug fields are regex-validated).

    • I would prefer to validate overlay_ids as UUIDs at the request boundary:
      • malformed UUID -> 422
      • valid but nonexistent UUID -> 404
  • Confirm the scope choice (create:districtr_maps) is the boundary you want for composing modules.

    • I think this is the appropriate boundary, assuming this scope continues to be issued only to CMS admins. It makes sense that an admin who can import the source data and create map modules can also configure other stuff about the module.

Comment thread backend/app/admin_ops/main.py
Comment thread backend/app/admin_ops/main.py Outdated
Comment thread backend/app/admin_ops/main.py
Comment thread backend/app/admin_ops/main.py Outdated
Comment thread backend/app/admin_ops/main.py
Comment thread backend/app/admin_ops/main.py Outdated
Comment thread backend/app/admin_ops/main.py Outdated
nofurtherinformation and others added 2 commits August 31, 2026 21:28
Two scope-gated endpoints so an admin UI can run data operations that
were CLI-only: POST /api/admin/gerrydb/import and
/api/admin/districtr-map/compose. Compose chains the same steps as the
CLI commands (shatterable view when a child layer is given, create map,
extent, parent-child edges, group membership, overlay links) as a
background task, after validating cheap preconditions in-request so
callers get 404/409 instead of a silent background failure.

Modules are always composed hidden (visible=false) — flip them on after
review. Nothing calls these until the CMS exists, so this is inert on
deploy. 24 tests cover the scope gate, slug/SQL-identifier validation,
step ordering, and the overlay attachment.

Reviewer note: a non-UUID string in overlay_ids currently surfaces as a
500 from the existence check rather than a 422.
…alidation

Review feedback on #713:
- run_gerrydb_import commits its owned session: import_gerrydb_view's
  final gerrydbtable upsert lands after its last internal commit, so the
  registration was rolled back on close and compose 404'd on the layer.
- create_parent_child_edges is now reached through
  create_or_copy_parent_child_edges, extracted from load_data's private
  wrapper: SET LOCAL statement_timeout='0' (large states exceed 120s)
  and copy a compatible map's edge partition instead of recomputing.
- Layer identifiers validate via assert_safe_ident (leading digits are
  not valid unquoted PostgreSQL identifiers).
- Shatterable slugs are rejected when slug + '_shatterable' would pass
  PostgreSQL's 63-char identifier limit and silently truncate.
- child_layer must differ from parent_layer.
- overlay_ids are typed UUIDs (malformed input 422s instead of a
  DataError 500).
- The compose-time 'visible' flag is gone: modules always compose
  hidden, publishing is a deliberate second step after review.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
@nofurtherinformation

Copy link
Copy Markdown
Collaborator Author

Notes:

  • Rebased onto origin/dev through the restacked stack; force-pushed. One new commit addresses all nine comments: 2963d20.

Responses to review comments

  • P1 — commit the GerryDB registration (codex, confirmed by @peterrrock2): fixed. run_gerrydb_import now commits its owned session after import_gerrydb_view returns — the final gerrydbtable upsert lands after the helper's last internal commit, so it was rolling back on close and compose then 404'd on the layer. (Peter was right that the CLI's session_scope used to cover this.)
  • P1 — statement timeout / copy compatible edges (codex + @peterrrock2): fixed with Peter's suggested shape. load_data's private wrapper is extracted as create_or_copy_parent_child_edges(session, districtr_map_uuid, force)SET LOCAL statement_timeout='0', and it copies a compatible map's edge partition (same parent/child layer pair) instead of re-running the spatial join. The compose path calls it.
  • P2 — 63-byte identifier limit: fixed — a model validator rejects shatterable slugs whose derived <slug>_shatterable view name would exceed PostgreSQL's limit (422 with an explanatory message); non-shatterable maps unaffected. Tested.
  • P2 — overlay IDs as UUIDs: fixed — overlay_ids: list[UUID] on the request model, so malformed input 422s instead of surfacing as a DataError 500. (Also covers the PP/O "validate the UUID shape" thread.) Tested.
  • PP — child_layer != parent_layer (@peterrrock2): added, with a test.
  • H — leading-digit identifiers / reuse assert_safe_ident (@peterrrock2): fixed — both identifier validators call assert_safe_ident from app.utils now; the local word-character regex is gone. Parametrized tests.
  • PP — validate layers exist (@peterrrock2): already in place — the endpoint 404s when parent/child layer isn't registered in gerrydbtable (and for unknown group/overlays) before scheduling anything.
  • I — the visible parameter bypassing the hidden-until-review stage (@peterrrock2): agreed, removed entirely. Compose always creates modules hidden; publishing is a deliberate second step on the DistrictrMap edit page. The CMS never sent visible, so only the contract slimmed (its compose payload and tests updated in pr5).

@nofurtherinformation

Copy link
Copy Markdown
Collaborator Author

A lot of this PR is superseded by dev work relating to the graph updates. I think the actual endpoints and auth matters look good here, which is what matters most from this PR in the stack

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.

2 participants