Skip to content

feat!(backend): add CrossRef writeEnabled flag, fail crossRef.isActive check if empty strings are passed#6687

Merged
tombch merged 20 commits into
mainfrom
crossref-write-enabled
Jun 16, 2026
Merged

feat!(backend): add CrossRef writeEnabled flag, fail crossRef.isActive check if empty strings are passed#6687
tombch merged 20 commits into
mainfrom
crossref-write-enabled

Conversation

@tombch

@tombch tombch commented Jun 16, 2026

Copy link
Copy Markdown
Collaborator

resolves #6643

BREAKING CHANGES

To enable DOI creation on PPX the writeEnabled flag now needs to be set to true (default is false), additionally we can now give staging access to the crossRef credentials and set writeEnabled: false for staging.

Summary

  • Adds a crossref.writeEnabled flag, defaulting to false, which when set to true enables Crossref write operations (in this case, generating DOIs).
  • Alters the createSeqSetDOI function to require the Crossref service is both active and write enabled before creating any entries in the database - previously, it would create an entry in the database, and then silently skip posting to Crossref if not active.
  • Updates the tests to set Crossref service is active, mocking required functions.
  • Fixes Crossref isActive value, by requiring non-blank values rather than just non-null. This then flagged an issue in the GitHub CI where empty secrets were previously being passed into the test, and the service would still be marked as active.
  • Removes some redundant .toString() conversions being flagged when linting the backend.

Screenshot

image image

PR Checklist

  • All necessary documentation has been adapted.
  • The implemented feature is covered by appropriate, automated tests.
  • Any manual testing that has been done is documented (i.e. what exactly was tested?)

🚀 Preview: https://crossref-write-enabled.loculus.org

@tombch tombch changed the title feat(backend): Crossref writeEnabled flag feat!(backend): Crossref writeEnabled flag Jun 16, 2026
@tombch tombch marked this pull request as ready for review June 16, 2026 08:41
@claude claude Bot added backend related to the loculus backend component deployment Code changes targetting the deployment infrastructure labels Jun 16, 2026
@claude

claude Bot commented Jun 16, 2026

Copy link
Copy Markdown
Contributor

This PR may be related to: #5471 (Seqsets created but don't actually exist on crossref)

@claude

claude Bot commented Jun 16, 2026

Copy link
Copy Markdown
Contributor

Claude finished @tombch's task in 4m 28s —— View job


Code Review

  • Gather context
  • Read changed files
  • Post review findings

Summary

The feature is clean and well-structured, but there's one significant bug: the caller in SeqSetCitationsDatabaseService.kt doesn't check isWriteEnabled before calling postCrossRefXML. This means that when isActive=true and isWriteEnabled=false, the createSeqSetDOI operation will:

  1. Write the DOI to the database
  2. Then throw a RuntimeException from postCrossRefXML

...causing the user-facing request to fail with an unhandled error, even though this read-only mode is supposed to just skip the CrossRef posting gracefully.

Fix needed in SeqSetCitationsDatabaseService.kt line 362:

// Before (buggy when writeEnabled=false):
if (crossRefService.isActive) {

// After:
if (crossRefService.isActive && crossRefService.isWriteEnabled) {

Fix this →


Other observations

values.yaml not updated: The crossRef section in values.yaml doesn't include writeEnabled. This is safe in practice — Go template zero-values + Spring Boot's null handling for empty Boolean? bindings default to false — but it mirrors the pattern of other optional fields already absent from values.yaml, so this is probably intentional. Worth considering adding writeEnabled: false explicitly for clarity.

Missing test for the write-enabled path: There's no test that verifies postCrossRefXML succeeds when writeEnabled=true (the existing HTTP posting logic). However, the actual HTTP call requires mocking the connection which is out of scope for a unit test, so this is acceptable.

values.schema.json formatting: Per kubernetes/AGENTS.md, prettier should be run after editing this file (npx prettier@3.6.2 --write kubernetes/loculus/values.schema.json). Please verify this was done.

Description style: The new writeEnabled description ends with a period ("...such as generating DOIs.") while adjacent fields like hostUrl don't. Minor, but worth being consistent.

@anna-parker anna-parker added the preview Triggers a deployment to argocd label Jun 16, 2026
@anna-parker anna-parker changed the title feat!(backend): Crossref writeEnabled flag feat!(backend): add CrossRef writeEnabled flag, fail crossRef.isActive check if empty strings are passed Jun 16, 2026
@anna-parker anna-parker self-requested a review June 16, 2026 11:44
@tombch

tombch commented Jun 16, 2026

Copy link
Copy Markdown
Collaborator Author

@claude review

@claude

claude Bot commented Jun 16, 2026

Copy link
Copy Markdown
Contributor

Claude finished @tombch's task in 1m 33s —— View job


Code Review

  • Gather context
  • Read changed files
  • Post review findings

Summary

The implementation is clean and correct. The bug flagged in the previous Claude review (DB write happening before isWriteEnabled check) has been fixedcreateSeqSetDOI now checks both isActive and isWriteEnabled before any database mutation (lines 346–352 of SeqSetCitationsDatabaseService.kt). The ordering is correct.


Issues Found

1. Missing test: isWriteEnabled=false path in endpoint tests — minor

All three endpoint test setup methods now mock isActive=true, isWriteEnabled=true, but there's no test asserting that createSeqSetDOI returns HTTP 403 when isWriteEnabled=false. This is a straightforward test to add and would protect against future regressions.

@Test
fun `createSeqSetDOI returns 403 when crossref write is not enabled`() {
    every { crossRefService.isWriteEnabled } returns false
    // ... call DOI endpoint and assert 403
}

Fix this →

2. values.schema.json prettier formatting — should verify

Per kubernetes/AGENTS.md, npx prettier@3.6.2 --write kubernetes/loculus/values.schema.json must be run after editing this file. Please confirm this was done — the diff looks structurally fine, but indentation-level formatting issues can be subtle.

3. values.yaml does not include writeEnabled — informational

The crossRef section in values.yaml has no writeEnabled key. This is safe: the Go template zero-value for boolean is false, and values.schema.json sets "default": false. However, it means that without explicit configuration, the template renders value: "false" which Spring Boot parses correctly. Still, adding writeEnabled: false explicitly would make the default self-documenting and consistent with other crossref fields.

4. Description style inconsistency — nit

values.schema.jsonwriteEnabled's description ends with a period ("...such as generating DOIs.") while adjacent fields like hostUrl don't. Worth being consistent either way.


What Looks Good

  • The isActive check now uses isNullOrBlank() (instead of just null-checking), correctly handling empty string secrets passed in CI — and removing those env vars from backend-tests.yml to prevent them from being passed as empty strings in the first place.
  • isWriteEnabled = properties.writeEnabled == true is a clean, safe way to handle Boolean?Boolean coercion.
  • postCrossRefXML has its own checkIsActive() and checkIsWriteEnabled() guards as defense-in-depth, even though createSeqSetDOI already gates them. Good.
  • The new CrossRefServiceTest tests (postCrossRefXML is rejected when write is not enabled and the Spring property coercion test) are well targeted.
  • Mock setup in endpoint tests now correctly includes generateCrossRefXML and postCrossRefXML stubs, so DOI creation tests exercise the full path without hitting real CrossRef.

Comment thread kubernetes/loculus/values.yaml Outdated
@anna-parker

Copy link
Copy Markdown
Contributor

side note: before merging lets prepare a PR for PPX with this config change and adding the secrets to staging

@tombch tombch merged commit 2c355cb into main Jun 16, 2026
41 of 42 checks passed
@tombch tombch deleted the crossref-write-enabled branch June 16, 2026 15:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

backend related to the loculus backend component deployment Code changes targetting the deployment infrastructure preview Triggers a deployment to argocd

Projects

None yet

Development

Successfully merging this pull request may close these issues.

SeqSet: add mint flag

2 participants