apispec icon indicating copy to clipboard operation
apispec copied to clipboard

wrong default value for 'name' in schema2parameters

Open codectl opened this issue 1 year ago • 3 comments

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.

codectl avatar Mar 07 '23 19:03 codectl

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.

lafrech avatar Mar 07 '23 20:03 lafrech

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.

codectl avatar Mar 07 '23 20:03 codectl

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)?

lafrech avatar Aug 17 '23 10:08 lafrech