fix(governance): safely delete version dependencies

Remove terminal review tasks before deleting an allowed skill version. Lock all versions of the aggregate in stable order so concurrent deletes preserve the last-version invariant and return business errors instead of 500 responses.

Signed-off-by: XiaoSeS <87064762+XiaoSeS@users.noreply.github.com>
This commit is contained in:
XiaoSeS 2026-07-30 17:52:08 +08:00
parent 6817d98007
commit b264f3a0d2
10 changed files with 308 additions and 13 deletions

View file

@ -98,7 +98,7 @@ public class SkillLifecycleAppService {
Map<Long, NamespaceRole> userNamespaceRoles,
AuditRequestContext auditContext) {
Skill skill = findSkill(namespace, slug, userId);
SkillVersion skillVersion = findVersion(skill.getId(), version);
SkillVersion skillVersion = findVersionForUpdate(skill.getId(), version);
skillGovernanceService.deleteVersion(
skill,
skillVersion,
@ -261,6 +261,13 @@ public class SkillLifecycleAppService {
.orElseThrow(() -> new DomainBadRequestException("error.skill.version.notFound", version));
}
private SkillVersion findVersionForUpdate(Long skillId, String version) {
return skillVersionRepository.findBySkillIdForUpdate(skillId).stream()
.filter(candidate -> candidate.getVersion().equals(version))
.findFirst()
.orElseThrow(() -> new DomainBadRequestException("error.skill.version.notFound", version));
}
private Map<Long, NamespaceRole> normalizeRoles(Map<Long, NamespaceRole> userNamespaceRoles) {
return userNamespaceRoles != null ? userNamespaceRoles : Map.of();
}

View file

@ -144,7 +144,8 @@ class SkillLifecycleControllerTest {
given(namespaceRepository.findBySlug("global")).willReturn(java.util.Optional.of(namespace));
given(skillSlugResolutionService.resolve(1L, "demo-skill", "usr_1", SkillSlugResolutionService.Preference.CURRENT_USER))
.willReturn(skill);
given(skillVersionRepository.findBySkillIdAndVersion(1L, "1.0.0")).willReturn(java.util.Optional.of(version));
given(skillVersionRepository.findBySkillIdForUpdate(1L))
.willReturn(java.util.List.of(version));
mockMvc.perform(delete("/api/web/skills/global/demo-skill/versions/1.0.0")
.requestAttr("userId", "usr_1")

View file

@ -0,0 +1,144 @@
package com.iflytek.skillhub.controller.portal;
import static org.assertj.core.api.Assertions.assertThat;
import static org.mockito.ArgumentMatchers.argThat;
import static org.mockito.Mockito.verify;
import static org.springframework.security.test.web.servlet.request.SecurityMockMvcRequestPostProcessors.authentication;
import static org.springframework.security.test.web.servlet.request.SecurityMockMvcRequestPostProcessors.csrf;
import static org.springframework.test.web.servlet.request.MockMvcRequestBuilders.delete;
import static org.springframework.test.web.servlet.result.MockMvcResultMatchers.jsonPath;
import static org.springframework.test.web.servlet.result.MockMvcResultMatchers.status;
import com.iflytek.skillhub.TestRedisConfig;
import com.iflytek.skillhub.auth.device.DeviceAuthService;
import com.iflytek.skillhub.auth.rbac.PlatformPrincipal;
import com.iflytek.skillhub.domain.namespace.Namespace;
import com.iflytek.skillhub.domain.namespace.NamespaceMemberRepository;
import com.iflytek.skillhub.domain.namespace.NamespaceRepository;
import com.iflytek.skillhub.domain.review.ReviewTask;
import com.iflytek.skillhub.domain.review.ReviewTaskRepository;
import com.iflytek.skillhub.domain.review.ReviewTaskStatus;
import com.iflytek.skillhub.domain.skill.Skill;
import com.iflytek.skillhub.domain.skill.SkillRepository;
import com.iflytek.skillhub.domain.skill.SkillVersion;
import com.iflytek.skillhub.domain.skill.SkillVersionRepository;
import com.iflytek.skillhub.domain.skill.SkillVersionStatus;
import com.iflytek.skillhub.domain.skill.SkillVisibility;
import com.iflytek.skillhub.storage.ObjectStorageService;
import java.util.Arrays;
import java.util.List;
import java.util.Set;
import java.util.UUID;
import org.junit.jupiter.api.Test;
import org.springframework.beans.factory.annotation.Autowired;
import org.springframework.boot.test.autoconfigure.web.servlet.AutoConfigureMockMvc;
import org.springframework.boot.test.context.SpringBootTest;
import org.springframework.boot.test.mock.mockito.MockBean;
import org.springframework.context.annotation.Import;
import org.springframework.security.authentication.UsernamePasswordAuthenticationToken;
import org.springframework.security.core.authority.SimpleGrantedAuthority;
import org.springframework.test.context.ActiveProfiles;
import org.springframework.test.web.servlet.MockMvc;
@SpringBootTest
@AutoConfigureMockMvc
@ActiveProfiles("test")
@Import(TestRedisConfig.class)
class SkillVersionDeleteFlowIntegrationTest {
@Autowired
private MockMvc mockMvc;
@Autowired
private NamespaceRepository namespaceRepository;
@Autowired
private SkillRepository skillRepository;
@Autowired
private SkillVersionRepository skillVersionRepository;
@Autowired
private ReviewTaskRepository reviewTaskRepository;
@MockBean
private ObjectStorageService objectStorageService;
@MockBean
private NamespaceMemberRepository namespaceMemberRepository;
@MockBean
private DeviceAuthService deviceAuthService;
@Test
void deleteRejectedVersion_removesOnlyItsReviewHistory() throws Exception {
String ownerId = "owner-1";
String suffix = UUID.randomUUID().toString().substring(0, 8);
Namespace namespace = namespaceRepository.save(
new Namespace("version-delete-" + suffix, "Version Delete " + suffix, ownerId)
);
Skill skill = new Skill(namespace.getId(), "demo-skill-" + suffix, ownerId, SkillVisibility.PUBLIC);
skill.setCreatedBy(ownerId);
skill.setUpdatedBy(ownerId);
skill = skillRepository.save(skill);
SkillVersion rejectedVersion = new SkillVersion(skill.getId(), "1.0.0", ownerId);
rejectedVersion.setStatus(SkillVersionStatus.REJECTED);
rejectedVersion = skillVersionRepository.save(rejectedVersion);
SkillVersion retainedVersion = new SkillVersion(skill.getId(), "2.0.0", ownerId);
retainedVersion.setStatus(SkillVersionStatus.REJECTED);
retainedVersion = skillVersionRepository.save(retainedVersion);
ReviewTask rejectedTask = new ReviewTask(rejectedVersion.getId(), namespace.getId(), ownerId);
rejectedTask.setStatus(ReviewTaskStatus.REJECTED);
rejectedTask = reviewTaskRepository.save(rejectedTask);
ReviewTask approvedTask = new ReviewTask(rejectedVersion.getId(), namespace.getId(), ownerId);
approvedTask.setStatus(ReviewTaskStatus.APPROVED);
approvedTask = reviewTaskRepository.save(approvedTask);
ReviewTask retainedTask = new ReviewTask(retainedVersion.getId(), namespace.getId(), ownerId);
retainedTask.setStatus(ReviewTaskStatus.REJECTED);
retainedTask = reviewTaskRepository.save(retainedTask);
Long skillId = skill.getId();
Long rejectedVersionId = rejectedVersion.getId();
mockMvc.perform(delete("/api/web/skills/{namespace}/{slug}/versions/{version}",
namespace.getSlug(), skill.getSlug(), rejectedVersion.getVersion())
.with(authentication(portalAuth(ownerId, "USER")))
.with(csrf()))
.andExpect(status().isOk())
.andExpect(jsonPath("$.code").value(0))
.andExpect(jsonPath("$.data.skillId").value(skillId))
.andExpect(jsonPath("$.data.versionId").value(rejectedVersionId))
.andExpect(jsonPath("$.data.action").value("DELETE_VERSION"))
.andExpect(jsonPath("$.data.status").value("1.0.0"));
assertThat(skillVersionRepository.findById(rejectedVersion.getId())).isEmpty();
assertThat(skillVersionRepository.findById(retainedVersion.getId())).isPresent();
assertThat(reviewTaskRepository.findById(rejectedTask.getId())).isEmpty();
assertThat(reviewTaskRepository.findById(approvedTask.getId())).isEmpty();
assertThat(reviewTaskRepository.findById(retainedTask.getId())).isPresent();
verify(objectStorageService).deleteObjects(argThat(keys ->
keys.equals(List.of("packages/" + skillId + "/" + rejectedVersionId + "/bundle.zip"))
));
}
private UsernamePasswordAuthenticationToken portalAuth(String userId, String... roles) {
PlatformPrincipal principal = new PlatformPrincipal(
userId,
userId,
userId + "@example.com",
"",
"session",
Set.of(roles)
);
List<SimpleGrantedAuthority> authorities = Arrays.stream(roles)
.map(role -> new SimpleGrantedAuthority("ROLE_" + role))
.toList();
return new UsernamePasswordAuthenticationToken(principal, null, authorities);
}
}

View file

@ -14,7 +14,9 @@ import com.iflytek.skillhub.domain.namespace.NamespaceRepository;
import com.iflytek.skillhub.domain.namespace.NamespaceRole;
import com.iflytek.skillhub.domain.review.ReviewService;
import com.iflytek.skillhub.domain.skill.Skill;
import com.iflytek.skillhub.domain.skill.SkillVersion;
import com.iflytek.skillhub.domain.skill.SkillVersionRepository;
import com.iflytek.skillhub.domain.skill.SkillVersionStatus;
import com.iflytek.skillhub.domain.skill.SkillVisibility;
import com.iflytek.skillhub.domain.skill.service.SkillGovernanceService;
import com.iflytek.skillhub.domain.skill.service.SkillPublishService;
@ -76,4 +78,50 @@ class SkillLifecycleAppServiceTest {
assertThat(response.status()).isEqualTo("ARCHIVED");
verify(skillGovernanceService).archiveSkill(11L, "owner-1", Map.of(7L, NamespaceRole.OWNER), "127.0.0.1", "JUnit", "cleanup");
}
@Test
void deleteVersion_locksAllSkillVersionsBeforeDelegatingLifecycleMutation() {
Namespace namespace = new Namespace("global", "Global", "owner-1");
ReflectionTestUtils.setField(namespace, "id", 7L);
Skill skill = new Skill(7L, "demo-skill", "owner-1", SkillVisibility.PUBLIC);
ReflectionTestUtils.setField(skill, "id", 11L);
SkillVersion version = new SkillVersion(11L, "1.0.0", "owner-1");
ReflectionTestUtils.setField(version, "id", 13L);
version.setStatus(SkillVersionStatus.REJECTED);
SkillVersion retainedVersion = new SkillVersion(11L, "2.0.0", "owner-1");
ReflectionTestUtils.setField(retainedVersion, "id", 14L);
retainedVersion.setStatus(SkillVersionStatus.UPLOADED);
when(namespaceRepository.findBySlug("global")).thenReturn(Optional.of(namespace));
when(skillSlugResolutionService.resolve(
7L,
"demo-skill",
"owner-1",
SkillSlugResolutionService.Preference.CURRENT_USER
)).thenReturn(skill);
when(skillVersionRepository.findBySkillIdForUpdate(11L))
.thenReturn(java.util.List.of(version, retainedVersion));
var response = service.deleteVersion(
"global",
"demo-skill",
"1.0.0",
"owner-1",
Map.of(7L, NamespaceRole.OWNER),
new AuditRequestContext("127.0.0.1", "JUnit")
);
assertThat(response.versionId()).isEqualTo(13L);
assertThat(response.action()).isEqualTo("DELETE_VERSION");
verify(skillVersionRepository).findBySkillIdForUpdate(11L);
verify(skillGovernanceService).deleteVersion(
skill,
version,
"owner-1",
Map.of(7L, NamespaceRole.OWNER),
"127.0.0.1",
"JUnit",
"global"
);
}
}

View file

@ -249,6 +249,11 @@ class ScanTaskConsumerLoggingTest {
throw new UnsupportedOperationException();
}
@Override
public List<SkillVersion> findBySkillIdForUpdate(Long skillId) {
throw new UnsupportedOperationException();
}
@Override
public List<SkillVersion> findBySkillIdAndStatus(Long skillId, SkillVersionStatus status) {
throw new UnsupportedOperationException();

View file

@ -415,6 +415,11 @@ class ScanTaskConsumerTest {
throw unsupported();
}
@Override
public List<SkillVersion> findBySkillIdForUpdate(Long skillId) {
throw unsupported();
}
@Override
public List<SkillVersion> findBySkillIdAndStatus(Long skillId, SkillVersionStatus status) {
throw unsupported();

View file

@ -12,6 +12,7 @@ public interface SkillVersionRepository {
List<SkillVersion> findBySkillIdIn(List<Long> skillIds);
List<SkillVersion> findBySkillIdInAndStatus(List<Long> skillIds, SkillVersionStatus status);
List<SkillVersion> findBySkillId(Long skillId);
List<SkillVersion> findBySkillIdForUpdate(Long skillId);
Optional<SkillVersion> findBySkillIdAndVersion(Long skillId, String version);
List<SkillVersion> findBySkillIdAndStatus(Long skillId, SkillVersionStatus status);
SkillVersion save(SkillVersion version);

View file

@ -3,6 +3,7 @@ package com.iflytek.skillhub.domain.skill.service;
import com.iflytek.skillhub.domain.audit.AuditLogService;
import com.iflytek.skillhub.domain.event.SkillStatusChangedEvent;
import com.iflytek.skillhub.domain.namespace.NamespaceRole;
import com.iflytek.skillhub.domain.review.ReviewTaskRepository;
import com.iflytek.skillhub.domain.security.SecurityScanService;
import com.iflytek.skillhub.domain.shared.exception.DomainBadRequestException;
import com.iflytek.skillhub.domain.shared.exception.DomainForbiddenException;
@ -41,6 +42,7 @@ public class SkillGovernanceService {
private final SkillRepository skillRepository;
private final SkillVersionRepository skillVersionRepository;
private final SkillFileRepository skillFileRepository;
private final ReviewTaskRepository reviewTaskRepository;
private final ObjectStorageService objectStorageService;
private final AuditLogService auditLogService;
private final ApplicationEventPublisher eventPublisher;
@ -51,6 +53,7 @@ public class SkillGovernanceService {
public SkillGovernanceService(SkillRepository skillRepository,
SkillVersionRepository skillVersionRepository,
SkillFileRepository skillFileRepository,
ReviewTaskRepository reviewTaskRepository,
ObjectStorageService objectStorageService,
AuditLogService auditLogService,
ApplicationEventPublisher eventPublisher,
@ -60,6 +63,7 @@ public class SkillGovernanceService {
this.skillRepository = skillRepository;
this.skillVersionRepository = skillVersionRepository;
this.skillFileRepository = skillFileRepository;
this.reviewTaskRepository = reviewTaskRepository;
this.objectStorageService = objectStorageService;
this.auditLogService = auditLogService;
this.eventPublisher = eventPublisher;
@ -172,6 +176,8 @@ public class SkillGovernanceService {
throw new DomainBadRequestException("error.skill.version.delete.lastVersion", version.getVersion());
}
// Rejected versions retain terminal review history whose FK must not outlive the version.
reviewTaskRepository.deleteBySkillVersionIdIn(List.of(version.getId()));
List<SkillFile> files = skillFileRepository.findByVersionId(version.getId());
List<String> storageKeys = new ArrayList<>();
files.stream()

View file

@ -5,40 +5,43 @@ import static org.junit.jupiter.api.Assertions.assertThrows;
import static org.mockito.ArgumentMatchers.argThat;
import static org.mockito.ArgumentMatchers.any;
import static org.mockito.ArgumentMatchers.anyList;
import static org.mockito.BDDMockito.given;
import static org.mockito.Mockito.doThrow;
import static org.mockito.Mockito.inOrder;
import static org.mockito.Mockito.never;
import static org.mockito.Mockito.verify;
import static org.mockito.Mockito.doThrow;
import static org.mockito.BDDMockito.given;
import com.iflytek.skillhub.domain.audit.AuditLogService;
import com.iflytek.skillhub.domain.event.SkillStatusChangedEvent;
import com.iflytek.skillhub.domain.namespace.NamespaceRole;
import com.iflytek.skillhub.domain.review.ReviewTaskRepository;
import com.iflytek.skillhub.domain.security.SecurityScanService;
import com.iflytek.skillhub.domain.shared.exception.DomainBadRequestException;
import com.iflytek.skillhub.domain.shared.exception.DomainForbiddenException;
import com.iflytek.skillhub.domain.skill.Skill;
import com.iflytek.skillhub.domain.skill.SkillFile;
import com.iflytek.skillhub.domain.skill.SkillFileRepository;
import com.iflytek.skillhub.domain.skill.SkillStatus;
import com.iflytek.skillhub.domain.skill.SkillRepository;
import com.iflytek.skillhub.domain.skill.SkillStatus;
import com.iflytek.skillhub.domain.skill.SkillVersion;
import com.iflytek.skillhub.domain.skill.SkillVersionRepository;
import com.iflytek.skillhub.domain.skill.SkillVersionStatus;
import com.iflytek.skillhub.storage.ObjectStorageService;
import java.time.Clock;
import java.time.Instant;
import java.util.Optional;
import java.util.Map;
import java.time.ZoneOffset;
import org.springframework.context.ApplicationEventPublisher;
import org.springframework.transaction.support.TransactionSynchronization;
import org.springframework.transaction.support.TransactionSynchronizationManager;
import java.util.Map;
import java.util.Optional;
import org.junit.jupiter.api.AfterEach;
import org.junit.jupiter.api.BeforeEach;
import org.junit.jupiter.api.Test;
import org.junit.jupiter.api.extension.ExtendWith;
import org.mockito.InOrder;
import org.mockito.Mock;
import org.mockito.junit.jupiter.MockitoExtension;
import org.springframework.context.ApplicationEventPublisher;
import org.springframework.transaction.support.TransactionSynchronization;
import org.springframework.transaction.support.TransactionSynchronizationManager;
@ExtendWith(MockitoExtension.class)
class SkillGovernanceServiceTest {
@ -52,6 +55,8 @@ class SkillGovernanceServiceTest {
@Mock
private SkillFileRepository skillFileRepository;
@Mock
private ReviewTaskRepository reviewTaskRepository;
@Mock
private ObjectStorageService objectStorageService;
@Mock
private AuditLogService auditLogService;
@ -70,6 +75,7 @@ class SkillGovernanceServiceTest {
skillRepository,
skillVersionRepository,
skillFileRepository,
reviewTaskRepository,
objectStorageService,
auditLogService,
eventPublisher,
@ -229,6 +235,35 @@ class SkillGovernanceServiceTest {
verify(auditLogService).record("owner", "DELETE_SKILL_VERSION", "SKILL_VERSION", 2L, null, "127.0.0.1", "JUnit", "{\"version\":\"1.0.0\"}");
}
@Test
void deleteVersion_removesReviewTasksBeforeRejectedVersion() {
Skill skill = new Skill(1L, "demo", "owner", com.iflytek.skillhub.domain.skill.SkillVisibility.PUBLIC);
setField(skill, "id", 1L);
SkillVersion rejectedVersion = new SkillVersion(1L, "1.0.0", "owner");
setField(rejectedVersion, "id", 2L);
rejectedVersion.setStatus(SkillVersionStatus.REJECTED);
SkillVersion otherVersion = new SkillVersion(1L, "2.0.0", "owner");
setField(otherVersion, "id", 3L);
otherVersion.setStatus(SkillVersionStatus.DRAFT);
given(skillVersionRepository.findBySkillId(1L))
.willReturn(java.util.List.of(rejectedVersion, otherVersion));
given(skillFileRepository.findByVersionId(2L)).willReturn(java.util.List.of());
service.deleteVersion(
skill,
rejectedVersion,
"owner",
Map.of(),
"127.0.0.1",
"JUnit",
"test-ns"
);
InOrder deletionOrder = inOrder(reviewTaskRepository, skillVersionRepository);
deletionOrder.verify(reviewTaskRepository).deleteBySkillVersionIdIn(java.util.List.of(2L));
deletionOrder.verify(skillVersionRepository).delete(rejectedVersion);
}
@Test
void deleteVersion_deletesStorageAfterCommitWhenSynchronizationIsActive() {
Skill skill = new Skill(1L, "demo", "owner", com.iflytek.skillhub.domain.skill.SkillVisibility.PUBLIC);
@ -309,10 +344,36 @@ class SkillGovernanceServiceTest {
assertThrows(DomainBadRequestException.class,
() -> service.deleteVersion(skill, version, "owner", Map.of(), "127.0.0.1", "JUnit", "test-ns"));
verify(reviewTaskRepository, never()).deleteBySkillVersionIdIn(anyList());
verify(skillVersionRepository, never()).delete(any());
verify(objectStorageService, never()).deleteObject(any());
}
@Test
void deleteVersion_rejectsUnauthorizedUserWithoutDeletingReviewTasks() {
Skill skill = new Skill(1L, "demo", "owner", com.iflytek.skillhub.domain.skill.SkillVisibility.PUBLIC);
setField(skill, "id", 1L);
SkillVersion version = new SkillVersion(1L, "1.0.0", "owner");
setField(version, "id", 2L);
version.setStatus(SkillVersionStatus.REJECTED);
assertThrows(
DomainForbiddenException.class,
() -> service.deleteVersion(
skill,
version,
"member",
Map.of(1L, NamespaceRole.MEMBER),
"127.0.0.1",
"JUnit",
"test-ns"
)
);
verify(reviewTaskRepository, never()).deleteBySkillVersionIdIn(anyList());
verify(skillVersionRepository, never()).delete(any());
}
@Test
void deleteVersion_rejectsLastRemainingVersion() {
Skill skill = new Skill(1L, "demo", "owner", com.iflytek.skillhub.domain.skill.SkillVisibility.PUBLIC);
@ -326,6 +387,7 @@ class SkillGovernanceServiceTest {
() -> service.deleteVersion(skill, version, "owner", Map.of(), "127.0.0.1", "JUnit", "test-ns"));
assertThat(ex.messageCode()).isEqualTo("error.skill.version.delete.lastVersion");
verify(reviewTaskRepository, never()).deleteBySkillVersionIdIn(anyList());
verify(skillVersionRepository, never()).delete(any());
}

View file

@ -3,16 +3,22 @@ package com.iflytek.skillhub.infra.jpa;
import com.iflytek.skillhub.domain.skill.SkillVersion;
import com.iflytek.skillhub.domain.skill.SkillVersionRepository;
import com.iflytek.skillhub.domain.skill.SkillVersionStatus;
import java.util.List;
import java.util.Optional;
import org.springframework.data.domain.Page;
import org.springframework.data.domain.Pageable;
import org.springframework.data.jpa.repository.JpaRepository;
import org.springframework.data.jpa.repository.Query;
import org.springframework.data.repository.query.Param;
import org.springframework.stereotype.Repository;
import java.util.List;
import java.util.Optional;
/**
* JPA-backed repository for skill version history and status-oriented version queries.
*
* <p>The deletion lock uses explicit {@code FOR UPDATE} SQL because Hibernate's PostgreSQL dialect
* emits {@code FOR NO KEY UPDATE}, which H2's PostgreSQL compatibility mode cannot execute. It
* locks every version in stable ID order so concurrent deletions cannot both remove the last
* versions of one skill.
*/
@Repository
public interface SkillVersionJpaRepository extends JpaRepository<SkillVersion, Long>, SkillVersionRepository {
@ -22,6 +28,16 @@ public interface SkillVersionJpaRepository extends JpaRepository<SkillVersion, L
List<SkillVersion> findBySkillIdInAndStatusOrderByCreatedAtDesc(List<Long> skillIds, SkillVersionStatus status);
Optional<SkillVersion> findBySkillIdAndVersion(Long skillId, String version);
@Override
@Query(value = """
SELECT skill_version.*
FROM skill_version
WHERE skill_version.skill_id = :skillId
ORDER BY skill_version.id
FOR UPDATE
""", nativeQuery = true)
List<SkillVersion> findBySkillIdForUpdate(@Param("skillId") Long skillId);
@Override
default List<SkillVersion> findBySkillIdAndStatus(Long skillId, SkillVersionStatus status) {
return findBySkillIdAndStatusOrderByCreatedAtDesc(skillId, status);