Repository: marshmallow-code/apispec
Pull request #490 (2019-08-31 12:47:29)

Fix documentation of default from missing with marshmallow 3
Addresses #342.

It sucks not being able to document default values automatically, especially since it would probably work most of the times, but I don't see a safe way to do it.

Without this feature removal, we may pass to the spec dict objects that are not json serializable, which will trigger an exception when serializing the spec file.

sloria: > OTOH, in most cases, dumping missing is perfectly fine, so removing automatic default documentation would be a regression.

This. As a user, I want apispec to work mostly transparently, i.e. without having to modify my app code. 

Maybe we could handle errors better to lead the user toward using `doc_default` when it's necessary, but I don't think we should degrade the 90% use case.
lafrech: Well, we could try to dump the `missing` value with the `Field`. Let's give it a try.

I don't know if we should document the `missing` value of `Nested` fields or container fields with `Nested` inside. The examples I found for default values are "scalar" values like integers or strings but I'm not sure a dict is allowed. Anyway, that's nothing new. Same applies to MA2.
lafrech: I just pushed a new commit to serialize `missing` with the field.

It has a few smells:

- Accesses underscored `_serialize` method (no big deal).
- Can't pass `attr` and `obj` parameters of `_serialize`. This shouldn't be an issue in most cases as those are almost never used. In fact, the only uses I know of are `Function` and `Method` (I already commented about this in https://github.com/marshmallow-code/marshmallow/issues/1046) and those are dump_only.

We should probably only document missing for fields that are not dump_only. It doesn't make sense anyway, does it?

Also, I'd need to add proper tests to show it works with a field where this matters, such as `DateTime`.
lafrech: Not quite.

Indeed we shouldn't document default on a `dump_only` field.

However, `Function` and `Method` may not be `dump_only` and in this case, the code is likely to crash. But maybe not, if the method can take `None` as a parameter.

> Maybe we could handle errors better to lead the user toward using `doc_default` when it's necessary

So shall we call `_serialize`, catch `Exception` and return a message telling the user to use `doc_default`? The result may even be wrong without an exception being raised, so this wouldn't be 100% sure. But admittedly, those would be corner cases. Perhaps a note in the docs would be sufficient.

Or rather than passing `None`, could we pass some sort of proxy object as `attr` and `obj` that would raise an exception when used? For `obj`, we could override getter methods, etc. But for `attr`...
sloria: For now, I think documentation may be the best course. 
lafrech: I added a piece of doc about `MarshmallowPlugin`.

Feedback welcome.

> Indeed we shouldn't document default on a `dump_only` field.

Maybe not but a field with `dump_only` and `missing` is wrong in marshmallow anyway, and we generally don't try to validate user inputs in apispec, so I removed the corresponding case from this PR.

Note: `default` in `_VALID_PROPERTIES` is useless because it can't be in `metadata` as it is a reserved marshmallow attribute. I thought of removing it from the list but things might change in https://github.com/marshmallow-code/marshmallow/issues/1350 (MA 4) so I left it there for now, it is harmless.
sloria: Go ahead and merge when you think this is ready.

Patch:
diff --git a/CHANGELOG.rst b/CHANGELOG.rst
index 33cfa14..d30d8db 100644
--- a/CHANGELOG.rst
+++ b/CHANGELOG.rst
@@ -13,6 +13,12 @@ Features:
   ``Schema`` class. This allows a custom resolver to generate different names
   depending on schema modifiers used in a ``Schema`` instance. (:pr:`476`)
 
+Bug fixes:
+
+- MarshmallowPlugin: With marshmallow 3, the default value of a field in the
+  documentation is the dump of the ``missing`` attribute, not ``missing``
+  itself (:pr:`490`).
+
 Other changes:
 
 - Drop support for Python 2 (:issue:`491`). Thanks :user:`hugovk` for the PR.
diff --git a/src/apispec/ext/marshmallow/__init__.py b/src/apispec/ext/marshmallow/__init__.py
index f68915d..df2d3c6 100644
--- a/src/apispec/ext/marshmallow/__init__.py
+++ b/src/apispec/ext/marshmallow/__init__.py
@@ -1,4 +1,4 @@
-"""Marshmallow plugin for apispec. Allows passing a marshmallow
+"""marshmallow plugin for apispec. Allows passing a marshmallow
 `Schema` to `spec.components.schema <apispec.core.Components.schema>`,
 `spec.components.parameter <apispec.core.Components.parameter>`,
 `spec.components.response <apispec.core.Components.response>`
@@ -7,6 +7,28 @@
 
 Requires marshmallow>=2.15.2.
 
+``MarshmallowPlugin`` maps marshmallow ``Field`` classes with OpenAPI types and
+formats.
+
+It inspects field attributes to automatically document properties
+such as read/write-only, range and length constraints, etc.
+
+OpenAPI properties can also be passed as metadata to the ``Field`` instance
+if they can't be inferred from the field attributes (`description`,...), or to
+override automatic documentation (`readOnly`,...). A metadata attribute is used
+in the documentation either if it is a valid OpenAPI property, or if it starts
+with `"x-"` (vendor extension).
+
+.. warning::
+
+    ``MarshmallowPlugin`` infers the ``default`` property from the ``missing``
+    attribute of the ``Field`` (unless ``missing`` is a callable).
+    In marshmallow 3, default values are entered in deserialized form,
+    so the value is serialized by the ``Field`` instance.
+    This may lead to inaccurate documentation in very specific cases.
+    The default value to display in the documentation can be
+    specified explicitly by passing ``doc_default`` as metadata.
+
 ::
 
     from pprint import pprint
diff --git a/src/apispec/ext/marshmallow/field_converter.py b/src/apispec/ext/marshmallow/field_converter.py
index 3c1cbfb..1340ca3 100644
--- a/src/apispec/ext/marshmallow/field_converter.py
+++ b/src/apispec/ext/marshmallow/field_converter.py
@@ -206,8 +206,9 @@ class FieldConverterMixin:
         else:
             default = field.missing
             if default is not marshmallow.missing and not callable(default):
+                if MARSHMALLOW_VERSION_INFO[0] >= 3:
+                    default = field._serialize(default, None, None)
                 ret["default"] = default
-
         return ret
 
     def field2choices(self, field, **kwargs):


Test patch:
diff --git a/tests/test_ext_marshmallow_field.py b/tests/test_ext_marshmallow_field.py
index 04736ca..dd520d4 100644
--- a/tests/test_ext_marshmallow_field.py
+++ b/tests/test_ext_marshmallow_field.py
@@ -1,3 +1,4 @@
+import datetime as dt
 import re
 
 import pytest
@@ -5,6 +6,7 @@ from marshmallow import fields, validate
 
 from apispec import APISpec
 from apispec.ext.marshmallow import MarshmallowPlugin
+from apispec.ext.marshmallow.openapi import MARSHMALLOW_VERSION_INFO
 
 from .schemas import CategorySchema, CustomList, CustomStringField, CustomIntegerField
 from .utils import build_ref
@@ -84,12 +86,21 @@ def test_field_with_missing(spec_fixture):
     assert res["default"] == "bar"
 
 
-def test_field_with_boolean_false_missing(spec_fixture):
+def test_boolean_field_with_false_missing(spec_fixture):
     field = fields.Boolean(default=None, missing=False)
     res = spec_fixture.openapi.field2property(field)
     assert res["default"] is False
 
 
+def test_datetime_field_with_missing(spec_fixture):
+    if MARSHMALLOW_VERSION_INFO[0] < 3:
+        field = fields.Date(missing=dt.date(2014, 7, 18).isoformat())
+    else:
+        field = fields.Date(missing=dt.date(2014, 7, 18))
+    res = spec_fixture.openapi.field2property(field)
+    assert res["default"] == dt.date(2014, 7, 18).isoformat()
+
+
 def test_field_with_missing_callable(spec_fixture):
     field = fields.Str(missing=lambda: "dummy")
     res = spec_fixture.openapi.field2property(field)