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

Replace multiple DB transactions with a single one #6315

Merged
merged 1 commit into from Oct 6, 2020

Conversation

phansys
Copy link
Member

@phansys phansys commented Aug 24, 2020

Subject

Replace multiple DB transactions with a single one.

I am targeting this branch, because these changes respect BC.

Closes #6280.

Changelog

### Changed
- Changed multiple calls to `ModelManagerInterface::find()` with `ModelManagerInterface::findBy()` in order to avoid multiple transactions.

@phansys phansys requested a review from a team August 24, 2020 01:06
@phansys phansys force-pushed the find_by branch 3 times, most recently from 9e9e804 to 3d17c27 Compare August 24, 2020 01:22
return '_labels' !== $key;
}, ARRAY_FILTER_USE_KEY);

$singleIdentifierFieldName = $this->modelManager->getIdentifierFieldNames($this->className)[0];
Copy link
Member

Choose a reason for hiding this comment

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

I think this wont be correct for composite keys

Copy link
Member Author

@phansys phansys Aug 24, 2020

Choose a reason for hiding this comment

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

I agree, but at the beginning I thought this was occurring with the previous implementation. Checking EntityManager::find() I see that isn't the case.
I guess we must build a custom query for composite keys, since if we call findBy() for each key, we have no guarantee about the keys' consistency.

Copy link
Member

Choose a reason for hiding this comment

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

What I dont know if it is better to delegate that to the persistence bundles, because I dont know if the odm works the same way. Maybe it is better to introduce the findMultiple you mentioned in the issue

@phansys phansys marked this pull request as draft August 24, 2020 09:10
@SonataCI
Copy link
Collaborator

Could you please rebase your PR and fix merge conflicts?

@phansys phansys force-pushed the find_by branch 4 times, most recently from d646fbf to f57b683 Compare September 28, 2020 10:20
@phansys
Copy link
Member Author

phansys commented Sep 28, 2020

I'm currently using the same approach as CRUDController::batchAction(), which is relying on methods already present in ModelManagerInterface():

$this->admin->getModelManager()->addIdentifiersToQuery($this->admin->getClass(), $query, $idx);

We could provide a shortcut ModelManagerInterface::findMultiple() based on this if we consider that as a requirement, but I'd like to know WDYT first.

@VincentLanglet
Copy link
Member

@phansys I re-read again the PR.

Do we really need a ModelManagerInterface::findMultiple method ?

If not implemented you're using

$query = $this->modelManager->createQuery($this->class);
$this->modelManager->addIdentifiersToQuery($this->class, $query, $value);
$result = $this->modelManager->executeQuery($query);

We could always use this code instead of requiring a new method in the interface which can be fully implemented with others methods.

@phansys
Copy link
Member Author

phansys commented Oct 1, 2020

I think there is no need for a new method, but the same was also true in the first commit (I guess): d02a65a.
I agree with the proposal of not adding a new method and leveraging the API we already have (ModelManagerInterface::addIdentifiersToQuery()).
@sonata-project/contributors, please share any concerns about this in order to unblock this PR.
I'll try to find some time this weekend to revisit this.

@phansys phansys force-pushed the find_by branch 3 times, most recently from 8aa4cba to 4ade0b7 Compare October 3, 2020 12:27
@phansys phansys added the minor label Oct 3, 2020
@phansys phansys changed the title Leverage ModelManagerInterface::findBy() Replace multiple DB transactions with a single one Oct 3, 2020
@phansys phansys marked this pull request as ready for review October 3, 2020 13:12
VincentLanglet
VincentLanglet previously approved these changes Oct 5, 2020
@phansys phansys requested a review from a team October 5, 2020 16:00
VincentLanglet
VincentLanglet previously approved these changes Oct 6, 2020
VincentLanglet
VincentLanglet previously approved these changes Oct 6, 2020
@phansys
Copy link
Member Author

phansys commented Oct 6, 2020

Sorry @VincentLanglet, I had to fix a CS issue in the new test.
Please, review again.

Copy link
Member

@franmomu franmomu left a comment

Choose a reason for hiding this comment

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

Nice

@VincentLanglet VincentLanglet merged commit fb6e2e5 into sonata-project:3.x Oct 6, 2020
@VincentLanglet
Copy link
Member

Thanks !

@phansys phansys deleted the find_by branch October 6, 2020 19:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Allow ModelManagerInterface to get multiple results in a single transaction
7 participants