scriptoriumadmin/skillhub
1
0
Fork 0
mirror of https://github.com/iflytek/skillhub.git synced 2026-08-28 11:25:00 +00:00
Code Issues Projects Releases Packages Wiki Activity Actions 3

fix(publish): constrain rejected version replacement

Browse source 
Signed-off-by: XiaoSeS <87064762+XiaoSeS@users.noreply.github.com>
This commit is contained in:
XiaoSeS 2026-07-28 13:45:02 +08:00
parent 466d50280e
commit ccc3a6e758
5 changed files with 215 additions and 192 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

8
server/skillhub-domain/src/main/java/com/iflytek/skillhub/domain/skill/service/SkillPublishService.java
View file

@ -62,6 +62,12 @@ public class SkillPublishService {
private static final DateTimeFormatter AUTO_VERSION_FORMATTER =
DateTimeFormatter.ofPattern("yyyyMMdd.HHmmss").withZone(ZoneId.systemDefault());
private static final Set<SkillVersionStatus> REPLACEABLE_VERSION_STATUSES = Set.of(
SkillVersionStatus.DRAFT,
SkillVersionStatus.SCAN_FAILED,
SkillVersionStatus.UPLOADED,
SkillVersionStatus.REJECTED
);
private static final Logger log = LoggerFactory.getLogger(SkillPublishService.class);
public record PublishResult(
@ -566,7 +572,7 @@ public class SkillPublishService {
}
private void deleteReplaceableVersionArtifacts(Skill skill, SkillVersion version, String namespaceSlug) {
if (version.getStatus() == SkillVersionStatus.PUBLISHED) {
if (!REPLACEABLE_VERSION_STATUSES.contains(version.getStatus())) {
throw new DomainBadRequestException("error.skill.version.exists", version.getVersion());
}

185
server/skillhub-domain/src/test/java/com/iflytek/skillhub/domain/skill/service/SkillPublishServiceReplaceTest.java
View file

@ -1,185 +0,0 @@
package com.iflytek.skillhub.domain.skill.service;
import com.fasterxml.jackson.databind.ObjectMapper;
import com.iflytek.skillhub.domain.namespace.NamespaceMemberRepository;
import com.iflytek.skillhub.domain.namespace.NamespaceRepository;
import com.iflytek.skillhub.domain.review.ReviewTaskRepository;
import com.iflytek.skillhub.domain.review.ReviewTaskStatus;
import com.iflytek.skillhub.domain.security.SecurityScanService;
import com.iflytek.skillhub.domain.shared.exception.DomainBadRequestException;
import com.iflytek.skillhub.domain.skill.Skill;
import com.iflytek.skillhub.domain.skill.SkillFileRepository;
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.domain.skill.metadata.SkillMetadataParser;
import com.iflytek.skillhub.domain.skill.validation.PrePublishValidator;
import com.iflytek.skillhub.domain.skill.validation.SkillPackageValidator;
import com.iflytek.skillhub.storage.ObjectStorageService;
import org.junit.jupiter.api.Test;
import org.junit.jupiter.api.extension.ExtendWith;
import org.mockito.Mock;
import org.mockito.junit.jupiter.MockitoExtension;
import org.mockito.junit.jupiter.MockitoSettings;
import org.mockito.quality.Strictness;
import org.springframework.context.ApplicationEventPublisher;
import java.lang.reflect.Field;
import java.lang.reflect.InvocationTargetException;
import java.lang.reflect.Method;
import java.time.Clock;
import java.util.List;
import static org.assertj.core.api.Assertions.assertThat;
import static org.assertj.core.api.Assertions.assertThatThrownBy;
import static org.mockito.ArgumentMatchers.any;
import static org.mockito.Mockito.never;
import static org.mockito.Mockito.verify;
import static org.mockito.Mockito.when;
/**
* Regression coverage for replacing a non-published version with the same version number.
*/
@ExtendWith(MockitoExtension.class)
@MockitoSettings(strictness = Strictness.LENIENT)
class SkillPublishServiceReplaceTest {
@Mock
private NamespaceRepository namespaceRepository;
@Mock
private NamespaceMemberRepository namespaceMemberRepository;
@Mock
private SkillRepository skillRepository;
@Mock
private SkillVersionRepository skillVersionRepository;
@Mock
private SkillFileRepository skillFileRepository;
@Mock
private ObjectStorageService objectStorageService;
@Mock
private SkillPackageValidator skillPackageValidator;
@Mock
private SkillMetadataParser skillMetadataParser;
@Mock
private PrePublishValidator prePublishValidator;
@Mock
private ReviewTaskRepository reviewTaskRepository;
@Mock
private SecurityScanService securityScanService;
@Mock
private SkillStorageDeletionCompensationService compensationService;
@Mock
private ApplicationEventPublisher eventPublisher;
private final ObjectMapper objectMapper = new ObjectMapper();
private SkillPublishService newService() {
return new SkillPublishService(
namespaceRepository,
namespaceMemberRepository,
skillRepository,
skillVersionRepository,
skillFileRepository,
objectStorageService,
skillPackageValidator,
skillMetadataParser,
prePublishValidator,
objectMapper,
reviewTaskRepository,
securityScanService,
compensationService,
eventPublisher,
Clock.systemUTC());
}
/** The id is database-generated, so tests set it directly. */
private static void setId(Object target, Long id) {
try {
Field field = target.getClass().getDeclaredField("id");
field.setAccessible(true);
field.set(target, id);
} catch (ReflectiveOperationException e) {
throw new IllegalStateException(e);
}
}
/** Invokes the package-private replacement routine under test. */
private void invokeDelete(SkillPublishService target, Skill skill, SkillVersion version) {
try {
Method method = SkillPublishService.class.getDeclaredMethod(
"deleteReplaceableVersionArtifacts", Skill.class, SkillVersion.class, String.class);
method.setAccessible(true);
method.invoke(target, skill, version, "ns");
} catch (InvocationTargetException e) {
if (e.getCause() instanceof RuntimeException runtimeException) {
throw runtimeException;
}
throw new IllegalStateException(e.getCause());
} catch (ReflectiveOperationException e) {
throw new IllegalStateException(e);
}
}
private Skill skill(Long id, Long latestVersionId) {
Skill skill = new Skill(1L, "demo", "owner", SkillVisibility.PUBLIC);
setId(skill, id);
skill.setLatestVersionId(latestVersionId);
return skill;
}
private SkillVersion version(Long id, SkillVersionStatus status) {
SkillVersion version = new SkillVersion(1L, "1.0.0", "owner");
setId(version, id);
version.setStatus(status);
return version;
}
/**
* A rejected version still owns a REJECTED review task. Deleting only the PENDING task
* left that row referencing the skill_version, so the delete hit a foreign key
* constraint and the re-upload surfaced as an HTTP 500.
*/
@Test
void replacingRejectedVersionRemovesReviewTasksOfEveryStatus() {
Skill skill = skill(1L, 10L);
SkillVersion rejected = version(10L, SkillVersionStatus.REJECTED);
when(skillFileRepository.findByVersionId(10L)).thenReturn(List.of());
SkillPublishService target = newService();
invokeDelete(target, skill, rejected);
verify(reviewTaskRepository).deleteBySkillVersionIdIn(List.of(10L));
verify(reviewTaskRepository, never())
.findBySkillVersionIdAndStatus(any(), any(ReviewTaskStatus.class));
verify(skillVersionRepository).delete(rejected);
}
@Test
void replacingLatestVersionClearsTheLatestPointerFirst() {
Skill skill = skill(1L, 10L);
SkillVersion pending = version(10L, SkillVersionStatus.PENDING_REVIEW);
when(skillFileRepository.findByVersionId(10L)).thenReturn(List.of());
SkillPublishService target = newService();
invokeDelete(target, skill, pending);
assertThat(skill.getLatestVersionId()).isNull();
verify(skillRepository).save(skill);
verify(reviewTaskRepository).deleteBySkillVersionIdIn(List.of(10L));
}
@Test
void replacingPublishedVersionIsStillRejected() {
Skill skill = skill(1L, 10L);
SkillVersion published = version(10L, SkillVersionStatus.PUBLISHED);
SkillPublishService target = newService();
assertThatThrownBy(() -> invokeDelete(target, skill, published))
.isInstanceOf(DomainBadRequestException.class);
verify(reviewTaskRepository, never()).deleteBySkillVersionIdIn(any());
verify(skillVersionRepository, never()).delete(any());
}
}

62
server/skillhub-domain/src/test/java/com/iflytek/skillhub/domain/skill/service/SkillPublishServiceTest.java
View file

@ -260,7 +260,7 @@ class SkillPublishServiceTest {
}
@Test
void testPublishFromEntries_ShouldReplaceDraftVersionWithSameVersion() throws Exception {
void testPublishFromEntries_ShouldReplaceRejectedVersionWithSameVersion() throws Exception {
String namespaceSlug = "test-ns";
String publisherId = "user-100";
String skillMdContent = "---\nname: test-skill\ndescription: Test\nversion: 1.0.0\n---\nBody";
@ -275,9 +275,9 @@ class SkillPublishServiceTest {
Skill skill = new Skill(1L, "test-skill", publisherId, SkillVisibility.PUBLIC);
setId(skill, 1L);
SkillVersion draftVersion = new SkillVersion(1L, "1.0.0", publisherId);
draftVersion.setStatus(SkillVersionStatus.DRAFT);
setId(draftVersion, 8L);
SkillVersion rejectedVersion = new SkillVersion(1L, "1.0.0", publisherId);
rejectedVersion.setStatus(SkillVersionStatus.REJECTED);
setId(rejectedVersion, 8L);
SkillFile oldFile = new SkillFile(8L, "SKILL.md", (long) skillMdContent.length(), "text/markdown", "abc", "skills/1/8/SKILL.md");
when(namespaceRepository.findBySlug(namespaceSlug)).thenReturn(Optional.of(namespace));
@ -288,7 +288,7 @@ class SkillPublishServiceTest {
when(skillRepository.findByNamespaceIdAndSlug(any(), eq("test-skill"))).thenReturn(List.of(skill));
when(skillRepository.findByNamespaceIdAndSlugAndOwnerId(any(), eq("test-skill"), eq(publisherId))).thenReturn(Optional.of(skill));
when(skillVersionRepository.findBySkillIdAndStatus(1L, SkillVersionStatus.PENDING_REVIEW)).thenReturn(List.of());
when(skillVersionRepository.findBySkillIdAndVersion(1L, "1.0.0")).thenReturn(Optional.of(draftVersion));
when(skillVersionRepository.findBySkillIdAndVersion(1L, "1.0.0")).thenReturn(Optional.of(rejectedVersion));
when(skillFileRepository.findByVersionId(8L)).thenReturn(List.of(oldFile));
when(skillVersionRepository.save(any(SkillVersion.class))).thenAnswer(invocation -> {
SkillVersion saved = invocation.getArgument(0);
@ -309,10 +309,60 @@ class SkillPublishServiceTest {
assertEquals("1.0.0", result.version().getVersion());
assertEquals(SkillVersionStatus.PENDING_REVIEW, result.version().getStatus());
verify(reviewTaskRepository).deleteBySkillVersionIdIn(List.of(8L));
verify(skillFileRepository).deleteByVersionId(8L);
verify(skillVersionRepository).delete(draftVersion);
verify(skillVersionRepository).delete(rejectedVersion);
verify(skillVersionRepository).flush();
verify(objectStorageService).deleteObjects(List.of("skills/1/8/SKILL.md", "packages/1/8/bundle.zip"));
ArgumentCaptor<ReviewTask> reviewTaskCaptor = ArgumentCaptor.forClass(ReviewTask.class);
verify(reviewTaskRepository).save(reviewTaskCaptor.capture());
assertEquals(result.version().getId(), reviewTaskCaptor.getValue().getSkillVersionId());
assertEquals(publisherId, reviewTaskCaptor.getValue().getSubmittedBy());
}
@Test
void testPublishFromEntries_ShouldRejectReplacementOfYankedVersion() throws Exception {
String namespaceSlug = "test-ns";
String publisherId = "user-100";
String skillMdContent = "---\nname: test-skill\ndescription: Test\nversion: 1.0.0\n---\nBody";
PackageEntry skillMd = new PackageEntry("SKILL.md", skillMdContent.getBytes(), skillMdContent.length(), "text/markdown");
List<PackageEntry> entries = List.of(skillMd);
Namespace namespace = new Namespace(namespaceSlug, "Test NS", "user-1");
setId(namespace, 1L);
NamespaceMember member = mock(NamespaceMember.class);
SkillMetadata metadata = new SkillMetadata("test-skill", "Test", "1.0.0", "Body", Map.of());
Skill skill = new Skill(1L, "test-skill", publisherId, SkillVisibility.PUBLIC);
setId(skill, 1L);
SkillVersion yankedVersion = new SkillVersion(1L, "1.0.0", publisherId);
yankedVersion.setStatus(SkillVersionStatus.YANKED);
setId(yankedVersion, 8L);
when(namespaceRepository.findBySlug(namespaceSlug)).thenReturn(Optional.of(namespace));
when(namespaceMemberRepository.findByNamespaceIdAndUserId(any(), eq(publisherId))).thenReturn(Optional.of(member));
when(skillPackageValidator.validate(entries)).thenReturn(ValidationResult.pass());
when(skillMetadataParser.parse(skillMdContent)).thenReturn(metadata);
when(prePublishValidator.validate(any())).thenReturn(ValidationResult.pass());
when(skillRepository.findByNamespaceIdAndSlug(any(), eq("test-skill"))).thenReturn(List.of(skill));
when(skillRepository.findByNamespaceIdAndSlugAndOwnerId(any(), eq("test-skill"), eq(publisherId))).thenReturn(Optional.of(skill));
when(skillVersionRepository.findBySkillIdAndVersion(1L, "1.0.0")).thenReturn(Optional.of(yankedVersion));
DomainBadRequestException exception = assertThrows(DomainBadRequestException.class, () ->
service.publishFromEntries(
namespaceSlug,
entries,
publisherId,
SkillVisibility.PUBLIC,
Set.of()
));
assertEquals("error.skill.version.exists", exception.messageCode());
verify(reviewTaskRepository, never()).deleteBySkillVersionIdIn(anyList());
verify(skillVersionRepository, never()).delete(any());
verify(skillFileRepository, never()).deleteByVersionId(any());
}
@Test

49
web/e2e/helpers/test-data-builder.ts
View file

@ -41,6 +41,12 @@ interface ReviewTaskSummary {
version: string
}
interface SkillVersionSummary {
id: number
version: string
status: string
}
interface NamespaceCandidate {
userId: string
displayName: string
@ -488,6 +494,40 @@ export class E2eTestDataBuilder {
throw new Error(`Timed out waiting for pending review ${namespaceSlug}/${skillSlug}@${version}`)
}
async waitForVersionStatus(
namespaceSlug: string,
skillSlug: string,
version: string,
expectedStatus: string,
): Promise<number> {
for (let attempt = 0; attempt < 60; attempt += 1) {
try {
const page = await parseEnvelope<{
items: SkillVersionSummary[]
}>(
await this.request.get(
`/api/web/skills/${encodeURIComponent(namespaceSlug)}/${encodeURIComponent(skillSlug)}/versions?page=0&size=100`,
),
)
const matched = page.items.find((item) =>
item.version === version && item.status === expectedStatus,
)
if (matched) {
return matched.id
}
} catch {
// Security scanning and version projection can complete asynchronously.
}
await new Promise((resolve) => setTimeout(resolve, 1_000))
}
throw new Error(
`Timed out waiting for ${namespaceSlug}/${skillSlug}@${version} to reach ${expectedStatus}`,
)
}
async approveReview(reviewTaskId: number, comment = 'Approved by Playwright E2E'): Promise<void> {
let lastError: unknown
for (let attempt = 0; attempt < 60; attempt += 1) {
@ -512,6 +552,15 @@ export class E2eTestDataBuilder {
throw lastError instanceof Error ? lastError : new Error('approveReview timed out')
}
async rejectReview(reviewTaskId: number, comment = 'Rejected by Playwright E2E'): Promise<void> {
await parseEnvelope<ReviewTaskSummary>(
await this.request.post(`/api/web/reviews/${reviewTaskId}/reject`, {
data: { comment },
headers: await csrfHeaders(this.page),
}),
)
}
async searchNamespaceMemberCandidates(slug: string, search: string): Promise<NamespaceCandidate[]> {
const query = new URLSearchParams({ search })
return parseEnvelope<NamespaceCandidate[]>(

103
web/e2e/rejected-version-republish.spec.ts Normal file
View file

@ -0,0 +1,103 @@
import { expect, test } from '@playwright/test'
import { setEnglishLocale } from './helpers/auth-fixtures'
import { loginWithCredentials, registerSession } from './helpers/session'
import { E2eTestDataBuilder } from './helpers/test-data-builder'
interface ReviewTaskSummary {
id: number
}
interface ReviewTaskPage {
items: ReviewTaskSummary[]
}
interface ApiEnvelope<T> {
code: number
data: T
}
function getOptionalEnv(name: string): string | undefined {
const value = process.env[name]?.trim()
return value ? value : undefined
}
function adminCredentials() {
return {
username: getOptionalEnv('E2E_ADMIN_USERNAME') ?? getOptionalEnv('BOOTSTRAP_ADMIN_USERNAME') ?? 'admin',
password: getOptionalEnv('E2E_ADMIN_PASSWORD') ?? getOptionalEnv('BOOTSTRAP_ADMIN_PASSWORD') ?? 'ChangeMe!2026',
}
}
test.describe('Rejected version replacement (Real API)', () => {
test.describe.configure({ timeout: 150_000 })
test.beforeEach(async ({ page }, testInfo) => {
await setEnglishLocale(page)
await registerSession(page, testInfo)
})
test('re-publishes the same version after rejection', async ({ page, browser }, testInfo) => {
const publisherBuilder = new E2eTestDataBuilder(page, testInfo)
await publisherBuilder.init()
const adminContext = await browser.newContext()
const adminPage = await adminContext.newPage()
const adminBuilder = new E2eTestDataBuilder(adminPage, testInfo)
await loginWithCredentials(adminPage, adminCredentials(), testInfo)
await adminBuilder.init()
try {
const namespace = await publisherBuilder.ensureWritableNamespace()
const skillName = `replace-rejected-${Date.now().toString(36)}`
const firstPublish = await publisherBuilder.publishSkill(namespace.slug, {
name: skillName,
version: '1.0.0',
})
const rejectedReviewId = await adminBuilder.waitForPendingReview(
namespace.slug,
firstPublish.slug,
firstPublish.version,
)
await publisherBuilder.waitForVersionStatus(
namespace.slug,
firstPublish.slug,
firstPublish.version,
'PENDING_REVIEW',
)
await adminBuilder.rejectReview(rejectedReviewId)
const replacement = await publisherBuilder.publishSkill(namespace.slug, {
name: skillName,
description: 'Replacement after review rejection',
version: '1.0.0',
})
const replacementReviewId = await adminBuilder.waitForPendingReview(
namespace.slug,
replacement.slug,
replacement.version,
)
await publisherBuilder.waitForVersionStatus(
namespace.slug,
replacement.slug,
replacement.version,
'PENDING_REVIEW',
)
expect(replacement.skillId).toBe(firstPublish.skillId)
expect(replacement.version).toBe(firstPublish.version)
expect(replacementReviewId).not.toBe(rejectedReviewId)
const rejectedReviewsResponse = await adminPage.request.get(
'/api/web/reviews?status=REJECTED&page=0&size=100&sortDirection=DESC',
)
expect(rejectedReviewsResponse.ok()).toBe(true)
const rejectedReviews = await rejectedReviewsResponse.json() as ApiEnvelope<ReviewTaskPage>
expect(rejectedReviews.code).toBe(0)
expect(rejectedReviews.data.items.some((item) => item.id === rejectedReviewId)).toBe(false)
} finally {
await adminBuilder.cleanup()
await adminContext.close()
await publisherBuilder.cleanup()
}
})
})