Dump Error Keys

[deckar01]

Store errors by key when dumping collections to prevent item errors from being reported as their parent.

Fixes #1132

---

Example:

from marshmallow import Schema, ValidationError, fields


class Test(Schema):
    foo = fields.List(fields.Int)

Test().dump({'foo': ['five']})
# Before: {'foo': ['Not a valid integer.']}
# After: {'foo': {0: ['Not a valid integer.']}}

[deckar01]

I'm not really happy with copy and pasting this much code. It is effectively 6 8 4 copies of the same thing, with slight variations. I suspect there is a utility interface that can abstract this functionality while also improving the code's readability.

[deckar01]

I took a first pass at deduplicating the logic. I didn't see a clean way to abstract the operations across field classes, but I avoided introducing any duplicate logic in this PR and deduplicated a few operations that were already present.

I need to test what the performance implications are for this abstraction. It should be minimal, because the primary difference is getattr being used in place of static attribute access once in each method. An alternative would be to use a lambda instead, but I don't have an intuition for which is preferable or faster.

A few methods were passing None into the attr and obj/data arguments instead of passing them through like the rest. Normalizing this did not affect the tests, but I suspect the original behavior was an obscure bug. I need to look at the implications of this change a little closer.

[deckar01]

I suspect the original behavior was an obscure bug.

Confirmed. See #1176.

[sloria]

Implementation looks correct; just a minor spelling change suggested.

I'm also not sure about how to deduplicate the logic across field classes without introducing a confusing amount of indirection. This could happen after @lafrech's suggested ContainerField becomes a reality, but no need to block merging of this PR nor #1066 for it.

Granted there are no performance regressions with this, I think this is on the right track.

[lafrech]

I pushed a ContainerMixin proposal in #1066.

[lafrech]

I finally dropped the ContainerMixin I proposed in #1066 and submitted a simpler PR (#1229) so don't count on it.

[deckar01]

I will get this up to date now that #1229 has landed.

[sloria]

@deckar01 Are you still working on this? Let us know if you need help with it.

[deckar01]

Benchmark:

Setup: benchmark.py

# dev
load 18.1108 µs ± 0.65%
dump 10.8205 µs ± 1.14%
load no err 18.0016 µs ± 0.69%
dump no err 10.7058 µs ± 2.38%

# 1132-nested-dump-errors
load 18.7329 µs ± 0.85%
dump 10.8929 µs ± 0.42%
load no err 18.6097 µs ± 0.85%
dump no err 10.9445 µs ± 0.97%

# change
load +3.43%
dump +0.66%
load +3.37%
dump +2.23%

[sloria]

It's a minor performance regression, but I'm wondering if we should incur it at all...we've since decided that validation should only happen on deserialization and that the dump should be considered valid. Do we want to continue supporting validation-on-serialization?

[deckar01]

The performance regression is a side effect of refactoring to avoid copy and pasting validation code. It could be omitted. I will continue the discussion about dump validation on the issue.

[deckar01]

Closing in favor of #1304.