-
Notifications
You must be signed in to change notification settings - Fork 16
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
Implement GraphQL mutations #927
base: master
Are you sure you want to change the base?
Conversation
# Conflicts: # schema/api.graphql
# Conflicts: # schema/api.graphql
Thank you for this pull request, @markspolakovs! @mstratford, would you mind reviewing this PR, please? Feel free to ask anyone for help if you're not quite sure what you're doing. |
schema/api.graphql
Outdated
} | ||
|
||
type Mutation { | ||
createShow(input: CreateShowInput): Show @bind(class: "\\MyRadio\\ServiceAPI\\MyRadio_Show", method: "create", callingConvention: FirstArgInput) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure how I feel about the FirstArgInput
hack - Relay's style guide recommends having all mutations take one argument called input
, but I feel like getting that to work with the ol' PHP stack will require more fuckery than it's worth.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
On the other hand, compare
mutation MyMutation($input: UpdatePostInput!) {
updatePost(input: $input) { ... }
}
and
mutation MyMutation($id: ID!, $newText: String, ...) {
updatePost(id: $id, newText: $newText, ...) { ... }
}
With lots of fields it could get out of hand.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll probably end up adding another calling convention that "splats" the input type across the arguments, kinda like we already do with query args.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done with InputAsArgs
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not 100% how all this works, but have some comments.
findShowByTitle(term: String!, limit: Int!): [FindShowByTitleResult!] @bind(class: "\\MyRadio\\ServiceAPI\\MyRadio_Scheduler", method: "findShowByTitle") @auth(constants: ["AUTH_APPLYFORSHOW"]) # TODO ditto | ||
} | ||
|
||
#input ShowCreditInput { # this is icky |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Lots of commented out stuff :/
@@ -347,7 +349,7 @@ type Timeslot implements Node & MyRadioObject @auth(hook: ViewShow) { | |||
photo: String | |||
messages: [Message] | |||
webpage: String! | |||
|
|||
uploadState: String @meta(key: "upload_state") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
duplicated?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
woops
@@ -462,8 +464,70 @@ type Query { | |||
|
|||
allGenres: [Genre!] @bind(class: "\\MyRadio\\ServiceAPI\\MyRadio_Scheduler", method: "getGenres") @auth(constants: []) | |||
allCreditTypes: [CreditType!] @bind(class: "\\MyRadio\\ServiceAPI\\MyRadio_Scheduler", method: "getCreditTypes") @auth(constants: []) | |||
|
|||
findMemberByName(name: String!, limit: Int): [MemberSearchResult!] @bind(class: "\\MyRadio\\ServiceAPI\\MyRadio_User", method: "findByName") @auth(constants: ["AUTH_APPLYFORSHOW"]) # TODO not 100% sure this is the best constant |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess out of scope, but the fact this uses a completely separate auth list to V2 makes me a bit sad.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It still defaults to V2 auth unless you override it.
The reason for this is that V2 auth is evaluated once (when you call the endpoint), while GraphQL auth is evaluated for every sub-method in the graph. The reason for that is that some object properties are really just methods (for example User.phoneNumber
), which need an auth check of their own (for example, you may want anyone to be able to see basic details of a user, but only see the phone number if you or they are on committee). In V2 this is handled either by changing the shape of the response (in the former case) or by mixins (in the latter) - neither of which are a viable solution for GraphQL because of the strict typing.
Probably about time I got this merged, so this branch doesn't have to live on for any longer