🐛 Request DOI metadata as CSL-JSON after BibTeX #1073

agoose77 · 2024-04-08T14:51:05Z

The bug in #1072 appears to be caused by the key passed to citation-js:

@article{EFSA Panel on Nutrition, Novel Foods and Food Allergens (NDA)_Turck_Castenmiller_de Henauw_Hirsch‐Ernst_Kearney_Knutsen_Maciuk_Mangelsdorf_McArdle_et al._2019,
	title        = {Dietary reference values for chloride},
	author       = {EFSA Panel on Nutrition, Novel Foods and Food Allergens (NDA) and Turck, Dominique and Castenmiller, Jacqueline and de Henauw, Stefaan and Hirsch‐Ernst, Karen‐Ildico and Kearney, John and Knutsen, Helle Katrine and Maciuk, Alexandre and Mangelsdorf, Inge and McArdle, Harry J and Pelaez, Carmen and Pentieva, Kristina and Siani, Alfonso and Thies, Frank and Tsabouri, Sophia and Vinceti, Marco and Aggett, Peter and Fairweather‐Tait, Susan and Martin, Ambroise and Przyrembel, Hildegard and de Sesmaisons‐Lecarré, Agnès and Naska, Androniki},
	year         = 2019,
	month        = {Sep},
	journal      = {EFSA Journal},
	volume       = 17,
	number       = 9,
	doi          = {10.2903/j.efsa.2019.5779},
	issn         = {18314732, 18314732},
	url          = {https://data.europa.eu/doi/10.2903/j.efsa.2019.5779},
	place        = {IT},
	abstractnote = {This publication is linked to the following EFSA Supporting Publications articles: http://onlinelibrary.wiley.com/doi/10.2903/sp.efsa.2019.EN-1679/full, http://onlinelibrary.wiley.com/doi/10.2903/sp.efsa.2017.e15121/full This publication is linked to the following EFSA Journal article: http://onlinelibrary.wiley.com/doi/10.2903/j.efsa.2019.5778/full}
}

This is not valid BibTeX, and therefore does not parse. One solution is to request CSL-JSON via content negotiation. This trades one class of bugs (invalid BibTeX markup) for another (invalid CSL structures). I think this is a better part of the problem space to find oneself in; we can more easily attempt to fix-up bad CSL than we can bad BibTeX.

Indeed, it looks like using CSL-JSON from content-negotiation isn't foolproof either; some DOIs like The Higgs Boson Paper report a name instead of a literal, or define a type of journal-article instead of article-journal. It looks like crossref is returning "extended citeproc": CrossRef/rest-api-doc#580 rather than true CSL-JSON.

Knowing that, we can probably do a reasonable job of coercing the result to the CSL-JSON that citation-js expects, but for now this PR does the minimum from testing the original DOI and the Higg's DOI.

I'm hoping that this change is more robust than the BibTeX approach, but we will need to try more DOIs than I've thrown at it.

Fixes #1072

rowanc1 · 2024-04-08T15:59:20Z

I don't have any data on which format is going to be better supported by cross-ref. I am pretty sure that bibtex has been around longer? My hesitation on bringing this on in the current state (or my understanding of it at least) is that it is really hard to know what our success rate is for the DOI parsing without this in the wild - I would rather an approach that improves/iterates the existing bibtex implementation rather than fully changes the implementation to using csl.

What about keeping the existing bibtex default and adding CSL content negotiation/parsing as a fallback if the bibtex fails to parse or is unavailable from doi.org? That should catch the case referenced and be a bit safer to roll out.

agoose77 · 2024-04-09T12:03:55Z

Good point! I'll rebase on main with the recommended changes.

packages/citation-js-utils/src/index.ts

agoose77 · 2024-04-09T14:21:53Z

packages/citation-js-utils/src/index.ts

+}
+
+export async function parseCSLJSON(source: object[]): Promise<CSL[]> {
+  return Promise.resolve(cleanCSL(source));


This doesn't validate the CSL, but does try to "clean" it. Should we also use Cite.async here too?

So - in parseBibTeX, Cite.async gives us bibtex ->CSL, but here, it would just give us CSL -> CSL so maybe unnecessary and just "cleaning" is all that's required?

If all that is true, then I guess the only reason to use Cite.async is if doi.org gives an invalid CSL response...? Not totally out of the question I suppose... but then Cite.async is another mostly unnecessary API request?

Any other nuance to this decision?

agoose77 · 2024-04-09T14:22:21Z

packages/citation-js-utils/src/index.ts

+  return Promise.resolve(cleanCSL(source));
+}
+
+export async function getCitationRenderers(data: CSL[]): Promise<CitationRenderer> {


Now this function creates renderers for CSL (i.e. offloads parsing)

agoose77 · 2024-04-09T14:23:12Z

packages/myst-cli/src/transforms/doi.spec.ts

+  {
+    'container-title': 'Monthly Weather Review',
+    author: [
+      { given: 'C. H. B.', family: 'PRIESTLEY' },
+      { given: 'R. J.', family: 'TAYLOR' },
+    ],
+    DOI: '10.1175/1520-0493(1972)100<0081:otaosh>2.3.co;2',
+    type: 'article-journal',
+    issue: '2',
+    issued: { 'date-parts': [[1972, 2]] },
+    page: '81-92',
+    publisher: 'American Meteorological Society',
+    title: 'On the Assessment of Surface Heat Flux and Evaporation Using Large-Scale Parameters',
+    volume: '100',
+  },
+];


I simplified this to the subset of equal-fields common to both resolvers. I think that's OK; things like the URL and ISBN slightly differ, but I think that's a consequence of the different metadata that's registered.

packages/myst-cli/src/transforms/dois.ts

rowanc1 · 2024-04-09T23:51:52Z

Works well in web mode, cache seems good and doesn't conflict. From my testing there is a current bug in writing out the bibtex: writeBibtexFromCitationRenderers errors out when there is a CSL input here.

I think this could/should be isolated into a test around falling back to CSL and then writing bibtex for the DOI of 10.2903/j.efsa.2019.5779.

fwkoch

This is looking great - keeping bibtex as the default but adding a CSL fallback, explicitly making CSL the internal representation.

I don't have any big concerns with the changes to exported functions - there are a couple dependent packages that will need to update, but giving citation-js-utils a major bump makes that pretty clear.

I do think we need to fix the writeBibtexFromCitationRenderers functionality before releasing this - Probably for now just a simple function we can call if the bibtex entry is not found buried in the cite._graph - just take the CSL and return a bibtex string with title/author/year/journal/doi filled in.

packages/myst-cli/src/transforms/dois.ts

packages/citation-js-utils/src/index.ts

fwkoch · 2024-04-10T06:18:48Z

packages/citation-js-utils/src/index.ts

+}
+
+export async function parseCSLJSON(source: object[]): Promise<CSL[]> {
+  return Promise.resolve(cleanCSL(source));


So - in parseBibTeX, Cite.async gives us bibtex ->CSL, but here, it would just give us CSL -> CSL so maybe unnecessary and just "cleaning" is all that's required?

If all that is true, then I guess the only reason to use Cite.async is if doi.org gives an invalid CSL response...? Not totally out of the question I suppose... but then Cite.async is another mostly unnecessary API request?

Any other nuance to this decision?

packages/citation-js-utils/types/citation-js/index.d.ts

packages/myst-cli/src/transforms/dois.ts

agoose77 · 2024-04-10T11:24:41Z

Good spot on the BibTeX extraction; I'd assumed that we were using citation-js for this and hadn't noticed that instead we were pulling from the graph.

This PR now uses citation-js to build a BibTeX output. I'm assuming that we don't need to do anything to tweak the citation key, as it should match the id that citation-js reports (I think). We might need to be careful to ensure this.

In general, I think this PR represents a hardening of our BibTeX handling in addition to now providing a fallback for bad-BibTeX responses; we don't rely on implementation details from citation-js to store the original BibTeX, for example.

There may be some non-round-tripping of certain BibTeX entries, but I think that should be very uncommon; BibTeX is a core plugin for citation-js.

packages/citation-js-utils/src/index.ts

agoose77 · 2024-04-10T15:55:26Z

@fwkoch

So - in parseBibTeX, Cite.async gives us bibtex ->CSL, but here, it would just give us CSL -> CSL so maybe unnecessary and just "cleaning" is all that's required?

If all that is true, then I guess the only reason to use Cite.async is if doi.org gives an invalid CSL response...? Not totally out of the question I suppose... but then Cite.async is another mostly unnecessary API request?

Any other nuance to this decision?

Yes, even the BibTeX method doesn't need to be async. I suppose I was anticipating that we might want to support async resolvers in future. I'll move them to sync for now.

packages/myst-cli/src/transforms/dois.ts

fwkoch

Thanks for addressing the bibtex writing - using citation-js for that is much better. 🙂

I noticed a couple more things when testing today:

First, I don't think this warning needs to be raised:

I don't think the user will care about implementation details of the underlying citation data format?

Also, totally non-existent citations were still causing a crash:

This is related to return undefined vs throwing an error

I went ahead and fixed these up in a commit: 559cbef

Hopefully that all looks ok? Otherwise, I'm happy with everything.

Co-authored-by: Franklin Koch <[email protected]>

Co-authored-by: Rowan Cockett <[email protected]>

fwkoch · 2024-04-10T23:16:10Z

One more bug I found on tex exports:

cite nodes resolved from CSL-JSON were not getting a label. Fixed here: ba9d0b9

agoose77 changed the title ~~wip: fix up citations~~ 🐛 Request DOI metadata as CSL-JSON Apr 8, 2024

agoose77 force-pushed the agoose77/fix-citation-parsing branch from 62476ac to 23adc24 Compare April 8, 2024 14:53

rowanc1 mentioned this pull request Apr 8, 2024

🐛 Do not fail on citation.js #1076

Merged

agoose77 force-pushed the agoose77/fix-citation-parsing branch from 23adc24 to 75b8a6a Compare April 9, 2024 12:04

agoose77 commented Apr 9, 2024

View reviewed changes

packages/citation-js-utils/src/index.ts Show resolved Hide resolved

agoose77 marked this pull request as ready for review April 9, 2024 14:14

agoose77 changed the title ~~🐛 Request DOI metadata as CSL-JSON~~ 🐛 Request DOI metadata as CSL-JSON after BibTeX Apr 9, 2024

agoose77 commented Apr 9, 2024

View reviewed changes

packages/myst-cli/src/transforms/dois.ts Show resolved Hide resolved

agoose77 commented Apr 9, 2024

View reviewed changes

packages/myst-cli/src/transforms/dois.ts Outdated Show resolved Hide resolved

fwkoch requested changes Apr 10, 2024

View reviewed changes

agoose77 force-pushed the agoose77/fix-citation-parsing branch from 6f0751d to 68538b8 Compare April 10, 2024 09:34

agoose77 closed this Apr 10, 2024

agoose77 reopened this Apr 10, 2024

rowanc1 reviewed Apr 10, 2024

View reviewed changes

packages/citation-js-utils/src/index.ts Show resolved Hide resolved

rowanc1 reviewed Apr 10, 2024

View reviewed changes

packages/myst-cli/src/transforms/dois.ts Outdated Show resolved Hide resolved

fwkoch approved these changes Apr 10, 2024

View reviewed changes

agoose77 added 6 commits April 10, 2024 16:13

feat: support CSL-JSON from doi.org too

30c17c0

chore: appease linter

0cfc146

test: fix test suite

15e4231

chore: changeset

f7055b0

fix: run built-in cleanup

86ade98

refactor: rename DOI function

61cd469

agoose77 and others added 15 commits April 10, 2024 16:13

Update packages/myst-cli/src/transforms/dois.ts

d62481d

Update packages/myst-cli/src/transforms/dois.ts

da1a9d3

Co-authored-by: Franklin Koch <[email protected]>

chore: add docstrings

6f5f47f

fix: export BibTeX using citation-js

dbb4bc9

chore: run formatter

08e3c9f

chore: remove unused file

1b62ec5

chore: add shim

76afc45

feat: reduce changeset bump

3217a65

refactor: drop async use

b6e3416

chore: run prettier

ddb095b

🔧 Handle totally non-existent dois

a002921

🔧 Build url from doiString not normaliedDoi in error message

0a9a487

Co-authored-by: Rowan Cockett <[email protected]>

🐛 Ensure citation nodes resolved from CSL-JSON have label

ba9d0b9

🧪 Add end-to-end tests for various DOIs

36785fa

🧪 Run end-to-end tests concurrently but with longer timeout

cec2d31

fwkoch force-pushed the agoose77/fix-citation-parsing branch from dd9db8e to cec2d31 Compare April 10, 2024 23:14

fwkoch merged commit 5c9338a into main Apr 10, 2024
4 checks passed

fwkoch deleted the agoose77/fix-citation-parsing branch April 10, 2024 23:24

rowanc1 mentioned this pull request Apr 11, 2024

Unreliable DOI resolution for some DOIs #947

Closed

agoose77 self-assigned this Apr 11, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

🐛 Request DOI metadata as CSL-JSON after BibTeX #1073

🐛 Request DOI metadata as CSL-JSON after BibTeX #1073

agoose77 commented Apr 8, 2024 •

edited

Loading

rowanc1 commented Apr 8, 2024

agoose77 commented Apr 9, 2024

agoose77 Apr 9, 2024

fwkoch Apr 10, 2024

agoose77 Apr 9, 2024

agoose77 Apr 9, 2024 •

edited

Loading

rowanc1 commented Apr 9, 2024

fwkoch left a comment

fwkoch Apr 10, 2024

agoose77 commented Apr 10, 2024

agoose77 commented Apr 10, 2024

fwkoch left a comment

fwkoch commented Apr 10, 2024

🐛 Request DOI metadata as CSL-JSON after BibTeX #1073

🐛 Request DOI metadata as CSL-JSON after BibTeX #1073

Conversation

agoose77 commented Apr 8, 2024 • edited Loading

rowanc1 commented Apr 8, 2024

agoose77 commented Apr 9, 2024

agoose77 Apr 9, 2024

Choose a reason for hiding this comment

fwkoch Apr 10, 2024

Choose a reason for hiding this comment

agoose77 Apr 9, 2024

Choose a reason for hiding this comment

agoose77 Apr 9, 2024 • edited Loading

Choose a reason for hiding this comment

rowanc1 commented Apr 9, 2024

fwkoch left a comment

Choose a reason for hiding this comment

fwkoch Apr 10, 2024

Choose a reason for hiding this comment

agoose77 commented Apr 10, 2024

agoose77 commented Apr 10, 2024

fwkoch left a comment

Choose a reason for hiding this comment

fwkoch commented Apr 10, 2024

agoose77 commented Apr 8, 2024 •

edited

Loading

agoose77 Apr 9, 2024 •

edited

Loading