Generated F# client throws NullReferenceException if the server returns an empty response

[Numpsy]

Hi,

I fell over this by accident when I pointed the client at the wrong server endpoint - if the server doesn't return any response content (for example, because it has returned a 404 status), then I get a NullReferenceException thrown from

Snowflaqe/src/CodeGen.fs

Line 1088 in 2da41cf

let response = responseJson.ToObject<GraphqlErrorResponse>(JsonSerializer.Create(settings))

because responseJson is null.

[Zaid-Ajaj]

I fell over this by accident when I pointed the client at the wrong server endpoint

Not sure how to fix this exactly, I can return or throw an empty error but I don't think Snowflaqe should start doing introspection to make sure this is indeed a GraphQL endpoint 🤔 any ideas? I kinda assume / expects to know this URL since it is the same one / same host used in the configuration

[Numpsy]

It was setup error when I was testing, but I think it won't always be the same host as in the config (I mean as in, I run the generator against a dev system to generate the client, but the client gets used against a server that's elsewhere)... it's still a config error, but I just thought that NullReferenceException wasn't very 'friendly'/explanitory.

I actually tried to see if the same thing can occur with other errors like authorization failures, and got a case where IIS returned a 401 status with a HTML error page as content (which Newtonsoft unsurprisingly throws on when asked to convert), and as callers might have to deal with HttpClient throwing HttpRequestException as well, it might just be a case of 'it's going to throw' and not a particular problem.

And now I'm not sure how much of an issue it really is - might depend on how you decide to do #29 and if it should always use its own exception type in error cases? (error handling, what a pain :-()

[Zaid-Ajaj]

maybe we can search the response headers and looks for content-type, then throw an exception (regardless of #29) if it is anything but application/json (or if it is text/html etc.)

[Numpsy]

That sounds reasonable (I'm not sure if i'm overcomplicating things after spending all together too much time lately on error handling related stuff, it just seems 'unfortunate' when things like serialization library specific exceptions escape (e.g. things that might differ depending on if you use Newtonsoft or System.Text.Json or whatever).

On a sort of related note - I saw your Twitter post about unit testing generated Http clients. I haven't got as far as trying that with Snowflaqe but FWIW I have some tests for existing generated C# REST/OpenApi clients that are created by NSwag, which use a combination of interfaces (Generated by NSwag) and https://github.com/richardszalay/mockhttp, so that may one thing that i'd try

[Zaid-Ajaj]

it just seems 'unfortunate' when things like serialization library specific exceptions escape

Using the current implementation shouldn't result in any JSON deserialization errors. That is when the response is actually JSON 😂

I have some tests for existing generated C# REST/OpenApi clients that are created by NSwag, which use a combination of interfaces

I am thinking of using a mocking library at the end but I don't want to mock methods at the HTTP level and return JSON responses (what MockHTTP does) because the generated client deals with typed inputs and responses.

Consider this piece of code that uses the generated client:

let process (client: GraphqlClient) : int = 
  match client.GetUser() with
  | Some user -> 
      match client.GetData { user = user.Id } with
      | Some data -> 42
      | _ -> -1
  | _ -> -1

this process function takes the GraphQL client as input and I you as a user want to test it. Then without refactoring the code, you should be able to do the following (to be implemented):

let interceptor = GraphqlInterceptor()
// provide here the test responses
interceptor.GetUser(fun () -> Result.Ok testUser)
interceptor.GetData(fun input -> Result.Ok testData)
// create a client that uses the interceptor
let testClient = interceptor.BuildClient()
// act
let output  = process testClient 
Expect.equal output 42 "The output is correct" 

Of course, the GraphqlInterceptor would be a class that is also generated that uses a mocking library under the hood.

This is all to be implemented of course. What do you think about this approach? 🤔

[Numpsy]

Seems reasonable to abstract mocking libraries and such away.

For comparison, the generator i'm using for C# Rest clients can generate things like

class ApiClient : IApiClient
{
    public ApiClient(HttpClient httpClient)
    {
    }
}

and a lot of the callers use the interfaces and are tested with alternate interface implementations (created manually using Moq or just test specific classes).

There are a few cases (not that many) that use HttpClients set up to return custom JSON data, which can have issues of it's own (e.g. the generated classes have attributes to control things so you have to match its assumptions, and then you can end up testing the Json lib rather than the client which isn't much use).

There are other tests that use HttpClients to test how things respond to different or unexpected Http Response codes, do things with query strings, things like that. Maybe that sort of thing is less useful for GraphQl though where there is one endpoint and a more highly specified interface? (things like the web server jumping in and returning HTML where you should only have JSON aside!)

[Numpsy]

Not sure how useful that is, but: Something simple built in to do general cases sounds nice, and nothing there prevents anything more specialised being done if it happens to be needed.

[Yakimych]

Hello folks! We ran into this during a mob session the other day, and it wasn't trivial to understand the issue. Even debugging generated code did not give any hints (due to the function calls being async). The issue was that we're generating the client off a statically build schema file, and not by introspecting a running an endpoint. In any case, it was as simple as "forgetting" /graphql at the end (so http://localhost:5000 instead of http://localhost:5000/graphql) to get this NullReferenceException. Maybe we can check for 2xx status code anyway?

[Zaid-Ajaj]

Maybe we can check for 2xx status code anyway?

@Yakimych I think this is not sufficient to check for the error because non-2xx status codes are still valid GraphQL responses. I don't know how exactly to "fix" this. It is a matter of configuring the tool correctly.

[Yakimych]

Sure, I understand the problem from this perspective.

It is a matter of configuring the tool correctly.

I don't think this is fair, however. The tool may be configured correctly for generating code statically, but in a real-world scenario the generated code may be run against many different deployed versions of the same API (master, feature branches, staging, live, etc.). Introspecting all of those might not always be possible (introspection disabled in Live, for example). We use it for tests btw.

I think this is not sufficient to check for the error because non-2xx status codes are still valid GraphQL responses.

Yes, definitely. It might still be helpful though - better than a generic NullReferenceException for sure. Even something like wrapping the exception with our own "Please check that the runtime URL is set correctly" message might save some time debugging the issue runtime.

In any case, just thought I would chime in with some practical experiences. Just thinking ahead - unfortunately achieving the equivalent of "type safety" in the DevOps world is not really feasible, so misconfiguring CI/CD pipelines and deployment scripts will happen. And when it does, there is a huge difference between getting a NullReferenceException from a failed test, and e.g. Double-check that this endpoint responds as expected: {endpoint} message.

Otherwise, I don't have any good suggestion for a "fix" either, so maybe we can consider closing this issue in that case?