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

Bugfix for :preload, and support ORDER BY query #12

Closed

Conversation

soichiro-nishizawa
Copy link

@soichiro-nishizawa soichiro-nishizawa commented Mar 23, 2021

Hello.
This PR is bugfix for :preload, and support ORDER BY query.

About bugfix

The behavior of Ecto's preload is that when you try to preload a table that has_many associations like Shipper and Order, Ecto will add the order_by of the associated key in the query to retrieve the Order table.

Ecto expects the list to be sorted by this order_by, but if an unsorted list is returned, the association does not seem to work correctly.
Specifically, it seems to be associating the first association key found in the list up to the point where the key is contiguous.

example

Model.Shipper
|> preload([_x], :orders)
|> Repo.all()

unsorted result

[
  %Northwind.Model.Shipper{
    __meta__: #Ecto.Schema.Metadata<:loaded, "shippers">,
    company_name: "Speedy Express",
    orders: [
      %Northwind.Model.Order{ ... },
      %Northwind.Model.Order{ ... }
    ],
    phone: "(503) 555-9831",
    shipper_id: 1
  },
  %Northwind.Model.Shipper{ ... },
  %Northwind.Model.Shipper{ ... }
]

sorted result

[
  %Northwind.Model.Shipper{
    __meta__: #Ecto.Schema.Metadata<:loaded, "shippers">,
    company_name: "Speedy Express",
    orders: [
      %Northwind.Model.Order{ ... },
      %Northwind.Model.Order{ ... },
      %Northwind.Model.Order{ ... },
      %Northwind.Model.Order{ ... },
      %Northwind.Model.Order{ ... },
      ...
      ...
      ...
    ],
    phone: "(503) 555-9831",
    shipper_id: 1
  },
  %Northwind.Model.Shipper{ ... },
  %Northwind.Model.Shipper{ ... }
]

soichiro.nishizawa added 2 commits March 18, 2021 14:16
@evadne
Copy link
Owner

evadne commented Mar 25, 2021

Thank you very much for this patch 🧠

@rupurt
Copy link

rupurt commented Jul 28, 2021

@evadne is there anything else required to get this patch merged? I've noticed that order by doesn't currently work on develop.

@evadne evadne closed this in 0722d4e May 20, 2022
@evadne
Copy link
Owner

evadne commented May 20, 2022

This is fixed in develop

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.

3 participants