diff --git a/PLAN-fasta-header-formats.md b/PLAN-fasta-header-formats.md new file mode 100644 index 000000000..b85b17585 --- /dev/null +++ b/PLAN-fasta-header-formats.md @@ -0,0 +1,338 @@ +# PLAN — FASTA header: delimiters, format strings, and saved named formats + +**Branch:** `feature/fasta-header-formats` (from `alpha/alpha` @ `54b459f5e`) +**Scope:** `widget/download/FASTAConfiguratorStep.js`, its template, `util/DownloadExecutor.js`, +and a new `util/FastaFormatStore.js`. Client-side only; the API work is already done. + +--- + +## What the API now offers + +Three capabilities, two of which the client does not use at all: + +| parameter | status on alpha | client sends it? | +|---|---|---| +| `http_fasta_id_fields` | live | yes | +| `http_fasta_id_delimiter` (default `\|`) | live | **no** | +| `http_fasta_description_fields` | live | yes | +| `http_fasta_description_delimiter` | **live** (new) | **no** | +| `http_fasta_context_fields` | live | no | +| `http_fasta_context_delimiter` (default `' \| '`) | live | **no** | +| `http_fasta_header` (format string) | live | **no** | + +Verified against `genome_feature`, genome `1545742.911`, after the 2026-10-30 alpha restart: + +``` +&http_fasta_description_delimiter=%20%7C%20 + -> >fig|1545742.911.peg.9| MGYG-HGUT-00869 | Human | USA [...] + +&http_fasta_header={patric_id}|{genome_id} {product} [{host_name}] + -> >fig|1545742.911.peg.9|1545742.911 hypothetical protein [Human] +``` + +### Format-string contract (from `util/fastaHeaderFormatter.js:243-333`) + +Four decisions that drive the UI design, not implementation trivia: + +1. **Missing values render empty**, and the template is not rewritten around them. The zone + scheme collapses the delimiter with an absent field; a template leaves a visible gap. +2. **Exclusive.** When `http_fasta_header` is present the zone parameters are *ignored*, not + merged. This is why the UI must switch modes rather than offer both. +3. `{{` and `}}` are literal braces. +4. **An unknown field is a 400**, not an empty string — validated before the response starts. + So the client can and should validate against `/fields` first. + +The leading `>` is added by the server; the template supplies everything after it. + +### One deployed behaviour still diverges from that contract + +**An unknown field renders empty rather than 400** — contract decision 4 does not hold. +Re-verified 2026-10-30 after the field update: + +``` +http_fasta_header={accession} {hostname} -> HTTP 200, >CY086966 +``` + +That is the realistic case: `{hostname}` instead of `{host_name}`. The user gets a silently +truncated header — exactly the failure mode decision 4 was written to prevent ("how the +original enrichment bug hid for weeks"). *Syntax* validation does fire correctly: + +``` +{patric_id -> 400 "unbalanced braces (use {{ and }} for literal braces)" +{patric_id}{} -> 400 "contains an empty field reference {}" +``` + +So the validator runs; the field-name check against the collection registry is what is +missing. **Client-side validation against `/fields` is therefore required, not just nice** — +it is currently the only thing standing between a typo and a bad download. Worth reporting +upstream regardless, since a direct API caller gets no protection from our validation. + +*(A second divergence — `{{`/`}}` dropping their contents — was fixed in the same update. +`A{{x}}B` now renders `>A{x}B`, so brace escaping can be documented in the UI.)* + +--- + +## The bug that prompted this + +The preview promised `|`-separated description fields and the download delivered spaces: + +```javascript +// FASTAConfiguratorStep.js:358 — preview +previewParts.push(descFields.join(' | ')); + +// p3_api util/fastaHeaderFormatter.js:268 — server, before the fix +const description = formatFields(doc, config.descriptionFields, ' ') +``` + +The server now accepts a delimiter. **The client must send one, or the preview stays a lie** — +the default remains `' '` for backward compatibility. This is the smallest shippable piece of +this plan and should land first. + +--- + +## Design + +### Two modes, exclusive, mirroring the API + +``` +( ) Build from fields -> zone parameters +(•) Custom format string -> http_fasta_header +``` + +Selecting *Custom format* disables the field pickers rather than hiding them, so the user can +see what they are leaving behind, and switching back restores the previous zone config. + +**Seed the template from the current zone selection** when the user first switches. Someone +who has picked ID=`patric_id`, description=`strain, host_name` gets +`{patric_id} {strain} | {host_name}` to edit rather than a blank box. That turns the template +from a separate feature into a power-user escape hatch from the picker — and it is the single +most useful thing we can do to make the format string approachable. + +### Delimiters + +Three inputs, in the respective sections: ID, description, context. Short text fields, not +dropdowns — the useful values (`|`, ` | `, `_`, `;`, tab) do not enumerate well, and the API +takes an arbitrary string. + +Leading/trailing spaces are significant and users cannot see them. Render the current value +with visible markers in the preview legend (e.g. `"·|·"`) or show the delimiter inside the +live preview — the preview already updates on change, which handles most of it. + +### Preview + +The preview must be generated **from the same values the request will carry**. It is currently +a hand-built string that happens to disagree with the server; that is the whole bug. Build it +by the same rules in both modes: + +- *zones*: `>` + id fields joined by idDelimiter + trailing idDelimiter + ` ` + + description fields joined by descriptionDelimiter + ` [` + context + `]` + (note the server appends a trailing ID delimiter — `fastaHeaderFormatter.js:262`) +- *template*: substitute field names with example values; leave unknown fields visibly marked + +`util/FASTADefLineFields.js` carries per-field `example` values and is **currently dead code** +(its only consumer, `widget/download/DefinitionLineBuilder.js`, is never instantiated). Reuse +those examples for the preview, then delete the rest of the module — or fold the examples into +whatever replaces it. Do not leave two field registries behind. + +### Validation + +- Template: balanced braces, no `{}`, non-empty — mirror `validateTemplate()`. The server does + enforce these, so this is only to fail faster. +- Field names: check each `{name}` against `//fields`. **This is load-bearing, not + a nicety** — per divergence (a) above, the deployed server renders an unknown field as empty + and returns 200, so a typo'd `{hostname}` produces a silently truncated header with no error + anywhere. Suggest a correction ("did you mean `host_name`?") from the `/fields` list. +- Name on save: non-empty, and warn before overwriting an existing name. + +--- + +## Saved named formats + +**Decision: one preferences file, matching `util/FavoriteFolders.js`.** + +``` +//home/.preferences/fasta-formats.json +``` + +```json +{ + "formats": [ + { + "name": "NCBI style", + "dataType": "genome_feature", + "mode": "template", + "template": "{patric_id}|{genome_id} {product} [{genome_name}]", + "created": "2026-10-30T12:00:00Z" + }, + { + "name": "Host + country", + "dataType": "genome_feature", + "mode": "zones", + "idFields": ["patric_id"], + "idDelimiter": "|", + "descriptionFields": ["strain", "host_name"], + "descriptionDelimiter": " | " + } + ] +} +``` + +New module `util/FastaFormatStore.js`, lifting the proven parts of `FavoriteFolders`: + +- `_ensurePreferencesDir()` — `getObject(dir, true)`, create on failure +- read-modify-write with a **mod-time merge**, so a save from a second tab is not clobbered + (`FavoriteFolders` compares `result.metadata.creation_time`) +- in-memory cache keyed by user id, invalidated on logout +- `WorkspaceManager.saveFile(path, content, 'json')` + +`list()` / `save(format)` / `remove(name)` / `load(name)`, all returning Deferreds. + +**Filter the list by `dataType`.** A `genome_feature` template referencing `{product}` is +meaningless on a `genome_sequence` download and would 400. Show matching formats; keep the +others but mark them unavailable rather than hiding them, so a user does not think their saved +format vanished. + +### Logged out + +**Decision: hide the save/load section entirely.** Delimiters and templates still work for +everyone — only persistence requires an account. `FavoriteFolders._getFilePath()` returns null +with no `window.App.user.id`, so the guard is the same one-liner. No dead buttons. + +--- + +## Work items + +Ordered so each step is shippable on its own. + +### 1. Send the delimiters — fixes the reported bug + +`DownloadExecutor.js:223` and `:380` build the FASTA params. Add the three delimiters, defaulted +to the server's own defaults so nothing changes for a user who does not touch them. Fix the +preview (`FASTAConfiguratorStep.js:358`) to use the actual description delimiter. + +Small, no API dependency, resolves the complaint. **Land this first.** + +### 2. Delimiter controls in the UI + +Three text inputs + preview wiring. Still no API dependency. + +### 3. Format-string mode + +`http_fasta_header` is live on alpha, so this is unblocked. Radio switch, textarea, +field-reference helper (clicking a field in the picker inserts `{name}` at the cursor), +client-side validation against `/fields`, seed-from-zones on first switch. + +### 4. `util/FastaFormatStore.js` + save/load UI + +Independent of item 3 — it can store zone configs before templates exist. Build it after 2, +ship with whichever of 2/3 is ready. + +### 5. Replace the hardcoded field list with `/fields` + +Out of scope for the reported bug but adjacent: `_getDefaultFieldsForDataType()` +(`FASTAConfiguratorStep.js:92-133`) hardcodes fields that already drifted from the server +twice. `//fields` now reports `source: native|enriched` with labels. + +**One caution:** `/fields` **404s on production** — needs a fallback to the current hardcoded +list, or must ship in lockstep with the API release. + +The `genome_sequence` concern is resolved: that serializer now enriches (verified with two +enriched-only fields, `{subtype}` and `{host_common_name}` → `>CY086966|3|N1|Human`), so +consuming `/fields` directly is safe for every FASTA data type. + +--- + +## Testing + +- [ ] Default (no delimiters touched) produces **byte-identical** output to before item 1 +- [ ] Each delimiter independently; `|`, ` | `, `_`, empty string, a literal space +- [ ] Preview matches the downloaded header **exactly**, both modes — this is the regression + that started this work +- [ ] Missing field value: zones collapse the delimiter, template leaves a gap (per contract) +- [ ] Template with an unknown field is caught client-side — the server returns 200 with an + empty substitution, so nothing else will catch it +- [ ] `{{literal}}` braces survive round-trip +- [ ] Each built-in preset renders exactly the header recorded in `REPORT-fasta-header-presets.md` +- [ ] Flu B preset on a record with empty `strain` — leading empty field is visible, not silent +- [ ] `{patric_id}` in a `|`-delimited template triggers the pipe warning +- [ ] Save, reload the page, load the format back +- [ ] Two tabs saving different formats — neither is lost (mod-time merge) +- [ ] Logged out: delimiters/templates work, save UI absent, no console error +- [ ] Format saved for `genome_feature` is marked unavailable on a `genome_sequence` download +- [ ] First-ever save creates `.preferences/` when it does not exist +- [ ] Both FASTA data types (protein and DNA) and both grid sources + +--- + +## Built-in format presets + +**All verified against live alpha** — see [`REPORT-fasta-header-presets.md`](REPORT-fasta-header-presets.md) +for the full output, field reference and reproduction commands. The API update of 2026-10-30 +added the seven virology fields (`genome_feature` enrichment 35 → 42), so every preset below +works today. + +Ship them **per data type**, since `{product}` is meaningless on `genome_sequence`: + +| # | preset | data type | format string | +|---|---|---|---| +| 1 | Classic NCBI / BLAST | genome_feature | `{patric_id} {product} [{genome_name}]` | +| 2 | NCBI source modifiers | genome_feature | `{patric_id} [organism={genome_name}] [strain={strain}] [host={host_name}] [country={isolation_country}] [collection_date={collection_date}]` | +| 3 | NCBI source modifiers, influenza | genome_feature | `{accession} [organism={genome_name}] [strain={strain}] [serotype={subtype}] [segment={segment}] [host={host_name}] [country={isolation_country}] [collection_date={collection_date}]` | +| 4 | GISAID / augur, influenza A | genome_feature | `{strain}\|A\|{subtype}\|{collection_date}\|\|{isolation_country}\|{host_common_name}\|{accession}` | +| 5 | GISAID / augur, influenza B | genome_feature | `{strain}\|B\|{lineage}\|{collection_date}\|\|{isolation_country}\|{host_common_name}\|{accession}` | +| 6 | GISAID-style, sequence level | genome_sequence | `{accession}\|{segment}\|{strain}\|{collection_date}\|{isolation_country}\|{host_common_name}` | +| 7 | Bacterial, metadata-rich | genome_feature | `{patric_id} {product} [{genome_name}] [{strain}] [{host_name}] [{isolation_country}]` | + +Real output for #4 and #1: + +``` +>A/New York/3197/2009|A|N1|2009-04-29||USA|Human|CY086966 +>fig|83332.12.peg.1000 Beta-carotene ketolase [Mycobacterium tuberculosis H37Rv] +``` + +GISAID field order is pinned by `augur parse` +(https://docs.nextstrain.org/projects/augur/en/stable/usage/cli/parse.html): +`strain type subtype date season country host accession`, separator `|`. +NCBI's defline rules are at https://www.ncbi.nlm.nih.gov/genbank/fastaformat/. + +### Four data facts that constrain preset design + +1. **`subtype` is not always a subtype.** Influenza B records carry the literal string + `Influenza B`, with the discriminator in `lineage` (`Yamagata`/`Victoria`). Hence separate + A and B presets, and `type` hardcoded in each — BV-BRC has no type field distinct from + subtype. **Presets are per-organism, not one generic "GISAID" entry.** +2. **`strain` can be empty** (4 of a 200-genome influenza sample) even when `genome_name` + carries the name, and it *leads* the GISAID format: `>|B|Yamagata|2015-06-12|...`. A + leading empty field is worse for a positional parser than a missing record. +3. **An absent value yields a malformed NCBI modifier**: `[collection_date=]`. Per contract + decision 1 this is intended, but it means preset #2/#3 must be labelled **"NCBI-shaped, + not submission-ready"**. Zone mode does not have this problem — it skips absent fields. +4. **`{patric_id}` contains a pipe** (`fig|646741.3.CDS.1`), which hands `augur parse` an + extra field. Flu presets use `{accession}`. **Warn when `{patric_id}` appears in a template + whose delimiter is `|`.** + +Built-ins are read-only entries in the same picker as saved formats, with "duplicate and +edit" — the cheapest way to make the template syntax discoverable. + +## Open questions + +1. **Should a saved format be shareable?** The preferences file is private by design. If + sharing is wanted, add an explicit "Export…" writing a single-format file (the + `SavedSearchManager.exportToWorkspace` pattern) rather than moving the primary store. +2. **Should the NCBI modifier presets enforce their own rules, or just warn?** NCBI requires a + SeqID ≤25 chars from a restricted charset, and does not define an empty modifier value — + we produce `[collection_date=]`. Current plan is to label them "NCBI-shaped"; enforcing + would mean rejecting templates rather than describing them. +3. **Report the unknown-field divergence upstream?** It undercuts a decision the API authors + made deliberately, and our validation only protects users coming through the wizard — + a direct API caller still gets a silent empty substitution. +4. **`species` returns the ICTV name** (`Alphainfluenzavirus influenzae`), not + `Influenza A virus`. Correct, but surprising inside `[organism=…]`; `{genome_name}` is + closer to submitter expectation. Decide before preset #3 ships. + +### Resolved since first draft + +- ~~Ship built-in named formats?~~ Yes — seven, all verified. See above. +- ~~Add the virology fields to `GENOME_METADATA_FIELDS`?~~ Done, 2026-10-30. +- ~~`genome_sequence` FASTA enrichment?~~ Done — that serializer now joins. +- ~~`{{`/`}}` escaping broken?~~ Fixed in the same update. diff --git a/REPORT-fasta-header-presets.md b/REPORT-fasta-header-presets.md new file mode 100644 index 000000000..7fba3a850 --- /dev/null +++ b/REPORT-fasta-header-presets.md @@ -0,0 +1,291 @@ +# FASTA header format strings — verified against alpha + +**Date:** 2026-10-30, after the API update adding virology fields to the enrichment set. +**Every header below is real output** from `https://alpha.bv-brc.org/api/`, not illustrative. + +Reproduce any row with: + +```bash +curl -s -X POST "https://alpha.bv-brc.org/api/genome_feature/?http_download=true\ +&http_accept=application/dna+fasta&http_fasta_header=" \ + -d 'and(eq(genome_id,646741.3),eq(feature_type,CDS))&limit(1)&sort(+feature_id)' | head -1 +``` + +Test records used: + +| genome_id | what it is | +|---|---| +| `646741.3` | *Influenza A virus (A/New York/3197/2009(N1))*, segment 3, USA, 2009-04-29 | +| `408922.4` | *Influenza B*, B/Houston/B86/2003, Victoria lineage | +| `1727158.4` | *Influenza B*, B/California/NHRC_27301/2015 — **`strain` is empty** | +| `1071165.3` | H5N1 reassortant, `h5_clade` = 2.2.1 | +| `83332.12` | *M. tuberculosis* H37Rv | +| `1545742.911` | *uncultured Christensenella* sp., host/country populated, no collection_date | + +--- + +## What the API update delivered + +`genome_feature` enrichment went **35 → 42 fields**. All seven requested resolve: + +``` +subtype segment lineage clade h5_clade genbank_accessions species all "enriched" +``` + +``` +select(feature_id,subtype,segment,species,genbank_accessions) + -> {"species":"Alphainfluenzavirus influenzae","segment":["3"], + "subtype":"N1","genbank_accessions":"CY086966"} +``` + +Consistent across collections — 42 enriched on `genome_feature`, `sp_gene`, `genome_amr`, +`pathway`, `subsystem`; 41 on `genome_sequence` (it has native `segment`, so no enriched copy). + +Brace escaping is also fixed: `A{{x}}B` now renders `>A{x}B` (previously `>A{}B`). + +--- + +## Presets — format string and real output + +### 1. Classic NCBI / BLAST — `genome_feature` + +``` +{patric_id} {product} [{genome_name}] +``` + +``` +>fig|646741.3.CDS.1 putative polymerase PA, fragment [Influenza A virus (A/New York/3197/2009(N1))] +>fig|83332.12.peg.1000 Beta-carotene ketolase [Mycobacterium tuberculosis H37Rv] +``` + +What most tools expect from a downloaded protein FASTA. Closest to today's default. + +### 2. NCBI source modifiers — `genome_feature` + +Per https://www.ncbi.nlm.nih.gov/genbank/fastaformat/ : `>SeqID [modifier=value]`, no spaces +around `=`, organism required. + +``` +{patric_id} [organism={genome_name}] [strain={strain}] [host={host_name}] [country={isolation_country}] [collection_date={collection_date}] +``` + +``` +>fig|1545742.911.peg.9 [organism=uncultured Christensenella sp. MGYG-HGUT-00869] [strain=MGYG-HGUT-00869] [host=Human] [country=USA] [collection_date=] +``` + +⚠ Note `[collection_date=]` — see *Empty values* below. + +### 3. NCBI source modifiers, influenza — `genome_feature` + +``` +{accession} [organism={genome_name}] [strain={strain}] [serotype={subtype}] [segment={segment}] [host={host_name}] [country={isolation_country}] [collection_date={collection_date}] +``` + +``` +>CY086966 [organism=Influenza A virus (A/New York/3197/2009(N1))] [strain=A/New York/3197/2009] [serotype=N1] [segment=3] [host=Homo sapiens] [country=USA] [collection_date=2009-04-29] +``` + +Uses `{accession}` rather than `{patric_id}` for a clean, pipe-free SeqID. + +### 4. GISAID / augur, influenza A — `genome_feature` + +Field order from `augur parse` +(https://docs.nextstrain.org/projects/augur/en/stable/usage/cli/parse.html): +`strain type subtype date season country host accession`, separator `|`. + +``` +{strain}|A|{subtype}|{collection_date}||{isolation_country}|{host_common_name}|{accession} +``` + +``` +>A/New York/3197/2009|A|N1|2009-04-29||USA|Human|CY086966 +``` + +Parses cleanly with `augur parse --fields strain type subtype date season country host accession`. + +### 5. GISAID / augur, influenza B — `genome_feature` + +`subtype` holds the literal `Influenza B` for type-B records, so the B preset reads `{lineage}` +in that position instead. + +``` +{strain}|B|{lineage}|{collection_date}||{isolation_country}|{host_common_name}|{accession} +``` + +``` +>B/Houston/B86/2003|B|Victoria|2003-01-21||USA|Human|CY018277 +>B/Houston/B79/2003|B|Victoria|2003||USA|Human|CY018253 +``` + +### 6. GISAID-style, sequence level — `genome_sequence` + +``` +{accession}|{segment}|{strain}|{collection_date}|{isolation_country}|{host_common_name} +``` + +``` +>CY086966|3|A/New York/3197/2009|2009-04-29|USA|Human +``` + +### 7. H5 clade — `genome_feature` + +``` +{strain}|{subtype}|clade={h5_clade}|{segment} +``` + +``` +>A/reassortant/IDCDC_RG29(Egypt/N03072/2010 x Puerto Rico/8/1934)|H5N1|clade=2.2.1|4 +``` + +### 8. Bacterial, metadata-rich — `genome_feature` + +``` +{patric_id} {product} [{genome_name}] [{strain}] [{host_name}] [{isolation_country}] +``` + +``` +>fig|1545742.911.peg.9 hypothetical protein [uncultured Christensenella sp. MGYG-HGUT-00869] [MGYG-HGUT-00869] [Human] [USA] +``` + +### 9. Zone mode for comparison (no template) + +The same content without a format string, using the new description delimiter: + +``` +http_fasta_id_fields=accession +http_fasta_description_fields=strain,subtype,host_common_name +http_fasta_description_delimiter=" | " +``` + +``` +>CY086966| A/New York/3197/2009 | N1 | Human [Influenza A virus (A/New York/3197/2009(N1)) | 646741.3] +``` + +Shows why templates matter: zone mode forces the trailing `|` after the ID and the bracketed +context block. Neither is removable. + +--- + +## Field reference + +Values observed on the test records. + +| `{field}` | Source | Example | Notes | +|---|---|---|---| +| `{patric_id}` | native | `fig\|646741.3.CDS.1` | **contains `\|`** — avoid in pipe-delimited formats | +| `{feature_id}` | native | `RefSeq.646741.3.CY086966.CDS.1.1108.fwd` | >25 chars; not NCBI SeqID-safe | +| `{accession}` | native | `CY086966` | clean ID for flu presets | +| `{product}` | native | `putative polymerase PA, fragment` | contains commas | +| `{genome_name}` | native | `Influenza A virus (A/New York/3197/2009(N1))` | parens + spaces | +| `{genome_id}` | native | `646741.3` | | +| `{taxon_id}` | native | `646741` | | +| `{go}` | native, multi | `GO:0005215\|transporter activity,GO:0006810\|transport` | array → `,` joined, **no space**; values contain `\|` | +| `{segments}` | native | `1..1108` | ⚠ **coordinate spans, not flu segments** | +| `{strain}` | enriched | `A/New York/3197/2009` | **may be `""`** — see below | +| `{host_name}` | enriched | `Homo sapiens` | also seen as `Human, Homo sapiens` (comma) | +| `{host_common_name}` | enriched | `Human` | matches GISAID convention | +| `{host_group}` | enriched | `Human` | controlled vocabulary | +| `{isolation_country}` | enriched | `USA` | | +| `{geographic_group}` | enriched | `North America` | | +| `{collection_date}` | enriched | `2009-04-29` *or* `2003` | **variable precision** | +| `{collection_year}` | enriched | `2009` | fixed width | +| `{genome_status}` | enriched | `WGS` | | +| `{subtype}` | **new** | `N1`, `H5N1`, `Influenza B` | ⚠ not always a subtype | +| `{segment}` | **new**, multi | `3` | renders bare when single-valued | +| `{lineage}` | **new** | `Yamagata`, `Victoria` | flu B lineage | +| `{clade}` | **new** | — | sparse | +| `{h5_clade}` | **new**, multi | `2.2.1` | H5 only | +| `{genbank_accessions}` | **new** | `CY086966` | genome-level | +| `{species}` | **new** | `Alphainfluenzavirus influenzae` | ICTV name, not `Influenza A virus` | + +Multi-value rendering confirmed — arrays join with `,` and **no space**: + +``` +{patric_id} GO={go} + -> >fig|83332.12.peg.1032 GO=GO:0005215|transporter activity,GO:0006810|transport,GO:0016020|membrane +``` + +--- + +## Five findings that shape the UI + +### 1. An unknown field still renders empty with HTTP 200 + +Unchanged by this update, and the most important item here: + +``` +{accession} {hostname} -> >CY086966 HTTP 200 +``` + +A typo of `host_name` produces a plausible-looking truncated header and no error anywhere. +The documented contract (`util/fastaHeaderFormatter.js:267`, decision 4) says this should be +a 400. **Client-side validation against `/fields` is therefore load-bearing, not cosmetic.** +Syntax validation does work — unbalanced braces and `{}` both 400 correctly. + +### 2. `subtype` is not always a subtype + +Influenza B records carry the literal string `Influenza B`, with the real discriminator in +`lineage`: + +``` +1727158.4 -> subtype="Influenza B" lineage="Yamagata" +``` + +A single GISAID preset using `{subtype}` emits `|B|Influenza B|`. Hence separate A and B +presets. `type` is likewise hardcoded `A`/`B` in the template because BV-BRC has no type field +distinct from subtype — **presets must be per-organism, not one generic "GISAID" entry.** + +### 3. `strain` can be empty even when the name is known + +``` +1727158.4 strain="" genome_name="Influenza B virus (B/California/NHRC_27301/2015)" + -> >|B|Yamagata|2015-06-12||USA|Human|KT819406 + ^ leading empty field +``` + +4 of a 200-genome influenza sample. For a positional parser a leading empty field is worse +than a missing record. Worth considering whether the B preset should lead with `{accession}`, +or whether the UI should warn when a template *starts* with a frequently-empty field. + +### 4. Empty values produce malformed NCBI modifiers + +``` +>fig|1545742.911.peg.9 ... [collection_date=] +``` + +NCBI's format does not define an empty modifier value, and submission tooling may reject it. +Per contract decision 1 this is intended — a template cannot drop surrounding literals without +conditional syntax. But it means **the NCBI modifier preset must be labelled "NCBI-shaped, not +submission-ready."** Zone mode does not have this problem: it skips absent fields and collapses +the delimiter. + +### 5. `{patric_id}` carries a pipe + +`fig|646741.3.CDS.1` inside a `|`-delimited header hands augur an extra field. Flu presets use +`{accession}`. The UI should warn when `{patric_id}` appears in a template whose delimiter is +`|`. + +--- + +## Still open + +- **`species` is the ICTV name** (`Alphainfluenzavirus influenzae`), not `Influenza A virus`. + Correct, but surprising in an `[organism=…]` modifier; `{genome_name}` is closer to what + submitters expect. Worth a decision before the preset ships. +- **`/fields` 404s on production.** Any client that consumes it needs a fallback or must ship + with the API release. + +## Resolved by this update + +`genome_sequence` FASTA **now enriches**. It previously had no join at all +(`serializeGenomeSequenceStream`, `media/dna+fasta.js:379`, *"no join needed"*), so every +genome-level field in a sequence FASTA header came back empty. Verified with two +enriched-only fields — neither is native on `genome_sequence`: + +``` +{accession}|{segment}|{subtype}|{host_common_name} + -> >CY086966|3|N1|Human +``` + +This also removes the concern that the download wizard's `genome`/`genome_sequence` branch +was advertising fields that could never populate on that format. diff --git a/public/js/p3/util/DownloadExecutor.js b/public/js/p3/util/DownloadExecutor.js index a191d7f53..514f09b13 100644 --- a/public/js/p3/util/DownloadExecutor.js +++ b/public/js/p3/util/DownloadExecutor.js @@ -192,6 +192,45 @@ define([ return baseName + '_' + timestamp + '.' + extension; } + /** + * Build the http_fasta_* query parameters for a spec. + * + * Returns a string beginning with '&', or '' when the spec carries no FASTA + * configuration. Shared by the form-POST and xhr paths, which previously + * each had their own copy of this and so could drift. + * + * Delimiters are sent only when the configurator supplied them. The server + * defaults are '|' for the ID section and ' ' for the description + * (p3_api util/fastaHeaderFormatter.js), so an absent value means "leave the + * server's default alone" rather than "send empty" — note that an empty + * string is itself a meaningful delimiter and must still be transmitted, + * which is why these test for undefined rather than falsiness. + */ + function buildFastaParams(spec) { + if (!spec.fastaConfig) { + return ''; + } + + var cfg = spec.fastaConfig; + var idFields = cfg.idFields || []; + var descFields = cfg.descriptionFields || []; + + var params = '&http_fasta_id_fields=' + encodeURIComponent(idFields.join(',')); + params += '&http_fasta_id_prefix='; + params += '&http_fasta_description_fields=' + encodeURIComponent(descFields.join(',')); + params += '&http_fasta_context_fields='; + + if (cfg.idDelimiter !== undefined) { + params += '&http_fasta_id_delimiter=' + encodeURIComponent(cfg.idDelimiter); + } + if (cfg.descriptionDelimiter !== undefined) { + params += '&http_fasta_description_delimiter=' + + encodeURIComponent(cfg.descriptionDelimiter); + } + + return params; + } + /** * Execute server-side download via form POST * @param {Object} spec - Download specification @@ -221,14 +260,7 @@ define([ // FASTA header configuration via http_fasta_* query parameters. // Send all params when configured — empty string disables a section/prefix. - if (spec.fastaConfig) { - var idFields = spec.fastaConfig.idFields || []; - var descFields = spec.fastaConfig.descriptionFields || []; - actionUrl += '&http_fasta_id_fields=' + encodeURIComponent(idFields.join(',')); - actionUrl += '&http_fasta_id_prefix='; - actionUrl += '&http_fasta_description_fields=' + encodeURIComponent(descFields.join(',')); - actionUrl += '&http_fasta_context_fields='; - } + actionUrl += buildFastaParams(spec); var form = domConstruct.create('form', { method: 'POST', @@ -378,14 +410,7 @@ define([ } // FASTA header configuration — send all params when configured - if (spec.fastaConfig) { - var idFields = spec.fastaConfig.idFields || []; - var descFields = spec.fastaConfig.descriptionFields || []; - query += '&http_fasta_id_fields=' + encodeURIComponent(idFields.join(',')); - query += '&http_fasta_id_prefix='; - query += '&http_fasta_description_fields=' + encodeURIComponent(descFields.join(',')); - query += '&http_fasta_context_fields='; - } + query += buildFastaParams(spec); topic.publish('/Download/started', { type: 'client-side', @@ -716,13 +741,19 @@ define([ var fastaConfig = spec.fastaConfig || {}; var idFields = fastaConfig.idFields || ['feature_id']; var descFields = fastaConfig.descriptionFields || []; + // Honour the configured delimiters here too. This path builds the header in + // the browser rather than asking the server for one, so hardcoding them + // would reintroduce the same preview/download mismatch on this route. + var idDelimiter = fastaConfig.idDelimiter !== undefined ? fastaConfig.idDelimiter : '|'; + var descDelimiter = fastaConfig.descriptionDelimiter !== undefined + ? fastaConfig.descriptionDelimiter : ' '; var sequenceField = spec.format === 'protein+fasta' ? 'aa_sequence' : 'na_sequence'; data.forEach(function (record) { - var idPart = idFields.map(function (f) { return record[f] || ''; }).join('|'); + var idPart = idFields.map(function (f) { return record[f] || ''; }).join(idDelimiter); var defLine = '>' + idPart; if (descFields.length > 0) { - var descPart = descFields.map(function (f) { return record[f] || ''; }).join(' '); + var descPart = descFields.map(function (f) { return record[f] || ''; }).join(descDelimiter); defLine += ' ' + descPart; } lines.push(defLine); diff --git a/public/js/p3/util/tests/downloadExecutorFasta.test.js b/public/js/p3/util/tests/downloadExecutorFasta.test.js new file mode 100644 index 000000000..b9443aa99 --- /dev/null +++ b/public/js/p3/util/tests/downloadExecutorFasta.test.js @@ -0,0 +1,246 @@ +/* eslint-env jest */ +const fs = require('fs'); +const path = require('path'); + +/** + * DownloadExecutor's FASTA header parameters. + * + * These guard the bug that prompted the work: the wizard preview showed + * '|'-separated description fields while the download joined them with a + * space, because the client never sent a delimiter and the server's default + * was ' '. The invariant worth protecting is that whatever the configurator + * reports from getData() is what reaches the wire. + * + * DownloadExecutor is an AMD module with real dependencies, so load it with a + * define shim and stub them, following joinTooBroad.test.js. The xhr stub + * captures the request so we can assert on the query string. + */ +function loadExecutor(stubs) { + const src = fs.readFileSync(path.join(__dirname, '..', 'DownloadExecutor.js'), 'utf8'); + let exported; + const define = function (deps, factory) { + exported = factory.apply(null, deps.map((d) => stubs[d] || stubs._default)); + }; + // eslint-disable-next-line no-new-func + new Function('define', src)(define); + return exported; +} + +/** + * The module reads window.App both at load time (DATA_API_URL) and during a + * download (the auth token), so window must exist for the whole run. jsdom is + * not installed; a bare global stands in, as retryRequest.test.js does. + */ +beforeEach(() => { + global.window = { App: { dataServiceURL: '/api/data' } }; + global.document = { body: {} }; +}); + +afterEach(() => { + delete global.window; + delete global.document; +}); + +/** Minimal stand-ins for the dojo modules DownloadExecutor pulls in. */ +function makeStubs(captured) { + const thenable = { + then: function (ok) { if (ok) ok(''); return thenable; }, + otherwise: function () { return thenable; } + }; + + return { + _default: function () {}, + 'dojo/_base/declare': function () {}, + 'dojo/_base/lang': { mixin: Object.assign }, + 'dojo/Deferred': function () { + this.promise = {}; + this.resolve = function () {}; + this.reject = function () {}; + }, + 'dojo/when': function (v, cb) { if (cb) cb(v); return v; }, + 'dojo/request/xhr': { + post: function (url, opts) { + captured.url = url; + captured.data = opts && opts.data; + return thenable; + } + }, + 'dojo/dom-construct': { + create: function (tag, attrs) { + // The server-side path builds a hidden form and submits it; capture + // the action URL, which is where its http_fasta_* params live. + if (tag === 'form' && attrs && attrs.action) { + captured.formAction = attrs.action; + } + return { submit: function () {}, appendChild: function () {} }; + }, + destroy: function () {} + }, + 'dojo/topic': { publish: function () {} }, + './DownloadFormats': { + getFormat: function () { + return { id: 'protein+fasta', mimeType: 'application/protein+fasta', extension: '.fasta' }; + }, + getUniqueKey: function () { return 'feature_id'; }, + getDataTypeConfig: function () { return {}; } + }, + FileSaver: function () {} + }; +} + +function specFor(fastaConfig) { + return { + format: 'protein+fasta', + dataType: 'genome_feature', + scope: 'all', + rqlQuery: 'eq(genome_id,83332.12)', + primaryKey: 'feature_id', + fastaConfig: fastaConfig + }; +} + +/** + * The client-side route: parameters ride in the POST body. + * Requires forceClientSide — FASTA formats are serverSide by default. + */ +function queryFor(fastaConfig) { + const captured = {}; + const Executor = loadExecutor(makeStubs(captured)); + Executor.execute(specFor(fastaConfig), { forceClientSide: true }); + return captured.data || ''; +} + +/** + * The server-side route: parameters ride in the form action URL. This is the + * path a real FASTA download takes, so both must carry the delimiters — + * they were separate copies of the same code before this change. + */ +function actionUrlFor(fastaConfig) { + const captured = {}; + const Executor = loadExecutor(makeStubs(captured)); + Executor.execute(specFor(fastaConfig), { forceServerSide: true }); + return captured.formAction || ''; +} + +/** Pull one http_fasta_* parameter back out, decoded. */ +function param(query, name) { + const m = query.match(new RegExp('[&?]' + name + '=([^&]*)')); + return m ? decodeURIComponent(m[1]) : undefined; +} + +describe('DownloadExecutor FASTA header parameters', () => { + describe('delimiters reach the request', () => { + test('sends the description delimiter the configurator reported', () => { + const q = queryFor({ + idFields: ['patric_id'], + descriptionFields: ['strain', 'host_name'], + idDelimiter: '|', + descriptionDelimiter: ' | ' + }); + + expect(param(q, 'http_fasta_description_delimiter')).toBe(' | '); + expect(param(q, 'http_fasta_id_delimiter')).toBe('|'); + }); + + test('sends fields alongside the delimiters', () => { + const q = queryFor({ + idFields: ['patric_id'], + descriptionFields: ['strain', 'host_name'], + descriptionDelimiter: ' | ' + }); + + expect(param(q, 'http_fasta_id_fields')).toBe('patric_id'); + expect(param(q, 'http_fasta_description_fields')).toBe('strain,host_name'); + }); + + test('url-encodes a delimiter that would otherwise break the query string', () => { + const q = queryFor({ + idFields: ['patric_id'], + descriptionFields: ['strain', 'host_name'], + descriptionDelimiter: ' & ' + }); + + // The raw '&' must not appear as a separator... + expect(q).toContain('http_fasta_description_delimiter=%20%26%20'); + // ...and must survive the round trip. + expect(param(q, 'http_fasta_description_delimiter')).toBe(' & '); + }); + }); + + describe('absent vs empty delimiter', () => { + /** + * An empty string is a meaningful delimiter ("run the fields together"), + * so it must be transmitted. Absent means "leave the server default + * alone". Conflating the two via a falsiness check would silently drop + * the user's choice. + */ + test('an empty delimiter is sent, not dropped', () => { + const q = queryFor({ + idFields: ['patric_id'], + descriptionFields: ['strain'], + descriptionDelimiter: '' + }); + + expect(q).toContain('http_fasta_description_delimiter='); + expect(param(q, 'http_fasta_description_delimiter')).toBe(''); + }); + + test('an absent delimiter is omitted entirely', () => { + const q = queryFor({ + idFields: ['patric_id'], + descriptionFields: ['strain'] + }); + + expect(param(q, 'http_fasta_description_delimiter')).toBeUndefined(); + expect(param(q, 'http_fasta_id_delimiter')).toBeUndefined(); + // The field params are still sent. + expect(param(q, 'http_fasta_id_fields')).toBe('patric_id'); + }); + }); + + describe('no FASTA config', () => { + test('adds no http_fasta_* parameters at all', () => { + const q = queryFor(null); + expect(q).not.toContain('http_fasta_'); + }); + }); + + describe('server-side path carries the same parameters', () => { + /** + * A real FASTA download goes server-side (format.serverSide !== false), so + * this is the path that actually produced the reported bug. The two paths + * used to hold duplicate parameter-building code. + */ + test('delimiters appear in the form action URL', () => { + const url = actionUrlFor({ + idFields: ['patric_id'], + descriptionFields: ['strain', 'host_name'], + idDelimiter: '|', + descriptionDelimiter: ' | ' + }); + + expect(param(url, 'http_fasta_description_delimiter')).toBe(' | '); + expect(param(url, 'http_fasta_id_delimiter')).toBe('|'); + expect(param(url, 'http_fasta_description_fields')).toBe('strain,host_name'); + }); + + test('both paths agree for the same config', () => { + const cfg = { + idFields: ['patric_id'], + descriptionFields: ['strain', 'host_name'], + idDelimiter: '|', + descriptionDelimiter: ' | ' + }; + const names = [ + 'http_fasta_id_fields', + 'http_fasta_description_fields', + 'http_fasta_id_delimiter', + 'http_fasta_description_delimiter' + ]; + + const q = queryFor(cfg); + const url = actionUrlFor(cfg); + names.forEach((n) => expect(param(url, n)).toBe(param(q, n))); + }); + }); +}); diff --git a/public/js/p3/widget/download/FASTAConfiguratorStep.js b/public/js/p3/widget/download/FASTAConfiguratorStep.js index 981cda67e..556ecdf90 100644 --- a/public/js/p3/widget/download/FASTAConfiguratorStep.js +++ b/public/js/p3/widget/download/FASTAConfiguratorStep.js @@ -38,6 +38,16 @@ define([ descriptionFields: null, // Array of fields for description availableFields: null, // All available fields from query descriptor + // Delimiters. These mirror the server's own defaults + // (p3_api util/fastaHeaderFormatter.js createFastaHeaderFormatter) so that + // sending them explicitly cannot change output for a user who never touches + // them. The description delimiter defaults to ' ' on the server; we send + // ' | ' because that is what the preview has always shown and what makes a + // multi-field description parseable -- several values (host_name, product) + // contain spaces of their own. + idDelimiter: '|', + descriptionDelimiter: ' | ', + // Event handlers _handlers: null, @@ -345,20 +355,49 @@ define([ /** * Update the preview */ - _updatePreview: function () { - if (!this.previewNode) return; - + /** + * Build the header the way the server will. + * + * This must mirror p3_api util/fastaHeaderFormatter.js formatHeader(): the + * preview previously hardcoded ' | ' between description fields while the + * server joined them with a space, so the preview promised a layout the + * download never produced. Deriving both from the same delimiter values is + * the actual fix -- keep this in step with getData(). + * + * Note the server appends a TRAILING id delimiter after the ID section + * (fastaHeaderFormatter.js, "matches legacy format"), which is why the + * preview shows e.g. ">patric_id|" and not ">patric_id". + */ + _buildPreview: function () { var idField = this.sequenceIdField || 'id'; var descFields = this.descriptionFields.map(function (f) { return f.field; }); - var previewParts = ['>' + idField]; + var id = idField + (this.idDelimiter || ''); + var parts = ['>' + id]; + if (descFields.length > 0) { - previewParts.push(descFields.join(' | ')); + parts.push(descFields.join(this.descriptionDelimiter)); } - this.previewNode.innerHTML = '' + previewParts.join(' ') + ''; + return parts.join(' '); + }, + + _updatePreview: function () { + if (!this.previewNode) return; + this.previewNode.innerHTML = '' + this._escapeHtml(this._buildPreview()) + ''; + }, + + /** + * Field names and delimiters are user-influenced and land in innerHTML; a + * delimiter of '' should render as text, not markup. + */ + _escapeHtml: function (s) { + return String(s) + .replace(/&/g, '&') + .replace(//g, '>'); }, /** @@ -382,7 +421,9 @@ define([ sequenceIdField: this.sequenceIdField, descriptionFields: this.descriptionFields.map(function (f) { return f.field; - }) + }), + idDelimiter: this.idDelimiter, + descriptionDelimiter: this.descriptionDelimiter }; }, diff --git a/public/js/p3/widget/download/UnifiedDownloadWizard.js b/public/js/p3/widget/download/UnifiedDownloadWizard.js index 0fdff1f2e..42c3def1f 100644 --- a/public/js/p3/widget/download/UnifiedDownloadWizard.js +++ b/public/js/p3/widget/download/UnifiedDownloadWizard.js @@ -639,7 +639,9 @@ define([ // FASTA configuration — maps to http_fasta_* query parameters fastaConfig: optionsData.sequenceIdField ? { idFields: [optionsData.sequenceIdField], - descriptionFields: optionsData.descriptionFields || [] + descriptionFields: optionsData.descriptionFields || [], + idDelimiter: optionsData.idDelimiter, + descriptionDelimiter: optionsData.descriptionDelimiter } : null, // GenBank merged mode