Remove MultipleErrors

[beckyconning]

Don't merge me until SlamData is updated please : )

[kritzcreek]

The MultipleErrors constructor is something the backend explicitly sends. If we now treat every error from Quasar as if there are multiple ones, that will introduce a lot of dead code.

What is the motivation for this?

[cryogenian]

Removing interleaving concepts I think. Why should we preserve 1-1 mapping with quasar JSON if it's possible to separate?

[kritzcreek]

What interleaving concepts? It's called a QError so I'd expect it be an error from Quasar. Initially I thought you were going to change | MultipleErrors (Array QError) into | MultipleErrors (NEList QError) which made sense to me. Afaik the MultipleErrors constructor only appears in the compilation errors though, so you're going to be handling them everywhere now, when they only rarely occur.

[beckyconning]

regarding "handling them everywhere" its not such a horrible thing given all the nice instances that come with NonEmptyList.

but for now i'll just add foldable. i kinda get both arguments but i prefer NonEmptyList QError for reasons i might want to try to explain when i'm less busy : )

[cryogenian]

What interleaving concepts?

• Error type: Forbidden and PaymentRequired is explanationary tag

• Error count: MultipleErrors is aggregational constructor

• Encoded JSON with those errors

• PS representation

Frankly my main concern is about MultipleErrors [MultipleErrors [MultipleErrors ... such thing has no sense. All that NEL.singleton ErrorCstr could use smart constructors errorCstr = NEL.singleton ErrorCstr and handling could use folds everywhere.

[kritzcreek]

See this in the context that I have the task to refine the way we communicate errors with Quasar together with JR. https://github.com/slamdata/slamdata/issues/1457

Error type: Forbidden and PaymentRequired is explanationary tag

That's not going away because we make all requests return a NonEmptyList of errors.

Error count: MultipleErrors is aggregational constructor

It maps exactly to an error Quasar returns. Just treating every request as if it returns multiple errors is just lying to ourselves. This data type is not something we construct, it is created by Quasar and we parse it.

Encoded JSON with those errors

The fact that we stringify and then wrap up Quasar errors in JavaScripts Error constructor is ridiculous, but that's not going away because we make all requests return a NonEmptyList of errors. That's a problem with the extremely String heavy error handling here.

Frankly my main concern is about MultipleErrors [MultipleErrors [MultipleErrors ... such thing has no sense.

Again, we're not the ones constructing these. And if Quasar does something silly and produces these kinds of errors we need to be able to handle them.

What we definitely need to do is to erase less information, so that we have the relevant information available when we present errors or suggestions to the user. (For example reducing every 404 into a nullary constructor needs to be changed. The backend uses the message body to communicate relevant information with us, see: https://github.com/slamdata/slamdata-backend/pull/241#discussion_r141148214

Which instance do you get by wrapping the QError datatype in a NonEmptyList that you couldn't use before?

[garyb]

I'm with @kRITZCREEK on this one - the representation not matching what Quasar gives us could be a bit of a problem, as it might well send us MultipleErrors inside MultipleErrors, etc. so trying to avoid that just gives us a new problem of having to somehow flatten that, but that would then lose the information that comes with the containing error.

I think the idea was we were going to work with the backend team to clean up the error representations anyway, and introduce tags and whatnot to make it easier to get semantic information about the errors that we get - so we may as well wait and see what comes of that before doing anything drastic to the representation here.

[garyb]

I'm still not too clear on what the motivation for this is either, aside from maybe a tech-debt-y kind of thing - I assume there was some specific thing that spawned the change rather than just wanting to clean it up a bit, since it's not that trivial a change.

[cryogenian]

If quasar can return MultipleErrors [ MultipleErrors [ then it can return MutltipleErrors [] too. 😞

This means that the only thing we can do is Foldable instance.

[beckyconning]

Basically I wanted foldable but MultipleError felt a bit weird and I noticed some slightly odd edge cases so I thought it should probs be NEL instead. Foldable works just as well for the specific use case so lets close this.

[kritzcreek]

What do you want to fold over? QError has the wrong kind for a Foldable instance.

EDIT:

Maybe a helper function to turn QError into Array QError gives you what you need?