octokit / octokit.net

A GitHub API client library for .NET
https://octokitnet.readthedocs.io/en/latest/
MIT License
2.69k stars 1.08k forks source link

Invalid cast on Deployment.Create #2250

Closed Hoopie closed 3 years ago

Hoopie commented 4 years ago

As per the documentation the deployment.payload is intended to accept json. Octokit.net has NewDeployment.Payload as string.

When creating a new deployment with a json serialized string in Payload the request is accepted and the deployment is created. However, it fails with an InvalidCastException when attempting return the Deployment response model.

On closer inspection it appears to be an issue with the Deployment response model expecting to be able to cast to a IReadOnlyDictionary<string,string> yet the NewDeployment request model accepts a string. See https://github.com/octokit/octokit.net/blob/main/Octokit/Models/Response/Deployment.cs#L60 and https://github.com/octokit/octokit.net/blob/main/Octokit/Models/Request/NewDeployment.cs#L62


Here is some of my sandbox code I used that raised the error. This is using Octokit 0.48.0

new NewDeployment("master")
{
    Environment = "TestEnvironment1",
    Description = "ShipIt",

    //Serialize a model in to the payload
    Payload = JsonSerializer.Serialize(new
    {
        Property1 = "prop1value",
        Property2 = "prop2value",
    })
})

I get the following error

System.InvalidCastException
Invalid cast from 'System.String' to 'System.Collections.Generic.IReadOnlyDictionary`2[[System.String, System.Private.CoreLib, Version=4.0.0.0, Culture=neutral, PublicKeyToken=7cec85d7bea7798e],[System.String, System.Private.CoreLib, Version=4.0.0.0, Culture=neutral, PublicKeyToken=7cec85d7bea7798e]]'.
   at System.Convert.DefaultToType(IConvertible value, Type targetType, IFormatProvider provider)
   at System.String.System.IConvertible.ToType(Type type, IFormatProvider provider)
   at System.Convert.ChangeType(Object value, Type conversionType, IFormatProvider provider)
   at Octokit.PocoJsonSerializerStrategy.DeserializeObject(Object value, Type type) in /home/runner/work/octokit.net/octokit.net/Octokit/SimpleJson.cs:line 1407
   at Octokit.Internal.SimpleJsonSerializer.GitHubSerializerStrategy.DeserializeObject(Object value, Type type) in /home/runner/work/octokit.net/octokit.net/Octokit/Http/SimpleJsonSerializer.cs:line 205
   at Octokit.PocoJsonSerializerStrategy.DeserializeObject(Object value, Type type) in /home/runner/work/octokit.net/octokit.net/Octokit/SimpleJson.cs:line 1494
   at Octokit.Internal.SimpleJsonSerializer.GitHubSerializerStrategy.DeserializeObject(Object value, Type type) in /home/runner/work/octokit.net/octokit.net/Octokit/Http/SimpleJsonSerializer.cs:line 205
   at Octokit.PocoJsonSerializerStrategy.DeserializeObject(Object value, Type type) in /home/runner/work/octokit.net/octokit.net/Octokit/SimpleJson.cs:line 1521
   at Octokit.Internal.SimpleJsonSerializer.GitHubSerializerStrategy.DeserializeObject(Object value, Type type) in /home/runner/work/octokit.net/octokit.net/Octokit/Http/SimpleJsonSerializer.cs:line 205
   at Octokit.SimpleJson.DeserializeObject(String json, Type type, IJsonSerializerStrategy jsonSerializerStrategy) in /home/runner/work/octokit.net/octokit.net/Octokit/SimpleJson.cs:line 590
   at Octokit.SimpleJson.DeserializeObject[T](String json, IJsonSerializerStrategy jsonSerializerStrategy) in /home/runner/work/octokit.net/octokit.net/Octokit/SimpleJson.cs:line 602
   at Octokit.Internal.SimpleJsonSerializer.Deserialize[T](String json) in /home/runner/work/octokit.net/octokit.net/Octokit/Http/SimpleJsonSerializer.cs:line 22
   at Octokit.Internal.JsonHttpPipeline.DeserializeResponse[T](IResponse response) in /home/runner/work/octokit.net/octokit.net/Octokit/Http/JsonHttpPipeline.cs:line 62
   at Octokit.Connection.Run[T](IRequest request, CancellationToken cancellationToken) in /home/runner/work/octokit.net/octokit.net/Octokit/Http/Connection.cs:line 653
   at Octokit.ApiConnection.GetPage[TU](Uri uri, IDictionary`2 parameters, String accepts, ApiOptions options) in /home/runner/work/octokit.net/octokit.net/Octokit/Http/ApiConnection.cs:line 622
   at Octokit.ApiConnection.<>c__DisplayClass18_0`1.<<GetAll>b__0>d.MoveNext() in /home/runner/work/octokit.net/octokit.net/Octokit/Http/ApiConnection.cs:line 212

Of note, i also tried creating a NewDeployment serializing a Dictionary<string,string> to payload but got the same result. Deployment was created on github but Octokit failed with a cast exception.

new NewDeployment("master")
{
    Environment = "TestEnvironment1",
    Description = "ShipIt",
    Payload = JsonSerializer.Serialize(new Dictionary<string, string>
    {
        {"prop1", "value1"},
        {"prop2", "value1"}
    })
})

When calling github directly via curl I can see my deployment object created as expected. Note payload is a string and not json.

...
"ref": "master",
"task": "deploy",
"payload": "{\"prop1\":\"value1\",\"prop2\":\"value1\"}",
"environment": "TestEnvironment1",
"description": "ShipIt",
...

Is this a bug or am i over thinking it?

haacked commented 3 years ago

I'm running into this too. Definitely looks like a bug. The question is does the GitHub API do anything with the Payload property? Or is that just round tripped by the API and ignored by GitHub. If it's ignored by GitHub, maybe the safest change is to change the type of Payload to string on Deployment.

Otherwise it might make sense to change Payload on NewDeployment to be a dictionary so things round trip properly.

haacked commented 3 years ago

Actually, probably needs to be a string since the Payload field can be created by other clients. I know the ruby client just lets you set that as a string. In order to be able to use this client to load deployments created by other clients, we can't assume they put it in a format deserializable to a dictionary.

haacked commented 3 years ago

Ah, changing it to a string probably won't help. The issue is that the serializer doesn't know that NewDeployment.Payload is supposed to be a JSON string. So when it serializes it, it escapes it.

[Fact]
public void CanSerialize()
{
    var deployment = new NewDeployment("ref")
    {
        Payload = @"{ ""environment"":""production""}"
    };
    var deserialized = new SimpleJsonSerializer().Serialize(deployment);

    Assert.Equal(@"{""ref"":""ref"",""task"":""deploy"",""payload"":""{ \""environment\"":\""production\""}""}", deserialized);
}

That's why it can't round trip it. I wonder if there's a way to tell JSON.NET to treat that property as raw JSON and don't encode it.

haacked commented 3 years ago

Ok, after digging into the code, I forgot that Octokit.net uses a simple serializer that doesn't embed type info. So I think the easy fix is to change NewDeployment.Payload to Dictionary<string, string>. It's a breaking change, but probably worth it considering that any deployments created prior are kind of broken.