Repository navigation
Adding label results in: json: cannot unmarshal number into Go struct field label.id of type string #8652
Description
Activity
- added a commit that references this issue
on Jan 31, 2024 Thanks for opening this bug and sorry for the inconvenience.
Looks likely related to #8516 @andyfeller can you look at this?
Reacted by Andy Feller@kwk can you provide the output of the failing command with
GH_DEBUG=apiplease.@williammartin sure:
$ GH_DEBUG=api gh --repo fedora-llvm-team/llvm-snapshots label create "arch/x86_64" --color C5DEF5 --force ⣾* Request at 2024-01-31 18:11:49.010277398 +0100 CET m=+0.029303388 * Request to https://api.github.com/repos/fedora-llvm-team/llvm-snapshots/labels > POST /repos/fedora-llvm-team/llvm-snapshots/labels HTTP/1.1 > Host: api.github.com > Accept: application/vnd.github.merge-info-preview+json, application/vnd.github.nebula-preview > Authorization: token ████████████████████ > Content-Length: 56 > Content-Type: application/json; charset=utf-8 > Time-Zone: Europe/Berlin > User-Agent: GitHub CLI 2.43.0 { "color": "C5DEF5", "description": "", "name": "arch/x86_64" } ⢿< HTTP/2.0 422 Unprocessable Entity < Access-Control-Allow-Origin: * < Access-Control-Expose-Headers: ETag, Link, Location, Retry-After, X-GitHub-OTP, X-RateLimit-Limit, X-RateLimit-Remaining, X-RateLimit-Used, X-RateLimit-Resource, X-RateLimit-Reset, X-OAuth-Scopes, X-Accepted-OAuth-Scopes, X-Poll-Interval, X-GitHub-Media-Type, X-GitHub-SSO, X-GitHub-Request-Id, Deprecation, Sunset < Content-Length: 182 < Content-Security-Policy: default-src 'none' < Content-Type: application/json; charset=utf-8 < Date: Wed, 31 Jan 2024 17:11:49 GMT < Referrer-Policy: origin-when-cross-origin, strict-origin-when-cross-origin < Server: GitHub.com < Strict-Transport-Security: max-age=31536000; includeSubdomains; preload < Vary: Accept-Encoding, Accept, X-Requested-With < X-Accepted-Oauth-Scopes: < X-Content-Type-Options: nosniff < X-Frame-Options: deny < X-Github-Api-Version-Selected: 2022-11-28 < X-Github-Media-Type: github.merge-info-preview; param=nebula-preview; format=json < X-Github-Request-Id: 2538:19A112:73F68C2:751C61D:65BA7F55 < X-Oauth-Client-Id: 178c6fc778ccc68e1d6a < X-Oauth-Scopes: admin:public_key, gist, read:org, repo < X-Ratelimit-Limit: 5000 < X-Ratelimit-Remaining: 4982 < X-Ratelimit-Reset: 1706722516 < X-Ratelimit-Resource: core < X-Ratelimit-Used: 18 < X-Xss-Protection: 0 { "message": "Validation Failed", "errors": [ { "resource": "Label", "code": "already_exists", "field": "name" } ], "documentation_url": "https://docs.github.com/rest/issues/labels#create-a-label" } * Request took 363.209682ms * Request at 2024-01-31 18:11:49.379709535 +0100 CET m=+0.398735524 * Request to https://api.github.com/repos/fedora-llvm-team/llvm-snapshots/labels/arch/x86_64 > PATCH /repos/fedora-llvm-team/llvm-snapshots/labels/arch/x86_64 HTTP/1.1 > Host: api.github.com > Accept: application/vnd.github.merge-info-preview+json, application/vnd.github.nebula-preview > Authorization: token ████████████████████ > Content-Length: 18 > Content-Type: application/json; charset=utf-8 > Time-Zone: Europe/Berlin > User-Agent: GitHub CLI 2.43.0 { "color": "C5DEF5" } ⣟< HTTP/2.0 200 OK < Access-Control-Allow-Origin: * < Access-Control-Expose-Headers: ETag, Link, Location, Retry-After, X-GitHub-OTP, X-RateLimit-Limit, X-RateLimit-Remaining, X-RateLimit-Used, X-RateLimit-Resource, X-RateLimit-Reset, X-OAuth-Scopes, X-Accepted-OAuth-Scopes, X-Poll-Interval, X-GitHub-Media-Type, X-GitHub-SSO, X-GitHub-Request-Id, Deprecation, Sunset < Cache-Control: private, max-age=60, s-maxage=60 < Content-Security-Policy: default-src 'none' < Content-Type: application/json; charset=utf-8 < Date: Wed, 31 Jan 2024 17:11:49 GMT < Etag: W/"9b05ddd9ed668ffe3d0e01c86a0c84d0faaa1cbbb6e1727fd757a4d21b5c860b" < Referrer-Policy: origin-when-cross-origin, strict-origin-when-cross-origin < Server: GitHub.com < Strict-Transport-Security: max-age=31536000; includeSubdomains; preload < Vary: Accept, Authorization, Cookie, X-GitHub-OTP < Vary: Accept-Encoding, Accept, X-Requested-With < X-Accepted-Oauth-Scopes: < X-Content-Type-Options: nosniff < X-Frame-Options: deny < X-Github-Api-Version-Selected: 2022-11-28 < X-Github-Media-Type: github.merge-info-preview; param=nebula-preview; format=json < X-Github-Request-Id: 2538:19A112:73F69E9:751C75C:65BA7F55 < X-Oauth-Client-Id: 178c6fc778ccc68e1d6a < X-Oauth-Scopes: admin:public_key, gist, read:org, repo < X-Ratelimit-Limit: 5000 < X-Ratelimit-Remaining: 4981 < X-Ratelimit-Reset: 1706722516 < X-Ratelimit-Resource: core < X-Ratelimit-Used: 19 < X-Xss-Protection: 0 { "id": 6175199621, "node_id": "LA_kwDOFBZt3s8AAAABcBIRhQ", "url": "https://api.github.com/repos/fedora-llvm-team/llvm-snapshots/labels/arch/x86_64", "name": "arch/x86_64", "color": "C5DEF5", "default": false, "description": "" } * Request took 304.566588ms json: cannot unmarshal number into Go struct field label.id of type string
Thanks, pretty suspicious line change here: c2f6a4e#diff-b919d95405a6827b4deaafd94935ed8158e4333cd308181bae75982f3526cb40L24
Unfortunately the author is no longer on the team or with GitHub so will have to try and understand this change without him.
Thanks, pretty suspicious line change here: c2f6a4e#diff-b919d95405a6827b4deaafd94935ed8158e4333cd308181bae75982f3526cb40L24
Unfortunately the author is no longer on the team or with GitHub so will have to try and understand this change without him.
Well, I'd say the
StructExportDatafunction being used instead of the reflection doesn't sound all too good:// Basic function that can be used with structs that need to implement // the exportable interface. It has numerous limitations so verify // that it works as expected with the struct and fields you want to export. // If it does not, then implementing a custom ExportData method is necessary. // Perhaps this should be moved up into exportData for the case when // a struct does not implement the exportable interface, but for now it will // need to be explicitly used.
(source: https://github.com/cli/cli/blob/trunk/pkg/cmdutil/json_flags.go#L267C1-L273C31)
I wonder why there's no test checking if the API still works. This error itself should have been caught by CI, no?
I wonder why there's no test checking if the API still works. This error itself should have been caught by CI, no?
That should be the case but doesn't look to be.
Well, I'd say the StructExportData function being used instead of the reflection doesn't sound all too good:
Not familiar with this function so can't say yet but in any case the error occurs earlier.
We can see that the
labelstruct says thatIdshould be astring:Line 24 in e461c89
But from your request the
Idis clearly a number:{ "id": 6175199621, "node_id": "LA_kwDOFBZt3s8AAAABcBIRhQ", "url": "https://api.github.com/repos/fedora-llvm-team/llvm-snapshots/labels/arch/x86_64", "name": "arch/x86_64", "color": "C5DEF5", "default": false, "description": "" }So it's failing trying to unmarshal the response here:
Lines 176 to 177 in e461c89
The obvious thing to do here is change the struct tag back and ship a patch release but I'm just hesitant because I don't understand the reason it was changed in the first place.
Alright I think I sort of see what might have happened here and it's a result of sharing a struct that probably shouldn't be. Here's the label struct:
Lines 20 to 29 in e461c89
It gets used when listing labels via
gh label listwhich hits the graphql API:Line 29 in e461c89
It also gets used for the response from creating or updating a label using
gh label createwhich hits the REST API:Lines 140 to 141 in e461c89
The problem is that the responses from these APIs are not the same schema.
Here's the graphql object and we can see that it does have an
idfield with typeID(which is effectivelystring): https://docs.github.com/en/graphql/reference/objects#labelAnd here's the REST object which has a
numberid and anode_idstring: https://docs.github.com/en/rest/issues/labels?apiVersion=2022-11-28#create-a-label
I suspect that the author of the change didn't look too closely at the usage of
createsince that PR was mostly aboutvariable list. I don't know why they decided to make a change in the labels code. I also have no idea why these commands use REST sometimes and graphql at others but there's probably some legacy reason since they've been around for a long time.The correct course of action here seems to be to split this type into two.
Although, I don't really understand why
create/updateneeds this type at all since the request result seems to get thrown away so maybe we can just stop using it.@kwk if you happen to be up for building from source could you try out this branch and see if it resolves your issue (and doesn't introduce anything else new). It seems to on my end.
@williammartin and @andyfeller I can confirm that https://github.com/cli/cli/releases/tag/v2.43.1 fixed the issue for me. Thank you for creating a fix so quickly!
@williammartin you've mentioned this test here:
--- FAIL: TestCreateRun (0.00s) --- FAIL: TestCreateRun/creates_label_(EXPECTED_TO_BREAK_FOR_DEMONSTRATION_PURPOSES) (0.00s) /Users/williammartin/workspace/cli/pkg/cmd/label/create_test.go:179: Error Trace: /Users/williammartin/workspace/cli/pkg/cmd/label/create_test.go:179 Error: Received unexpected error: json: cannot unmarshal number into Go struct field label.id of type string Test: TestCreateRun/creates_label_(EXPECTED_TO_BREAK_FOR_DEMONSTRATION_PURPOSES)Did this test ever pass before your patch? I mean this looks like a CI test that should have blocked the old PR in the first place, shouldn't it?
Anyways, thank you again so much! Feel free to close the issue.
@kwk I wrote that test to demonstrate the issue programmatically. There was no existing test that covered this. Here's the PR from 2 years ago when
label createwas added: #5316You can see that the response was unmarshaled but never used: https://github.com/ZaiZheTingDun/cli/blob/ff75841657b402cdda63b7f0b7e8bf90d668ac58/pkg/cmd/label/create/create.go#L131-L133
And so presumably the tests were written to match the idea that we didn't care about the data: https://github.com/ZaiZheTingDun/cli/blob/ff75841657b402cdda63b7f0b7e8bf90d668ac58/pkg/cmd/label/create/create_test.go#L101
Unfortunately no one noticed that even though we don't care about the values of the response, there was a possible error path in the unmarshaling with regards the schema. Of course we'd like to have coverage of every branch but things will slip through. One of my focuses for this year is to think about how we can improve our confidence around API interactions.
Anyway, thanks for the confirmation and very sorry that we broke you.
Describe the bug
This command used to work with
ghin version 2.36.0.After updating to version 2.43.0 the same command results in an error:
Steps to reproduce the behavior
gh --repo <YOURREPOHERE> label create arch/x86_64 --color C5DEF5 --forcejson: cannot unmarshal number into Go struct field label.id of type stringExpected vs actual behavior
I want a label to be created without an error as it was with
ghin version 2.36.0.Logs
I've enabled the debug output and here is the output: