Skip to content

fix(1011): default molimg height/width when not supplied - #1012

Open
claude-im wants to merge 1 commit into
stagingfrom
fragalysis-backend_abc-1011-molimg-params
Open

fix(1011): default molimg height/width when not supplied#1012
claude-im wants to merge 1 commit into
stagingfrom
fragalysis-backend_abc-1011-molimg-params

Conversation

@claude-im

Copy link
Copy Markdown
Collaborator

Fixes #1011.

Problem

/api/molimg/ returns 500 in production. MolImageSerializer.get_mol_image sized the image from the request's height/width query parameters, but guarded that lookup with a bare if params::

params = request.query_params
if params:                                  # true if ANY parameter is present
    return draw_mol(
        obj.smiles,
        height=int(float(params["height"])),  # KeyError when height wasn't one of them
        width=int(float(params["width"])),
    )
else:
    return draw_mol(obj.smiles, height=125, width=125)

MolImgFilter accepts target, cmpd, smiles and site_observation_groups, so /api/molimg/?target=1 — or plain ?page=2 pagination — makes params truthy with no size in it, and the lookup raises MultiValueDictKeyError: 'height'. The all-or-nothing guard only ever worked for requests that supplied both dimensions or no parameters at all.

Fix

Each dimension is read through a small module-level _int_param() helper and defaults independently, so filtering and pagination parameters no longer affect sizing:

params = self.context["request"].query_params
return draw_mol(
    obj.smiles,
    height=_int_param(params, "height", self.DEFAULT_HEIGHT),
    width=_int_param(params, "width", self.DEFAULT_WIDTH),
)

The 125×125 defaults become named class attributes rather than repeated literals. Behaviour is otherwise unchanged: values are still read via float() so decimal strings are accepted and truncated.

No API contract change — responses that worked before are byte-identical.

Out of scope (noted, not fixed)

A supplied but unparseable size (?height=abc) still raises ValueError → 500. That is pre-existing behaviour and a different defect; properly it should be a 400 from request validation. Happy to file it separately if you'd like it dealt with.

Testing

New regression test viewer/tests/test_molimg_serialization.py, written first (TDD) — it reproduced the exact production MultiValueDictKeyError before the fix. It covers:

  • a filtering parameter with no size → defaults (the reported 500);
  • no parameters at all → defaults;
  • explicit height/width → honoured;
  • only one dimension supplied → the other defaults;
  • float-valued sizes → truncated.

Full suite: 350 passed, 1 skipped. pre-commit (isort/black/mypy/pylint) clean.

🤖 Generated with Claude Code

MolImageSerializer.get_mol_image sized the image from the request's
height/width query parameters but guarded that lookup with a bare
`if params:`, which is true whenever *any* query parameter is present.
A request that filters or paginates without asking for a size - e.g.
/api/molimg/?target=1 - hit the parameter lookup with no height, raising
MultiValueDictKeyError and surfacing as a 500 on /api/molimg/.

Read each dimension through a small _int_param() helper so it falls back
to its default independently, and promote the 125x125 defaults to named
class attributes. A supplied but unparseable value still raises, as
before - that is a separate concern.

Add a regression test that reproduces the production error (a filtering
parameter with no size) and covers the no-parameter, explicit-size,
one-dimension-only and float-valued cases.

Co-Authored-By: Claude Opus 5 <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.

MultiValueDictKeyError : viewer/serializers.py", line 439, (get_mol_image)

2 participants