search

graphql-python / graphene-django

Build powerful, efficient, and flexible GraphQL APIs with seamless Django integration.
http://docs.graphene-python.org/projects/django/en/latest/
MIT License
4.28k stars 766 forks source link

Enforcing model default ordering (#1495) may have been overzealous #1516

Closed gsvr closed 4 months ago

gsvr commented 5 months ago

1495 - released in 3.2.1 forces every model in a project to have a default ordering specified.

The impact of this is not trivial:

  1. Each project using this will have to set ordering defaults on every model that a DjangoConnectionField is being applied to (in only one of my projects this is over 100 changes)
  2. Ordering is not a free operation (please see https://docs.djangoproject.com/en/5.0/ref/models/options/#ordering)

I do agree the problem raised in #1495 is a good catch, but I think a better approach to handling this could be:

  1. If there is no default ordering specified, use the models primary key as the default for sorting, since every model should have one (https://docs.djangoproject.com/en/5.0/topics/db/models/#automatic-primary-key-fields) and/or
  2. Warn the user of potential problems rather than enforcing model updates

Please feel free to disagree with me if I'm misunderstanding or overlooking something here, in the meantime I'm pinning to 3.2.0 😊

kiendang commented 5 months ago

Tagging @tcleonard. @gsvr I agree with this. Requiring declaring ordering does break backward compatibility and if we do require it it should not have been a patch release. I think solution 1 is best here, setting default ordering to pk if it's not declared. @tcleonard @gsvr do one of you mind submitting a PR for this?

gsvr commented 5 months ago

@kiendang I looked at opening a PR, but with my limited knowledge of the codebase I couldn't easily understand what the correct place would be to apply the default ordering.

I'd love to contribute here but I think it would probably take me too long to fully understand all the nuances and class inheritances here before I can come up with something useful. If you could point me in the right direction I might be able to attempt a change, but it might be worth reverting the original change in the meantime.

pecheneff commented 5 months ago

Totally agree with @gsvr . We also have a large project with many models so using default ordering everywhere is not free. Using 3.2.0 is the only way for now to avoid issues. Please avoid adding breaking changes like this to a patch versions. Thanks in advance.