RFC: Remove ErrorStore.error_kwargs (because its update is undeterministic)

[lafrech]

While working on #879, I did a little bit of cleanup in marshalling.py (#995).

I noticed the error_kwargs attribute of ErrorStore.

I don't think it is documented and it wasn't tested until a bug was opened because it was not reset upon ErrorStore instantiation. This bug was fixed in #564 and a test was added.

I might be misunderstanding the feature but it looks undeterministic: When multiple ValidationErrors are raised with arbitrary kwargs, those kwargs are aggregated in the ValidationError that is generated at the end of the dump or load process. Since the kwargs are added to error_kwargs using update, there can be key collisions and last error speaking has priority. This is the part that I think is undeterministic.

I was about to suggest removing this because I didn't see the point.

But before doing so, I checked what it would do to webargs. And in fact, it breaks a few tests. Always the same use case but once for each web framework:

@app.route("/error400", methods=["GET", "POST"])
def error400():
    def always_fail(value):
        raise ValidationError("something went wrong", status_code=400)  # <--- here

    args = {"text": fields.Str(validate=always_fail)}
    return J(parser.parse(args))

A field validator raises an error with a status code, and this status code is used in the response.

From there, I found the history of the feature:

First PR: #418, refined in @sloria's implementation : #428.

And related webargs issue (marshmallow-code/webargs#85) and fix (marshmallow-code/webargs#97).

So it definitely is intentional.

But what if several fields return different values of status_code? The resulting status_code depends on the validation order. I know the order may be enforced, but still.

I'm unsure about the use case. What we do in our APIs (no expertise claimed, just an example) is always return default 422 on input validation error, and use abort to exit with a specific code, either from the view function or a view decorator (authentication,...). We never had to pass a status_code from an input schema validator.

It seems people rely on this so even if I'm right about the feature being smelly, I won't insist on removing it. It's not such a burden. I just thought it would be worth mentioning.

[lafrech]

In my cleanup frenzy, I'm still leaning towards removing this attribute.

[lafrech]

I submitted the change to webargs. If it is accepted, I think we're fine removing this here.

[sloria]

I agree with the rationale here. error_kwargs was added to support marshmallow-code/webargs#85 and, as I said in marshmallow-code/webargs#327 (comment) , it was the wrong solution.

Once status_code is removed from webargs, let's move forward with this.