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

Adding SQLAlchemyInputObjectType #213

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

Rafik-Belkadi
Copy link

No description provided.

@jnak
Copy link
Collaborator

jnak commented May 15, 2019

Hello @Rafik-Belkadi, thanks for sending a PR. I agree with you that there is potential for graphene-sqlalchemy to help with mutations. That said, it's not entirely clear how you intend to use your change since you haven't added any tests.

Since it's a whole new functionality, I suggest that before adding tests you start a separate issue / conversation that outlines what your goals are, the different strategies to go about this and your proposed solution (see #212 (comment)). You can see examples of recent successful design conversations here and here. These conversations are great way to gather early feedback and reach consensus before you start get into the details of the implementation. Obviously, if the problem you are solving is straightforward, the conversation does not need to be as long as these two examples.

Here are some of the considerations that I think we should discuss as a group:

  • Why not use use a the already defined SQLAlchemyObjectType types in the mutation?
class ShipMutation(relay.ClientIDMutation):
    class Input:
        ship = graphene.Field(Ship)
    ship = graphene.Field(Ship)
  • Should the input include connection relationships to other types (as it is in the current version of this PR)? If not, how do want to handle those?
  • Does it make sense to include hybrid properties, composite columns and column properties?

Cheers
J

@sreerajkksd
Copy link

sreerajkksd commented Aug 6, 2020

There are some discussions about the mutation support in #29 and https://gist.github.com/qubitron/6e7d48667a05120dc58bdfd0516b3980 seems to have helped some users.

@sreerajkksd
Copy link

sreerajkksd commented Aug 11, 2020

@jnak that might not work.

class ShipMutation(relay.ClientIDMutation):
    class Input:
        ship = graphene.Field(Ship)
    ship = graphene.Field(Ship)

ShipMutationInput.ship field should be of Input type. Ship in your example is a subclass of SQLAlchemyObjectType.

@himat
Copy link

himat commented Aug 28, 2020

I'm getting this error while trying to use this code
TypeError: convert_sqlalchemy_column() missing 1 required positional argument: 'resolver'

@baderj
Copy link

baderj commented Feb 19, 2021

class ShipMutation(graphene.Mutation):
    class Input:
        ship = graphene.Field(Ship)
    ship = graphene.Field(Ship)

This fails with ValueError: Expected input to be Argument, but received Field. Currently the only way I found is use the graphene.InputObjectType and specify each column again from the sqlalchemy model.

Is there a reason, why SQLAlchemyInputObjectType was never merged? Having a nice

class Ship(SQLAlchememyObjectType):
    class Meta:
        model = tables.Ship

followed by the verbose

class ShipInput(InputObjectType):
    name = graphene.String()
    registration = graphene.String()
    height = graphene.Number()
    width = graphene.Number()
    ... 50 other columns copied from orm

Is not only very annoying to type out, it also look very inconsistent. I don't really need the SQLAlchemyInputObjectType if there were a way to use an SQLAlchemyObjectType as Arguments or Input to the graphene.Mutation

@seanxlliu
Copy link

@jnak Thanks for making this great library! Sorry for bringing back this old question again. I have seen many ppl request this feature
#212 #216 #285
Now I have encoutered the exactly same issue in my project. We have a huge mount of data models need mutations though graphql APIs. I have read your comment above, however, the types cannot be reused as inputs. here is the philosophy of GraphQL. Long in short:

The GraphQL Object type (ObjectTypeDefinition)... is inappropriate for re‐use [as an input], because Object types can contain fields that define arguments or contain references to interfaces and unions, neither of which is appropriate for use as an input argument. For this reason, input objects have a separate type in the system.

So, what is the next move for the improvement so that we can keep working on?

@seanxlliu
Copy link

seanxlliu commented Mar 27, 2021

More from GraphQL spec:

IsInputType(type)
If type is a List type or Non‐Null type:
Let unwrappedType be the unwrapped type of type.
Return IsInputType(unwrappedType)

If type is a Scalar, Enum, or Input Object type:
Return true
Return false

IsOutputType(type)
If type is a List type or Non‐Null type:
Let unwrappedType be the unwrapped type of type.
Return IsOutputType(unwrappedType)
If type is a Scalar, Object, Interface, Union, or Enum type:
Return true
Return false

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

Successfully merging this pull request may close these issues.

None yet

6 participants