docs: перенос документации из IPWhiteList + CONTEXT.md (резюме для нового чата)
This commit is contained in:
@@ -0,0 +1,141 @@
|
||||
# Код-ревью schema.sql — индексы, constraint'ы, FK, типы
|
||||
|
||||
> Дата: 2026-05-30
|
||||
|
||||
## 🔴 Нет уникального constraint на активный CIDR компании
|
||||
|
||||
Дубликаты предотвращаются только в коде (`createEntry`), а это уязвимо к гонке (см. ревью queries.js — TOCTOU). БД должна гарантировать уникальность активного адреса в рамках компании независимо от кода:
|
||||
|
||||
```sql
|
||||
CREATE UNIQUE INDEX IF NOT EXISTS uq_entries_active_cidr
|
||||
ON whitelist_entries(company_id, value_cidr) WHERE deleted_at IS NULL;
|
||||
```
|
||||
|
||||
Это превращает существующий `idx_entries_active` в уникальный (можно заменить им) — параллельные INSERT одинакового CIDR упадут на втором, гонка закрывается на уровне БД. Пересечения (overlaps) так не закрыть — для них нужен `inet`/GiST (см. ниже) или транзакция.
|
||||
|
||||
## 🔴 value_cidr хранится как VARCHAR — нет валидации и пересечений на уровне БД
|
||||
|
||||
```sql
|
||||
value_cidr VARCHAR(18) NOT NULL,
|
||||
```
|
||||
|
||||
`VARCHAR(18)` хранит произвольную строку — БД не проверяет, что это валидный CIDR, и не умеет искать пересечения. Production-вариант — нативный тип `cidr`:
|
||||
|
||||
```sql
|
||||
value_cidr CIDR NOT NULL,
|
||||
```
|
||||
|
||||
Преимущества: БД отвергает мусор; операторы `&&` (overlaps), `<<=` (subnet); можно сделать exclusion constraint на пересечения внутри компании:
|
||||
|
||||
```sql
|
||||
CREATE EXTENSION IF NOT EXISTS btree_gist;
|
||||
ALTER TABLE whitelist_entries
|
||||
ADD CONSTRAINT excl_entries_overlap
|
||||
EXCLUDE USING gist (company_id WITH =, value_cidr inet_ops WITH &&)
|
||||
WHERE (deleted_at IS NULL);
|
||||
```
|
||||
|
||||
Это закрывает гонку пересечений (RC №2 из ревью queries.js) на уровне БД. Если тип менять не хотите — оставить VARCHAR, но тогда уникальность/пересечения держатся только на транзакциях в коде. Минимум — CHECK на формат:
|
||||
|
||||
```sql
|
||||
ALTER TABLE whitelist_entries
|
||||
ADD CONSTRAINT chk_cidr_format CHECK (value_cidr ~ '^(\d{1,3}\.){3}\d{1,3}/\d{1,2}$');
|
||||
```
|
||||
|
||||
## 🔴 FK без ON DELETE / нет каскада
|
||||
|
||||
```sql
|
||||
company_id INTEGER NOT NULL REFERENCES companies(id),
|
||||
```
|
||||
|
||||
Поведение по умолчанию — `NO ACTION`: удалить компанию нельзя, пока есть записи. Для сервиса с soft-delete это, скорее, правильно (компании не удаляются физически). Но это надо сделать осознанно: явно указать `ON DELETE RESTRICT` (документирует намерение) либо `ON DELETE CASCADE`, если компании реально удаляются. Сейчас умолчание неявное.
|
||||
|
||||
## 🟡 audit_log.company_id без FK и без типизации действий
|
||||
|
||||
```sql
|
||||
company_id INTEGER NOT NULL,
|
||||
action VARCHAR(32) NOT NULL,
|
||||
```
|
||||
|
||||
`company_id` в audit_log не ссылается на `companies` — допустимо (аудит должен переживать удаление компании), но тогда стоит это зафиксировать комментарием. `action` — свободный VARCHAR, можно записать что угодно. Ограничить:
|
||||
|
||||
```sql
|
||||
ALTER TABLE audit_log
|
||||
ADD CONSTRAINT chk_action CHECK (action IN ('CREATE','UPDATE','DELETE'));
|
||||
```
|
||||
|
||||
`entry_id` тоже без FK — ок (запись может быть hard-удалена в будущем, аудит сохраняется).
|
||||
|
||||
## 🟡 Индекс аудита недостаточен для типичных запросов
|
||||
|
||||
```sql
|
||||
CREATE INDEX idx_audit_company ON audit_log(company_id);
|
||||
```
|
||||
|
||||
Аудит почти всегда смотрят «по компании, свежие сверху». Нужен составной с временем:
|
||||
|
||||
```sql
|
||||
CREATE INDEX IF NOT EXISTS idx_audit_company_time
|
||||
ON audit_log(company_id, created_at DESC);
|
||||
```
|
||||
|
||||
## 🟡 Нет автообновления updated_at
|
||||
|
||||
`updated_at` в companies имеет DEFAULT NOW(), но при UPDATE не меняется автоматически — код должен сам выставлять. Для надёжности — триггер:
|
||||
|
||||
```sql
|
||||
CREATE OR REPLACE FUNCTION set_updated_at() RETURNS trigger AS $$
|
||||
BEGIN NEW.updated_at = NOW(); RETURN NEW; END $$ LANGUAGE plpgsql;
|
||||
|
||||
CREATE TRIGGER trg_companies_updated
|
||||
BEFORE UPDATE ON companies
|
||||
FOR EACH ROW EXECUTE FUNCTION set_updated_at();
|
||||
```
|
||||
|
||||
## 🟡 SERIAL вместо IDENTITY
|
||||
|
||||
`SERIAL` — легаси-приём. Для нового кода предпочтительнее:
|
||||
|
||||
```sql
|
||||
id INTEGER GENERATED ALWAYS AS IDENTITY PRIMARY KEY,
|
||||
```
|
||||
|
||||
Не критично, но это современный стандарт PG 10+ (чище права на sequence, нельзя случайно вставить id вручную).
|
||||
|
||||
## 🟡 custom_limit без CHECK на неотрицательность
|
||||
|
||||
```sql
|
||||
custom_limit INTEGER DEFAULT NULL,
|
||||
```
|
||||
|
||||
Можно записать отрицательный лимит. Добавить:
|
||||
|
||||
```sql
|
||||
ALTER TABLE companies
|
||||
ADD CONSTRAINT chk_custom_limit CHECK (custom_limit IS NULL OR custom_limit >= 0);
|
||||
```
|
||||
|
||||
(Связано с багом `custom_limit = 0` из ревью queries.js — на уровне БД 0 разрешён, в коде игнорируется.)
|
||||
|
||||
## 🟡 comment/created_by — длины
|
||||
|
||||
`created_by VARCHAR(255)` под email — ок. `comment VARCHAR(255)` — приемлемо, но если ТЗ не ограничивает комментарий — рассмотреть TEXT. Не критично.
|
||||
|
||||
## 🟢 Изоляция через БД
|
||||
|
||||
Структурно обойти изоляцию нельзя: записи привязаны к `company_id`, утечка возможна только через код (запрос без фильтра company_id — см. `/export` в ревью server.js), не через схему.
|
||||
|
||||
## 🟢 Партиal-индекс idx_entries_active
|
||||
|
||||
Правильный приём — индекс только по активным записям, soft-deleted не раздувают индекс. Хорошо.
|
||||
|
||||
---
|
||||
|
||||
**Итог:**
|
||||
1. 🔴 Добавить UNIQUE на активный (company_id, value_cidr) — закрывает гонку дублей на уровне БД.
|
||||
2. 🔴 Рассмотреть тип `CIDR` + exclusion constraint (btree_gist) — закрывает гонку пересечений в БД; иначе минимум CHECK на формат.
|
||||
3. 🔴 Явно задать ON DELETE для FK company_id.
|
||||
4. 🟡 CHECK на action, на custom_limit ≥ 0; составной индекс аудита (company_id, created_at DESC); FK-политику аудита задокументировать.
|
||||
5. 🟡 Триггер updated_at; перейти на IDENTITY вместо SERIAL.
|
||||
|
||||
Главное: текущая схема перекладывает уникальность и проверку пересечений целиком на код, который к ним уязвим в гонках. Перенос этих гарантий в БД (UNIQUE + exclusion/CHECK) — основной production-апгрейд.
|
||||
Reference in New Issue
Block a user