graphql-python / graphene-sqlalchemy

Graphene SQLAlchemy integration
http://docs.graphene-python.org/projects/sqlalchemy/en/latest/
MIT License
980 stars 226 forks source link

Enable sorting when batching is enabled #355

Closed PaulSchweizer closed 2 years ago

PaulSchweizer commented 2 years ago

This is a followup of our discussion from July 18th about batching and sorting. I took a deeper dive into the batching feature and have a better understanding of it now. Basically the core issue was, that the batching resolver did not take care of running the initial query. Instead, the initial query had to be run "manually" in a custom "resolve_" field as indicated in the tests: https://github.com/graphql-python/graphene-sqlalchemy/blob/dfee3e9417cdb8a6ec67b5cd79ee203ce4f72ed7/graphene_sqlalchemy/tests/test_batching.py#L67 That of course bypassed the "get_query" function in the resolver and thus also made sorting "impossible". Let me know what you think.

erikwrede commented 2 years ago

https://github.com/graphql-python/graphene-sqlalchemy/blob/ac57fd4ba78a786f36aa0c948596a66766c33ec7/graphene_sqlalchemy/enums.py#L147

Changing this to field_name would fix the errors with sqa-13. It seems like column_prop of Reporter has the name set to something similar to %(4447102096 anon)s at the time of sort enum generation. This causes def sort_enum_for_object_type to generate a sort enum with field names not allowed by GQL (%(4447102096 anon)s_ASC & %(4447102096 anon)s_DESC), causing the test failure. Couldn't figure out why get_schema does not reproduce the same behavior although the same models were used.

PaulSchweizer commented 2 years ago

I still need to investigate those failing tests

codecov-commenter commented 2 years ago

Codecov Report

Merging #355 (027ea0e) into master (bb7af4b) will increase coverage by 0.12%. The diff coverage is 96.38%.

@@            Coverage Diff             @@
##           master     #355      +/-   ##
==========================================
+ Coverage   96.37%   96.50%   +0.12%     
==========================================
  Files           9        9              
  Lines         773      801      +28     
==========================================
+ Hits          745      773      +28     
  Misses         28       28              
Impacted Files Coverage Δ
graphene_sqlalchemy/batching.py 95.45% <94.11%> (+1.70%) :arrow_up:
graphene_sqlalchemy/fields.py 99.15% <97.87%> (+0.13%) :arrow_up:
graphene_sqlalchemy/enums.py 97.80% <100.00%> (ø)

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

PaulSchweizer commented 2 years ago

The tests pass now, the remaining issue was that sqla1.2 is creating slighlty different select statments so my asserts where not working there. This is ready from my side now, please give it another look.

PaulSchweizer commented 2 years ago

It would be great @sabard, if you could write the test for the potential issue you have in mind. If it fails I will look into it and work on a solution.

erikwrede commented 2 years ago

@PaulSchweizer @sabard are we ready to merge?