Adjust serializer selection fallback procedure

[softworkz]

Summary:

1. Choose by request type only when the server didn't provide a content type and it wasn't possible to detect it
2. If the server did provide a content type but we don't have a deserializer for it, try detection as a fallback

Rationale

If we do have some indication about the content type, either from the response header or from detection, then there's no reason to ignore that information, just because we have no deserializer for it available. If we don't have a matching one for the given situation and parameters, then that's a fact, but it's logically not valid to choose a deserializer by request type.

Now, one might argue that servers are sometimes sending invcorrect headers, which is true of course, but for the case of a possibly (we don't know, we only know that we have no deserializer for it) incorrect header, we can use a better fallback: we can try to detect - which is currently only happening when there's no content tpye indicated by the server.
Doing this as a fallback makes more sense, because when our assumption (built on thin ice) would be true, we should at least get a different detection result than what the server indicates.
If that's not the case, then we can surely stop, because the probabiliy of 2 chances failing coincidently is near zero.
Simple word example: when the server says it's not Json and our detection doesn't say it's Json, then there's really no point in assuming that it's Json - even when the RequestType was Json.

So, using the RequestType for selection when we already have contradicting evidence is not quite valid. But there is in fact a case where it makes sense to use it as fallback, and that's cases where we have nothing at all: No server header and no result from detection. When we know nothing at all, then the RequestType will be our best bet.

This PR is re-ordering the selection logic according to the above.

Please let me know what you think.

[softworkz]

@dotnet-policy-service agree

[alexeyzimarev]

Looks good 👍

[alexeyzimarev]

Adding a couple of tests would be nice. Mock a response with an incorrect content type, and see if it will work.

[softworkz]

Adding a couple of tests would be nice. Mock a response with an incorrect content type, and see if it will work.

The case how I came to it was this:

• I have implemented a custom serializer (using ServiceStack.Text) which throws on invalid Json
  (I want it to to throw because swallowing errors makes it hard to trace back problems)
• I'm calling an API (using Json for all requests and responses) via .ExecuteAsync<T>()
• In case of an error, the API returns an error message with content type 'text/html' or 'text/plain'
• The current implementation falls back to using a Json Deserializer (because of the RequestType being Json) which causes an exception when deserializing
• When I call RestResponse.ThrowIfError() then, it throws the deserialization exception instead of the expected exception showing the actual error from the server

[softworkz]

Alternative approaches I was considering:

1. Disable fallbacks for deserializer selection in case of HTTP errors

This was the initial idea for changing GetContentDeserializer() - to be more strict in case of HTTP errors and choose a serializer only based on the content type indicated by the server without detection and fallbacks.

Eventually, I realized that the applied logic is not ideal, even in non-error cases, which is why I preferred the general change of behavior proposed by this PR.

2. Change handling of deserialization exceptions In case of HTTP errors

I think it is correct to deserialize content (if present) even in case of HTTP errors.

The behavior for throwing exceptions, which is controlled by ThrowOnAnyError, FailOnDeserializationError and ThrowOnDeserializationError makes sense to me as well - in general (see RestSerializers.Deserialize<T>()).

What I'm wondering though, is whether it might be better to implement a slightly different handling in case of HTTP errors, where the HTTP error is typically the primary subject of interest.

Which means, in case of HTTP errors

• The deserialization error should not be set on the response (with RestResponse.AddException()), because it hides the original error
• When the user has set ThrowOnAnyError, then the HTTP error should be thrown, not a deserialization error
• When the user has set ThrowOnDeserializationError - I'm not sure about that case...

Concluding

I'm not sure what you think - #2 might still make sense, but for a first-time contribution, I wanted to keep this PR atomic and focused.

[alexeyzimarev]

Often when the server returns an error, it also returns something like problem details response. The issue atm is there's no de-factor standard for such responses. We can say "ok, let's assume it's problem details". Well, basically, problem detail response has a custom content type, so it might make sense to add a special path for that, should be relatively easy to do. If you get an error from something like Express, it has its own, quite well-know JSON format, but it has no definition as a standard. I was thinking a while ago how to make the client to expect one type for success and another for errors, but generic type constraints in C# make the code quite verbose. That's the reason I made content deserialisation function public, so one can check if the request was not successful (status code) and try deserialising content as an error.

If I understand you correctly, in brief, if there's a server error, we should keep trying to deserialise the response content, but we can obviously expect that the JSON response is either missing, or it has a different schema (like problem details). So, deserialisation will most probably fail, but the user gets the deserialisation error instead of the actual error. Changing ti makes total sense to me.

I'd say we can do the following:

• Implement your (2) suggestion
• Add problem detail deserialisation based on content type detection

I can suggest making the first part done in this PR, and open an issue for the second part, address it in a different PR

[softworkz]

Often when the server returns an error, it also returns something like problem details response. The issue atm is there's no de-factor standard for such responses.

If you mean by "de-facto standard" that every API is behaving differently, then that is true. But the OpenAPI standard covers this pretty well and explicitly leaves room for all kinds of different behavior. With OpenAPI, you can define a different response schema (model) for different HTTP status codes - which can also be a plain string.
(see https://swagger.io/specification/#responses-object)

That's the reason I made content deserialisation function public, so one can check if the request was not successful (status code) and try deserialising content as an error.

Yes, I think that makes the most sense.

If I understand you correctly, in brief, if there's a server error, we should keep trying to deserialise the response content, but we can obviously expect that the JSON response is either missing, or it has a different schema (like problem details)

I think this PR makes the most sense in the first place..

And on top of that we could modify the behavior in error case in the following ways:

• Don't use the request type as a fallback for deserializer selection
• When there's no response type in the response headers, don't even try to deserialize
  (or maybe provide an option: "Never try to deserialize in case of error")
• When deserialization fails, don't set the (deserializer-)exception on the response object
  (so it doesn't overwrite the exception from the failed operation)