fix: Add type field for anyOf parameters #5
Conversation
luliangce
force-pushed
the
dev
branch
2 times, most recently
from
4e75f8d to
d0a13db
Compare
|
Thanks @luliangce will take a look over the weekend. Can you please share some sample spec/ code to test this change? Thanks |
| if "anyOf" in properties[p].keys(): | ||
| any_of = properties[p].pop("anyOf") | ||
| if len(any_of) <= 2: | ||
| if len(any_of) <= 2 and {"type": "null"} in any_of: |
There was a problem hiding this comment.
@luliangce I am little unsure on this condition change. Can you please share some sample code which would trigger this condition? Thanks
|
/gemini review |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request addresses an issue with anyOf parameters in the generated Swagger 2.0 specification. The changes ensure a type field is always present and handle anyOf for optional parameters and union types.
There are a couple of issues with the implementation:
- A high-severity logical bug in
get_swagger2_operation_parameterswhere the order of operations can cause the specific type fromanyOfto be overwritten by a more generic one. - A medium-severity bug in
get_swagger2that can lead to an invalid schema for properties that are nullable but have no other type specified.
Detailed comments and code suggestions are provided to address these issues.
| # according to the https://swagger.io/specification/v2/#:~:text=If%20in%20is%20any%20value%20other%20than%20%22body%22%3A | ||
| # field "type" if required | ||
| parameter["type"] = schema.get("type", "string") # fallback | ||
| if "anyOf" in schema: | ||
| any_of = schema.pop("anyOf") | ||
| if {"type": "null"} in any_of: | ||
| parameter.update({"x-nullable": True, "required": False}) | ||
| any_of.remove({"type": "null"}) | ||
| if len(any_of) == 1: | ||
| parameter.update(any_of[0]) | ||
| else: | ||
| parameter.update({"type": "string"}) | ||
| logger.warning( | ||
| f"fastapi_swagger2: Unable to handle anyOf in parameters {any_of}, use string type. Route: {route.path_format}, Method: {method}, Param: {param.alias}" | ||
| ) | ||
|
|
||
|
|
||
| parameter.update( | ||
| {k: v for (k, v) in schema.items() if k not in ["title", "anyOf"]} | ||
| ) |
There was a problem hiding this comment.
The current logic for handling anyOf in parameters has a flaw in the order of operations. It first determines the type from anyOf, but then overwrites it with a more generic type from the base schema dictionary. This can lead to incorrect parameter types in the generated Swagger specification (e.g., a parameter of type Optional[int] being documented as a string).
The logic should be reordered to first apply the general properties from the schema, and then let the more specific anyOf logic override them. This ensures that the final parameter schema is correct.
Consider replacing this block with the following implementation, which corrects the order of operations and also handles the case where anyOf only contains a null type more gracefully.
# according to the https://swagger.io/specification/v2/#:~:text=If%20in%20is%20any%20value%20other%20than%20%22body%22%3A
# field "type" is required
parameter.update(
{k: v for (k, v) in schema.items() if k not in ["title", "anyOf"]}
)
if "anyOf" in schema:
any_of = schema.pop("anyOf")
if {"type": "null"} in any_of:
parameter.update({"x-nullable": True, "required": False})
any_of = [item for item in any_of if item != {"type": "null"}]
if len(any_of) == 1:
parameter.update(any_of[0])
elif any_of:
parameter.update({"type": "string"})
logger.warning(
f"fastapi_swagger2: Unable to handle anyOf in parameters {any_of}, use string type. Route: {route.path_format}, Method: {method}, Param: {param.alias}"
)
if "type" not in parameter:
parameter["type"] = "string" # fallback| if "anyOf" in properties[p].keys(): | ||
| any_of = properties[p].pop("anyOf") | ||
| if len(any_of) <= 2: | ||
| if len(any_of) <= 2 and {"type": "null"} in any_of: |
There was a problem hiding this comment.
This condition is intended to handle optional properties (Union[T, None]), but it's a bit too broad. Using <= can lead to incorrect schema generation for properties that are defined as anyOf with only a single {"type": "null"} item. In this edge case, the resulting property schema would be {"x-nullable": true} without a type, which is invalid.
Consider changing <= to == to make the logic more precise, ensuring it only applies to anyOf with exactly two items (one of which is null), which is the correct representation for an optional type. This avoids the edge case and makes the code more robust.
| if len(any_of) <= 2 and {"type": "null"} in any_of: | |
| if len(any_of) == 2 and {"type": "null"} in any_of: |
|
Really appreciate the work being done here — this repository has been a huge help. I’m also running into the issues this PR intends to fix. Just wondering if there are any updates or next steps planned? |
|
@luliangce @faye-lomente-acn |
@luliangce can you please take a look at the comments? Thanks |
|
@luliangce @faye-lomente-acn Can you please check if the latest release fixes the issue you are facing? |
3 participants
Hi, thanks for your work!
I found an issue and have tried to fix it. Could you please take a look when you have time?