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: provide override for eager loading models on different connections #2654

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

Conversation

wojo1206
Copy link

  • Added or updated tests
  • Documented user facing changes
  • Updated CHANGELOG.md

Changes

Introduced config var that can override isSameConnection() for batchload relations. This fixes n+1 problem for models that exists on different connections.

Breaking changes

None

@spawnia
Copy link
Collaborator

spawnia commented Jan 23, 2025

Have you actually tried this? The check to avoid batch loading if separate connections are involved was added because the resulting database queries for batch loading uses tables of both models, which would not work when they are on different databases. In what instance would this not hold true?

Comment on lines +75 to +78
static fn(): RelationBatchLoader => new RelationBatchLoader(
$paginationArgs === null
? new SimpleModelsLoader($relationName, $decorateBuilder)
: new PaginatedModelsLoader($relationName, $decorateBuilder, $paginationArgs),
? new SimpleModelsLoader($relationName, $decorateBuilder)
: new PaginatedModelsLoader($relationName, $decorateBuilder, $paginationArgs),
Copy link
Collaborator

Choose a reason for hiding this comment

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

Please revert reformatting.

@spawnia spawnia added the enhancement A feature or improvement label Jan 23, 2025
@spawnia spawnia marked this pull request as draft January 23, 2025 16:47
@wojo1206
Copy link
Author

Yes, I use this fork on my production already. Works like a charm. I think you are suggesting that the check to avoid batch loading on separate connections creates possible conflicts due to resulting queries not being database prefixed? I don't know what is the default Eloquant behavior nowadays. It is good practice to include databases prefixes in queries. I believe in the past the Eloquant didn't do that. I have multi database setup and I override getTable() on all models to ensure that my queries are database prefixed.

I think, this config var is valuable addition to the project because it isn't braking change and provides option to the users. In future, this functionality should better align to what the recent Eloquant behavior is.

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

Successfully merging this pull request may close these issues.

2 participants