Дозакрыты находки ревью по слиянию сущностей
- Правило покрытия получило второй разряд (условный, как у точек), запрет вырождения формы и счёт содержательных элементов ряда: скелет из скаляров и ряд из null больше не затирают маршрут. Победитель внутри доставки стал функцией множества версий — общим помощником с точками, — а провенанс поднимается и при совпавшем хеше, иначе отложенная доставка возвращала витрину к прежнему содержимому. - Одно поле не того типа больше не уносит сущность, а пропуски видны в учётной записи доставки (миграция 00008, NULL = «не измерялось»); каноническая форма считается один раз и вне транзакции; откат бинаря поверх новой схемы отказывает на старте; текст ошибки разбора не несёт значений из тела. - Ревью кода профилем deep (девять проходов) нашло две регрессии и обе закрыты: безусловный второй разряд запирал законный досчёт навсегда, а выбор победителя был квадратичен по числу присланных версий одного ключа.
This commit is contained in:
+63
-12
@@ -117,7 +117,7 @@ func (s *Service) Fold(ctx context.Context, deliveryID string) (stats Stats, err
|
||||
}
|
||||
stats = Stats{}
|
||||
err = fmt.Errorf("%w: %v", ErrPanicked, r) //nolint:errorlint // причину раскрываем текстом, sentinel — для ветвления
|
||||
s.fail(ctx, deliveryID, err, nil)
|
||||
s.fail(ctx, deliveryID, err, parseResidue{})
|
||||
}()
|
||||
|
||||
d, err := s.store.DeliveryForParse(ctx, deliveryID)
|
||||
@@ -131,7 +131,7 @@ func (s *Service) Fold(ctx context.Context, deliveryID string) (stats Stats, err
|
||||
|
||||
body, err := s.readBody(d.RawPath)
|
||||
if err != nil {
|
||||
s.fail(ctx, deliveryID, err, nil)
|
||||
s.fail(ctx, deliveryID, err, parseResidue{})
|
||||
return stats, err
|
||||
}
|
||||
|
||||
@@ -149,7 +149,11 @@ func (s *Service) Fold(ctx context.Context, deliveryID string) (stats Stats, err
|
||||
// Список непокрытых секций переживает отказ: доставка, у которой не
|
||||
// определился слой, обязана остаться записью о том, что в теле есть
|
||||
// невосстановимая секция.
|
||||
s.fail(ctx, deliveryID, err, parsed.Uncovered)
|
||||
//
|
||||
// А вот число пропущенных сущностей — НЕ переживает: разбор, вернувший
|
||||
// ошибку, отдаёт нулевые счётчики по построению, а не по измерению, и
|
||||
// записать этот ноль значило бы объявить доставку проверенной.
|
||||
s.fail(ctx, deliveryID, err, residueOf(parsed))
|
||||
return stats, err
|
||||
}
|
||||
|
||||
@@ -171,7 +175,12 @@ func (s *Service) Fold(ctx context.Context, deliveryID string) (stats Stats, err
|
||||
ReceivedAt: d.ReceivedAt,
|
||||
})
|
||||
if err != nil {
|
||||
s.fail(ctx, deliveryID, err, parsed.Uncovered)
|
||||
// Здесь разбор досчитал: отказало слияние. Значит счётчик пропусков
|
||||
// измерен и обязан дойти до учёта — в отличие от ветки выше.
|
||||
s.fail(ctx, deliveryID, err, parseResidue{
|
||||
uncovered: parsed.Uncovered,
|
||||
skipped: skippedEntities(parsed),
|
||||
})
|
||||
return stats, err
|
||||
}
|
||||
stats.MergeStats = merge
|
||||
@@ -184,10 +193,11 @@ func (s *Service) Fold(ctx context.Context, deliveryID string) (stats Stats, err
|
||||
status = store.ParsePartial
|
||||
}
|
||||
out := store.ParseOutcome{
|
||||
Status: status,
|
||||
Points: int64(stats.Points),
|
||||
Layer: stats.Layer,
|
||||
Uncovered: parsed.Uncovered,
|
||||
Status: status,
|
||||
Points: int64(stats.Points),
|
||||
Layer: stats.Layer,
|
||||
Uncovered: parsed.Uncovered,
|
||||
SkippedEntities: skippedEntities(parsed),
|
||||
}
|
||||
if err := s.finish(ctx, deliveryID, out); err != nil {
|
||||
s.log.ErrorContext(ctx, "delivery fold failed", "error", err, "delivery_id", deliveryID)
|
||||
@@ -237,6 +247,7 @@ func (s *Service) logResult(ctx context.Context, deliveryID string, st Stats) {
|
||||
"records", st.Records,
|
||||
"records_written", st.RecordsWritten,
|
||||
"entities_held", st.EntitiesHeld,
|
||||
"entities_diverging", st.EntitiesDiverging,
|
||||
"skipped_entities", skippedEntities,
|
||||
"layer", st.Layer,
|
||||
"layer_mismatch", st.LayerMismatch,
|
||||
@@ -255,6 +266,9 @@ func (s *Service) logResult(ctx context.Context, deliveryID string, st Stats) {
|
||||
if len(st.HeldAt) > 0 {
|
||||
attrs = append(attrs, "held_at", formatEntityRefs(st.HeldAt))
|
||||
}
|
||||
if len(st.DivergingAt) > 0 {
|
||||
attrs = append(attrs, "diverging_at", formatEntityRefs(st.DivergingAt))
|
||||
}
|
||||
|
||||
// Доставка, у которой отброшены ВСЕ точки, — это сломавшийся формат, а не
|
||||
// штатная работа. Без этого условия смена формата метки выглядела бы как
|
||||
@@ -270,6 +284,14 @@ func (s *Service) logResult(ctx context.Context, deliveryID string, st Stats) {
|
||||
// за отказ объединять поля: событие обязано быть видно, потому что на
|
||||
// живом потоке оно не наступало ни разу и правило держится на этом.
|
||||
s.log.WarnContext(ctx, "delivery folded, poorer entity version held", attrs...)
|
||||
case st.EntitiesDiverging > 0:
|
||||
// Событие другого рода и с другим лечением: в одном теле приехали
|
||||
// версии одного ключа с разным содержанием. Победитель лёг в витрину
|
||||
// целиком, терять нечего — но корпус такого не производил, и молчать
|
||||
// об этом нельзя. Отдельной ветвью, а не общей с удержанием: сообщение
|
||||
// «удержана обеднённая версия» отправляло бы владельца искать то, чего
|
||||
// не случилось.
|
||||
s.log.WarnContext(ctx, "delivery folded, entity versions diverge in one body", attrs...)
|
||||
case allEntitiesSkipped:
|
||||
s.log.WarnContext(ctx, "delivery folded, all entities skipped", attrs...)
|
||||
case st.UncoveredDropped > 0:
|
||||
@@ -349,7 +371,35 @@ func (s *Service) finish(ctx context.Context, deliveryID string, out store.Parse
|
||||
// большое тело, исчерпанный дедлайн — свойства самой доставки, и повторять их
|
||||
// бесполезно: статус `failed`, тело ждёт пересборки. Приём при этом не
|
||||
// затрагивается: сохранили значит приняли.
|
||||
func (s *Service) fail(ctx context.Context, deliveryID string, cause error, uncovered []string) {
|
||||
// parseResidue — то, что разбор успел узнать о доставке до отказа и что обязано
|
||||
// пережить его в учёте: список непокрытых секций и число пропущенных сущностей.
|
||||
//
|
||||
// Структурой, а не двумя параметрами: у `fail` их стало бы четыре, и следующий
|
||||
// счётчик неизбежно перепутали бы местами с предыдущим. Пустое значение —
|
||||
// «разбор до этого не дошёл», и оно честно: отказ на чтении тела ничего о
|
||||
// содержимом не знает.
|
||||
type parseResidue struct {
|
||||
uncovered []string
|
||||
// skipped — nil означает «разбор до конца не дошёл, пропусков никто не
|
||||
// считал». Ноль означал бы «проверено, терять нечего», а по этому числу
|
||||
// ретеншен принимает необратимое решение об удалении тела.
|
||||
skipped *int64
|
||||
}
|
||||
|
||||
func residueOf(parsed hae.Result) parseResidue {
|
||||
return parseResidue{uncovered: parsed.Uncovered}
|
||||
}
|
||||
|
||||
// skippedEntities — сколько сущностей с собственным `id` разбор пропустил.
|
||||
// Сумма трёх классов, а не три колонки: ретеншен спрашивает «есть ли что
|
||||
// терять», а не «почему», а разбор класса живёт в логе свёртки, где все три
|
||||
// счётчика идут атрибутами.
|
||||
func skippedEntities(parsed hae.Result) *int64 {
|
||||
n := int64(parsed.SkippedNoID + parsed.SkippedEntityNoTime + parsed.SkippedEntityMalformed)
|
||||
return &n
|
||||
}
|
||||
|
||||
func (s *Service) fail(ctx context.Context, deliveryID string, cause error, residue parseResidue) {
|
||||
if store.Transient(cause) {
|
||||
// WARN, а не ERROR: пройдёт само, разбирать нечего. Строка нужна, чтобы
|
||||
// повтор не выглядел беспричинным.
|
||||
@@ -375,9 +425,10 @@ func (s *Service) fail(ctx context.Context, deliveryID string, cause error, unco
|
||||
// строка здесь оборвала бы цепочку наследования, то есть изменила бы
|
||||
// результат пересборки журнала.
|
||||
out := store.ParseOutcome{
|
||||
Status: store.ParseFailed,
|
||||
Layer: keepLayer,
|
||||
Uncovered: uncovered,
|
||||
Status: store.ParseFailed,
|
||||
Layer: keepLayer,
|
||||
Uncovered: residue.uncovered,
|
||||
SkippedEntities: residue.skipped,
|
||||
}
|
||||
if err := s.finish(ctx, deliveryID, out); err != nil {
|
||||
s.log.ErrorContext(ctx, "delivery parse status not recorded", "error", err, "delivery_id", deliveryID)
|
||||
|
||||
@@ -386,3 +386,131 @@ func TestFoldНепонятоеСодержимоеВыводитИзОчере
|
||||
t.Errorf("parse_status = %q, ожидался %q", status, store.ParseFailed)
|
||||
}
|
||||
}
|
||||
|
||||
// Пропущенная сущность обязана быть видна в БАЗЕ, а не только в логе. Ретеншен
|
||||
// сырого архива решает «что потеряется, если тело удалить», по учётной записи,
|
||||
// и до этого счётчика получал ответ «терять нечего» ровно там, где потеряна
|
||||
// тренировка с маршрутом: сущность в витрину не попала, список непокрытых
|
||||
// секций пуст, статус `parsed`.
|
||||
func TestFoldПропускиСущностейВидныВУчёте(t *testing.T) {
|
||||
t.Parallel()
|
||||
|
||||
f, arch, st := newFold(t)
|
||||
ctx := context.Background()
|
||||
|
||||
deliver(t, arch, st, "d1", "Minutes", "a1", fixture(t, "handmade_entities.json"))
|
||||
stats, err := f.Fold(ctx, "d1")
|
||||
if err != nil {
|
||||
t.Fatalf("свёртка: %v", err)
|
||||
}
|
||||
skipped := stats.SkippedNoID + stats.SkippedEntityNoTime + stats.SkippedEntityMalformed
|
||||
if skipped == 0 {
|
||||
t.Fatal("фикстура перестала давать пропуски — тест проверяет не то")
|
||||
}
|
||||
|
||||
d, err := st.LastDelivery(ctx)
|
||||
if err != nil {
|
||||
t.Fatalf("чтение доставки: %v", err)
|
||||
}
|
||||
if d.SkippedEntities == nil {
|
||||
t.Fatal("счётчик пропусков пуст: доставка выглядит как «не измерялась»")
|
||||
}
|
||||
if *d.SkippedEntities != int64(skipped) {
|
||||
t.Errorf("в базе %d пропусков, разбор дал %d", *d.SkippedEntities, skipped)
|
||||
}
|
||||
}
|
||||
|
||||
// Число замещает прежнее значение целиком, включая замещение нулём: доставка,
|
||||
// пропуски которой исчезли вместе с поумневшим разбором, не должна остаться
|
||||
// помеченной навсегда.
|
||||
func TestFoldПересвёрткаБезПропусковОбнуляетСчётчик(t *testing.T) {
|
||||
t.Parallel()
|
||||
|
||||
f, arch, st := newFold(t)
|
||||
ctx := context.Background()
|
||||
|
||||
deliver(t, arch, st, "d1", "Minutes", "a1", fixture(t, "handmade_entities.json"))
|
||||
if _, err := f.Fold(ctx, "d1"); err != nil {
|
||||
t.Fatalf("свёртка: %v", err)
|
||||
}
|
||||
before, err := st.LastDelivery(ctx)
|
||||
if err != nil {
|
||||
t.Fatalf("чтение доставки: %v", err)
|
||||
}
|
||||
if before.SkippedEntities == nil || *before.SkippedEntities == 0 {
|
||||
t.Fatal("фикстура перестала давать пропуски — тест проверяет не то")
|
||||
}
|
||||
|
||||
// Тело подменяется на такое же, но без кривых элементов: ровно то, что
|
||||
// произойдёт при пересвёртке поумневшим разбором.
|
||||
deliver(t, arch, st, "d2", "Minutes", "a1", fixture(t, "workout_indoor.json"))
|
||||
if _, err := f.Fold(ctx, "d2"); err != nil {
|
||||
t.Fatalf("повторная свёртка: %v", err)
|
||||
}
|
||||
after, err := st.LastDelivery(ctx)
|
||||
if err != nil {
|
||||
t.Fatalf("чтение доставки: %v", err)
|
||||
}
|
||||
if after.SkippedEntities == nil || *after.SkippedEntities != 0 {
|
||||
t.Errorf("счётчик %v, ожидался ноль", after.SkippedEntities)
|
||||
}
|
||||
}
|
||||
|
||||
// Доставка, свёрнутая разбором, который пропусков не считал, обязана быть
|
||||
// отличима от доставки с нулём: подстановка нуля объявила бы её проверенной, и
|
||||
// ретеншен получил бы ложное «терять нечего» с видом измерения.
|
||||
func TestFoldДоНачалаУчётаПропускиНеИзмерены(t *testing.T) {
|
||||
t.Parallel()
|
||||
|
||||
_, arch, st := newFold(t)
|
||||
ctx := context.Background()
|
||||
|
||||
deliver(t, arch, st, "d1", "Minutes", "a1", fixture(t, "minute.json"))
|
||||
d, err := st.LastDelivery(ctx)
|
||||
if err != nil {
|
||||
t.Fatalf("чтение доставки: %v", err)
|
||||
}
|
||||
if d.SkippedEntities != nil {
|
||||
t.Errorf("несвёрнутая доставка отдаёт %d вместо «не измерялось»", *d.SkippedEntities)
|
||||
}
|
||||
}
|
||||
|
||||
// Отказ, при котором разбор не досчитал, обязан оставить счётчик НЕТРОНУТЫМ.
|
||||
// Ноль здесь означал бы «проверено, терять нечего» — то самое ложное измерение,
|
||||
// ради отказа от которого колонка заведена без умолчания. Тело при этом может
|
||||
// нести сотни тренировок, ни одна из которых не сохранена.
|
||||
func TestFoldОтказРазбораНеПодделываетСчётчикПропусков(t *testing.T) {
|
||||
t.Parallel()
|
||||
|
||||
f, arch, st := newFold(t)
|
||||
ctx := context.Background()
|
||||
|
||||
// Сперва успешная свёртка: счётчик измерен и ненулевой.
|
||||
deliver(t, arch, st, "d1", "Minutes", "a1", fixture(t, "handmade_entities.json"))
|
||||
if _, err := f.Fold(ctx, "d1"); err != nil {
|
||||
t.Fatalf("свёртка: %v", err)
|
||||
}
|
||||
before, err := st.LastDelivery(ctx)
|
||||
if err != nil {
|
||||
t.Fatalf("чтение доставки: %v", err)
|
||||
}
|
||||
if before.SkippedEntities == nil || *before.SkippedEntities == 0 {
|
||||
t.Fatal("фикстура перестала давать пропуски — тест проверяет не то")
|
||||
}
|
||||
|
||||
// Теперь тело, которое разбор не понимает: секция есть, но конверт оборван.
|
||||
deliver(t, arch, st, "d2", "Minutes", "a1", []byte(`{"data":{"workouts":[`))
|
||||
if _, err := f.Fold(ctx, "d2"); err == nil {
|
||||
t.Fatal("разбор оборванного тела не отказал")
|
||||
}
|
||||
after, err := st.LastDelivery(ctx)
|
||||
if err != nil {
|
||||
t.Fatalf("чтение доставки: %v", err)
|
||||
}
|
||||
if after.ID != "d2" {
|
||||
t.Fatalf("прочитана доставка %q, ожидалась d2", after.ID)
|
||||
}
|
||||
if after.SkippedEntities != nil {
|
||||
t.Errorf("отказ разбора записал %d пропусков как измерение", *after.SkippedEntities)
|
||||
}
|
||||
}
|
||||
|
||||
Reference in New Issue
Block a user