Refatorações e ajustes finais da Milestone 2 - #45
Merged
Conversation
…cation.getUser()` Escolhi fazer esta refatoração, pois o método `getAuthenticatedUser()`estava muito longo. Então dentro do contexto do model `authentication`,achei que continuaria legível com somente `getUser()` e mantendo aassinatura com o objeto pedindo por `email` e `password`, e guardando oresultado na variável `authenticatedUser` para se entender que é um"usuário autenticado" que está sendo buscado, por exemplo:```js const authenticatedUser = await authentication.getUser( userInputValues.email, userInputValues.password, );
Faltou um `return` no final do handler do endpoint de status que não causava nenhum bug, mas melhor adicionar por padronização e também para evitar bugs futuros. Esta sugestão foi apontada pelo aluno `lbpereira` 🤝
Padroniza o uso do caractere ` para envolver nomes de campos ou outros termos técnicos nas descrições dos testes.
…gin}`
Após a introdução do `webserver` com o `origin` do serviço web, vários
testes continuaram apontando de forma fixa para `http://localhost:3000`.
Este commit padroniza o uso do `${webserver.origin}` e agora o servidor
local dos testes pode ser apontado para outro lugar apenas alterando uma
única linha dentro do `webserver`.
Estes dois módulos estavam listados como dependência para rodar o serviço em produção, porém eles estão sendo usados exclusivamente pelo `jest.config.js` e não há nenhum plano de usar esses módulos em outros ambientes por enquanto.
…ookie()` Estes dois métodos do componente `controller.js` estavam declarados como funções assíncronas de forma indevida, pois operam apenas de forma síncrona.
Este commit conserta o formato da data que utilizei em um dos testes unitários do model `authorization`. Não teve nenhum impacto, pois o objeto de `resource` estava apenas em memória e era usado diretamente no filtro que hoje não faz validação do formato das coisas, mas no irá fazer.
…` test descriptions O termo "Running" casa melhor com a ação que um `POST` contra o endpoint realmente faz que é de "Rodar" as migrações. Imagino que na hora de duplicar o arquivo do teste, acabei esquecendo de atualizar a descrição dos testes. Este apontamento foi feito pelo aluno `IIsrael` 🤝
…s/[username]` Depois de aplicar o filtro de saída que agora faz não devolver mais a propriedade `email` do `user`, eu simplesmente removi a expectativa deste valor ser retornado na resposta da API de todos os testes. E apesar de que isto continua válido, no teste de atualização de email ficou faltando a cobertura se a propriedade `email` foi atualizada de forma persistende no banco de dados. Então este commit busca o usuário no banco e verifica se de fato a informação foi alterada como deveria. Isto foi apontado pelo aluno `IIsrael` 🤝
Antes estava sendo criado o objeto de router do `next-connect` e contra este objeto é que estava sendo "anexado" os métodos disponíveis para um certo endpoint na API (incluindo o `handler`) onde, depois disso, esse objeto de router era exportado como `default` do arquivo do controller. Mas agora com a refatoração deste commit, todo o export e encadeamento de métodos e handler acontece de uma só vez, pois fica mais limpo e direto de se ler o código. Isto é um tipo de refatoração que é muito bom de se fazer quando você tem uma cobertua boa de testes `E2E` 🙏
Aqui eu faço o método `orchestrator.createSession()` aceitar um objeto de `user` por inteiro (ao invés só do `id`) para ficar com a mesma interface pública de `orchestrator.activateUser()`.
Os testes de `GET` do endpoint de status utiliza um usuário privilegiado e era um teste que estava passando mesmo sem ter as migrations executadas. Isto estava funcionando, pois numa bateria completa de `npm test`, algum outro teste já teria feito o trabalho de criar as tabelas, sendo que quando o teste era executado de forma isolada, naturalmente quebrava por não ter a tabela `user` (necessária para criar um usuário privilegiado). Então foi só questão de padronizar o hook principal para limpar o banco de dados e rodar as migrations igual ao restante dos outros testes. Isto foi apontado pelo aluno `gleniosp` 🤝
O teste do `GET /api/v1/status` testava o retorno do endpoint com um usuário anônimo e com um usuário privilegiado, mas não testava com um usuário padrão, onde neste caso, não deveria vir a propriedade `version` sobre o banco de dados. Esse teste então adiciona esta cobertura.
Esta modificação tem impacto real apenas para navegadores mais antigos, antes de 2020 aproximadamente, porque os navegadores modernos já adotam `SameSite=Lax` por padrão. O `SameSite=Lax` faz o navegador ser mais criterioso na hora de enviar o cookie de sessão (ou qualquer outro cookie), principalmente quando a navegação ou a requisição foi iniciada a partir de outro site, por exemplo, algum hacker tentando interferir em um site que você esteja logado através de um outro site que ele tenha algum controle. Então imagine o cenário: 1. Usuário está logado no FinTab usando um navegador antigo. 2. FinTab não definiu a propriedade `SameSite=Lax`. 3. Usuário acessa outro site externo em que hacker possui algum controle, seja direto ou indireto (injeção de script por XSS). 3. Este site externo pode ter um comportamento malicioso de disparar uma requisição `POST` contra um endpoint do FinTab que necessite de autenticação, por exemplo, adicionar um comentário com um conteúdo de marketing ou link para um vírus em uma publicação qualquer. 4. Neste cenário, o navegador irá reaproveitar a sessão ativa e outros cookies do cookie jar do endereço do FinTab para executar esta ação, efetivamente adicionando o comentário. 5. Agora, caso o cookie de sessão (ou qualquer outro cookie) esteja marcado com `SameSite=Lax`, o navegador não irá enviar ele no cabeçalho caso a origem da requisição tenha vindo de um outro endereço, efetivamente bloqueando a tentativa do hacker de criar uma resposta através da sua conta usando a sua sessão. De qualquer forma, mesmo o `SameSite=Lax` sendo o padrão em navegadores modernos, ainda assim faz sentido declarar ele no código de forma explícita porque deixa a intenção de segurança documentada, evita depender de comportamento implícito do navegador e abre as portas para decidirmos mais para frente adotar o valor `Strict` e entender as diferenças. Fora tudo isso, o impacto da mudança é mínimo e o ganho de clareza e previsibilidade compensa. Isto foi apontado pelo aluno `arthurmousinho` 🤝
Nos testes sobre criar uma sessão válida, a propriedade `expires_at` do objeto de sessão é calculada na camada da aplicação, antes da persistência. Já a propriedade `created_at` é calculada depois, lá na camada do banco de dados, o que faz uma sessão não ter exatamente 30 dias de expiração em milissegundos que seriam `2592000000` e ficando então com valores muito próximos como `2591999991`, por exemplo, que são 30 dias menos 9 milissegundos. Os testes atuais já tentavam compensar esta diferença ao zerar os segundos das datas envolvidas, mas é uma alternativa que possui um furo dependendo de algumas condições como o virar do minuto. Tentei pegar este comportamento para mostrar em uma aula, mas não consegui e isto estava me agoniando ao pensar que algum aluno poderia ver o seu CI quebrando e não entender o motivo. Então para já evitar esta situação daqui para frente e depois de sugestões de vários alunos, optei por adicionar uma margem de erro de 5 segundos entre o que é esperado (30 dias exatos de expiração) e o que o objeto de sessão de fato possui de tempo de expiração.
Essa foi uma atualização bem simples de fazer. Os passos foram: 1. Primeiro verifiquei se eu estava com a versão certa do Node.js ao rodar `node -v` e eu estava com a `v24.13.0`. 2. Rodei `npx npm-check-updates --format group`, que agrupa o resultado por `patch`, `minor e `major`, e me foquei apenas no `patch` e `minor`. 3. Não utilizei a flag `-i` que habilita o modo interativo do módulo, porque eu queria ir manualmente instalando as versões e testando. 4. Então num comando só eu instalei todas as novas versões dos módulos que entraram no grupo `patch` e ficou assim (sugiro instalar as mesmas versões aí do seu lado para ficarmos sincronizados): ``` npm i -E bcryptjs@3.0.3 commitizen@4.3.1 husky@9.1.7 ``` Depois de instalado, rodei `npm test` para ser se tudo estava ok, tudo estava e, novamente, como é bom ter testes E2E 🙏 Como esse estado da aplicação estava ok, eu já fiz o commit dele com a mensagem que está nesse commit mesmo: ``` chore: bump project dependencies within `minor` and `patch` releases ``` A idéia é a cada passo com sucesso, eu ir fazendo `git commit --amend` contra esse mesmo commit, até ter todas as versões `minor` e `patch` atualizadas dentro desse mesmo commit aqui, para em outros commits trabalhar nos módulos com atualizações `major` que geralmente são mais complicadas de se fazer. Mas bom, com as versões `patch` estabilizadas, parti para atualizar de uma só vez todas as `minor` para ver o que iria acontecer e rodei o seguinte: ``` npm i -E cookie@1.1.1 pg@8.20.0 prettier@3.8.1 swr@2.4.1 ``` Nesse caso, rodei de novo um `npm test`, deu tudo ok, mas como foi atualizado o `Prettier`, eu aproveitei para também rodar o script dele: ``` npm run lint:prettier:check ``` E tudo funcionou. E então com os testes e `Prettier` funcionando, fiz `amend` neste commit aqui para nos próximos commits me focar nas atualizações `major`.
Neste commit eu resolvi colocar todas as atualizações de `major` que não tive problema algum em atualizar, foi de forma transparente, sem necessidade de alteração de código. Só que o curioso é que o commit começou tentando atualizar o `uuid` para versão `13.0.0` e quebrou muita coisa. Quebrou porque o módulo, a partir da versão `12`, deprecou por completo o suporte ao `CommonJS` e o Jest tentava importar o módulo usando esse padrão. Tinha algumas formas de contornar isso, mas um contribuinte do módulo sugeriu pinar a dependência na versão `11` que já era a que o projeto usava: uuidjs/uuid#903 (comment) Vou esperar o Jest melhorar o seu suporte a módulos `ESM` e caso o módulo `uuid` venha a se tornar um problema, resolvo isso ou troco de módulo por ele fazer coisas simples. Se tornou uma operação de saldo negativo continuar tentando batalhar pela atualização dele, dado que só vou investir tempo sem ter nenhum retorno prático. E aí falando em Jest, resolvi atualizar ele e fiz dessa forma: ``` npm i -E jest@30.3.0 ``` Os testes continuaram passando de forma transparente, tanto de forma geral com `npm test` quanto com `npm run test:watch -- user/get` e foi daí que esse commit começou com a seguinte mensagem: ``` chore: bump `jest` to the latest major version ``` Próximo passo foi atualizar o `set-cookie-parser` e fiz assim: ``` npm i -E set-cookie-parser@3.0.1 ``` Testes continuaram passando sem problemas e então fiz o amend das alterações contra esse mesmo commit aqui, atualizando também a lista dos módulos na mensagem principal dele ficando assim: ``` chore: bump `jest`, `set-cookie-parser` to the latest major version ``` Só que daí percebi que com essa estratégia, iria rapidamente estourar a quantidade de caracteres recomendados para uma mensagem de commit e mudei então para sinalizar que aqui vão estar módulos da versão major que foram atualizados sem problema algum, sem precisar modificar nada a mais e ficou assim: ``` chore: upgrade `major` dependencies that required no code changes ``` Daí se eu instalar um próximo módulo e tudo ficar 100%, faço `amend`, mas se der problema, desfaço as alterações e tento outro módulo. Então abaixo segue o restante das instalações que funcionaram de forma transparente, na ordem que eu executei: ``` npm i -E concurrently@9.2.1 npm i -E @commitlint/cli@20.5.0 @commitlint/config-conventional@20.5.0 npm i -E dotenv@17.3.1 dotenv-expand@12.0.3 npm i -E dotenv-expand@12.0.3 npm i -E nodemailer@8.0.3 ``` O curioso é que o módulo `dotenv@17.3.1` instalou de forma transparente, mas ele começou a injetar propagandas dentro dos logs no terminal. Então escolhi voltar a versão que já estava para procurar uma alternativa. Em paralelo, seguem os módulos que não instalaram de forma transparente: ``` @faker-js/faker@10.3.0 eslint@10.0.3 eslint-config-next@16.2.0 eslint-config-prettier@10.1.8 eslint-plugin-jest@29.15.0 node-pg-migrate@8.0.4 uuid@13.0.0 ``` Fora isso, o que faltou foi o Next.js e React, mas estes vou fazer através de um negócio chamado `codemod` em um próximo commit e que inclusive vai ajudar na atualização do `eslint` e seus sub-módulos.
…ions Neste commit eu inicio utilizando o `codemod` do Next.js para atualizar tanto o framework, quanto o React e de quebra o ESLint. Um `codemod` é uma facilidade que uma empresa pode fornecer para fazer por você as migrações de código e dependência necessárias para que o sistema volte a ficar compatível. Então primeiro rodei o comando principal do `codemod` apontando para uma versão específica para eu e você continuarmos sincronizados: ``` npx @next/codemod upgrade 16.2.0 ``` A primeira pergunta que o `codemod` fez foi se eu queria continuar no React versão `18` e escolhi que não, que queria atualizar para a `19`. Depois disso ele me listou todos os modificadores que ele iria aplicar no código, escolhi aplicar todos, sendo que no nosso caso, o principal é o abaixo dado que o Next.js não faz mais o tunelamento do linting para o ESLint e agora devemos usar diretamente o ESLint: ``` next-lint-to-eslint-cli ``` Depois ele perguntou se queria então rodar o modificador do React 19 e o modificador dos tipos, e respondi sim para ambos, mesmo não usando TypeScript no projeto. Só que ao tentar aplicar o `codemod` para valer, o `npm` reclamou de uma incompatibilidade de `Peer Dependency`: ``` Upgrading your project to Next.js 16.2.0... npm error code ERESOLVE npm error ERESOLVE unable to resolve dependency tree npm error npm error While resolving: clone-tabnews@1.0.0 npm error Found: eslint@8.57.0 npm error node_modules/eslint npm error dev eslint@"8.57.0" from the root project npm error npm error Could not resolve dependency: npm error peer eslint@">=9.0.0" from eslint-config-next@16.2.0 npm error node_modules/eslint-config-next npm error dev eslint-config-next@"16.2.0" from the root project ``` Em resumo, o `codemod` quer instalar o módulo `eslint-config-next` na sua versão `16.2.0`, mas ele agora necessita que o módulo `eslint` esteja no mínimo na versão `9.0.0` e o que está instalado no projeto é a versão `8.57.0`. Dado a isso, o `npm` não consegue resolver essa exigência na árvore de dependências e interrompe a instalação com um erro. Então eu desfiz todas as alterações que o `codemod` tentou fazer: ``` git checkout -- . ``` Reinstalei todas as dependências de novo para voltar como estava antes: ``` npm i ``` Atualizei todas as dependências relacionadas a ESLint da seguinte forma antes de tentar de novo o `codemod`: ``` npm i -E eslint@10.0.3 eslint-config-next@16.2.0 \ eslint-config-prettier@10.1.8 eslint-plugin-jest@29.15.0 ``` O `npm` retornou alguns warnings a respeito de sobrescrever algumas peer dependencies, mas instalou sem problemas. Rodei de novo o `codemod` e ele começou a trabalhar de fato: ``` npx @next/codemod upgrade 16.2.0 ``` Começou a aparecer um monte de coisa no terminal, até ele perguntar se eu faço o deploy do meu app na Vercel, respondi que sim. Mas infelizmente, final da história, o comando terminou com o seguinte erro: ``` npm error code ECOMPROMISED npm error Lock compromised npm error A complete log of this run can be found in: /home/codespace/.npm/_logs/... Error: Legacy codemod command failed with exit code: Some(1) Error: Command failed: npx --yes codemod@latest react/19/migration-recipe --no-interactive ``` Doideira, então eu entendi que teria que arregaçar as mangas e continuar de onde ele parou. Analisando o repositório, o `codemod` alterou meu `package.json` com as novas versões dos módulos e também alterou o `lint:eslint:check` para apontar diretamente para o ESLint: ``` "lint:eslint:check": "eslint .", ``` Então decidi testar e ao rodar o ESLint através do script, quebrou: ``` npm run lint:eslint:check ``` Recebi um erro que começa desta forma: ``` Error [ERR_MODULE_NOT_FOUND]: Cannot find package '@eslint/js' imported from ... ``` Investigando, o `codemod` no momento da migração leu o arquivo legado de configuração do ESLint chamado `.eslintrc.json` e converteu ele para uma nova versão chamado `eslint.config.mjs` e esta nova versão está pedindo por um módulo chamado `@eslint/js` que não está instalado no projeto e, de fato, não está. Aí investigando na documentação de migração do ESLint no endereço abaixo, eu vi que eles também fornecem um `codemod`: https://eslint.org/docs/v10.x/use/configure/migration-guide Então antes de rodar ele, removi o arquivo criado pelo `codemod` do Next.js: ``` rm eslint.config.mjs ``` E rodei o `codemod` do ESLint: ``` npx @eslint/migrate-config .eslintrc.json ``` Ele rodou sem problemas, mas pelo menos me avisou: ``` You will need to install the following packages to use the new config: - @eslint/js - @eslint/eslintrc ``` Então eu instalei da seguinte forma: ``` npm i -E @eslint/js@10.0.1 @eslint/eslintrc@3.3.5 ``` Rodei o script para testar de novo o ESLint: ``` npm run lint:eslint:check ``` E recebi um novo erro que começa com: ``` TypeError: Converting circular structure to JSON --> starting at object with constructor 'Object' | property 'configs' -> object with constructor 'Object' ``` Aí pensei: “Essa migração automática está um LIXO… o arquivo `eslint.config.mjs` está um NOJO. Vou começar tudo do zero”... e o primeiro passo foi desinstalar todos os módulos relacionados ao ESLint. ``` npm uninstall @eslint/js @eslint/eslintrc eslint \ eslint-config-next eslint-config-prettier eslint-plugin-jest ``` Removi também o arquivo de configuração: ``` rm eslint.config.mjs ``` Daí fui para a documentação do módulo sobre como instalar ele, o “Getting Started”, pra realmente começar do zero: https://eslint.org/docs/v10.x/use/getting-started E para nós dois rodarmos o mesmo comando, rode o seguinte: ``` npm init @eslint/config@1.11.0 ``` De início o ESLint perguntou “What do you want to lint?” e eu escolhi todas as opções: ``` ◉ JavaScript ◉ JSON ◉ JSON with comments ◉ JSON5 ◉ Markdown ◉ CSS ``` Depois ele perguntou “How would you like to use ESLint?” e escolhi: ``` ▸ To check syntax and find problems ``` Depois ele perguntou “What type of modules does your project use?” e escolhi: ``` ▸ JavaScript modules (import/export) ``` Depois ele perguntou “Which framework does your project use?” e escolhi: ``` ▸ React ``` Depois ele perguntou “Does your project use TypeScript?” e eu escolhi: ``` ‣ No ``` Depois ele perguntou “Where does your code run?” e eu escolhi as duas opções: ``` ◉ Browser ◉ Node ``` Depois ele perguntou “What flavor of Markdown do you want to lint?” e eu escolhi: ``` ▸ GitHub Flavored Markdown ``` E então ele me avisou que iria instalar alguns pacotes e perguntou se eu queria instalar eles agora, mas respondi que não! Respondi isso, porque se não ele iria instalar a última versão dos pacotes e eu e você poderíamos ficar dessincronizados. Dado a isso, escolha a opção `No` também e instale manualmente comigo estas versões: ``` npm i -E -D eslint@9.39.4 @eslint/js@9.39.4 globals@17.4.0 \ @eslint/json@1.1.0 @eslint/markdown@7.5.1 @eslint/css@1.0.0 \ eslint-config-next@16.2.0 eslint-plugin-jest@29.15.0 eslint-config-prettier@10.1.8 ``` Dois detalhes: 1. Atualmente a versão mais atualizada do ESLint é a `10`, que foi lançada faz pouco tempo e, dado a isso, estou instalando a `9` porque o ecossistema de plugins ainda não conseguiu se atualizar e ficar compatível com as novas APIs da versão `10`. Um dos plugins é o `eslint-plugin-react` que é utilizado por baixo dos panos pelo `eslint-config-next`. 2. No comando de instalação já aproveitei para instalar o `eslint-config-next`, `eslint-plugin-jest` e `eslint-config-prettier` para ficar compatível com o que tínhamos anteriormente. Em paralelo a tudo isso, o `codemod` do ESLint deve ter criado um arquivo chamado `eslint.config.mjs` com algumas configurações iniciais, mas que para utilizar os plugins extras e continuar exatamente como estava antes, utilize a versão do arquivo encontrada neste commit que apenas inclui o uso dos plugins extras que utilizamos para linting do Next.js, Jest e evitar conflito das regras do Prettier. Outro detalhe é que, com a atualização do ESLint e a criação deste novo arquivo de configuração, o arquivo anterior `.eslintrc.json` não é mais utilizado e pode ser removido, então antes do commit eu executei: ``` git rm .eslintrc.json ``` E mais um detalhe interessante, agora proposto pelo aluno `rodrigofinkler`, é fazer o ESLint retornar um `exit code` acima de `0` também para `warnings` para, por exemplo, quebrar o pipeline do CI caso encontre alguma situação assim. Então o script do npm ficou desta forma: ``` "lint:eslint:check": "eslint . --max-warnings 0", ``` Por fim, rodei um `npm test` para entender se a camada da aplicação estava ok e para minha surpresa estava tudo ok, todos os testes rodaram 100%. Também subi manualmente o servidor web com `npm run dev`, testei a página do `/status` e também tudo funcionando 100%. Último teste foi fazer um erro proposital no código para ver se o novo ESLint continuaria quebrando o fluxo no CI e quebrou com sucesso e daí parti para o commit final que é este que você está lendo neste exato momento. Próximo passo vai ser testar criação e ativação de usuários lá na URL de homologação para ver se tudo continua ok.
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
18 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Este PR agrega todos os ajustes finais conforme descrito na issue #44 e como explicado lá, irei parcialmente atualizando este PR com novos commits e atualizando a lista de tarefas 🤝
Sugestão: não olhe o
diffagregado de todos os commits deste PR, pois ficará muito confuso. Olhe odiffde commit a commit: https://github.com/AlAnNuB/clone-tabnews/pull/45/commits