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

wrong default value for 'name' in schema2parameters #829

Open
codectl opened this issue Mar 7, 2023 · 3 comments
Open

wrong default value for 'name' in schema2parameters #829

codectl opened this issue Mar 7, 2023 · 3 comments

Comments

@codectl
Copy link
Contributor

codectl commented Mar 7, 2023

The signature of the method schema2parameters in openapi.py is the following:

    def schema2parameters(
        self,
        schema,
        *,
        location,
        name: str = "body",
        required: bool = False,
        description: str | None = None,
    ):
...

I would say body is the default value for location and not name.

@lafrech
Copy link
Member

lafrech commented Mar 7, 2023

Nope. This is the default name for OAS2 body parameter. User may specify a name or we fallback on "body" as a dummy name (needed because "name" is required in the spec).

I just tried to remove this line and it fails test_schema_expand_parameters_v2.

@codectl
Copy link
Contributor Author

codectl commented Mar 7, 2023

I understand that name is required for both OAS2 and OAS3. This part only affects OAS2 since location="body" only exists in OAS2. Thing is that it just looks weird that body is a default value for name and thinking about it would also be weird if it was default value for location.

I'd say it may make more sense to have something like the following:

    def schema2parameters(
        self,
        schema,
        *,
        location: str,
        name: str | None = None,
        required: bool = False,
        description: str | None = None,
    ):
        location = __location_map__.get(location, location)
        # OAS 2 body parameter
        if location == "body":
            name = (
                name or schema.__class__.__name__
                if isinstance(schema, marshmallow.Schema)
                else str(name)
            )
            param = {
                "in": location,
                "required": required,
                "name": name,
                "schema": self.resolve_nested_schema(schema),
            }

The test suite still works for this case. Let me know your thoughts on it.

@lafrech
Copy link
Member

lafrech commented Aug 17, 2023

Sounds reasonable. I don't think it matters much but why not.

Should we consider the case where schema is not a Schema and name is not specified, and in this case use "body" as default name, to avoid using str(None)?

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

No branches or pull requests

2 participants