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

Invalid cast on Deployment.Create #2250

Closed
Hoopie opened this issue Sep 16, 2020 · 4 comments · Fixed by #2303
Closed

Invalid cast on Deployment.Create #2250

Hoopie opened this issue Sep 16, 2020 · 4 comments · Fixed by #2303

Comments

@Hoopie
Copy link

Hoopie commented Sep 16, 2020

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?

@Hoopie Hoopie changed the title NewDeployment.Payload accepts json string but RESPONSE Deployment.payload wants IReadonlyDictionary Invalid cast on Deployment.Create Sep 16, 2020
@haacked
Copy link
Contributor

haacked commented Feb 8, 2021

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
Copy link
Contributor

haacked commented Feb 8, 2021

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
Copy link
Contributor

haacked commented Feb 8, 2021

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
Copy link
Contributor

haacked commented Feb 8, 2021

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.

haacked added a commit to haacked/octokit.net that referenced this issue Feb 8, 2021
When serializing the `NewDeployment` type, the `Payload` is serialized as an escaped string because JSON.NET doesn't know it's meant to be JSON.

This causes a problem when you call the API because the Payload is supposed to be a JSON dictionary that's just part of the overall payload. It's not supposed to be an escaped string.

That's why the JSON deserializer fails on it. Not only that, any deployments created using the current Octokit.net will create an invalid payload.

This PR fixes it by changing the type of `Payload` to a dictionary. THIS IS A BREAKING CHANGE, but the old behavior was broken so it forces a new correct behavior.

Fixes octokit#2250
haacked added a commit to haacked/octokit.net that referenced this issue Feb 9, 2021
When serializing the `NewDeployment` type, the `Payload` is serialized as an escaped string because JSON.NET doesn't know it's meant to be JSON.

This causes a problem when you call the API because the Payload is supposed to be a JSON dictionary that's just part of the overall payload. It's not supposed to be an escaped string.

That's why the JSON deserializer fails on it. Not only that, any deployments created using the current Octokit.net will create an invalid payload.

This PR fixes it by changing the type of `Payload` to a dictionary. THIS IS A BREAKING CHANGE, but the old behavior was broken so it forces a new correct behavior.

Fixes octokit#2250
haacked added a commit to haacked/octokit.net that referenced this issue Feb 19, 2021
When serializing the `NewDeployment` type, the `Payload` is serialized as an escaped string because JSON.NET doesn't know it's meant to be JSON.

This causes a problem when you call the API because the Payload is supposed to be a JSON dictionary that's just part of the overall payload. It's not supposed to be an escaped string.

That's why the JSON deserializer fails on it. Not only that, any deployments created using the current Octokit.net will create an invalid payload.

This PR fixes it by changing the type of `Payload` to a dictionary. THIS IS A BREAKING CHANGE, but the old behavior was broken so it forces a new correct behavior.

Fixes octokit#2250
shiftkey pushed a commit that referenced this issue Feb 21, 2021
When serializing the `NewDeployment` type, the `Payload` is serialized as an escaped string because JSON.NET doesn't know it's meant to be JSON.

This causes a problem when you call the API because the Payload is supposed to be a JSON dictionary that's just part of the overall payload. It's not supposed to be an escaped string.

That's why the JSON deserializer fails on it. Not only that, any deployments created using the current Octokit.net will create an invalid payload.

This PR fixes it by changing the type of `Payload` to a dictionary. THIS IS A BREAKING CHANGE, but the old behavior was broken so it forces a new correct behavior.

Fixes #2250
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants