feat(content): adjust validator function and create findChildrenCount function #366
Conversation
|
Someone is attempting to deploy this pull request to the TabNews Team on Vercel. To accomplish this, the commit author's email address needs to be associated with a GitHub account. Learn more about how to change the commit author information. |
|
@EmanoelCristhian is attempting to deploy a commit to the TabNews Team on Vercel. To accomplish this, @EmanoelCristhian needs to request access to the Team. Afterwards, an owner of the Team is required to accept their membership request. If you're already a member of the respective Vercel Team, make sure that your Personal Vercel Account is connected to your GitHub account. |
EmanoelCristhian
changed the title
There was a problem hiding this comment.
Excelente implementação e o ponto sobre os testes reclamarem quando há um campo novo eu concordo 100%! Daqui para frente toda refatoração de testes deveria ter isso como boa prática 🤝
Em paralelo, fiz algumas sugestões para tentar manter o children_count igual entre os tipos de visualizações que um objeto content possa ter 👍
| const childrenCount = await content.findChildrenCount({ | ||
| where: { | ||
| parent_id: contentFound.id, | ||
| }, | ||
| }); | ||
|
|
||
| contentFound['children_count'] = childrenCount; | ||
|
|
There was a problem hiding this comment.
Tava pensando aqui, talvez isso devesse ficar dentro do content.findAll(), uma vez que está sendo mesclado ao objeto um dado muito importante e que vai precisar existir em vários pontos da aplicação e não somente dentro desse controller. Por exemplo:
/api/v1/contents/[username]/[slug] - está retornando o `children_count`
/api/v1/contents/ - não está retornando o `children_count`
Isso vai pesar entre a aplicação e o banco, pois para cada linha do findAll() vamos precisar bater de novo no Banco, mas para agora não vamos nos preocupar com otimização.
| @@ -118,7 +118,7 @@ describe('GET /api/v1/contents/[username]/[slug]/children', () => { | |||
| expect(responseBody.length).toEqual(2); | |||
| expect(responseBody).toStrictEqual([ | |||
| { | |||
| id: responseBody[0].id, | |||
| id: childBranchALevel1.id, | |||
There was a problem hiding this comment.
Nossa, excelente alteração!
models/content.js
Outdated
| }); | ||
|
|
||
| list.forEach((row) => { | ||
| if (table[row.parent_id]) { | ||
| table[row.parent_id].children_count++; |
There was a problem hiding this comment.
Muito delicinha as duas linhas adicionadas nessa implementação, porém o comportamento não vai ficar igual ao resultado obtido pelo findChildrenCount(), uma vez que ele só irá somar os filhos diretos (e não irá considerar o número de sub-filhos numa árvore profunda).
Tanto que isso é revelado pelos testes, que em resumo hoje estão fazendo o assertion considerando isso como resultado:
[
{
"id": "fdbcbe30-c6bb-4e1f-b5a2-402b7f096236",
"children_count": 1
"children": [
{
"id": "3fb91d2e-0049-4845-8dd6-4e13e58f3655",
"children_count": 1
"children": [
{
"id": "8662cc5e-da61-4468-aa4d-c86289920e2c",
"children_count": 0
"children": [
],
}
],
}
],
}
]Quando deveria ser:
[
{
"id": "fdbcbe30-c6bb-4e1f-b5a2-402b7f096236",
"children_count": 2 // Esse conteúdo tem 2 children
"children": [
{
"id": "3fb91d2e-0049-4845-8dd6-4e13e58f3655",
"children_count": 1
"children": [
{
"id": "8662cc5e-da61-4468-aa4d-c86289920e2c",
"children_count": 0
"children": [
],
}
],
}
],
}
]Isso fariam os resultados de children_count ficarem diferentes entre você consultar a árvore pelo nó do root ou diretamente o objeto de um content.
There was a problem hiding this comment.
@filipedeschamps acredito que podemos melhorar essa semântica.
O que você acha de renomearmos o children_count para algo como children_deep_count ou children_recursive_count ou ainda chamá-lo de interactions ?
Do jeito que está causa esse tipo de confusão, principalmente para quem for consumir a API no futuro, tendo em vista que temos o children com length (que em algumas linguagens é chamado de count) igual a 1 e o children_count será igual a 2.
There was a problem hiding this comment.
Tembra, interessantíssimo. Essa diferença poderia até ser usada na interface para quando quisermos esconder os comentários após um certo nível e mostrar quantos comentários diretos existem.
O que estou me perguntando é se a propriedade deveria respeitar a modelagem no banco, dado que todo child, independente do nível, faz parte do grupo children. Então talvez deveria ser o contrário e os children diretos deveriam ser children_shallow_count, mas isso confunde a semântica como você apontou (a diferença entre filhos diretos e o valor encontrado no count que pode ser maior).
Dos nomes que você sugeriu, o que me fez entender sem pensar muito foi o children_deep_count 🤝
O que acha @EmanoelCristhian ? De deixarmos o children_count com o valor de filhos diretos, e o children_deep_count que soma toda a árvore embaixo de um nó. Lembrando que toda vez que um objeto content estiver presente, essas propriedades também precisarão estar presentes.
There was a problem hiding this comment.
De deixarmos o
children_countcom o valor de filhos diretos
Realmente ele precisaria existir? Tendo em vista que (no JavaScript) um children.length obtém o mesmo resultado.
Por outro lado o children_deep_count já exige um certo processamento devido a interação recursiva, a qual estaria sendo executada direto no banco de dados.
There was a problem hiding this comment.
Eu me perguntei a mesma coisa, mas daí comecei a perguntar duas coisas:
- Qualquer tipo de client (em qualquer tecnologia) pode ser construído e cima da API, e talvez ter esse número computado seja uma mão na roda.
- Na lista de contents
/api/v1/contentsa propriedadechildrennão é retornada (inclusive tava pensando até que obodynesse caso não deveria ser retornado, mas isso fica pra outra thread) e daí nesse caso o client pode decidir o que achar mais justo mostrar (shallow ou deep).
Mas posso estar over engineering 🤝
There was a problem hiding this comment.
2. Na lista de contents
/api/v1/contentsa propriedadechildrennão é retornada
Parei de ler aqui 😅 Você me convenceu hehe.
Por mim fechado. Devemos ter o children_count e o children_deep_count.
inclusive tava pensando até que o
bodynesse caso não deveria ser retornado
Concordo total. Tráfego de dados desnecessário. Acredito que o objetivo deste endpoint seja apenas listar. Na outra thread poderíamos abrir para discussão o fato de torná-lo opcional, trazendo a informação apenas mediante o preenchimento de um parâmetro via query string, como with=body ou body=true.
There was a problem hiding this comment.
@filipedeschamps @tembra, eu concordo totalmente com vocês.
Essa task me fez bugar um pouco quando eu pensei se era para eu retornar os filhos diretos ou todos os filhos de tal content.
Irei ajustar os códigos de acordo com o que foi acordado aqui!
There was a problem hiding this comment.
Show de bola @EmanoelCristhian ! A gente vai se bugando juntos até chegar numa conclusão 🤝 e fico muito feliz com sua contribuição, pois isso me possibilita nesse exato momento estar trabalhando no PR sobre o reset de senha 👍
models/content.js
Outdated
| export default Object.freeze({ | ||
| findAll, | ||
| findOne, | ||
| findChildren, | ||
| findChildrenTree, | ||
| findChildrenCount, |
There was a problem hiding this comment.
Dado as outras considerações, talvez a gente não deveria exportar essa função por enquanto.
| }, | ||
| ], | ||
| children_count: 1, |
There was a problem hiding this comment.
Esse foi o detalhe que apontei no review no arquivo do model sobre a geração da árvore ter que considerar os children de forma deep.
… function Created the function that counts how many children a content has recursively. tests have been adjusted to test this new functionality.
…ontents recursively
filipedeschamps
force-pushed
the
childrenCount
branch
from
b6fe0e8
to
c8632c0
Compare
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
|
@EmanoelCristhian estou dando continuidade nessa branch, fiz rebase com a Conteúdo de teste (versão página):https://tabnews-git-childrencount-tabnews.vercel.app/filipedeschamps/testando-childrendeepcount Conteúdo de teste (versão API -
|
…on every `content`
filipedeschamps
force-pushed
the
childrenCount
branch
from
ca7fec4
to
3c06347
Compare
|
Pessoal, decisão não sei se polêmica, mas removi o Falando em custo, não sei se deveríamos nos preocupar agora, mas do jeito que foi implementado, para cada conteúdo nós fazemos uma query adicional em busca da quantidade de todos os filhos (independente da profundidade) e ela é feita de forma serial (dá para paralelizar, mas tenho medo de metralhar o banco de dados). Dado a isso, olha o gráfico abaixo do ambiente de Homologação que mostra a média de demora do endpoint
Por outro lado, não estamos usando índice e isso não ajuda nas buscas pelos valores. Bom, as alterações estão no commit 3c06347 e aproveitei para adicionar testes que identificaram um bug na query de pegar o número de filhos 🤝 ela retornava o valor de Agora tempos |
|
@filipedeschamps vendo um vídeo do Akita, que ele montou um banco de dados em JS em 3 dias hahaha!! Lembrei dele falando sobre índice, que só serve para melhorar a velocidade da consulta e deixa o insert / update mais lento. Pensando nessa lógica, você não acha uma boa cada PAI ser responsável pela contagem de seus filhos? Exemplo o número de filhos só é alterado quando alguém:
Não é justo fazer sempre essa consulta em toda listagem de conteúdo, sabendo que não foi alterado. Objetivo seria criar na tabela content a coluna children_count que seria atualizada nesses 2 momentos. Quando alguém fizer um comentário, fazer um update no PAI somando mais UM. |
|
Show! Outra forma que eu estava pensando era anotar em todos os contents o De qualquer forma, eu gosto de quando a gente faz algo novo sem performance, e daí dá o tempo necessário para que alguém bolar algo muito mais otimizado, e como tem bastante cobertura de testes, fazer esse tipo de refatoração também é muito massa 🤝 |
|
Número de comentários aparecendo na lista: https://tabnews-git-childrencount-tabnews.vercel.app/
Esta parte foi inspirada no PR #340 do @gabrielew tabnews.com.br/pages/interface/components/ContentList/index.js Lines 76 to 78 in 63a8def |
Perfeito! A solução do jeito que foi implementada não escala. Não havia falado antes pois sempre sugestões desse tipo são avaliadas para serem implementadas somente no futuro. Quando os bancos de dados começam a crescer precisamos guardar esses dados computados dentro da própria tabela. Aqui minha experiência com ERP volta a confirmar a normalidade disso. O único ponto de atenção necessário é no momento da implementação para que esses dados sempre fiquem íntegros. A ideia é sempre realizar essas atualizações em um único lugar para não sair do controle.
😆 Lembra da nossa discussão? Mas eu apoio ainda, apesar de você ter me convencido devido ao outro endpoint não ter a informação. |
Show 😂 🤝 Estou fazendo o merge na |
|
Merged! Isso levou a Milestone 4 para 83% concluída 🤝
Let's goooooo!!! |
|
@EmanoelCristhian você foi citado na última publicação comemorativa: https://www.tabnews.com.br/filipedeschamps/nova-melhoria-numero-de-comentarios-nas-publicacoes 😍 |
|
An owner of the TabNews Team on Vercel declined @EmanoelCristhian's request to join. In order for their commit to be deployed, @EmanoelCristhian must push and request access again. |
Created the function that counts how many children a content has recursively. tests have been
adjusted to test this new functionality.
#333
OBS: Ao adicionar a nova funcionalidade e rodar os testes, apenas os do children falharam. Ou seja, os testes de conteúdos raiz não estão testando caso tenha um dado a mais no JSON, apenas se tem a menos.