fruitymedley commented 5 months ago

Implements optional pagination as requested in #167

Description

Types of Changes

[ ] Core
[ ] Bugfix
[x] New feature
[ ] Enhancement/optimization
[ ] Documentation

Issues Fixed or Closed by This PR

167

Checklist

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

botberry commented 5 months ago

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.

fruitymedley commented 5 months ago

@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!

codspeed-hq[bot] commented 5 months ago

CodSpeed Performance Report

Merging #168 will not alter performance

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

Summary

✅ 1 untouched benchmarks

fruitymedley commented 5 months ago

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

patrick91 commented 5 months ago

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

novag commented 1 month ago

@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 commented 1 month ago

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 commented 1 month ago

@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 commented 1 month ago

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

bellini666 commented 1 month ago

@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 commented 1 month ago

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 commented 3 weeks ago

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.

Ckk3 commented 2 weeks ago

I will continue the development of this PR.

codecov-commenter commented 2 weeks ago

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

```diff @@ 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 commented 1 week ago

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 commented 1 week ago

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

botberry commented 1 week ago

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 🔥

strawberry-graphql / strawberry-sqlalchemy

Optional Pagination #168

Description

Types of Changes

Issues Fixed or Closed by This PR

167

Checklist

CodSpeed Performance Report

Merging #168 will not alter performance

Summary

Codecov Report