pimcore / admin-ui-classic-bundle

Other
11 stars 100 forks source link

[Improvement] Refactor query for performance gain #748

Open mattamon opened 1 week ago

mattamon commented 1 week ago

Resolves #700 - at least it is a start.

Refactoring the query is one part. We should also introduce an index like the following, but we can do this in the core itself with a migration. See https://github.com/pimcore/pimcore/pull/17834

CREATE INDEX idx_users_workspaces ON users_workspaces_object (userId, cpath, list);
CREATE INDEX idx_users_workspaces ON users_workspaces_asset (userId, cpath, list);
fashxp commented 1 week ago

Sadly, this query does not deliver the correct results :disappointed:.

Take the following permission configuration which must not show /Product Data/Accessories/rims: image

The old query looks something like this for userid 20

SELECT id, path, `key` FROM objects WHERE path = '/Product Data/Accessories/' AND `key` = 'rims' AND
((select list from users_workspaces_object where userId in (20) and LOCATE(CONCAT("/Product Data/Accessories/","rims"),cpath)=1
    ORDER BY LENGTH(cpath) DESC LIMIT 1)=1
OR
(select list from users_workspaces_object where userId in (20) and LOCATE(cpath,CONCAT("/Product Data/Accessories/","rims"))=1
    ORDER BY LENGTH(cpath) DESC LIMIT 1)=1
); 

... and returns zero results - which is correct.

The new query looks something like this

SELECT id, path, `key` FROM objects WHERE path = '/Product Data/Accessories/' AND `key` = 'rims' AND
    EXISTS (
        SELECT 1
        FROM users_workspaces_object
        WHERE userId IN (20)
          AND (
            LOCATE(CONCAT("/Product Data/Accessories/","rims"), cpath) = 1
                OR LOCATE(cpath, CONCAT("/Product Data/Accessories/","rims")) = 1
            )
          AND list = 1
    );

... and returns results - which is wrong.

The reason why while testing in the UI it doesn't show a result is that line - which double checks permissions with calling isAllowed for each element.

This in turns becomes a problem for example with following permission configuration, which should show one line in the grid: image

With the old query, it works as expected. With the new query it doesn't - because the query delivers a whole page of false-positives (which are filtered out) so the true-positive doesn't make it into the result.

sonarcloud[bot] commented 1 week ago

Quality Gate Passed Quality Gate passed

Issues
2 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud

mattamon commented 1 week ago

Sadly, this query does not deliver the correct results 😞.

Take the following permission configuration which must not show /Product Data/Accessories/rims: image

The old query looks something like this for userid 20

SELECT id, path, `key` FROM objects WHERE path = '/Product Data/Accessories/' AND `key` = 'rims' AND
((select list from users_workspaces_object where userId in (20) and LOCATE(CONCAT("/Product Data/Accessories/","rims"),cpath)=1
    ORDER BY LENGTH(cpath) DESC LIMIT 1)=1
OR
(select list from users_workspaces_object where userId in (20) and LOCATE(cpath,CONCAT("/Product Data/Accessories/","rims"))=1
    ORDER BY LENGTH(cpath) DESC LIMIT 1)=1
); 

... and returns zero results - which is correct.

The new query looks something like this

SELECT id, path, `key` FROM objects WHERE path = '/Product Data/Accessories/' AND `key` = 'rims' AND
    EXISTS (
        SELECT 1
        FROM users_workspaces_object
        WHERE userId IN (20)
          AND (
            LOCATE(CONCAT("/Product Data/Accessories/","rims"), cpath) = 1
                OR LOCATE(cpath, CONCAT("/Product Data/Accessories/","rims")) = 1
            )
          AND list = 1
    );

... and returns results - which is wrong.

The reason why while testing in the UI it doesn't show a result is that line - which double checks permissions with calling isAllowed for each element.

This in turns becomes a problem for example with following permission configuration, which should show one line in the grid: image

With the old query, it works as expected. With the new query it doesn't - because the query delivers a whole page of false-positives (which are filtered out) so the true-positive doesn't make it into the result.

Valid points. I used now only some micro-optimization since LIKE seems to performe better than Locate.

At least the results look now better to me but I am not sure what big of an impact it has. For my tests we are not far off the admin time.