Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix missing Field.default when it's value is None in openapi spec #189

Merged
merged 5 commits into from
Nov 10, 2024

Conversation

ddorian
Copy link
Contributor

@ddorian ddorian commented Oct 24, 2024

Checklist:

  • Run pytest tests and no failed.
  • Run ruff check flask_openapi3 tests examples and no failed.
  • Run mypy flask_openapi3 and no failed.
  • Run mkdocs serve and no failed.

@@ -242,6 +242,9 @@ def api_doc(self) -> Dict:

# Convert spec to JSON
self.spec_json = json.loads(spec.model_dump_json(by_alias=True, exclude_none=True, warnings=False))
# fix to include `default=None` in fields
components_only = json.loads(spec.model_dump_json(by_alias=True, exclude_unset=True, warnings=False))["components"]
Copy link
Owner

Choose a reason for hiding this comment

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

Why components only?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The other models have many fields with default None that show up in the output (example=None, security=None, deprecated=None etc).

This worked for me on a 5K lines json schema.

Copy link
Owner

Choose a reason for hiding this comment

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

I agree to replace exclude_none with exclude_unset, but I don't think it's perfect just for components.
Can you write an example of an error other than the action components?

Copy link
Contributor Author

@ddorian ddorian Oct 26, 2024

Choose a reason for hiding this comment

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

Using this:

self.spec_json = spec.model_dump(mode="json", by_alias=True, exclude_unset=True, warnings=False)

These are the fields that get removed:

RequestBody.required = True

These are the extra fields that show up in json. I guess they are being set from functions.

Operation.deprecated = None
Operation.externalDocs = None
Opeation.example = None
Operation.examples = None
Operation.servers = None
Operation.security = None
Parameter.deprecated = None
Parameter.example = None
Parameter.examples = None

So we can do my way or fix functions that set the values above to None.

Copy link
Owner

Choose a reason for hiding this comment

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

Some models end up in parameters, so just using the unset rule in components is not a good solution.

I committed a commit with all assignments using the unset rule.

Hopefully we can agree.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Works great for me.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

After my 2 commits my tests are fine and the json is valid openapi spec.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@luolingchun can you merge my 2 PRs?

Copy link
Contributor Author

@ddorian ddorian left a comment

Choose a reason for hiding this comment

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

On paths without parameters, parameters shows as null, but it's not an issue.


After this, we can add even more defaults to make the code nicer. Like setting:

# instead of 
webhooks: Optional[Dict[str, Union[PathItem, Reference]]] = None
# we do
webhooks: Dict[str, Union[PathItem, Reference]] = {}
# and output should be the same in all cases

flask_openapi3/utils.py Outdated Show resolved Hide resolved
@luolingchun luolingchun merged commit 9b94105 into luolingchun:master Nov 10, 2024
10 checks passed
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.

2 participants