From b168749ceb80ade4ef22664c6736f176c441b2f1 Mon Sep 17 00:00:00 2001 From: vsxd Date: Thu, 19 Mar 2026 16:52:23 +0800 Subject: [PATCH] fix: optimize skill search listing performance --- .../service/SkillSearchAppService.java | 83 +++----------- ...28__optimize_skill_search_newest_order.sql | 3 + ...optimize_skill_search_popularity_order.sql | 7 ++ .../controller/SkillSearchControllerTest.java | 21 ++++ .../service/SkillSearchAppServiceTest.java | 70 ++++++++---- .../domain/skill/SkillVersionRepository.java | 1 + .../SkillLifecycleProjectionService.java | 45 ++++++++ .../infra/jpa/SkillVersionJpaRepository.java | 6 + .../PostgresFullTextQueryService.java | 43 +++++--- .../PostgresFullTextQueryServiceTest.java | 104 +++++++++++++++++- 10 files changed, 274 insertions(+), 109 deletions(-) create mode 100644 server/skillhub-app/src/main/resources/db/migration/V28__optimize_skill_search_newest_order.sql create mode 100644 server/skillhub-app/src/main/resources/db/migration/V29__optimize_skill_search_popularity_order.sql diff --git a/server/skillhub-app/src/main/java/com/iflytek/skillhub/service/SkillSearchAppService.java b/server/skillhub-app/src/main/java/com/iflytek/skillhub/service/SkillSearchAppService.java index 05d3cab3..2a8449e3 100644 --- a/server/skillhub-app/src/main/java/com/iflytek/skillhub/service/SkillSearchAppService.java +++ b/server/skillhub-app/src/main/java/com/iflytek/skillhub/service/SkillSearchAppService.java @@ -2,14 +2,11 @@ package com.iflytek.skillhub.service; import com.iflytek.skillhub.domain.namespace.NamespaceRole; import com.iflytek.skillhub.domain.namespace.Namespace; -import com.iflytek.skillhub.domain.namespace.NamespaceStatus; import com.iflytek.skillhub.domain.namespace.NamespaceRepository; import com.iflytek.skillhub.domain.namespace.NamespaceService; import com.iflytek.skillhub.domain.skill.Skill; import com.iflytek.skillhub.domain.skill.SkillRepository; -import com.iflytek.skillhub.domain.skill.VisibilityChecker; import com.iflytek.skillhub.domain.skill.service.SkillLifecycleProjectionService; -import com.iflytek.skillhub.domain.shared.exception.DomainBadRequestException; import com.iflytek.skillhub.dto.SkillSummaryResponse; import com.iflytek.skillhub.search.SearchQuery; import com.iflytek.skillhub.search.SearchQueryService; @@ -34,7 +31,6 @@ public class SkillSearchAppService { private final SkillRepository skillRepository; private final NamespaceRepository namespaceRepository; private final NamespaceService namespaceService; - private final VisibilityChecker visibilityChecker; private final SkillLifecycleProjectionService skillLifecycleProjectionService; public SkillSearchAppService( @@ -42,13 +38,11 @@ public class SkillSearchAppService { SkillRepository skillRepository, NamespaceRepository namespaceRepository, NamespaceService namespaceService, - VisibilityChecker visibilityChecker, SkillLifecycleProjectionService skillLifecycleProjectionService) { this.searchQueryService = searchQueryService; this.skillRepository = skillRepository; this.namespaceRepository = namespaceRepository; this.namespaceService = namespaceService; - this.visibilityChecker = visibilityChecker; this.skillLifecycleProjectionService = skillLifecycleProjectionService; } @@ -72,7 +66,7 @@ public class SkillSearchAppService { SearchVisibilityScope scope = buildVisibilityScope(userId, userNsRoles); - return searchVisibleSkills(keyword, namespaceId, sortBy != null ? sortBy : "newest", page, size, userId, userNsRoles, scope); + return searchVisibleSkills(keyword, namespaceId, sortBy != null ? sortBy : "newest", page, size, scope); } private Long resolveNamespaceId(String namespaceSlug, String userId, Map userNsRoles) { @@ -106,46 +100,20 @@ public class SkillSearchAppService { String sortBy, int page, int size, - String userId, - Map userNsRoles, SearchVisibilityScope scope) { - int batchSize = Math.max(size, 20); - long rawTotal = Long.MAX_VALUE; - int rawPage = 0; - long visibleSeen = 0; - int visibleStart = page * size; - List pageItems = new java.util.ArrayList<>(); - - while ((long) rawPage * batchSize < rawTotal) { - SearchResult result = searchQueryService.search(new SearchQuery( - keyword, - namespaceId, - scope, - sortBy, - rawPage, - batchSize - )); - rawTotal = result.total(); - List visibleBatch = mapVisibleSkillSummaries(result.skillIds(), userId, userNsRoles); - for (SkillSummaryResponse item : visibleBatch) { - if (visibleSeen >= visibleStart && pageItems.size() < size) { - pageItems.add(item); - } - visibleSeen++; - } - if (result.skillIds().isEmpty()) { - break; - } - rawPage++; - } - - return new SearchResponse(pageItems, visibleSeen, page, size); + SearchResult result = searchQueryService.search(new SearchQuery( + keyword, + namespaceId, + scope, + sortBy, + page, + size + )); + List pageItems = mapVisibleSkillSummaries(result.skillIds()); + return new SearchResponse(pageItems, result.total(), page, size); } - private List mapVisibleSkillSummaries( - List skillIds, - String userId, - Map userNsRoles) { + private List mapVisibleSkillSummaries(List skillIds) { if (skillIds.isEmpty()) { return List.of(); } @@ -164,24 +132,20 @@ public class SkillSearchAppService { .collect(Collectors.toMap(Namespace::getId, Function.identity())); Map namespaceSlugsById = namespacesById.entrySet().stream() .collect(Collectors.toMap(Map.Entry::getKey, entry -> entry.getValue().getSlug())); + Map projectionsBySkillId = + skillLifecycleProjectionService.projectPublishedSummaries(matchedSkills); return skillIds.stream() .map(skillsById::get) .filter(java.util.Objects::nonNull) - .filter(skill -> visibilityChecker.canAccess(skill, userId, userNsRoles != null ? userNsRoles : Map.of())) - .filter(skill -> namespaceVisible(skill.getNamespaceId(), namespacesById, userId, userNsRoles)) - .map(skill -> toSummaryResponse(skill, namespaceSlugsById)) + .map(skill -> toSummaryResponse(skill, namespaceSlugsById, projectionsBySkillId.get(skill.getId()))) .toList(); } private SkillSummaryResponse toSummaryResponse( Skill skill, - Map namespaceSlugsById) { - SkillLifecycleProjectionService.Projection projection = skillLifecycleProjectionService.projectForViewer( - skill, - null, - Map.of() - ); + Map namespaceSlugsById, + SkillLifecycleProjectionService.Projection projection) { String namespaceSlug = namespaceSlugsById.get(skill.getNamespaceId()); return new SkillSummaryResponse( @@ -216,17 +180,4 @@ public class SkillSearchAppService { ); } - private boolean namespaceVisible( - Long namespaceId, - Map namespacesById, - String userId, - Map userNsRoles) { - NamespaceStatus status = java.util.Optional.ofNullable(namespacesById.get(namespaceId)) - .map(Namespace::getStatus) - .orElse(NamespaceStatus.ACTIVE); - if (status != NamespaceStatus.ARCHIVED) { - return true; - } - return userId != null && userNsRoles != null && userNsRoles.containsKey(namespaceId); - } } diff --git a/server/skillhub-app/src/main/resources/db/migration/V28__optimize_skill_search_newest_order.sql b/server/skillhub-app/src/main/resources/db/migration/V28__optimize_skill_search_newest_order.sql new file mode 100644 index 00000000..1e2e884f --- /dev/null +++ b/server/skillhub-app/src/main/resources/db/migration/V28__optimize_skill_search_newest_order.sql @@ -0,0 +1,3 @@ +CREATE INDEX IF NOT EXISTS idx_skill_active_visible_updated + ON skill (updated_at DESC, id DESC) + WHERE status = 'ACTIVE' AND hidden = FALSE; diff --git a/server/skillhub-app/src/main/resources/db/migration/V29__optimize_skill_search_popularity_order.sql b/server/skillhub-app/src/main/resources/db/migration/V29__optimize_skill_search_popularity_order.sql new file mode 100644 index 00000000..f3817ae3 --- /dev/null +++ b/server/skillhub-app/src/main/resources/db/migration/V29__optimize_skill_search_popularity_order.sql @@ -0,0 +1,7 @@ +CREATE INDEX IF NOT EXISTS idx_skill_active_visible_downloads + ON skill (download_count DESC, updated_at DESC, id DESC) + WHERE status = 'ACTIVE' AND hidden = FALSE; + +CREATE INDEX IF NOT EXISTS idx_skill_active_visible_rating + ON skill (rating_avg DESC, updated_at DESC, id DESC) + WHERE status = 'ACTIVE' AND hidden = FALSE; diff --git a/server/skillhub-app/src/test/java/com/iflytek/skillhub/controller/SkillSearchControllerTest.java b/server/skillhub-app/src/test/java/com/iflytek/skillhub/controller/SkillSearchControllerTest.java index 8531bf79..8916de13 100644 --- a/server/skillhub-app/src/test/java/com/iflytek/skillhub/controller/SkillSearchControllerTest.java +++ b/server/skillhub-app/src/test/java/com/iflytek/skillhub/controller/SkillSearchControllerTest.java @@ -55,4 +55,25 @@ class SkillSearchControllerTest { .andExpect(jsonPath("$.timestamp").isNotEmpty()) .andExpect(jsonPath("$.requestId").isNotEmpty()); } + + @Test + void searchShouldPassExplicitSortPageAndSize() throws Exception { + when(skillSearchAppService.search( + eq(null), + eq(null), + eq("newest"), + eq(0), + eq(12), + any(), + any())) + .thenReturn(new SkillSearchAppService.SearchResponse(List.of(), 0, 0, 12)); + + mockMvc.perform(get("/api/web/skills") + .param("sort", "newest") + .param("page", "0") + .param("size", "12")) + .andExpect(status().isOk()) + .andExpect(jsonPath("$.data.size").value(12)) + .andExpect(jsonPath("$.data.page").value(0)); + } } diff --git a/server/skillhub-app/src/test/java/com/iflytek/skillhub/service/SkillSearchAppServiceTest.java b/server/skillhub-app/src/test/java/com/iflytek/skillhub/service/SkillSearchAppServiceTest.java index dc176c8b..dd442b95 100644 --- a/server/skillhub-app/src/test/java/com/iflytek/skillhub/service/SkillSearchAppServiceTest.java +++ b/server/skillhub-app/src/test/java/com/iflytek/skillhub/service/SkillSearchAppServiceTest.java @@ -21,10 +21,13 @@ import org.mockito.junit.jupiter.MockitoExtension; import java.util.List; import java.util.Map; -import java.util.Optional; import static org.junit.jupiter.api.Assertions.assertEquals; import static org.junit.jupiter.api.Assertions.assertThrows; +import static org.mockito.ArgumentMatchers.any; +import static org.mockito.ArgumentMatchers.anyList; +import static org.mockito.Mockito.times; +import static org.mockito.Mockito.verify; import static org.mockito.Mockito.when; @ExtendWith(MockitoExtension.class) @@ -54,57 +57,46 @@ class SkillSearchAppServiceTest { skillRepository, namespaceRepository, namespaceService, - new VisibilityChecker(), new SkillLifecycleProjectionService(skillVersionRepository) ); } @Test void search_shouldExcludeArchivedNamespaceSkillsForAnonymousUsers() { - Skill archivedSkill = new Skill(1L, "archived-skill", "owner-1", SkillVisibility.PUBLIC); - setField(archivedSkill, "id", 10L); - - Namespace archivedNamespace = new Namespace("archived-team", "Archived Team", "owner-1"); - setField(archivedNamespace, "id", 1L); - archivedNamespace.setStatus(NamespaceStatus.ARCHIVED); - when(searchQueryService.search(org.mockito.ArgumentMatchers.any())) - .thenReturn(new SearchResult(List.of(10L), 1, 0, 20)); - when(skillRepository.findByIdIn(List.of(10L))).thenReturn(List.of(archivedSkill)); - when(namespaceRepository.findByIdIn(List.of(1L))).thenReturn(List.of(archivedNamespace)); + .thenReturn(new SearchResult(List.of(), 0, 0, 20)); SkillSearchAppService.SearchResponse response = service.search("archive", null, "newest", 0, 20, null, null); assertEquals(0, response.items().size()); assertEquals(0, response.total()); + verify(skillRepository, times(0)).findByIdIn(anyList()); } @Test void search_shouldFillVisiblePageAcrossArchivedNamespaceResults() { - Skill archivedSkill = new Skill(1L, "archived-skill", "owner-1", SkillVisibility.PUBLIC); - setField(archivedSkill, "id", 10L); - archivedSkill.setLatestVersionId(110L); Skill visibleSkill = new Skill(2L, "visible-skill", "owner-1", SkillVisibility.PUBLIC); setField(visibleSkill, "id", 11L); visibleSkill.setLatestVersionId(111L); - Namespace archivedNamespace = new Namespace("archived-team", "Archived Team", "owner-1"); - setField(archivedNamespace, "id", 1L); - archivedNamespace.setStatus(NamespaceStatus.ARCHIVED); Namespace activeNamespace = new Namespace("team-a", "Team A", "owner-1"); setField(activeNamespace, "id", 2L); activeNamespace.setStatus(NamespaceStatus.ACTIVE); when(searchQueryService.search(org.mockito.ArgumentMatchers.any())) - .thenReturn(new SearchResult(List.of(10L, 11L), 2, 0, 20)); - when(skillRepository.findByIdIn(List.of(10L, 11L))).thenReturn(List.of(archivedSkill, visibleSkill)); - when(namespaceRepository.findByIdIn(List.of(1L, 2L))).thenReturn(List.of(archivedNamespace, activeNamespace)); + .thenReturn(new SearchResult(List.of(11L), 1, 0, 20)); + when(skillRepository.findByIdIn(List.of(11L))).thenReturn(List.of(visibleSkill)); + when(namespaceRepository.findByIdIn(List.of(2L))).thenReturn(List.of(activeNamespace)); + when(skillVersionRepository.findByIdIn(List.of(111L))).thenReturn(List.of()); + when(skillVersionRepository.findBySkillIdInAndStatus(List.of(11L), com.iflytek.skillhub.domain.skill.SkillVersionStatus.PUBLISHED)) + .thenReturn(List.of()); SkillSearchAppService.SearchResponse response = service.search("skill", null, "newest", 0, 1, null, null); assertEquals(1, response.items().size()); assertEquals("visible-skill", response.items().getFirst().slug()); assertEquals(1, response.total()); + verify(searchQueryService, times(1)).search(any()); } @Test @@ -141,9 +133,12 @@ class SkillSearchAppServiceTest { namespace.setStatus(NamespaceStatus.ACTIVE); when(searchQueryService.search(org.mockito.ArgumentMatchers.any())) - .thenReturn(new SearchResult(List.of(10L, 11L), 2, 0, 20)); - when(skillRepository.findByIdIn(List.of(10L, 11L))).thenReturn(List.of(visibleSkill, hiddenSkill)); + .thenReturn(new SearchResult(List.of(10L), 1, 0, 20)); + when(skillRepository.findByIdIn(List.of(10L))).thenReturn(List.of(visibleSkill)); when(namespaceRepository.findByIdIn(List.of(1L))).thenReturn(List.of(namespace)); + when(skillVersionRepository.findByIdIn(List.of(101L))).thenReturn(List.of()); + when(skillVersionRepository.findBySkillIdInAndStatus(List.of(10L), com.iflytek.skillhub.domain.skill.SkillVersionStatus.PUBLISHED)) + .thenReturn(List.of()); SkillSearchAppService.SearchResponse response = service.search("skill", null, "newest", 0, 20, "user-9", Map.of()); @@ -152,6 +147,35 @@ class SkillSearchAppServiceTest { assertEquals(1, response.total()); } + @Test + void search_shouldResolvePublishedVersionsInBatch() { + Skill first = new Skill(1L, "skill-a", "owner-1", SkillVisibility.PUBLIC); + setField(first, "id", 10L); + first.setLatestVersionId(101L); + Skill second = new Skill(1L, "skill-b", "owner-1", SkillVisibility.PUBLIC); + setField(second, "id", 11L); + second.setLatestVersionId(102L); + + Namespace namespace = new Namespace("team-a", "Team A", "owner-1"); + setField(namespace, "id", 1L); + namespace.setStatus(NamespaceStatus.ACTIVE); + + when(searchQueryService.search(any())) + .thenReturn(new SearchResult(List.of(10L, 11L), 2, 0, 20)); + when(skillRepository.findByIdIn(List.of(10L, 11L))).thenReturn(List.of(first, second)); + when(namespaceRepository.findByIdIn(List.of(1L))).thenReturn(List.of(namespace)); + when(skillVersionRepository.findByIdIn(List.of(101L, 102L))).thenReturn(List.of()); + when(skillVersionRepository.findBySkillIdInAndStatus(List.of(10L, 11L), com.iflytek.skillhub.domain.skill.SkillVersionStatus.PUBLISHED)) + .thenReturn(List.of()); + + SkillSearchAppService.SearchResponse response = service.search(null, null, "newest", 0, 20, null, null); + + assertEquals(2, response.items().size()); + verify(skillVersionRepository, times(1)).findByIdIn(List.of(101L, 102L)); + verify(skillVersionRepository, times(1)) + .findBySkillIdInAndStatus(List.of(10L, 11L), com.iflytek.skillhub.domain.skill.SkillVersionStatus.PUBLISHED); + } + private void setField(Object target, String fieldName, Object value) { try { java.lang.reflect.Field field = target.getClass().getDeclaredField(fieldName); diff --git a/server/skillhub-domain/src/main/java/com/iflytek/skillhub/domain/skill/SkillVersionRepository.java b/server/skillhub-domain/src/main/java/com/iflytek/skillhub/domain/skill/SkillVersionRepository.java index eaeacd13..c98e2364 100644 --- a/server/skillhub-domain/src/main/java/com/iflytek/skillhub/domain/skill/SkillVersionRepository.java +++ b/server/skillhub-domain/src/main/java/com/iflytek/skillhub/domain/skill/SkillVersionRepository.java @@ -10,6 +10,7 @@ public interface SkillVersionRepository { Optional findById(Long id); List findByIdIn(List ids); List findBySkillIdIn(List skillIds); + List findBySkillIdInAndStatus(List skillIds, SkillVersionStatus status); List findBySkillId(Long skillId); Optional findBySkillIdAndVersion(Long skillId, String version); List findBySkillIdAndStatus(Long skillId, SkillVersionStatus status); diff --git a/server/skillhub-domain/src/main/java/com/iflytek/skillhub/domain/skill/service/SkillLifecycleProjectionService.java b/server/skillhub-domain/src/main/java/com/iflytek/skillhub/domain/skill/service/SkillLifecycleProjectionService.java index e374f3b9..c038b57d 100644 --- a/server/skillhub-domain/src/main/java/com/iflytek/skillhub/domain/skill/service/SkillLifecycleProjectionService.java +++ b/server/skillhub-domain/src/main/java/com/iflytek/skillhub/domain/skill/service/SkillLifecycleProjectionService.java @@ -8,6 +8,9 @@ import com.iflytek.skillhub.domain.skill.SkillVersionStatus; import java.util.Comparator; import java.util.List; import java.util.Map; +import java.util.Objects; +import java.util.function.Function; +import java.util.stream.Collectors; import org.springframework.stereotype.Service; /** @@ -62,6 +65,44 @@ public class SkillLifecycleProjectionService { return new Projection(headlineVersion, publishedVersion, ownerPreviewVersion, resolutionMode); } + public Map projectPublishedSummaries(List skills) { + if (skills.isEmpty()) { + return Map.of(); + } + + Map latestVersionsById = skillVersionRepository.findByIdIn( + skills.stream() + .map(Skill::getLatestVersionId) + .filter(Objects::nonNull) + .distinct() + .toList()) + .stream() + .collect(Collectors.toMap(SkillVersion::getId, Function.identity())); + + Map publishedBySkillId = new java.util.HashMap<>(); + List unresolvedSkillIds = new java.util.ArrayList<>(); + for (Skill skill : skills) { + SkillVersion latestVersion = latestVersionsById.get(skill.getLatestVersionId()); + if (latestVersion != null && latestVersion.getStatus() == SkillVersionStatus.PUBLISHED) { + publishedBySkillId.put(skill.getId(), latestVersion); + } else { + unresolvedSkillIds.add(skill.getId()); + } + } + + if (!unresolvedSkillIds.isEmpty()) { + for (SkillVersion version : skillVersionRepository.findBySkillIdInAndStatus(unresolvedSkillIds, SkillVersionStatus.PUBLISHED)) { + publishedBySkillId.merge(version.getSkillId(), version, this::newerVersion); + } + } + + return skills.stream().collect(Collectors.toMap(Skill::getId, skill -> { + VersionProjection publishedVersion = toProjection(publishedBySkillId.get(skill.getId())); + ResolutionMode resolutionMode = publishedVersion == null ? ResolutionMode.NONE : ResolutionMode.PUBLISHED; + return new Projection(publishedVersion, publishedVersion, null, resolutionMode); + })); + } + private SkillVersion resolvePublishedVersion(Skill skill) { if (skill.getLatestVersionId() != null) { SkillVersion latest = skillVersionRepository.findById(skill.getLatestVersionId()).orElse(null); @@ -109,6 +150,10 @@ public class SkillLifecycleProjectionService { .thenComparing(SkillVersion::getId, Comparator.nullsLast(Comparator.naturalOrder())); } + private SkillVersion newerVersion(SkillVersion left, SkillVersion right) { + return versionComparator().compare(left, right) >= 0 ? left : right; + } + private VersionProjection toProjection(SkillVersion version) { if (version == null) { return null; diff --git a/server/skillhub-infra/src/main/java/com/iflytek/skillhub/infra/jpa/SkillVersionJpaRepository.java b/server/skillhub-infra/src/main/java/com/iflytek/skillhub/infra/jpa/SkillVersionJpaRepository.java index ca35771f..8bddd442 100644 --- a/server/skillhub-infra/src/main/java/com/iflytek/skillhub/infra/jpa/SkillVersionJpaRepository.java +++ b/server/skillhub-infra/src/main/java/com/iflytek/skillhub/infra/jpa/SkillVersionJpaRepository.java @@ -19,6 +19,7 @@ public interface SkillVersionJpaRepository extends JpaRepository findByIdIn(List ids); List findBySkillId(Long skillId); List findBySkillIdIn(List skillIds); + List findBySkillIdInAndStatusOrderByCreatedAtDesc(List skillIds, SkillVersionStatus status); Optional findBySkillIdAndVersion(Long skillId, String version); @Override @@ -26,6 +27,11 @@ public interface SkillVersionJpaRepository extends JpaRepository findBySkillIdInAndStatus(List skillIds, SkillVersionStatus status) { + return findBySkillIdInAndStatusOrderByCreatedAtDesc(skillIds, status); + } + List findBySkillIdAndStatusOrderByCreatedAtDesc(Long skillId, SkillVersionStatus status); Page findBySkillIdAndStatus(Long skillId, SkillVersionStatus status, Pageable pageable); } diff --git a/server/skillhub-search/src/main/java/com/iflytek/skillhub/search/postgres/PostgresFullTextQueryService.java b/server/skillhub-search/src/main/java/com/iflytek/skillhub/search/postgres/PostgresFullTextQueryService.java index d5eb7e43..479df7cb 100644 --- a/server/skillhub-search/src/main/java/com/iflytek/skillhub/search/postgres/PostgresFullTextQueryService.java +++ b/server/skillhub-search/src/main/java/com/iflytek/skillhub/search/postgres/PostgresFullTextQueryService.java @@ -98,22 +98,33 @@ public class PostgresFullTextQueryService implements SearchQueryService { : query.visibilityScope().adminNamespaceIds(); StringBuilder sql = new StringBuilder(); - sql.append("SELECT skill_id FROM skill_search_document WHERE 1=1 "); + sql.append("SELECT d.skill_id "); + sql.append("FROM skill_search_document d "); + sql.append("JOIN skill s ON s.id = d.skill_id "); + sql.append("JOIN namespace n ON n.id = d.namespace_id "); + sql.append("WHERE 1=1 "); // Visibility filtering - sql.append("AND (visibility = 'PUBLIC' "); + sql.append("AND (d.visibility = 'PUBLIC' "); if (query.visibilityScope().userId() != null) { - sql.append("OR (visibility = 'NAMESPACE_ONLY' AND namespace_id IN :memberNamespaceIds) "); - sql.append("OR (visibility = 'PRIVATE' AND (namespace_id IN :adminNamespaceIds OR owner_id = :userId)) "); + sql.append("OR (d.visibility = 'NAMESPACE_ONLY' AND d.namespace_id IN :memberNamespaceIds) "); + sql.append("OR (d.visibility = 'PRIVATE' AND (d.namespace_id IN :adminNamespaceIds OR d.owner_id = :userId)) "); } sql.append(") "); // Status filtering - sql.append("AND status = 'ACTIVE' "); + sql.append("AND d.status = 'ACTIVE' "); + sql.append("AND s.status = 'ACTIVE' "); + sql.append("AND s.hidden = FALSE "); + sql.append("AND (n.status <> 'ARCHIVED' "); + if (query.visibilityScope().userId() != null) { + sql.append("OR d.namespace_id IN :memberNamespaceIds "); + } + sql.append(") "); // Namespace filtering if (query.namespaceId() != null) { - sql.append("AND namespace_id = :namespaceId "); + sql.append("AND d.namespace_id = :namespaceId "); } // Full-text search @@ -123,7 +134,7 @@ public class PostgresFullTextQueryService implements SearchQueryService { if (useShortPrefixTitleSearch) { sql.append(TITLE_VECTOR_SQL).append(" @@ to_tsquery('simple', :tsQuery) "); } else { - sql.append("search_vector @@ to_tsquery('simple', :tsQuery) "); + sql.append("d.search_vector @@ to_tsquery('simple', :tsQuery) "); } sql.append(" OR "); } @@ -133,13 +144,11 @@ public class PostgresFullTextQueryService implements SearchQueryService { // Sorting if ("downloads".equals(query.sortBy())) { - sql.append("ORDER BY (SELECT download_count FROM skill WHERE id = skill_id) DESC, "); - sql.append("(SELECT updated_at FROM skill WHERE id = skill_id) DESC, skill_id DESC "); + sql.append("ORDER BY s.download_count DESC, s.updated_at DESC, d.skill_id DESC "); } else if ("rating".equals(query.sortBy())) { - sql.append("ORDER BY (SELECT rating_avg FROM skill WHERE id = skill_id) DESC, "); - sql.append("(SELECT updated_at FROM skill WHERE id = skill_id) DESC, skill_id DESC "); + sql.append("ORDER BY s.rating_avg DESC, s.updated_at DESC, d.skill_id DESC "); } else if ("newest".equals(query.sortBy())) { - sql.append("ORDER BY (SELECT updated_at FROM skill WHERE id = skill_id) DESC, skill_id DESC "); + sql.append("ORDER BY s.updated_at DESC, d.skill_id DESC "); } else if (useRelevanceOrdering) { sql.append("ORDER BY CASE "); sql.append("WHEN ").append(TITLE_SQL).append(" = :titleExact THEN 4 "); @@ -148,14 +157,14 @@ public class PostgresFullTextQueryService implements SearchQueryService { sql.append("ELSE 1 END DESC, "); if (useShortPrefixTitleSearch) { sql.append("ts_rank_cd(").append(TITLE_VECTOR_SQL) - .append(", to_tsquery('simple', :tsQuery)) DESC, updated_at DESC, skill_id DESC "); + .append(", to_tsquery('simple', :tsQuery)) DESC, d.updated_at DESC, d.skill_id DESC "); } else if (hasTsQuery) { - sql.append("ts_rank_cd(search_vector, to_tsquery('simple', :tsQuery)) DESC, updated_at DESC, skill_id DESC "); + sql.append("ts_rank_cd(d.search_vector, to_tsquery('simple', :tsQuery)) DESC, d.updated_at DESC, d.skill_id DESC "); } else { - sql.append("updated_at DESC, skill_id DESC "); + sql.append("d.updated_at DESC, d.skill_id DESC "); } } else { - sql.append("ORDER BY updated_at DESC, skill_id DESC "); + sql.append("ORDER BY s.updated_at DESC, d.skill_id DESC "); } // Pagination @@ -193,7 +202,7 @@ public class PostgresFullTextQueryService implements SearchQueryService { .toList(); // Count total - String countSql = sql.toString().replaceFirst("SELECT skill_id", "SELECT COUNT(*)"); + String countSql = sql.toString().replaceFirst("SELECT d\\.skill_id", "SELECT COUNT(*)"); int orderByIndex = countSql.indexOf("ORDER BY"); if (orderByIndex >= 0) { countSql = countSql.substring(0, orderByIndex); diff --git a/server/skillhub-search/src/test/java/com/iflytek/skillhub/search/postgres/PostgresFullTextQueryServiceTest.java b/server/skillhub-search/src/test/java/com/iflytek/skillhub/search/postgres/PostgresFullTextQueryServiceTest.java index 5ca138e6..ac0f44ab 100644 --- a/server/skillhub-search/src/test/java/com/iflytek/skillhub/search/postgres/PostgresFullTextQueryServiceTest.java +++ b/server/skillhub-search/src/test/java/com/iflytek/skillhub/search/postgres/PostgresFullTextQueryServiceTest.java @@ -8,6 +8,7 @@ import com.iflytek.skillhub.search.SearchVisibilityScope; import jakarta.persistence.EntityManager; import jakarta.persistence.Query; import org.junit.jupiter.api.Test; +import org.mockito.ArgumentCaptor; import java.util.List; import java.util.Set; @@ -108,7 +109,7 @@ class PostgresFullTextQueryServiceTest { var sqlCaptor = org.mockito.ArgumentCaptor.forClass(String.class); verify(entityManager, org.mockito.Mockito.times(2)).createNativeQuery(sqlCaptor.capture()); assertThat(sqlCaptor.getAllValues().getFirst()).contains("search_vector @@ to_tsquery('simple', :tsQuery)"); - assertThat(sqlCaptor.getAllValues().getFirst()).contains("ts_rank_cd(search_vector, to_tsquery('simple', :tsQuery))"); + assertThat(sqlCaptor.getAllValues().getFirst()).contains("ts_rank_cd(d.search_vector, to_tsquery('simple', :tsQuery))"); } @Test @@ -233,7 +234,11 @@ class PostgresFullTextQueryServiceTest { var sqlCaptor = org.mockito.ArgumentCaptor.forClass(String.class); verify(entityManager, org.mockito.Mockito.times(2)).createNativeQuery(sqlCaptor.capture()); assertThat(sqlCaptor.getAllValues().getFirst()) - .contains("ORDER BY (SELECT download_count FROM skill WHERE id = skill_id) DESC, (SELECT updated_at FROM skill WHERE id = skill_id) DESC, skill_id DESC"); + .contains("JOIN skill s ON s.id = d.skill_id") + .contains("JOIN namespace n ON n.id = d.namespace_id") + .contains("AND s.hidden = FALSE") + .contains("AND (n.status <> 'ARCHIVED' ") + .contains("ORDER BY s.download_count DESC, s.updated_at DESC, d.skill_id DESC"); } @Test @@ -263,7 +268,100 @@ class PostgresFullTextQueryServiceTest { var sqlCaptor = org.mockito.ArgumentCaptor.forClass(String.class); verify(entityManager, org.mockito.Mockito.times(2)).createNativeQuery(sqlCaptor.capture()); assertThat(sqlCaptor.getAllValues().getFirst()) - .contains("ORDER BY updated_at DESC, skill_id DESC"); + .contains("ORDER BY s.updated_at DESC, d.skill_id DESC"); + } + + @Test + void authenticatedQueriesShouldAllowArchivedNamespacesForMembers() { + EntityManager entityManager = mock(EntityManager.class); + Query nativeQuery = mock(Query.class); + Query countQuery = mock(Query.class); + when(entityManager.createNativeQuery(anyString())) + .thenReturn(nativeQuery) + .thenReturn(countQuery); + when(nativeQuery.setParameter(anyString(), org.mockito.ArgumentMatchers.any())).thenReturn(nativeQuery); + when(countQuery.setParameter(anyString(), org.mockito.ArgumentMatchers.any())).thenReturn(countQuery); + when(nativeQuery.getResultList()).thenReturn(List.of()); + when(countQuery.getSingleResult()).thenReturn(0L); + + PostgresFullTextQueryService service = new PostgresFullTextQueryService(entityManager); + + service.search(new SearchQuery( + null, + null, + new SearchVisibilityScope("user-1", Set.of(7L), Set.of()), + "newest", + 0, + 12 + )); + + var sqlCaptor = org.mockito.ArgumentCaptor.forClass(String.class); + verify(entityManager, org.mockito.Mockito.times(2)).createNativeQuery(sqlCaptor.capture()); + assertThat(sqlCaptor.getAllValues().getFirst()).contains("OR d.namespace_id IN :memberNamespaceIds"); + } + + @Test + void maliciousKeywordShouldBeBoundAsParameterInsteadOfInlinedIntoSql() { + EntityManager entityManager = mock(EntityManager.class); + Query nativeQuery = mock(Query.class); + Query countQuery = mock(Query.class); + when(entityManager.createNativeQuery(anyString())) + .thenReturn(nativeQuery) + .thenReturn(countQuery); + when(nativeQuery.setParameter(anyString(), org.mockito.ArgumentMatchers.any())).thenReturn(nativeQuery); + when(countQuery.setParameter(anyString(), org.mockito.ArgumentMatchers.any())).thenReturn(countQuery); + when(nativeQuery.getResultList()).thenReturn(List.of()); + when(countQuery.getSingleResult()).thenReturn(0L); + + PostgresFullTextQueryService service = new PostgresFullTextQueryService(entityManager); + String payload = "x%' OR 1=1 --"; + + service.search(new SearchQuery( + payload, + null, + new SearchVisibilityScope(null, Set.of(), Set.of()), + "relevance", + 0, + 12 + )); + + ArgumentCaptor sqlCaptor = ArgumentCaptor.forClass(String.class); + verify(entityManager, org.mockito.Mockito.times(2)).createNativeQuery(sqlCaptor.capture()); + assertThat(sqlCaptor.getAllValues().getFirst()).doesNotContain(payload); + verify(nativeQuery).setParameter("titleLike", "%" + payload.toLowerCase() + "%"); + verify(countQuery).setParameter("titleLike", "%" + payload.toLowerCase() + "%"); + } + + @Test + void maliciousSortShouldFallBackWithoutBeingInlinedIntoSql() { + EntityManager entityManager = mock(EntityManager.class); + Query nativeQuery = mock(Query.class); + Query countQuery = mock(Query.class); + when(entityManager.createNativeQuery(anyString())) + .thenReturn(nativeQuery) + .thenReturn(countQuery); + when(nativeQuery.setParameter(anyString(), org.mockito.ArgumentMatchers.any())).thenReturn(nativeQuery); + when(countQuery.setParameter(anyString(), org.mockito.ArgumentMatchers.any())).thenReturn(countQuery); + when(nativeQuery.getResultList()).thenReturn(List.of()); + when(countQuery.getSingleResult()).thenReturn(0L); + + PostgresFullTextQueryService service = new PostgresFullTextQueryService(entityManager); + String payload = "newest desc; drop table skill; --"; + + service.search(new SearchQuery( + null, + null, + new SearchVisibilityScope(null, Set.of(), Set.of()), + payload, + 0, + 12 + )); + + ArgumentCaptor sqlCaptor = ArgumentCaptor.forClass(String.class); + verify(entityManager, org.mockito.Mockito.times(2)).createNativeQuery(sqlCaptor.capture()); + assertThat(sqlCaptor.getAllValues().getFirst()) + .doesNotContain(payload) + .contains("ORDER BY s.updated_at DESC, d.skill_id DESC"); } @Test