Valida metadata de todos tipos de evento e usa snake_case

[Rafatcb]

Mudanças realizadas

O PR #1638 usa snakeize no event.create(), e ao realizar um rebase, alguns testes do models/event quebraram por terem um metadata chamado updatedFields, e não updated_fields. Também percebi que alguns metadatas não são validados no validator, então:

1. 0fc32ef: passa a validar o metadata de eventos que não eram validados antes.
2. 9b3ca67: remove tipo de evento não usado: system:update:tabcoins.
3. 228f64d: uma sugestão para padronizarmos as propriedades do metadata na forma snake_case, onde a única que quebrava esse padrão era a updatedFields.

Tipo de mudança

• Correção de bug
• Refatoração

Checklist:

• As modificações não geram novos logs de erro ou aviso (warning).
• Eu adicionei testes que provam que a correção ou novo recurso funciona conforme esperado.
• Tanto os novos testes quanto os antigos estão passando localmente.

[vercel]

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name	Status	Preview	Comments	Updated (UTC)
tabnews	✅ Ready (Inspect)	Visit Preview	💬 Add feedback	Jul 20, 2024 9:07pm

[aprendendofelipe]

Ótimo que trouxe isso para ser analisado de forma isolada de todo o ruído do #1638 👍

Padronizar metadados em snake_case

Estou em dúvida se vale a pena mudar como armazenamos metadata. Se mudar isso, a gente deveria atualizar todos dados antigos no Postgres para corresponder ao novo padrão. Não é?

Se não for atualizar os dados antigos, devemos deixar o validador flexível e criar testes para que seja mantido assim, mas isso anularia algum possível ganho com a padronização.

Ou então deixamos os dados antigos em outro padrão e incompatíveis com o código atual, mas precisamos documentar de alguma forma que temos dados antigos em um padrão diferente.

Ou seja, se for adotar esse padrão, é mais fácil atualizar os dados antigos, mas vale a pena? Sei que fui eu que trouxe essa padronização lá no #1638 como uma possibilidade quando foi falado sobre a diferença de dados de entrada e saída, mas não sei se é a melhor opção. Até porque existem outras diferenças de validação de dados de input e output, então pode ser que esse trabalho seja em vão.

Se concluirmos que vale mesmo a pena mudar esse padrão, dá para evitar a sobrecarga do snakeize atualizando as chamadas de event.create.

Validar todos os metadados

Relacionado com a preocupação sobre mudar as chaves para snake_case, chegou a analisar o histórico de modificações desses metadados para descobrir se podemos ser rígidos nessas validações ou se antes é preciso adequar os dados antigos?

Por exemplo, está deixando updated_fields como required, mas como vão ficar os eventos criados antes da adoção desse campo?

Superadas as questões acima, e seguindo sua linha de raciocínio, acho que você gostaria de adicionar um otherwise: Joi.object().forbidden() (ou algo parecido) no último metadata para obrigar que qualquer novo type tenha um esquema apropriado.

system:update:tabcoins

É o evento que foi criado manualmente pelo Filipe, depois que os TabCoins foram criados, para executar a distribuição retroativa . Só existe um evento desse tipo, mas tem mais de 5000 linhas nas tabcoin_operations que são relacionadas com ele.

A metadata desse evento contém:

{"description": "Backfill all TabCoins from contents created before '2022-07-03 16:48:12.457+00'."}

[Rafatcb]

Se concluirmos que vale mesmo a pena mudar esse padrão, dá para evitar a sobrecarga do snakeize atualizando as chamadas de event.create.

Não vejo problema em não atualizarmos, tanto que no rebase que fiz na outra branch estava tratando conforme codigo abaixo para não alterar o metadata recebido:

async function create(object, options = {}) {
  const snakeizedWithOriginalMetadata = { ...snakeize(object), metadata: object.metadata };
  const cleanObject = validateObject(snakeizedWithOriginalMetadata);

  const query = {
    text: `INSERT INTO events (type, originator_user_id, originator_ip, metadata)
               VALUES($1, $2, $3, $4) RETURNING *;`,
    values: [cleanObject.type, cleanObject.originator_user_id, cleanObject.originator_ip, cleanObject.metadata],
  };

  const results = await database.query(query, options);
  return results.rows[0];
}

Foi apenas uma sugestão, mas como você mencionou, acho que não faz sentido o trabalho por causa dos dados antigos.

chegou a analisar o histórico de modificações desses metadados para descobrir se podemos ser rígidos nessas validações ou se antes é preciso adequar os dados antigos?

Eu havia pensado apenas nos dados que são criados e retornados, ou seja, tudo novo funcionaria normalmente, e os dados retornados eram apenas os do outro PR, por isso funcionaria. Agora ficou claro que "se quisermos mudar para retornar dados antigos" quebraria, mas o pior é que, se só temos dados no modo antigo em produção, será impossível perceber que quebrou algo ao modificar em ambiente de desenvolvimento, visto que os testes passariam mesmo que talvez não devessem.

Meu pensamento era "ou tudo deve ser validado, ou nada, não faz sentido validar metade das coisas".

É o evento que foi criado manualmente pelo Filipe, depois que os TabCoins foram criados, para executar a distribuição retroativa.

Ah, entendi. Não tinha visto referências no código, mesmo vendo pelo git blame, então pensei que havia sido criado e esqueceram de remover.

---

Bom, como estamos numa situação ruim onde não sabemos como são os dados da tabela, criar a validação agora seria um tiro no escuro, e remover as validações existentes seria dificultar ainda mais no futuro o entendimento de como os eventos estão no banco de dados.

Então, vou fechar o PR. A discussão serviu para confirmar o comportamento do event.create no PR #1764, obrigado 👍