Support upserting cards during JSON bulk import

[mucsi96]

Summary

Enable the JSON bulk import feature to update existing cards with matching IDs instead of only creating new cards. This allows users to refresh card content from JSON sources while preserving learning metadata.

Note: POST /api/card is shared by all card-creation flows, so the upsert semantics (and the fix for silently overwriting existing rows, including their FSRS progress) apply beyond JSON import.

Key Changes

• CardController.createCard(): Modified to check if a card with the given ID already exists. If it does, only the card data is updated — source, page, type, readiness, and FSRS learning metadata are preserved; if not, a new card is created with all provided fields. This implements an upsert pattern and replaces the previous behavior where a colliding ID silently overwrote the whole row (resetting learning progress).
• New validation (new 400 responses):
  • Missing id is rejected with 400 instead of failing inside the repository lookup.
  • An ID that already exists under a different source is rejected with 400, so an import into one source can no longer modify another source's card.
  • An ID that already exists with a different card type is rejected with 400.
• Transactional boundary: createCard is now @Transactional, consistent with updateCard, so the read-then-write upsert is atomic.
• Test coverage: Added Playwright tests verifying:
  • Existing cards are updated with new data from JSON import
  • Learning metadata (readiness, state, reps) and sourcePageNumber are preserved on updates
  • New cards are created as drafts alongside updated cards
  • Cross-source ID collisions are rejected and reported as failed imports, leaving the other source's card untouched
  • Cross-type ID collisions are rejected and reported as failed imports

Implementation Details

The upsert logic uses Java's Optional API to handle both cases:

• If card exists (same source and type): update only the data field, preserving all other attributes
• If card doesn't exist: create a new card with all fields from the request

This approach maintains backward compatibility while enabling the new upsert functionality.

https://claude.ai/code/session_01Ek3VrnXDFQjs6R2HY9DNxD

[claude]

Overview

This PR changes CardController.createCard() (POST /card) from a pure "create" into an upsert: if a card with request.getId() already exists, only its data field is updated (learning metadata like readiness, state, reps, stability, etc. is preserved); otherwise a new card is built as before. A Playwright E2E test covers the happy path for JSON bulk import.

Code quality / style

• The Optional.map/orElseGet chain is idiomatic and readable, consistent with the "functional over procedural" preference in AGENTS.md.
• existingCard.setData(request.getData()) mutates the loaded JPA entity in place. AGENTS.md calls for avoiding mutation and preferring immutable transformations; for a Hibernate-managed @Entity this is the normal, expected pattern (dirty checking needs a live managed instance), so this is a reasonable, pragmatic exception rather than a real issue — flagging it only because it technically departs from the stated style rule.
• Minor: the two branches now duplicate the concept of "which fields define a card" in two places (mutate vs. builder). Not a problem at this size, but if more upsert-only fields are added later, consider consolidating into a single applyRequest(Card target, CardCreateRequest req) helper.

Potential issues / risks

1. createCard is a shared endpoint, not JSON-import-specific. It's also used by the PDF word/sentence-based bulk creation flow (bulk-card-creation.service.ts, the vocabulary/grammar/speech-card-type.strategy.ts files via /api/word-id -> exists). Today that flow filters out already-existing ids client-side (itemsToCreate = source.items.filter(item => !item.exists)), so in practice it shouldn't hit the new upsert path — but if that client-side filter is ever bypassed (stale cache, a race between the existence check and creation, a retried request), the new behavior silently preserves the old card's source, sourcePageNumber, type, readiness, scheduling data, etc. and only swaps data, whereas previously it would have fully overwritten the row. Worth making explicit (comment or dedicated method) that this upsert semantics is intended for every caller of this endpoint, not just JSON import.

2. No validation that the existing card belongs to the same source/type as the request. If a card with a colliding id exists under a different source (or with a different type), the current code updates its data without checking existingCard.getSource().getId() / existingCard.getType() against the incoming request. That could let an import for source A silently overwrite content belonging to source B. Since ids are user/deterministically supplied strings rather than DB-generated, a cross-source id collision isn't far-fetched. Consider validating existingCard.getSource().getId().equals(source.getId()) (and possibly type) before treating it as an update target, returning a clear error otherwise.

3. cardRepository.findById(request.getId()) throws IllegalArgumentException if id is null. Previously a null id just flowed into Card.builder().id(null)..., failing later at save/constraint time with a clearer error. Worth confirming id is guaranteed non-null for every caller of this endpoint, since a null id now surfaces as a less-specific failure at the findById call.

Test coverage

• The new Playwright test (bulk JSON import updates existing cards with matching ids) covers: existing card data updated, learning metadata (readiness, state, reps) preserved, and a new card in the same batch created as a draft — matches the repo's E2E-first testing convention nicely.
• Not covered: the cross-source id-collision scenario from point 2, and the null/absent id case from point 3. Consider a negative test if intended behavior there is defined.
• No backend unit/integration test for CardController itself, only E2E — consistent with this repo's existing pattern, so not a blocker.

Performance / security

• No security concerns — the endpoint keeps its existing @PreAuthorize check and doesn't introduce new inputs or bypass authorization.
• Performance impact is negligible — the added findById per card is a single indexed PK lookup next to the existing per-card save.

Overall a small, focused change that achieves what the description states for the JSON-import happy path. The main open question before merging: are the upsert semantics meant to apply repo-wide to every caller of POST /card, and if so, should cross-source id collisions be guarded against (point 2)?

[mucsi96]

Addressed the review in d98f013:

• Cross-source/type id collisions (point 2): POST /card now rejects with 400 when the existing card belongs to a different source or has a different type, instead of silently updating the other source's card. Added an E2E test (bulk JSON import does not touch cards belonging to another source) covering this — the import reports the collision as a failed card and leaves the other source's card untouched.
• Null id (point 3): a missing id is now rejected with an explicit 400 before the repository lookup.

On the open question: yes, the upsert semantics are intended for every caller of POST /card, scoped to same-source, same-type ids — the new guards make that boundary explicit. The PDF-based bulk creation flow still filters existing ids client-side, and if that filter is ever bypassed, the card's source, type, readiness, and scheduling data can no longer be clobbered.

---

Generated by Claude Code

[claude]

Review: Support upserting cards during JSON bulk import

Overview
Changes CardController.createCard() (POST /card) from a pure create into an upsert: if a Card with the given id already exists, only data is updated (preserving FSRS/learning metadata); otherwise a new card is created as before. Two guard checks are added (source mismatch, type mismatch), plus a new id-required check. Two new Playwright E2E tests cover the upsert and cross-source protection cases.

Strengths

• Fixes a real latent bug, not just JSON import. POST /card is shared by the JSON importer and the generic bulk creation flow (bulk-card-creation.service.ts), where card IDs are deterministic (word/sentence IDs). Since Card.id is manually assigned (no @GeneratedValue), cardRepository.save() on a colliding ID previously fell through to a JPA merge, silently overwriting readiness/state/FSRS fields (and even source) of an existing card with whatever the new draft request contained - effectively resetting a user's review progress if the same word was ever re-imported/re-created. This PR closes that hole by only touching data and by explicitly rejecting cross-source/cross-type collisions with a 400 instead of silently corrupting the row. Worth calling out in the PR description since it's a behavioral fix beyond the stated JSON-import scope.
• Good test coverage for the two most important new behaviors: preserving learning metadata on update, and the cross-source intruder rejection.
• Follows existing controller conventions (ResponseStatusException + HttpStatus.BAD_REQUEST, .formatted() message style) consistent with the rest of the file.

Issues / Suggestions

1. sourcePageNumber and other request fields are silently ignored on update - worth confirming this is intended. On the update path only data is copied from the request; sourcePageNumber, readiness, due, FSRS fields, etc. from the JSON payload are dropped even if the caller supplied different values. That matches the PR description (only the card data is updated), but since CardCreateRequest still accepts all these fields, a caller could reasonably expect e.g. an updated sourcePageNumber to take effect. Consider documenting this explicitly (or using a dedicated upsert-specific request shape carrying only id/data) so future readers don't assume all fields are honored.

2. No transactional boundary around the read-then-write. Unlike updateCard (annotated @transactional), the new findById(...).map(...).orElseGet(...) + saveCard(...) sequence isn't wrapped in a transaction. For a single request this is low-risk, but the client imports in concurrent chunks (Promise.allSettled, CARD_CREATION_CHUNK_SIZE = 10), so if two requests raced on the same id within a batch, both could read not-found and attempt inserts, or one could clobber the other's read-modify-write. Worth a @transactional for consistency with updateCard and to close that race.

3. Mutating a fetched entity (existingCard.setData(...)) conflicts with the repo's stated style guide (Avoid variable mutations - create new values instead of modifying existing ones, per AGENTS.md). This mirrors the existing pattern in updateCard just above it in the same file, so it's consistent with current practice rather than a regression - but since this PR is net-new code, it could have used existingCard.toBuilder().data(request.getData()).build() to stay closer to the guideline. Not blocking given the precedent, but flagging since AGENTS.md is meant to override default behavior.

4. Type-mismatch error message omits the requested type. "Card %s already exists with type %s" reports only the existing card's type, not the requested type that caused the conflict, making the 400 harder to debug from the JSON import UI. Consider "Card %s already exists with type %s (requested %s)".

5. Test coverage gap: the new tests check topic, category, frontText, backText, readiness, state, reps are preserved/updated correctly, and cover the source-mismatch rejection, but don't cover the new type-mismatch 400 path, or assert sourcePageNumber is left untouched on update. Consider adding a case for re-importing a card with the same id but a different type.

6. Minor: the new id == null check changes existing behavior for missing id from a raw DB NOT NULL failure (likely a 500) to a clean 400 - a nice improvement, just flagging the behavior change for reviewers/API consumers.

Security
No concerns - the endpoint is already gated by @PreAuthorize(hasAuthority('APPROLE_DeckCreator') and hasAuthority('SCOPE_createDeck')), and the added source/type checks actually reduce risk of cross-source data corruption compared to before.

Performance
Negligible - one extra findById per card (PK lookup), fine for the bulk-import chunk sizes used (10 per chunk).

---

Overall this is a solid, well-tested fix that also closes an unrelated latent data-loss bug in the shared bulk-creation path. The suggestions above (transaction boundary, clarifying which fields are/aren't updated on upsert) are worth considering but not blocking.

[mucsi96]

Follow-ups from the second review round landed in e28b3e3:

• createCard is now @Transactional, consistent with updateCard (point 2).
• The type-collision error message includes the requested type (point 4).
• Added an E2E test for the type-collision 400 path and an assertion that sourcePageNumber is untouched on update (point 5).

On point 1: yes, updating only data on the upsert path is intended — the update is scoped to the card's details, while identity (source, page, type) and learning state (readiness, FSRS fields) are deliberately preserved; the new sourcePageNumber test now pins that behavior. Kept the entity-setter pattern over toBuilder() (point 3) since it matches updateCard and Hibernate dirty checking.

---

Generated by Claude Code

[claude]

Review: Support upserting cards during JSON bulk import (#399)

Overview

Turns POST /api/card into an upsert: if a card with the given id already exists, only data is updated (learning metadata like readiness/state/reps/due is preserved); otherwise a new card is created as before. Adds @Transactional, a required-id check, and cross-source/type-mismatch guards, plus three well-designed Playwright tests.

Code quality & conventions

• Good use of Optional#map/orElseGet to express the upsert branch functionally, consistent with the project's "prefer functional programming" guideline in AGENTS.md.
• @Transactional on createCard is correct now that the handler does a read-then-write, and mirrors the existing updateCard endpoint's annotation.
• Mutating existingCard via setters inside the .map() lambda is consistent with the mutable-JPA-entity pattern already used elsewhere in this controller (updateCard, selectVoiceForCard, addAudioToCard), even though it technically diverges from the "avoid mutation" style rule — acceptable given the existing codebase convention.
• Throwing ResponseStatusException from inside the .map() lambda works (it's unchecked) but reads a bit like a side-effecting validation step disguised as a mapping function. Consider pulling the source/type checks into a small private validation method invoked before the Optional chain, purely for readability — not a correctness issue.

Potential issues

1. Undocumented behavior additions: the cross-source hijack guard and card-type mismatch guard are valuable, well-tested additions, but they aren't mentioned in the PR's "Key Changes" summary (which only describes the update-preserves-metadata behavior). Worth calling out explicitly in the description since they change the API's error surface (new 400 responses for previously-unvalidated combinations).
2. Race on concurrent creates of the same new id: findById → orElseGet(create) → save is not atomic. If two requests for the same new card id land concurrently (e.g. within the same 10-item chunk from the frontend's Promise.allSettled batches — unlikely since ids come from distinct suggestions, but possible with retries/duplicate JSON entries), both could miss the findById and attempt an insert, causing a DataIntegrityViolationException (500) instead of a clean upsert. Pre-existing risk class, just worth being aware of; a DB-level upsert (ON CONFLICT) would remove it entirely if this becomes a real scenario.
3. Frontend messaging now slightly inaccurate: json-page.component.ts reports Imported N cards as drafts on success, but with this change some of those N cards may be pre-existing cards whose readiness/state was preserved (not draft, e.g. REVIEW). Minor UX nit — consider adjusting the message now that "import" can mean "update" too.
4. Error message wording: "Card %s already exists in source %s" only names the existing card's source, not the source the caller was trying to import into — slightly ambiguous when debugging. Including both source ids would make the 400 response more actionable.

Test coverage

Strong addition — three new Playwright tests cover:

• Update-preserves-metadata (readiness/state/reps) while new sibling cards are still created as drafts.
• Cross-source protection (card id owned by another source is rejected, and the partial-failure UI path is exercised).
• Card-type mismatch rejection.

Tests correctly follow the repo's Playwright conventions (role/label-based selectors, no assertion messages, verifying via direct DB queries for backend state). One nice-to-have: none of the new tests assert the HTTP status code returned for the rejected cases — only the resulting UI text/DB state — so a regression that changed the guard from 400 to, say, 500 wouldn't be caught. Not blocking given the UI-level assertions already verify the practical behavior.

Security

The cross-source ownership check is a good defense against one source's JSON import silently overwriting/hijacking a card id that belongs to a different source — nice catch, and it's properly authorization-scoped behind the existing DeckCreator/createDeck checks.

Summary

Solid, well-tested change that matches existing code patterns in the file. Main suggestions are documentation (mention the new validation behaviors in the PR description) and two minor polish items (error message clarity, frontend "as drafts" wording). No blocking issues found.

[claude]

Review: Support upserting cards during JSON bulk import

Overall this is a solid, well-scoped fix: it closes a real data-loss bug (a colliding ID used to silently overwrite the whole row, resetting FSRS progress), adds sensible validation, and comes with good E2E coverage for the new failure/success paths. A few things worth a look before merging.

Code quality / correctness

• request.getData() is written unconditionally on update, with no null guard (CardController.java:174). CardCreateRequest.data has no @NotNull/validation, so a caller that omits data (or sends null) will silently wipe the existing card's content on update. Contrast this with updateCard(), which guards every field with if (request.getX() != null) ... before applying it. Today's two frontend callers (json-source.service.ts, bulk-card-creation.service.ts) always populate data, so this isn't currently reachable — but since the PR description explicitly calls out that POST /api/card is shared by all card-creation flows and is meant to stop silent overwrites, it'd be more defensive (and consistent with updateCard's pattern and the "avoid fallback values that hide missing data" project convention) to reject a null data with a 400 rather than rely on every current/future caller always sending it.

• Lazy-load on existingCard.getSource().getId() (CardController.java:159) triggers an extra query per collision check. Minor — only hit on the (hopefully rare) collision path — but if Card.source is FetchType.LAZY, consider whether cardRepository.findById already fetches it or whether this adds an N+1 in bulk-import loops where many IDs collide.

• readiness is silently ignored on update, by design — worth confirming this is the intended product behavior. If a user re-imports a JSON source and explicitly picks "READY" as the import readiness, an existing DRAFT card that gets updated will stay DRAFT forever (since only data is touched). That matches the stated goal of "preserving learning metadata," but readiness/workflow state arguably isn't FSRS learning metadata in the same sense as stability/difficulty/reps. Might be worth a short note in the PR description or a follow-up ticket if this surprises users.

Style / conventions

• Good use of Optional.map/.orElseGet here — consistent with the "prefer functional over procedural" convention in AGENTS.md, and a nice improvement over a mutable if/else.
• @Transactional addition on createCard correctly matches the existing pattern on updateCard, and the read-then-write is now atomic — the two validation exceptions are thrown before any entity mutation, so a mid-map() throw won't leave a half-updated card at rollback time.
• Error messages read a little ambiguous ("Card %s already exists in source %s (requested %s)"). Consider clarifying with explicit labels, e.g. "...already exists in source %s, but import targeted source %s", to make the 400 body more actionable for API consumers/log readers.

Test coverage

• The three new Playwright tests are thorough and follow the repo's testing conventions (role-based selectors, DB assertions via withDbConnection, no assertion messages). They cover: update-preserves-metadata, cross-source rejection, and cross-type rejection — exactly the three new validation branches.
• One gap: there's no test for the "missing id" 400 case (request.getId() == null), which is new behavior introduced in this PR. Worth adding a quick case, or at least confirming it's covered elsewhere.
• No server-side (JUnit) test was added for CardController.createCard. The new branching logic (source mismatch, type mismatch, missing id, update vs. insert) is a good candidate for a fast unit/slice test in addition to the E2E coverage, since Playwright tests are comparatively expensive to run and slower to pinpoint failures in.

Security

• No new security concerns. The endpoint remains behind the existing DeckCreator/createDeck authorization, and the cross-source check is actually a security-relevant improvement (previously one source's import could clobber another source's card).

Summary

The core upsert logic and validation are correct and well tested at the E2E level. The main thing I'd want addressed before merge is the missing null-check on data during update, since it's the one place this PR could reintroduce a silent-overwrite footgun for future callers of the shared endpoint. Everything else is minor polish (error message wording, an optional unit test, a product question about readiness).

[mucsi96]

The pre-merge item from the latest review is addressed in 75fb981: a null data payload is now rejected with 400 ("data is required") alongside the id guard, covering both the create and update paths. (Strictly it couldn't have silently wiped content — the column is NOT NULL, so it would have failed at flush with a 500 — but the explicit 400 is cleaner and consistent.)

Notes on the remaining points: Card.source is a default-EAGER @ManyToOne, so the collision check doesn't add a lazy-load query; the readiness-preserved-on-update behavior is intended and documented in the PR description; and per repo convention the branching is covered by E2E tests rather than JUnit slices.

---

Generated by Claude Code

[claude]

Review: Support upserting cards during JSON bulk import

Overview

This PR changes POST /api/card from an always-create/overwrite endpoint into an upsert: when a card with the given id already exists, only data is updated and everything else (source, page, type, readiness, FSRS scheduling state) is preserved; otherwise a new card is created as before. It also adds guard rails (missing id/data, cross-source id reuse, cross-type id reuse → 400) and makes createCard @Transactional to match updateCard. Good catch on the underlying bug — silently resetting a card's FSRS progress on an id collision was a real data-integrity risk, and since this endpoint is shared by all card-creation flows (JSON import, bulk creation, single-word add), fixing it here benefits more than just JSON import.

Code quality & style

• The Optional.map/.orElseGet upsert pattern fits the project's functional-style guidance well and reads cleanly.
• Mutating existingCard via setData(...) inside the map lambda is a reasonable exception to the "avoid mutation" rule — JPA dirty-checking needs the managed entity mutated in place; rebuilding via the builder and merge-ing would be more ceremony for no benefit here.
• Throwing ResponseStatusException from inside the lambda is fine (it's unchecked and propagates through Optional.map), but it does mean the "upsert" lambda is also doing validation — worth a short comment or extracting the two if checks into a small private helper (e.g. assertMatchesExisting(existingCard, source, request)) if this logic grows further. Not blocking at current size.

Potential issues

• Silent field drop on update: when a card already exists, sourcePageNumber, due, stability, difficulty, readiness, etc. from the incoming request are entirely ignored — only data is applied. This is clearly intentional per the PR description, but it means, e.g., the bulk-extraction flow (bulk-card-creation.service.ts's processFromExtractedItem) always sends the current pageNumber, which will now be silently discarded if the card id already exists on that source (say the word appears again on a later page). Worth confirming this is the desired behavior for all callers of this shared endpoint, not just JSON import — a code comment on why only data is preserved/updated would help future readers.
• Race condition on new-id inserts: @Transactional makes the read-then-write atomic in terms of visibility within the transaction, but two concurrent requests for the same new id can both pass findById → empty, then both attempt save, and the second will fail on the DB unique constraint rather than returning a clean 400. This is a pre-existing condition (not introduced by this PR) but since the PR is hardening validation around id collisions, it may be worth deciding whether to also catch DataIntegrityViolationException and map it to a 400 here. Low priority given current usage patterns (chunked, not usually concurrent for the same id).
• Minor: the two new validation checks (id is required, data is required) don't have corresponding tests (unlike the cross-source/cross-type cases, which are well covered). Given the project's E2E-only test strategy, consider adding a quick negative-path Playwright case, or note why it's considered low-risk enough to skip.

Performance

No meaningful concerns — the extra findById lookup is a single indexed primary-key read, and the change doesn't add N+1 queries or loops.

Security

• The cross-source check is a good hardening: it prevents a JSON import targeted at one source from mutating (or resetting the learning progress of) a card that belongs to a different source, closing an accidental "confused deputy"-style data corruption path. Authorization is still coarse-grained (DeckCreator/createDeck, not source-scoped), but that's consistent with the rest of the app and out of scope here.
• Error messages echo back ids/source ids that the caller already supplied in the request, so no new information disclosure.

Test coverage

The three new Playwright tests are solid and test from the user's perspective (via the JSON import dialog UI plus DB assertions), covering:

• update-preserves-metadata (good, checks readiness, state, reps, source_page_number are untouched)
• cross-source rejection (verifies the other source's card is untouched)
• cross-type rejection (verifies the existing card's type/data are untouched)

This is good coverage of the new behavior described in the PR. As noted above, the two basic required-field validations (id, data) aren't covered by a test.

Overall this is a well-scoped, well-tested fix for a real bug. The one thing I'd want the author to double check before merging is the "page number and other request fields are dropped on update" behavior against all current callers of POST /api/card (especially the bulk-extraction flow), to make sure that's universally the desired semantics and not just true for the JSON-import use case it was designed around.