openapi: include headers

[verdie-g]

Closes #1367

QUALITY CHECKLIST

• Changes implemented in code
• Complies with our contributing guidelines
• Adapted tests
• Documentation updated

[codecov]

Codecov Report

Attention: Patch coverage is 97.14286% with 3 lines in your changes are missing coverage. Please review.

Project coverage is 90.75%. Comparing base (69d91d6) to head (a9f15f5).

Files	Patch %	Lines
...rc/JsonApiDotNetCore.OpenApi.Client/ApiResponse.cs	82.35%	3 Missing ⚠️

Additional details and impacted files

@@             Coverage Diff             @@
##           openapi    #1459      +/-   ##
===========================================
+ Coverage    90.69%   90.75%   +0.05%     
===========================================
  Files          390      390              
  Lines        12784    12883      +99     
  Branches      2044     2044              
===========================================
+ Hits         11595    11692      +97     
- Misses         774      776       +2     
  Partials       415      415              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[verdie-g]

This change does make the headers appear in the OpenAPI document but the swagger generation remains the same. The reason is that the methods of the generated clients only return the deserialized body. I think an option must be enabled to actually return an object with status code and headers (RicoSuter/NSwag#3557 (comment)). Though, I would say it's out of the scope of this PR.

[bkoelman]

Thanks. I'm out for the rest of this week, will take a look when I get back.

Please do add anything related to headers in this PR. Maurits probably missed the one you mentioned when the issue was created.

Can you check if the headers are available in Kiota? There's a branch with sample. And can you try setting up NSwag server to see what it would look like? I wonder if required has any impact on the nullability of the generated client (see Options in test project to activate NRT). Perhaps we could/should have an end-to-end test, depending on the outcome of these investigations.

[verdie-g]

An NSwag method looks like that

The argument name if_None_Match is not great but what's even less great are these lines

if (status_ == 304)
{
    string responseText_ = ( response_.Content == null ) ? string.Empty : await response_.Content.ReadAsStringAsync().ConfigureAwait(false);
    throw new ApiException("The resource was not modified.", status_, responseText_, headers_, null);
}

It throws on 403. Though, I'm not sure what would be a better behavior.

[bkoelman]

The argument name if_None_Match is not great

Can we use RicoSuter/NSwag#808 somehow?

It throws on 403

We have https://github.com/json-api-dotnet/JsonApiDotNetCore/blob/openapi/src/JsonApiDotNetCore.OpenApi.Client/ApiResponse.cs for that; probably needs to be adapted to handle 403 as well.

I've clarified the description in the original issue. For NSwag, we may even provide our own ResponseClass (pass /GenerateResponseClasses:false) if that smoothens the caching experience. For Kiota, see https://learn.microsoft.com/en-us/openapi/kiota/query-parameters-request-headers#request-headers and microsoft/kiota#2963.

[verdie-g]

Can we use RicoSuter/NSwag#808 somehow?

Probably but I'm not sure we can use by just passing a command line option so maybe that will fall outside of the scope of the PR.

Also I'm so confused about how clients are generated.

This code clones the repo, makes a change and can't find that change in the generate clients. What am I missing?

git clone git@github.com:json-api-dotnet/JsonApiDotNetCore.git test
cd test
git checkout openapi
dotnet build

sed -i 's/"query"/"hello"/' src/JsonApiDotNetCore.OpenApi/SwaggerComponents/JsonApiOperationDocumentationFilter.cs
dotnet build

grep hello test/OpenApiClientTests/obj/OpenApiClient.cs # nothing

dotnet clean
dotnet build

grep hello test/OpenApiClientTests/obj/OpenApiClient.cs # nothing

git clean -fdx
dotnet clean
dotnet build

grep hello test/OpenApiClientTests/obj/OpenApiClient.cs # nothing
grep hello test/OpenApiEndToEndTests/obj/QueryStringsClient.cs # nothing

[bkoelman]

Can we use RicoSuter/NSwag#808 somehow?

Probably but I'm not sure we can use by just passing a command line option so maybe that will fall outside of the scope of the PR.

I don't understand, can you clarify?

Also I'm so confused about how clients are generated.

When building project OpenApiTests, your changes from JsonApiDotNetCore.OpenApi are compiled. When running the tests in OpenApiTests, it writes updated swagger.json into the OpenApiClientTests and/or OpenApiEndToEndTests test project (for example, here). When adding Kiota support, I'd like to change that to output into the server-test project itself, so that multiple clients can reference it.

When building JsonApiDotNetCoreExample, it starts the webserver and writes GeneratedSwagger/JsonApiDotNetCoreExample.json (which is consumed by JsonApiDotNetCoreExampleClient) because of this line. Turn it off when the JsonApiDotNetCore.OpenApi produces something broken, so you can run the example project and debug it.

[bkoelman]

Also, NSwag doesn't always detect changes properly. For example, if project options have changed but the source swagger.json has not, it won't regenerate. Delete the obj directory in the client project to workaround that.

[verdie-g]

Delete the obj directory in the client project to workaround that.

Oh that's it thanks.

We have https://github.com/json-api-dotnet/JsonApiDotNetCore/blob/openapi/src/JsonApiDotNetCore.OpenApi.Client/ApiResponse.cs for that; probably needs to be adapted to handle 403 as well.

Though I'm not sure what to return in that case. I've generated a response object to be able to get the response headers (e.g. ETag) so maybe it could return { status: 403, headers: /* ... */, data: null } and the user should know that if status is 403 then data will be null.

I don't understand, can you clarify?

NSwag clients are generated using a command line from the csproj. How can I pass an implementation of IParameterNameGenerator there 🤔 I think a custom generator needs to be created like in here https://github.com/daveaglick/NetlifySharp/blob/main/tools/NetlifySharp.Generator/Program.cs.

[bkoelman]

@verdie-g Looking at your last commit message, you may have faced the same mystery as I did this week. See #1463.

[verdie-g]

Oh actually I think I just didn't know anymore what I changed so I wrote some random message.

[bkoelman]

Thanks for looking into all these details. You're on the right track. Sorry if this turned out more complicated than I initially imagined, I appreciate you're not giving up.

I don't understand, can you clarify?

NSwag clients are generated using a command line from the csproj. How can I pass an implementation of IParameterNameGenerator there 🤔 I think a custom generator needs to be created like in here https://github.com/daveaglick/NetlifySharp/blob/main/tools/NetlifySharp.Generator/Program.cs.

Thanks, I understand now. It goes a bit too far to build our own generator for this, so let's just stick with if_None_Match. Feel free to open an issue in the NSwag repo with a request to add a switch to adhere to the C# naming conventions.

We have https://github.com/json-api-dotnet/JsonApiDotNetCore/blob/openapi/src/JsonApiDotNetCore.OpenApi.Client/ApiResponse.cs for that; probably needs to be adapted to handle 403 as well.

Though I'm not sure what to return in that case. I've generated a response object to be able to get the response headers (e.g. ETag) so maybe it could return { status: 403, headers: /* ... */, data: null } and the user should know that if status is 403 then data will be null.

I propose to extend the ApiResponse class in JADNC.OpenApi.Client project in the following way:

• Add classes ApiResponse and ApiResponse<>, similar to what NSwag generates. I don't think the name JsonApiResponse is appropriate, as it contains nothing specific to JSON:API.
• Extend existing methods to respond to 304 and return null.
• Add new methods (for returning void and T) for wrapped responses (a user may choose to use them or not), ie:

  public static async Task<ApiResponse<T?>> TranslateWrappedAsync<T>(Func<Task<ApiResponse<T>>> operation)
      where T : class
  {
      try
      {
          ApiResponse<T> response = await operation();
          return new ApiResponse<T?>(response.StatusCode, response.Headers, response.Result);
      }
      catch (ApiException exception) when (exception.StatusCode == (int)HttpStatusCode.NotModified)
      {
          return new ApiResponse<T?>(exception.StatusCode, exception.Headers, null);
      }
  }

• Move ApiException outside of the .Exceptions namespace, so we'll only need a single /AdditionalNamespaceUsages switch.

[bkoelman]

Aside from comments, remaining things to address:

• openapi: include headers #1459 (comment)
• End-to-end tests (subset of what's in ETagTests.cs)
• Possibly update sample to show how ETag works, ie: call wrapped GetPersonCollectionAsync, capture ETag from header, call again with that ETag, and observe the result is null with status 304.
• Document the (optional) use of /WrapResponses:true /GenerateResponseClasses:false in docs/usage/openapi.md
• Try if it works with Kiota (no commit needed, but would be nice if you can add some working code in a PR comment so I can integrate it later)

[verdie-g]

Document the (optional) use of /WrapResponses:true /GenerateResponseClasses:false in docs/usage/openapi.md

Hmm I was expecting to see a NSwag section there, not sure what to add.

Try if it works with Kiota (no commit needed, but would be nice if you can add some working code in a PR comment so I can integrate it later)

I don't think I'll have the time/energy to test with Kiota. Though this library looks promising.

[bkoelman]

Please update this PR to the latest version of the base branch.

[bkoelman]

For future reference, here's what the sample looks like using Kiota:

var headerInspector = new HeadersInspectionHandlerOption
{
    InspectResponseHeaders = true
};

PersonCollectionResponseDocument? getResponse1 = await client.Api.People.GetAsync(
    configuration => configuration.Options.Add(headerInspector));

string eTag = headerInspector.ResponseHeaders["ETag"].Single();

try
{
    PersonCollectionResponseDocument? getResponse2 = await client.Api.People.GetAsync(
        configuration => configuration.Headers.Add("If-None-Match", eTag));
}
catch (ApiException exception) when (exception.ResponseStatusCode == (int)HttpStatusCode.NotModified)
{
    Console.WriteLine("The HTTP response hasn't changed, so no response body was returned.");
}

The try/catch is needed due to a bug in Kiota: it fails to map the 304 status, despite being in the OAS.

[bkoelman]

Please add HttpStatusCode.NotModified to the appropriate entries in OpenApiEndpointConvention.GetSuccessStatusCodesForEndpoint, so that status codes are ordered alphabetically in the OAS document.

[bkoelman]

Final round of review. Thanks for all the hard work!
I'll propose something to add to the docs; with that included, this looks ready for merge.
Meanwhile, can you update the PR description, with an updated SwaggerUI screenshot?

[verdie-g]

I'm not committing the suggestions one by one ever again :D

[bkoelman]

I'm not committing the suggestions one by one ever again :D

I'd welcome that. You can batch them into a single commit from the Files changed tab.

I've created a suggestion to update the docs, see the diff at openapi...openapi-header-docs. Probably just easiest for you to cherry-pick the commit, assuming you agree with the change.

[verdie-g]

You can batch them into a single commit from the Files changed tab.

Good to know.

Probably just easiest for you to cherry-pick the commit

Done

[bkoelman]

See comments