diff --git a/server/src/main/java/org/eclipse/openvsx/ExtensionService.java b/server/src/main/java/org/eclipse/openvsx/ExtensionService.java index 75f51c05a..656023d8c 100644 --- a/server/src/main/java/org/eclipse/openvsx/ExtensionService.java +++ b/server/src/main/java/org/eclipse/openvsx/ExtensionService.java @@ -218,14 +218,47 @@ public void reactivateExtensions(UserData user) { @Transactional(rollbackOn = ErrorResultException.class) public ResultJson deleteExtension( + UserData user, String namespaceName, String extensionName, - List targetVersions, - UserData user + List targetVersions ) throws ErrorResultException { - // Lock the extension row (NOWAIT) so a delete-all and a concurrent publish can't interleave. - // Publishing takes the same lock (waiting); if the lock can not be acquired, this fails fast, - // and we ask the user to retry. + var extension = lockExtension(namespaceName, extensionName); + if (repositories.isDeleteAllVersions(namespaceName, extensionName, targetVersions, user)) { + return deleteExtension(user, extension); + } + + // A user may only delete versions they published; foreign/unknown versions resolve to null + // and surface as 404 (without leaking that another publisher's version exists). + return deleteExtensionVersions(user, targetVersions.stream() + .map(target -> { + var extVersion = repositories.findVersionForUser(user, target.version(), target.targetPlatform(), extensionName, namespaceName); + if (extVersion == null) throw new ErrorResultException( + "Extension not found: " + NamingUtil.toLogFormat(namespaceName, extensionName, target.targetPlatform(), target.version()), + HttpStatus.NOT_FOUND); + return extVersion; + }) + .toList()); + } + + /** + * Deletes the given pre-resolved extension versions without any ownership check. + * Callers are responsible for authorisation (e.g. scoping the lookup to the owning user + * before calling this, or using an admin-level unscoped lookup). + */ + public ResultJson deleteExtensionVersions(UserData user, List versions) { + var results = new ArrayList(); + for (var version : versions) { + results.add(deleteExtension(user, version)); + } + return combineResults(results); + } + + /** + * Locks the extension row ({@code SELECT … FOR UPDATE NOWAIT}). + * this fails fast with {@code 409} + */ + public Extension lockExtension(String namespaceName, String extensionName) throws ErrorResultException { Extension extension; try { extension = repositories.findExtensionForUpdateNoWait(extensionName, namespaceName); @@ -235,34 +268,26 @@ public ResultJson deleteExtension( + " can not be locked due to concurrent modification. Please try again.", HttpStatus.CONFLICT); } - if (extension == null) { - var message = "Extension not found: " + NamingUtil.toExtensionId(namespaceName, extensionName); - throw new ErrorResultException(message, HttpStatus.NOT_FOUND); - } - - var results = new ArrayList(); - if (repositories.isDeleteAllVersions(namespaceName, extensionName, targetVersions, user)) { - results.add(deleteExtension(user, extension)); - } else { - for (var targetVersion : targetVersions) { - var extVersion = repositories.findVersion(user, targetVersion.version(), targetVersion.targetPlatform(), extensionName, namespaceName); - if (extVersion == null) { - var message = "Extension not found: " + NamingUtil.toLogFormat(namespaceName, extensionName, targetVersion.targetPlatform(), targetVersion.version()); - throw new ErrorResultException(message, HttpStatus.NOT_FOUND); - } - - results.add(deleteExtension(user, extVersion)); - } + throw new ErrorResultException( + "Extension not found: " + NamingUtil.toExtensionId(namespaceName, extensionName), + HttpStatus.NOT_FOUND); } + return extension; + } + /** + * Merges the per-version delete outcomes into a single result, concatenating any success and + * error messages. + */ + public ResultJson combineResults(List results) { var result = new ResultJson(); result.setError(results.stream().map(ResultJson::getError).filter(Objects::nonNull).collect(Collectors.joining("\n"))); result.setSuccess(results.stream().map(ResultJson::getSuccess).filter(Objects::nonNull).collect(Collectors.joining("\n"))); return result; } - protected ResultJson deleteExtension(UserData user, Extension extension) throws ErrorResultException { + public ResultJson deleteExtension(UserData user, Extension extension) throws ErrorResultException { var bundledRefs = repositories.findBundledExtensionsReference(extension); if (!bundledRefs.isEmpty()) { throw new ErrorResultException("Extension " + NamingUtil.toExtensionId(extension) @@ -280,13 +305,14 @@ protected ResultJson deleteExtension(UserData user, Extension extension) throws } cache.evictExtensionJsons(extension); + cache.evictNamespaceDetails(extension); + cache.evictLatestExtensionVersion(extension); for (var extVersion : repositories.findVersions(extension)) { removeExtensionVersion(extVersion); } for (var review : repositories.findAllReviews(extension)) { entityManager.remove(review); } - var deprecatedExtensions = repositories.findDeprecatedExtensions(extension); for (var deprecatedExtension : deprecatedExtensions) { deprecatedExtension.setReplacement(null); @@ -301,7 +327,7 @@ protected ResultJson deleteExtension(UserData user, Extension extension) throws return result; } - protected ResultJson deleteExtension(UserData user, ExtensionVersion extVersion) { + public ResultJson deleteExtension(UserData user, ExtensionVersion extVersion) { var extension = extVersion.getExtension(); removeExtensionVersion(extVersion); extension.getVersions().remove(extVersion); diff --git a/server/src/main/java/org/eclipse/openvsx/UserAPI.java b/server/src/main/java/org/eclipse/openvsx/UserAPI.java index 1803c4846..90820bc2e 100644 --- a/server/src/main/java/org/eclipse/openvsx/UserAPI.java +++ b/server/src/main/java/org/eclipse/openvsx/UserAPI.java @@ -20,7 +20,6 @@ import java.util.LinkedHashMap; import java.util.List; -import java.util.Optional; import java.util.concurrent.TimeUnit; import org.eclipse.openvsx.accesstoken.AccessTokenService; @@ -393,7 +392,7 @@ public ResponseEntity deleteExtension( throw new ResponseStatusException(HttpStatus.FORBIDDEN); } try { - var result = extensions.deleteExtension(namespaceName, extensionName, targetVersions, user); + var result = extensions.deleteExtension(user, namespaceName, extensionName, targetVersions); return ResponseEntity.ok(result); } catch (ErrorResultException exc) { return exc.toResponseEntity(); diff --git a/server/src/main/java/org/eclipse/openvsx/admin/AdminService.java b/server/src/main/java/org/eclipse/openvsx/admin/AdminService.java index 14b4c189c..1eca30c8c 100644 --- a/server/src/main/java/org/eclipse/openvsx/admin/AdminService.java +++ b/server/src/main/java/org/eclipse/openvsx/admin/AdminService.java @@ -18,7 +18,6 @@ import static org.eclipse.openvsx.entities.FileResource.VSIXMANIFEST; import java.time.ZoneId; -import java.util.ArrayList; import java.util.Comparator; import java.util.HashMap; import java.util.LinkedHashSet; @@ -53,7 +52,6 @@ import org.eclipse.openvsx.mail.MailService; import org.eclipse.openvsx.migration.HandlerJobRequest; import org.eclipse.openvsx.repositories.RepositoryService; -import org.eclipse.openvsx.scanning.ExtensionScanPersistenceService; import org.eclipse.openvsx.search.SearchUtilService; import org.eclipse.openvsx.storage.StorageUtilService; import org.eclipse.openvsx.util.ErrorResultException; @@ -90,7 +88,6 @@ public class AdminService { private final JobRequestScheduler scheduler; private final MailService mail; private final LogService logs; - private final ExtensionScanPersistenceService scanPersistenceService; public AdminService( RepositoryService repositories, @@ -105,8 +102,7 @@ public AdminService( CacheService cache, JobRequestScheduler scheduler, MailService mail, - LogService logs, - ExtensionScanPersistenceService scanPersistenceService + LogService logs ) { this.repositories = repositories; this.extensions = extensions; @@ -121,7 +117,6 @@ public AdminService( this.scheduler = scheduler; this.mail = mail; this.logs = logs; - this.scanPersistenceService = scanPersistenceService; } @EventListener @@ -156,28 +151,7 @@ public void deleteExtensionAndDependencies(Extension extension, UserData admin, deleteExtensionAndDependencies(dependRef, admin, depth); } - for (var extVersion : repositories.findVersions(extension)) { - removeExtensionVersion(extVersion); - } - for (var review : repositories.findAllReviews(extension)) { - entityManager.remove(review); - } - - var deprecatedExtensions = repositories.findDeprecatedExtensions(extension); - for (var deprecatedExtension : deprecatedExtensions) { - deprecatedExtension.setReplacement(null); - cache.evictExtensionJsons(deprecatedExtension); - } - - entityManager.remove(extension); - - // evict the cache entries only after the changes have been commited - cache.evictExtensionJsons(extension); - cache.evictNamespaceDetails(extension); - cache.evictLatestExtensionVersion(extension); - - search.removeSearchEntry(extension); - logs.logAction(admin, ResultJson.success("Deleted " + NamingUtil.toExtensionId(extension))); + extensions.deleteExtension(admin, extension); } protected void deleteExtensionAndDependencies(ExtensionVersion extVersion, UserData admin, int depth) { @@ -187,10 +161,15 @@ protected void deleteExtensionAndDependencies(ExtensionVersion extVersion, UserD return; } - removeExtensionVersion(extVersion); - extension.getVersions().remove(extVersion); - extensions.updateExtension(extension); - logs.logAction(admin, ResultJson.success("Deleted " + NamingUtil.toLogFormat(extVersion))); + extensions.deleteExtension(admin, extVersion); + } + + public ResultJson deleteExtension( + UserData adminUser, + String namespaceName, + String extensionName + ) { + return this.deleteExtension(adminUser, namespaceName, extensionName, null); } @Transactional(rollbackOn = ErrorResultException.class) @@ -200,99 +179,21 @@ public ResultJson deleteExtension( String extensionName, List targetVersions ) { + var extension = extensions.lockExtension(namespaceName, extensionName); if (targetVersions == null || repositories.countVersions(namespaceName, extensionName) == targetVersions.size()) { - return deleteExtension(namespaceName, extensionName, adminUser); - } - - var results = new ArrayList(); - for (var targetVersion : targetVersions) { - results.add(deleteExtension(namespaceName, extensionName, targetVersion.targetPlatform(), targetVersion.version(), adminUser)); - } - - var result = new ResultJson(); - result.setError(results.stream().map(ResultJson::getError).filter(Objects::nonNull).collect(Collectors.joining("\n"))); - result.setSuccess(results.stream().map(ResultJson::getSuccess).filter(Objects::nonNull).collect(Collectors.joining("\n"))); - return result; - } - - @Transactional(rollbackOn = ErrorResultException.class) - public ResultJson deleteExtension(String namespaceName, String extensionName, UserData admin) - throws ErrorResultException { - var extension = repositories.findExtension(extensionName, namespaceName); - if (extension == null) { - var extensionId = NamingUtil.toExtensionId(namespaceName, extensionName); - throw new ErrorResultException("Extension not found: " + extensionId, HttpStatus.NOT_FOUND); - } - return deleteExtension(extension, admin); - } - - @Transactional(rollbackOn = ErrorResultException.class) - public ResultJson deleteExtension(String namespaceName, String extensionName, String targetPlatform, String version, UserData admin) - throws ErrorResultException { - var extVersion = repositories.findVersion(version, targetPlatform, extensionName, namespaceName); - if (extVersion == null) { - var message = "Extension not found: " + NamingUtil.toLogFormat(namespaceName, extensionName, targetPlatform, version); - - throw new ErrorResultException(message, HttpStatus.NOT_FOUND); - } - - return deleteExtension(extVersion, admin); - } - - protected ResultJson deleteExtension(Extension extension, UserData admin) throws ErrorResultException { - var bundledRefs = repositories.findBundledExtensionsReference(extension); - if (!bundledRefs.isEmpty()) { - throw new ErrorResultException("Extension " + NamingUtil.toExtensionId(extension) - + " is bundled by the following extension packs: " - + bundledRefs.stream() - .map(NamingUtil::toFileFormat) - .collect(Collectors.joining(", "))); - } - var dependRefs = repositories.findDependenciesReference(extension); - if (!dependRefs.isEmpty()) { - throw new ErrorResultException("The following extensions have a dependency on " + NamingUtil.toExtensionId(extension) + ": " - + dependRefs.stream() - .map(NamingUtil::toFileFormat) - .collect(Collectors.joining(", "))); + return extensions.deleteExtension(adminUser, extension); } - for (var extVersion : repositories.findVersions(extension)) { - removeExtensionVersion(extVersion); - } - - for (var review : repositories.findAllReviews(extension)) { - entityManager.remove(review); - } - - var deprecatedExtensions = repositories.findDeprecatedExtensions(extension); - for (var deprecatedExtension : deprecatedExtensions) { - deprecatedExtension.setReplacement(null); - cache.evictExtensionJsons(deprecatedExtension); - } - - entityManager.remove(extension); - - // evict the cache entries only after the changes have been commited - cache.evictExtensionJsons(extension); - cache.evictNamespaceDetails(extension); - cache.evictLatestExtensionVersion(extension); - - search.removeSearchEntry(extension); - - var result = ResultJson.success("Deleted " + NamingUtil.toExtensionId(extension)); - logs.logAction(admin, result); - return result; - } - - protected ResultJson deleteExtension(ExtensionVersion extVersion, UserData admin) { - var extension = extVersion.getExtension(); - removeExtensionVersion(extVersion); - extension.getVersions().remove(extVersion); - extensions.updateExtension(extension); - - var result = ResultJson.success("Deleted " + NamingUtil.toLogFormat(extVersion)); - logs.logAction(admin, result); - return result; + // Admins may delete any publisher's version, so the unscoped lookup is used here. + return extensions.deleteExtensionVersions(adminUser, targetVersions.stream() + .map(target -> { + var extVersion = repositories.findVersion(target.version(), target.targetPlatform(), extensionName, namespaceName); + if (extVersion == null) throw new ErrorResultException( + "Extension not found: " + NamingUtil.toLogFormat(namespaceName, extensionName, target.targetPlatform(), target.version()), + HttpStatus.NOT_FOUND); + return extVersion; + }) + .toList()); } @Transactional(rollbackOn = ErrorResultException.class) @@ -333,16 +234,6 @@ private ResultJson deleteNamespace(Namespace namespace, UserData admin) { return result; } - private void removeExtensionVersion(ExtensionVersion extVersion) { - // Clean up any pending scan jobs for this extension version - // to prevent "file not found" errors after deletion - scanPersistenceService.deleteScansForExtensionVersion(extVersion.getId()); - - repositories.findFiles(extVersion).map(RemoveFileJobRequest::new).forEach(scheduler::enqueue); - repositories.deleteFiles(extVersion); - entityManager.remove(extVersion); - } - private String userNotFoundMessage(String user) { return "User not found: " + user; } diff --git a/server/src/main/java/org/eclipse/openvsx/migration/FixTargetPlatformsJobRequestHandler.java b/server/src/main/java/org/eclipse/openvsx/migration/FixTargetPlatformsJobRequestHandler.java index cb90fb5af..447f937ab 100644 --- a/server/src/main/java/org/eclipse/openvsx/migration/FixTargetPlatformsJobRequestHandler.java +++ b/server/src/main/java/org/eclipse/openvsx/migration/FixTargetPlatformsJobRequestHandler.java @@ -13,6 +13,7 @@ import org.eclipse.openvsx.ExtensionService; import org.eclipse.openvsx.admin.AdminService; import org.eclipse.openvsx.entities.ExtensionVersion; +import org.eclipse.openvsx.json.TargetPlatformVersionJson; import org.eclipse.openvsx.util.NamingUtil; import org.jobrunr.jobs.annotations.Job; import org.jobrunr.jobs.context.JobRunrDashboardLogger; @@ -23,6 +24,7 @@ import org.springframework.stereotype.Component; import java.nio.file.Files; +import java.util.List; @Component @ConditionalOnProperty(value = "ovsx.data.mirror.enabled", havingValue = "false", matchIfMissing = true) @@ -79,11 +81,10 @@ public void run(MigrationJobRequest jobRequest) throws Exception { private void deleteExtension(ExtensionVersion extVersion) { var extension = extVersion.getExtension(); admins.deleteExtension( + service.getUser(), extension.getNamespace().getName(), extension.getName(), - extVersion.getTargetPlatform(), - extVersion.getVersion(), - service.getUser() + List.of(new TargetPlatformVersionJson(extVersion.getTargetPlatform(), extVersion.getVersion())) ); } } diff --git a/server/src/main/java/org/eclipse/openvsx/mirror/DataMirrorJobRequestHandler.java b/server/src/main/java/org/eclipse/openvsx/mirror/DataMirrorJobRequestHandler.java index 3a6bf8cee..23eda56ba 100644 --- a/server/src/main/java/org/eclipse/openvsx/mirror/DataMirrorJobRequestHandler.java +++ b/server/src/main/java/org/eclipse/openvsx/mirror/DataMirrorJobRequestHandler.java @@ -130,7 +130,7 @@ private void deleteOtherExtensions(List extensionIds, UserData mirrorUse jobContext().logger().info("deleting " + extensionId); try { var namespace = extension.getNamespace(); - admin.deleteExtension(namespace.getName(), extension.getName(), mirrorUser); + admin.deleteExtension(mirrorUser, namespace.getName(), extension.getName()); } catch (ErrorResultException e) { if (e.getStatus() != HttpStatus.NOT_FOUND) { logger.warn("mirror: failed to delete extension {}", extensionId, e); diff --git a/server/src/main/java/org/eclipse/openvsx/mirror/DataMirrorService.java b/server/src/main/java/org/eclipse/openvsx/mirror/DataMirrorService.java index 2b8bc4180..01fdedfc3 100644 --- a/server/src/main/java/org/eclipse/openvsx/mirror/DataMirrorService.java +++ b/server/src/main/java/org/eclipse/openvsx/mirror/DataMirrorService.java @@ -23,6 +23,7 @@ import org.eclipse.openvsx.json.ExtensionJson; import org.eclipse.openvsx.json.NamespaceJson; import org.eclipse.openvsx.json.ReviewJson; +import org.eclipse.openvsx.json.TargetPlatformVersionJson; import org.eclipse.openvsx.json.UserJson; import org.eclipse.openvsx.repositories.RepositoryService; import org.eclipse.openvsx.storage.StorageUtilService; @@ -240,11 +241,10 @@ private void addReview(ReviewJson json, Extension extension) { public void deleteExtensionVersion(ExtensionVersion extVersion, UserData user) { var extension = extVersion.getExtension(); admin.deleteExtension( + user, extension.getNamespace().getName(), extension.getName(), - extVersion.getTargetPlatform(), - extVersion.getVersion(), - user + List.of(new TargetPlatformVersionJson(extVersion.getTargetPlatform(), extVersion.getVersion())) ); } diff --git a/server/src/main/java/org/eclipse/openvsx/repositories/RepositoryService.java b/server/src/main/java/org/eclipse/openvsx/repositories/RepositoryService.java index 5ee335dd0..9542eb9ab 100644 --- a/server/src/main/java/org/eclipse/openvsx/repositories/RepositoryService.java +++ b/server/src/main/java/org/eclipse/openvsx/repositories/RepositoryService.java @@ -253,7 +253,7 @@ public ExtensionVersion findVersion(String version, String targetPlatform, Strin return extensionVersionRepo.findByVersionAndTargetPlatformAndExtensionNameIgnoreCaseAndExtensionNamespaceNameIgnoreCase(version, targetPlatform, extensionName, namespace); } - public ExtensionVersion findVersion(UserData user, String version, String targetPlatform, String extensionName, String namespace) { + public ExtensionVersion findVersionForUser(UserData user, String version, String targetPlatform, String extensionName, String namespace) { return extensionVersionRepo.findByPublishedWithUserAndVersionAndTargetPlatformAndExtensionNameIgnoreCaseAndExtensionNamespaceNameIgnoreCase(user, version, targetPlatform, extensionName, namespace); } diff --git a/server/src/test/java/org/eclipse/openvsx/ExtensionServiceDeleteTest.java b/server/src/test/java/org/eclipse/openvsx/ExtensionDeleteTest.java similarity index 79% rename from server/src/test/java/org/eclipse/openvsx/ExtensionServiceDeleteTest.java rename to server/src/test/java/org/eclipse/openvsx/ExtensionDeleteTest.java index cd266d416..f38a14b74 100644 --- a/server/src/test/java/org/eclipse/openvsx/ExtensionServiceDeleteTest.java +++ b/server/src/test/java/org/eclipse/openvsx/ExtensionDeleteTest.java @@ -10,6 +10,7 @@ package org.eclipse.openvsx; import jakarta.persistence.EntityManager; +import org.eclipse.openvsx.admin.AdminService; import org.eclipse.openvsx.entities.*; import org.eclipse.openvsx.json.TargetPlatformVersionJson; import org.eclipse.openvsx.repositories.RepositoryService; @@ -40,25 +41,27 @@ import static org.mockito.Mockito.doAnswer; /** - * Concurrency test for {@link ExtensionService#deleteExtension}. + * Concurrency tests for delete-all operations in {@link ExtensionService} and + * {@link AdminService}. *

- * This test simulates a race condition where an extension owner deletes all of their published - * versions at the same time as a different publisher commits a brand-new version of the same - * extension. + * Both services expose a delete-all path that first checks how many versions exist + * and then deletes them. Without an extension-row lock, a concurrent publish can slip + * in between the check and the actual deletion and have its newly committed version + * silently removed as a side effect. *

- * Without an extension-row lock, the delete-all operation may accidentally remove the other - * publisher's newly committed version, which is an unauthorized side effect. With the lock, - * the publishing is forced to wait until the delete finishes, preventing any unintended data loss. + * The race condition is deliberately triggered by intercepting the boundary call that + * separates the check from the act (using a spy), pausing the delete operation just + * long enough for the concurrent publish to commit. *

- * The race condition is deliberately triggered by intercepting the call to - * {@link RepositoryService#isDeleteAllVersions} using a spy, which pauses the delete operation - * just long enough for the concurrent publish to slip in. + * {@link ExtensionService#deleteExtension} is protected by a {@code SELECT … FOR UPDATE NOWAIT} + * lock and therefore passes. {@link AdminService#deleteExtension} carries no such lock and + * is expected to fail these tests until the same protection is added. */ @SpringBootTest(webEnvironment = SpringBootTest.WebEnvironment.RANDOM_PORT, properties = { "ovsx.elasticsearch.enabled=false" }) @ActiveProfiles("test_db") -class ExtensionServiceDeleteTest { +class ExtensionDeleteTest { private static final String NAMESPACE = "race-testns"; private static final String EXTENSION = "race-testext"; @@ -74,6 +77,9 @@ class ExtensionServiceDeleteTest { @Autowired ExtensionService extensionService; + @Autowired + AdminService adminService; + @MockitoSpyBean RepositoryService repositories; @@ -127,7 +133,7 @@ void setUp() { } @Test - void deleteExtension_doesNotDeleteAnotherPublishersConcurrentlyAddedVersion() throws Exception { + void userDeleteExtension_doesNotDeleteAnotherPublishersConcurrentlyAddedVersion() throws Exception { // Force a concurrent publish into the window between the check and the actual deletion. doAnswer(invocation -> { var result = invocation.callRealMethod(); @@ -144,7 +150,7 @@ void deleteExtension_doesNotDeleteAnotherPublishersConcurrentlyAddedVersion() th // The owner asks to delete the only version they published. var targets = List.of(new TargetPlatformVersionJson(TargetPlatform.NAME_UNIVERSAL, "1.0.0")); - extensionService.deleteExtension(NAMESPACE, EXTENSION, targets, owner); + extensionService.deleteExtension(owner, NAMESPACE, EXTENSION, targets); // Let the (possibly blocked) publisher run to completion now that the delete committed. if (publisherThread != null) { @@ -161,6 +167,51 @@ void deleteExtension_doesNotDeleteAnotherPublishersConcurrentlyAddedVersion() th .isFalse(); } + /** + * {@link AdminService#deleteExtension} decides whether to delete the whole extension by + * comparing {@code countVersions} against the size of the requested target list. That + * comparison is not protected by any lock, so a concurrent publish that commits between + * the count check and the actual deletion will have its version silently swept away by + * the delete-all. + */ + @Test + void adminDeleteExtension_doesNotDeleteAnotherPublishersConcurrentlyAddedVersion() throws Exception { + // Intercept the count check, which is the boundary between "decide delete-all" and + // "execute delete-all". Starting the concurrent publish here ensures it commits before + // the delete transaction opens, so findVersions will return both versions and both + // will be deleted — demonstrating the race condition. + doAnswer(invocation -> { + var result = invocation.callRealMethod(); + if (raceTriggered.compareAndSet(false, true)) { + startConcurrentPublish(); + publisherFinished.await(RACE_WINDOW_TIMEOUT_SECONDS, TimeUnit.SECONDS); + } + return result; + }).when(repositories).countVersions(any(), any()); + + var admin = new UserData(); + admin.setId(ownerId); + admin.setLoginName(OWNER_LOGIN); + + // The admin asks to delete the only version that currently exists. + var targets = List.of(new TargetPlatformVersionJson(TargetPlatform.NAME_UNIVERSAL, "1.0.0")); + adminService.deleteExtension(admin, NAMESPACE, EXTENSION, targets); + + // Let the (possibly blocked) publisher run to completion now that the delete committed. + if (publisherThread != null) { + publisherThread.join(TimeUnit.SECONDS.toMillis(10)); + } + + var otherVersionExists = versionExists("2.0.0"); + assertThat(publisherSucceeded.get() && !otherVersionExists) + .as("a version successfully published by another user must never be silently " + + "deleted by a concurrent admin delete-all on the same extension") + .isFalse(); + assertThat(otherVersionExists && !extensionExists()) + .as("a committed version must never be left orphaned by a removed extension") + .isFalse(); + } + /** * When another publisher already has a version, deleting your own version must not be treated * as a delete-all: the extension record survives and the other publisher's version stays intact @@ -181,7 +232,7 @@ void deleteExtension_keepsExtensionAndOtherUsersVersionWhenDeletingOwnVersion() // The owner deletes only their own version; this is not a delete-all. var targets = List.of(new TargetPlatformVersionJson(TargetPlatform.NAME_UNIVERSAL, "1.0.0")); - extensionService.deleteExtension(NAMESPACE, EXTENSION, targets, owner); + extensionService.deleteExtension(owner, NAMESPACE, EXTENSION, targets); assertThat(extensionExists()) .as("the extension must survive while another publisher's version remains") @@ -228,7 +279,7 @@ void deleteExtension_failsFastWhenAPublishHoldsTheLock() throws Exception { var targets = List.of(new TargetPlatformVersionJson(TargetPlatform.NAME_UNIVERSAL, "1.0.0")); try { - assertThatThrownBy(() -> extensionService.deleteExtension(NAMESPACE, EXTENSION, targets, owner)) + assertThatThrownBy(() -> extensionService.deleteExtension(owner, NAMESPACE, EXTENSION, targets)) .as("the delete must fail fast (not block) while a publish holds the lock") .isInstanceOf(ErrorResultException.class); } finally { diff --git a/server/src/test/java/org/eclipse/openvsx/UserAPITest.java b/server/src/test/java/org/eclipse/openvsx/UserAPITest.java index 4b9c8796f..55355742e 100644 --- a/server/src/test/java/org/eclipse/openvsx/UserAPITest.java +++ b/server/src/test/java/org/eclipse/openvsx/UserAPITest.java @@ -723,7 +723,7 @@ private List mockExtension(UserData user, int numberOfVersions versions.add(extVersion); Mockito.when(repositories.findFiles(extVersion)) .thenReturn(Streamable.empty()); - Mockito.when(repositories.findVersion(user, extVersion.getVersion(), TargetPlatform.NAME_UNIVERSAL, "baz", "foobar")) + Mockito.when(repositories.findVersionForUser(user, extVersion.getVersion(), TargetPlatform.NAME_UNIVERSAL, "baz", "foobar")) .thenReturn(extVersion); } diff --git a/server/src/test/java/org/eclipse/openvsx/admin/AdminAPITest.java b/server/src/test/java/org/eclipse/openvsx/admin/AdminAPITest.java index aebc46752..3639c2d10 100644 --- a/server/src/test/java/org/eclipse/openvsx/admin/AdminAPITest.java +++ b/server/src/test/java/org/eclipse/openvsx/admin/AdminAPITest.java @@ -1743,6 +1743,8 @@ private List mockExtension(int numberOfVersions, int numberOfB Mockito.when(entityManager.merge(extension)).thenReturn(extension); Mockito.when(repositories.findExtension("baz", "foobar")) .thenReturn(extension); + Mockito.when(repositories.findExtensionForUpdateNoWait("baz", "foobar")) + .thenReturn(extension); Mockito.when(repositories.findExtensions(namespace)) .thenReturn(Streamable.of(extension)); @@ -1962,8 +1964,7 @@ AdminService adminService( CacheService cache, JobRequestScheduler scheduler, MailService mail, - LogService logs, - ExtensionScanPersistenceService scanPersistenceService + LogService logs ) { return new AdminService( repositories, @@ -1978,8 +1979,7 @@ AdminService adminService( cache, scheduler, mail, - logs, - scanPersistenceService + logs ); } diff --git a/server/src/test/java/org/eclipse/openvsx/cache/CacheServiceTest.java b/server/src/test/java/org/eclipse/openvsx/cache/CacheServiceTest.java index 60e01466f..7a9ba485b 100644 --- a/server/src/test/java/org/eclipse/openvsx/cache/CacheServiceTest.java +++ b/server/src/test/java/org/eclipse/openvsx/cache/CacheServiceTest.java @@ -18,6 +18,7 @@ import org.eclipse.openvsx.entities.*; import org.eclipse.openvsx.json.ExtensionJson; import org.eclipse.openvsx.json.ReviewJson; +import org.eclipse.openvsx.json.TargetPlatformVersionJson; import org.eclipse.openvsx.repositories.RepositoryService; import org.eclipse.openvsx.security.IdPrincipal; import org.eclipse.openvsx.util.TempFile; @@ -229,7 +230,7 @@ void testDeleteExtension() throws IOException { registry.getExtension(namespace.getName(), extension.getName(), extVersion.getTargetPlatform(), extVersion.getVersion()); - admins.deleteExtension(namespace.getName(), extension.getName(), admin); + admins.deleteExtension(admin, namespace.getName(), extension.getName(), null); assertNull(cache.getCache(CACHE_EXTENSION_JSON).get(cacheKey, ExtensionJson.class)); } } @@ -254,7 +255,7 @@ void testDeleteExtensionVersion() throws IOException { assertTrue(json.getAllVersions().containsKey(newVersion)); assertTrue(json.getAllVersions().containsKey(oldVersion)); - admins.deleteExtension(namespace.getName(), extension.getName(), extVersion.getTargetPlatform(), newVersion, admin); + admins.deleteExtension(admin, namespace.getName(), extension.getName(), List.of(new TargetPlatformVersionJson(extVersion.getTargetPlatform(), newVersion))); assertNull(cache.getCache(CACHE_EXTENSION_JSON).get(cacheKey, ExtensionJson.class)); entityManager.flush(); diff --git a/server/src/test/java/org/eclipse/openvsx/repositories/RepositoryServiceSmokeTest.java b/server/src/test/java/org/eclipse/openvsx/repositories/RepositoryServiceSmokeTest.java index 8880209c2..6f760ac88 100644 --- a/server/src/test/java/org/eclipse/openvsx/repositories/RepositoryServiceSmokeTest.java +++ b/server/src/test/java/org/eclipse/openvsx/repositories/RepositoryServiceSmokeTest.java @@ -306,7 +306,7 @@ void testExecuteQueries() { () -> repositories.findNotMigratedItems(page), () -> repositories.findRemoveFileResourceTypeResourceMigrationItems(0, 1), () -> repositories.findTargetPlatformsGroupedByVersion(extension, userData), - () -> repositories.findVersion(userData,"version", "targetPlatform", "extensionName", "namespace"), + () -> repositories.findVersionForUser(userData,"version", "targetPlatform", "extensionName", "namespace"), () -> repositories.findLatestVersion(userData, "namespaceName", "extensionName"), () -> repositories.isDeleteAllVersions("namespaceName", "extensionName", Collections.emptyList(), userData), () -> repositories.deactivateAccessTokens(userData), diff --git a/webui/CHANGELOG.md b/webui/CHANGELOG.md index e6609f384..d87e60289 100644 --- a/webui/CHANGELOG.md +++ b/webui/CHANGELOG.md @@ -13,6 +13,10 @@ This change log covers only the frontend library (webui) of Open VSX. - Migrate admin dashboard to use `@tanstack/react-query` ([#1917](https://github.com/eclipse-openvsx/openvsx/pull/1917) - Replace formatting from `stylistic` with `prettier` ([#1916](https://github.com/eclipse-openvsx/openvsx/pull/1916)) +### Fixed + +- Refresh the extension version list in the delete views when a delete fails with a conflict + ## [v1.0.2] (23/06/2026) ### Changed diff --git a/webui/src/context.ts b/webui/src/context.ts index deae63d70..17a28e2d4 100644 --- a/webui/src/context.ts +++ b/webui/src/context.ts @@ -17,7 +17,7 @@ import { ErrorResponse } from './server-request'; export interface MainContext { service: ExtensionRegistryService; pageSettings: PageSettings; - handleError: (err: Error | Partial) => void; + handleError: (err: Error | Partial, options?: { onClose?: () => void }) => void; user?: UserData; updateUser: () => void; loginProviders?: Record; diff --git a/webui/src/main.tsx b/webui/src/main.tsx index 262734e21..c6dd41bf2 100644 --- a/webui/src/main.tsx +++ b/webui/src/main.tsx @@ -37,6 +37,8 @@ export const Main: FunctionComponent = props => { const [error, setError] = useState<{ message: string; code?: number | string }>(); const [isErrorDialogOpen, setIsErrorDialogOpen] = useState(false); const abortController = useRef(new AbortController()); + // Optional callback to run when the error dialog is dismissed (e.g. re-fetch stale data). + const errorDialogOnClose = useRef<(() => void) | undefined>(undefined); useEffect(() => { getLoginProviders(); @@ -83,7 +85,7 @@ export const Main: FunctionComponent = props => { } }; - const onError = (err: Error | Partial | ReportedError) => { + const onError = (err: Error | Partial | ReportedError, options?: { onClose?: () => void }) => { if (err instanceof DOMException && err.message.trim() === 'The operation was aborted.') { // ignore error caused by AbortController.abort() return; @@ -91,12 +93,16 @@ export const Main: FunctionComponent = props => { const message = handleError(err); const code = (err as ReportedError).code; + errorDialogOnClose.current = options?.onClose; setError({ message, code }); setIsErrorDialogOpen(true); }; const onErrorDialogClose = () => { setIsErrorDialogOpen(false); + const onClose = errorDialogOnClose.current; + errorDialogOnClose.current = undefined; + onClose?.(); }; const renderPageContent = (): ReactNode => { diff --git a/webui/src/pages/admin-dashboard/extension-admin.tsx b/webui/src/pages/admin-dashboard/extension-admin.tsx index d497562e2..355675308 100644 --- a/webui/src/pages/admin-dashboard/extension-admin.tsx +++ b/webui/src/pages/admin-dashboard/extension-admin.tsx @@ -82,15 +82,20 @@ export const ExtensionAdmin: FunctionComponent = () => { return; } - await deleteExtension({ - namespace: extension.namespace, - extension: extension.name, - targetPlatformVersions: targetPlatformVersions?.map(({ version, targetPlatform }) => ({ - version, - targetPlatform - })) - }); - await refetch(); + try { + await deleteExtension({ + namespace: extension.namespace, + extension: extension.name, + targetPlatformVersions: targetPlatformVersions?.map(({ version, targetPlatform }) => ({ + version, + targetPlatform + })) + }); + await refetch(); + } catch (err) { + const isConflict = (err as { status?: number }).status === 409; + handleError(err, isConflict ? { onClose: () => refetch() } : undefined); + } }; return ( diff --git a/webui/src/pages/admin-dashboard/extension-remove-dialog.tsx b/webui/src/pages/admin-dashboard/extension-remove-dialog.tsx index fa2d6308c..8b9ccdd7f 100644 --- a/webui/src/pages/admin-dashboard/extension-remove-dialog.tsx +++ b/webui/src/pages/admin-dashboard/extension-remove-dialog.tsx @@ -37,11 +37,13 @@ export const ExtensionRemoveDialog: FunctionComponent ({ - version, - targetPlatform - })) - }); - await findExtension(); + try { + await service.deleteExtensions(abortController.current, { + namespace: extension.namespace, + extension: extension.name, + targetPlatformVersions: targetPlatformVersions?.map(({ version, targetPlatform }) => ({ + version, + targetPlatform + })) + }); + await findExtension(); + } catch (err) { + const isConflict = (err as { status?: number }).status === 409; + handleError(err, isConflict ? { onClose: () => findExtension() } : undefined); + } }; return (