Serialization error causing 400 Bad Request with GitHub API

kfcampbell commented 3 months ago

What are you generating using Kiota, clients or plugins?

API Client/SDK

In what context or format are you using Kiota?

Nuget tool

Client library/SDK language

Go

Describe the bug

I am trying to call client.Repos().ByOwnerId("my-owner-id").ByRepoId("my-repo-id").Properties().Values().Patch(context.Background(), patchRequestBody, nil) with Kiota version 1.14.0+fc4b39c65d89f7bfc8c7f1813c197e95e206da09.

Alternately, this can be reproduced using octokit/go-sdk@v0.0.21.

The serialized payload going out looks like:

"{\"properties\":[{\"property_name\":\"environment\",\"value\":\"test-from-go-sdk\"}]}"

and returns a 400 Bad Request error.

Expected behavior

I expect a 200 to come back from the API. Using the equivalent curl request:

curl -L   -X PATCH   -H "Accept: application/vnd.github+json"   -H "Authorization: Bearer $GITHUB_TOKEN"   -H "X-GitHub-Api-Version: 2022-11-28" https://api.github.com/repos/$OWNER_ID/$REPO_ID/properties/values -d '{"properties":[{"property_name":"environment","value":"test-from-curl"}]}'

The request is successful, no output is returned, and repository custom properties are updated.

How to reproduce

Follow this guide to set a custom property for your organization.
Ensure that the above curl snippet works successfully to set it for your repository, and you can view the property changing in the UI in repository settings --> repository sidebar --> custom properties
Clone octokit/go-sdk and replace the contents of cmd/token-example/main.go with the following:

package main

import (
    "context"
    "log"
        "os"
    "time"

    "github.com/octokit/go-sdk/pkg"
    "github.com/octokit/go-sdk/pkg/github/models"
    "github.com/octokit/go-sdk/pkg/github/repos"
)

func main() {
    client, err := pkg.NewApiClient(
        pkg.WithUserAgent("my-user-agent"),
        pkg.WithRequestTimeout(5*time.Second),
        pkg.WithBaseUrl("https://api.github.com"),
        pkg.WithTokenAuthentication(os.Getenv("GITHUB_TOKEN")),
    )

    if err != nil {
        log.Fatalf("error creating client: %v", err)
    }

    customPropertyValue := models.NewCustomPropertyValue()
    propertyName := "environment"
    propertyValue := "test-from-go-sdk"

    customPropertyValue.SetPropertyName(&propertyName)
    value := models.NewCustomPropertyValue_CustomPropertyValue_value()
    value.SetString(&propertyValue)
    customPropertyValue.SetValue(value)

    patchRequestBody := repos.NewItemItemPropertiesValuesPatchRequestBody()
    patchRequestBody.SetProperties([]models.CustomPropertyValueable{customPropertyValue})

    err = client.Repos().ByOwnerId("kfcampbell-terraform-provider").ByRepoId("tf-acc-test-destroy-trfiz").
        Properties().Values().Patch(context.Background(), patchRequestBody, nil)
    if err != nil {
        log.Fatalf("error setting custom properties: %v", err)
    }
}

Replace the OwnerId and RepoId values with your organization and repository values. Export a GITHUB_TOKEN with appropriate permissions.
Run go run cmd/token-example/main.go and see the request return a 400 error. Debug for more information.

Open API description file

https://raw.githubusercontent.com/github/rest-api-description/main/descriptions/api.github.com/api.github.com.json

Kiota Version

1.14.0+fc4b39c65d89f7bfc8c7f1813c197e95e206da09

Latest Kiota version known to work for scenario above?(Not required)

No response

Known Workarounds

N/A

Configuration

N/A

Debug output

N/A

Other information

It might be a red herring, but the request that Kiota sends out is:

"{\"properties\":[{\"property_name\":\"environment\",\"value\":\"test-from-go-sdk\"}]}"

and a successful request in curl looks like:

'{"properties":[{"property_name":"environment","value":"test-from-curl"}]}'

I'm not sure if the quotes and/or backslashes make a difference when serializing/sending the information to the API, but it's worth thinking about.

baywet commented 3 months ago

Thank you for reporting this issue . I am not sure I understand what's wrong here. Are you saying that the double quotes are getting escaped in the payload before it's sent?

kfcampbell commented 3 months ago

Well, I'm not positive, but I think that may be the problem. I could be leading you on a bit of a wild goose chase here, but let's talk about it. Pasting the Kiota-given payload (with escapes) into the otherwise successful curl request, like:

 sh$ curl -L -v  -X PATCH   -H "Accept: application/vnd.github+json"   -H "Authorization: Bearer $GITHUB_TOKEN"   -H "X-GitHub-Api-Version: 2022-11-28" https://api.github.com/repos/$OWNER_ID/$REPO_ID/properties/values -d '"{\"properties\":[{\"property_name\":\"environment\",\"value\":\"test-from-go-sdk\"}]}'

gives a 422 instead of the 400 I'm seeing through the SDK:

{
  "message": "Invalid request.\n\nInvalid input: data cannot be null.",
  "documentation_url": "https://docs.github.com/rest/repos/custom-properties#create-or-update-custom-property-values-for-a-repository",
  "status": "422"
}

Which is not intuitive to me: I'd expect to see a 400 here like we're seeing in the SDK. However, simply removing all the backslashes from the curl command makes the request 204 as we'd expect.

I'm unable to do the reverse (remove the backslashes from the outgoing request while debugging the SDK) as the kiota-abstractions-go.RequestInformation type doesn't seem to allow editing in the debugger:

Do you have thoughts on how I might troubleshoot this further?

baywet commented 3 months ago

Maybe this is something you could use the dev proxy for. Although I couldn't find an example in the documentation to instruct it to simply log request bodies @waldekmastykarz

Alternatively you could use ngrok as a proxy, and leverage the console page at http://127.0.0.1:4040/ to observe the request bodies.

baywet commented 3 months ago

I'm not saying the presence of backslashes is impossible, but I'd be surprised for it to be the case since we have thousands of applications using the Microsoft Graph Go SDK in production for over a year now. I suspect if we had an issue along those lines, it'd have come up already.

kfcampbell commented 3 months ago

Hmm...do you have any idea of what else might be the cause of the 400 Bad Request in that case?

baywet commented 3 months ago

Alright, so if I update your sample by replacing the call to the API by

        // and adding this import kjson "github.com/microsoft/kiota-abstractions-go/serialization"
        serializedValue, err := kjson.SerializeToJson(patchRequestBody)
    if err != nil {
        log.Fatalf("error serializing request body: %v", err)
    }
    strSerValue := string(serializedValue)
    log.Printf("serialized request body: %v", strSerValue)

I get {"properties":[{"property_name":"environment","value":"test-from-curl"}]} which is strictly identical in structure to the curl value AND does not contain any escaping of the double quotes as expected.

baywet commented 3 months ago

This one sent me down a rabbit hole... It's caused because the target API does not support Content-Encoding: gzip for the request body AND does not return a 415 status code when it receives encoded payloads....

Hardcoding the client.go client initialization to this fixed the issue.

// GetDefaultClientOptions returns a new instance of ClientOptions with default values.
// This is used in the NewApiClient constructor before applying user-defined custom options.
func GetDefaultClientOptions() *ClientOptions {
    options, _ := kiotaHttp.GetDefaultMiddlewaresWithOptions(kiotaHttp.NewCompressionOptions(false))
    return &ClientOptions{
        UserAgent:  "octokit/go-sdk",
        APIVersion: "2022-11-28",
        Middleware: options,
    }
}

baywet commented 3 months ago

My recommendation here is to talk to the owning team so they implement support for content encoding. Not to disable it in the SDK.

kfcampbell commented 3 months ago

Oh interesting, thank you for spelling it out! I somehow didn't catch that Kiota gzipped everything by default. Do you mind giving me a brief explanation of what the ramifications of disabling compression in the SDK might be? I imagine payloads would be larger and therefore slower by some factor (.25? 2?).

I'm pursuing API support, though that historically has been much slower and more difficult for us to change than spec and SDK issues (especially for changes that could be considered breaking, like this one).

kfcampbell commented 3 months ago

Also, am I crazy or does this not seem to be an option in the .NET helpers the way it is in the Go helpers?

Is there a difference in the way this is handled by the underlying .NET libraries by default?

baywet commented 3 months ago

Also, am I crazy or does this not seem to be an option in the .NET helpers the way it is in the Go helpers?

Is there a difference in the way this is handled by the underlying .NET libraries by default?

The main reason why I though about looking into this one is because I was going through an inventory of where we were at for request body compression. And you're correct, at the moment only Go has it implemented, not dotnet. https://github.com/microsoft/kiota-dotnet/issues/304

Oh interesting, thank you for spelling it out! I somehow didn't catch that Kiota gzipped everything by default. Do you mind giving me a brief explanation of what the ramifications of disabling compression in the SDK might be? I imagine payloads would be larger and therefore slower by some factor (.25? 2?).

I'm pursuing API support, though that historically has been much slower and more difficult for us to change than spec and SDK issues (especially for changes that could be considered breaking, like this one).

We didn't run our own comparisons as it's widely documented on the web 1 2 3. It varies based on the payload between a 75% reduction at best, to ~30% at worst. So non-negligeable benefits. While the CPU takes a small hit for doing the extra work, it's largely compensated by the reduction of transfer time.

microsoft-github-policy-service[bot] commented 3 months ago

This issue has been automatically marked as stale because it has been marked as requiring author feedback but has not had any activity for 4 days. It will be closed if no further activity occurs within 3 days of this comment.

microsoft / kiota