Ревью серверной части — Фаза 2: Models и Data Layer

Дата: 2026-06-02
Ветка: dev
Область: server/SHServ/{Models,Entities,Middleware,Helpers,Factory}, server/Fury/Modules/ThinBuilder

Сводка

Проверены модели, Entity-классы, ORM ThinBuilder, фабрики и вспомогательные хелперы. Обнаружено 34 находки: 5 критических, 14 высокого приоритета, 15 среднего/информационного.

🔴 Критические

1. Entity::select_from_db() молча падает при отсутствии строки в БД

Файл: server/SHServ/Middleware/Entity.php:36-45

protected function select_from_db() {
    list($this -> data) = $this -> thin_builder() -> select(
        $this -> entity_tablename,
        [],
        ['id', '=', $this -> entity_id],
        ['id'],
        'DESC',
        [0, 1]
    );
}

Если записи с таким id нет, select() возвращает []. Конструкция list($this->data) = [] присваивает $this->data = null. Поле $was_filled всё равно устанавливается в true. При следующем обращении к свойству через __get() → get() происходит обращение к $this->data[$field_name] где $this->data === null — fatal error.

Последствия: Конструирование Entity с несуществующим ID приводит к fatal error при первом доступе к полю, вместо управляемой ошибки.

Рекомендация: Проверять результат select() и бросать исключение RecordNotFound если строка отсутствует.

2. DeviceAuth::kill() упадёт в production из-за отсутствия колонки update_at

Файлы: server/SHServ/Entities/DeviceAuth.php:7-9, server/SHServ/Middleware/Entity.php:67-79

DeviceAuth::$fields не содержит update_at. Entity::update() безусловно инжектирует update_at в modified_fields и генерирует UPDATE ... SET update_at = ?. Тестовая схема (DevicesModelTransactionTest.php:44-52) создаёт таблицу device_auth с колонкой update_at, поэтому тесты проходят. В production схема создана отдельно, и колонки update_at может не быть — вызов kill() приведёт к SQL-ошибке.

Последствия: Удаление устройства (Devices::remove_device()) вызывает DeviceAuth::kill(), которое бросает PDOException. Токен устройства остаётся активным в БД.

Рекомендация: Либо добавить update_at в production-таблицу device_auth, либо переопределить update() в DeviceAuth без инъекции update_at.

3. Device::remove() не инвалидирует токен авторизации

Файл: server/SHServ/Entities/Device.php:36-39

public function remove() {
    $this -> status = "removed";
    return $this -> update();
}

Метод меняет статус устройства на removed, но не трогает таблицу device_auth. Строка авторизации остаётся со статусом active, и токен продолжает считаться валидным.

Последствия: "Удалённое" устройство теоретически может продолжать отправлять события и выполнять команды, пока токен не протухнет.

Рекомендация: В remove() дополнительно вызывать $this->auth()->kill() или делать remove() транзакционной операцией.

4. Area::remove() выполняет 3 обновления без транзакции

Файл: server/SHServ/Entities/Area.php:267-282

public function remove(): Bool {
    app() -> thin_builder -> update(Device::$table_name, ["area_id" => 0], ...);
    app() -> thin_builder -> update(self::$table_name, ["parent_id" => 0], ...);
    $this -> remove_entity();
    return true;
}

Три независимых запроса к БД без beginTransaction. При падении после первого или второго запроса часть устройств/под-ареа останется с битой ссылкой, а сама ареа может или не удалиться.

Рекомендация: Обернуть в транзакцию.

5. Scripts::select_scripts_by_aliases_types() падает на пустом массиве

Файл: server/SHServ/Models/Scripts.php:63-73

public function select_scripts_by_aliases_types(String $type, Array $uniq_names): Array {
    $result = $this -> thin_builder() -> select(
        Script::$table_name,
        Script::get_fields(),
        [ ["type", "=", $type], "AND", [ "uniq_name", 'IN', $uniq_names ] ]
    );

При $uniq_names = [] генерируется ... WHERE type = ? AND uniq_name IN () — синтаксическая ошибка SQL.

Последствия: Вызов метода с пустым списком псевдонимов бросает PDOException.

Рекомендация: В начале метода делать ранний возврат [] если массив пуст.

🟠 Высокий приоритет

6. ThinBuilder не валидирует операторы WHERE

Файл: server/Fury/Modules/ThinBuilder/ThinBuilderProcessing.php:147-156

$operator = strtoupper($w_item[1]);
if($operator == 'IN'){ ... }
else {
    $sql_parts[] = "{$field} {$operator} ?";
}

Любая строка проходит как оператор прямо в SQL. Значения параметризованы, но сам оператор — сырой текст. Хотя validate_identifier() защищает от инъекции в имена колонок, оператор не проходит whitelist.

Рекомендация: Добавить whitelist допустимых операторов: =, !=, <> <, >, <=, >=, LIKE, IN, IS, NOT IN, BETWEEN.

7. validate_identifier() разрешает числовой старт имени

Файл: server/Fury/Modules/ThinBuilder/ThinBuilderProcessing.php:62-67

Регулярка /^[a-zA-Z0-9_]+$/ пропускает идентификаторы вида 123abc, что недопустимо в SQL. Правильная регулярка: /^[a-zA-Z_][a-zA-Z0-9_]*$/.

8. escape_string_in_arr() — мёртвый код с опасным экранированием

Файл: server/Fury/Modules/ThinBuilder/ThinBuilderProcessing.php:69-79

Метод использует addslashes() вместо prepared statements. Он нигде не вызывается в кодовой базе (кроме определения), но его наличие создаёт риск, что кто-то воспользуется "для быстрого фикса".

Рекомендация: Удалить метод.

9. MetaManager::create_or_update() — race condition SELECT-then-INSERT

Файл: server/SHServ/Models/MetaManager.php:75-86

public function create_or_update(String $name, String $value, String $assignment, int $ent_id): bool {
    $entry = $this -> get_one_by_name($name, $assignment, $ent_id);
    if($entry) {
        $entry -> value = $value;
        $result = $entry -> update();
    } else {
        $result = $this -> create($name, $value, $assignment, $ent_id);
    }

При конкурентных запросах два потока могут одновременно пройти if(!$entry) и вставить дубль.

Рекомендация: Использовать INSERT ... ON DUPLICATE KEY UPDATE (MySQL) или добавить UNIQUE-индекс (assignment, ent_id, name) и ловить дубли на уровне БД.

10. User::__construct() жадно загружает профиль

Файл: server/SHServ/Entities/User.php:16-28

Каждое создание User — даже при массовой выборке — инициирует дополнительный SELECT из profiles. Это классическая N+1 проблема.

Рекомендация: Сделать загрузку профиля ленивой (через get_pet_instance как last_session()), либо добавить eager-join в Factory\Getter::get_user_by().

11. Scripts::remove_scope() — файловая система и БД рассинхронизируются при ошибке

Файл: server/SHServ/Models/Scripts.php:29-51

if(!@unlink($filepath)) { return false; }
$script = new Script($name);
$script -> remove();

Если unlink() прошёл успешно, а $script->remove() упал, файл уже удалён из диска, но запись в БД осталась.

Рекомендация: Либо удалять файл после успешного удаления из БД, либо оборачивать в транзакцию с компенсирующим действием.

12. Entity::update() всегда возвращает true, игнорируя результат БД

Файл: server/SHServ/Middleware/Entity.php:67-79

$this -> thin_builder() -> update($this -> entity_tablename, $this -> modified_fields, $where);
$this -> modified_fields = [];
return true;

ThinBuilder::update() возвращает rowCount(), но Entity игнорирует его. Если UPDATE не затронул ни одной строки (например, запись была удалена параллельно), метод всё равно вернёт true.

Рекомендация: Проверять rowCount() > 0 или возвращать его, и не сбрасывать modified_fields при нулевом rowCount.

13. IN-клауза с не-массивом падает на PHP 8+

Файл: server/Fury/Modules/ThinBuilder/ThinBuilderProcessing.php:149-151

if($operator == 'IN'){
    $placeholders = array_fill(0, count($w_item[2]), '?');

В PHP 7 count("string") возвращает 1. В PHP 8+ count() на скаляре бросает TypeError. Если по ошибке передать строку вместо массива в IN, приложение упадёт с необработанным исключением.

Рекомендация: Добавить проверку is_array($w_item[2]) с понятным исключением.

14. Короткий синтаксис where не поддерживает IN

Файл: server/Fury/Modules/ThinBuilder/ThinBuilderProcessing.php:115-130

Плоский массив из двух элементов ['field', ['v1', 'v2']] (подразумевая IN) превращается в ['field', '=', ['v1', 'v2']] из-за ветки count($where) === 2. В итоге массив значений пытается забиндиться как скаляр.

Рекомендация: В normalize_where обрабатывать случай, когда второй элемент — массив (IN).

15. Entity::update() не сбрасывает modified_fields при исключении

Файл: server/SHServ/Middleware/Entity.php:67-79

Если thin_builder()->update() бросает PDOException, modified_fields остаётся заполненным. При повторном вызове update() (например, после retry или в catch-блоке выше) будут записаны устаревшие + новые изменения.

Рекомендация: Обернуть в try/finally и сбрасывать modified_fields только при успехе.

16. Device::set_device_token() делает избыточные вызовы auth()

Файл: server/SHServ/Entities/Device.php:94-102

public function set_device_token(String $token) {
    $this -> device_api() -> remote_set_token($token);
    if(!$this -> auth()) return false;
    $this -> auth() -> device_token = $token;        // вызов #2
    $this -> device_api_instance -> set_local_token($token);
    return $this -> auth() -> update();              // вызов #3
}

auth() вызывается трижды. Метод device_api() тоже вызывает auth() внутри себя.

Рекомендация: Закэшировать $auth = $this->auth() в переменную.

17. Scripts::set_script_state() чрезвычайно неэффективен

Файл: server/SHServ/Models/Scripts.php:75-128

Для scope-типа на каждый вызов делает ReflectionClass на все scope-инстансы. Для regular/action вызывает regular_scripts_list() / actions_scripts_list(), которые тянут из БД все записи и делают array_filter в цикле (сложность O(n·m)).

Рекомендация: Построить хеш-таблицу (keyed by uniq_name) один раз вместо линейного поиска.

18. Area::get_exists_types() сканирует всю таблицу вместо DISTINCT

Файл: server/SHServ/Models/Areas.php:36-57

$areas = app() -> thin_builder -> select( Area::$table_name, [ "type" ], [] );
// ... array_map + ручной foreach-dedup

Запрашивает все строки таблицы, затем дедуплицирует в PHP. При 1000 ареа это 1000 строк в памяти.

Рекомендация: Использовать SELECT DISTINCT type FROM areas через query() или добавить метод distinct() в ThinBuilder.

19. Area::get_recursive_inner_areas_ids() жёстко ограничен глубиной 10

Файл: server/SHServ/Entities/Area.php:129-158

if($lvl >= 10) { return []; }

При глубине дерева > 10 метод молча отрезает хвост. Нет ни лога, ни исключения.

Рекомендация: Либо увеличить лимит, либо бросать исключение при превышении.

20. ThinBuilder::table_fields() ломается на типах без длины

Файл: server/Fury/Modules/ThinBuilder/ThinBuilder.php:197-213

list($type, $length) = explode('(', $raw_field[1]);

Для типов INT, TEXT, DATETIME, BOOLEAN в SHOW COLUMNS нет скобки, и explode возвращает массив из одного элемента. list($type, $length) присвоит $length = null, но $length = intval($length) даст 0 — логика выживет, но это хрупкое поведение.

🟡 Средний / Информационный

21. ThinBuilder::tables() только для MySQL

Файл: server/Fury/Modules/ThinBuilder/ThinBuilder.php:215-222

SHOW TABLES — MySQL-специфичный синтаксис. Драйвер поддерживает SQLite (create_connect), но tables() с ним не работает.

22. Несогласованные типы возврата ошибок в Models

Areas::create_new_area() → Area | null
Devices::connect_new_device() → Device | Array (массив с err_alias)

Это заставляет вызывающий код делать is_array() + is_object() проверки вразнобой.

Рекомендация: Ввести统一ный Result-объект или Exception-based flow.

23. Area::alias_is_uniq() не исключает саму себя при обновлении

Файл: server/SHServ/Models/Areas.php:27-34

При вызове для проверки уникальности во время обновления существующей ареа её собственный alias будет засчитан как дубль. Метод используется только в create_new_area(), но если его переиспользовать для update — баг.

24. Area и AreaPlacing trait дублируют метод place_in_area_id

Файл: server/SHServ/Entities/Area.php:68-71, server/SHServ/Entities/Traits/AreaPlacing.php:12-15

Оба определяют идентичный place_in_area_id(). В Area он имеет приоритет (класс побеждает trait). Для Script и Device используется trait-версия. Дублирование без причины.

25. Creator::create_meta() подделывает update_at при конструировании

Файл: server/SHServ/Factory/Creator.php:32-50

return new Meta($meta_id, array_merge([
    "id" => $meta_id,
    "update_at" => $data["create_at"]   // фиктивное значение
], $data));

Единственная причина — удовлетворить Entity::update(), который требует update_at. Это workaround вместо исправления корневой проблемы.

26. Factory\Getter::get_profile_by() тянет только id, форсируя ленивую загрузку

Файл: server/SHServ/Factory/Getter.php:25-39

В отличие от get_user_by() (который тянет все поля), get_profile_by() запрашивает только id, а затем создаёт new Profile($result[0]["id"]) без данных. Это заставляет Entity при первом доступе к свойству делать select_from_db().

Рекомендация: Либо загружать все поля сразу, либо явно документировать ленивость.

27. Model-конструктор вызывает devtools на каждую инстанциацию

Файл: server/SHServ/Middleware/Model.php:8-13

public function __construct(){
    parent::__construct();
    app() -> devtools -> using_model(get_class($this));
}

using_model() вызывается при каждом new Devices(), new Scripts() и т.д. В production это лишний вызов метода на объекте-синглтоне.

28. Area::remove() присваивает строку "0" вместо integer

Файл: server/SHServ/Models/Areas.php:20

"parent_id" => "0",

parent_id везде сравнивается как integer (if(!$this->get_parent_id()), $this->parent_id и т.д.). Строка "0" в PHP приводится к 0 в числовом контексте, но это неявное поведение.

29. MetaWrap::$meta_manager_instance — статический mutable singleton

Файл: server/SHServ/Helpers/MetaWrap.php:9

protected static $meta_manager_instance;

Один инстанс MetaManager разделяется между всеми MetaWrap. Не критично, но затрудняет тестирование и многопоточность.

30. ThinBuilder::query() — raw SQL без параметризации

Файл: server/Fury/Modules/ThinBuilder/ThinBuilder.php:17-42

Метод принимает строку SQL и выполняет напрямую через $pdo->query(). В тестах используется для CREATE TABLE, но если где-то в production коде прокрадётся пользовательский input — SQL-инъекция.

Рекомендация: Задокументировать как "internal/DDL only" или обернуть в internal-маркер.

31. Нет DB-уровневых UNIQUE-индексов

Во всём слое Models уникальность проверяется через count(...) или table_row_is_exists() на уровне приложения. Нет гарантии от race condition на уровне БД для:

areas.alias
devices.alias
devices.device_hard_id (active)
users.alias / users.email
meta.(assignment, ent_id, name)

Рекомендация: Добавить UNIQUE INDEX в production-схему.

32. Scripts::get_scripts_list() — линейный поиск вместо хеш-таблицы

Файл: server/SHServ/Models/Scripts.php:170-184

foreach($scripts as $alias => $script_data) {
    $script_entity = array_values(array_filter($scripts_entities, function($item) use($alias) {
        return $item -> uniq_name == $alias;
    }));
    $data[] = $this -> prepare_script_to_view($type, $script_data, $script_entity[0] ?? null);
}

Для N скриптов и M entity сложность O(N·M). При 50 скриптах и 50 entity — 2500 сравнений вместо 50 хеш-поисков.

33. Area::remove() напрямую обращается к app()->thin_builder вместо $this->thin_builder()

Файл: server/SHServ/Entities/Area.php:268-278

app() -> thin_builder -> update(...);

В то время как внутри того же класса используется $this -> thin_builder() -> select(...). Функционально эквивалентно, но нарушает единообразие.

34. Несоответствие тестовой и production схемы device_auth

Файл: server/tests/DevicesModelTransactionTest.php:44-52

Тесты создают device_auth с update_at TEXT, но DeviceAuth::$fields не содержит эту колонку. Это означает, что:

Тесты проходят
Production может отличаться
Нет единого источника правды для схемы

Рекомендация: Создать единую миграционную систему или хотя бы файл schema.sql в репозитории.

Приоритет	Находка	Файл
🔴 P0	Entity::select_from_db() падает на отсутствующей строке	`Middleware/Entity.php`
🔴 P0	DeviceAuth::kill() + отсутствие update_at	`Entities/DeviceAuth.php`, `Middleware/Entity.php`
🔴 P0	Device::remove() не инвалидирует токен	`Entities/Device.php`
🔴 P0	Area::remove() без транзакции	`Entities/Area.php`
🔴 P0	IN () на пустом массиве	`Models/Scripts.php`
🟠 P1	ThinBuilder whitelist операторов	`Fury/Modules/ThinBuilder/ThinBuilderProcessing.php`
🟠 P1	validate_identifier числовой старт	`Fury/Modules/ThinBuilder/ThinBuilderProcessing.php`
🟠 P1	MetaManager race condition	`Models/MetaManager.php`
🟠 P1	User жадно тянет профиль	`Entities/User.php`
🟠 P1	Entity::update() возвращает true всегда	`Middleware/Entity.php`
🟠 P1	Scripts::remove_scope() файловая/БД рассинхрон	`Models/Scripts.php`
🟠 P1	IN с не-массивом на PHP 8+	`Fury/Modules/ThinBuilder/ThinBuilderProcessing.php`
🟠 P1	Короткий where и IN	`Fury/Modules/ThinBuilder/ThinBuilderProcessing.php`
🟡 P2	DISTINCT вместо PHP-dedup	`Models/Areas.php`
🟡 P2	O(n·m) в Scripts::get_scripts_list()	`Models/Scripts.php`
🟡 P2	Удалить escape_string_in_arr()	`Fury/Modules/ThinBuilder/ThinBuilderProcessing.php`
🟡 P2	Единая схема БД в репозитории	новый файл `schema.sql`