Skip to content
New issue

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

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

Already on GitHub? # to your account

perf(core,mentions): limit mentionedBy post relation results #3780

Merged
merged 12 commits into from
Apr 19, 2023

Conversation

SychO9
Copy link
Member

@SychO9 SychO9 commented Mar 31, 2023

Fixes #3705

Changes proposed in this pull request:

  • Limits loaded mentionedBy posts to 4, and allows viewing all the rest in a paginated modal list.
    • Uses a third-party package to introduce the ability to limit a relation's results per model when eager loading.
    • Applies the feature on the mentionedBy relation by introducing an eager loading callback.
    • Eager loads the total count of mentionedBy.
    • Adds a mentionedPost filter to grab posts that mention X post.
    • Adapts the UI to use the new information and pagina mentionedBy.
    • Adds backend tests.

Reviewers should focus on:

  • Why use staudenmeir/eloquent-eager-limit?: by default, it is not possible to limit a relation's results per model, the point of eager loading is that it queries related models of all models in one query (per relation). If you apply a limit on the relation without package, what you get isn't a limited relation result per model, but a limited relation result for all models. The package introduces the ability to limit relations per model, with a trait.
  • Why remove the FilterVisiblePosts class?: it had one goal, to remove invisible mentionedBy posts from the relation results, by querying the visible IDs of them. This PR just uses a visibility scoper directly on the relatin when limiting results (in an eager load).
  • Why the changes to AbstractModel and Collection?: by default Laravel applies a table alias in the subquery of loading an aggregate (count) t void conflicts since mentionedBy is a Post <-> Post relation, the problem with that is hat it creates conflicts when using visibility scoping, so the changes here apply an alias on the parent query instead. Readmore details on the new Collection class phpdoc.

Screenshot
Screenshot from 2023-03-31 20-09-21
Screenshot from 2023-03-31 20-18-35

Necessity

  • Has the problem that is being solved here been clearly explained?
  • If applicable, have various options for solving this problem been considered?
  • For core PRs, does this need to be in core, or could it be in an extension?
  • Are we willing to maintain this for years / potentially forever?

Confirmed

  • Frontend changes: tested on a local Flarum installation.
  • Backend changes: tests are green (run composer test).
  • Core developer confirmed locally this works as intended.
  • Tests have been added, or are not appropriate here.

@SychO9 SychO9 requested a review from a team as a code owner March 31, 2023 20:58
Signed-off-by: Sami Mazouz <sychocouldy@gmail.com>
Signed-off-by: Sami Mazouz <sychocouldy@gmail.com>
Signed-off-by: Sami Mazouz <sychocouldy@gmail.com>
Signed-off-by: Sami Mazouz <sychocouldy@gmail.com>
@SychO9 SychO9 added this to the 1.8 milestone Apr 1, 2023
@SychO9 SychO9 marked this pull request as draft April 1, 2023 20:58
SychO9 and others added 6 commits April 1, 2023 23:55
Signed-off-by: Sami Mazouz <sychocouldy@gmail.com>
Signed-off-by: Sami Mazouz <sychocouldy@gmail.com>
Signed-off-by: Sami Mazouz <sychocouldy@gmail.com>
Signed-off-by: Sami Mazouz <sychocouldy@gmail.com>
Signed-off-by: Sami Mazouz <sychocouldy@gmail.com>
@SychO9 SychO9 marked this pull request as ready for review April 2, 2023 06:12
@imorland imorland merged commit fbbece4 into main Apr 19, 2023
@imorland imorland deleted the sm/3705-mentions-perf branch April 19, 2023 07:23
@github-actions github-actions bot mentioned this pull request May 17, 2023
# for free to join this conversation on GitHub. Already have an account? # to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Flarum Mentions performance issue
4 participants