feat(enterprise-import): conta as linhas descartadas por motivo - #23
Conversation
ad53095 to
5185b8e
Compare
O progresso do job guardava done, total e page, e nada sobre o que foi descartado. Cada caminho de perda era um continue mudo ou um conflito engolido pelo ON CONFLICT, então uma importação que perdesse metade das linhas terminava com a mesma cara de uma que não perdeu nenhuma. Passa a contar quatro motivos: linha recusada pelo customize ou por FK não resolvida, chave natural vazia, repetida dentro da página, e conflito engolido no insert. O último não dá para saber pelo retorno do insert — nenhum dos dois caminhos de escrita reporta quantas linhas gravou —, então é medido contando as linhas presentes depois da escrita. É o que revela a linha que passa pelo pré-filtro da chave natural e ainda perde para outra constraint única da tabela. Passa a registrar também quantas linhas vieram da origem. Para contatos esse é o único denominador existente: o /contacts do Enterprise devolve o tamanho da página no lugar do total, e por isso o importer nem reporta total. Um passo que leu linhas e não gravou nenhuma deixa de ser marcado como "empty" e passa a ser all_discarded — chamar isso de vazio era parte do que escondia a perda. O bloco de descartes é omitido quando não há nenhum, para que uma importação limpa continue limpa na tela.
5185b8e to
bb72d35
Compare
…er e corrige o all_discarded (BMS-18) O all_discarded era inalcançável sempre que a origem informava total: o branch de resume vinha primeiro e fechava o passo em done=totalKnown, marcando como importado por inteiro um passo que não gravou nenhuma linha. Ler linhas e não gravar nenhuma significa que todas foram descartadas, então essa decisão passa na frente. mapped_null contava dois casos com correção oposta: a linha que o customize recusou e a linha cuja FK não resolveu em scope=account. A primeira é decisão de qualidade de dado; a segunda quer dizer que um passo pai não importou o que a linha aponta. Viram mapper_rejected e fk_unresolved. Os tipos do msgops-api não tinham acompanhado seen e discarded. O progress é repassado verbatim do jsonb, então nada quebrava, mas o tipo que documenta o GET /imports/:jobId não mencionava o que ele devolve.
filipecrosk
left a comment
filipecrosk
left a comment
There was a problem hiding this comment.
Requesting changes before merge.
-
totalDonestill increases bycandidates.length, not actual inserted rows. When every candidate loses to an ignored unique conflict, the step reportsdonerather than terminalall_discarded, masking the loss this change is meant to expose. Base the terminal state and progress on actual inserts, with an all-conflict regression case. -
all_discardednow recordsseenbut nodone;cleanupOrphanAccount()only treatsdone > 0as progress. A later failure can therefore soft-delete an account whose job already recorded discarded source rows. Treat an existing/seen progress entry as progress, and add the cross-step failure regression.
totalDone += candidates.length somava linhas montadas, não as que chegaram ao banco. Quando todo o lote de candidatas perde para o orIgnore() por conflito de índice único, o passo fechava em done=N com zero linha escrita — a exata leitura falsa que o contador de descarte deveria expor. Passa a somar o que a página de fato tem no banco ao final da transação (pré-existentes + inseridas líquidas), com regressão de página inteira em conflito único caindo em all_discarded.
… órfão cleanupOrphanAccount() só tratava done>0 como progresso, mas all_discarded grava seen sem done — todo candidato foi lido e descartado, o que é progresso real sobre dado que existia, não um passo vazio. Uma falha num passo posterior soft-deletava a conta mesmo com linhas de origem já consumidas. Passa a aceitar seen>0 também como sinal de progresso.
filipecrosk
left a comment
filipecrosk
left a comment
There was a problem hiding this comment.
Both requested correctness fixes are complete. Progress now counts rows actually present after the write instead of candidates, the all-conflict case terminates as , and orphan cleanup treats as real progress.\n\nValidation at :\n- Focused enterprise-import suites: 2 suites / 26 tests passed\n- enterprise-import type-check passed\n- msgops-api type-check passed\n\nApproved. (The repository currently reports no GitHub checks.)
filipecrosk
left a comment
filipecrosk
left a comment
There was a problem hiding this comment.
Both requested correctness fixes are complete. Progress now counts rows actually present after the write instead of candidates, the all-conflict case terminates as all_discarded, and orphan cleanup treats seen > 0 as real progress.
Validation at a8d98ba8d031d19216eb2288ddfb4917b2c391a4:
- Focused enterprise-import suites: 2 suites / 26 tests passed
- enterprise-import type-check passed
- msgops-api type-check passed
Approved. (The repository currently reports no GitHub checks.)
3 participants
Summary
O progresso de um job de importação guardava
done,totalepage, e nada sobre o que foi descartado. Cada caminho de perda era umcontinuemudo ou um conflito engolido peloON CONFLICT, então uma importação que perdesse metade das linhas terminava com exatamente a mesma cara de uma que não perdeu nenhuma. Este PR dá número ao que se perde.A motivação é concreta. Comparando o que os jobs já rodados reportaram com o que existe no banco:
As 11 linhas da conta 2 sumiram sem deixar rastro, e
doneas contou como importadas — ele soma candidatas, não linhas gravadas. E essa é a única perda que hoje dá para enxergar: os outros três caminhos descartam a linha antes de ela virar candidata, então não aparecem em nenhum dos dois números.Changes
ImportProgressEntryganhaseen(linhas lidas da origem) ediscarded(contagem por motivo), com o tipoDiscardReasonenumerando os quatro caminhos.BaseImporterconta cada um deles:mapped_null(customize recusou ou FK não resolveu),empty_natural_key,duplicate_in_pageeinsert_conflict.insert_conflicté medido contando as linhas presentes depois da escrita. Nenhum dos dois caminhos de escrita reporta quantas linhas gravou —orIgnore()em account-scope eON CONFLICT DO NOTHINGem instance-scope —, e é justamente esse buraco que deixa passar a linha que clareia o pré-filtro da chave natural e ainda perde para outra constraint única da tabela.skipped: emptye passa aall_discarded. Chamar isso de vazio era parte do que escondia a perda.discardedé omitido quando não há descarte, para que uma importação limpa continue limpa na tela. Quando há, sai também umlogger.warn.Type of change
fix:)feat:)refactor:)docs:)chore:)Testing
Sete casos novos em
base.importer.spec.ts, um por caminho de descarte mais os dois de borda:scope=account— ambos contammapped_null;donecontar candidatas e não gravadas;discarded;all_discarded, nãoempty.O
FakeRepodo spec ganhoucount, que é o método novo que o importer passou a usar.pnpm type-checkpassespnpm lintpassespnpm testpasses —src/importers/, 32 testes em 3 suítesNão rodei uma importação real: exige uma instância Enterprise de origem, que não existe no ambiente local. Os números da tabela acima vêm dos jobs já gravados em
enterprise_import_jobs, não de uma execução minha.Screenshots
Não se aplica.
Notes for reviewers
Relação com o #19 — de leitura, não de código. Este PR sai direto de
maine pode ser mergeado sozinho, em qualquer ordem em relação ao #19: aquele PR vive emapps/msgops-api/src/modules/enterprise-importe no frontend, e não toca uma linha deapps/enterprise-import. Mas o que ele estabelece é o que dá sentido a este: a sessão de reconcile trata a unicidade de email como resultado de primeira classe. Um item que colidiria nasce com statusconflicte umfailureReasonlegível, e o resumo contaapplied,failed,conflict,pendingeskipped. É exatamente a disciplina que faltava do lado do worker, onde a mesma classe de colisão é engolida em silêncio. Este PR estende essa disciplina ao importador.Por que os contatos chegam com email mascarado, e o que isso implica na constraint. O
GET /contactsdo Enterprise devolve o email mascarado (lucas***@gmail.com) — é a razão de existir do módulo de reconcile. A tabelacontactstemUNIQUE (email, hashed_email, account_id), e ohashed_emailvem da origem calculado sobre o email real: o importer escreve por SQL cru e porupdateEntity(false), que não disparam o@BeforeInsertque recalcularia o hash. Confirmei no banco — ohashed_emailgravado não é o sha256 do email mascarado.A consequência é a que interessa: dois contatos com o mesmo email real chegam com a mesma máscara e o mesmo hash, colidem na constraint e o segundo é engolido pelo
orIgnore, contado como importado. Já dois contatos com emails reais diferentes que mascaram para a mesma string não colidem, porque o hash difere — na conta 2 há uma máscara com 203 linhas e 203 hashes distintos. Ou seja, a colisão é rara mas real, e é uma candidata direta para as 11 linhas. O contador é o que vai confirmar.O denominador continua faltando, e isso é deliberado neste PR. Para contatos o
/contactsdevolve o tamanho da página no lugar do total — por isso oContactsImporterdeclarareportsTotal = false. Oseenque este PR adiciona é o melhor denominador disponível hoje, mas ele conta o que a origem entregou, não o que a origem tem. Se a paginação por offset pular uma linha, ela não aparece nem emseennem emdiscarded. Fechar esse buraco depende de um total confiável na origem e não cabe aqui.