Input payload validation fails on fields with required=False #179

Open
postrational opened this Issue Jun 7, 2016 · 8 comments

Comments

Projects
None yet
4 participants
@postrational

Hi there,

we're using Flask-Restplus 0.9.0 and we're big fans.

However we keep running into an issue when trying to use optional fields.

Our code is similar to the following:

model = api.model('Example model', {
    'optional_field': fields.String(required=False),
})

@ns.route('/test/<int:item_id>/')
class Item(Resource):

    @api.marshal_with(model)
    def get(self, item_id):
        """
        Returns an Item.
        """
        return get_item(item_id)

    @api.expect(model)
    def put(self, item_id):
        """
        Updates an Item.
        """
        item_data = request.json
        return update_item(item_id, item_data)

Now when we do a GET to the API endpint, we will receive an Item with the optional field containing a null value.

{
    "optional_field": null
}

Unfortunately, when we try to later send the same item to the API, we get an error message:

{
    "message": "Input payload validation failed", 
    "errors": {"optional_field": "None is not of type 'string'"}
}

This behavior seems inconsistent. I think it would be better

  • not to serialize the optional_field at all in the marshal_with method
  • or alternatively to accept null as a valid value of a field which has required=False regardless of its type.

Is there a workaround for this which allows the optional_field to stay undefined?

Thanks for all your great work!

@petroslamb

This comment has been minimized.

Show comment
Hide comment
@petroslamb

petroslamb Jun 8, 2016

Hi @postrational,

We had the same issue and solved it by extending the json schema type of the field String to two types, like so:

class NullableString(fields.String):
    __schema_type__ = ['string', 'null']
    __schema_example__ = 'nullable string'

Hope this helps.

Hi @postrational,

We had the same issue and solved it by extending the json schema type of the field String to two types, like so:

class NullableString(fields.String):
    __schema_type__ = ['string', 'null']
    __schema_example__ = 'nullable string'

Hope this helps.

@postrational

This comment has been minimized.

Show comment
Hide comment
@postrational

postrational Jun 8, 2016

@petroslamb Cool, thanks, we'll give this a try.

@petroslamb Cool, thanks, we'll give this a try.

@postrational

This comment has been minimized.

Show comment
Hide comment
@postrational

postrational Jun 9, 2016

We tested the solution with NullableString and it works well.

However it still add fields with null values ("optional_field": null) to our models.

When we persist these models, we either have to ignore these additional null fields or we would have to remove them manually before saving.

In my opinion, a more perfect solution would simply not add a String field to a model if the value is null.

Is this a discussion to be had here or in the upstream Flask forum?

postrational commented Jun 9, 2016

We tested the solution with NullableString and it works well.

However it still add fields with null values ("optional_field": null) to our models.

When we persist these models, we either have to ignore these additional null fields or we would have to remove them manually before saving.

In my opinion, a more perfect solution would simply not add a String field to a model if the value is null.

Is this a discussion to be had here or in the upstream Flask forum?

@aviaryan aviaryan referenced this issue in fossasia/open-event-server Jun 11, 2016

Closed

Full custom payload validation for API #717

@aviaryan

This comment has been minimized.

Show comment
Hide comment
@aviaryan

aviaryan Jun 11, 2016

I tried this for a nullable Integer Field and the integer field vanished from the swagger UI.
I did some testing and found that the field dissappears from swagger as soon as schema_type becomes a list.
Even __schema_type__ = ['integer'] fails.
Using restplus 0.9.2

I tried this for a nullable Integer Field and the integer field vanished from the swagger UI.
I did some testing and found that the field dissappears from swagger as soon as schema_type becomes a list.
Even __schema_type__ = ['integer'] fails.
Using restplus 0.9.2

@aviaryan

This comment has been minimized.

Show comment
Hide comment
@aviaryan

aviaryan Jun 11, 2016

or alternatively to accept null as a valid value of a field which has required=False regardless of its type.

👍

or alternatively to accept null as a valid value of a field which has required=False regardless of its type.

👍

@aviaryan

This comment has been minimized.

Show comment
Hide comment
@aviaryan

aviaryan Jun 11, 2016

I tried this for a nullable Integer Field and the integer field vanished from the swagger UI.

I set __schema_example__ also and it is now working. Can't understand why 😓

I tried this for a nullable Integer Field and the integer field vanished from the swagger UI.

I set __schema_example__ also and it is now working. Can't understand why 😓

@DHager

This comment has been minimized.

Show comment
Hide comment
@DHager

DHager May 3, 2018

Digging up this issue because I'm having the same problem now. This bug causes problems round-tripping data and in testing, since it basically corrupts data. An optional key-value which is absent reappears with an illegal null payload.

A quick demo to show the principle:

from flask_restplus.namespace import Namespace
from flask_restplus import Resource, fields

api = Namespace('test')

test_model = api.model('Test', [
    ('Name', fields.String(
        description='A non-required number',
    )),
])


@api.route('/')
class Test(Resource):
    @api.marshal_with(test_model)
    @api.expect(test_model, validate=True)
    def put(self):
        data = self.api.payload
        return data, 200
  1. Send {}
  2. Get back {"Name":null}
  3. Send {"Name":null}
  4. Bzzt! Validation error!

DHager commented May 3, 2018

Digging up this issue because I'm having the same problem now. This bug causes problems round-tripping data and in testing, since it basically corrupts data. An optional key-value which is absent reappears with an illegal null payload.

A quick demo to show the principle:

from flask_restplus.namespace import Namespace
from flask_restplus import Resource, fields

api = Namespace('test')

test_model = api.model('Test', [
    ('Name', fields.String(
        description='A non-required number',
    )),
])


@api.route('/')
class Test(Resource):
    @api.marshal_with(test_model)
    @api.expect(test_model, validate=True)
    def put(self):
        data = self.api.payload
        return data, 200
  1. Send {}
  2. Get back {"Name":null}
  3. Send {"Name":null}
  4. Bzzt! Validation error!

DHager added a commit to DHager/flask-restplus that referenced this issue May 3, 2018

@DHager

This comment has been minimized.

Show comment
Hide comment
@DHager

DHager May 4, 2018

Here's one possible workaround, all it does is recursively strips out key-values where the payload is None before everything gets turned into a JSON string.

def _stripNone(data):
    if isinstance(data, dict):
        return {k: _stripNone(v) for k, v in data.items() if k is not None and v is not None}
    elif isinstance(data, list):
        return [_stripNone(item) for item in data if item is not None]
    elif isinstance(data, tuple):
        return tuple(_stripNone(item) for item in data if item is not None)
    elif isinstance(data, set):
        return {_stripNone(item) for item in data if item is not None}
    else:
        return data


def fix_null_marshalling(fn):
    """
    Intended to be applied around (above) the Flask-restplus decorator
    @api.marshal_with(...)

    See: https://github.com/noirbizarre/flask-restplus/issues/179
    """

    @wraps(fn)
    def wrapper(*args, **kwargs):
        result = fn(*args, **kwargs)
        return _stripNone(result)

    return wrapper

Applied like so:

@api.route('/')
class Echo(Resource):

    @fix_null_marshalling
    @api.marshal_with(model)
    @api.expect(model)
    def post(self):
        data = self.api.payload
        return data, 200

I also experimented with a decorator that takes the same model-object, so that it can more-intelligently decide which nulls to erase, but that started to seem like overkill compared to a PR.

DHager commented May 4, 2018

Here's one possible workaround, all it does is recursively strips out key-values where the payload is None before everything gets turned into a JSON string.

def _stripNone(data):
    if isinstance(data, dict):
        return {k: _stripNone(v) for k, v in data.items() if k is not None and v is not None}
    elif isinstance(data, list):
        return [_stripNone(item) for item in data if item is not None]
    elif isinstance(data, tuple):
        return tuple(_stripNone(item) for item in data if item is not None)
    elif isinstance(data, set):
        return {_stripNone(item) for item in data if item is not None}
    else:
        return data


def fix_null_marshalling(fn):
    """
    Intended to be applied around (above) the Flask-restplus decorator
    @api.marshal_with(...)

    See: https://github.com/noirbizarre/flask-restplus/issues/179
    """

    @wraps(fn)
    def wrapper(*args, **kwargs):
        result = fn(*args, **kwargs)
        return _stripNone(result)

    return wrapper

Applied like so:

@api.route('/')
class Echo(Resource):

    @fix_null_marshalling
    @api.marshal_with(model)
    @api.expect(model)
    def post(self):
        data = self.api.payload
        return data, 200

I also experimented with a decorator that takes the same model-object, so that it can more-intelligently decide which nulls to erase, but that started to seem like overkill compared to a PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment