Skip to content
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

feat: add preload_order for has_one #4505

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

saveman71
Copy link
Contributor

@saveman71 saveman71 commented Sep 3, 2024

Hello there,

Stumbling on https://elixirforum.com/t/ecto-has-one-sort-association/62923 earlier this afternoon and after considering what adding support for preload_order to the has_one assoc would entail, I gave it a shot.

I know this is probably a solution to a bad architecture, but it could save us the trouble of converting our assoc to an has_many then adding a limit and/or always considering getting the latest object in business logic.

A brief explanation on why we need this:

  • We have Documents

  • Documents can be signed and thus some SigningRequests are attached

  • Only one request is active at any time, we will never show multiple requests in the UI

  • However, some can fail, etc.

  • Up until recently we had the following:

    has_one :signing_request, SigningRequest, where: [state: {:in, [Statuses.created(), Statuses.signed()]}]

    Which worked but it hid any failed SigningRequest.

    When noticing this, I didn't thing too much about it and removed the filter. However, since the sort is undefined, in some cases if you have a failed signature and a successful one, it returns the first found, the failed one.

  • To fix this, with preload_order support we could do:

    has_one :signing_request, SigningRequest, preload_order: [desc: :id]

    (or on inserted_at, etc.) which would ensure that we always return the latest one, regardless of it's status (if a user chose to overwrite a successful signature, why not)

This PR is meant as an RFC, and the new case is not pretty (its here to avoid Ecto.Association.BelongsTo which doesn't have preload_order). Documentation would follow if accepted.

Made a quick tests and it seems to work for our use case

Cheers!

@saveman71 saveman71 force-pushed the feat/preload_order-for-has_one branch from b1164b0 to b116bd7 Compare September 3, 2024 14:23
@greg-rychlewski
Copy link
Member

Hi @saveman71 ,

Regarding the example you gave, what do you mean by the request failed? Basically I'm not sure I understand what was your original reason for adding the filter

@saveman71
Copy link
Contributor Author

saveman71 commented Sep 3, 2024

To be sure, I was talking about the business request entity, not Ecto's request :) I amended my first message to make it more explicit

Regarding my example:

A request can have 3 states, created/failed/signed

basically we want to display them by order of importance :

created > signed > failed

so the previous developer, having no other option filtered them by [created, signed] and just ignored the failed requests, but this meant that a once a pending request became failed request it just disappeared. I wanted to bring them back, but then this made sense to retry the signature, and now we have multiple SigningRequest per Document and we have the undefined sort.

This was acceptable to just never display the failed requests, but a better approach is to consider the latest SigningRequest - by id or timestamp - instead of by status.

The preload_order would allow this.

@josevalim
Copy link
Member

The big question is why do you need it to be an association?

If you only need to query data, it is better to write a regular query, and not pollute fields in the parent struct with associations. I would only recommend associations when they are an essential part of your schema (you always access/preload them) or when you want to both read and write (via changesets) to them. Using associations as query helpers is definitely an anti-pattern.

@saveman71
Copy link
Contributor Author

saveman71 commented Sep 3, 2024

Using associations as query helpers is definitely an anti-pattern

We indeed never write to this assoc, if this is what you mean. In that case well... we definitely overuse this pattern. Wasn't the preload_order of has_many added for that same reason as well?

I don't think we're the only ones doing this out there

EDIT: when rendering this document, it's also helpful to be able to access the signing request via document.signing_request instead of passing around two structs (and in many cases, it would be more than 2)

@josevalim
Copy link
Member

You can use preload_order with a has_many and write to the has_many as usual. preload_order in a has_one most often indicates using an association exclusively from querying. At least I can't come up with counter-examples right now.

@greg-rychlewski
Copy link
Member

One thing that is nice with associations is you get the parallel preload for free. I could see that as a reason to use it for querying only.

But in this case I actually am hesitant about the idea of adding preload_order to has_one. It is common for people to do join preload for 1:1 relationships and separate preload query for 1:n. So this could hide the fact that it is not a 1:1 relationship and cause people to choose the less performant option.

We recently added support to preload subqueries. You could probably make a subquery with what you want and preload that .

@saveman71
Copy link
Contributor Author

saveman71 commented Sep 3, 2024

We recently added support to preload subqueries.

Could you please point me to the documentation? Is it at the schema level, or when you give the preload options to a query? We've been doing the later for some time (on other schemas), but it would be nice to have some defaults.

@josevalim you're right that the use case is debatable. It also questions what happens if you edit that assoc, it would update the "ordered one" which if I'm playing the devil's advocate is better than to edit the random entry you got since it doens't ensure that there's only 1 matched record.

We'll also discuss internally what we like best, if we go the other way (manual query), we won't need this, however, when looking around I can find other discussions linked to that "situation":

https://elixirforum.com/t/ecto-has-one-sort-association/62923 (linked if first post)
https://stackoverflow.com/q/67745506
https://groups.google.com/g/elixir-ecto/c/K-colmFz1dc (not directly related but good context)

@greg-rychlewski
Copy link
Member

greg-rychlewski commented Sep 3, 2024

I think we didn't update the documentation, apologies. But you should be able to do something like this

squery = from c in Comment, where: c.post_id == parent_as(:post).id, c.order_by: [desc: :id], limit: 1
from p in Post, as: :post, lateral_join: s in subquery(squery), on: true, preload: [one_comment: s]

Where one_comment is an association in the Post schema.

edit:

Or perhaps you can have a regular has_many assoc so you can load/change all of them and then use the above query to return a list of 1 to the same assoc when you want to do that.

@JoeriDijkstra
Copy link

I would very much like this to go into production, I don't understand what is so bad about this structure comparing to the has_many.

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

Successfully merging this pull request may close these issues.

4 participants