fix(store): a not-determined marker no longer overwrites a determination - #586
Merged
Merged
Conversation
STORE.md §6 lets a re-fetch rewrite the [doiget] table, and says why: "This is intentional, not silent: ... the operator always learns the entry was downgraded and why." The permission is conditional on the report. Since #539 there is a path with no report -- `metadata_only` without `include_oa_location` never runs the OA lookup, so it carries `oa_status: None` and `license: "unknown"`, those won, and nothing said so. No note: line, no pdf.status, no log row. Both are markers, not readings. The schema says as much: oa_status is "omitted when not determined" (#281), license falls back to "unknown". A paper that genuinely stops being open access reports Some("closed"); a license that changes reports the new string. So a call carrying the marker did not look, and preferring the stored value is not a guess about which is newer -- only one of the two is a value. merge_metadata's [doiget] arm now keeps the stored oa_status when the incoming one is None, and the stored license when the incoming one is LICENSE_UNDETERMINED. Everything else in the table still follows §6. "unknown" got a name so the check does not hang off a bare literal matching the others scattered through the orchestrator. Two tests: the preserve, and the other half -- a re-write that actually determined gold -> closed still wins, because a guard that suppressed real news would be worse than the bug. Mutation-checked: with the arm reverted the first fails on `left: None, right: Some("gold")`. The issue as filed said `oa_url` was overwritten with null. It is not. I wrote that from `build_metadata_only_metadata` and stopped before `FsStore::write`, which reads the existing file and merges; `merge_opt!(url)` already protected it. Probed before writing any of this, and #583 is corrected. Only the two [doiget] fields were ever affected, so the fix is a merge rule, not the store partition the issue proposed. Nothing BiblioFetch.jl reads changes shape: no field added, removed or renamed, schema_version unmoved, reserved top-level fields untouched. Refs #583 Signed-off-by: Souta <shimozono-sota631@g.ecc.u-tokyo.ac.jp>
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
Signed-off-by: Souta <shimozono-sota631@g.ecc.u-tokyo.ac.jp> # Conflicts: # Cargo.lock # Cargo.toml
github-actions Bot
added a commit
that referenced
this pull request
Aug 31, 2026
fix(store): a not-determined marker no longer overwrites a determination a4265cc
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
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.
Refs #583. The issue is corrected in a comment there — its central claim was wrong, and this fixes what is actually broken.
The permission was conditional, and the condition stopped holding
docs/STORE.md§6 lets doiget rewrite the[doiget]table on a re-fetch:Since #539,
metadata_onlyonly runs the OA lookup wheninclude_oa_locationis set. Omit it — the ordinary call shape — and the record carriesoa_status: Noneandlicense: "unknown". Those won, and the caller got nonote:line, nopdf.status, no log row. The clause the permission rests on was not met.What the fix rests on
Both values are markers, and the schema already says so:
oa_statusis "omitted when not determined (#281)",licenseis "an OA license string, or the literalunknown".oa_status: Some("closed")Neither reports the marker. So a call carrying
None/"unknown"did not look, and preferring the stored value is not a guess about which is newer — only one of the two is a value.merge_metadata's[doiget]arm keeps the stored value in exactly that case. Everything else in the table still follows §6."unknown"gained a name (LICENSE_UNDETERMINED) so the check does not hang off a bare literal matching the others scattered through the orchestrator.I got the issue wrong, and measured instead of re-reading
I filed #583 from
build_metadata_only_metadataand stopped beforeFsStore::write, which reads the existing file and merges. Probed it:url(whereoa_urllands)Some("https://example.org/paper.pdf")Some(…)— preserved[doiget].oa_statusSome("gold")None[doiget].license"CC-BY-4.0""unknown"merge_opt!(url)already protectedoa_url. So "the store file can be overwritten with nulls" was not true of the field the title named, and the store-partition / schema-change options the issue proposed were sized against a problem that is not there. Only the two[doiget]fields were affected; the fix is a merge rule.Tests
a_default_rewrite_does_not_downgrade_a_known_oa_status_or_license— the preserve, and it pinsurltoo so the two halves of A default metadata-only re-write silently downgrades [doiget].oa_status and .license to their not-determined markers #583 cannot drift aparta_rewrite_that_determined_a_new_oa_status_or_license_still_wins— the other half; a guard that suppressedgold → closedwould be worse than the bugMutation-checked: with the
[doiget]arm reverted, the first fails onleft: None, right: Some("gold").Also restored a
#[test]attribute I had knocked offmerge_metadata_preserves_existing_arxiv_categorieswhile working here — clippy's dead-code lint caught it, but a test that silently stops running is the same class of defect #462 was about.Docs
ADR-0056, and
docs/STORE.md§6's note is amended to scope its claim to determinations rather than markers — a NORMATIVE change, hence the ADR (ADR-0014).Nothing BiblioFetch.jl reads changes shape: no field added, removed or renamed,
schema_versionunmoved, reserved top-level fields keep themerge_opt!behaviour they had.Verified locally
clippy -D warningsclean atoa-only,citation;doiget-coresuite green (482 tests).