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

Stored Procedure is not Relay compliant #110

Closed
ssomnoremac opened this issue Sep 7, 2016 · 5 comments
Closed

Stored Procedure is not Relay compliant #110

ssomnoremac opened this issue Sep 7, 2016 · 5 comments
Milestone

Comments

@ssomnoremac
Copy link

ssomnoremac commented Sep 7, 2016

Because the mutation payload of a stored procedure is nested in the output field, the Relay fatQuery must look something like this:

getFatQuery() {
    return Relay.QL`fragment on myStoredProcedurePayload {
      output {
          applicationByApplicationId
      }
    }`
}

output is the only acceptable root field for the fatQuery fragment besides viewer. However, the fatQuery must include a field that you intend to modify in the getConfigs():

getConfigs() {
    return [{
        type: 'FIELDS_CHANGE',
        fieldIDs: { application : this.props.application.id }
    }];
}

if the response from the stored procedure wasn't nested in the output field and the response fields matched the field types, all would be fine. But that isn't the case and I'm not sure it's possible to fix this using Relay.

@calebmer
Copy link
Collaborator

calebmer commented Sep 7, 2016

What is the type signature for your procedure?

(As a side note, I can't wait for Relay 2 where mutations won't be so declarative…)

@tombollich
Copy link

tombollich commented Sep 7, 2016

Hello Caleb, I'm working with @ssomnoremac on the problem. The proc returns type application which is a table in the db. The issue we are having is that you are wrapping the return with output{}, so it works fine with the graphiql interface but fails with relay. Relay cannot access a nested object, output.application is not allowed.

@calebmer
Copy link
Collaborator

calebmer commented Sep 8, 2016

Why not do a FIELDS_CHANGE on the output field? This should correctly propagate changes.

As a side note, I see how the solution may seem simple (move application to the root mutation payload type), but it's actually much more complex and nuanced. If we have the mutation directly return the table, we wouldn't be compliant with the Relay spec, we'd also be severely limiting future design space (as we couldn't put fields viewer and clientMutationId parallel to output). So we need to keep output nested, so the next question is how and what do we need to put in the payload type alongside output? All the fields of output that may return single nodes? If we do that, then we've got other things to worry about like the query optimizations I'm working on. Also the having two ways to get the same data may confuse new users in the future. It's a tough problem.

If this is critical for you and running FIELD_CHANGE on output won't work let's see if we can come to an ideal solution knowing this problem may be short term, but the fix would be in for a while 👍

@tombollich
Copy link

Yes I agree the problem is very complicated and nuanced, especially with all the different parts that need to work together in a sometimes hidden way (relay mostly). I believe the heart of the problem could be that most of these technologies are designed around a document database and not a relational. Anyway, I have a couple of more things I want to try so I can give you concrete examples of what is not working.

@calebmer calebmer mentioned this issue Oct 10, 2016
4 tasks
@calebmer calebmer added this to the 2.0.0 milestone Oct 13, 2016
@calebmer
Copy link
Collaborator

I’m going to close this as its in the PostGraphQL 2.0 beta 🎉

Specifically, all related tables are including in create mutations, update mutations, delete mutations, and procedure mutations that return a table.

Start playing with the pre-release, the final release should be out really soon. Tell me what you think! 👍

npm install -g postgraphql@next
postgraphql

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants