diff --git a/server/src/main/java/org/eclipse/openvsx/accesstoken/AccessTokenService.java b/server/src/main/java/org/eclipse/openvsx/accesstoken/AccessTokenService.java index a3b821f6f..6a56bdb54 100644 --- a/server/src/main/java/org/eclipse/openvsx/accesstoken/AccessTokenService.java +++ b/server/src/main/java/org/eclipse/openvsx/accesstoken/AccessTokenService.java @@ -377,12 +377,17 @@ public int expireAccessTokens() { @Transactional public void scheduleTokenExpirationNotification(PersonalAccessToken token) { - token = entityManager.merge(token); - if (token.getType().isNotify() && !token.isNotified()) { + // find, not merge: only `notified` is this method's to change, and merging the whole + // detached token reverted any column that moved since it was loaded - see #989. + var managedToken = entityManager.find(PersonalAccessToken.class, token.getId()); + if (managedToken == null) { + return; + } + if (managedToken.getType().isNotify() && !managedToken.isNotified()) { try { - mail.scheduleAccessTokenExpiryNotification(token); + mail.scheduleAccessTokenExpiryNotification(managedToken); } finally { - token.setNotified(true); + managedToken.setNotified(true); } } } diff --git a/server/src/main/java/org/eclipse/openvsx/admin/ChangeNamespaceService.java b/server/src/main/java/org/eclipse/openvsx/admin/ChangeNamespaceService.java index 78dad75f5..40d0c38b7 100644 --- a/server/src/main/java/org/eclipse/openvsx/admin/ChangeNamespaceService.java +++ b/server/src/main/java/org/eclipse/openvsx/admin/ChangeNamespaceService.java @@ -61,16 +61,24 @@ public void changeNamespaceInDatabase( if (createNewNamespace) { entityManager.persist(newNamespace); } else { + // Deliberately a merge, unlike the rest of #989's call sites: the caller mutates this + // detached namespace before handing it over (ChangeNamespaceJobRequestHandler sets + // logoName on it), so the update belongs to the caller and only merge can carry it. + // find-then-set would silently drop that rename. newNamespace = entityManager.merge(newNamespace); } changeExtensionNamespace(extensions, newNamespace); changeMembershipNamespace(oldNamespace, newNamespace, removeOldNamespace); - updatedResources.forEach(entityManager::merge); + renameResources(updatedResources); if (removeOldNamespace) { - oldNamespace = entityManager.merge(oldNamespace); - entityManager.remove(oldNamespace); + // find, not merge: this row is about to go, so merging every column of a detached copy + // first was a wasted UPDATE - and would have resurrected a row already deleted. + var managedOldNamespace = entityManager.find(Namespace.class, oldNamespace.getId()); + if (managedOldNamespace != null) { + entityManager.remove(managedOldNamespace); + } } cache.evictSitemap(); @@ -78,6 +86,20 @@ public void changeNamespaceInDatabase( search.updateSearchEntries(extensions.filter(Extension::isActive).toList()); } + /** + * Applies the names the caller computed for the copied resources. Only the name changes there + * (see {@code ChangeNamespaceJobRequestHandler}), so this writes that field rather than merging + * every column of each detached resource back - see #989. + */ + private void renameResources(List updatedResources) { + for (var resource : updatedResources) { + var managedResource = entityManager.find(FileResource.class, resource.getId()); + if (managedResource != null) { + managedResource.setName(resource.getName()); + } + } + } + private void changeExtensionNamespace(Streamable extensions, Namespace newNamespace) { for (var extension : extensions) { // findExtensions ran inside changeNamespaceInDatabase's transaction, so these are diff --git a/server/src/main/java/org/eclipse/openvsx/eclipse/EclipseService.java b/server/src/main/java/org/eclipse/openvsx/eclipse/EclipseService.java index 800dc617c..f09522e24 100644 --- a/server/src/main/java/org/eclipse/openvsx/eclipse/EclipseService.java +++ b/server/src/main/java/org/eclipse/openvsx/eclipse/EclipseService.java @@ -195,8 +195,14 @@ public EclipseProfile getPublicProfile(String personId) { */ @Transactional public void updateUserData(UserData user, EclipseProfile profile) { - user = entityManager.merge(user); - user.setEclipsePersonId(profile.getName()); + // find-then-set rather than merge: merge writes every column of the detached copy, so + // anything that changed on the row since it was loaded gets reverted. Only the field + // below is this method's to change - see #989. + var managedUser = entityManager.find(UserData.class, user.getId()); + if (managedUser == null) { + return; + } + managedUser.setEclipsePersonId(profile.getName()); } public void enrichUserJsonWithPublisherAgreement(UserJson json, UserData user) { diff --git a/server/src/main/java/org/eclipse/openvsx/migration/CheckPotentiallyMaliciousExtensionVersionsService.java b/server/src/main/java/org/eclipse/openvsx/migration/CheckPotentiallyMaliciousExtensionVersionsService.java index 667b758d0..00090cf5b 100644 --- a/server/src/main/java/org/eclipse/openvsx/migration/CheckPotentiallyMaliciousExtensionVersionsService.java +++ b/server/src/main/java/org/eclipse/openvsx/migration/CheckPotentiallyMaliciousExtensionVersionsService.java @@ -33,9 +33,9 @@ public CheckPotentiallyMaliciousExtensionVersionsService(EntityManager entityMan @Transactional public void checkPotentiallyMaliciousExtensionVersion(ExtensionVersion extVersion, TempFile extensionFile) { + boolean isMalicious; try (var extProcessor = new ExtensionProcessor(extensionFile)) { - boolean isMalicious = extProcessor.isPotentiallyMalicious(); - extVersion.setPotentiallyMalicious(isMalicious); + isMalicious = extProcessor.isPotentiallyMalicious(); if (isMalicious) { logger.atWarn() .setMessage("Extension version is potentially malicious: {}") @@ -43,6 +43,14 @@ public void checkPotentiallyMaliciousExtensionVersion(ExtensionVersion extVersio .log(); } } - entityManager.merge(extVersion); + + // find-then-set rather than merge: merge writes every column of the detached copy, so + // anything that changed on the row since it was loaded gets reverted. Only the field + // below is this method's to change - see #989. + var managedVersion = entityManager.find(ExtensionVersion.class, extVersion.getId()); + if (managedVersion == null) { + return; + } + managedVersion.setPotentiallyMalicious(isMalicious); } } diff --git a/server/src/main/java/org/eclipse/openvsx/migration/FileResourceSizeJobRequestHandler.java b/server/src/main/java/org/eclipse/openvsx/migration/FileResourceSizeJobRequestHandler.java index 51c8d2e96..758b08755 100644 --- a/server/src/main/java/org/eclipse/openvsx/migration/FileResourceSizeJobRequestHandler.java +++ b/server/src/main/java/org/eclipse/openvsx/migration/FileResourceSizeJobRequestHandler.java @@ -73,7 +73,7 @@ public void run(MigrationJobRequest jobRequest) throws Exception { try { resource.setSize(migrations.getFileSize(resource)); - migrations.updateResource(resource); + migrations.updateResourceSize(resource); } catch (FileNotFoundInStorageException e) { // The object backing this resource is confirmed gone, not just temporarily // unreachable -- there's nothing further this job can do about that diff --git a/server/src/main/java/org/eclipse/openvsx/migration/MigrationService.java b/server/src/main/java/org/eclipse/openvsx/migration/MigrationService.java index d3c14a150..f4e119d46 100644 --- a/server/src/main/java/org/eclipse/openvsx/migration/MigrationService.java +++ b/server/src/main/java/org/eclipse/openvsx/migration/MigrationService.java @@ -126,9 +126,18 @@ public FileResource getResource(MigrationJobRequest jobRequest) { return entityManager.find(FileResource.class, jobRequest.getEntityId()); } + /** + * Persists the size its caller determined for {@code resource}, and nothing else. Named for the + * one field it writes: a generic "update this resource" could only be implemented by merging the + * whole detached copy, which reverts any column that changed since it was loaded (#989). + */ @Transactional - public void updateResource(FileResource resource) { - entityManager.merge(resource); + public void updateResourceSize(FileResource resource) { + var managedResource = entityManager.find(FileResource.class, resource.getId()); + if (managedResource == null) { + return; + } + managedResource.setSize(resource.getSize()); } @Retryable @@ -161,8 +170,10 @@ public void persistFileResource(FileResource resource) { @Transactional public void deleteFileResource(FileResource resource) { - resource = entityManager.merge(resource); - entityManager.remove(resource); + var managedResource = entityManager.find(FileResource.class, resource.getId()); + if (managedResource != null) { + entityManager.remove(managedResource); + } } public FileResource getFileResource(ExtensionVersion extVersion, String type) { diff --git a/server/src/main/java/org/eclipse/openvsx/migration/RenameDownloadsJobRequestHandler.java b/server/src/main/java/org/eclipse/openvsx/migration/RenameDownloadsJobRequestHandler.java index 88783d387..7991982ef 100644 --- a/server/src/main/java/org/eclipse/openvsx/migration/RenameDownloadsJobRequestHandler.java +++ b/server/src/main/java/org/eclipse/openvsx/migration/RenameDownloadsJobRequestHandler.java @@ -54,7 +54,7 @@ public void run(MigrationJobRequest jobRequest) throws Exception { migrations.removeFile(download); download.setName(name); - service.updateResource(download); + service.updateResourceName(download); } logger.info("Updated download name to: {}", name); diff --git a/server/src/main/java/org/eclipse/openvsx/migration/RenameDownloadsService.java b/server/src/main/java/org/eclipse/openvsx/migration/RenameDownloadsService.java index 93e41b23c..a9d6fd6e8 100644 --- a/server/src/main/java/org/eclipse/openvsx/migration/RenameDownloadsService.java +++ b/server/src/main/java/org/eclipse/openvsx/migration/RenameDownloadsService.java @@ -43,8 +43,16 @@ public FileResource cloneResource(FileResource resource, String name) { return clone; } + /** + * Persists the new name its caller set on {@code resource}, and nothing else - see + * {@code MigrationService#updateResourceSize} for why this is named for its field. + */ @Transactional - public void updateResource(FileResource resource) { - entityManager.merge(resource); + public void updateResourceName(FileResource resource) { + var managedResource = entityManager.find(FileResource.class, resource.getId()); + if (managedResource == null) { + return; + } + managedResource.setName(resource.getName()); } } diff --git a/server/src/main/java/org/eclipse/openvsx/migration/SetPreReleaseJobService.java b/server/src/main/java/org/eclipse/openvsx/migration/SetPreReleaseJobService.java index 479b128e7..549977d65 100644 --- a/server/src/main/java/org/eclipse/openvsx/migration/SetPreReleaseJobService.java +++ b/server/src/main/java/org/eclipse/openvsx/migration/SetPreReleaseJobService.java @@ -44,11 +44,21 @@ public List getExtensionVersions(MigrationJobRequest jobReques @Transactional public void updatePreviewAndPreRelease(ExtensionVersion extVersion, TempFile extensionFile) { + boolean preRelease; + boolean preview; try (var extProcessor = new ExtensionProcessor(extensionFile)) { - extVersion.setPreRelease(extProcessor.isPreRelease()); - extVersion.setPreview(extProcessor.isPreview()); + preRelease = extProcessor.isPreRelease(); + preview = extProcessor.isPreview(); } - entityManager.merge(extVersion); + // find-then-set rather than merge: merge writes every column of the detached copy, so + // anything that changed on the row since it was loaded gets reverted. Only the field + // below is this method's to change - see #989. + var managedVersion = entityManager.find(ExtensionVersion.class, extVersion.getId()); + if (managedVersion == null) { + return; + } + managedVersion.setPreRelease(preRelease); + managedVersion.setPreview(preview); } } diff --git a/server/src/main/java/org/eclipse/openvsx/publish/ExtensionVersionIntegrityService.java b/server/src/main/java/org/eclipse/openvsx/publish/ExtensionVersionIntegrityService.java index 3d0c3fea8..9f0592868 100644 --- a/server/src/main/java/org/eclipse/openvsx/publish/ExtensionVersionIntegrityService.java +++ b/server/src/main/java/org/eclipse/openvsx/publish/ExtensionVersionIntegrityService.java @@ -97,8 +97,14 @@ public boolean verifyExtensionVersion(TempFile extensionFile, TempFile signature @Transactional public void setSignatureKeyPair(ExtensionVersion extVersion, SignatureKeyPair keyPair) { - extVersion = entityManager.merge(extVersion); - extVersion.setSignatureKeyPair(keyPair); + // find-then-set rather than merge: merge writes every column of the detached copy, so + // anything that changed on the row since it was loaded gets reverted. Only the field + // below is this method's to change - see #989. + var managedVersion = entityManager.find(ExtensionVersion.class, extVersion.getId()); + if (managedVersion == null) { + return; + } + managedVersion.setSignatureKeyPair(keyPair); } public TempFile generateSignature(TempFile extensionFile, SignatureKeyPair keyPair) throws IOException { diff --git a/server/src/main/java/org/eclipse/openvsx/publish/PublishExtensionVersionService.java b/server/src/main/java/org/eclipse/openvsx/publish/PublishExtensionVersionService.java index 620544826..928c30f45 100644 --- a/server/src/main/java/org/eclipse/openvsx/publish/PublishExtensionVersionService.java +++ b/server/src/main/java/org/eclipse/openvsx/publish/PublishExtensionVersionService.java @@ -91,8 +91,14 @@ public void persistResource(FileResource resource) { @Transactional public void markExtensionAsPotentiallyMalicious(ExtensionVersion extVersion) { - extVersion = entityManager.merge(extVersion); - extVersion.setPotentiallyMalicious(true); + // find-then-set rather than merge: merge writes every column of the detached copy, so + // anything that changed on the row since it was loaded gets reverted. Only the field + // below is this method's to change - see #989. + var managedVersion = entityManager.find(ExtensionVersion.class, extVersion.getId()); + if (managedVersion == null) { + return; + } + managedVersion.setPotentiallyMalicious(true); } @Transactional diff --git a/server/src/main/java/org/eclipse/openvsx/ratelimit/CustomerService.java b/server/src/main/java/org/eclipse/openvsx/ratelimit/CustomerService.java index f939832dc..5215542b2 100644 --- a/server/src/main/java/org/eclipse/openvsx/ratelimit/CustomerService.java +++ b/server/src/main/java/org/eclipse/openvsx/ratelimit/CustomerService.java @@ -193,8 +193,17 @@ private String generateTokenValue() { @Transactional public ResultJson deactivateRateLimitToken(RateLimitToken token) { - token = entityManager.merge(token); - token.setActive(false); - return ResultJson.success("Deactivated rate limit token for customer " + token.getCustomer().getName() + "."); + // find-then-set rather than merge: merge writes every column of the detached copy, so + // anything that changed on the row since it was loaded gets reverted. Only the field + // below is this method's to change - see #989. + var managedToken = entityManager.find(RateLimitToken.class, token.getId()); + if (managedToken == null) { + // RateLimitAPI already established the token exists before calling, so this only + // happens if the row went away in between; matches how this class reports a missing row. + throw new ErrorResultException("Rate limit token not found: " + token.getId()); + } + managedToken.setActive(false); + return ResultJson + .success("Deactivated rate limit token for customer " + managedToken.getCustomer().getName() + "."); } } diff --git a/server/src/test/java/org/eclipse/openvsx/migration/FileResourceSizeJobRequestHandlerTest.java b/server/src/test/java/org/eclipse/openvsx/migration/FileResourceSizeJobRequestHandlerTest.java index 3f88b0dbd..92a2510e8 100644 --- a/server/src/test/java/org/eclipse/openvsx/migration/FileResourceSizeJobRequestHandlerTest.java +++ b/server/src/test/java/org/eclipse/openvsx/migration/FileResourceSizeJobRequestHandlerTest.java @@ -58,8 +58,8 @@ void run_recordsSizeForEveryUnsizedResourceOfTheExtensionVersionAndSkipsAlreadyS new FileResourceSizeJobRequestHandler(migrations).run(jobRequest); - verify(migrations).updateResource(unsized); - verify(migrations, never()).updateResource(alreadySized); + verify(migrations).updateResourceSize(unsized); + verify(migrations, never()).updateResourceSize(alreadySized); verify(migrations, never()).getFileSize(alreadySized); } @@ -78,8 +78,8 @@ void run_skipsAMissingResourceButStillProcessesTheRestOfTheExtensionVersion() th assertThatCode(() -> handler.run(jobRequest)).doesNotThrowAnyException(); - verify(migrations, never()).updateResource(missing); - verify(migrations).updateResource(present); + verify(migrations, never()).updateResourceSize(missing); + verify(migrations).updateResourceSize(present); } private ExtensionVersion extVersion() { diff --git a/server/src/test/java/org/eclipse/openvsx/publish/PublishExtensionVersionServiceTest.java b/server/src/test/java/org/eclipse/openvsx/publish/PublishExtensionVersionServiceTest.java index 9659740b6..268e95fc0 100644 --- a/server/src/test/java/org/eclipse/openvsx/publish/PublishExtensionVersionServiceTest.java +++ b/server/src/test/java/org/eclipse/openvsx/publish/PublishExtensionVersionServiceTest.java @@ -179,6 +179,51 @@ void mirrorResource_recordsTheSizeOfTheExtractedBytes() throws Exception { verify(entityManager).persist(resource); } + // #989: markExtensionAsPotentiallyMalicious used to merge the whole detached extVersion to set + // one boolean, so any column that had moved since the caller loaded it was reverted - the same + // failure mode that once left published extensions inactive. It now writes the flag on the + // managed row, matching what activateExtension above already did. + @Test + void markExtensionAsPotentiallyMalicious_flagsTheManagedRow() { + var stale = version(1L, false); + var managed = version(1L, false); + when(entityManager.find(ExtensionVersion.class, 1L)).thenReturn(managed); + + svc.markExtensionAsPotentiallyMalicious(stale); + + assertThat(managed.isPotentiallyMalicious()).isTrue(); + verify(entityManager, never()).merge(any()); + } + + @Test + void markExtensionAsPotentiallyMalicious_leavesConcurrentChangesAlone() { + // The caller's copy was loaded while the version was still inactive; it has since been + // activated. Merging that stale copy back would have deactivated it again. + var stale = version(1L, false); + stale.setActive(false); + var managed = version(1L, false); + managed.setActive(true); + when(entityManager.find(ExtensionVersion.class, 1L)).thenReturn(managed); + + svc.markExtensionAsPotentiallyMalicious(stale); + + assertThat(managed.isPotentiallyMalicious()).isTrue(); + assertThat(managed.isActive()).as("a concurrent activation must survive the flag write").isTrue(); + } + + // The row can be purged between the caller loading it and this running; merge would have tried + // to resurrect it rather than doing nothing. + @Test + void markExtensionAsPotentiallyMalicious_ignoresAVersionThatIsGone() { + var stale = version(1L, false); + when(entityManager.find(ExtensionVersion.class, 1L)).thenReturn(null); + + svc.markExtensionAsPotentiallyMalicious(stale); + + verify(entityManager, never()).merge(any()); + verify(entityManager, never()).persist(any()); + } + private ExtensionVersion version(long id, boolean removed) { var namespace = new Namespace(); namespace.setName("redhat");