Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Kiota throws ApiException when server returns 304 #4190

Closed
bkoelman opened this issue Feb 18, 2024 · 10 comments · Fixed by #4367
Closed

Kiota throws ApiException when server returns 304 #4190

bkoelman opened this issue Feb 18, 2024 · 10 comments · Fixed by #4367
Assignees
Labels
enhancement New feature or request generator Issues or improvements relater to generation capabilities. WIP
Milestone

Comments

@bkoelman
Copy link

Our server returns HTTP 304 (Not Modified) without a response body when the incoming fingerprint in the If-None-Match header matches the stored version. This enables clients to poll for changes without receiving a response body, unless it has changed since it was last fetched.

Status 304 is described in the OAS document, but Kiota throws an ApiException saying the error isn't mapped:

Microsoft.Kiota.Abstractions.ApiException: The server returned an unexpected status code and no error factory is registered for this code: 304

OAS fragment:

{
  "paths": {
    "/api/people": {
      "get": {
        "tags": [
          "people"
        ],
        "summary": "Retrieves a collection of people.",
        "operationId": "getPersonCollection",
        "responses": {
          "200": {
            "description": "Successfully returns the found people, or an empty array if none were found.",
            "headers": {
              "ETag": {
                "description": "A fingerprint of the HTTP response, which can be used in an If-None-Match header to only fetch changes.",
                "required": true,
                "schema": {
                  "type": "string"
                }
              }
            },
            "content": {
              "application/vnd.api+json": {
                "schema": {
                  "$ref": "#/components/schemas/personCollectionResponseDocument"
                }
              }
            }
          },
          "400": {
            "description": "The query string is invalid.",
            "content": {
              "application/vnd.api+json": {
                "schema": {
                  "$ref": "#/components/schemas/errorResponseDocument"
                }
              }
            }
          },
          "304": {
            "description": "The fingerprint of the HTTP response matches one of the ETags from the incoming If_None_Match header.",
            "headers": {
              "ETag": {
                "description": "A fingerprint of the HTTP response, which can be used in an If-None-Match header to only fetch changes.",
                "required": true,
                "schema": {
                  "type": "string"
                }
              }
            }
          }
        }
      }
    }
  }
}

Generated C# code:

namespace GeneratedClient.Api.People {
    public class PeopleRequestBuilder : BaseRequestBuilder {
        /// <summary>
        /// Retrieves a collection of people.
        /// </summary>
        /// <param name="cancellationToken">Cancellation token to use when cancelling requests</param>
        /// <param name="requestConfiguration">Configuration for the request such as headers, query parameters, and middleware options.</param>
#if NETSTANDARD2_1_OR_GREATER || NETCOREAPP3_1_OR_GREATER
#nullable enable
        public async Task<PersonCollectionResponseDocument?> GetAsync(Action<RequestConfiguration<PeopleRequestBuilderGetQueryParameters>>? requestConfiguration = default, CancellationToken cancellationToken = default) {
#nullable restore
#else
        public async Task<PersonCollectionResponseDocument> GetAsync(Action<RequestConfiguration<PeopleRequestBuilderGetQueryParameters>> requestConfiguration = default, CancellationToken cancellationToken = default) {
#endif
            var requestInfo = ToGetRequestInformation(requestConfiguration);
            var errorMapping = new Dictionary<string, ParsableFactory<IParsable>> {
                {"400", ErrorResponseDocument.CreateFromDiscriminatorValue},
            };
            return await RequestAdapter.SendAsync<PersonCollectionResponseDocument>(requestInfo, PersonCollectionResponseDocument.CreateFromDiscriminatorValue, errorMapping, cancellationToken).ConfigureAwait(false);
        }
    }
}

To work around this, we need to catch an exception to detect HTTP 304 is being returned:

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.");
}

This looks like a bug to me. In KiotaBuilder.cs, I found the following line:

    private static readonly HashSet<string> noContentStatusCodes = new(StringComparer.OrdinalIgnoreCase) { "201", "202", "204", "205" };

Should that just be changed to include 304?

@andrueastman
Copy link
Member

Thanks for raising this @bkoelman

I believe the other issue is the request adapter only considers codes in the 2XX range as success codes and therefore will throw an exception even if we add the value to the noContentStatusCodes collection.
https://github.com/microsoft/kiota-http-dotnet/blob/48cd42f46d4224e9c210fbee2c9f0815b6042241/src/HttpClientRequestAdapter.cs#L385

Would you be willing to submit a PR to update that as well as add the value to the collection?

@bkoelman
Copy link
Author

Hi @andrueastman, thanks for asking. I'm pretty overloaded with work right now, so I wouldn't mind if someone else fixes this.

@andrueastman
Copy link
Member

No worries. Will add this to pool to be worked on.

For clarity, we need to

  • - add 304 to the noContentStatusCodes collection in the KiotaBuilder.
  • - Verify and fix across supported languages that the requestAdapter implementation allow for the returning of the code and not throw an error/exception.

@bkoelman
Copy link
Author

Was this closed unintentionally? From what I understand, the reported bug hasn't been fixed yet.

@baywet
Copy link
Member

baywet commented Mar 22, 2024

It's been fixed on the generation side, now additional changes may be required in the http libraries. (see the linked issues above)

@bkoelman
Copy link
Author

So effectively, the reported bug isn't fixed yet. Can it be reopened? I reported a kiota feature not working, don't really care about internal components. Marking a bug as fixed means we can update and observe the reported problem doesn't happen anymore, which is not the case. Marking bugs as fixed when they really aren't kinda defeats the purpose of bug status tracking by anyone that depends on it.

@baywet
Copy link
Member

baywet commented Mar 22, 2024

I understand the disagreement/confusion this can cause.
Coordinating cross languages efforts has been a challenge since the start of the project. Where we've had more success has been into implementing generation work first, and then breaking things up into smaller issues for each languages.
The alternative, blocking the issue until all languages are fixed, meant that issues stayed open forever, and didn't bring any-more clarity to the team.
Please follow the related dotnet issue to get notified about the progress.

@bkoelman
Copy link
Author

I don't mind breaking it up and have multiple PRs contribute to solving a reported problem. In this case, the .NET part needs to be implemented to resolve the problem reported in this issue, so please reopen. This issue defines a bug from an outside user perspective. Our users see it in the list of limitations and understand how to work around it, based on the provided information here. Pointing them instead to a cryptic issue that only the kiota team understands is not community friendly. Our users will incorrectly infer the reported problem here does no longer occur, because it was closed as completed. Managing issues like that just doesn't make sense.

@baywet
Copy link
Member

baywet commented Mar 22, 2024

Feedback noted around making it harder for the community to understand the state things are in.
Would have it made it easier if I had left a comment here explaining to go follow the follow-up issues when I closed this one?

@bkoelman
Copy link
Author

No. When I'm booking a vacation at a travel agency, which involves booking both a hotel and a flight, will they notify me of a succesful booking when in fact they only booked the flight? Of course not. If I open a support ticket for a Windows bug, and they fixed it in the kernel, but the Win32 API still doesn't call the fixed kernel, so my app still crashes, will they close the ticket as completed? Of course not.

This issue reports a problem when using kiota, the product, not one of its internal components. The described issue isn't fixed, so it should remain open. I've never encountered a project, open or closed source, that uses such a non-intuitive workflow. A real paying customer would never accept such behavior. This again demonstrates the team cares about internals only, not its users. But as you seem unwilling to step out of that bubble, forget about it, I'm done with this silly discussion.

bkoelman added a commit to json-api-dotnet/JsonApiDotNetCore that referenced this issue May 9, 2024
- Generated project files target .NET 6 instead of 8 (going out of support end of this year).
- Generated C# files don't have the "auto-generated" comment on the first line, causing code style warnings.
- Running "liblab build" generates a separate project for the SDK and one for its usage; it should generate a single (combined) project by default for simplicity.
- The inner loop experience is just as awful as kiota: no msbuild integration. What it should be: rebuild server project, which updates openapi.json, which regenerates the sdk (like NSwag does).
  - Would regenerate even work when I'm on a plane? Requiring an internet connection is a huge limitation.
- Generated install.sh doesn't run on Windows, should use PowerShell instead.
- Handling of required/nullable is handled better than kiota does, though not entirely correct (example: return type of PatchPersonAsync() should be nullable).
- Inheritance using allOf with discriminator does not work, ie: TodoItemDataInResponse should inherit from DataInResponse (also oddly named "Type_" property generated).
- Setting "responseHeaders": true in liblab.config.json does not work: generated code remains the same.
- A new HttpClient instance is created each time during construction, which isn't scalable (and doesn't respond to DNS changes when long-lived). Should use HttpClientFactory instead.
- There's no way to inject a DelegatingHandler, for example to log the HTTP requests and responses.
- When sending a request, the user agent is set to "dotnet/7.0", looks like a bug.
- Posting with a request body requires nasty code; should use "required init" properties instead of constructor parameters. Also the parameter order from openapi.json isn't respected.
- There's no way to set the Content-Type when sending a request body.
- Incorrect generated Type for "meta" (should be IDictionary<string, object?>? instead of object?)
- There doesn't seem to be a way to intercept serialization to support partial patch (ie: sending "firstName: null" clears the value, whereas omitting the property doesn't change it).
  - NSwag allows to inject custom serializer options, where we add a custom JsonConverter. kiota handles it more elegantly by tracking property assignments.
- Generated "response.EnsureSuccessStatusCode()" isn't correct: it fails when HTTP 304 is returned (similar to microsoft/kiota#4190).
- Crash in response deserialization when HTTP 204 is returned with empty response body.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request generator Issues or improvements relater to generation capabilities. WIP
Projects
Archived in project
Development

Successfully merging a pull request may close this issue.

3 participants