Adiciona a extensão correspondente no arquivo que está sendo baixado

AlexJBSilva commented 7 months ago

Descrição

Adiciona a extensão correspondente no arquivo que está sendo baixado, quando o scrapy não consegue achar a extensão pela url de download. Essa parte é uma implementação (com ajustes) do código sugerido pelo @ogecece em https://github.com/okfn-brasil/querido-diario/pull/946#pullrequestreview-1656259871 que utiliza a biblioteca filetype.

Para resolver o problema de "nos casos onde forçamos a detecção da extensão, o arquivo sempre seria baixado novamente de forma desnecessária", o método stat_file foi sobrescrito, com a inclusão da busca por um arquivo com extensão quando o scrapy não consegue achar a extensão pela url de download para comparar se o arquivo já foi baixado. Resolve #819 O PR #946 pode ser encerrado.

Checklist - Novo spider

[ ] Você executou uma extração completa do spider localmente e os dados retornados estavam corretos.
[x] Você executou uma extração por período (start_date e end_date definidos) ao menos uma vez e os dados retornados estavam corretos.
[x] Você verificou que não existe nenhum erro nos logs (log_count/ERROR igual a zero).
[ ] Você definiu o atributo de classe start_date no seu spider com a data do Diário Oficial mais antigo disponível na página da cidade.
[ ] Você garantiu que todos os campos que poderiam ser extraídos foram extraídos de acordo com a documentação.

ogecece commented 3 months ago

@rennerocha tu consegue dar uma olhada nesse PR? Eu comecei a revisar hoje com a @trevineju e me pareceu que o caminho que o @AlexJBSilva tá seguindo tá correto, mas precisaríamos que também fosse implementado o S3FilesStore.

Pensa em outro caminho de implementação?

rennerocha commented 3 months ago

@ogecece eu já tinha dado uma olhada nisso a um tempo atrás. Vou ver com calma amanhã e sugiro algo (ou não)

ogecece commented 2 months ago

@rennerocha conseguisse olhar?

rennerocha commented 2 months ago

@rennerocha conseguisse olhar?

Estou analisando agora. Aparentemente é esse o caminho. Estou analisando para ver se é possível deixar mais simples, evitando mudar demais o padrão do Scrapy. Porém como você mesmo comentou, essa alteração só funciona para arquivos armazenados localmente. É necessário uma implementação do stat_file para o S3FilesStore, ou senão não irá funcionar em produção.

rennerocha commented 2 months ago

Acabei de analisar com mais calma e encontrei um problema com a solução proposta que pode não afetar o armazenamento do arquivo, mas cria uma inconsistência de resultados. Na chave path, retornamos o caminho onde o arquivo está armazenado. Porém se executarmos mais de uma vez para a mesma data, as execuções seguintes corretamente não fazem o download, adicionando o status com uptodate, mas o path fica sem a extensão. Isso ocorre porque como o arquivo não precisa ser baixado na segunda execução, não é necessário definir o filepath (que adiciona a extensão).

Primeira execução:

{'date': '2024-04-18',
 'file_urls': ['https://diariooficial.santos.sp.gov.br/edicoes/inicio/download/2024-04-18'],
 'files': [{'checksum': '09d4708a4d58287e88303f1719f88ba5',
            'path': '3548500/2024-04-18/52662a6f24c42ba582c341db6cc345eb9d78fe67.pdf',
            'status': 'downloaded',
            'url': 'https://diariooficial.santos.sp.gov.br/edicoes/inicio/download/2024-04-18'}],
 'is_extra_edition': False,
 'power': 'executive_legislative',
 'scraped_at': '2024-04-19T00:18:25.031138Z',
 'territory_id': '3548500'}

Segunda execução:

{'date': '2024-04-18',
 'file_urls': ['https://diariooficial.santos.sp.gov.br/edicoes/inicio/download/2024-04-18'],
 'files': [{'checksum': '09d4708a4d58287e88303f1719f88ba5',
            'path': '3548500/2024-04-18/52662a6f24c42ba582c341db6cc345eb9d78fe67',
            'status': 'uptodate',
            'url': 'https://diariooficial.santos.sp.gov.br/edicoes/inicio/download/2024-04-18'}],
 'is_extra_edition': False,
 'power': 'executive_legislative',
 'scraped_at': '2024-04-19T00:18:36.238569Z',
 'territory_id': '3548500'}

O que eu comentei acima pode ser solucionado provavelmente alterando o QueridoDiarioFSFilesStore e também a mesma versão do S3FileStore que precisa ser implementada.

rennerocha commented 2 months ago

Estou achando essa solução com a implementação de FilesStores customizados mais complicada do que precisava ser. Considero solução mais simples, apenas definindo a extensão no método file_path e refazendo o download em algumas situações é aceitável pois:

Rodamos cada raspador uma vez por dia, buscando apenas os dados do último dia, então se por acaso formos refazer o download, será no máximo de apenas 1-2 arquivos (sem impacto grande no tempo de execução)
São poucos os raspadores que vão precisar definir a extensão dessa maneira.

Minha sugestão (e se concordarem eu faço o ajuste no PR) é usar o filetype para identificar o tipo do arquivo e definir a extensão, sem se preocupar se ele vai ser baixado de novo ou não em uma re-execução do raspador.

O código acabará muito mais simples de se manter e resolvemos esse problema imediato rapidamente. Se no futuro percebermos algum impacto grande no tempo de execução dos raspadores, aí podemos reconsiderar a mudança mais complexa.

O que acham? @ogecece @trevineju @AlexJBSilva

trevineju commented 2 months ago

Oi, @rennerocha! Obrigada por revisar!

Como o @ogecece avançou mais do que eu nessa revisão, acho que ele pode comentar melhor sobre a solução pelo lado mais técnico. Mas pelo lado de impacto...

São poucos os raspadores que vão precisar definir a extensão dessa maneira.

Integrados, ainda são poucos, realmente. Mas conhecidos são bastantes já, como os sistemas DOSP (~250 municípios) e SAIIO (~300 municípios), por exemplo, todos eles tem esse problema. Já foram identificados casos individuais também, mas não me recordo deles de cabeça agora.

Será que não vale a pena já pensarmos na solução mais ideal desde agora? Pq nos daria mais liberdade de adicionar essas cidades sem agravar a situação (pensando que já temos esse problema em produção hoje e vamos ter que parar para corrigi-lo, em breve)

rennerocha commented 2 months ago

Será que não vale a pena já pensarmos na solução mais ideal desde agora? Pq nos daria mais liberdade de adicionar essas cidades sem agravar a situação (pensando que já temos esse problema em produção hoje e vamos ter que parar para corrigi-lo, em breve)

O problema em produção que temos, de não ter a extensão correta, pode ser resolvido pela solução simples (que eu prefiro, apesar de baixar um Diário mais de uma vez SE o raspador for executado novamente naquela data, o que não acontece em produção) ou com a complexa (que vai exigir mais tempo para testar os casos). Então não vai agravar nenhum problema, e vai resolver ele rapidamente, com menos código.

ogecece commented 2 months ago

@rennerocha, concordo. Se encontrou outro problema que ainda precisaríamos resolver e complexificar mais ainda a solução, melhor lidar com essa consequência que é contornável. Vai na mesma linha que eu havia comentado aqui, inicialmente.

Você consegue fazer os ajustes então pra gente mesclar?

rennerocha commented 1 month ago

Esse PR foi complementado por https://github.com/okfn-brasil/querido-diario/pull/1153, que irá fechar o problema.

okfn-brasil / querido-diario

Adiciona a extensão correspondente no arquivo que está sendo baixado #1045

Descrição

Checklist - Novo spider