Skip to content

fix: allow omitted query parameters in API client requests - #224

Draft
Kayvan-Zahiri wants to merge 1 commit into
lob:mainfrom
Kayvan-Zahiri:codex/optional-query-params
Draft

Kayvan-Zahiri wants to merge 1 commit into
lob:mainfrom
Kayvan-Zahiri:codex/optional-query-params

Conversation

@Kayvan-Zahiri

Copy link
Copy Markdown

Problem

Calling the public ApiClient.call_api('/addresses', 'GET') without query parameters raises TypeError: 'NoneType' object is not iterable before reaching the transport. The same occurs for all seven supported HTTP methods, despite query_params=None being the documented default. Generated resource methods that already supply an empty list are unaffected.

Change

Normalize absent query parameters to an empty list before applying the existing query serialization helper. Populated parameters continue through the same helper.

Add a public API-client regression with 28 subcases across all seven HTTP methods: omitted parameters, explicit None, empty lists, and populated lists. The transport is mocked; no account credentials or live API calls are needed. Before the fix, the 14 omitted/None cases error. Afterward all 28 pass and the expected outgoing URL or query fields are checked.

Validation

  • Documented unittest discovery: 248 tests passed on Python 3.12 (including the new test with its 28 subcases).
  • Wheel and source distribution build.
  • git diff --check passes.

This file is generated, so maintainers may also wish to carry the normalization into the generation workflow.

@qodo-code-review

Copy link
Copy Markdown

Code Review by Qodo

🐞 Bugs (0) 📘 Rule violations (0) 📎 Requirement gaps (0)

Grey Divider

Great, no issues found!

Qodo reviewed your code and found no material issues that require review

Grey Divider

Tip of the day
💡 Did you know, you can open the rule or relationship behind a finding in one click

More tips ↗ | Customize Qodo ↗ | Qodo docs ↗

Grey Divider

Qodo Logo

@qodo-code-review

Copy link
Copy Markdown

PR Summary by Qodo

Allow omitted query parameters in API client requests

🐞 Bug fix 🧪 Tests 🕐 10-20 Minutes

Grey Divider

AI Description

• Treat omitted or None query parameters as empty, preventing requests from failing before
 transport.
• Add mocked regression coverage for all seven HTTP methods and four parameter cases.
Diagram

graph TD
  Client["ApiClient.call_api"] --> REST["REST normalization"] --> Helper["Query helper"] --> Branch{"HTTP method"}
  Branch --> Fields["GET HEAD fields"] --> Transport["urllib3 transport"]
  Branch --> URL["Other URL query"] --> Transport
Loading
High-Level Assessment

The following are alternative approaches to this PR:

1. Fix the generation template
  • ➕ Preserves the behavior when the generated REST client is regenerated.
  • ➖ The generation template is not present in this repository, so it cannot replace the immediate fix here.

Recommendation: Keep the normalization in the shared REST request path: it fixes every supported method without changing populated-parameter handling. Carry the same fix into the generation workflow when that template is available.

Files changed (2) +43 / -1

Bug fix (1) +1 / -1
rest.pyDefault absent query parameters before serialization +1/-1

Default absent query parameters before serialization

• Passes an empty list to the existing query helper when parameters are omitted or None. This prevents a pre-transport TypeError across all supported HTTP methods while retaining the existing path for populated parameters.

lob_python/rest.py

Tests (1) +42 / -0
test_optional_query_params.pyCover optional query parameters through the public client +42/-0

Cover optional query parameters through the public client

• Adds 28 mocked subcases spanning seven HTTP methods and omitted, None, empty, and populated parameters. Assertions check the outgoing method and URL, plus fields for GET and HEAD, without making live API calls.

test/Unit/test_optional_query_params.py

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

Development

Successfully merging this pull request may close these issues.

1 participant