Optional Pagination #168

fruitymedley · 2024-05-24T21:43:39Z

Implements optional pagination as requested in #167

Description

Types of Changes

Issues Fixed or Closed by This PR

#167

Checklist

My code follows the code style of this project.
My change requires a change to the documentation.
I have updated the documentation accordingly.
I have read the CONTRIBUTING document.
I have added tests to cover my changes.
I have tested the changes and verified that they work and don't break anything (as well as I can manage).

sourcery-ai

Hey @fruitymedley - I've reviewed your changes and they look great!

Here's what I looked at during the review

🟡 General issues: 1 issue found
🟢 Security: all looks good
🟢 Testing: all looks good
🟢 Complexity: all looks good

Sourcery is free for open source - if you like our reviews please consider sharing them ✨

_{Help me be more useful! Please click 👍 or 👎 on each comment to tell me if it was helpful.}

src/strawberry_sqlalchemy_mapper/relay.py

botberry · 2024-05-24T21:44:31Z

Thanks for adding the RELEASE.md file!

Here's a preview of the changelog:

Add an optional function to exclude relationships from relay pagination and use traditional strawberry lists.
Default behavior preserves original behavior for backwords compatibilty.

Co-authored-by: sourcery-ai[bot] <58596630+sourcery-ai[bot]@users.noreply.github.com>

fruitymedley · 2024-05-28T19:45:10Z

@bellini666 Hi! Wanted to flag this to you for review since you look like the most recent active contributor, or if you know who would be best, I would greatly appreciate it!

bellini666

The solution in general looks good. I would just change it to be an argument instead, so it is more aligned to how we do those kind of configurations on strawberry.

Maybe @patrick91 and/or @erikwrede can also take a look at this?

bellini666 · 2024-05-29T08:26:53Z

src/strawberry_sqlalchemy_mapper/relay.py

@@ -146,6 +146,20 @@ async def resolve_async(nodes=nodes):
        return resolve_nodes(session)


+def exclude_relay(


I wonder if this would be better defined in mapper.type as an argument?

Also, the name itself IMO implies you are excluding relay functionality, which is not totally true. My suggestion would be do the argument and call it map_relations_as_list: bool = True, in which you can opt-out as False (would be even better to be the other way around, but then it would be a breaking change)

I could see a situation in which a type might have both relay and list. Would it make sense to add a class property in the same vein as __exclude__?

codspeed-hq · 2024-05-29T16:30:47Z

CodSpeed Performance Report

Merging #168 will not alter performance

_{Comparing fruitymedley:optional-pagination (02ee627) with main (d1fc069)}

Summary

✅ 1 untouched benchmarks

fruitymedley · 2024-06-11T15:18:25Z

I've made a few updates in accordance with the suggestions. Please review as available.

patrick91 · 2024-06-14T16:25:26Z

@fruitymedley would this work for you as a global configuration, instead of a configuration on the type itself?

novag · 2024-09-27T17:58:56Z

@fruitymedley Now that the project has a maintainer again in @Ckk3, perhaps we can get this merged. @Ckk3 What do you think about the global vs typed-based configuration?

erikwrede · 2024-09-27T19:30:31Z

Sorry, I'm a bit late to the party...

I agree with Patrick about having a global config on how to handle *-n relationships. Having a class attribute seems to be an easy fix, but I'm sceptical about the devX (no linting on field renames or other errors, the schema type will just change)

Another, perhaps more opinionated approach, would be to work with the global config and use an explicit field definition, like doNotUseRelayField: list[MyType] , or even list[auto] on each field that doesn't follow the global config. This is certainly a different approach, but I prefer that it's in line with the declarative style of other strawberry fields.

Another option would be to have a custom strawberry.sqlfield, similar to graphene-sqlalchemy's ORMField which gives an option to input the uselist param.

Generally, I think we should discuss a bit further on this and I'm open to all ideas.
@bellini666 how does strawberry Django handle this?

Ckk3

Thanks a lot, @fruitymedley , for your contribution! 🙌 This PR is looking great, and I just have a minor change related to the tests. I haven’t made any changes to the PR itself, but I wanted to note that if think we’ll need to update the documentation later to reflect the new functionality.

I'll also send a separate message soon with my thoughts on the global vs. type-based configuration discussion.

Looking forward to your feedback on the test updates!

Ckk3 · 2024-09-28T17:05:56Z

tests/test_mapper.py

+    assert len(mapped_department_type.__strawberry_definition__._fields) == 3
+    department_type_fields = mapped_department_type.__strawberry_definition__._fields
+    name = next(iter(filter(lambda f: f.name == "employees", department_type_fields)))
+    assert type(name.type) != StrawberryOptional


@fruitymedley, I believe the comparison should be done using isinstance(name.type, StrawberryOptional) instead of using !=. This aligns with the conventions we're following in the codebase when checking if a type is StrawberryOptional.

I came across a couple of instances in the code that confirm this pattern:

Here, we're using isinstance to check for StrawberryOptional in the type_.of_type.
Similarly, here, the same pattern is used for type_.

What do you think?

Ckk3 · 2024-09-29T05:22:43Z

@fruitymedley Now that the project has a maintainer again in @Ckk3, perhaps we can get this merged. @Ckk3 What do you think about the global vs typed-based configuration?

Hi everyone!

I don’t have a strong preference, but after reviewing the code and looking at the implementation in Strawberry Django, I think the current approach in this PR solves the issue. However, I believe a global configuration might be a better solution.

Please correct me if I’m wrong, @bellini666 , as I know you’re more familiar with the Django integration.

In Strawberry Django’s documentation, I noticed that pagination can be configured both at the type level and on individual fields.

In the Pagination Docs, we see pagination being set using pagination=True inside @strawberry_django.type.
Then, in the Fields Docs, under the "Field Customization" section, pagination is set on a specific field within a type.

If I understand correctly, this PR introduces the second approach (configuring pagination on individual fields within a type), but I agree with @erikwrede's point:

devX (no linting on field renames or other errors, the schema type will just change)

I also align with @bellini666 later comment. In my opinion, we should apply this configuration at the type level, similar to @strawberry_django.type(pagination=True), as it simplifies development. Afterward, we could discuss adding an option to configure it on individual fields if needed.

What do you all think? Please let me know if I misunderstood anything, I'm still getting familiar with the Django integration.

Ckk3 · 2024-10-02T13:43:22Z

Hi, @fruitymedley , we have fixed the CI, can you please update your branch from main?

bellini666 · 2024-10-06T14:34:36Z

@Ckk3 @erikwrede so, the way Strawberry Django currently does is to enforce a declarative style, in which you can use auto or properly annotate the relation.

For example:

@strawberry_django.type(Fruit):
class FruitType:
    ...


@strawberry_django.type(Color):
class ColorType:
    # Does not auto resolve to the type, as we don't keep a mapping internally.
    # Instead, one-to-one/many-to-one relations are mapped to `DjangoModelType` and
    # one-to-many/many-to-many are mapped to `list[DjangoModelType]`
    # `DjangoModelType` is a simple type that contains `pk` in it:
    # https://github.com/strawberry-graphql/strawberry-django/blob/main/strawberry_django/fields/types.py#L75
    fruits: auto

    # The recommended way for lists. The integration will still know that we have a
    # relation in the model and do what needs to be done regarding optimization
    fruits: list[Fruit]

    # The same as list above, but use relay connections instead.
    fruits: relay.ListConnection[Fruit]

I know that strawberry-sqlalchemy currently doesn't support the declarative style very well. Hence, a solution like the one proposed here is probably the way to go for now. Still, I'd love to see this integration moving forward to now only allow, but maybe actually enforce that in the future, so it is more aligned to strawberry style and recommendations in general.

Ckk3 · 2024-10-06T20:38:59Z

I agree with @bellini666 , lets continue with this PR when you can @fruitymedley
I also hope we can improve this lib later to make declarative style easier to develop.

Ckk3 · 2024-10-23T21:22:35Z

Hi @fruitymedley,

This PR has been inactive for two weeks. Are you still able to work on it? If not, please let me know so we can continue the development from this point.

…al-pagination

Ckk3 · 2024-10-31T16:10:23Z

I will continue the development of this PR.

codecov-commenter · 2024-10-31T16:13:23Z

Codecov Report

Attention: Patch coverage is 89.65517% with 3 lines in your changes missing coverage. Please review.

Project coverage is 85.51%. Comparing base (d1fc069) to head (02ee627).

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #168      +/-   ##
==========================================
+ Coverage   85.38%   85.51%   +0.12%     
==========================================
  Files          16       16              
  Lines        1615     1629      +14     
  Branches      128      139      +11     
==========================================
+ Hits         1379     1393      +14     
+ Misses        183      173      -10     
- Partials       53       63      +10

Ckk3 · 2024-11-11T23:14:27Z

I've fixed this PR tests and update the code with the new strawberry version.
Please take a look again when you can @patrick91 @bellini666 !

Ckk3 · 2024-11-12T13:09:27Z

I'll merge this PR.
Thanks @fruitymedley for your work!

botberry · 2024-11-12T13:09:58Z

Thanks for contributing to Strawberry! 🎉 You've been invited to join
the Strawberry GraphQL organisation 😊

You can also request a free sticker by filling this form: https://forms.gle/dmnfQUPoY5gZbVT67

And don't forget to join our discord server: https://strawberry.rocks/discord 🔥

fruitymedley added 3 commits May 24, 2024 14:55

Pre-commit-update

a3e6b9a

Add option to exclude relay pagination

711230a

Use standard resolver for exclude relay

f6d0b9a

sourcery-ai bot reviewed May 24, 2024

View reviewed changes

src/strawberry_sqlalchemy_mapper/relay.py Outdated Show resolved Hide resolved

fruitymedley and others added 4 commits May 24, 2024 16:45

Clarify docstring

10aa828

Co-authored-by: sourcery-ai[bot] <58596630+sourcery-ai[bot]@users.noreply.github.com>

Documentation

355f969

Add exclude relay test

dbc0c3e

Add release.md

d12e078

botberry added bot:has-release-file bot:release-type-minor labels May 28, 2024

bellini666 reviewed May 29, 2024

View reviewed changes

bellini666 requested review from patrick91 and erikwrede May 29, 2024 14:54

jojo and others added 3 commits May 30, 2024 12:20

Use property of class

49a69b8

Restore relay.py

140bc65

Upd test to use list

a2e5da0

patrick91 requested a review from bellini666 June 12, 2024 19:35

Ckk3 requested changes Sep 29, 2024

View reviewed changes

Ckk3 self-assigned this Sep 30, 2024

botberry removed the bot:has-release-file label Oct 10, 2024

Merge remote-tracking branch 'strawberry-sqlalchemy/main' into option…

61a8d01

…al-pagination

botberry added the bot:has-release-file label Oct 31, 2024

Ckk3 added 3 commits October 31, 2024 16:25

fix tests

0fe7cd2

adding type ignore on return

4b1728a

fix test

9a1f133

use isinstance on test

02ee627

bellini666 approved these changes Nov 12, 2024

View reviewed changes

Ckk3 approved these changes Nov 12, 2024

View reviewed changes

Ckk3 merged commit 7cd14e8 into strawberry-graphql:main Nov 12, 2024
19 checks passed

Ckk3 mentioned this pull request Nov 12, 2024

Make Pagination Optional #167

Closed

3 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Optional Pagination #168

Optional Pagination #168

fruitymedley commented May 24, 2024 •

edited by Ckk3

Loading

sourcery-ai bot left a comment

botberry commented May 24, 2024 •

edited

Loading

fruitymedley commented May 28, 2024

bellini666 left a comment

bellini666 May 29, 2024

fruitymedley May 29, 2024

codspeed-hq bot commented May 29, 2024 •

edited

Loading

fruitymedley commented Jun 11, 2024

patrick91 commented Jun 14, 2024

novag commented Sep 27, 2024

erikwrede commented Sep 27, 2024 •

edited

Loading

Ckk3 left a comment

Ckk3 Sep 28, 2024

Ckk3 commented Sep 29, 2024

Ckk3 commented Oct 2, 2024

bellini666 commented Oct 6, 2024

Ckk3 commented Oct 6, 2024

Ckk3 commented Oct 23, 2024

Ckk3 commented Oct 31, 2024

codecov-commenter commented Oct 31, 2024 •

edited

Loading

Ckk3 commented Nov 11, 2024

Ckk3 commented Nov 12, 2024

botberry commented Nov 12, 2024

		@@ -146,6 +146,20 @@ async def resolve_async(nodes=nodes):
		return resolve_nodes(session)


		def exclude_relay(

Optional Pagination #168

Optional Pagination #168

Conversation

fruitymedley commented May 24, 2024 • edited by Ckk3 Loading

Description

Types of Changes

Issues Fixed or Closed by This PR

Checklist

sourcery-ai bot left a comment

Choose a reason for hiding this comment

botberry commented May 24, 2024 • edited Loading

fruitymedley commented May 28, 2024

bellini666 left a comment

Choose a reason for hiding this comment

bellini666 May 29, 2024

Choose a reason for hiding this comment

fruitymedley May 29, 2024

Choose a reason for hiding this comment

codspeed-hq bot commented May 29, 2024 • edited Loading

CodSpeed Performance Report

Merging #168 will not alter performance

Summary

fruitymedley commented Jun 11, 2024

patrick91 commented Jun 14, 2024

novag commented Sep 27, 2024

erikwrede commented Sep 27, 2024 • edited Loading

Ckk3 left a comment

Choose a reason for hiding this comment

Ckk3 Sep 28, 2024

Choose a reason for hiding this comment

Ckk3 commented Sep 29, 2024

Ckk3 commented Oct 2, 2024

bellini666 commented Oct 6, 2024

Ckk3 commented Oct 6, 2024

Ckk3 commented Oct 23, 2024

Ckk3 commented Oct 31, 2024

codecov-commenter commented Oct 31, 2024 • edited Loading

Codecov Report

Ckk3 commented Nov 11, 2024

Ckk3 commented Nov 12, 2024

botberry commented Nov 12, 2024

fruitymedley commented May 24, 2024 •

edited by Ckk3

Loading

botberry commented May 24, 2024 •

edited

Loading

codspeed-hq bot commented May 29, 2024 •

edited

Loading

erikwrede commented Sep 27, 2024 •

edited

Loading

codecov-commenter commented Oct 31, 2024 •

edited

Loading