Enforcing default ordering is too invasive (Django model <model> has to have a default ordering to be used in a Connection.)

[Flauschbaellchen]

Since version 3.2.1 a default ordering is enforced for models.
First of all, I want to say that I run into the problem which the PR #1495 tries to fix several times so the change is high appreciated.

Previously, I resolved this by always let the client to choose the ordering (as the pagination is, in my optinion, more a frontend-related problem and nothing the backend should care about).

Enforcing the ordering on the model level with Meta.ordering has (IMO) some disadvantages:

• The ORM would always order all queries independent of pagination is required/used or not. As the model is (obviously) used on other places than the GraphQL endpoints, this would reduce the overall performance of the app.
• Third-party apps/models would need to be adjusted, too (see below)

Additionally, the change feels more like a breaking change, as it requires the codebase to be adjusted.

• What is the current behavior?

For our own models, we had a base class which was adjusted in the following

class MyBaseModel(models.Model):
    class Meta:
        abstract = True
        ordering = ["pk"]

But as we do not only use our own models for the API, we had the problem that for third-party models the same error occurs:

TypeError: Query fields cannot be resolved.
Django model contenttypes.ContentType has to have a default ordering to be used in a Connection.

• What is the expected behavior?

As said before, the intention of the change itself is high appriciated.
However, I would like to have a more non-invasive approach.

Would it be possible to just "fall back" to an ordering of pk in case Meta does not specify it in the context of the Connection?
Idk if this is something the user should be warned about or if it should be even be a setting the developer can adjust if it is "enforced", "fall-back-to-pk" or "silenced".

• Please tell us about your environment:
  • Version: Django 4.2.7
  • Platform: Python 3.10.x

[kiendang]

@Flauschbaellchen did setting ordering = ["pk"] actually enforce an ordering? My first approach for #1518 was to set ordering default to ["pk"] too but after searching through both graphene-django and graphene codebase I couldn't find any reference to ordering, so I'm not sure if setting ordering actually does anything. I could miss something though.

[Flauschbaellchen]

Hi @kiendang, I'm unsure if we are talking about the same :)
I've used Meta.ordering as this seems to be the option the PR checks and validates. As soon as this was set, the validation returned successfully.
Please see in my example above, that I used models.Model, the base class of the Django models, not DjangoObjectType, which is provided by graphene-django.

When using Meta.ordering, it is used as the default for all queries.
Thus, MyModel.objects.all() would also use the ordering specified in the Meta class, even if not used within an GraphQL endpoint/pagination request.

It's also likely that I missed something - that actually another ordering should be used, however, I was unable to get past the validation except when specifying it directly on the Django model.
And the error message as well tells you that the ordering directly on the model is missing, see here.

[kiendang]

I see. Thanks for the clarification. Looks like setting ordering = ["pk"] as default in case it's not set is the way to go here. Would you like to submit a PR?

[Flauschbaellchen]

@kiendang Sure, I can try to get something together.

Can we talk about the general implementation first?
After some thinking, I'd propose the following:

Whenever a request is being processed, the ordering is checked:

1. Take the order in the following "order" 😆 :
   1. what the user specified/requested
   2. what is specified in the model's graphql type DjangoObjectType.Meta.ordering (see below)
   3. fall back to model.Model.Meta.ordering of the model itself.
   4. no ordering specified
2. Check if the keys (as there can be multiple) contain an unique attribute:
   • Keys do contain a unique key => pass
   • Keys do not contain a unique key => error-or-warning

Checking for unique keys is important as to only check for an order is not enough.
Example: When I order by name, and name is not unique, I would have the same issue as before, that it is not deterministic which instance comes first.
This is currently the case for #1495, too, as it only checks for existence of the ordering, but not "what".

Error or warning:
In case no unique key has been specified, I'd like to implement two possibilities which can be configured through a setting (naming tbd).

• warning: add pk to the end of the list for ordering to make it unique, log warning.
• errror: raise error telling the dev that no order was given and it is not deterministic.

In this case, it is non-invasive and can be set to "error" in development and "warning" in production.
I would set it to 'warning' by default to not break existing code.
Also, in difference to 3.2.0, the ordering is validated on runtime and not startup, thus a possible exception would be hidden from the developer at first, making "error" a bad choice for production environments.

Location for "ordering":
As the model itself is not a good place for specifying an ordering, I would like have the setting on the DjangoObjectType.Meta. Best place would be on the connection class itself, but than it would be very hard for the developer to specify them as everytime he wants to do this, connection_class would needs to be set and inherit.
Thus choosing DjangoObjectType, example:

class MyModelType(DjangoObjectType):
    class Meta:
        model = MyModel
        ordering = ["name", "pk"]

As even now, the developer could specify a non-unique ordering, the validation should still be executed as described above.

What do you think?

[oliverhaas]

Hi. I'm currently upgrading a project and I encountered this issue.

I think your considerations are valid and it would work as far as I can see, but the approach is a bit convoluted in my opinion. The most common case for me is that I don't have any ordering and don't need any. Do I have to specify something explicitly to suppress errors/warnings?

So I don't think django-graphene should enforce default ordering at all, and maybe a warning might be already too much. Most DBs don't enforce ordering per default for good reasons; neither should the first layer on top of it (the model) per default. 95% of the time I don't need it. I admit that I personally had to learn about (not) ordering and pagination "the hard way", but it's a bit too much of a guardrail as of 3.2.1 and the suggestions here. Just my opinion; some of it based on what I see elsewhere (e.g. in the DBs as mentioned above).

[Flauschbaellchen]

@oliverhaas I have the same opinion as you, as I've mentioned before:

Previously, I resolved this by always let the client to choose the ordering (as the pagination is, in my optinion, more a frontend-related problem and nothing the backend should care about).

I would be totally fine if the changes are rolled back alltogether and only within the doc mentioned that you need to remember to specify an ordering.

However, trying to help those users is also fine with me, that's why I proposed the aforementioned solution which does not enforce any ordering by default - but yes, it would require some changes to the code and might be too complex to maintain.
It does not give you a simple answer in case your ordering is messed up because you would always say "if-then-but-not-if-bla-foo" over multiple layers (Graphene -> Model -> Database).

Your comparison with the database is very good. It's the same issue: The ordering is random, until you specify something and really care about it.... the DB does not error out or reminds you on every query that it might not be the results you actually wanted. 🤷
That (most?) databases order by the primary key is more like a convention and is based on internal performance improvements, than something you can really rely on.

/cc @kiendang

[m-reichle]

I'm currently running into this issue trying to make my (django.auth) User model visible via graphene. Is there a way to provide the ordering from within the Node or resolver function or do I need to change my project to a custom user model?

[kiendang]

I would be totally fine if the changes are rolled back alltogether

@oliverhaas @Flauschbaellchen the rollback has already been merged into dev #1518. Can one of you (or anyone else) make a PR bump to 3.2.2 similar to #1512? I'll merge and make a new 3.2.2 release.