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

Optimize matching zones to a given address #15275

Merged
merged 24 commits into from
Sep 27, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
24 commits
Select commit Hold shift + click to select a range
84aaf55
Add eager loading for Zone's members
jakubtobiasz Sep 6, 2023
ba06fc1
Add a method finding all zones by address to the Zone's repository
jakubtobiasz Sep 11, 2023
a69fb7a
Add a method finding all zones by a zone to the Zone's repository
jakubtobiasz Sep 11, 2023
8b6c365
Implement more optimized ZoneMatcher::matchAll method
jakubtobiasz Sep 12, 2023
660c95d
Implement more optimized ZoneMatcher::match method
jakubtobiasz Sep 12, 2023
43004fe
Provide ECS fixes
jakubtobiasz Sep 12, 2023
5c6033d
Move ZoneRepositoryInterface from the Bundle to the Component
jakubtobiasz Sep 12, 2023
7491c61
Take into account 'all' scope in the Zone
jakubtobiasz Sep 12, 2023
5a3923a
Add AliceDataFixturesBundle to the test app in the AddressingBundle
jakubtobiasz Sep 12, 2023
3287b8c
Add selecting zone members to prevent lazy loading them
jakubtobiasz Sep 15, 2023
7bf31b2
Add purging the test database between phpunit tests
jakubtobiasz Sep 15, 2023
830c741
Remove the visibility definitions for methods in the spec
jakubtobiasz Sep 15, 2023
c152bb5
Make the createFindByAddressQueryBuilder public
jakubtobiasz Sep 15, 2023
7e975d3
Replace overriding the zones argument with a new zonesCodes variable
jakubtobiasz Sep 15, 2023
bea9452
Fix database-based tests
jakubtobiasz Sep 15, 2023
ce72d4b
Rename the findAllByZones method to findZonesByMembers
jakubtobiasz Sep 18, 2023
6e5bcee
Remove the unnecessary eager loading of zone's members
jakubtobiasz Sep 21, 2023
68a47ca
Rename the "findZonesByMembers" method to "findByMembers"
jakubtobiasz Sep 21, 2023
66cfe9c
Rename the "findAllByAddress" method to "findByAddress"
jakubtobiasz Sep 21, 2023
8922d7e
Rename the "createFindByAddressQueryBuilder" method to "createByAddre…
jakubtobiasz Sep 21, 2023
fc261db
Adjust scopes' names to better reflect Sylius' zone scopes
jakubtobiasz Sep 21, 2023
89a5e8a
Adjust tables' aliases
jakubtobiasz Sep 21, 2023
8365fbc
Replace the `findOneByAddress` method with `findOneByAddressAndType`
jakubtobiasz Sep 21, 2023
d72ffda
Rename $query variables to $queryBuilder
jakubtobiasz Sep 27, 2023
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@
use Sylius\Bundle\AddressingBundle\Form\Type\ProvinceType;
use Sylius\Bundle\AddressingBundle\Form\Type\ZoneMemberType;
use Sylius\Bundle\AddressingBundle\Form\Type\ZoneType;
use Sylius\Bundle\AddressingBundle\Repository\ZoneRepository;
use Sylius\Bundle\ResourceBundle\Controller\ResourceController;
use Sylius\Bundle\ResourceBundle\SyliusResourceBundle;
use Sylius\Component\Addressing\Model\Address;
Expand Down Expand Up @@ -142,7 +143,7 @@ private function addResourcesSection(ArrayNodeDefinition $node): void
->scalarNode('model')->defaultValue(Zone::class)->cannotBeEmpty()->end()
->scalarNode('interface')->defaultValue(ZoneInterface::class)->cannotBeEmpty()->end()
->scalarNode('controller')->defaultValue(ResourceController::class)->cannotBeEmpty()->end()
->scalarNode('repository')->cannotBeEmpty()->end()
->scalarNode('repository')->defaultValue(ZoneRepository::class)->cannotBeEmpty()->end()
Copy link
Member

Choose a reason for hiding this comment

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

Sylius bundles never specify repository at Configuration.php

image

I'm questioning this for years, but never actually research how and why it works this way.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If there's a custom repository then it's declared in the config
CleanShot 2023-09-25 at 10 44 58

Copy link
Member

@diimpp diimpp Sep 27, 2023

Choose a reason for hiding this comment

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

It's not true for 1.12, every instance that you've found was introduced in 1.13.

I still don't know why it was done this way and how it even work, but custom repositories are not specified for Configuration for any of the bundle

->scalarNode('factory')->defaultValue(Factory::class)->end()
->scalarNode('form')->defaultValue(ZoneType::class)->cannotBeEmpty()->end()
->end()
Expand Down
121 changes: 121 additions & 0 deletions src/Sylius/Bundle/AddressingBundle/Repository/ZoneRepository.php
Original file line number Diff line number Diff line change
@@ -0,0 +1,121 @@
<?php

/*
* This file is part of the Sylius package.
*
* (c) Sylius Sp. o o.o.
*
* For the full copyright and license information, please view the LICENSE
* file that was distributed with this source code.
*/

declare(strict_types=1);

namespace Sylius\Bundle\AddressingBundle\Repository;

use Doctrine\ORM\QueryBuilder;
use Sylius\Bundle\ResourceBundle\Doctrine\ORM\EntityRepository;
use Sylius\Component\Addressing\Model\AddressInterface;
use Sylius\Component\Addressing\Model\Scope;
use Sylius\Component\Addressing\Model\ZoneInterface;
use Sylius\Component\Addressing\Repository\ZoneRepositoryInterface;

/**
* @implements ZoneRepositoryInterface<ZoneInterface>
*/
class ZoneRepository extends EntityRepository implements ZoneRepositoryInterface
Copy link
Contributor

Choose a reason for hiding this comment

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

The fact that a new repository for an existing resource has shown up should be noted in the upgrade file. If someone has already created a custom one prior, the matcher will just fail.

{
public function findOneByAddressAndType(AddressInterface $address, string $type, ?string $scope = null): ?ZoneInterface
{
$queryBuilder = $this->createByAddressQueryBuilder($address, $scope);

$queryBuilder
->andWhere($queryBuilder->expr()->eq('o.type', ':type'))
->setParameter('type', $type)
->setMaxResults(1)
jakubtobiasz marked this conversation as resolved.
Show resolved Hide resolved
;

return $queryBuilder->getQuery()->getOneOrNullResult();
}

/** @return ZoneInterface[] */
public function findByAddress(AddressInterface $address, ?string $scope = null): array
{
return $this->createByAddressQueryBuilder($address, $scope)->getQuery()->getResult();
}

public function createByAddressQueryBuilder(AddressInterface $address, ?string $scope = null): QueryBuilder
{
$queryBuilder = $this->createQueryBuilder('o')
->select('o', 'members')
->leftJoin('o.members', 'members')
;

if (null !== $scope) {
$queryBuilder
->andWhere($queryBuilder->expr()->in('o.scope', ':scopes'))
->setParameter('scopes', array_unique([$scope, Scope::ALL]))
;
}

$orConditions = [];

if ($address->getCountryCode() !== null) {
$orConditions[] = $queryBuilder->expr()->andX(
$queryBuilder->expr()->eq('o.type', ':country'),
$queryBuilder->expr()->eq('members.code', ':countryCode'),
);

$queryBuilder->setParameter('country', ZoneInterface::TYPE_COUNTRY);
$queryBuilder->setParameter('countryCode', $address->getCountryCode());
}

if ($address->getProvinceCode() !== null) {
$orConditions[] = $queryBuilder->expr()->andX(
$queryBuilder->expr()->eq('o.type', ':province'),
$queryBuilder->expr()->eq('members.code', ':provinceCode'),
);

$queryBuilder->setParameter('province', ZoneInterface::TYPE_PROVINCE);
$queryBuilder->setParameter('provinceCode', $address->getProvinceCode());
}

$queryBuilder->andWhere($queryBuilder->expr()->orX(...$orConditions));

return $queryBuilder;
}

/**
* @param array<ZoneInterface> $members
*
* @return array<ZoneInterface>
*/
public function findByMembers(array $members, ?string $scope = null): array
{
$zonesCodes = array_map(
fn (ZoneInterface $zone): string => $zone->getCode(),
$members,
);

$queryBuilder = $this->createQueryBuilder('o')
->select('o', 'members')
->leftJoin('o.members', 'members')
;

if (null !== $scope) {
$queryBuilder
->andWhere($queryBuilder->expr()->in('o.scope', ':scopes'))
->setParameter('scopes', array_unique([$scope, Scope::ALL]))
;
}

$queryBuilder
->andWhere('o.type = :type')
->andWhere($queryBuilder->expr()->in('members.code', ':zones'))
->setParameter('type', ZoneInterface::TYPE_ZONE)
->setParameter('zones', $zonesCodes)
jakubtobiasz marked this conversation as resolved.
Show resolved Hide resolved
;

return $queryBuilder->getQuery()->getResult();
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,85 @@
<?php

/*
* This file is part of the Sylius package.
*
* (c) Sylius Sp. z o.o.
*
* For the full copyright and license information, please view the LICENSE
* file that was distributed with this source code.
*/

declare(strict_types=1);

namespace Sylius\Bundle\AddressingBundle\Tests\Matcher;

use Doctrine\Common\DataFixtures\Purger\ORMPurger;
use Doctrine\ORM\EntityManagerInterface;
use Fidry\AliceDataFixtures\LoaderInterface;
use Fidry\AliceDataFixtures\Persistence\PurgeMode;
use Sylius\Component\Addressing\Model\Address;
use Symfony\Bundle\FrameworkBundle\Test\KernelTestCase;

final class ZoneMatcherTest extends KernelTestCase
{
protected function setUp(): void
{
$this->loadFixtures();
}

/** @test */
public function it_matches_all_zones_with_their_parents(): void
{
$address = new Address();
$address->setCountryCode('PL');
$address->setProvinceCode('MA');

$zoneMatcher = self::getContainer()->get('sylius.zone_matcher');
$matchedZones = [];

foreach ($zoneMatcher->matchAll($address) as $zone) {
$matchedZones[$zone->getCode()] = $zone;
}

$this->assertCount(4, $matchedZones);
$this->assertArrayHasKey('EU', $matchedZones);
$this->assertArrayHasKey('VISEGRAD_GROUP', $matchedZones);
$this->assertArrayHasKey('PL', $matchedZones);
$this->assertArrayHasKey('NATO', $matchedZones);
}

/** @test */
public function it_matches_all_zones_with_their_parents_with_restricting_by_scope(): void
{
$address = new Address();
$address->setCountryCode('PL');
$address->setProvinceCode('MA');

$zoneMatcher = self::getContainer()->get('sylius.zone_matcher');
$matchedZones = [];

foreach ($zoneMatcher->matchAll($address, 'shipping') as $zone) {
$matchedZones[$zone->getCode()] = $zone;
}

$this->assertCount(3, $matchedZones);
$this->assertArrayHasKey('NATO', $matchedZones);
$this->assertArrayHasKey('PL', $matchedZones);
$this->assertArrayHasKey('VISEGRAD_GROUP', $matchedZones);
}

private function loadFixtures(): void
{
/** @var LoaderInterface $fixtureLoader */
$fixtureLoader = self::getContainer()->get('fidry_alice_data_fixtures.loader.doctrine');

/** @var EntityManagerInterface $manager */
$manager = self::getContainer()->get('doctrine.orm.default_entity_manager');

(new ORMPurger($manager))->purge();

$fixtureLoader->load([
__DIR__ . '/ZoneMatcherTest/fixtures.yaml',
], [], [], PurgeMode::createDeleteMode());
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,42 @@
Sylius\Component\Addressing\Model\Country:
country_PL:
code: PL

Sylius\Component\Addressing\Model\Province:
province_{MA, LD, MZ, DS}:
code: <current()>
name: <current()>
abbreviation: <current()>
country: "@country_PL"

Sylius\Component\Addressing\Model\ZoneMember:
member_province_{MA, LD, MZ, DS}:
code: <current()>
member_{poland_visegrad_group, poland_nato}:
code: PL
member_visegrad_group:
code: "VISEGRAD_GROUP"

Sylius\Component\Addressing\Model\Zone:
zone_poland:
code: PL
name: Poland
type: province
members: ["@member_province_MA", "@member_province_LD", "@member_province_MZ", "@member_province_DS"]
zone_visegrad_group:
code: VISEGRAD_GROUP
name: Visegrad Group
type: zone
members: ["@member_poland_visegrad_group"]
zone_eu:
code: EU
name: European Union
type: zone
members: ["@member_visegrad_group"]
scope: 'tax'
zone_nato:
code: NATO
name: NATO
type: zone
members: ["@member_poland_nato"]
scope: 'shipping'