Add functionality to catch many upload problems at once and return them all

[JackWilb]

Right now this just deals with the csv uploader. Looks like some of this work overlaps with @AlmightyYakob but expands functionality to catch many errors in one pass and to catch decoding errors. (closes #101)

[jjnesbitt]

My other PR only contains one change, which yours also contains, so I'll close that one. For clarity, I'll just re-iterate that this deals with the backend side of #68, but doesn't add the client component. Also, there's more that can be done to validate the CSV upload, but this is a start. That can probably be done separate from this.

[JackWilb]

The next step for me is to implement some json testing but that's tricky without examples. Are there existing examples, @waxlamp, or will I have to generate some?

[waxlamp]

What do mean by "json testing"?

[JackWilb]

Sorry, that wasn't clear. I meant json upload validation, i.e. verifying the structure is correct (like we've done with CSV).

[JackWilb]

There is currently no validation for the nested json file structure so I've opened a new issue for that (#159).

[JackWilb]

@AlmightyYakob making those changes has broken the validation tests since they now don't return the errors, they raise the error instead. Any idea how we can handle this? Importing unittest? Or return the errors and then raise the error in the upload methods

[waxlamp]

I think you can have pytest assert that such-and-such exception was raised.

[JackWilb]

Ready for review @AlmightyYakob @waxlamp

[waxlamp]

I think you could also probably catch the exception yourself, then run the asserts in the exception handler.

[JackWilb]

Is that a preferable solution compared to what I just pushed?
b7941f7

[jjnesbitt]

I think the preferred solution is something like:

with pytest.raises(ValidationFailed) as v_error:
    validate_csv(rows)

validation_resp = v_error.value.errors
...etc...

Pytest docs regarding this. FWIW I've testing this method locally and it does work, and still allows us to make those assertions.

[waxlamp]

@AlmightyYakob, would the validation_resp line go inside the scope of the with block?

In any case, that pattern gets us the best of both worlds: you can assert that the function raises an exception, and you can examine the structure of that exception too.

[JackWilb]

Yeah I like it. Can you review my last commit and see if that aligns better with the new tests

[jjnesbitt]

@AlmightyYakob, would the validation_resp line go inside the scope of the with block?

In any case, that pattern gets us the best of both worlds: you can assert that the function raises an exception, and you can examine the structure of that exception too.

I think you may be able to, but it's not necessary. I prefer to not include that line in the scope, because the only line that would raise the exception is the call to validate_csv.

[waxlamp]

I think you may be able to, but it's not necessary. I prefer to not include that line in the scope, because the only line that would raise the exception is the call to validate_csv.

Ah so the block just converts a try-except into a linear flow, neat.

[waxlamp]

Can you review my last commit and see if that aligns better with the new tests

Looks right to me!

[jjnesbitt]

Ah so the block just converts a try-except into a linear flow, neat.

Yes, and also will fail the test if the exception is not raised, which is very handy.

[waxlamp]

Looks great overall!

[waxlamp]

I think the error should read "utf8", and the detail should get whatever error message or error data UnicodeDecodeError provides.

[waxlamp]

Sorry, I should have caught this earlier, but DecodeFailed ought to just receive a single argument value, rather than a list, and that value should just be the string value of the UnicodeDecodeError object.

So I would change this line to take in str(e), and change the definition of DecodeFailed to reflect that it's expecting just a string to describe how the decode failed.

[JackWilb]

I agree that this is probably best for this one function but since ValidationFailed returns {"errors": errors} maybe we should be consistent? This would allow us to do just one check from the client.

[waxlamp]

I don't think there's any reason for DecodeFailed to be made consistent with ValidationFailed -- they are different kinds of error conditions. DecodeFailed is really just our variant of UnicodeDecodeError, which does signal a single error, so we should be consistent with that.

[waxlamp]

Oh I see.

I think ValidationError should also lose the "errors" dict format. Let's change both.

[JackWilb]

The problem with that solution is that ValidationError can return multiple errors. How should we update that to handle the multiple errors?

[waxlamp]

It already handles multiple errors properly; the change here is in how it reports those errors. It can just return the JSONified string representing the list of errors, instead of the JSON object with the errors key that it currently uses.