Skip to content

fix(dl): recognise a document by its id, not by its file name - #398

Open
LudwigJMarx wants to merge 2 commits into
pytr-org:masterfrom
LudwigJMarx:fix/391-duplicate-downloads
Open

LudwigJMarx wants to merge 2 commits into
pytr-org:masterfrom
LudwigJMarx:fix/391-duplicate-downloads

Conversation

@LudwigJMarx

Copy link
Copy Markdown

dl_docs derives the target path from the event title and checks only that
one path before downloading. For certificates the title carries the current
barrier and moves daily, so every run misses the file it wrote yesterday and
fetches the same document again under a new name.

Reproduction

No account and no debug log needed. tests/events/tilgung.json is a
certificate. Three runs of dl_callback into one directory, changing nothing
but the event title (event id, document id, timestamp and content identical):

run 1  event.title='Long 237,3067 $'  doc.id=75aa8fe4-cec5-4963-bf8a-2f5aec65c4f5
       -> Dividende/2025-01-10 10_00 Long 237,3067 $ - Tilgung.pdf
run 2  event.title='Long 237,2596 $'  doc.id=75aa8fe4-cec5-4963-bf8a-2f5aec65c4f5
       -> Dividende/2025-01-10 10_00 Long 237,2596 $ - Tilgung.pdf
run 3  event.title='Long 237,2126 $'  doc.id=75aa8fe4-cec5-4963-bf8a-2f5aec65c4f5
       -> Dividende/2025-01-10 10_00 Long 237,2126 $ - Tilgung.pdf

3 files for one document

That matches the three PDFs in the screenshot of #391.

Cause

Location What happens there
dl.py:347 title = f"{doc['title']} - {event['title']} - {event['subtitle']}{suffix}". For derivatives the event title is the moving instrument name.
dl.py:416 filename_fmt.format(...), default {iso_date} {time} {title} (main.py:285). The stable document id is not part of it.
dl.py:475 if filepath.is_file() is False: downloads. Only that one computed path is checked, never the document.

The document id is stable and already available. Today it is used only as a
fallback on name collisions (filename_with_doc_id), never for recognition.

Fix

all_events.json already maps document id to the path used last time:
--store-event-database is the default (main.py:309), and timeline.py:267
appends the same dict it hands to dl_callback, so the local_filepath
written in dl_doc ends up in the database. dl_doc now asks that database
before falling back to the computed name, and accepts an entry only while its
file is still on disk. Read lazily on first use, so --flat and a first ever
run never touch it.

+53/-1 in pytr/dl.py, +105 in tests/test_dl_paths.py. --flat is untouched, the addition sits
in the non-flat branch.

Evidence

Measured on e7f3ba3, Python 3.14.7, macOS:

test commit f84ace8 (test present, fix absent):
    assert first_document(today)["local_filepath"] == str(yesterday)
    E  -  Long 237,3067 $ - Tilgung.pdf
    E  +  Long 237,2596 $ - Tilgung.pdf
    1 failed, 226 passed in 2.69s

fix commit fb29439:
    230 passed in 9.37s

ruff check: All checks passed
ruff format: 28 files already formatted
mypy: Success: no issues found in 22 source files

Reproduction script unchanged across both runs:

before                              after
Long 237,3067 $ - Tilgung.pdf       Long 237,3067 $ - Tilgung.pdf
Long 237,2596 $ - Tilgung.pdf
Long 237,2126 $ - Tilgung.pdf
3 files for one document            1 file for one document

Of the four added tests, one is the regression test above; the other three
cover an empty document id (buy.json ships one, and it must not match every
other id-less document), a database entry whose file has been moved away, and
a truncated database. Each was checked by mutation: dropping the id filter in
read_event_database, the is_file() guard, or the lookup in dl_doc turns
exactly one of them red.

Alternatives rejected

Approach Why not
Put the id into the default filename_fmt Fixes it, but renames every future download and makes the whole existing archive look new once.
Scan the output directory for the id Only works if the id is part of the name, which it is not in the default format.
Index from events_with_documents.json Same mapping, but the file exists only under --dump-raw-data, so it would help nobody who has not already opted in.

Deliberate side effect

After a change to --format, documents already on disk keep their old path
instead of being downloaded a second time under the new one. Nothing is
renamed, which is what happens today as well.

Known limit

A relative output path is stored as given, so running pytr from a different
working directory misses the entry and downloads again. That is today's
behaviour too.

Not fixed here

dl.py:455 tests filepath in self.filepaths, a Path against a list that
dl.py:464 fills with str, so the comparison is always False and the
filename_with_doc_id fallback next to it has never run. Separate defect,
separate PR.

HSGEV reports in #391 that four years of files are re-downloaded. That is
not reproduced here and looks like a one-off filename scheme change rather
than a moving title.

Refs #391

Certificates are renamed every day. The instrument keeps its ISIN and the
document keeps its id, but the event title carries the current barrier, so
"Long 237,3067" becomes "Long 237,2596" overnight. dl_doc derives the target
path from that title (dl.py:347 feeds filename_fmt at dl.py:416), and the
only existence check is `filepath.is_file()` on that freshly computed path
(dl.py:475). Every run therefore misses the file it wrote yesterday and
fetches the same document again, which is what pytr-org#391 reports with three
identical PDFs.

The test replays two runs into one directory from tests/events/tilgung.json,
changing nothing but the event title, and pins both halves of the promise:
the second run must reuse yesterday's path, and the directory must still hold
exactly one PDF.

It fails on this commit:

    assert first_document(today)["local_filepath"] == str(yesterday)
    E  -  Long 237,3067 $ - Tilgung.pdf
    E  +  Long 237,2596 $ - Tilgung.pdf
    1 failed, 226 passed in 2.69s

The fixture is written as all_events.json between the runs because that is
what the next run reads: --store-event-database is on by default
(main.py:309), and timeline.py:267 appends the very dict it then hands to the
callback, so the database carries local_filepath per document.
events_with_documents.json would be the wrong anchor, it only exists with
--dump-raw-data.

Refs pytr-org#391
The target path is built from the event title (dl.py:347 -> filename_fmt at
dl.py:416), and `filepath.is_file()` at dl.py:475 is the only thing standing
between an event and a download. For certificates the title carries the
current barrier and changes daily, so the computed path never matches the
file written yesterday and the same document arrives again under a new name.

The document id is stable, and all_events.json already maps it to the path
used last time: --store-event-database is the default (main.py:309), and
timeline.py:267 appends the same dict it hands to dl_callback, so the
local_filepath written in dl_doc ends up in the database. dl_doc now asks
that database before falling back to the computed name, and accepts an entry
only while its file is still on disk.

Reproduction, three runs with a moving title into one directory, event id,
document id and timestamp unchanged:

    before                              after
    Long 237,3067 $ - Tilgung.pdf       Long 237,3067 $ - Tilgung.pdf
    Long 237,2596 $ - Tilgung.pdf
    Long 237,2126 $ - Tilgung.pdf
    3 files for one document            1 file for one document

    pytest: 1 failed, 226 passed  ->  230 passed
    ruff check / ruff format / mypy: clean (python 3.14.7)

Three added tests cover what the change promises: the moving title, an empty
document id (buy.json ships one, and it must not match every other id-less
document), a database entry whose file has been moved away, and a truncated
database. Each was checked by mutation: dropping the id filter in
read_event_database, the is_file() guard, or the lookup in dl_doc turns
exactly one of them red.

Alternatives rejected. Putting the id into the default filename_fmt would fix
it too, but renames every future download and makes the whole existing
archive look new once. Scanning the output directory for the id only works if
the id is part of the name, which it is not in the default format.
events_with_documents.json carries the same mapping but exists only under
--dump-raw-data, so it would help nobody who has not already opted in.

Deliberate side effect: after a change to --format, documents already on disk
keep their old path instead of being downloaded a second time under the new
one. Files are never renamed, which is what happens today as well.

Known limit: a relative output path is stored as given, so running pytr from
a different working directory misses the entry and downloads again, which is
today's behaviour.

Not touched here, separate defect: dl.py:455 tests `filepath in
self.filepaths`, a Path against a list that dl.py:464 fills with str, so the
comparison is always False and the filename_with_doc_id fallback next to it
has never run.

Refs pytr-org#391
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant