Relax request body param handling & improve operationId formatting #109

dougborg · 2025-08-10T23:37:51Z

Summary
This PR introduces two focused improvements discovered while generating a client from an external spec (Karakeep):

Relaxed request body parameter handling
Previously, generation failed with an exception when a requestBody schema had schema.type == None (common for inline object schemas). Now generate_body_param tolerates these cases instead of raising:

$ref schemas: still serialized via .dict().
Arrays: if items are model-like, serialize each via .dict(); primitive arrays passed through.
Object-like, primitive, or missing-type schemas: passed through unchanged.

Added tests:

Missing-type object-like body schema
Array of primitives
Array of object-like schemas

Improved operationId formatting when paths contain parameters
Adds an underscore separator before each {param} placeholder so GET /lists/{listId} becomes get_lists_listId (instead of get_listslistId).

Added test: operationId path param underscore insertion.

No template changes required; logic is isolated to service_generator.py plus new tests in test_service_generator.py.
All tests pass locally.

See also
Follow-up enhancement proposed in Issue #108 (generate models for inline anonymous object schemas) which would further improve typing. This PR is a prerequisite quality-of-life fix but does not attempt inline model generation itself.

Let me know if you’d like separate PRs for the two concerns or additional edge-case tests (e.g., nested arrays, mixed item types).

Copilot

Pull Request Overview

This PR improves request body parameter handling and operationId formatting in the OpenAPI Python generator. The changes make the generator more robust when dealing with real-world API specifications that may have incomplete schema definitions.

Relaxed request body parameter handling to gracefully handle schemas without explicit types
Improved operationId formatting to include underscore separators before path parameters
Added comprehensive test coverage for the new functionality

Reviewed Changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.

File	Description
`src/openapi_python_generator/language_converters/python/service_generator.py`	Updated `generate_body_param()` to handle missing schema types and improved `generate_operation_id()` path parameter formatting
`tests/test_service_generator.py`	Added test cases for missing-type object schemas, array handling, and operationId path parameter formatting

src/openapi_python_generator/language_converters/python/service_generator.py

…arcoMuellner#98) ## Summary Refactored `type_converter` function to eliminate high cyclomatic complexity, code duplication, and fragile import aggregation. The function is now more maintainable, testable, and easier to extend with future OpenAPI 3.1 features. ## Changes ### Extracted Helper Functions - `_normalize_schema_type()`: Unified handling of DataType enum, strings, and lists - `_is_type()`: Simplified type checking across all schema type representations - `_handle_format_conversions()`: Consolidated UUID/datetime logic (eliminated 3x duplication) - `_wrap_optional()`: Replaced scattered pre_type/post_type pattern - `_collect_unique_imports()`: Safe import aggregation without blind indexing - `_convert_primitive_type()`: Handles string, int, float, bool, object, null types - `_convert_array_type()`: Isolated array type handling with items processing - `_convert_composite_schema()`: Unified allOf/oneOf/anyOf composition logic ### Main Function Improvements - Reduced from ~270 lines to 53 lines (80% reduction) - Cyclomatic complexity reduced from >16 to <10 - Removed noqa C901 suppression comment - Clear separation of concerns with early returns ## Benefits - **Maintainability**: Each responsibility isolated in well-documented helpers - **Safety**: No more direct indexing of potentially empty import lists - **Testability**: Individual functions can be tested independently - **Extensibility**: Adding new types/formats now straightforward - **Zero behavioral changes**: All 250 tests pass (55 model_generator specific) ## Metrics | Metric | Before | After | Improvement | |---------------------------|--------|-------|--------------------| | Function size | ~270 | 53 | 80% reduction | | Cyclomatic complexity | >16 | <10 | Below threshold | | Code duplication (UUID) | 3x | 1x | 100% eliminated | | Import aggregation safety | Fragile| Safe | No IndexError risk | Closes MarcoMuellner#98

Address review comment about array type handling. The previous implementation incorrectly used _wrap_optional on the entire List[...] after already handling optionality for reference items, which would produce incorrect types. ## Problem For arrays with Reference items: - Required array with items: `List[ItemType]` ✅ - Optional array with items: should be `Optional[List[Optional[ItemType]]]` ✅ (not `Optional[List[ItemType]]` ❌) ## Solution Build the Optional[List[...]] wrapper explicitly and pass the array's required status to _generate_property_from_reference's force_required parameter. This preserves the original behavior where reference items inherit the array's required status. For schema items (non-reference), always pass True since items are always required within the array. ## Testing - All 55 model_generator tests pass - All 250 project tests pass - Preserves original behavior for Optional[List[Optional[Type]]] pattern Co-authored-by: copilot-pull-request-reviewer

Replace lowercase 'list' type hints with uppercase 'List' from typing module to support Python 3.9. The lowercase generic types require either: - Python 3.9+ with 'from __future__ import annotations' - Python 3.10+ without future import Since the project supports Python 3.9, use the typing.List import instead. Fixes ty type checker errors in CI.

The _convert_primitive_type function already handles None internally (line 186), so the type hint should accept Optional[str] not just str. This fixes the ty type checker error: error[invalid-argument-type]: Argument to function _convert_primitive_type is incorrect Expected `str`, found `str | None` ✅ Verified with: poetry run ty check src/ ✅ All tests pass

Run black formatter to fix code style issues. ✅ Verified: poetry run ty check src/ ✅ Verified: poetry run black --check . ✅ Verified: All tests pass

Removed itertools import that was no longer needed after refactoring. Detected by ruff linter: F401 'itertools' imported but unused ✅ All CI checks pass locally: - ty check: passed - black: passed - ruff: passed - pytest: 250 passed, 94.78% coverage

…rivation

… param underscores

dougborg · 2025-10-26T17:47:01Z

Rebased onto PR #122

This PR has been rebased to work on top of #122 (the type_converter complexity reduction refactor).

Changes Made During Rebase

The rebase required resolving conflicts to make this PR's relaxed request body handling compatible with #122's multi-version type system:

1. Updated generate_body_param() function:

Combined multi-version isinstance checks from refactor: reduce complexity in model_generator.type_converter (fixes #98) #122 with defensive getattr approach from this PR
All type checks now handle both OpenAPI 3.0 and 3.1: isinstance(schema, (Schema, Schema30, Schema31))
Preserved this PR's relaxed handling for missing types, object-like schemas, and array processing

2. Merged test suites:

Both refactor: reduce complexity in model_generator.type_converter (fixes #98) #122's new tests and this PR's tests are now included
All 4 tests specific to this PR pass locally

3. CI Verification:
All checks pass locally:

✅ Type checking with ty
✅ Code formatting with black
✅ Linting with ruff
✅ Tests pass

This PR is now ready for review once #122 merges.

Address review feedback by making the error message more helpful when operationId and path_name are both missing. The error now includes: - Operation summary (if present) - HTTP operation (get, post, etc.) - Path name value This helps developers quickly identify which operation in their OpenAPI spec needs to be fixed. Also adds test coverage to verify the improved error message includes all expected details.

dougborg · 2025-10-26T17:54:43Z

Addressed Review Feedback

Thanks for the review! I've addressed the feedback:

✅ Improved Error Message (line 251)

Updated the RuntimeError in generate_operation_id() to include helpful debugging details:

raise RuntimeError(
    f"Missing operationId and path_name for operation. "
    f"Details: summary={getattr(operation, 'summary', None)!r}, "
    f"http_op={http_op!r}, path_name={path_name!r}"
)

This makes it much easier for developers to identify which operation in their OpenAPI spec needs fixing.

✅ Added Test Coverage

Added test_generate_operation_id_error_message_includes_details() to verify the error message includes all expected details (summary, http_op, path_name).

Note on MediaType Constructor

The first review comment about MediaType(schema=...) vs MediaType(media_type_schema=...) doesn't apply to the current code - that issue was already resolved during the rebase onto #122, which uses version-aware helper functions instead.

All CI checks pass locally:

✅ Type checking with ty
✅ Code formatting with black
✅ Linting with ruff
✅ New test passes

Copilot AI review requested due to automatic review settings August 10, 2025 23:37

Copilot AI reviewed Aug 10, 2025

View reviewed changes

src/openapi_python_generator/language_converters/python/service_generator.py Outdated Show resolved Hide resolved

src/openapi_python_generator/language_converters/python/service_generator.py Outdated Show resolved Hide resolved

Doug Borg added 9 commits October 26, 2025 10:05

style: apply black formatting

d7d4215

Run black formatter to fix code style issues. ✅ Verified: poetry run ty check src/ ✅ Verified: poetry run black --check . ✅ Verified: All tests pass

feat: relaxed request body param handling and improved operationId de…

8657cf1

…rivation

test: add coverage for relaxed body param handling & operationId path…

9450627

… param underscores

fixup: apply black formatting and remove unused import

9f0b5a4

dougborg force-pushed the fix/karakeep-body-param branch from f84193a to 9f0b5a4 Compare October 26, 2025 17:45

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Relax request body param handling & improve operationId formatting #109

Relax request body param handling & improve operationId formatting #109

dougborg commented Aug 10, 2025

Uh oh!

Copilot AI left a comment

Uh oh!

Uh oh!

Uh oh!

dougborg commented Oct 26, 2025

Uh oh!

dougborg commented Oct 26, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

Relax request body param handling & improve operationId formatting #109

Are you sure you want to change the base?

Relax request body param handling & improve operationId formatting #109

Conversation

dougborg commented Aug 10, 2025

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Reviewed Changes

Uh oh!

Uh oh!

Uh oh!

dougborg commented Oct 26, 2025

Rebased onto PR #122

Changes Made During Rebase

Uh oh!

dougborg commented Oct 26, 2025

Addressed Review Feedback

✅ Improved Error Message (line 251)

✅ Added Test Coverage

Note on MediaType Constructor

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant