Repository navigation
fix(client): persist file metadata on the second _update_state call - #358
Conversation
`download()` records a file twice: once as DOWNLOADING with no
metadata, then as COMPLETED with the year, month, state and group
parsed from the remote file. The second call finds the row the first
one created and only ever touched `status` and `last_synced`, so
`year`, `month`, `state` and `group` stayed NULL:
>>> await p._update_state(p, r, "ftp", DownloadStatus.DOWNLOADING)
>>> await p._update_state(p, r, "ftp", DownloadStatus.COMPLETED,
... year=2024, month=1, state="SP", group="DC")
>>> r.year, r.month, r.state, r.group
(None, None, None, None)
Every file PySUS had ever downloaded was missing all four columns, and
`get_local_hierarchy` keys its output off `group`, so the tree came out
wrong as well.
The existing record now takes `remote_path` and `client_name` (always
supplied) plus `year`/`month`/`state`/`group` when they are not None.
The None check matters: the FAILED call in `download()`'s except block
also passes no metadata, and must not erase what COMPLETED recorded.
|
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #358 +/- ##
=======================================
Coverage ? 97.22%
=======================================
Files ? 180
Lines ? 22790
Branches ? 0
=======================================
Hits ? 22158
Misses ? 632
Partials ? 0 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
luabida
left a comment
luabida
left a comment
There was a problem hiding this comment.
Please solve the conflicts in order to merge this PR
Resolves the conflict with AlertaDengue#359 (already merged), which appends test classes to the same two files. The two fixes touch different regions of pysus/api/client.py and merge cleanly there. pysus/tests/api/test_client.py conflicted because AlertaDengue#358 and AlertaDengue#359 both append at end of file. Beyond concatenating them, this fixes two problems that AlertaDengue#359 introduced when it merged: - It defined a second `TestGetLocalHierarchy`, shadowing the existing class of the same name, so `test_get_local_hierarchy_all_branches` silently stopped being collected. It is now `TestGetLocalHierarchyDatasetLevel` and both classes run (51 tests in this file, up from 46). - That existing test asserted the *buggy* output of AlertaDengue#359: it expected a grouped file under its group name (`ftp_dict["DC"]`) and an ungrouped one under the client name (`ftp_dict["ftp"]`). Its assertions are updated to the corrected shape, and tightened: the old ones happened to pass partly by coincidence, so it now also pins down the directory record, the ungrouped bucket, and that "DC", "ftp" and "short" are not dataset keys.
|
Conflito resolvido — o PR agora está A causa era o #359, que já foi mergeado e mexe nos mesmos dois arquivos. As duas correções em Ao resolver, encontrei dois problemas que o #359 deixou no 1. Uma classe de teste foi silenciosamente desativada. O #359 definiu um segundo 2. O teste existente afirmava o comportamento incorreto. Ele esperava o arquivo agrupado sob o nome do grupo ( Também vale notar que as asserções antigasEncode o bug de um jeito que só passava por acidente:
@lubabida — se preferir, posso abrir isso como um PR separado (só a reparação do teste) e deixar o #358 restrito apenas ao |
|
🎉 This PR is included in version 2.11.4 🎉 The release is available on:
Your semantic-release bot 📦🚀 |
Summary
download()records a file twice: once asDOWNLOADINGwith no metadata, then asCOMPLETEDwith the year, month, state and group parsed from the remote file. The second call finds the row the first one created and only ever touchedstatusandlast_synced:year,month,stateandgroupstayed NULL for every file PySUS had ever downloaded.Impact
local_file_stateare permanently emptyget_local_hierarchybuilds its{client: {dataset: {group: [...]}}}tree fromgroup, so the tree was keyed off the wrong level and ungrouped files landed under the client's own nameFix
The existing record now takes
remote_pathandclient_name(always supplied positionally), plusyear/month/state/groupwhen they are notNone.The
Nonecheck is load-bearing: theFAILEDcall indownload()'sexceptblock also passes no metadata and must not erase whatCOMPLETEDrecorded.Tests
The existing
TestLocalFileStateonly covered the create path, which is why this was invisible. Added 4 tests reproducing the real two-step sequence: metadata is persisted on the second call, only one row is created, aFAILEDcall keeps the completed metadata, and stale metadata is refreshed.All 4 fail against the previous code.
1737 passed, 6 skippedblackandisortclean