Skip to content

Deduplicate stats queries and return proper HTTP errors - #187

Merged
skearnes merged 1 commit into
mainfrom
cleanup/backend-consistency
Jun 21, 2026
Merged

Deduplicate stats queries and return proper HTTP errors#187
skearnes merged 1 commit into
mainfrom
cleanup/backend-consistency

Conversation

@skearnes

@skearnes skearnes commented Jun 14, 2026

Copy link
Copy Markdown
Member

Backend API consistency cleanup (Tier 2). No change to successful responses.

Changes

  • Deduplicate the dataset stats queries. fetch_dataset_most_used_smiles_for_inputs and ..._for_products were identical apart from three table/column names. They now delegate to one private helper, with the differing identifiers composed via psycopg.sql.Identifier (schema-qualified, injection-safe) instead of duplicated query text. Public function signatures are unchanged.
  • Return meaningful HTTP errors instead of bare ValueErrors (which surface as opaque 500s):
    • /molfile with an unparseable SMILES → 400
    • /reaction_summary for an unknown reaction_id404 (and 500 reserved for the genuinely-impossible "multiple matches" case)
  • Regression tests for both new error paths.

Deliberately deferred

The frontend fetch-error consolidation (also in the Tier 2 plan) touches ChartView.tsx and MainDatasetView.tsx, which are actively being edited on another branch — folding it in now would create merge conflicts, so it'll come separately.

Testing

  • ruff check / ruff format --check: clean
  • ty check ord_interface: clean
  • pytest ord_interface/api (with conda postgres): 54 passing, including the 2 new error-case tests and the existing stats tests that cover the refactored helper

🤖 Generated with Claude Code

Greptile Summary

This PR cleans up backend API consistency by deduplicating two identical stats SQL queries into a shared private helper and replacing bare ValueError raises with proper FastAPI HTTPException responses.

  • _fetch_dataset_most_used_smiles in queries.py composes schema-qualified table and column identifiers via psycopg.sql.Identifier, correctly mirroring both original queries; the {compound}.{foreign_key} three-part reference is valid PostgreSQL.
  • /molfile with an unparseable SMILES now returns HTTP 400; /reaction_summary with an unknown ID returns 404, and the previously-combined guard for the impossible multi-match case is split into its own 500.
  • Regression tests cover both new error paths.

Confidence Score: 5/5

Safe to merge — all changes are narrowly scoped, public function signatures are unchanged, and the refactored SQL has been validated against the two original queries it replaces.

The SQL refactoring correctly reconstructs both original queries using psycopg.sql.Identifier for all dynamic identifiers, the three-part schema.table.column reference is valid PostgreSQL, and the error-handling changes are straightforward substitutions of ValueError with typed HTTPExceptions. Regression tests confirm the new error paths.

No files require special attention.

Important Files Changed

Filename Overview
ord_interface/api/queries.py Deduplicates two identical SQL queries into a private helper using psycopg sql.Identifier for injection-safe table/column composition; logic is correct and mirrors original queries exactly.
ord_interface/api/search.py Replaces bare ValueError with HTTPException(400) for invalid SMILES input; straightforward improvement consistent with existing 404 pattern in the same file.
ord_interface/api/view.py Splits the original combined zero-or-many guard into separate 404 (not found) and 500 (impossible multiple-match) cases with descriptive detail messages.
ord_interface/api/search_test.py Adds regression test for the new 400 path in /molfile with an invalid SMILES string.
ord_interface/api/view_test.py Adds regression test for the new 404 path in /reaction_summary with an unknown reaction ID.

Reviews (1): Last reviewed commit: "Deduplicate stats queries and return pro..." | Re-trigger Greptile

Backend consistency cleanup (Tier 2), no change to successful responses:

- Collapse the near-identical `fetch_dataset_most_used_smiles_for_inputs`
  and `..._for_products` into one private helper, with the input/product
  table names passed as safely-composed `psycopg.sql.Identifier`s rather
  than duplicated query text.
- Replace bare `ValueError`s in API handlers with `HTTPException`s so
  clients get meaningful status codes instead of 500s:
  - `/molfile` with an unparseable SMILES -> 400
  - `/reaction_summary` for an unknown reaction_id -> 404 (and 500 only
    for the genuinely-impossible "multiple matches" case)
- Add regression tests for both error paths.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
@codecov-commenter

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 95.00000% with 1 line in your changes missing coverage. Please review.
✅ Project coverage is 83.08%. Comparing base (52ff89d) to head (f045828).

Files with missing lines Patch % Lines
ord_interface/api/view.py 80.00% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #187      +/-   ##
==========================================
+ Coverage   82.88%   83.08%   +0.20%     
==========================================
  Files          16       16              
  Lines        1075     1082       +7     
==========================================
+ Hits          891      899       +8     
+ Misses        184      183       -1     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@skearnes
skearnes requested a review from bdeadman June 14, 2026 01:34
@skearnes
skearnes merged commit 74183e1 into main Jun 21, 2026
16 checks passed
@skearnes
skearnes deleted the cleanup/backend-consistency branch June 21, 2026 01:30
skearnes added a commit that referenced this pull request Jun 21, 2026
#187 introduced two HTTPException calls in view.py (and one test line in
view_test.py) wider than 88 chars after #188 lowered the line length.
Because it merged after #188 without rebasing, the `ruff format --check`
step in the check_python CI job has been failing on main ever since.

Pure `ruff format` reflow; no behavior change.

Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
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.

3 participants