Skip to content

Schema preload_order cannot be overriden by passing ordered query #4535

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
sabiwara opened this issue Oct 30, 2024 · 5 comments
Open

Schema preload_order cannot be overriden by passing ordered query #4535

sabiwara opened this issue Oct 30, 2024 · 5 comments
Labels

Comments

@sabiwara
Copy link

sabiwara commented Oct 30, 2024

Elixir version

Elixir 1.17.3 (compiled with Erlang/OTP 27)

Database and Version

PostgreSQL 17.0

Ecto Versions

ecto 3.12.4

Database Adapter and Versions (postgrex, myxql, etc)

postgrex 0.19.2

Current behavior

# in Blog.Comment schema
   ...
   field(:likes, :integer)

# in Blog.Post schema
  ...
  has_many(:comments, Comment, preload_order: [asc: :likes])
  
# in Blog context
  query = from(p in Post, preload: [comments: ^from(c in Comment, order_by: [desc: :likes])])
  Repo.all(query)

# => results ordered by ascending likes
  comments: [%Comment{likes: 1, ...}, %Comment{likes: 42, ...}]

Expected behavior

I would have expected the order_by from the preload query (^from(c in Comment, order_by: [desc: :likes])]) to take precedence over the schema ordering preload_order: [asc: :likes].

If however, it is not a bug but an expected feature, would it be possible to achieve this in some other way?

  • setting some default ordering in the schema, which works most of the times
  • in some specific function, we need a different ordering overriding the default
@josevalim
Copy link
Member

I think you could argue for both behaviours. And that's exactly the issue with these "default" values: at some point you end-up wanting to customize them, and now you need to provide mechanisms to undo the defaults. Lost of information could be equally confusing. :( It partially feels the best course of action here is to remove preload_order altogether?

@sabiwara
Copy link
Author

and now you need to provide mechanisms to undo the defaults

Indeed, but to be fair in this case the mechanism already exists, in the sense that preloading using an ordered query is already in the absence of a default.

Lost of information could be equally confusing.

I agree in more complex cases involving query composition this could happen, but given that the "normal way" of preloading using the default is that simple (preload: [:comments]), people who go out of their way to write preload: ^(from ..., order_by: [...]) are probably nor expecting the default preload. Besides, making it impossible to override here makes it strictly less powerful that the other way around.

It partially feels the best course of action here is to remove preload_order altogether?

In my case this works indeed, so this is not a blocker. But I figured it would be nice to at least open an issue for the visibility in case somebody else ends up here, even if we keep the status quo 🙂

@josevalim
Copy link
Member

I am thinking for this one, we should apply our order, but it should come later. It probably doesn't make sense to override the user supplied one.

@sabiwara
Copy link
Author

@josevalim I'm not sure I understand your proposal, in the example above, would it mean asc: :likes, desc: :likes?
It feels just adding the order feels a bit too magical and unexpected.
Also, I'd argue it is less powerful than override: if you want to preload more things in a specific query, e.g. add asc: :id to the default asc: :likes, you can still do that explicitly as asc: :likes, asc: :id with the override approach?

In my mind, it makes sense to have a way to set sane default that you are able to overwrite, a bit like the :load_in_query option. You can use it to avoid always loading a field by default, but you still have the option of querying the field when you need it. It doesn't mean: "always load in query / never load in query, and make it impossible to override".

Another way to look at this could be to say: if somebody tries to override the preload order set in the schema at the query level, instead of silently ignoring it, we could warn or raise to make it clear it's not doing anything?

@josevalim
Copy link
Member

I'm not sure I understand your proposal, in the example above, would it mean asc: :likes, desc: :likes?

The opposite: desc: :likes, asc: :likes. Warning is also a great idea.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

2 participants