From 0dd694859a0cfd812bd4e87d958f4c82d13c45b2 Mon Sep 17 00:00:00 2001 From: XiaoSeS <87064762+XiaoSeS@users.noreply.github.com> Date: Wed, 9 Sep 2026 20:36:49 +0800 Subject: [PATCH] fix(suite): close final review gaps Signed-off-by: XiaoSeS <87064762+XiaoSeS@users.noreply.github.com> --- ...killSuiteVersionOptimisticLockingTest.java | 89 +++++++++++++++++++ .../domain/suite/SkillSuiteDraftService.java | 2 +- .../suite/SkillSuiteVersionRepository.java | 1 + .../suite/SkillSuiteDraftServiceTest.java | 2 +- .../jpa/SkillSuiteVersionJpaRepository.java | 12 +++ web/src/pages/suite-detail.test.tsx | 7 +- web/src/pages/suite-detail.tsx | 8 +- 7 files changed, 116 insertions(+), 5 deletions(-) diff --git a/server/skillhub-app/src/test/java/com/iflytek/skillhub/repository/SkillSuiteVersionOptimisticLockingTest.java b/server/skillhub-app/src/test/java/com/iflytek/skillhub/repository/SkillSuiteVersionOptimisticLockingTest.java index 6062896d..e476a74a 100644 --- a/server/skillhub-app/src/test/java/com/iflytek/skillhub/repository/SkillSuiteVersionOptimisticLockingTest.java +++ b/server/skillhub-app/src/test/java/com/iflytek/skillhub/repository/SkillSuiteVersionOptimisticLockingTest.java @@ -5,22 +5,33 @@ import static org.assertj.core.api.Assertions.assertThatThrownBy; import com.iflytek.skillhub.domain.namespace.Namespace; import com.iflytek.skillhub.domain.suite.SkillSuite; +import com.iflytek.skillhub.domain.suite.SkillSuiteMemberSelection; import com.iflytek.skillhub.domain.suite.SkillSuiteVersion; +import com.iflytek.skillhub.domain.suite.SkillSuiteVersionMember; +import com.iflytek.skillhub.domain.suite.SkillSuiteVersionMemberRepository; +import com.iflytek.skillhub.domain.suite.SkillSuiteVersionRepository; import com.iflytek.skillhub.domain.suite.SkillSuiteVersionStatus; import com.iflytek.skillhub.domain.skill.SkillVisibility; import jakarta.persistence.EntityManager; import jakarta.persistence.EntityManagerFactory; import jakarta.persistence.OptimisticLockException; import java.time.Instant; +import java.util.List; +import java.util.concurrent.CountDownLatch; +import java.util.concurrent.Executors; +import java.util.concurrent.TimeUnit; import org.junit.jupiter.api.Test; import org.springframework.beans.factory.annotation.Autowired; import org.springframework.boot.test.autoconfigure.jdbc.AutoConfigureTestDatabase; import org.springframework.boot.test.autoconfigure.orm.jpa.DataJpaTest; +import org.springframework.orm.ObjectOptimisticLockingFailureException; import org.springframework.test.context.ActiveProfiles; import org.springframework.test.context.DynamicPropertyRegistry; import org.springframework.test.context.DynamicPropertySource; +import org.springframework.transaction.PlatformTransactionManager; import org.springframework.transaction.annotation.Propagation; import org.springframework.transaction.annotation.Transactional; +import org.springframework.transaction.support.TransactionTemplate; import org.testcontainers.containers.PostgreSQLContainer; import org.testcontainers.junit.jupiter.Container; import org.testcontainers.junit.jupiter.Testcontainers; @@ -47,6 +58,15 @@ class SkillSuiteVersionOptimisticLockingTest { @Autowired private EntityManagerFactory entityManagerFactory; + @Autowired + private SkillSuiteVersionRepository versionRepository; + + @Autowired + private SkillSuiteVersionMemberRepository memberRepository; + + @Autowired + private PlatformTransactionManager transactionManager; + @Test @Transactional(propagation = Propagation.NOT_SUPPORTED) void publishingRejectsAConcurrentStaleDraftUpdate() { @@ -85,6 +105,52 @@ class SkillSuiteVersionOptimisticLockingTest { } } + @Test + @Transactional(propagation = Propagation.NOT_SUPPORTED) + void publishingRejectsAConcurrentMemberOnlyDraftUpdate() throws Exception { + PersistedSuite persisted = persistDraft(); + CountDownLatch editorLoaded = new CountDownLatch(1); + CountDownLatch publisherCommitted = new CountDownLatch(1); + TransactionTemplate transactions = new TransactionTemplate(transactionManager); + try (var executor = Executors.newSingleThreadExecutor()) { + var staleEdit = executor.submit(() -> transactions.executeWithoutResult(status -> { + SkillSuiteVersion version = versionRepository.findByIdForDefinitionUpdate(persisted.versionId()) + .orElseThrow(); + editorLoaded.countDown(); + await(publisherCommitted); + + memberRepository.deleteBySuiteVersionId(version.getId()); + memberRepository.saveAll(List.of(member(version.getId(), "replacement-after-publish"))); + })); + + assertThat(editorLoaded.await(10, TimeUnit.SECONDS)).isTrue(); + transactions.executeWithoutResult(status -> { + SkillSuiteVersion version = versionRepository.findById(persisted.versionId()).orElseThrow(); + version.setStatus(SkillSuiteVersionStatus.PUBLISHED); + version.setPublishedAt(Instant.parse("2026-09-09T08:00:00Z")); + versionRepository.save(version); + }); + publisherCommitted.countDown(); + + assertThatThrownBy(() -> staleEdit.get(10, TimeUnit.SECONDS)) + .satisfies(error -> assertThat( + hasCause(error, ObjectOptimisticLockingFailureException.class)).isTrue()); + + transactions.executeWithoutResult(status -> { + SkillSuiteVersion saved = versionRepository.findById(persisted.versionId()).orElseThrow(); + List members = + memberRepository.findBySuiteVersionIdOrderByPosition(persisted.versionId()); + assertThat(saved.getStatus()).isEqualTo(SkillSuiteVersionStatus.PUBLISHED); + assertThat(members).singleElement() + .extracting(SkillSuiteVersionMember::getSkillSlugSnapshot) + .isEqualTo("original-member"); + }); + } finally { + publisherCommitted.countDown(); + deleteSuite(persisted); + } + } + private PersistedSuite persistDraft() { EntityManager entityManager = entityManagerFactory.createEntityManager(); try { @@ -96,6 +162,7 @@ class SkillSuiteVersionOptimisticLockingTest { SkillSuiteVersion version = new SkillSuiteVersion( suite.getId(), "1.0.0", "Original draft", "Summary", SkillVisibility.PUBLIC, "owner"); entityManager.persist(version); + entityManager.persist(member(version.getId(), "original-member")); entityManager.getTransaction().commit(); return new PersistedSuite(namespace.getId(), suite.getId(), version.getId()); } finally { @@ -108,6 +175,9 @@ class SkillSuiteVersionOptimisticLockingTest { EntityManager entityManager = entityManagerFactory.createEntityManager(); try { entityManager.getTransaction().begin(); + entityManager.createQuery("DELETE FROM SkillSuiteVersionMember member WHERE member.suiteVersionId = :id") + .setParameter("id", persisted.versionId()) + .executeUpdate(); entityManager.createQuery("DELETE FROM SkillSuiteVersion version WHERE version.id = :id") .setParameter("id", persisted.versionId()) .executeUpdate(); @@ -141,6 +211,25 @@ class SkillSuiteVersionOptimisticLockingTest { return false; } + private SkillSuiteVersionMember member(Long suiteVersionId, String slug) { + return new SkillSuiteVersionMember( + suiteVersionId, + new SkillSuiteMemberSelection(null, null, "global", slug, "1.0.0", "sha256:" + slug), + 0, + true); + } + + private void await(CountDownLatch latch) { + try { + if (!latch.await(10, TimeUnit.SECONDS)) { + throw new AssertionError("Timed out waiting for concurrent transaction"); + } + } catch (InterruptedException error) { + Thread.currentThread().interrupt(); + throw new AssertionError("Interrupted while waiting for concurrent transaction", error); + } + } + private record PersistedSuite(Long namespaceId, Long suiteId, Long versionId) { } } diff --git a/server/skillhub-domain/src/main/java/com/iflytek/skillhub/domain/suite/SkillSuiteDraftService.java b/server/skillhub-domain/src/main/java/com/iflytek/skillhub/domain/suite/SkillSuiteDraftService.java index 3a82e509..c6a6587f 100644 --- a/server/skillhub-domain/src/main/java/com/iflytek/skillhub/domain/suite/SkillSuiteDraftService.java +++ b/server/skillhub-domain/src/main/java/com/iflytek/skillhub/domain/suite/SkillSuiteDraftService.java @@ -128,7 +128,7 @@ public class SkillSuiteDraftService { SkillSuite suite = suiteRepository.findById(suiteId) .orElseThrow(() -> new DomainNotFoundException("error.suite.notFound", suiteId)); requireWritableNamespace(suite.getNamespaceId()); - SkillSuiteVersion version = versionRepository.findById(versionId) + SkillSuiteVersion version = versionRepository.findByIdForDefinitionUpdate(versionId) .orElseThrow(() -> new DomainNotFoundException("error.suite.version.notFound", versionId)); if (!suiteId.equals(version.getSuiteId())) { throw new DomainBadRequestException("error.suite.version.mismatch"); diff --git a/server/skillhub-domain/src/main/java/com/iflytek/skillhub/domain/suite/SkillSuiteVersionRepository.java b/server/skillhub-domain/src/main/java/com/iflytek/skillhub/domain/suite/SkillSuiteVersionRepository.java index 072925d0..b717387e 100644 --- a/server/skillhub-domain/src/main/java/com/iflytek/skillhub/domain/suite/SkillSuiteVersionRepository.java +++ b/server/skillhub-domain/src/main/java/com/iflytek/skillhub/domain/suite/SkillSuiteVersionRepository.java @@ -6,6 +6,7 @@ import java.util.Optional; /** Persistence contract for immutable Suite version snapshots. */ public interface SkillSuiteVersionRepository { Optional findById(Long id); + Optional findByIdForDefinitionUpdate(Long id); Optional findBySuiteIdAndVersion(Long suiteId, String version); List findByIdIn(List ids); List findBySuiteId(Long suiteId); diff --git a/server/skillhub-domain/src/test/java/com/iflytek/skillhub/domain/suite/SkillSuiteDraftServiceTest.java b/server/skillhub-domain/src/test/java/com/iflytek/skillhub/domain/suite/SkillSuiteDraftServiceTest.java index 0769b31c..73a7ff5f 100644 --- a/server/skillhub-domain/src/test/java/com/iflytek/skillhub/domain/suite/SkillSuiteDraftServiceTest.java +++ b/server/skillhub-domain/src/test/java/com/iflytek/skillhub/domain/suite/SkillSuiteDraftServiceTest.java @@ -97,7 +97,7 @@ class SkillSuiteDraftServiceTest { setId(version, 20L); when(namespaceRepository.findById(1L)).thenReturn(Optional.of(namespace)); when(suiteRepository.findById(10L)).thenReturn(Optional.of(suite)); - when(versionRepository.findById(20L)).thenReturn(Optional.of(version)); + when(versionRepository.findByIdForDefinitionUpdate(20L)).thenReturn(Optional.of(version)); when(versionRepository.save(any())).thenAnswer(invocation -> invocation.getArgument(0)); when(memberRepository.saveAll(any())).thenAnswer(invocation -> invocation.getArgument(0)); diff --git a/server/skillhub-infra/src/main/java/com/iflytek/skillhub/infra/jpa/SkillSuiteVersionJpaRepository.java b/server/skillhub-infra/src/main/java/com/iflytek/skillhub/infra/jpa/SkillSuiteVersionJpaRepository.java index f6525a1d..01fa1ccf 100644 --- a/server/skillhub-infra/src/main/java/com/iflytek/skillhub/infra/jpa/SkillSuiteVersionJpaRepository.java +++ b/server/skillhub-infra/src/main/java/com/iflytek/skillhub/infra/jpa/SkillSuiteVersionJpaRepository.java @@ -2,11 +2,23 @@ package com.iflytek.skillhub.infra.jpa; import com.iflytek.skillhub.domain.suite.SkillSuiteVersion; import com.iflytek.skillhub.domain.suite.SkillSuiteVersionRepository; +import jakarta.persistence.LockModeType; import org.springframework.data.jpa.repository.JpaRepository; +import org.springframework.data.jpa.repository.Lock; +import org.springframework.data.jpa.repository.Query; +import org.springframework.data.repository.query.Param; import org.springframework.stereotype.Repository; +import java.util.Optional; + /** JPA adapter for Suite version snapshots. */ @Repository public interface SkillSuiteVersionJpaRepository extends JpaRepository, SkillSuiteVersionRepository { + + /** Ensure member-only draft edits still conflict with a concurrent lifecycle transition. */ + @Override + @Lock(LockModeType.OPTIMISTIC_FORCE_INCREMENT) + @Query("select version from SkillSuiteVersion version where version.id = :id") + Optional findByIdForDefinitionUpdate(@Param("id") Long id); } diff --git a/web/src/pages/suite-detail.test.tsx b/web/src/pages/suite-detail.test.tsx index d534e0ac..c277eef5 100644 --- a/web/src/pages/suite-detail.test.tsx +++ b/web/src/pages/suite-detail.test.tsx @@ -11,6 +11,7 @@ const mocks = vi.hoisted(() => ({ detail: { data: undefined as SkillSuite | undefined, isLoading: false, error: null as Error | null }, submit: { mutateAsync: vi.fn(), isPending: false }, })) +const originalRuntimeConfig = window.__SKILLHUB_RUNTIME_CONFIG__ vi.mock('@tanstack/react-router', () => ({ useNavigate: () => mocks.navigate, @@ -91,6 +92,7 @@ describe('SuiteDetailPage', () => { afterEach(() => { cleanup() vi.clearAllMocks() + window.__SKILLHUB_RUNTIME_CONFIG__ = originalRuntimeConfig }) it('adds entry skill guidance to the overview and keeps the full member grid separate', () => { @@ -172,6 +174,9 @@ describe('SuiteDetailPage', () => { }) it('places Suite metadata and installation in the detail sidebar', () => { + window.__SKILLHUB_RUNTIME_CONFIG__ = { + appBaseUrl: 'https://registry.internal.example/skillhub', + } mocks.detail = { data: suite(), isLoading: false, error: null } render() @@ -180,7 +185,7 @@ describe('SuiteDetailPage', () => { expect(within(sidebar).getByText('v1.0.0')).not.toBeNull() expect(within(sidebar).getByText('suite.installCommand')).not.toBeNull() expect(within(sidebar).getByText( - 'skillhub suite install @global/care-workflow --version 1.0.0', + 'skillhub suite install @global/care-workflow --version 1.0.0 --registry https://registry.internal.example/skillhub', )).not.toBeNull() expect(within(sidebar).getByLabelText('suite.copyInstallCommand')).not.toBeNull() }) diff --git a/web/src/pages/suite-detail.tsx b/web/src/pages/suite-detail.tsx index ae796ec1..45247c13 100644 --- a/web/src/pages/suite-detail.tsx +++ b/web/src/pages/suite-detail.tsx @@ -6,6 +6,7 @@ import { useSuiteDetail, useSuiteVersions, useSubmitSuite } from '@/shared/hooks import { suiteBlockingReasonLabel, suiteStatusLabel, suiteVisibilityLabel } from '@/features/suite/suite-labels' import { SuiteManagementActions } from '@/features/suite/suite-management-actions' import { MarkdownRenderer } from '@/features/skill/markdown-renderer' +import { getBaseUrl } from '@/features/skill/install-command' import { Card } from '@/shared/ui/card' import { Button, buttonVariants } from '@/shared/ui/button' import { Tabs, TabsContent, TabsList, TabsTrigger } from '@/shared/ui/tabs' @@ -23,9 +24,12 @@ export function SuiteDetailPage() { const { data: suite, isLoading, error } = useSuiteDetail(namespace, slug, search.version) const { data: versions } = useSuiteVersions(namespace, slug) const submitMutation = useSubmitSuite() + const registryUrl = useMemo(() => getBaseUrl(), []) const command = useMemo( - () => suite ? `skillhub suite install @${suite.namespace}/${suite.slug} --version ${suite.version}` : '', - [suite], + () => suite + ? `skillhub suite install @${suite.namespace}/${suite.slug} --version ${suite.version} --registry ${registryUrl}` + : '', + [registryUrl, suite], ) const suitePath = `/suite/${encodeURIComponent(namespace)}/${encodeURIComponent(slug)}` const returnTo = search.version