Skip to content

Commit 4238443

Browse files
author
Doug Borg
committed
Address code review feedback on design document
Changes: 1. Replace bare except with specific exception types (AttributeError, UnicodeDecodeError) 2. Narrow exception handling for error model parsing to specific types (ValidationError, ValueError, TypeError, JSONDecodeError) 3. Improve status code classification logic with better comments and defensive fallback to APIError for unexpected status codes 4. Add documentation for has_wildcard_success flag to clarify when it should be set (when OpenAPI spec uses '2XX' wildcard pattern) 5. Add inline comments explaining wildcard success handling in templates Addresses Copilot review comments on PR #124
1 parent a00597e commit 4238443

File tree

1 file changed

+20
-4
lines changed

1 file changed

+20
-4
lines changed

DESIGN_ISSUE_100.md

Lines changed: 20 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -828,7 +828,8 @@ def _create_error_from_response(
828828
# Get full response body
829829
try:
830830
body_text = response.text
831-
except:
831+
except (AttributeError, UnicodeDecodeError):
832+
# Response object doesn't have .text or body contains invalid UTF-8
832833
body_text = None
833834

834835
# Determine which error model to use
@@ -853,8 +854,8 @@ def _create_error_from_response(
853854
try:
854855
model_class = _ERROR_MODELS[model_name]
855856
error_model = model_class.model_validate_json(body_text)
856-
except Exception:
857-
# Parsing failed - invalid JSON, schema mismatch, etc.
857+
except (ValidationError, ValueError, TypeError, JSONDecodeError):
858+
# Parsing failed - invalid JSON, schema mismatch, wrong type, etc.
858859
# Keep error_model as None, use raw body instead
859860
pass
860861

@@ -868,7 +869,15 @@ def _create_error_from_response(
868869
message += f": {str(error_model)}"
869870

870871
# Choose exception class based on status code
871-
exception_class = ServerError if status_code >= 500 else ClientError
872+
# Defensive: status_code should always be in 4xx or 5xx range at this point
873+
# due to earlier validation, but we handle edge cases gracefully
874+
if status_code >= 500:
875+
exception_class = ServerError
876+
elif status_code >= 400:
877+
exception_class = ClientError
878+
else:
879+
# Fallback for unexpected status codes (e.g., 3xx, 1xx)
880+
exception_class = APIError
872881

873882
return exception_class(
874883
status_code=status_code,
@@ -1015,12 +1024,17 @@ class OpReturnType(BaseModel):
10151024
type: Optional[TypeConversion] = None
10161025
status_code: int # Primary/first success code
10171026
success_codes: List[int] = [] # All success codes
1027+
has_wildcard_success: bool = False # True if spec uses '2XX' wildcard pattern
10181028
response_models: Dict[int, str] = {} # {200: 'User', 201: 'CreatedUser'}
10191029
error_models: Dict[str, str] = {} # {'404': 'ErrorResponse'}
10201030
complex_type: bool = False
10211031
list_type: Optional[str] = None
10221032
```
10231033

1034+
**Field Descriptions**:
1035+
1036+
- `has_wildcard_success`: Set to `True` when the OpenAPI spec defines `'2XX'` wildcard in responses section, indicating any 2xx status code should be accepted as success. When `True`, generated code uses range check `(200 <= status < 300)` instead of explicit list check.
1037+
10241038
### Template Structure
10251039

10261040
**service.jinja2**:
@@ -1078,8 +1092,10 @@ def _create_error_from_response(...):
10781092
10791093
# Check success
10801094
{% if operation.has_wildcard_success %}
1095+
# OpenAPI spec uses '2XX' wildcard - accept any 2xx status code
10811096
if not (200 <= response.status_code < 300):
10821097
{% else %}
1098+
# OpenAPI spec defines explicit success codes
10831099
if response.status_code not in {{ operation.success_codes }}:
10841100
{% endif %}
10851101
raise _create_error_from_response(...)

0 commit comments

Comments
 (0)