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 global Schema state corruption #107

Merged
merged 1 commit into from Feb 17, 2017

Conversation

@serebrov
Copy link
Contributor

commented Feb 15, 2017

The Schemas setup is done on the class level,
so Schema and it's fields are global objects, initialized during the
application startup.

The global field objects are available as SchemaClass._declared_fields
or as schema_object._declared_fields when we are working with the Schema
instance.

Schema itself does this:

    self.declared_fields = copy.deepcopy(self._declared_fields)`

before doing anything with the fields.

So self._declared_fields are global, and self.declared_fields are local
in the field object.
But apispec uses global copy during the serialization thus modifying this
global state and affecting further Schema usages.

This issue was tracked down with Nested field, which has __schema property.
This property is not set on object creation and modified later, during
the serialization (see https://github.com/marshmallow-code/marshmallow/blob/4457426e7447195e5b390605ec90349b81ded507/marshmallow/fields.py#L398).

So the following assertion was always true:

assert GalleryLocationSchema._declared_fields['city']._Nested__schema is None

But it failed after exporting APISpec to json:

apispec.init_app(current_app)
data = apispec.to_dict()
json.dumps(data)
assert GalleryLocationSchema._declared_fields['city']._Nested__schema is None  # Fails!

The assert started failing after json.dumps, which triggered the serialization
of the data in LazyDicts inside the apispec.ext.marshmallow.swagger which
in turn triggered the serialization process for the global
Schema._declared_fields.

The overall effect of the issue in our case was a complete breakage of the
application, our API started serving empty objects after someone accessed
the API docs in swagger.

The fix is to do the same thing Schema.__init__ does and copy the fields
before doing anything to them.

Fix global Schema state corruption
The `Schema`s setup is done on the class level,
so `Schema` and it's fields are global objects, initialized during the
application startup.

The global field objects are available as `SchemaClass._declared_fields`
or as `schema_object._declared_fields` when we are working with the Schema
instance.

`Schema` itself does this:

```
    self.declared_fields = copy.deepcopy(self._declared_fields)`
```

before doing anything with the fields.

So `self._declared_fields` are global, and `self.declared_fields` are local
in the field object.
And apispec uses global copy during the serialization thus modifying this
global state and affecting further Schema usages.

This issue was tracked down with `Nested` field, which has `__schema` property.
This property is not set on object creation and modified later, during
the serialization (see https://github.com/marshmallow-code/marshmallow/blob/4457426e7447195e5b390605ec90349b81ded507/marshmallow/fields.py#L398).

So the following assertion was always true:

```
assert GalleryLocationSchema._declared_fields['city']._Nested__schema is None
```

But it failed after exporting APISpec to json:

```
apispec.init_app(current_app)
data = apispec.to_dict()
json.dumps(data)
assert GalleryLocationSchema._declared_fields['city']._Nested__schema is None  # Fails!
```

The assert started failing after json.dupms, which triggers the serialization
of the data in `LazyDict`s inside the apispec.ext.marshmallow.swagger which
in turn triggered the serialization process for the global
`Schema._declared_fields`.

The overall effect of the issue in our case was a complete breakage of the
application, our API started serving empty objects after someone access
the API docs in swagger.

The fix is to do the same thing `Schema.__init__` does and copy the fields
before doing anything to them.
@sloria

This comment has been minimized.

Copy link
Member

commented Feb 17, 2017

Good catch! Thanks for the patch.

@sloria sloria merged commit 665c0ee into marshmallow-code:dev Feb 17, 2017

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.