Repository navigation
perf(mgr-api): вернуть LIMIT/OFFSET в листинг товаров категории (#473) - #910
Merged
Merged
Conversation
Листинг был обычной постраничной выборкой, пока #473 не добавил ACL уровня документа. Проверять checkPolicy('view') можно только по загруженному объекту, поэтому прямой вызов getPage() заменили сканированием: collectVisibleListPage() — сканирует от начала батчами и пропускает start записей в PHP; countVisibleListResults() — проходит всю категорию батчами по 200 и считает видимые, ради одного числа total. На категории с 2114 товарами это 18 запросов и 953 мс на первой странице, 33 запроса и 1350 мс на start=800 — цена растёт с номером страницы. Сервис при этом всё умеет сам: COUNT(DISTINCT msProduct.id) одним запросом и limit($limit, $start). Контроллер просто перестал этим пользоваться. Ограничить видимость ресурса может только группа ресурсов с записью доступа: modResource::findPolicy() собирает политику джойном modAccessResourceGroup + modResourceGroupResource и отдаёт пустой набор, когда настройка контекста выключена. Значит нет ни одной записи доступа — нет ни одного закрытого товара, и построчный фильтр заведомо пропускает всё; для sudo он пропускает всё всегда. В этих случаях включается быстрый путь, то есть ровно тот код, что был до #473. Если группы ресурсов настроены, остаётся прежнее сканирование: права важнее скорости. Замер после правки на той же категории: nested=false 1 запрос, 24-31 мс на любой странице (было 18-33 / 953-1350) nested=true 22 запроса, 32 мс (22 — обход дерева категорий, он был и раньше) total совпадает с прежним сканирующим подсчётом в обоих режимах: 2114. Проверено и обратное — при появлении записи modAccessResourceGroup условие переключается на false, после уборки обратно на true. Фильтрация листинга по ACL тестами не покрыта (ни collectVisibleListPage, ни countVisibleListResults, ни filterListResultsByDocumentView в тестах не упоминаются), поэтому добавлен тест на само условие переключения: ошибка в нём либо вернёт медленный листинг, либо обойдёт проверку прав. Стабу modX добавлены getCount() и optionOverrides. Подпись getCount() намеренно без типов: тесты переопределяют метод в анонимных подклассах со своей, более слабой.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Листинг товаров внутри категории в админке долго думает при пагинации. Нашлось при разборе наблюдения пользователя.
Что происходит
Листинг был обычной постраничной выборкой, пока
9f091d1e(#473, «document-level ACL для category products») не добавил проверкуcheckPolicy('view')на каждом товаре. Проверять политику можно только по загруженному объекту, поэтому прямой вызовgetPage()заменили сканированием:collectVisibleListPage()max(limit*2, 20)и пропускаетstartзаписей в PHPcountVisibleListResults()totalБыло до #473:
Замер на категории с 2114 товарами:
Базовые ~950 мс не зависят от страницы — это подсчёт
totalполным проходом. Сверху линейная доплата за дальность. На 10 000 товаров выйдет 4–5 секунд.Важно: сам
limitв SQL не сломан, Vue получает ровно 20 строк. Сломана схема запросов вокруг него. И сервис всё умеет правильно —COUNT(DISTINCT msProduct.id)одним запросом плюсlimit($limit, $start)(CategoryProductsListService.php:58-67); контроллер просто перестал этим пользоваться.Решение
Быстрый путь, когда построчная проверка заведомо никого не отбрасывает.
Видимость ресурса ограничивает только группа ресурсов с записью доступа:
modResource::findPolicy()собирает политику джойномmodAccessResourceGroup+modResourceGroupResourceи отдаёт пустой набор, если настройка контекста выключена. Отсюда условие — нет ни одной записиmodAccessResourceGroup, либо выключенаaccess_resource_group_enabled, либо пользователь sudo. Тогда работает ровно тот код, что был до #473.Если группы ресурсов настроены — остаётся прежнее сканирование: права важнее скорости.
После правки, та же категория
Время больше не растёт с номером страницы.
Эквивалентность проверена:
totalбыстрого пути совпадает с прежним сканирующим подсчётом в обоих режимах — 2114 против 2114. То есть на этих данных фильтр и правда ничего не отбрасывал.Обратное тоже проверено: создал на стенде группу ресурсов с записью доступа — условие переключилось на
false, после уборки вернулось вtrue.Про тесты
Фильтрация листинга по ACL не покрыта тестами — ни
collectVisibleListPage, ниcountVisibleListResults, ниfilterListResultsByDocumentViewв тестах не упоминаются. Так что зелёные гейты сами по себе сохранность прав здесь не доказывали, отсюда живая проверка выше и новый тест на само условие переключения: ошибка в нём либо вернёт медленный листинг, либо, что хуже, обойдёт проверку прав.tests/Unit/Controllers/CategoryProductsListFastPathTest.php— четыре случая: нет записей ACL, есть записи (1 и 25), выключенная настройка контекста, и memoisation в пределах запроса.Стабу
modXдобавленыgetCount()иoptionOverrides. ПодписьgetCount()намеренно без типов — существующие тесты переопределяют метод в анонимных подклассах со своей, более слабой (CustomerGroupsControllerAclCacheTest), строгая подпись их ломала.Проверка остальных листингов
По просьбе пользователя посмотрел, нет ли того же в других местах. Паттерн сканирования уникален для этого контроллера:
ManagerOrderListService)COUNT(DISTINCT msOrder.id)+limit($limit, $start)OrdersPageService)getCount()+limit($limit, $offset)COUNT+limitcount($modx->getCollection(…))вместоgetCount()вsrc/не встречается.Гейты локально: PHPUnit 834/834, smoke 124/124, PHPStan по изменённому файлу чисто.