Skip to content

Share server submit option parsing#20

Closed
jolovicdev wants to merge 2 commits into
masterfrom
test/server-submit-options
Closed

Share server submit option parsing#20
jolovicdev wants to merge 2 commits into
masterfrom
test/server-submit-options

Conversation

@jolovicdev
Copy link
Copy Markdown
Owner

Summary

  • move repeated /submit task option extraction into a shared helper
  • use the same parsing path for sync and async HTTP submit handlers
  • keep the handler bodies focused on decode, execute, and response rendering

Testing

  • .venv/bin/ruff check src/cashet/_client_base.py src/cashet/server.py
  • .venv/bin/pytest tests/test_server.py -q
  • .venv/bin/pytest tests/ -q

Copy link
Copy Markdown

@ds-review ds-review Bot left a comment

Choose a reason for hiding this comment

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

PR Review

PR: Share server submit option parsing

Important

Verdict: Request changes - 1 actionable finding, highest severity P1.

Findings (1)

P1 High cache option overrides explicit false to true

src/cashet/_client_base.py:91

The shared helper uses data.get("cache") or True, which treats a client-supplied false as falsy and replaces it with True. The original inline code used data.get("cache", True), which correctly preserved an explicit False. This silently re-enables caching when the client explicitly requested no caching, potentially returning stale results.

How To Recheck

Reply @ds-review recheck under the relevant inline finding after pushing a fix.

def resolve_submit_options(
data: Mapping[str, Any],
) -> tuple[bool, dict[str, str], int, bool, Any, Any]:
cache = data.get("cache") or True
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P1 High cache option overrides explicit false to true

The shared helper uses data.get("cache") or True, which treats a client-supplied false as falsy and replaces it with True. The original inline code used data.get("cache", True), which correctly preserved an explicit False. This silently re-enables caching when the client explicitly requested no caching, potentially returning stale results.

Suggested change
cache = data.get("cache") or True
cache = data.get("cache", True)

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

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

Good job!

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Thank you! I'm glad the review helped. Once you apply the fix to use data.get("cache", True), the caching behavior will correctly respect an explicit False.

@jolovicdev
Copy link
Copy Markdown
Owner Author

Closing this test PR; opening a clean follow-up.

@jolovicdev jolovicdev closed this May 9, 2026
@jolovicdev jolovicdev deleted the test/server-submit-options branch May 10, 2026 23:26
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