Skip to content

Commit

Permalink
[AllBundles] Postgres compatibility fixes (Kunstmaan#1778)
Browse files Browse the repository at this point in the history
* Compare a boolean field against a boolean instead of integer. Postgres is more strict (more sql compliant) so this fails otherwise

* Column/alias names are default lowercase in the sql standard and need to be explicitly double-quoted to be case-sensitive so renamed the alias to avoid this.

* Use postgres COALESCE function instead of IF (not available in postgres)

* Postgres is more strict in the group by fields that need to be specified. Added more fields to be compliant and this also makes it more "safe"

* Query tweaks to keep mysql and postgres compatibility

* Use the correct identifier quote char for each platform

* Acl test fixes
  • Loading branch information
acrobat authored and Devolicious committed Jan 30, 2018
1 parent 3065488 commit 2027349
Show file tree
Hide file tree
Showing 17 changed files with 98 additions and 75 deletions.
36 changes: 22 additions & 14 deletions src/Kunstmaan/AdminBundle/Helper/Security/Acl/AclHelper.php
Original file line number Diff line number Diff line change
Expand Up @@ -138,9 +138,12 @@ public function apply(QueryBuilder $queryBuilder, PermissionDefinition $permissi
private function getPermittedAclIdsSQLForUser(Query $query)
{
$aclConnection = $this->em->getConnection();
$databasePrefix = is_file($aclConnection->getDatabase()) ? '' : $aclConnection->getDatabase().'.';
$databasePlatform = $this->em->getConnection()->getDatabasePlatform()->getName();
$mask = $query->getHint('acl.mask');
$rootEntity = '"' . str_replace('\\', '\\\\', $query->getHint('acl.root.entity')) . '"';
$rootEntity = $query->getHint('acl.root.entity');
if ($databasePlatform === 'mysql') {
$rootEntity = str_replace('\\', '\\\\', $rootEntity);
}

/* @var $token TokenInterface */
$token = $this->tokenStorage->getToken();
Expand All @@ -152,37 +155,42 @@ private function getPermittedAclIdsSQLForUser(Query $query)
}

// Security context does not provide anonymous role automatically.
$uR = array('"IS_AUTHENTICATED_ANONYMOUSLY"');
$uR = array('\'IS_AUTHENTICATED_ANONYMOUSLY\'');

/* @var $role RoleInterface */
foreach ($userRoles as $role) {
// The reason we ignore this is because by default FOSUserBundle adds ROLE_USER for every user
if ($role->getRole() !== 'ROLE_USER') {
$uR[] = '"' . $role->getRole() . '"';
$uR[] = '\'' . $role->getRole() . '\'';
}
}
$uR = array_unique($uR);
$inString = implode(' OR s.identifier = ', $uR);

if (is_object($user)) {
$inString .= ' OR s.identifier = "' . str_replace(
'\\',
'\\\\',
get_class($user)
) . '-' . $user->getUserName() . '"';
$userClass = get_class($user);
if ($databasePlatform === 'mysql') {
$userClass = str_replace('\\', '\\\\', $userClass);
}
$inString .= ' OR s.identifier = \'' . $userClass . '-' . $user->getUserName() . '\'';
}

$objectIdentifierColumn = 'o.object_identifier';
if ($databasePlatform === 'postgresql') {
$objectIdentifierColumn = 'o.object_identifier::BIGINT';
}

$selectQuery = <<<SELECTQUERY
SELECT DISTINCT o.object_identifier as id FROM {$databasePrefix}acl_object_identities as o
INNER JOIN {$databasePrefix}acl_classes c ON c.id = o.class_id
LEFT JOIN {$databasePrefix}acl_entries e ON (
SELECT DISTINCT {$objectIdentifierColumn} as id FROM acl_object_identities as o
INNER JOIN acl_classes c ON c.id = o.class_id
LEFT JOIN acl_entries e ON (
e.class_id = o.class_id AND (e.object_identity_id = o.id
OR {$aclConnection->getDatabasePlatform()->getIsNullExpression('e.object_identity_id')})
)
LEFT JOIN {$databasePrefix}acl_security_identities s ON (
LEFT JOIN acl_security_identities s ON (
s.id = e.security_identity_id
)
WHERE c.class_type = {$rootEntity}
WHERE c.class_type = '{$rootEntity}'
AND (s.identifier = {$inString})
AND e.mask & {$mask} > 0
SELECTQUERY;
Expand Down
36 changes: 22 additions & 14 deletions src/Kunstmaan/AdminBundle/Helper/Security/Acl/AclNativeHelper.php
Original file line number Diff line number Diff line change
Expand Up @@ -59,13 +59,15 @@ public function apply(QueryBuilder $queryBuilder, PermissionDefinition $permissi
{
$aclConnection = $this->em->getConnection();

$databasePrefix = is_file($aclConnection->getDatabase()) ? '' : $aclConnection->getDatabase().'.';
$rootEntity = $permissionDef->getEntity();
$linkAlias = $permissionDef->getAlias();
// Only tables with a single ID PK are currently supported
$linkField = $this->em->getClassMetadata($rootEntity)->getSingleIdentifierColumnName();
$databasePlatform = $this->em->getConnection()->getDatabasePlatform()->getName();

$rootEntity = '"' . str_replace('\\', '\\\\', $rootEntity) . '"';
if ($databasePlatform === 'mysql') {
$rootEntity = str_replace('\\', '\\\\', $rootEntity);
}
$query = $queryBuilder;

$builder = new MaskBuilder();
Expand All @@ -84,37 +86,43 @@ public function apply(QueryBuilder $queryBuilder, PermissionDefinition $permissi
}

// Security context does not provide anonymous role automatically.
$uR = array('"IS_AUTHENTICATED_ANONYMOUSLY"');
$uR = array('\'IS_AUTHENTICATED_ANONYMOUSLY\'');

/* @var $role RoleInterface */
foreach ($userRoles as $role) {
// The reason we ignore this is because by default FOSUserBundle adds ROLE_USER for every user
if ($role->getRole() !== 'ROLE_USER') {
$uR[] = '"' . $role->getRole() . '"';
$uR[] = '\'' . $role->getRole() . '\'';
}
}
$uR = array_unique($uR);
$inString = implode(' OR s.identifier = ', (array) $uR);

if (is_object($user)) {
$inString .= ' OR s.identifier = "' . str_replace(
'\\',
'\\\\',
get_class($user)
) . '-' . $user->getUserName() . '"';
$userClass = get_class($user);
if ($databasePlatform === 'mysql') {
$userClass = str_replace('\\', '\\\\', $userClass);
}

$inString .= ' OR s.identifier = \'' . $userClass . '-' . $user->getUserName() . '\'';
}

$objectIdentifierColumn = 'o.object_identifier';
if ($databasePlatform === 'postgresql') {
$objectIdentifierColumn = 'o.object_identifier::BIGINT';
}

$joinTableQuery = <<<SELECTQUERY
SELECT DISTINCT o.object_identifier as id FROM {$databasePrefix}acl_object_identities as o
INNER JOIN {$databasePrefix}acl_classes c ON c.id = o.class_id
LEFT JOIN {$databasePrefix}acl_entries e ON (
SELECT DISTINCT {$objectIdentifierColumn} as id FROM acl_object_identities as o
INNER JOIN acl_classes c ON c.id = o.class_id
LEFT JOIN acl_entries e ON (
e.class_id = o.class_id AND (e.object_identity_id = o.id
OR {$aclConnection->getDatabasePlatform()->getIsNullExpression('e.object_identity_id')})
)
LEFT JOIN {$databasePrefix}acl_security_identities s ON (
LEFT JOIN acl_security_identities s ON (
s.id = e.security_identity_id
)
WHERE c.class_type = {$rootEntity}
WHERE c.class_type = '{$rootEntity}'
AND (s.identifier = {$inString})
AND e.mask & {$mask} > 0
SELECTQUERY;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -220,9 +220,9 @@ public function testApply()
$this->assertEquals('n', $query->getHint('acl.entityRootTableDqlAlias'));

$aclQuery = $query->getHint('acl.extra.query');
$this->assertContains('"ROLE_SUBJECT"', $aclQuery);
$this->assertContains('"ROLE_KING"', $aclQuery);
$this->assertContains('"IS_AUTHENTICATED_ANONYMOUSLY"', $aclQuery);
$this->assertContains('\'ROLE_SUBJECT\'', $aclQuery);
$this->assertContains('\'ROLE_KING\'', $aclQuery);
$this->assertContains('\'IS_AUTHENTICATED_ANONYMOUSLY\'', $aclQuery);
$this->assertContains('MyUser', $aclQuery);
}

Expand Down Expand Up @@ -277,7 +277,7 @@ public function testApplyAnonymous()
$this->assertEquals('n', $query->getHint('acl.entityRootTableDqlAlias'));

$aclQuery = $query->getHint('acl.extra.query');
$this->assertContains('"IS_AUTHENTICATED_ANONYMOUSLY"', $aclQuery);
$this->assertContains('\'IS_AUTHENTICATED_ANONYMOUSLY\'', $aclQuery);
}

/**
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -167,9 +167,9 @@ public function testApply()
$qb = $this->object->apply($queryBuilder, $permissionDef);
$query = $qb->getSQL();

$this->assertContains('"ROLE_SUBJECT"', $query);
$this->assertContains('"ROLE_KING"', $query);
$this->assertContains('"IS_AUTHENTICATED_ANONYMOUSLY"', $query);
$this->assertContains('\'ROLE_SUBJECT\'', $query);
$this->assertContains('\'ROLE_KING\'', $query);
$this->assertContains('\'IS_AUTHENTICATED_ANONYMOUSLY\'', $query);
$this->assertContains('MyUser', $query);
}

Expand Down Expand Up @@ -210,7 +210,7 @@ public function testApplyAnonymous()
$qb = $this->object->apply($queryBuilder, $permissionDef);
$query = $qb->getSQL();

$this->assertContains('"IS_AUTHENTICATED_ANONYMOUSLY"', $query);
$this->assertContains('\'IS_AUTHENTICATED_ANONYMOUSLY\'', $query);
}

/**
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -112,7 +112,7 @@ public function adaptQueryBuilder(QueryBuilder $queryBuilder, array $params = []
->from('kuma_node_translations', 'nt')
->innerJoin('nt', 'kuma_nodes', 'n', 'nt.node_id = n.id')
->where('n.ref_entity_name = :pageClass')
->andWhere('n.deleted = 0')
->andWhere('n.deleted = false')
->orderBy('nt.updated', 'DESC');

/**
Expand Down
3 changes: 3 additions & 0 deletions src/Kunstmaan/AdminListBundle/Helper/DoctrineDBALAdapter.php
Original file line number Diff line number Diff line change
Expand Up @@ -65,11 +65,14 @@ public function getNbResults()
{
$query = clone $this->queryBuilder;
$distinctString = '';
$groupBy = null;
if ($this->useDistinct) {
$groupBy = $this->countField;
$distinctString = 'DISTINCT ';
}
$statement = $query->select('COUNT('. $distinctString . $this->countField.') AS total_results')
->orderBy($this->countField)
->groupBy($groupBy)
->setMaxResults(1)
->execute();

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -105,7 +105,7 @@ public function adaptQueryBuilder(QueryBuilder $queryBuilder)
$queryBuilder->innerJoin('b.node', 'n', 'WITH', 'b.node = n.id');
$queryBuilder->innerJoin('b.nodeVersions', 'nv', 'WITH', 'b.publicNodeVersion = nv.id');
$queryBuilder->andWhere('b.lang = :lang');
$queryBuilder->andWhere('n.deleted = 0');
$queryBuilder->andWhere('n.deleted = false');
$queryBuilder->andWhere('n.refEntityName = :class');
$queryBuilder->addOrderBy("b.updated", "DESC");
$queryBuilder->setParameter('lang', $this->locale);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,7 @@ public function findActiveOverviewPages()
->innerJoin('KunstmaanNodeBundle:NodeVersion', 'v', 'WITH', 'a.id = v.refId')
->innerJoin('KunstmaanNodeBundle:NodeTranslation', 't', 'WITH', 't.publicNodeVersion = v.id')
->innerJoin('KunstmaanNodeBundle:Node', 'n', 'WITH', 't.node = n.id')
->where('n.deleted = 0')
->where('n.deleted = false')
->andWhere('v.refEntityName = :refname')
->setParameter('refname', $this->getEntityName());

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -178,7 +178,7 @@ public function adaptQueryBuilder(QueryBuilder $queryBuilder)
{
$queryBuilder->andWhere('b.folder = :folder')
->setParameter('folder', $this->folder->getId())
->andWhere('b.deleted = 0')
->andWhere('b.deleted = false')
->orderBy('b.updatedAt', 'DESC');

if ($this->request->get('_route') == 'KunstmaanMediaBundle_chooser_show_folder') {
Expand Down
4 changes: 2 additions & 2 deletions src/Kunstmaan/MenuBundle/Form/MenuItemAdminType.php
Original file line number Diff line number Diff line change
Expand Up @@ -91,10 +91,10 @@ public function buildForm(FormBuilderInterface $builder, array $options)
$qb = $er->createQueryBuilder('nt')
->innerJoin('nt.publicNodeVersion', 'nv')
->innerJoin('nt.node', 'n')
->where('n.deleted = 0')
->where('n.deleted = false')
->andWhere('nt.lang = :lang')
->setParameter('lang', $locale)
->andWhere('nt.online = 1')
->andWhere('nt.online = true')
->orderBy('nt.title', 'ASC');
if ($rootNode) {
$qb->andWhere('n.lft >= :left')
Expand Down
4 changes: 2 additions & 2 deletions src/Kunstmaan/MenuBundle/Repository/MenuItemRepository.php
Original file line number Diff line number Diff line change
Expand Up @@ -26,11 +26,11 @@ public function getMenuItemsForLanguage($menuName, $locale)
->setParameter('locale', $locale)
->andWhere('m.name = :name')
->setParameter('name', $menuName)
->andWhere('nt.online = 1 OR mi.type = :url_type')
->andWhere('nt.online = true OR mi.type = :url_type')
->setParameter('url_type', BaseMenuItem::TYPE_URL_LINK);

$query = $query->getQuery();

return $query->getArrayResult();
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -243,7 +243,7 @@ public function adaptQueryBuilder(QueryBuilder $queryBuilder)
->select('b,n')
->innerJoin('b.node', 'n', 'WITH', 'b.node = n.id')
->andWhere('b.lang = :lang')
->andWhere('n.deleted = 0')
->andWhere('n.deleted = false')
->addOrderBy('b.updated', 'DESC')
->setParameter('lang', $this->locale);

Expand Down
2 changes: 1 addition & 1 deletion src/Kunstmaan/NodeBundle/Form/NodeChoiceType.php
Original file line number Diff line number Diff line change
Expand Up @@ -49,7 +49,7 @@ public function configureOptions(OptionsResolver $resolver)
->innerJoin('nt.publicNodeVersion', 'nv')
->andWhere('nt.online = :online')
->andWhere('nt.lang = :lang')
->andWhere('n.deleted != 1')
->andWhere('n.deleted != true')
->andWhere('n.refEntityName IN(:refEntityName)')
->setParameter('lang', $options['locale'] ? $options['locale'] : $this->getCurrentLocale())
->setParameter('refEntityName', $options['page_class'])
Expand Down
21 changes: 11 additions & 10 deletions src/Kunstmaan/NodeBundle/Repository/NodeRepository.php
Original file line number Diff line number Diff line change
Expand Up @@ -74,7 +74,7 @@ public function getChildNodes(
'WITH',
't.publicNodeVersion = v.id'
)
->where('b.deleted = 0')
->where('b.deleted = false')
->setParameter('lang', $lang)
->addOrderBy('t.weight', 'ASC')
->addOrderBy('t.title', 'ASC');
Expand Down Expand Up @@ -283,7 +283,8 @@ public function getAllMenuNodes(
case 'sqlite':
$statement = 'CASE WHEN %s THEN %s ELSE %s END';
break;

case 'postgresql':
return sprintf('COALESCE(%s, %s)', $trueValue, $falseValue);
default:
$statement = 'IF(%s, %s, %s)';
}
Expand All @@ -295,7 +296,7 @@ public function getAllMenuNodes(
n.id, n.parent_id AS parent, t.url, t.id AS nt_id,
{$createIfStatement('t.weight IS NULL', 'v.weight', 't.weight')} AS weight,
{$createIfStatement('t.title IS NULL', 'v.title', 't.title')} AS title,
{$createIfStatement('t.online IS NULL', '0', 't.online')} AS online,
t.online AS online,
n.hidden_from_nav AS hidden,
n.ref_entity_name AS ref_entity_name
SQL;
Expand All @@ -315,8 +316,8 @@ public function getAllMenuNodes(
'v',
'(v.node_id = n.id AND v.lang <> :lang)'
)
->where('n.deleted = 0')
->addGroupBy('n.id')
->where('n.deleted = false')
->addGroupBy('n.id, t.id, v.id')
->addOrderBy('t.weight', 'ASC')
->addOrderBy('t.title', 'ASC');

Expand Down Expand Up @@ -370,7 +371,7 @@ public function getAllParents(Node $node = null, $lang = null)
'WITH',
't.publicNodeVersion = v.id'
)
->where('node.deleted = 0');
->where('node.deleted = false');

if ($lang) {
$qb->andWhere('t.lang = :lang')
Expand Down Expand Up @@ -414,7 +415,7 @@ public function getRootNodeFor(Node $node = null, $lang = null)
'WITH',
't.publicNodeVersion = v.id'
)
->where('node.deleted = 0')
->where('node.deleted = false')
->andWhere('node.parent IS NULL');

if ($lang) {
Expand Down Expand Up @@ -446,7 +447,7 @@ public function getAllTopNodes()
'WITH',
't.publicNodeVersion = v.id'
)
->where('b.deleted = 0')
->where('b.deleted = false')
->andWhere('b.parent IS NULL');

$result = $qb->getQuery()->getResult();
Expand Down Expand Up @@ -479,7 +480,7 @@ public function getNodesByInternalName(
'WITH',
't.publicNodeVersion = v.id'
)
->where('n.deleted = 0')
->where('n.deleted = false')
->andWhere('n.internalName = :internalName')
->setParameter('internalName', $internalName)
->andWhere('t.lang = :lang')
Expand Down Expand Up @@ -516,7 +517,7 @@ public function getNodeByInternalName($internalName)
{
$qb = $this->createQueryBuilder('n')
->select('n')
->where('n.deleted = 0')
->where('n.deleted = false')
->andWhere('n.internalName = :internalName')
->setParameter('internalName', $internalName);

Expand Down
Loading

0 comments on commit 2027349

Please sign in to comment.