Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Migração para criar novas tabelas de saldo #1656

Merged
merged 1 commit into from
Apr 10, 2024

Conversation

Rafatcb
Copy link
Collaborator

@Rafatcb Rafatcb commented Mar 22, 2024

Mudanças realizadas

Conforme discutido em #1491 (comment), separei o balance_operations em três tabelas distintas: uma para as operações de TabCoins de conteúdos, outra para as de TabCoins de usuários e outra para as de TabCash.

Não adicionei balance_type na tabela user_tabcoin_operations e user_tabcash_operations porque na balance_operations os valores eram user:tabcoin e user:tabcash, então seria bem redundante.

Esse processo será feito em alguns PRs diferentes:

  1. O primeiro PR é este, e contém a criação das tabelas e das funções.
  2. O segundo PR permitirá colocar os endpoints em manutenção de forma dinâmica (Permitir desabilitar rotas temporariamente pela variável de ambiente UNDER_MAINTENANCE #1662), conforme discutido em Migração para criar novas tabelas de saldo #1656 (comment) e nos comentários seguintes.
  3. O terceiro PR (Salvar operações de saldo em balance_operations e nas novas tabelas e reabilitar endpoints #1661) fará a leitura e escrita nas tabelas novas.
  4. Um quarto PR (Remove a tabela balance_operations #1673 ) fará uma migração para remover a tabela balance_operations e as funções que não são mais utilizadas.

Tipo de mudança

  • Refatoração

@Rafatcb Rafatcb added the back Envolve modificações no backend label Mar 22, 2024
Copy link

vercel bot commented Mar 22, 2024

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 Apr 8, 2024 11:50pm

Copy link
Collaborator

@aprendendofelipe aprendendofelipe left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Conforme discutido em #1491 (comment), separei o balance_operations em três tabelas distintas: uma para as operações de TabCoins de conteúdos, outra para as de TabCoins de usuários e outra para as de TabCash.

Show @Rafatcb, mas junto disso você trouxe outras refatorações, que são interessantes, mas que acho melhor fazer separadamente (e avaliando se realmente são necessárias).

Pelo que eu tinha sugerido, fora as adequações nas tabelas e funções do PostgreSQL, apenas os códigos relacionados com as queries precisariam de alterações. E o ponto principal é que todos os testes antigos deveriam passar sem nenhuma adequação.

Se estiver tudo certo, posso passar o primeiro commit (com a criação das funções e tabelas) para outro PR.

O primeiro PR acredito que será isso mesmo, mas será necessário pelo menos mais um PR intermediário que irá ler apenas das tabelas antigas, mas gravar de maneira duplicada para garantir a consistência quando o terceiro PR alterar a leitura para as tabelas novas. Nesse último já deve ser possível parar de duplicar os dados e apagar as tabelas antigas.

  • Não sei se podemos considerar breaking change a mudança do error_location_code de MODEL:BALANCE:RATE_CONTENT:NOT_ENOUGH para MODEL:CONTENT:RATE:NOT_ENOUGH, já que ninguém deveria depender disso (é uma propriedade instável).

Acho que nesse PR não deveria mexer nisso.

  • Eu não adicionei balance_type na tabela user_tabcoin_operations e user_tabcash_operations porque na balance_operations os valores eram user:tabcoin e user:tabcash, então seria bem redundante. Fiquei em dúvida se faria sentido usar os tipos credit e debit, como ocorre com os conteúdos.

Isso eu manteria como você fez mesmo 👍

  • Faz sentido criar uma migration para apagar a tabela balance_operations, get_content_balance_credit_debit e get_current_balance em um outro PR? Assim essa tabela não existirá desnecessariamente, e podemos garantir que a migration só seja executada depois de termos tudo funcionando corretamente.

Faz sentido sim! 👍

Eu optei por copiar o sequence para ficar mais fácil de identificar qual foi o último copiado, mas isso também poderia ser comparado pelo id, já que estou copiando o id de balance_operations:

Pela razão de existir do sequence, acho que não faz sentido copiar o valor dele para a nova tabela, a não ser que crie uma coluna temporária como sequence_old, mas que não me parece necessária se tomarmos os devidos cuidados.

O que precisamos é garantir que a tabela antiga tenha todas as entradas com os valores possíveis de sequence do primeiro ao último, sem pular nenhum, que as novas tabelas tenham a mesma sequência em sequence (não o valor em si, mas a mesma ordem da tabela original e sem pular valores) e que exista a correspondência correta entre as entradas da tabela antiga e as novas.

Então a migração dos dados vai precisar de etapas extras para garantir tudo isso.

Quando colocado em produção, pode ser que ocorra atualizações em balance_operations entre a última cópia e o merge do PR. Nessa situação, seria necessário usar a query para copiar novamente a partir de terminado sequence, mas sem copiar o valor do sequence para não conflitar com interações que ocorreram após o merge.

Então, é isso que precisa ser melhor pensado para garantir a mesma ordem em sequence.

models/ban.js Outdated
for (const eventBalanceOperation of eventBalanceOperations) {
await balance.undo(eventBalanceOperation.id, options);
}
await Promise.all(promises);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Em modo local podemos abrir mais de uma conexão com o PostgreSQL, mas em produção limitamos a uma conexão por lambda, então será preciso executar um por vez.

Mas, na verdade, acho que é melhor não refatorar isso nesse PR.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Eu havia mudado do findAll com WHERE originator_id seguido de um for com balance.undo para usar direto o undoAllByOriginatorId porque senão ficaria assim (em pseudo-código):

const contentBalanceOperations = await getContentBalanceOperationsFromEvent();
const userTabcoinsOperations = await getUserTabcoinsOperationsFromEvent();
const userTabcashOperations = await getUserTabcashOperationsFromEvent();

for (const eventContentBalanceOperations of contentBalanceOperations) {
   await contentTabcoin.undo();
}

for (const eventTabcoinOperations of userTabcoinOperations) {
   await userTabcoin.undo();
}

for (const userTabcashOperations of userTabcashOperations) {
   await userTabcash.undo();
}

Ou seja, três SELECTs, um para cada tabela, seguido de três laços iterando sobre cada retorno dos SELECTs.

Se preferir que o PR 3 (leitura das tabelas novas) fique com o código mencionado acima, e que abra um quarto PR para a refatoração, tudo bem.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Por que o PR 3 não poderia permanecer com o código original aqui?

      for (const eventBalanceOperation of eventBalanceOperations) {
        await balance.undo(eventBalanceOperation.id, options);
      }

Minha sugestão original (criar novas tabelas no banco) não envolvia a criação de novos models. Isso é mesmo necessário?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Não é necessário criar novos models se o models/balance lidar com o "malabarismo" de usar a tabela adequada dependendo dos argumentos recebidos na função em questão. Seria um model conhecendo a estrutura de três tabelas diferentes, similar à função rate criada em models/content, porém para todas funções que forem necessárias (create, getTotal, undo e findOne, que é usada apenas pelo próprio undo).

Pensei que fizesse mais sentido separar a responsabilidade para models diferentes.

Copy link
Collaborator

@aprendendofelipe aprendendofelipe Mar 27, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pensei que fizesse mais sentido separar a responsabilidade para models diferentes.

Sim Rafa, deve fazer sentido separar os models, mas não necessariamente por estamos separando as tabelas. Apenas acredito ser melhor separar(remover desse PR) o que não é necessário ao processo de separação das tabelas

@Rafatcb Rafatcb added the pendente Aguardando ação do autor do Pull Request label Mar 27, 2024
@Rafatcb
Copy link
Collaborator Author

Rafatcb commented Mar 30, 2024

O que precisamos é garantir que a tabela antiga tenha todas as entradas com os valores possíveis de sequence do primeiro ao último, sem pular nenhum, que as novas tabelas tenham a mesma sequência em sequence (não o valor em si, mas a mesma ordem da tabela original e sem pular valores) e que exista a correspondência correta entre as entradas da tabela antiga e as novas.

Que tipo de migração de dados você teve em mente que permitiria realizar as inserções nas tabelas novas antes do PR 2 entrar em produção?

A sugestão que eu dei inicialmente no PR teria o mesmo problema, mas eu estava considerando não deixar ordenado os últimos possíveis casos: saldos X que ocorreram após a migração de dados e antes do merge do PR, sendo que após o merge pode ter surgido novos saldos Y nas tabelas novas, então apesar dos X serem mais antigos que os Y, ficariam com um sequence maior.

Pensei em duas opções para evitar/corrigir isso:

  1. Deixar o site em "modo leitura" (funcionalidade que ainda não temos) enquanto é realizado o merge do PR 2 para garantir que nada novo será inserido até que tudo antigo seja transferido.
  2. Seguir o processo cogitado anteriormente, mas corrigir o sequence dos saldos X e Y das tabelas novas com base no created_at. Seriam poucas linhas modificadas, e precisaria realizar o processo nas três tabelas. A query poderia ser algo como:
WITH ordered_rows AS (
    SELECT id, ROW_NUMBER() OVER (ORDER BY created_at) + 999999 AS new_sequence
    FROM content_tabcoin_operations
    WHERE sequence >= 1000000
)
UPDATE content_tabcoin_operations
SET sequence = ordered_rows.new_sequence
FROM ordered_rows
WHERE content_tabcoin_operations.id = ordered_rows.id;

O exemplo acima considera que os saldos possivelmente desordenados são os saldos a partir do sequence = 1000000 (vaolr de exemplo), para não precisar atualizar o sequence dos saldos que sabemos que estão corretos (ou seja, os saldos criados antes do merge do PR 2).

@aprendendofelipe
Copy link
Collaborator

O que precisamos é garantir que a tabela antiga tenha todas as entradas com os valores possíveis de sequence do primeiro ao último, sem pular nenhum, que as novas tabelas tenham a mesma sequência em sequence (não o valor em si, mas a mesma ordem da tabela original e sem pular valores) e que exista a correspondência correta entre as entradas da tabela antiga e as novas.

Que tipo de migração de dados você teve em mente que permitiria realizar as inserções nas tabelas novas antes do PR 2 entrar em produção?

Para ativar provisoriamente o salvamento dos dados de forma duplicada na tabela antiga e nas novas, a duplicação só começa após o PR 2, pois as migrations não rodam automaticamente no merge do PR 1, então não dá para começar a duplicar antes de criar e popular as tabelas. Então teria no mínimo 3 PRs, talvez 4.

Pensei em duas opções para evitar/corrigir isso:

  1. Deixar o site em "modo leitura" (funcionalidade que ainda não temos) enquanto é realizado o merge do PR 2 para garantir que nada novo será inserido até que tudo antigo seja transferido.

Deixar o site em "modo leitura" é uma ótima opção para garantir o sucesso de primeira e com menos trabalho. A desvantagem de deixar o sistema parcialmente inoperante pode ser mitigada ao realizarmos o procedimento em um momento de menor trafego, como nas madrugadas.

Poderia ser em algo assim:

  1. O PR 1 cria as migrations (novas tabelas e funções do PostgreSQL) e coloca o site em modo leitura pelo middleware (no passado isso foi feito direcionando para a resposta de rate-limit). Não pode bloquear totalmente a API, pois as migrations precisam rodar, então deve ser suficiente bloquear apenas POST para endpoint que comece com /api/v1/contents.
  2. O PR 2 já deve estar pronto para ser rapidamente mesclado logo após fazermos as verificações se tudo correu bem ao rodar as migrations do PR 1. Esse PR 2 volta a API ao modo normal e passa a usar as novas tabelas.
  3. Um terceiro PR cria as migrations para excluir a tabela antiga após termos certeza de tudo ter corrido como planejado. Esse terceiro é sem urgência.

@Rafatcb Rafatcb added the refatoração Melhoria no código que não modifica o comportamento externo label Mar 30, 2024
@Rafatcb Rafatcb force-pushed the refactor-table-balance-operations branch from be9132c to d97e826 Compare March 30, 2024 22:13
@Rafatcb Rafatcb marked this pull request as draft March 30, 2024 22:13
@Rafatcb
Copy link
Collaborator Author

Rafatcb commented Mar 30, 2024

Estou deixando este como o "PR 1", ainda vou atualizar a descrição do PR e criar a segunda branch (a partir desta, para já remover o modo de manutenção) e abrir o PR.

  1. Subi as alterações com o modo leitura, tratando também o endpoint GET /api/v1/user por causa do reward, veja se faz sentido.
  2. Criei uma resposta específica para o "modo de manutenção", assim enviamos uma mensagem adequada para o usuário.
  3. Mudei o nome do arquivo da migração porque imagino que vamos realizar o merge do Reverter ganhos de TabCoins em conteúdos pegos pelo firewall interno e outros ajustes #1638 primeiro, que também tem uma migração e não é tão sensível quanto este.

Estou deixando este PR como draft para não realizarmos o merge sem querer.

Para as queries de migração de dados será necessário adaptar as queries que mencionei na descrição deste issue apenas removendo o sequence, para gerar automaticamente, consequentemente não precisando da query para atualizar o SERIAL, e também remover o CASE do balance_type do content_tabcoin_operations, já que não irá mudar o tipo, certo? Ficando assim:

INSERT INTO content_tabcoin_operations(id, balance_type, content_id, amount, originator_type, originator_id, created_at)
SELECT
  id,
  balance_type,
  recipient_id AS content_id,
  amount,
  originator_type,
  originator_id,
  created_at
FROM balance_operations
WHERE balance_type IN ('content:tabcoin:credit', 'content:tabcoin:debit', 'content:tabcoin:initial')
ORDER BY sequence;
INSERT INTO user_tabcash_operations(id, user_id, amount, originator_type, originator_id, created_at)
SELECT
  id,
  recipient_id AS user_id,
  amount,
  originator_type,
  originator_id,
  created_at
FROM balance_operations
WHERE balance_type = 'user:tabcash'
ORDER BY sequence;
INSERT INTO user_tabcoin_operations(id, user_id, amount, originator_type, originator_id, created_at)
SELECT
  id,
  recipient_id AS user_id,
  amount,
  originator_type,
  originator_id,
  created_at
FROM balance_operations
WHERE balance_type = 'user:tabcoin'
ORDER BY sequence;

Parece que não será possível realizar o merge com os testes quebrando. Não sei se é possível tirar temporariamente o required dessa verificação ou se você recomenda alguma outra abordagem.

Mensagem "Required statuses must pass before merging"

@Rafatcb Rafatcb removed the pendente Aguardando ação do autor do Pull Request label Mar 30, 2024
@Rafatcb Rafatcb changed the title Salvar operações de saldo em diferentes tabelas no banco de dados Criar novas tabelas de saldo e desabilitar alguns endpoints temporariamente Mar 30, 2024
@Rafatcb Rafatcb marked this pull request as ready for review March 30, 2024 23:46
@Rafatcb Rafatcb marked this pull request as draft March 30, 2024 23:47
@aprendendofelipe
Copy link
Collaborator

  1. Criei uma resposta específica para o "modo de manutenção", assim enviamos uma mensagem adequada para o usuário.

Acho que não faz sentido gerar logs de ServiceError, pois os alarmes falsos vão atrapalhar no momento da migração.

Não é necessário criar um endpoint e, principalmente, usar as lambdas para criar as respostas. É melhor responder diretamente pela Edge.

Parece que não será possível realizar o merge com os testes quebrando. Não sei se é possível tirar temporariamente o required dessa verificação ou se você recomenda alguma outra abordagem.

Abraçando a ótima ideia de manter fácil a possibilidade de desabilitar endpoints, acho que implementação e discussão merecem um PR separado, mesmo que não seja algo complexo, vão ficar dois assuntos diferentes misturados aqui, e complicar para quem quiser revisitar qualquer dos dois assuntos no futuro (modo leitura vs desmembramento da balance_operations).

Mas já respondendo... Para algo mais flexível, que possa ser usado rapidamente em caso de emergência, e que não quebre os testes, penso que é melhor usar as variáveis de ambiente para escolher os endpoints que ficarão indisponíveis, no estilo do que é feito para o rate-limit. Talvez uma variável de ambiente UNDER_MAINTENANCE={"methodsAndPaths":["POST /api/v1/contents.*","GET /api/v1/user"]}.

Daí pode existir algo como infra/maintenance.js que é chamado pelo middleware e, se der match, já devolve a resposta de manutenção diretamente pela Edge. Dependendo do motivo de estarmos em manutenção no futuro, será ainda mais importante responder sem ocupar as lambdas.

@aprendendofelipe
Copy link
Collaborator

e também remover o CASE do balance_type do content_tabcoin_operations, já que não irá mudar o tipo, certo?

Acho que, com as tabelas separadas, faz sentido mudar o tipo, exatamente como você tinha feito antes 👍

@aprendendofelipe
Copy link
Collaborator

Estou deixando este como o "PR 1"

Como sugestão para os próximos casos, quando for quebrando o PR em problemas menores, é melhor colocar essas partes menores nos novos PRs, e manter o grosso no PR original. Não tem problema criar PRs mais novos, mas que precisem ser mesclados antes dos já existentes. 👍

Assim mantemos a documentação mais próxima do código. Por exemplo, o grosso do código que foi discutido aqui acabou indo para outro PR (é verdade que, nesse caso, parte do código foi para lugar nenhum 😅) e a discussão em cima do código vai acabar se perdendo. Resolveria isso, além de ser mais simples, ter criado um PR novo apenas com a migration (e, como sugeri hoje, outro com o "modo leitura").

Uma outra enorme vantagem de manter o grosso no mesmo PR é facilitar a revisão, pois permite usar melhor as ferramentas do GitHub, como o histórico dos arquivos que já foram marcados como revisados (algo privado para cada revisor). Isso é muito útil, pois não é preciso analisar novamente os arquivos que não foram modificados a cada novo commit. Com um PR novo, todos os arquivos precisam ser revisados novamente.

Não tem problema manter assim, não estou sugerindo mudar o que já planejou, e esse caso não ficou complexo, pois, no final, acabamos ficando apenas com PRs pequenos. Só achei uma boa oportunidade de expor esse meu pensamento, e também de descobrir se ele faz sentido para as outras pessoas. 🤝

@Rafatcb
Copy link
Collaborator Author

Rafatcb commented Apr 2, 2024

Acho que não faz sentido gerar logs de ServiceError, pois os alarmes falsos vão atrapalhar no momento da migração.

Certo 👍

É melhor responder diretamente pela Edge.

Você consegue clarear para mim como isso funciona? Não tenho certeza se é uma alteração no código (e em qual parte) ou fora dele que fará isso.

Edit: Eu dei uma lida e parece que para usar uma função Edge basta colocar um export const config = { runtime: 'edge' }; no arquivo, mas ainda fiquei em dúvida onde esse arquivo seria colocado.

penso que é melhor usar as variáveis de ambiente para escolher os endpoints que ficarão indisponíveis, no estilo do que é feito para o rate-limit. Talvez uma variável de ambiente UNDER_MAINTENANCE={"methodsAndPaths":["POST /api/v1/contents.*","GET /api/v1/user"]}

Gostei da ideia. Realmente isso aumenta a complexidade (principalmente a parte de permitir wildcards * ), mas é bem útil. Acha que faz sentido levar essa informação para o #1181 ou deveria ser um novo issue?

Nesse PR poderíamos usar algo mais simples, como BALANCE_OPERATIONS_MAINTENANCE=true. O que acha?

Edit: Como mencionado no issue #1181, faz sentido usar Edge Config ao invés de variável de ambiente? Já que pretendemos usar Edge para esse redirecionamento. Provavelmente algo assim:

import { NextResponse } from 'next/server';
import { get } from '@vercel/edge-config';
 
export const runtime = 'edge';
 
// Não sei se dá para ter um outro middleware
export async function middleware(request) {
  const url = request.nextUrl;

  const isPostContents = url.pathname.startsWith('/api/v1/contents') && request.method === 'POST';
  const isReward = url.pathname === '/api/v1/user' && request.method === 'GET';

  if (isPostContents || isReward) {
    const balanceMaintenanceConfig = await get('balance_operations_maintenance');
    if (balanceMaintenanceConfig === 'true') {
      const error = new ServiceError({
        message: 'O TabNews está em manutenção.',
        action: 'Não é possível realizar esta ação agora, tente novamente mais tarde.',
      });

      return NextResponse.json(snakeize(error), { status: error.statusCode });
    }
  }
  return NextResponse.next();
}

e também remover o CASE do balance_type do content_tabcoin_operations, já que não irá mudar o tipo, certo?

Acho que, com as tabelas separadas, faz sentido mudar o tipo, exatamente como você tinha feito antes 👍

Ok 👍

Só achei uma boa oportunidade de expor esse meu pensamento, e também de descobrir se ele faz sentido para as outras pessoas. 🤝

Eu havia pensado em fazer como você mencionou, mas mudei os planos por dois motivos, onde o primeiro leva ao segundo:

  1. O primeiro PR mesclado contém a migração, até aí tudo bem. O segundo PR contém as escritas na nova tabela e leituras na antiga. Então, pensei que já precisaríamos ter feito a cópia dos dados antes do PR 2, logo imaginei que seria necessário colocar o "modo manutenção" no PR 1 (ou então ficariam 3 PRs, onde o segundo seria apenas o modo manutenção).
  2. Considerando a importância do PR que remove o modo manutenção poder ser mesclado rapidamente, me pareceu ser necessário criar um branch para ele a partir do branch que colocava o modo manutenção, o que permitiria já remover o modo manutenção sem modificar o código após o primeiro PR ser mesclado, apenas precisaria realizar o rebase.

Mas, ao usar uma variável de ambiente para definir esse comportamento, perde-se essa necessidade da segunda branch se basear na primeira para já remover o código de manutenção (falo especificamente sobre o commit 450ce14 do PR #1661).

Vendo a análise que você realizou no PR #1661, me pergunto se você acha mais adequado:

  1. Deixar este PR Migração para criar novas tabelas de saldo #1656 apenas com a migração, e colocar a verificação da variável de ambiente na Edge no PR Salvar operações de saldo em balance_operations e nas novas tabelas e reabilitar endpoints #1661; ou
  2. Criar um terceiro PR, passando a migração para lá e deixando a variável de ambiente na Edge neste.

Como há bastante coisa para arrumar, pode me falar o jeito que prefere que eu arrumo os PRs para ficarem de acordo. Posso passar de volta para cá a parte relevante da implementação, por causa da discussão que tivemos.

@aprendendofelipe
Copy link
Collaborator

É melhor responder diretamente pela Edge.

Você consegue clarear para mim como isso funciona? Não tenho certeza se é uma alteração no código (e em qual parte) ou fora dele que fará isso.

Edit: Eu dei uma lida e parece que para usar uma função Edge basta colocar um export const config = { runtime: 'edge' }; no arquivo, mas ainda fiquei em dúvida onde esse arquivo seria colocado.

O middleware é executado na Edge, assim como também podemos configurar as rotas da API para serem executadas na Edge. Essa configuração que você viu seria para mudar o runtime de uma rota da API, mas a minha sugestão é não criar uma nova rota, e sim responder diretamente pelo middleware (producing-a-response). Já fazemos isso no trecho abaixo:

if (url.pathname === '/api/v1/swr') {
return new NextResponse(JSON.stringify({ timestamp: Date.now() }), {
status: 200,
headers: { 'Content-Type': 'application/json' },
});
}

penso que é melhor usar as variáveis de ambiente para escolher os endpoints que ficarão indisponíveis, no estilo do que é feito para o rate-limit. Talvez uma variável de ambiente UNDER_MAINTENANCE={"methodsAndPaths":["POST /api/v1/contents.*","GET /api/v1/user"]}

Gostei da ideia. Realmente isso aumenta a complexidade (principalmente a parte de permitir wildcards * ), mas é bem útil.

Nem é tão complexo. Segue um exemplo (não testado):

const isUnderMaintenance = methodsAndPathsUnderMaintenance.some((methodAndPath) =>
  new RegExp(methodAndPath).test(`${method} ${path}`),
);

Acha que faz sentido levar essa informação para o #1181 ou deveria ser um novo issue?

Seria outra issue, mas talvez seja mais simples já criar o PR do que detalhar o que é para ser feito em uma issue. 😅 Se quiser, eu abro o PR. 👍

Nesse PR poderíamos usar algo mais simples, como BALANCE_OPERATIONS_MAINTENANCE=true. O que acha?

Daí teria que desfazer tudo no PR 2. Acho que dá menos trabalho, e é mais útil, já abrir um novo PR com a versão que irá permanecer no código.

Edit: Como mencionado no issue #1181, faz sentido usar Edge Config ao invés de variável de ambiente? Já que pretendemos usar Edge para esse redirecionamento.

Acho que não, mesmo que em alguns momentos possamos usar as duas funcionalidades juntas, acho que, desativar um endpoint da API, e mostrar uma mensagem nas páginas, são coisas diferentes. E usar a Edge Config no middleware, ou seja, em praticamente todas as requisições, nos traria um custo "desnecessário" de uns $15 por mês (pelo tráfego atual, se isso não entrar nos custos cobertos pela parceria com a Vercel).

Provavelmente algo assim:
...
// Não sei se dá para ter um outro middleware

Não dá. E talvez você esteja confundindo as duas possibilidades de rodar código na Edge (API e Middleware). São coisas diferentes. Seu código de exemplo seria para criar uma rota da API que, diferente do padrão, seria executada na Edge. Não é um código que faz sentido para o middleware.

Como há bastante coisa para arrumar, pode me falar o jeito que prefere que eu arrumo os PRs para ficarem de acordo. Posso passar de volta para cá a parte relevante da implementação, por causa da discussão que tivemos.

  1. Acho que merece um novo PR para adicionar a possibilidade de desativar um endpoint da API (que eu posso criar). E, pela discussão, não faz sentido isso ser a única coisa que sobrar nesse PR, mesmo que seja algo como BALANCE_OPERATIONS_MAINTENANCE=true. Então eu criaria outro.

  2. Depois virá o PR que cria as novas tabelas.

  3. Em seguida, o PR que faz o restante (menos excluir a tabela balance_operations).

@Rafatcb
Copy link
Collaborator Author

Rafatcb commented Apr 2, 2024

Se quiser, eu abro o PR. 👍

Acredito que eu tenha entendido, mas pode abrir, senão provavelmente teríamos mais algumas rodadas de revisão e alterações.

Quer que eu inverta o conteúdo dos PRs (#1656 e #1661) para deixar a leitura/inserção das tabelas novas aqui e a migração no outro?

@aprendendofelipe
Copy link
Collaborator

Se quiser, eu abro o PR. 👍

Acredito que eu tenha entendido, mas pode abrir, senão provavelmente teríamos mais algumas rodadas de revisão e alterações.

Pode deixar comigo 👍

Quer que eu inverta o conteúdo dos PRs (#1656 e #1661) para deixar a leitura/inserção das tabelas novas aqui e a migração no outro?

Não precisa inverter 👍

@Rafatcb Rafatcb changed the title Criar novas tabelas de saldo e desabilitar alguns endpoints temporariamente Migração para criar novas tabelas de saldo Apr 3, 2024
@Rafatcb Rafatcb force-pushed the refactor-table-balance-operations branch from d97e826 to cc2659d Compare April 3, 2024 00:56
@Rafatcb Rafatcb force-pushed the refactor-table-balance-operations branch from cc2659d to be33b98 Compare April 4, 2024 01:26
@Rafatcb Rafatcb force-pushed the refactor-table-balance-operations branch from be33b98 to 8df126f Compare April 5, 2024 01:37
@Rafatcb
Copy link
Collaborator Author

Rafatcb commented Apr 6, 2024

Conforme dito nessa sequência de comentários: #1661 (comment), estou fechando este PR porque a migração será realizada junto das alterações no código, já que uma das alterações modifica uma função SQL existente, que faz os testes quebrarem caso o código não seja atualizado.

@Rafatcb Rafatcb closed this Apr 6, 2024
@aprendendofelipe
Copy link
Collaborator

Conforme dito nessa sequência de comentários: #1661 (comment), estou fechando este PR porque a migração será realizada junto das alterações no código,

Ainda é necessário avaliar melhor se realmente não será preciso criar as novas tabelas em um PR separado.

Talvez dê problemas no build se houverem conteúdos na lista de relevantes. A lista de relevantes estava vazia em homologação, mas já criei uma publicação para tirar essa dúvida no próximo build.

Também tem o fato de que outras funcionalidades ficariam com erro até que fosse(m) executada(s) a(s) migration(s). Não apenas os endpoints que travarmos propositalmente, mas outras coisas que não necessariamente precisariam ficar fora. E isso pode disparar alertas indesejados no momento que queremos estar focados em realizar as cópias e testes das novas tabelas.

Por tudo isso, já deixei a variável de ambiente configurada para "modo leitura" também nessa branch, caso ela ainda seja utilizada.

já que uma das alterações modifica uma função SQL existente, que faz os testes quebrarem caso o código não seja atualizado.

Isso não me parece um problema para o processo de PRs separados. Não acha suficiente deixar a atualização da get_content_balance_credit_debit em uma nova migration no PR seguinte? Eu tinha comentado lá sobre adiar a criação das novas funções, mas não podemos. É só a atualização dessa única função que precisa ficar para o outro PR.

@Rafatcb
Copy link
Collaborator Author

Rafatcb commented Apr 7, 2024

Não acha suficiente deixar a atualização da get_content_balance_credit_debit em uma nova migration no PR seguinte? (...) É só a atualização dessa única função que precisa ficar para o outro PR.

Quando comecei a ler seu comentário pensei nisso como alternativa. Vou fazer isso.

@aprendendofelipe
Copy link
Collaborator

A partir de agora, todos os próximos deploys em homologação já vão estar travados com:

{"methodsAndPaths":["POST /api/v1/contents","GET /api/v1/user$"]}

Acho que já podemos dar esse PR como pronto, mas é melhor aguardar o #1661 também estar pronto (talvez já esteja), pois assim não deixamos o ambiente de homologação travado por muito tempo.

Antes de rodar a migration desse PR, vou tirar do ar todos deploys que ainda existem em homologação, e somente poderemos voltar ao normal após a cópia dos dados para as novas tabelas.

Mesmo após a cópia, é bom garantir que ninguém mais consiga fazer um deploy que consiga gravar na tabela balance_operations. Então vou liberar apenas a branch do PR #1161, e as demais poderão ser liberadas apenas após a exclusão dessa tabela (ou o travamento dos dados dela de outra maneira).

@Rafatcb
Copy link
Collaborator Author

Rafatcb commented Apr 8, 2024

as demais poderão ser liberadas apenas após a exclusão dessa tabela (ou o travamento dos dados dela de outra maneira).

Apenas após a exclusão do balance_operations (uma migration que será feita num novo PR) ou quando derem um rebase para utilizar as tabelas novas (após o merge dos PRs #1656 e #1661)?

@aprendendofelipe
Copy link
Collaborator

Apenas após a exclusão do balance_operations (uma migration que será feita num novo PR) ou quando derem um rebase para utilizar as tabelas novas (após o merge dos PRs #1656 e #1661)?

rebase não impede que voltem a gravar na balance_operations com alguma reversão no código ou outras modificações. Na verdade, nada baseado apenas no código impede. Então, apenas como uma precaução contra algum equívoco de um contribuidor, é melhor liberar as rotas apenas após termos certeza de poder descartar esses dados ,não necessariamente após apagar, mas quando a tabela antiga não tiver mais importância para a nossa validação de que tudo correu bem como esperado.

Create three tables to separately store content TabCoins, user TabCoins and user TabCash operations.
@Rafatcb Rafatcb force-pushed the refactor-table-balance-operations branch from b0237bc to 60d75e3 Compare April 8, 2024 23:46
@aprendendofelipe aprendendofelipe marked this pull request as ready for review April 9, 2024 22:02
@aprendendofelipe aprendendofelipe merged commit 199c0e3 into main Apr 10, 2024
7 checks passed
@aprendendofelipe aprendendofelipe deleted the refactor-table-balance-operations branch April 10, 2024 06:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
back Envolve modificações no backend refatoração Melhoria no código que não modifica o comportamento externo
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants