Skip to content

Feat/documents biblio author sort - #1742

Open
dawoldo wants to merge 7 commits into
developfrom
feat/documents-biblio-author-sort
Open

Feat/documents biblio author sort#1742
dawoldo wants to merge 7 commits into
developfrom
feat/documents-biblio-author-sort

Conversation

@dawoldo

@dawoldo dawoldo commented Jul 27, 2026

Copy link
Copy Markdown
Contributor

🤔 What

Add the ability to sort bibliography (document) search results by author, both persons and organizations.

  • New backend sort field authorsSort on the documents Typesense collection
  • Exposed via the advanced-search endpoint: sort=authorsSort:asc / :desc for entity=documents
  • Organization authors (j_document_grotto_author) are now indexed and searchable — previously they weren't indexed at all
  • Unblocks the front-end "sort by author" column that was disabled with a "the search index exposes no sortable author field" comment

🤷‍♂️ Why

Typesense cannot sort on array fields, and a document's authors are stored as string[] (authors.nickname) split across two relations — persons (j_document_caver_author) and organizations (j_document_grotto_author). There was no scalar field to sort on, so biblio results couldn't be ordered by author, hurting the browsing UX. This adds the denormalized sort key the index needs.

🔍 How

  • New helper api/utils/computeDocumentAuthorsSort.js — computes a single scalar key = the alphabetically-first author name across both persons and organizations, normalized (diacritics stripped via NFKD, lowercased, whitespace collapsed) so Typesense's byte-order sort matches human A→Z. Authorless documents get a ~ sentinel that sorts last on ascending order.
  • Schema (api/dbSync/entities/document.js) — added { name: 'authorsSort', type: 'string', optional: true, sort: true }, plus a j_document_grotto_author join so org names feed the key. The key is computed per row during the full reindex.
  • Single-doc consistency (api/services/DocumentService.js) — updateInSearch recomputes authorsSort via the same helper on every upsert, so incremental edits match the reindexed baseline.
  • API guard (advanced-search.js) — authorsSort is scoped to entity=documents; using it elsewhere returns a clean 400 instead of a raw Typesense error. Sorting itself flows through the existing generic sort param — no new wiring.
  • Follows the existing dataQuality computed-sort-field pattern already in the codebase.

Production actions required

Important

This PR changes the Typesense schema. The new authorsSort field is not populated until a full search reindex runs in each environment. Until then, sorting by author returns empty/unsorted results.

  • After deploying to production (and staging), run the reindex.
  • No DB migration is needed — this is search-index-only.

⚠️ Limitations of the current implementation

This uses an alphabetical-first-author approximation, not true bibliographic ordering. Accepted trade-offs:

  • Not first-listed-author order. The author join tables have no ordinal column, so author order isn't stored and can't be recovered. We sort by the alphabetically-smallest name, not the first-listed one — e.g. a work by "Zola & Adam" sorts under A, not Z.
  • Co-authors don't tie-break. Only the single smallest name drives ordering; documents sharing that name fall back to the _text_match tiebreaker, not their second author.
  • Persons and organizations are pooled as plain strings — no person-before-org preference (an org "AAA Speleo Club" outranks a person "Aabar").
  • Transliteration-only normalization. Non-Latin scripts (Cyrillic, Greek, …) aren't romanized and group after Latin names on ascending sort.
  • Locale-insensitive — raw code-point order, not language-aware collation.
  • Nickname-based — persons are keyed on nickname (may start with a first name), not a structured surname.

Future upgrade path: add an ordinal column to the author join tables and key on the ranked-first name. Only the helper's name-selection changes — schema, query wiring, and the front-end contract stay identical.

🧪 Testing

  • Unit tests: test/integration/2_utils/computeDocumentAuthorsSort.test.js (alphabetical-first, diacritics, whitespace, null/blank handling, empty-authors sentinel).
  • A full search reindex is required to populate the new field. Locally: node scripts/resync-search.js. Verified against the local dev stack — 38 documents reindexed, authorsSort present in the live schema, and sort_by=authorsSort:asc/:desc return correctly ordered results.
  • Manual check:
    GET /api/search/... ?entity=documents&sort=authorsSort:asc
    

📸 Previews

N/A — backend-only change. Front-end will attach screenshots of the enabled "sort by author" column.

@dawoldo
dawoldo requested a review from ClemRz July 27, 2026 20:00
@ClemRz

ClemRz commented Jul 30, 2026

Copy link
Copy Markdown
Contributor

Is this PR related to an existing issue?

ClemRz
ClemRz previously approved these changes Jul 30, 2026

@ClemRz ClemRz left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggestions (Should Consider)

  • [assets/swaggerV1.yaml ~L4805] The advanced-search sort description lists dataQuality as an entity-specific field but doesn't mention authorsSort. Since this field's whole purpose is a sortable author key and it silently returns 400 for non-document entities, it's worth documenting alongside dataQuality:

    Entity-specific sort fields:
      - entrances: "dataQuality:asc" or "dataQuality:desc" (sort by data quality score)
      - documents: "authorsSort:asc" or "authorsSort:desc" (sort by alphabetical-first author)

    The second sort description block (around line 4893) could also benefit from the same addition for completeness.

  • [api/utils/computeDocumentAuthorsSort.js] This is a pure, side-effect-free function — exactly the kind the tech stack targets with fast-check property-based tests. The current test suite covers concrete examples well, but a few properties are worth encoding as invariants:

    • Normalization idempotency: computeDocumentAuthorsSort([x]) should equal computeDocumentAuthorsSort([normalizeName(x)]) for any name.
    • Sentinel ordering: for any non-empty input, computeDocumentAuthorsSort([x]) < EMPTY_AUTHORS_SORT_KEY (the sentinel always sorts last).
    • Pool commutativity: the result must not depend on whether a name is in the personNames or organizationNames argument — swapping them should produce the same output.

    These invariants would catch regressions in normalizeName or the pool-merge logic that hand-picked examples might miss.

Nitpicks (Optional)

  • [test/integration/2_utils/computeDocumentAuthorsSort.test.js:1] The /* eslint-disable func-names */ directive at the top is a no-op here — there are no function keyword expressions in this file (all blocks use arrow functions). It appears to have been copied from the .property.test.js files in the same directory, where it's needed because fc.assert callbacks use function. Safe to remove.

  • [api/services/DocumentService.js:499] Minor code quality note: const [parents] = await Promise.all([...]) destructures only the first element, leaving the second (NameService.setNames result) unnamed. This is intentional since setNames mutates in place, but it can look like a bug to a new reader. A comment like // setNames mutates grottos in place; result is intentionally unused would make the intent immediately obvious without needing to trace into the service.

@ClemRz ClemRz left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Issues (Must Fix)

  • [api/controllers/v1/caver/delete.js] toCaver now calls toCitationDocument for documents (changed in converters.js), but caver/delete.js passes the caver directly from CaverService.getCaver() without the DocumentService.getDocumentsForCitation() enrichment step that caver/find.js correctly adds. The documents at that point only have the basic Waterline .populate('documents', ...) fields — no authors, authorsOrganization, identifierType, editor, or library. toCitationDocument won't crash (toList returns [] for missing fields), but the delete response will silently return documents with empty citation arrays, which is now a schema contract violation since CitationDocument is the advertised shape. Either add the enrichment step before the toCaver call:
    caver.documents = await DocumentService.getDocumentsForCitation(
      caver.documents.map((d) => d.id)
    );
    or revert the delete.js response to use a converter that matches the actual data shape (toSimpleDocument).

Suggestions (Should Consider)

  • [test/integration/4_routes/Search/] There are no route-level tests for the new authorsSort entity guard. The analogous dataQuality guard has a dedicated test file (advanced-search-data-quality-sort.test.js) with cases for valid use (documents), invalid use on a non-document entity (expecting 400), and multi-field sort combinations. Adding the equivalent for authorsSort would lock in the 400 behaviour and catch any future refactors to ENTITY_SPECIFIC_SORT_FIELDS. The existing test pattern (sinon stub on SearchService.collectionSearch + supertest) can be reused directly.

  • [assets/swaggerV1.yaml — SimpleOrganization schema] The new SimpleOrganization schema has a name field typed as string. However, toSimpleOrganization in converters.js delegates to getMainName(source), which can return null when no name is available. Consider adding nullable: true to the name property to avoid clients misinterpreting a null as a missing field:

    name:
      type: string
      nullable: true
  • [api/services/DocumentService.js:503 — getDocumentsForCitation] The JSDoc says "Same as getDocuments()" but the new method also fetches identifierType, editor, and library, which getDocuments() does not. The comment is accurate in spirit but may mislead future maintainers who look at both methods side by side. A small amendment like "Like getDocuments(), but also populates citation fields (identifierType, authors, authorsOrganization, editor, library) and resolves parent descriptions and grotto names." would make the distinction explicit.

Nitpicks (Optional)

  • [test/integration/2_utils/computeDocumentAuthorsSort.property.test.js:1] /* eslint-disable func-names */ is needed here because the test body uses function() (for this.timeout()). This is correct and intentional — no action needed. It's also worth noting this is a good example of the documented exception to the arrow-function rule in the steering file.

  • [api/services/DocumentService.js:540 — getDocumentsForCitation] for...of on documents (a plain array coming from TDocument.find(...)) is fine, but the ESLint config restricts for...in. This is for...of so it's clean — just confirming it won't trip the linter.

  • [api/dbSync/entities/document.js:198] row.authors?.map(...) uses optional chaining, but the join always sets row.authors (to an empty array if no matches). row.authorsOrganization has the same guarantee. The ?. is harmless but slightly misleading since the arrays can never be undefined at that point. Consistent with the existing row.cave?.[0] pattern in the file though, so feel free to leave as-is.

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.

2 participants