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

Bugfix/return field name or load from on validation errors #232

Merged
merged 3 commits into from Jul 8, 2015

Conversation

Projects
None yet
2 participants
@alexmorken

alexmorken commented Jul 6, 2015

Fixed bug where attribute (not field_name or load_from param) was being returned on errors when deserializing data, wrote tests and updated docs.

Alex Morken added some commits Jul 6, 2015

Alex Morken
Fixed bug where attribute (not field_name or load_from param) was bei…
…ng returned when deserializing data

Fixed bug where attribute (not field_name or load_from param) was being returned on errors when deserializing data
'years': 'abc'
}
result, errors = AliasingUserSerializer().load(data)
assert errors['UserName'] == [u'"foobar.com" is not a valid email address.']

This comment has been minimized.

@sloria

sloria Jul 7, 2015

Member

I'm not sure about this behavior: why should the key on errors change if the input value is missing vs. present? My expectation (as a user) would be that the key on errors is always the same as the name of the field.

This comment has been minimized.

@alexmorken

alexmorken Jul 7, 2015

Yes, I had the same concern, but decided to do it this way because it is
the actual field name the data was loaded from (being that it only gets set
if it can't load from the attribute or field name).

I am fine with only passing back the actual field name, but I do prefer the
clear reflection of what was actually happening when loading the data.

On Mon, Jul 6, 2015 at 8:40 PM, Steven Loria notifications@github.com
wrote:

In tests/test_deserialization.py
#232 (comment)
:

  •    result, errors = AliasingUserSerializer().load(data)
    
  •    assert errors
    
  •    assert errors['username'] == [u'"foobar.com" is not a valid email address.']
    
  • def test_deserialize_with_attribute_param_error_returns_load_from_not_attribute_name(self):
  •    class AliasingUserSerializer(Schema):
    
  •        name = fields.String(load_from='Name')
    
  •        username = fields.Email(attribute='email', load_from='UserName')
    
  •        years = fields.Integer(attribute='age', load_from='Years')
    
  •    data = {
    
  •        'Name': 'Mick',
    
  •        'UserName': 'foobar.com',
    
  •        'years': 'abc'
    
  •    }
    
  •    result, errors = AliasingUserSerializer().load(data)
    
  •    assert errors['UserName'] == [u'"foobar.com" is not a valid email address.']
    

I'm not sure about this behavior: why should the key on errors change if
the input value is missing vs. present? My expectation (as a user) would be
that the key on errors is always the same as the name of the field.


Reply to this email directly or view it on GitHub
https://github.com/marshmallow-code/marshmallow/pull/232/files#r34005850
.

This comment has been minimized.

@sloria

sloria Jul 7, 2015

Member

OK, I see now why you'd want the error keys to be the same as load_from because the field names could be considered internal aliases for the value passed by the client. However, I'm still not clear on why you would only want to use load_from when the value is missing from the input. It seems more consistent to always use load_from if it is set.

This comment has been minimized.

@alexmorken

alexmorken Jul 7, 2015

What I did was just follow the behavior of where the data is actually
loaded from. I thought it would be easier for the client to introspect if
we handed back the name of the field they specified instead of the one
specified by load_from. load_from only gets used as fallback and I
thought I would reflect that in the error handling.

On Tue, Jul 7, 2015 at 5:12 AM, Steven Loria notifications@github.com
wrote:

In tests/test_deserialization.py
#232 (comment)
:

  •    result, errors = AliasingUserSerializer().load(data)
    
  •    assert errors
    
  •    assert errors['username'] == [u'"foobar.com" is not a valid email address.']
    
  • def test_deserialize_with_attribute_param_error_returns_load_from_not_attribute_name(self):
  •    class AliasingUserSerializer(Schema):
    
  •        name = fields.String(load_from='Name')
    
  •        username = fields.Email(attribute='email', load_from='UserName')
    
  •        years = fields.Integer(attribute='age', load_from='Years')
    
  •    data = {
    
  •        'Name': 'Mick',
    
  •        'UserName': 'foobar.com',
    
  •        'years': 'abc'
    
  •    }
    
  •    result, errors = AliasingUserSerializer().load(data)
    
  •    assert errors['UserName'] == [u'"foobar.com" is not a valid email address.']
    

OK, I see now why you'd want the error keys to be the same as load_from
because the field names could be considered internal aliases for the value
passed by the client. However, I'm still not clear on why you would only
want to use load_from when the value is missing from the input. It seems
more consistent to always use load_from if it is set.


Reply to this email directly or view it on GitHub
https://github.com/marshmallow-code/marshmallow/pull/232/files#r34032640
.

This comment has been minimized.

@sloria

sloria Jul 8, 2015

Member

Ah ok. Yes, that seems right. Thanks for the clarification!

sloria added a commit that referenced this pull request Jul 8, 2015

Merge pull request #232 from alexmorken/bugfix/return_field_name_or_l…
…oad_from_on_validation_errors

Bugfix/return field name or load from on validation errors

@sloria sloria merged commit e4112f9 into marshmallow-code:dev Jul 8, 2015

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@alexmorken alexmorken deleted the alexmorken:bugfix/return_field_name_or_load_from_on_validation_errors branch Jul 15, 2015

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