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

Provide basic support for graphene/graphene-sqlalchemy>=3.0 #63

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

Conversation

cooldudemcgeexl
Copy link

  • Update references to ResolveInfo to GraphQLResolveInfo
  • Add handling for beta tags in Graphene version

@erikwrede
Copy link

I'm getting 'GraphQLResolveInfo' object has no attribute 'field_asts' connection_field.py using this on a simple query with a filter. Using field_nodes instead of field_asts does the trick for me:

field_name = info.field_asts[0].name.value

It seems they have renamed this property in GQL Python 3
See here: https://github.com/graphql-python/graphql-core/blob/8e72bfc8a8a9368d665e4c3b881ddb0ad8a9ebdf/src/graphql/type/definition.py#L541

@cooldudemcgeexl
Copy link
Author

Updated the field_asts references to field_nodes, thanks for the catch @erikwrede.
Also added a less hacky way of building the gqls_version tuple when dealing with versions with beta tags i,e, 3.0.0b1. The remaining GraphQLResolveInfo references have been updated in the tests as well.

@erikwrede
Copy link

Seems to be working for me, thanks!

Furthermore, I have observed that with Graphene 3, Variable types for filter Variables seem to be parsed more strictly.
Currently, to prevent having to implement custom filters for many-to-one relationships, I filter using the Integer SQLAlchemy foreign keys. I'm not using the foreign keys anywhere else in the model.

Example:

query$parentUid: Int) {
  child(filters: {parentUid: $parentUid})
}

Using graphene2, i could easily pass the desired ID of the parent as a string (which was convenient, because db primary keys are automatically converted to ID by graphene, which is a string). With graphene 3, this hack does not work anymore and I need to parse the string first, otherwise the query will fail. Maybe someone else has got this use case as well, so it might be helpful to note some "breaking" changes somewhere?

Maybe it should also be noted, that graphene-sqlalchemy 3 itself implemented a solution for N+1 batching, so maybe the reference to the SQLalchemy package is no longer necessary?

Other than that, everything seems to be working as usual.

@art1415926535 any chance we could get a beta release on that for further testing?

@thaonc97
Copy link

graphene has officially upgraded to v3, waiting for this to be merged.

@dave-brennan
Copy link

@art1415926535 any chance of getting this merged?
Thanks for an excellent lib and the work that's gone into this.

@ed-turner
Copy link

Hi team. Just want to know when this could be merged? The work here has been fantastic, and I was looking to use this.

@@ -18,7 +18,7 @@


requirements = [
'graphene-sqlalchemy>=2.1.0,<3',
'graphene-sqlalchemy>=2.1.0',

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should be graphene-sqlalchemy>=3 because the GraphQLResolveInfo was introduced in v3 and is not in 2.x

@palisadoes
Copy link

Graphene v3 officially supports SQLAlchemy v1.4 which has more intuitive syntax for database selects, deletes, and updates that are compatible with the upcoming SQLAlchemy v2 release. It also supports async IO for dataloaders, which I hope will improve performance. This is the last package in the python SQLAlchemy, Flask, GraphQL stack that we use that is dependent on the older graphene version.

The file changes in the PR seem straightforward. Is there a reason, such as an unforseen dependency, that is causing the delay? Of course these are COVID times too, so I'm hoping for the best.

As others have stated, this has been a valuable addition. It was the final piece of the puzzle in our switch to using GraphQL.

@palisadoes
Copy link

After looking at the code for the FilterableConnectionField Class, I see that dataloaders are supported by default, after some troubleshooting of the graphene documentation. Hooray. Is this applied to all fields, including those that are not filtered? I'm not that versed in some of the concepts in the file, hence the question.

@riyasdeens
Copy link

When this PR would be merged? Please comment

@palisadoes
Copy link

palisadoes commented Mar 29, 2022

@cooldudemcgeexl I've tried to contact art1415926535 about merging this with no response. What's the best way to implement this external to the current graphene-sqlalchemy-filter python package? Should I just download the code from this PR and add it as a separate library for my application?

I've opened an issue with the graphene-sqlalchemy repo to see whether they would be interested in taking over this code as it is so useful.

@erikwrede
Copy link

@palisadoes the graphene sqlalchemy repo is also almost unmaintained... an important fix pull request has been stuck waiting for approval for months now.
As long as you're waiting, just add this to your requirements.txt instead of the current library dependency:

git+https://github.com/cooldudemcgeexl/graphene-sqlalchemy-filter.git

it will work as long as @cooldudemcgeexl does not modify the repo, which I hope he doesn't since its a great quick fix!

@palisadoes
Copy link

Thanks. I'll do that as a stop gap.

If neither graphene-sqlalchemy-filter nor graphene-sqlalchemy is being maintained. Should we all migrate to Django or something else? I don't want to be trapped using unmaintained code.

@riyasdeens
Copy link

I used @cooldudemcgeexl fix and everything seems to be working fine. The feature this library provides is mandatory but its sad to see this going unmaintained.

@erikwrede
Copy link

erikwrede commented Mar 29, 2022

@palisadoes For me personally, it's too late to move my projects over to Django, since they're too far in terms of development.

One of the main developers of the graphql python core laid the issue out very well over here:
graphql-python/graphene-sqlalchemy#326 (comment)

Sadly, I don't think I have enough experience with graphene or sqlalchemy to start maintaining such a complex library ecosystem, but me and many others would like to help out and have some guidance by someone more experienced with SQLAlchemy.

@palisadoes
Copy link

@erikwrede I'm willing to partially sponsor integration of this feature into graphene-sqlalchemy. Would any of the others on this thread be willing to approach their employers, or do so personally?

@palisadoes
Copy link

@cooldudemcgeexl Would you be interested in doing some sponsored work to integrate graphene-sqlalchemy-filter into graphene-sqlalchemy?

@dave-brennan
Copy link

@palisadoes I'd also partially sponsor getting this integrated

@riyasdeens
Copy link

@palisadoes How much of sponsor required for this? I would do partial sponsor as well.

@palisadoes
Copy link

Thanks @dave-brennan and @riyasdeens

@sabard and @PaulSchweizer are working on a requirements Google Doc to help determine the expected time and cost. Join us on the #graphene-sqlalchemy-filter channel on the Graphene Slack organization. The link can be found here https://github.com/graphql-python/graphene

Let us know on the channel whether you'd like to participate in the doc and your comments on the pricing, etc. as the appropriate times approach.

@Redysz
Copy link

Redysz commented Aug 3, 2022

What's the status? 🙂

@erikwrede
Copy link

@Redysz planning is done. Implementation is in progress, please check the recent PRs in graphene-sqlalchemy.

@palisadoes
Copy link

What's the status? slightly_smiling_face

Please take a look here, for example:

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.

8 participants