Enable sorting when batching is enabled #355

PaulSchweizer · 2022-07-23T23:19:16Z

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:

graphene-sqlalchemy/graphene_sqlalchemy/tests/test_batching.py

Line 67 in dfee3e9

def resolve_articles(self, info):

That of course bypassed the "get_query" function in the resolver and thus also made sorting "impossible".
Let me know what you think.

PaulSchweizer · 2022-07-23T23:22:17Z

graphene_sqlalchemy/batching.py

+# Cache this across `batch_load_fn` calls
+# This is so SQL string generation is cached under-the-hood via `bakery`
+# Caching the relationship loader for each relationship prop.
+RELATIONSHIP_LOADERS_CACHE = {}


We store the loaders per relationship so each item accesses the same cached result ensuring that the selectin is only triggered once

PaulSchweizer · 2022-07-23T23:22:50Z

graphene_sqlalchemy/batching.py

+            )
+            RELATIONSHIP_LOADERS_CACHE[relationship_prop] = loader
+        else:
+            loader.loop = get_event_loop()


I'm not 100% sure about this. Do any of you see issues with this?

Why do we need the get_event_loop() call in the else part? Isn't that initially covered by aiodataloader already, and shouldn't the cache also get a full reset if the event loop changes (implying thread restart/change)?

aiodataloader recommends to create a new loader for each request, this would keep the loaders between different queries, right?

Makse sense, I changed it

September update: There is also preliminary work in Graphene to vendor the DataLoader from aiodataloader into Graphene, and also change it's __init__ behavior to not ask for an even loop so early on init. Just warning because that might touch the same points we are touching here.

graphene_sqlalchemy/fields.py

PaulSchweizer · 2022-07-23T23:25:38Z

graphene_sqlalchemy/tests/models.py

+    )
+
+
+class ArticleReader(Base):


Had to add some deeper nesting in the test data for more robust tests

I added a Tags class in #357. Will see if there's a way to consolidate over there after this is merged.

PaulSchweizer · 2022-07-23T23:28:55Z

graphene_sqlalchemy/tests/test_batching.py

+            batching = True
+            connection_class = Connection
+
+    class Query(graphene.ObjectType):


As mentioned, the original test schema defines custom resolve_ methods which I find odd, but maybe people use it like that. We use it like this.

Also using it the way you suggest it, LGTM. Original schema doesn't use relay - without the relay connection field classes your resolvers don't automatically generate the SQA-Queries.

PaulSchweizer · 2022-07-23T23:29:33Z

graphene_sqlalchemy/tests/test_batching.py

    )
    session.add(reporter_1)
    reporter_2 = Reporter(
-      first_name='Reporter_2',
+        first_name='Reporter_2',


unfortunately there were some flake8 errors I had to fix, so ignore these indentation changes

erikwrede · 2022-07-26T19:37:09Z

graphene-sqlalchemy/graphene_sqlalchemy/enums.py

Line 147 in ac57fd4

asc_name = get_name(column.key, True)

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.

…pLoader between queries

…re the enum will get the actula field_name

PaulSchweizer · 2022-07-31T16:03:07Z

I still need to investigate those failing tests

…qla1.2

codecov-commenter · 2022-08-06T10:10:45Z

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%)`	⬆️
graphene_sqlalchemy/fields.py	`99.15% <97.87%> (+0.13%)`	⬆️
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 · 2022-08-06T10:22:17Z

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.

erikwrede

Tested this locally. Working nicely!
I suggested a few minor adjustments. Please check my comments.

graphene_sqlalchemy/batching.py

graphene_sqlalchemy/enums.py

graphene_sqlalchemy/fields.py

erikwrede · 2022-08-07T16:53:23Z

graphene_sqlalchemy/tests/test_batching.py

+            batching = True
+            connection_class = Connection
+
+    class Query(graphene.ObjectType):


Also using it the way you suggest it, LGTM. Original schema doesn't use relay - without the relay connection field classes your resolvers don't automatically generate the SQA-Queries.

…it gets passed.

sabard

Aside from Erik's comments, all LGTM! Let me know if you'd like me to write a test case for custom ORMField attributes.

graphene_sqlalchemy/enums.py

sabard · 2022-08-15T17:03:22Z

graphene_sqlalchemy/tests/models.py

+    )
+
+
+class ArticleReader(Base):


I added a Tags class in #357. Will see if there's a way to consolidate over there after this is merged.

PaulSchweizer · 2022-08-15T18:42:26Z

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.

sabard

Looks like all works well for custom ORMField names.

Off topic, but is sorting supported on non-Relay-style batching? I wasn't able to get it to work; maybe deserves its own test if supported?

graphene_sqlalchemy/tests/test_batching.py

erikwrede · 2022-09-09T15:17:11Z

@PaulSchweizer @sabard are we ready to merge?

Enable sorting when batching is enabled

ac57fd4

PaulSchweizer requested review from sabard, erikwrede and palisadoes July 23, 2022 23:19

PaulSchweizer commented Jul 23, 2022

View reviewed changes

graphene_sqlalchemy/fields.py Show resolved Hide resolved

PaulSchweizer commented Jul 23, 2022

View reviewed changes

PaulSchweizer added 2 commits July 31, 2022 17:41

Deprecate UnsortedSQLAlchemyConnectionField and resetting Relationshi…

6417061

…pLoader between queries

Use field_name instead of column.key to build sort enum names to ensu…

535afbe

…re the enum will get the actula field_name

sabard mentioned this pull request Aug 2, 2022

Draft: Add filter tests for discussion #356

Closed

Adjust batching test to honor different selet in query structure in s…

b91fede

…qla1.2

erikwrede mentioned this pull request Aug 7, 2022

feat(async): add support for async sessions #350

Merged

2 tasks

erikwrede requested changes Aug 7, 2022

View reviewed changes

erikwrede mentioned this pull request Aug 12, 2022

v3.0 Release #348

Open

9 tasks

Ensure that UnsortedSQLAlchemyConnectionField skips sort argument if …

515060b

…it gets passed.

sabard reviewed Aug 15, 2022

View reviewed changes

add test for batch sorting with custom ormfield

d6746e3

sabard reviewed Aug 15, 2022

View reviewed changes

graphene_sqlalchemy/tests/test_batching.py Show resolved Hide resolved

graphene_sqlalchemy/tests/test_batching.py Show resolved Hide resolved

Merge branch 'master' into enable-sorting-for-batching

027ea0e

sabard self-requested a review September 9, 2022 14:49

sabard approved these changes Sep 9, 2022

View reviewed changes

sabard requested a review from erikwrede September 9, 2022 14:53

erikwrede approved these changes Sep 9, 2022

View reviewed changes

erikwrede merged commit 43df4eb into master Sep 9, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Enable sorting when batching is enabled #355

Enable sorting when batching is enabled #355

PaulSchweizer commented Jul 23, 2022

PaulSchweizer Jul 23, 2022

PaulSchweizer Jul 23, 2022

erikwrede Jul 24, 2022

PaulSchweizer Jul 31, 2022

flipbit03 Sep 6, 2022

PaulSchweizer Jul 23, 2022

sabard Aug 15, 2022

PaulSchweizer Jul 23, 2022

erikwrede Aug 7, 2022

PaulSchweizer Jul 23, 2022

erikwrede commented Jul 26, 2022

PaulSchweizer commented Jul 31, 2022

codecov-commenter commented Aug 6, 2022 •

edited by codecov bot

Loading

PaulSchweizer commented Aug 6, 2022

erikwrede left a comment

erikwrede Aug 7, 2022

sabard left a comment

sabard Aug 15, 2022

PaulSchweizer commented Aug 15, 2022

sabard left a comment

erikwrede commented Sep 9, 2022 •

edited

Loading

Enable sorting when batching is enabled #355

Enable sorting when batching is enabled #355

Conversation

PaulSchweizer commented Jul 23, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

erikwrede commented Jul 26, 2022

PaulSchweizer commented Jul 31, 2022

codecov-commenter commented Aug 6, 2022 • edited by codecov bot Loading

Codecov Report

PaulSchweizer commented Aug 6, 2022

erikwrede left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sabard left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

PaulSchweizer commented Aug 15, 2022

sabard left a comment

Choose a reason for hiding this comment

erikwrede commented Sep 9, 2022 • edited Loading

codecov-commenter commented Aug 6, 2022 •

edited by codecov bot

Loading

erikwrede commented Sep 9, 2022 •

edited

Loading