DEVPROD-5460: Update GraphQL API with more specific arguments #7682
Conversation
| hasVersion(id: String!): Boolean! | ||
| version(id: String!): Version! | ||
| hasVersion(id: String @deprecated(reason: "use patchId instead"), patchId: String): Boolean! | ||
| version(id: String @deprecated(reason: "use versionId instead"), versionId: String): Version! |
There was a problem hiding this comment.
For clarification, will this change ever be safe to make in a backwards compatible way? It seems like at some point we'll need to deploy a version of the backend that only has one argument and would need to update the front end at the same time.
There was a problem hiding this comment.
oh, I was going to deploy this, then change the frontend to only use the new args (e.g. patchId). Do you think that wouldn't work? (I also edited the resolvers) 👀
There was a problem hiding this comment.
Would it be possible for the front end to only use the new arg? I thought since the query args are order dependent, we'd still need to have null or an empty string as the legacy id argument, which would make it impossible to remove from the backend safely. But I may be wrong 😳
There was a problem hiding this comment.
oh!! that's a good point! I tested it out and it seems ok but let me know if it seems wack 🫣
There was a problem hiding this comment.
oh that's awesome, you learn something new every day! thanks for looking into it.
| @@ -20,6 +20,7 @@ It contains information about project settings (e.g. Build Baron configurations, | |||
| update the settings for a given project. | |||
| """ | |||
| input ProjectSettingsInput { | |||
| projectId: String # TODO: Make this required. | |||
There was a problem hiding this comment.
I'm not sure how I feel about duplicating the same field twice (here and in projectRef.Id, and in other structs). What do you think is the main benefit?
There was a problem hiding this comment.
Ahh the reason I did this is if we have projectRef.Id or repo.Id, just having Id isn't specific enough to describe what type of input it is. I think we wouldn't be able to put the new directive on the projectRef or repoRef objects because they wouldn't convert properly into a map, and we'd have to put it on the Id field. I'm open to other ideas though!
There was a problem hiding this comment.
Ah the new directives, that makes perfect sense! Thanks 🤩
There was a problem hiding this comment.
In #7644 we discussed using projectIdentifier as the key for projectIds do we still want to do that?
There was a problem hiding this comment.
I'm sort of against it in this case because this input mirrors the RepoSettingsInput, and the repo uses ID (also, the input to this will truly be an ID, and not an identifier)
There was a problem hiding this comment.
Makes sense thanks for clarifying!
|
removed app review since its probably not necessary |
| } | ||
| if p != nil { | ||
| return false, nil | ||
| } | ||
| } | ||
| return false, ResourceNotFound.Send(ctx, fmt.Sprintf("Unable to find patch or version %s", id)) | ||
| return false, ResourceNotFound.Send(ctx, fmt.Sprintf("Unable to find patch or version %s", patchId)) |
There was a problem hiding this comment.
| return false, ResourceNotFound.Send(ctx, fmt.Sprintf("Unable to find patch or version %s", patchId)) | |
| return false, ResourceNotFound.Send(ctx, fmt.Sprintf("finding patch or version '%s'", patchId)) |
There was a problem hiding this comment.
lmao...... turns out I can't update this one because the e2e tests specifically check for this message 🤣 (in pain)
minnakt
changed the title
minnakt
changed the title
minnakt
changed the title
DEVPROD-5460
Description
We are updating the GraphQL API to be more explicit about parameters. For example, if the input is expected to be a version ID, the parameter should explicitly be called
versionIdrather than justid. We need these explicit parameters in order to support the new project permissions directive.The old parameters will need to continue to be supported temporarily for backwards compatibility.
Note for the UI team
I think the guidelines for using objects for inputs/payloads are only for mutations. When I checked some famous GraphQL APIs (like GitHub and Shopify), they seemed to only use that pattern for mutations.
Testing