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

fix foreign query reference, use exists, realias sub-segment query, a… #6871

Conversation

galvani
Copy link
Contributor

@galvani galvani commented Nov 13, 2018

Fix segment foreign reference bug

Please be sure you are submitting this against the staging branch.

Q A
Bug fix? Y
New feature? N
Automated tests included? N
Issues addressed (#s or URLs) none
BC breaks? N
Deprecations? N

Description:

Segments with reference to another segment were not built correctly if grouped into condition using in and notIn. This PR fixes it and improves performance by replacing join with exists.

Steps to reproduce the bug and test this PR:

  1. Create a segment that defines reference in to multiple other segments that contain different contacts. It should be sufficient that referenced segments contain one contact only.
  2. Run and verify that not all contacts that should be there are there.
  3. Checkout this PR an rerun
  4. Now everything should match.

…dd get alias function for leads table to query builder
@npracht npracht added bug Issues or PR's relating to bugs ready-to-test PR's that are ready to test code-review-needed PR's that require a code review before merging labels Nov 13, 2018
@npracht npracht added this to the 2.15.1 milestone Nov 13, 2018
Copy link
Contributor

@Enc3phale Enc3phale left a comment

Choose a reason for hiding this comment

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

I have tested this PR with a segment that include segment filter in and notIn but now we have this error on some segments:
[2018-11-19 09:42:31] mautic.NOTICE: Mautic\LeadBundle\Segment\Query\QueryException: The given alias 'l' is not part of any FROM or JOIN clause table. The currently registered aliases are: ASKqQsTW, GUERVocT. (uncaught exception) at /app/bundles/LeadBundle/Segment/Query/QueryException.php line 37 while running console command mautic:segments:update [] []

@npracht npracht added the pending-feedback PR's and issues that are awaiting feedback from the author label Nov 19, 2018
@galvani
Copy link
Contributor Author

galvani commented Nov 21, 2018

@Enc3phale could you please paste full backtrace?

@Enc3phale
Copy link
Contributor

@galvani of course:

[Mautic\LeadBundle\Segment\Query\QueryException]                                                                             
  The given alias 'l' is not part of any FROM or JOIN clause table. The currently registered aliases are: gSsyEfzB, ihxXSuVO. 


Exception trace:
 () at /app/bundles/LeadBundle/Segment/Query/QueryException.php:37
 Mautic\LeadBundle\Segment\Query\QueryException::unknownAlias() at /app/bundles/LeadBundle/Segment/Query/QueryBuilder.php:1216
 Mautic\LeadBundle\Segment\Query\QueryBuilder->verifyAllAliasesAreKnown() at /app/bundles/LeadBundle/Segment/Query/QueryBuilder.php:1202
 Mautic\LeadBundle\Segment\Query\QueryBuilder->getFromClauses() at /app/bundles/LeadBundle/Segment/Query/QueryBuilder.php:1160
 Mautic\LeadBundle\Segment\Query\QueryBuilder->getSQLForSelect() at /app/bundles/LeadBundle/Segment/Query/QueryBuilder.php:270
 Mautic\LeadBundle\Segment\Query\QueryBuilder->getSQL() at /app/bundles/LeadBundle/Segment/Query/Filter/SegmentReferenceFilterQueryBuilder.php:124
 Mautic\LeadBundle\Segment\Query\Filter\SegmentReferenceFilterQueryBuilder->applyQuery() at /app/bundles/LeadBundle/Segment/ContactSegmentFilter.php:172
 Mautic\LeadBundle\Segment\ContactSegmentFilter->applyQuery() at /app/bundles/LeadBundle/Segment/Query/ContactSegmentQueryBuilder.php:86
 Mautic\LeadBundle\Segment\Query\ContactSegmentQueryBuilder->assembleContactsSegmentQueryBuilder() at /app/bundles/LeadBundle/Segment/ContactSegmentService.php:222
 Mautic\LeadBundle\Segment\ContactSegmentService->getNewSegmentContactsQuery() at /app/bundles/LeadBundle/Segment/ContactSegmentService.php:74
 Mautic\LeadBundle\Segment\ContactSegmentService->getNewLeadListLeadsCount() at /app/bundles/LeadBundle/Model/ListModel.php:902
 Mautic\LeadBundle\Model\ListModel->rebuildListLeads() at /app/bundles/LeadBundle/Command/UpdateLeadListsCommand.php:86
 Mautic\LeadBundle\Command\UpdateLeadListsCommand->execute() at /vendor/symfony/console/Command/Command.php:241
 Symfony\Component\Console\Command\Command->run() at /vendor/symfony/console/Application.php:861
 Symfony\Component\Console\Application->doRunCommand() at /vendor/symfony/console/Application.php:193
 Symfony\Component\Console\Application->doRun() at /vendor/symfony/framework-bundle/Console/Application.php:84
 Symfony\Bundle\FrameworkBundle\Console\Application->doRun() at /vendor/symfony/console/Application.php:117
 Symfony\Component\Console\Application->run() at mautic_command_CLI.php:62

 mautic:segments:update [-b|--batch-limit [BATCH-LIMIT]] [-m|--max-contacts [MAX-CONTACTS]] [-i|--list-id [LIST-ID]] [-f|--force] [--bypass-locking] [-t|--timeout TIMEOUT] [-x|--lock_mode LOCK_MODE] [-h|--help] [-q|--quiet] [-v|vv|vvv|--verbose] [-V|--version] [--ansi] [--no-ansi] [-n|--no-interaction] [-s|--shell] [--process-isolation] [-e|--env ENV] [--no-debug] [--] <command>

@npracht
Copy link
Member

npracht commented Nov 28, 2018

Hi @galvani did you have the opportunity to look at it ?

@npracht npracht modified the milestone: 2.15.1 Dec 3, 2018
@heathdutton heathdutton added this to Code Review (2 required) in Mautic 2 Dec 6, 2018
@npracht
Copy link
Member

npracht commented Dec 18, 2018

Hello @galvani any update on this ?

@npracht npracht moved this from Code Review (2 required) to Changes Requested / Review in Mautic 2 Jan 3, 2019
@alanhartless alanhartless added this to Has Conflicts and/or Failing Tests in 2.15.1 Jan 14, 2019
@Enc3phale
Copy link
Contributor

Enc3phale commented Jan 16, 2019

Hi @galvani, in app/bundles/LeadBundle/Segment/Query/Filter/SegmentReferenceFilterQueryBuilder.php

I found this lines

$subSegmentLeadsTableAlias = $this->generateRandomParameterName();
$segmentQueryBuilder->resetQueryParts(['from'])->from(MAUTIC_TABLE_PREFIX.'leads', $subSegmentLeadsTableAlias);

causing the error:
QueryException: The given alias 'l' is not part of any FROM or JOIN clause table.

If I remove lines no more error and segment is compiled, but I don't know if this could be removed.

Copy link
Member

@npracht npracht left a comment

Choose a reason for hiding this comment

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

Hi guys, we tried to work hard on it because we had some issues on segments. The conclusion is that this PR is creating more issues than solving. The root of the issue is coming from somewhere else.

In fact, the issue is: when you delete a segment which is included in another segment (with the filter "segment membership") then the deleted segment keep being saved in the filter of the other segment in the DB ---> that generates tones of issues.
This PR #6566 helps a lot to avoid future issues because you cannot delete a used segment but does not solve issues retroactively (you still need to re-do your old segments).

@galvani
Copy link
Contributor Author

galvani commented Jan 18, 2019

@npracht we should close it and refuse to merge this PR. I am not longer sure what it was made for. We don't have it in mautic-inc. It's too old.

@galvani galvani closed this Jan 18, 2019
@alanhartless alanhartless removed this from the 2.15.1 milestone Mar 5, 2019
@alanhartless alanhartless removed this from Has Conflicts and/or Failing Tests in 2.15.1 Mar 5, 2019
@npracht npracht removed this from Changes Requested / Review in Mautic 2 Apr 5, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Issues or PR's relating to bugs code-review-needed PR's that require a code review before merging pending-feedback PR's and issues that are awaiting feedback from the author ready-to-test PR's that are ready to test
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants