Properly handle the new style of Node IDs in the GitHub GraphQL API #914
Conversation
|
@jcudit can we get some eyes? This bug is popping up for all new repositories while existing seems to work. |
github/util_v4_repository.go
Outdated
| // Check if the name is a base 64 encoded node ID | ||
| _, err := base64.StdEncoding.DecodeString(name) | ||
| if err != nil { | ||
| return false, nil | ||
| } |
There was a problem hiding this comment.
Are there any backwards compatibility risks with removing this? Aiming to read more on this front, but hesitating to merge this without more clarity around if this will be a breaking change for some.
There was a problem hiding this comment.
@jcudit That's what I wanted to verify with mocking out the Github API in the unit tests I wrote. I saw in the edit history that every version that accepted a node_id had this local check before calling out to the github api. Originally, it was checking the length. It was checking for base64, but I never saw a version without a pre-check, so it wasn't clear if it was originally put in to address an issue, or if the idea was to simply save an api call.
If you can think of any other cases that'd be worth testing for, I'd be more than happy to add them.
There was a problem hiding this comment.
originally put in to address an issue
Yes, the aim was to allow passing in a repository name to improve the configuration experience. See #593 for details on this.
There was a problem hiding this comment.
c5a7393 suggests we check for both new and old formats. How does everyone feel about this approach? Seems like a 👎🏾 from a performance / rate limiting perspective but 👍🏾 for reliability.
| @@ -16,6 +16,12 @@ func getRepositoryID(name string, meta interface{}) (githubv4.ID, error) { | |||
| return githubv4.ID(name), nil | |||
| } | |||
|
|
|||
| // Interpret `name` as a legacy node ID | |||
| exists, _ = repositoryLegacyNodeIDExists(name, meta) | |||
There was a problem hiding this comment.
What's the reason the error is ignored here? This function already returns an error, so perhaps it's best to add a condition like the following:
| exists, _ = repositoryLegacyNodeIDExists(name, meta) | |
| exists, err = repositoryLegacyNodeIDExists(name, meta) | |
| if err != nil { | |
| return nil, err | |
| } |
There was a problem hiding this comment.
Ohhh, I see, it's to continue to check the non-legacy case. Please disregard me! This looks good.
…ntegrations#914) * Properly handle the new style of Node IDs in the GitHub GraphQL API https://github.blog/2021-02-10-new-global-id-format-coming-to-graphql/ * fixup! maintain compatibility with deprecated Global ID format Co-authored-by: Emma May <emma@starhawking.com> Co-authored-by: Jeremy Udit <jcudit@github.com>
Properly handle branches using the new style of Node IDs in the GraphQL API
https://github.blog/2021-02-10-new-global-id-format-coming-to-graphql/
Addresses #908