K-Phoen / rulerz

Powerful implementation of the Specification pattern in PHP
MIT License
872 stars 97 forks source link

Add Doctrine Obscure Alias Support #45

Closed bobdercole closed 7 years ago

bobdercole commented 8 years ago

Hello,

I'm having an issue with RulerZ when attempting to auto-join tables with obscure aliases. In particular, when an association is assigned an alias in the query builder that does not match the name of the association.

This query builder works fine in the current version (0.19.1) of RulerZ:

$entity_manager->createQueryBuilder()
    ->select('permission')
    ->from('Permission', 'permission')
    ->innerJoin('permission.entitlement', 'entitlement')
    ->innerJoin('entitlement.product', 'product');

However, this query builder throws a RuntimeException: Could not automatically join table "product"

$entity_manager->createQueryBuilder()
    ->select('permission')
    ->from('Permission', 'permission')
    ->innerJoin('permission.entitlement', 'entitlement')
    ->innerJoin('entitlement.product', 'product_1');

In both cases, my specification rule looks like this:

public function getRule()
{
    return 'entitlement.product.id = :entitlement_product_id';
}

Do you have any thoughts on this issue? My fix appears to revolve it.

K-Phoen commented 8 years ago

It's weird. I tried to reproduce the bug using a similar request and it worked. Could you try to reproduce it using the code and entities contained in the examples folder?

Also, you might want to have a look to the doctrine-compile-time-metadata branch. I reworked a few things in rulerz, including how the Doctrine ORM target works.

bobdercole commented 8 years ago

Sure. I was able to reproduce it with the example code. Although I had to tweak a few things to prove the point. Take a look at this branch for my example. Thanks for taking the time to explore this.

I checked out your doctrine-compile-time-metadata branch and poked around a bit. The issue persists, but the changes look awesome.

K-Phoen commented 8 years ago

Great!

I'll look into it :)

bobdercole commented 8 years ago

I should also mention that you need to uncomment this line to see it break.

K-Phoen commented 8 years ago

Actually, I think that the current behavior is valid.

What whould your PR do with the following query and rule?

$queryBuilder = $entityManager->createQueryBuilder()
    ->select('permission')
    ->from('Permission', 'permission')
    ->innerJoin('permission.entitlement', 'entitlement')
    ->innerJoin('entitlement.product', 'product_1', 'ON', 'SOME CONDITION')
    ->innerJoin('entitlement.product', 'product_2', 'ON', 'SOME OTHER CONDITION');

$rule = 'entitlement.product.id = :entitlement_product_id';

$rulerz->filter($queryBuilder, $rule);

I think that if you define an alias in your query builder, the same alias should be used in the rule.

WDYT?

bobdercole commented 8 years ago

That's true. My PR would introduce some wonky behavior with your example. Personally I would create a specific repository method to support a more complex query like that one.

I would argue that defining an alias in the query builder and expecting that same alias in the rule creates a tight coupling between infrastructure and domain service code. In my application I bundled my specifications with my domain rather than my infrastructure layer. Perhaps this is the wrong approach?

Additionally, if a rule uses an obscure alias that was defined in the query builder, doesn't that break specification re-usability when filtering objects from other data sources (arrays for example)?

bobdercole commented 7 years ago

I just tried out the new version (0.19.3), and it seems like the library is now able to handle this situation.

For this query builder, RulerZ now seems to automatically add ->leftJoin('entitlement.product', 'product')

$entity_manager->createQueryBuilder()
    ->select('permission')
    ->from('Permission', 'permission')
    ->innerJoin('permission.entitlement', 'entitlement')
    ->innerJoin('entitlement.product', 'product_1');

However, if I modify the query builder to use the same alias as my specification, it does not join anything extra:

$entity_manager->createQueryBuilder()
    ->select('permission')
    ->from('Permission', 'permission')
    ->innerJoin('permission.entitlement', 'entitlement')
    ->innerJoin('entitlement.product', 'product');

I think the code that you posted above would simply add a ->leftJoin('entitlement.product', 'product'), since the alias does not match.

I'm not sure if this behavior is intended, but I really like the way it's working now 👍. This implementation enables separation between the rule written in the specification and the query builder; the specification does not need to know about any extra joins and/or aliases. I suppose you can close this PR now.

By the way, thanks for the new release! The Doctrine auto-join logic is much easier to follow now.