Skip to content

Manifest rows missing SeriesNumber/licence/DOI, and /v3/sql has no parameter binding #63

Closed

Description

@pieper

Written and posted by Claude (Claude Code), working at the direction of @pieper. The measurements, the patch and this text are mine; please hold him responsible for the direction, and me for any errors.

Opened at @fedorov's suggestion in pieper/SlicerRAD-IDC#5.

Context

We are moving SlicerRAD-IDC — a browser viewer that opens IDC studies from OHIF-style deep links — off a self-hosted copy of the index and onto this API. It works well: CORS is open, and a single POST /v3/sql returns a whole study's series list, joined to ct/mr/pt_index and volume_geometry_index, in 1.4–5 KB and ~200 ms measured from https://pieper.github.io. Thank you for building it.

Two small things would let us do that without raw SQL, or make raw SQL safer if we keep it.

1. /v3/cohort/manifest rows are three fields short of a viewer's needs

The structured endpoint is the one we would rather depend on: a statically deployed browser bundle cannot be hotfixed, so coupling it to internal table and column names during a beta is a risk we would rather not take, and filters.terms.StudyInstanceUID expresses our query exactly.

Its series rows carry crdc_series_uuid, aws_bucket and series_aws_url — everything needed to fetch — but not:

  • SeriesNumber — how a DICOM-aware viewer orders a study's series,
  • license_short_name and source_DOI — what has to be displayed alongside the data.

So we fall back to /v3/sql purely to get three columns the structured endpoint could return from the same index scan.

2. /v3/sql has no parameter binding

We interpolate a StudyInstanceUID from our own page URL into the statement. We guard it with ^[0-9.]{1,64}$, which is adequate — but it is a guard every client has to remember, on every path, forever. The services in this repo already bind their own values (compile_filters → backend.query(..., params=...)); exposing the same mechanism on the user-SQL path removes the obligation from callers. It does not change the sandbox, which is what makes arbitrary SQL safe to run in the first place.

Patch

A PR implementing both is at #64 — two independent commits, so either can be dropped. pytest (112 passed), ruff check, ruff format --check and bandit all pass locally, and both changes are additive (MINOR under your versioning, no contract break), with CHANGELOG.md entries under Unreleased.

Entirely your call whether either is worth taking — they are small enough that we can live without them, and /v3/sql already does everything we need.

Activity

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Type

    No type

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions