search

tfoxy / graphene-django-optimizer

Optimize database access inside graphene queries
MIT License
427 stars 86 forks source link

Fix compatibility with graphql-core v3.2 #83

Closed nikolaik closed 1 year ago

nikolaik commented 2 years ago

This fixes the compatibility issues with graphql-core v3.2.

The relevant changes in graphql-core are these two commits:

Fixes #82

There was a failing relay test which I couldn't find any solution for, so I made a workaround in 5a6791d. Do you have an idea what could be wrong?

Also the promise library was failing to install on python 3.7 so I bumped the python patch version which includes a newer setuptools I believe

codecov-commenter commented 2 years ago

Codecov Report

Merging #83 (f18d827) into master (474bc68) will increase coverage by 0.07%. The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master      #83      +/-   ##
==========================================
+ Coverage   93.63%   93.71%   +0.07%     
==========================================
  Files           7        7              
  Lines         330      334       +4     
==========================================
+ Hits          309      313       +4     
  Misses         21       21
Impacted Files Coverage Δ
graphene_django_optimizer/query.py 92.10% <100.00%> (-0.03%) :arrow_down:
graphene_django_optimizer/utils.py 100.00% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update 474bc68...f18d827. Read the comment docs.

helgihg commented 1 year ago

@tfoxy Any movement on this getting merged?

hishamkaram commented 1 year ago

@sicksid @tfoxy this PR is not working, i have tested it. there is no optimization applied to the queryset select_related and prefetch_related are not being added to the queryset

helgihg commented 1 year ago

Notifying @nikolaik of that, too.

tfoxy commented 1 year ago

Hi everyone, sorry for not being active here, I took some time out from coding to prioritize other things in my life.

I would like to confirm what @hishamkaram said. @nikolaik @sicksid does it optimize the queries for you? Tests are passing fine but they are not integration tests so there may be something wrong where the info being passed in the optimizer is not being actually used now when Django applies the query.

nikolaik commented 1 year ago

I have not been using this package in any current project, so haven't been able to prioritise this PR. I initially tried looking at the relevant changes between v3.1 and 3.2 and I believe the two linked commits in the PR description are the relevant ones. @tfoxy Do you understand why my changes are not working?

tfoxy commented 1 year ago

I'm not sure right now. I need to read my code again and I will try this PR with a sample app to debug.

andrei-datcu commented 1 year ago

Gave this a go: The PR is good and should be merged. The observations above are unfortunately correct, but definitely not related to these changes. The select_related optimizations are discarded currently because of https://github.com/graphql-python/graphene-django/pull/1315/files#r1106051616

To get around that particular issue, a new converter for ForeignKey/OneToOne fields may be reregistered. But again, this is not a problem related to this PR. You should merge this as is.

sjdemartini commented 1 year ago

(FYI as to the separate question raised above about failing query-optimization in newer graphene-django versions, I've opened a PR in graphene-django https://github.com/graphql-python/graphene-django/pull/1401 which hopefully will be merged to resolve those issues.)

firaskafri commented 1 year ago

Fix is now available in graphene-django v3.0.2.

Just a heads up for folks who use get_queryset as an auth layer before this release. If you need that, you can stick with versions between >=3.0.0b9 and <3.1.0 for now. Feel free to suggest a different way that won't lead to those pesky N+1 SQL query issues.

sjdemartini commented 1 year ago

I have tested this change locally, and it worked seamlessly for my company's project that has quite a bit of graphene-django-optimizer usage, as well as for my package graphene-django-permissions.

I have tests that check for query optimization/performance as well, and all queries were still optimized. The select_related/prefetch_related concerns mentioned above were due to an issue in graphene-django which is fixed on v3.0.2 as mentioned above, so that is not an issue on this PR and has been resolved separately.

Would it be possible to merge and publish when you have a chance @tfoxy? Thanks so much! And thank you nikolaik for these fixes!

salcedo commented 1 year ago

Is this good to go using relay? I tried this and the relay query test is still failing.

sjdemartini commented 1 year ago

@salcedo what are you referring to by "the relay query test is still failing"? CI passed on this branch: https://github.com/tfoxy/graphene-django-optimizer/pull/83/checks. If you're referring to the test_should_return_valid_result_in_a_relay_query test, that test is failing on master (so unrelated to anything in this PR), but is actually passing here since nikolaik patched it in this branch as a workaround to get CI to pass, as he mentioned in the PR description.

salcedo commented 1 year ago

Yes I was unsure if that test being bypassed was a workaround for CI only or if the PR was unfinished. All appears to be working on Django 4.2.1 with Graphene 3.2.2

sjdemartini commented 1 year ago

Hi @tfoxy, this PR is coming up on its 1 year mark, and I definitely appreciate that you may not be able to dedicate any further time toward maintaining graphene-django-optimizer. graphene-django had a long hiatus, but has recently been revived and has been getting regular updates and improvements, so it would be a shame to have to fork graphene-django-optimizer in order to keep it up to date. I would be happy to join as a maintainer of graphene-django-optimizer if that's an option (as I also recently joined graphene-django), or perhaps you have other folks in mind who may be able to review code, merge fixes like these, and publish newer versions. (For instance, there's a pending PR in graphene-django that is likely to break some more tests here on master https://github.com/graphql-python/graphene-django/pull/1411, but I'd be glad to work on and publish fixes for them.)

Thanks again for creating this package and maintaining it for so long! Let me know if there's anything else I or others can do to help.

tfoxy commented 1 year ago

Hi everyone! Sorry for not responding for so long. I took some time off from programming for personal reasons.

@sjdemartini your comment seems good. I'm going to make you a maintainer of this repo. Not expecting you to do any work here of course, but if it's useful to you then any contributions are welcome. Almost since creating this repo that I'm not working with Python or GraphQL, so it's kind of hard for me to understand the problems people have and how the code fixes them without having any side effects.

I'm going to merge this and release a new version since many here already used this PR successfully.

tfoxy commented 1 year ago

v0.10.0 is up!

Please check if it works correctly. Had to upload manually because Travis no longer works.

Thanks @nikolaik for taking the time to make the code changes and everyone else for making sure this PR works correctly.

@sjdemartini I made you a contributor. Let me know if you need anything else. I will set up GitHub Actions now so that we have testing for PRs and automatic publishing.

nikolaik commented 1 year ago

Thanks for taking the time @tfoxy 🤗

sjdemartini commented 1 year ago

@tfoxy The v0.10.0 release seems to be working for me, resolving the graphql-core v3.2 compatibility issue. Thanks again for releasing, and thank you @nikolaik for this PR!