Skip to content

Add default order by ID to API list queries#7424

Merged
mrcasals merged 3 commits intodecidim:developfrom
mainio:fix/api-default-order
Feb 22, 2021
Merged

Add default order by ID to API list queries#7424
mrcasals merged 3 commits intodecidim:developfrom
mainio:fix/api-default-order

Conversation

@ahukkanen
Copy link
Copy Markdown
Contributor

@ahukkanen ahukkanen commented Feb 19, 2021 •
edited
Loading

🎩 What? Why?

Currently the list queries in API are not working consistently because they do not define a default order for the query. This can be seen especially when you have multiple pages of records in the API.

PostgreSQL works in mysterious ways but recently we discovered that the default sort order can be randomly changed in case the records are updated. This affects the API list queries in case we have multiple pages of records. If the following happens, we can expect random ordering and possible duplicates between the different pages of the records:

  • Have lots of proposals in the DB (multiple pages in the API)
  • Call the API to list the proposals without a defined sort order, iterate over every page of the result
  • During the API call iteration, run a script at the same time that randomly updates the proposals
  • When you run these two simultaneously enough many times, you should expect to see duplicates in the results and possibly some "disappearing" items that have moved to another page during the record updates.

To fix this problem, let's add a default order by ID to the API list query. This won't affect the queries that have defined order through the GraphQL query, as it will be added at the last stage of the query when it becomes a secondary sort criteria in case there is already another sort criteria added before it.

Testing

  • Create 1000 proposals
  • Create a script to fetch all proposals from the API page by page
  • Create a script that endlessly runs an update to the decidim_proposals_proposals table's records selected randomly. It does not matter what the update is (you could update e.g. the updated_at column), as long as it's done against the main table being queried through the API.
  • Start the API fetching script
  • Start the script that randomly updates the records (running simultaneously with the API calls)

If the API fetching ends too fast, increase the amount of records in the DB. If you didn't receive any duplicates (or missing records), re-run the scripts as many times as you will bump into the issue.

📋 Checklist

  • ❓ CONSIDER adding a unit test if your PR resolves an issue.
  • ✔️ DO check open PR's to avoid duplicates.
  • ✔️ DO keep pull requests small so they can be easily reviewed.
  • ✔️ DO build locally before pushing.
  • ✔️ DO make sure tests pass.
  • ✔️ DO make sure any new changes are documented in docs/.
  • ✔️ DO add and modify seeds if necessary.
  • ✔️ DO add CHANGELOG upgrade notes if required.
  • ✔️ DO add to GraphQL API if there are new public fields.
  • ✔️ DO add link to MetaDecidim if it's a new feature.
  • ❌AVOID breaking the continuous integration build.
  • ❌AVOID making significant changes to the overall architecture.

@mrcasals
Copy link
Copy Markdown
Contributor

mrcasals commented Feb 19, 2021

What about adding a default scope to all ApplicationRecord classes to always sort by ID by default? This would solve the issue for the API, and also for any other collection (thinking about admin dashboards, for example).

What do you think?

@ahukkanen
Copy link
Copy Markdown
Contributor Author

ahukkanen commented Feb 19, 2021 •
edited
Loading

@mrcasals Yes, I was thinking the same but there are some problems with that:

  • Rails 6 provides a implicit_order_column which does this BUT Decidim is still using Rails 5 until Rails 6 upgrade #7086 is merged. I'm also not sure if it works for all queries, according to some resources, it only works for a subset of methods. ("This has impact on methods like first, last and many more where implicit ordering is used."). E.g. .all is not in the list of the finder methods.
  • Using a default_scope is not a great idea because it gets added first. If you would add default_scope { order(:id) } to ApplicationRecord, you would break ordering of any records in the app. What would happen e.g. if you want to then order by :published_at would be similar to calling Record.published.order(:id).order(:published_at). This would not obviously work.

So, if you have ideas how to do that, let me also know.

@mrcasals
Copy link
Copy Markdown
Contributor

mrcasals commented Feb 22, 2021

@ahukkanen we could overwrite the ActiveRecord#all method and add the order there, but this might open a can of worms...

Let's stick with your solution for now!

@mrcasals
Copy link
Copy Markdown
Contributor

mrcasals commented Feb 22, 2021

I'm rerunning the generators job!

@ahukkanen
Copy link
Copy Markdown
Contributor Author

ahukkanen commented Feb 22, 2021

@mrcasals Overriding ActiveRecord::Base.all won't cover all the cases. E.g. if you do Decidim::Proposals::Proposal.where(state: "accepted"), it won't be covered by that.

The only way to achieve this reliably would be to override the ActiveRecord::Relation class and its ordering logic but doing that you will end up pretty deep in Rails core, and you also probably have to override some things in ActiveRecord::Base.

I would suggest against that as it's probably going to be way easier to just fix it case by case where it needs to be fixed, e.g. in the admin listing views that you mentioned. The proposals admin view, for example, is already sorted by position by default:

decidim/decidim-proposals/app/controllers/concerns/decidim/proposals/admin/filterable.rb

Line 20 in e12c833

return collection.order(:position) if current_component.settings.participatory_texts_enabled?

@mrcasals
Copy link
Copy Markdown
Contributor

mrcasals commented Feb 22, 2021

The proposals admin view, for example, is already sorted by position by default

Only if the component is configured as a participatory text component, otherwise it doesn't seem to be ordered.

But anyway, let's not dig into overwriting stuff, since it's a source of problems, and as you say let's fix the issues one by one.

This PR is a nice start!

@mrcasals
Copy link
Copy Markdown
Contributor

mrcasals commented Feb 22, 2021

It seems there's an issue with the generators, I'll merge this PR since the code doesn't affect it and fix the issue in a different PR.

@mrcasals mrcasals merged commit e3557cc into decidim:develop Feb 22, 2021
@mrcasals mrcasals mentioned this pull request Feb 22, 2021
Merged
@ahukkanen ahukkanen deleted the fix/api-default-order branch February 22, 2021 08:47
@ahukkanen
Copy link
Copy Markdown
Contributor Author

ahukkanen commented Feb 22, 2021

Only if the component is configured as a participatory text component, otherwise it doesn't seem to be ordered.

Yeah, true that.

For the admin case I believe it is still rather easy to fix:

decidim/decidim-admin/app/controllers/concerns/decidim/admin/filterable.rb

Line 34 in ff3e693

@query ||= base_query.ransack(ransack_params)

(this is where all the filterable admin views get their results from)

And this is how you can add default sorting with that:
https://github.com/activerecord-hackery/ransack/wiki/Sorting-in-the-Controller

@mrcasals
Copy link
Copy Markdown
Contributor

mrcasals commented Feb 22, 2021

Thanks for checking this out! I'll try to find time to work on this, but I can't promise a date. Let me open an issue to track it!

@mrcasals mrcasals mentioned this pull request Feb 22, 2021
Open
This was referenced Feb 22, 2021
Merged
Merged
entantoencuanto added a commit that referenced this pull request Feb 25, 2021
@entantoencuanto
Merge branch 'develop' into private_participants_share_token
d177144 
* develop: (232 commits)
  Add Votings to Open Data export (#7388)
  Add order in not highlighted assemblies by weight (#7444)
  Resort Menus (#7460)
  Polling Officer Zone (#7439)
  Fix session timeout when using multiple windows or tabs (#7459)
  Fix display of debates with multiple dates (#7393)
  chore: split election tests (#7451)
  style: make selected values primary labels with delete button (#7448)
  Fix and tests to avoid registered users being invited again (#7392)
  Migrate Admin menus to Menu Registry (#7368)
  New Crowdin updates (#7338)
  Bump to carrierwave 2.2.0 (#7441)
  Voting: show callout when Polling Stations miss Polling Officers (#7417)
  Further default orders for the API (#7436)
  Fix random order inconsistencies (#7437)
  Ensure Rails is locked to 5.2.4.x series (#7430)
  Add default order by ID to API list queries (#7424)
  Update dependencies (#7422)
  Ignore warning on CI when no artifacts to upload (#7420)
  Filter and search polling officers (#7411)
  ...
@ahukkanen ahukkanen mentioned this pull request May 30, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

module: api

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@ahukkanen @mrcasals