refactor: stop using EntityManager.merge to obtain managed entities

[netomi]

First batch of the #989 audit — the sites where EntityManager.merge was never meant to update anything, plus the ones where it did nothing at all. 8 of the 21 call sites; the remaining 13 are listed at the bottom.

Context: the instance originally reported by @hoangphamEclipse (VersionService.java:38, where merging a stale detached entity on a read path silently reverted a concurrent write and left extensions inactive) is long gone. This is the audit @amvanbaren asked for on top of it.

The same defect, in a new place

DownloadCountProcessor#evictCaches was carrying the motive in its own comment:

@Transactional // needs transaction for lazy-loading versions
public void evictCaches(Extension extension) {
    var mergedExtension = entityManager.merge(extension);

That is @hoangphamEclipse's diagnosis almost word for word — the merge existed only so the lazy version collections could be loaded. Nothing there updates the extension; it is cache eviction. But it wrote the whole row back, and the entity had already been loaded and mutated (setDownloadCount) in an earlier step of the same batch, so a concurrent update could be reverted exactly as before.

Now uses find, and skips eviction if the extension has since been deleted rather than letting merge attempt to resurrect the row.

The rest of this batch

site	was	now
DownloadCountProcessor#evictCaches	merge to load lazy collections	find
TrustedPublishingService	merge the caller's user to get an association target	find
MigrationService#scheduleMigration	merge, then only read getJobName/getEntityId	find
RenameDownloadsService#cloneResource	merge, then only read fields for the clone	find
ChangeNamespaceService#changeExtensionNamespace	merge of an already-managed entity	dropped
PublisherComplianceChecker (×2)	merge of already-managed entities	dropped
AccessTokenService#deactivateAccessToken	merge to make equals short-circuit	id comparison

The three dropped ones were plain no-ops: changeExtensionNamespace runs inside its own @Transactional method and both PublisherComplianceChecker merges run inside checkPublishers' TransactionTemplate, so all three were handed entities that were already managed and merge just returned the same instance — the setters are what persist. That leaves PublisherComplianceChecker with no use for an EntityManager, so the dependency is gone.

RenameDownloadsService#cloneResource still needs a managed resource, because it reads the lazy getExtension(); find provides that just as well, without the write.

The one that needed care

AccessTokenService#deactivateAccessToken looked like a free deletion — merge, then only equals and getLoginName(). It wasn't. UserData#equals compares every field, including tokens and memberships, so the merge was load-bearing: it made user the same managed instance as token.getUser() and let equals short-circuit on ==. Dropping it alone would start rejecting any caller holding a user that differs from the stored row in any field. The ownership check now compares ids, which is what it should have been doing regardless.

It also requires the id to be assigned, and that part came from the test suite rather than from me. My first version compared ids alone, and testDeleteAccessTokenWrongUser failed: its two users were both id 0, because mockUserData() never set one, so an unpersisted entity made the check fail open and a stranger could deactivate the token. The fixtures now carry real ids — which production entities always do — and the check rejects an unassigned id outright.

Testing

Full server suite green (1142 tests). Three new cases in AccessTokenServiceTest cover the ownership check: the owner path (asserting verifyNoInteractions(entityManager), i.e. that the user is no longer written back), a different user, and a token with no user at all. Confirmed non-vacuous — two of the three fail against the old code, including an NPE on the ownerless token that the old token.getUser().equals(user) would have thrown.

Deliberately not in this PR

Left for follow-ups so this stays reviewable, and because they need per-site judgement rather than a find-and-replace:

• merge-before-remove (2 sites: MigrationService#deleteFileResource, ChangeNamespaceService old namespace) — works, but emits a pointless UPDATE ahead of the DELETE.
• update intended, whole detached copy merged (11 sites, incl. EclipseService#updateUserData, PublishExtensionVersionService#markExtensionAsPotentiallyMalicious, ExtensionVersionIntegrityService#setSignatureKeyPair, CustomerService#deactivateRateLimitToken) — each merges a detached entity to set one field, so any column that changed meanwhile is reverted. find-then-set is the safer shape. Worth knowing that SetPreReleaseJobService (writes preRelease/preview) and CheckPotentiallyMaliciousExtensionVersionsService (writes potentiallyMalicious) both load an ExtensionVersion, do slow VSIX work, then merge the whole row — overlapping on the same version, the later merge reverts the other's field, and either can revert active.

Refs #989

🤖 Generated with Claude Code

[Copilot]

🟡 Changes recommended

Several new EntityManager.find(...) usages are dereferenced without null-handling, which can introduce NPEs if rows are deleted or principals are stale between transactions.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Pull request overview

This PR is the first batch of the #989 audit to remove unintended side effects from EntityManager.merge(...) calls that were used only to obtain managed entities (or were no-ops), replacing them with find(...), dropping redundant merges, and tightening access-token ownership checks to compare user identity by id.

Changes:

• Replaced merge with find in multiple read-only / association-target paths to prevent detached-entity state from being written back unintentionally.
• Removed redundant merge calls where entities are already managed within the surrounding transaction.
• Updated access-token deactivation ownership checks to compare user ids (and added tests/fixtures to cover and enforce the new behavior).

File summaries

File	Description
server/src/test/java/org/eclipse/openvsx/UserAPITest.java	Updates token-deletion test setup to reflect removal of merge(user) and ensures fixture users have assigned ids.
server/src/test/java/org/eclipse/openvsx/accesstoken/AccessTokenServiceTest.java	Adds regression tests for id-based ownership checks and ensures EntityManager is no longer invoked during deactivation.
server/src/main/java/org/eclipse/openvsx/trustedpublishing/TrustedPublishingService.java	Uses find(UserData, id) instead of merge(user) when setting the createdBy association.
server/src/main/java/org/eclipse/openvsx/storage/log/DownloadCountProcessor.java	Uses find(Extension, id) (and skips eviction if missing) to avoid merge side effects during cache eviction.
server/src/main/java/org/eclipse/openvsx/migration/RenameDownloadsService.java	Uses find(FileResource, id) instead of merge(resource) to avoid unintended writes when cloning a resource.
server/src/main/java/org/eclipse/openvsx/migration/MigrationService.java	Uses find(MigrationItem, id) instead of merge(item) for read-only scheduling logic.
server/src/main/java/org/eclipse/openvsx/eclipse/PublisherComplianceChecker.java	Drops redundant merges and removes EntityManager dependency where entities are already managed in the transaction template.
server/src/main/java/org/eclipse/openvsx/admin/ChangeNamespaceService.java	Removes redundant merge in a transactional namespace-change path where entities are already managed.
server/src/main/java/org/eclipse/openvsx/accesstoken/AccessTokenService.java	Switches token ownership verification from entity equality to id comparison and avoids merging the caller’s UserData.

Review details

Suppressed comments (2)

server/src/main/java/org/eclipse/openvsx/migration/MigrationService.java:94

• entityManager.find can return null if the MigrationItem row was deleted between fetching the slice and entering this transaction. The subsequent item.getJobName() dereference would then throw an NPE and abort scheduling for the batch; handle the missing row explicitly (log + skip, or throw a clear exception).

        // Only read from here on, so find rather than merge - see #989.
        item = entityManager.find(MigrationItem.class, item.getId());
        var jobIdText = item.getJobName() + "->itemId=" + item.getId();
        var jobId = uuidService.generateFromName(jobIdText);
        var handler = JOB_HANDLERS.get(item.getJobName());
        scheduler.schedule(jobId, scheduledAt, new MigrationJobRequest<>(handler, item.getEntityId(), item.getId()));

server/src/main/java/org/eclipse/openvsx/migration/RenameDownloadsService.java:35

• entityManager.find can return null; if the FileResource row no longer exists, the following resource.getStorageType() / getType() / getExtension() calls will throw an NPE. Add an explicit null check so the failure mode is deterministic.

        resource = entityManager.find(FileResource.class, resource.getId());
        var clone = new FileResource();
        clone.setName(name);
        clone.setStorageType(resource.getStorageType());
        clone.setType(resource.getType());

• Files reviewed: 9/9 changed files
• Comments generated: 2
• Review effort level: Lite

---

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

[Copilot]

🟢 Approval recommended

The refactor removes risky merge usage in favor of side-effect-free reads and is supported by focused tests, with only minor maintainability nits remaining.

Review details

• Files reviewed: 11/11 changed files
• Comments generated: 2
• Review effort level: Lite