Skip to content

Conversation

@smarcet
Copy link
Collaborator

@smarcet smarcet commented Oct 6, 2025

ref: https://app.clickup.com/t/86b6z5t03
feat: add new load graph by relation trait
chore: improve performance of get summit by id ( N + 1 ) problem

@smarcet smarcet marked this pull request as ready for review October 6, 2025 15:56
@smarcet smarcet requested a review from Copilot October 6, 2025 15:56
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

This PR adds new functionality to load entity graphs with relations efficiently to address N+1 query problems when fetching summit data. The main goal is to improve performance by implementing a graph loading trait that can eagerly load related entities in optimized queries.

Key changes:

  • Added a new GraphLoaderTrait for efficient entity graph loading with relation support
  • Enhanced the Summit repository with new methods that utilize the graph loading functionality
  • Updated the Summit finder strategy to support relation preloading

Reviewed Changes

Copilot reviewed 7 out of 7 changed files in this pull request and generated 4 comments.

Show a summary per file
File Description
app/libs/Utils/Doctrine/GraphLoaderTrait.php New trait implementing graph loading logic for entities with relations
app/Repositories/Summit/DoctrineSummitRepository.php Added graph loading trait usage and new methods for loading summits with relations
app/Models/Foundation/Summit/Repositories/ISummitRepository.php Added interface methods for relation-based summit retrieval
app/Http/Controllers/Apis/Protected/Summit/Strategies/ISummitFinderStrategy.php Updated interface to support relations parameter
app/Http/Controllers/Apis/Protected/Summit/Strategies/CurrentSummitFinderStrategy.php Implemented relation support in summit finder strategy
app/Http/Controllers/Apis/Protected/Summit/OAuth2SummitApiController.php Updated summit retrieval to use relation preloading
tests/OAuth2SummitApiTest.php Added test case for summit retrieval with relations parameter

Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.

public function find($summit_id)
public function find($summit_id, array $relations = [])
{
if(count($relations) > 0){
Copy link

Copilot AI Oct 6, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Missing space after 'if' keyword. Should be 'if (count($relations) > 0) {' to follow PHP coding standards.

Suggested change
if(count($relations) > 0){
if (count($relations) > 0){

Copilot uses AI. Check for mistakes.
{
if(count($relations) > 0){
$summit = $summit_id === 'current' ? $this->repository->getCurrentAndRelations($relations) : $this->repository->getByIdAndRelations(intval($summit_id), $relations);
if(is_null($summit))
Copy link

Copilot AI Oct 6, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Missing space after 'if' keyword. Should be 'if (is_null($summit))' to follow PHP coding standards.

Suggested change
if(is_null($summit))
if (is_null($summit))

Copilot uses AI. Check for mistakes.
chore: improvee performance of get summit by id ( N + 1 ) problem
@smarcet smarcet force-pushed the feature/load-graph-by-relations-at-repo branch from f6ae70b to 347251f Compare October 6, 2025 21:02
@smarcet smarcet merged commit c4a8a24 into main Oct 6, 2025
3 checks passed
smarcet added a commit that referenced this pull request Oct 7, 2025
chore: improvee performance of get summit by id ( N + 1 ) problem
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.

2 participants