[LFXV2-456] feat: Add QueryResourcesCount API endpoint #20
Conversation
Implement resource count aggregation endpoint to provide efficient counting of resources with access control support. Changes include: - Add new GET /query/resources/count endpoint in Goa design - Implement QueryResourcesCount method in service layer - Add resource count domain model and service interfaces - Update mock implementation to support count operations - Fix OpenSearch aggregation field to use .keyword subfield - Add access control integration for private resource counting Generated with Claude Code (https://claude.ai/code) Signed-off-by: Andres Tobon <[email protected]>
Add complete test coverage for the new resource count functionality: - Service layer tests for QueryResourcesCount endpoint with various scenarios - OpenSearch implementation tests with mock client for count and aggregation - Mock resource searcher tests covering count operations and filters - Enhanced mock implementations with test helper methods - Service struct verification for new resourceCountService field Tests cover success scenarios, error handling, access control integration, and all query parameters (name, type, tags, parent filters). Generated with Claude Code (https://claude.ai/code) Signed-off-by: Andres Tobon <[email protected]>
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughAdds a new GET /query/resources/count endpoint and routing rule. Implements count and aggregation support across service, domain, and OpenSearch layers, including new models, criteria, renderers, and client methods. Introduces access-control-aware counting logic, mocks, tests, and a default bucket size constant. No existing endpoint behavior changed. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor C as Client
participant API as query-svc (HTTP)
participant SVC as Service Handler
participant RS as Resource Service
participant SRCH as ResourceSearcher (domain)
participant OSR as OpenSearchSearcher (infra)
participant OSC as OpenSearch Client
C->>API: GET /query/resources/count<br/>Authorization, query params
API->>SVC: QueryResourcesCount(payload)
SVC->>RS: QueryResourcesCount(countCriteria, aggCriteria)
alt Anonymous principal
RS->>OSR: QueryResourcesCount(public countCriteria, aggCriteria, publicOnly=true)
OSR->>OSC: Count(index, query)
OSC-->>OSR: CountResponse
opt Aggregation requested
OSR->>OSC: AggregationSearch(index, query)
OSC-->>OSR: AggregationResponse
end
OSR-->>RS: CountResult (public only)
RS-->>SVC: CountResult (+cache-control anonymous)
else Authenticated principal
RS->>OSR: QueryResourcesCount(public countCriteria, aggCriteria, publicOnly=false)
OSR->>OSC: Count(index, query)
OSC-->>OSR: CountResponse
OSR->>OSC: AggregationSearch(index, query)
OSC-->>OSR: AggregationResponse (group_by buckets)
OSR-->>RS: CountResult (public agg)
RS->>RS: BuildCountMessage(buckets)
RS->>AC: CheckAccess(message)
AC-->>RS: Access decisions per bucket
RS->>RS: Combine public + allowed private
RS-->>SVC: Final CountResult
end
SVC-->>API: CountResult
API-->>C: 200 OK (count, has_more, cache_control)
note over RS,OSR: Error paths wrap and return BadRequest/500 per handler logic
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests
Tip 👮 Agentic pre-merge checks are now available in preview!Pro plan users can now enable pre-merge checks in their settings to enforce checklists before merging PRs.
Please see the documentation for more information. Example: reviews:
pre_merge_checks:
custom_checks:
- name: "Undocumented Breaking Changes"
mode: "warning"
instructions: |
Pass/fail criteria: All breaking changes to public APIs, CLI flags, environment variables, configuration keys, database schemas, or HTTP/GraphQL endpoints must be documented in the "Breaking Change" section of the PR description and in CHANGELOG.md. Exclude purely internal or private changes (e.g., code not exported from package entry points or explicitly marked as internal).Please share your feedback with us on this Discord post. Comment |
There was a problem hiding this comment.
Pull Request Overview
This PR implements a new QueryResourcesCount API endpoint that provides efficient resource counting with access control support. The endpoint allows users to count resources based on various criteria without fetching full resource data, improving performance for scenarios where only counts are needed.
Key changes include:
- Added
/query/resources/countGET endpoint with filtering support for name, type, tags, and parent - Implemented access control integration to count both public and private resources based on user permissions
- Added OpenSearch count and aggregation query support with template enhancements
- Enhanced mock implementations for comprehensive testing coverage
Reviewed Changes
Copilot reviewed 37 out of 37 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| pkg/constants/query.go | Added DefaultBucketSize constant for aggregation queries |
| internal/service/resource_count.go | New service layer implementing count logic with access control |
| internal/service/resource_count_test.go | Comprehensive test suite for resource count service |
| internal/infrastructure/opensearch/searcher.go | Added QueryResourcesCount method and count response conversion |
| internal/infrastructure/opensearch/template.go | Enhanced query template with aggregation and conditional size handling |
| internal/infrastructure/opensearch/models.go | Added count and aggregation response models |
| internal/infrastructure/opensearch/client.go | Implemented Count and AggregationSearch methods |
| internal/infrastructure/mock/resource_searcher.go | Enhanced mock with count operations and test helpers |
| internal/domain/port/searcher.go | Added QueryResourcesCount interface method |
| internal/domain/model/search_criteria.go | Added count-specific criteria and result models |
| gen/* | Generated API code for new endpoint including HTTP handlers, clients, and OpenAPI specs |
| cmd/service/service.go | Integrated resource count service into main service |
| design/query-svc.go | API design definition for count endpoint |
Comments suppressed due to low confidence (1)
internal/infrastructure/opensearch/searcher.go:1
- The method
convertResponsehas been renamed toconvertSearchResponsein the diff, but this reference wasn't updated. This will cause a compilation error.
// Copyright The Linux Foundation and each contributor to LFX.
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
internal/infrastructure/opensearch/searcher.go (1)
151-158: Bug: wrong field used for total results.convertSearchResponse sets Total from response.Value, which does not exist (should be response.Hits.Total.Value). This will not compile or will produce zero totals if a similarly named field was added.
Apply this diff:
result := &model.SearchResult{ Resources: make([]model.Resource, 0, len(response.Hits.Hits)), PageToken: response.PageToken, - Total: response.Value, + Total: response.Hits.Total.Value, }
🧹 Nitpick comments (23)
pkg/constants/query.go (1)
10-11: Clarify constant comment to match group-by semantics.Prefer “terms aggregation bucket size” to align with GroupBy/GroupBySize naming across the codebase.
- // DefaultBucketSize is the default size of the bucket for queries + // DefaultBucketSize is the default terms aggregation bucket size for group-by queriesinternal/domain/port/searcher.go (1)
19-21: Remove redundant publicOnly parameter from the port.Public visibility is already carried by SearchCriteria (PublicOnly/PrivateOnly). Passing an extra boolean invites divergence.
- // QueryResourcesCount searches for resources based on the provided criteria - QueryResourcesCount(ctx context.Context, countCriteria model.SearchCriteria, aggregationCriteria model.SearchCriteria, publicOnly bool) (*model.CountResult, error) + // QueryResourcesCount counts resources and (optionally) returns grouped aggregations + QueryResourcesCount(ctx context.Context, countCriteria model.SearchCriteria, aggregationCriteria model.SearchCriteria) (*model.CountResult, error)If you need to override visibility centrally, set criteria.PublicOnly/PrivateOnly in the service before calling the port.
internal/domain/model/search_criteria.go (2)
50-60: Use uint64 for CountResult.Count to match API and OpenSearch.The design uses UInt64 and OpenSearch returns non-negative counts. Align the domain type.
type CountResult struct { - // Count number of resources found - Count int + // Count number of resources found + Count uint64 // Aggregations Aggregation TermsAggregation // HasMore indicates if there are more results HasMore bool // Cache control header CacheControl *string }
30-35: Document/validate exclusivity of PublicOnly vs PrivateOnly.Make these flags mutually exclusive at validation time to avoid ambiguous queries.
// PrivateOnly indicates if only private resources should be returned - PrivateOnly bool + PrivateOnly bool // must not be true when PublicOnly is trueOptionally add a central validator in the service layer to reject conflicting inputs.
design/query-svc.go (1)
107-109: Apply the same parent format constraint as query-resources.Ensure parity and early input validation.
- dsl.Attribute("parent", dsl.String, "Parent (for navigation; varies by object type)", func() { - dsl.Example("project:123") - }) + dsl.Attribute("parent", dsl.String, "Parent (for navigation; varies by object type)", func() { + dsl.Example("project:123") + dsl.Pattern(`^[a-zA-Z]+:[a-zA-Z0-9_-]+$`) + })cmd/service/converters.go (2)
82-105: Parent vs ParentRef: set both to avoid selector mismatchesDownstream searchers sometimes key off
Parent(navigation) vsParentRef(exact ref). To be safe/consistent withpayloadToCriteria, set both when provided.if payload.Parent != nil { - criteria.ParentRef = payload.Parent + criteria.Parent = payload.Parent + criteria.ParentRef = payload.Parent }Would you confirm which field the OpenSearch templates consume today? If only one is honored, we can drop the other.
107-119: Avoid magic string for GroupBy; extract a constant
"access_check_query.keyword"must match the index mapping. Centralize it to prevent typos and ease future changes.+// at top of file (package scope) +const groupByAccessCheckQuery = "access_check_query.keyword" ... criteria := model.SearchCriteria{ GroupBySize: constants.DefaultBucketSize, // We only want the aggregation, not the actual results. PageSize: 0, // The aggregation results will only count private resources. PrivateOnly: true, // Set the attribute to aggregate on. // Use .keyword subfield for aggregation on text fields - GroupBy: "access_check_query.keyword", + GroupBy: groupByAccessCheckQuery, }internal/infrastructure/mock/access_control.go (1)
43-51: Type-safety for test helpersPrefer accepting/returning
model.AccessCheckResultin helpers to avoid accidental type drift if the alias changes.-// SetCheckAccessResponse sets the mock response for CheckAccess calls -func (m *MockAccessControlChecker) SetCheckAccessResponse(response map[string]string) { - m.checkAccessResponse = response -} +// SetCheckAccessResponse sets the mock response for CheckAccess calls +func (m *MockAccessControlChecker) SetCheckAccessResponse(response model.AccessCheckResult) { + m.checkAccessResponse = response +}No behavior change; improves compile-time guarantees.
internal/infrastructure/mock/resource_searcher_test.go (1)
110-118: Make aggregation intent explicit in the test
aggregationCriteriasets an emptyResourceTypebut not the grouping field. If the mock relies on defaults today, future changes may break this test. Consider settingGroupByexplicitly to the grouped attribute used by the mock (e.g.,"type").-aggregationCriteria := model.SearchCriteria{ - ResourceType: stringPtr(""), -} +aggregationCriteria := model.SearchCriteria{ + GroupBy: "type", +}Also applies to: 127-135
internal/service/resource_count_test.go (1)
80-110: Test name vs expectation mismatchName says “with private access,” but expectation equals the public-only count and the comment says private access didn’t work. Clarify intent: either assert the augmented count (public + allowed private) or rename to reflect ignoring private results.
- name: "successful count with authenticated user - with private access", + name: "successful count with authenticated user - private access ignored (by design)",If the service should add allowed private counts, update
expectedCountaccordingly and ensure mocks reflect that.cmd/service/service_test.go (1)
292-294: Assert boolean value, not non-nil
HasMoreis a bool;assert.NotNilis vacuous. Assert the expected boolean instead.- // HasMore is returned from the service - assert.NotNil(t, result.HasMore) + // HasMore is returned from the service + assert.Equal(t, false, result.HasMore)If specific scenarios should set it true, assert that per test case.
cmd/service/service.go (2)
70-74: Fix typos in public method commentMinor doc nits: “hase” → “has”; pluralize “relationships”.
-// QueryResourcesCount returns an aggregate count of resources the user hase -// access to, by implementing an aggregation over the stored OpenFGA -// relationship. +// QueryResourcesCount returns an aggregate count of resources the user has +// access to, by implementing an aggregation over the stored OpenFGA +// relationships.
79-90: Consider validating payload.Version or documenting behaviorIf multiple versions are planned, validate
p.Versionhere (or in transport) to avoid silent divergence.Would you like a small guard that rejects unknown versions (e.g., anything other than "1")?
internal/infrastructure/mock/resource_searcher.go (1)
221-343: Honor PrivateOnly and GroupBy in mock count; current behavior can mislead tests.
- PrivateOnly in countCriteria is ignored unless publicOnly is true.
- Aggregation ignores GroupBy/GroupBySize and always groups by type.
Recommend minimal support so tests reflect realistic behavior.
Apply this diff:
@@ - // Filter by public only if requested - for _, resource := range m.resources { - if publicOnly && !resource.Public { - continue - } - filteredResources = append(filteredResources, resource) - } + // Filter by public/private + for _, resource := range m.resources { + if publicOnly && !resource.Public { + continue + } + if countCriteria.PrivateOnly && resource.Public { + continue + } + filteredResources = append(filteredResources, resource) + } @@ - // If aggregation criteria has a resource type, group by that type - if aggregationCriteria.ResourceType != nil && *aggregationCriteria.ResourceType != "" { - // Group resources by type - for _, resource := range filteredResources { - aggregationBuckets[resource.Type]++ - } - } else { - // Default aggregation by resource type - for _, resource := range filteredResources { - aggregationBuckets[resource.Type]++ - } - } + // Group by requested field (mock supports only object_type/type for now) + switch strings.ToLower(aggregationCriteria.GroupBy) { + case "object_type", "type": + for _, resource := range filteredResources { + aggregationBuckets[resource.Type]++ + } + default: + // Default aggregation by resource type + for _, resource := range filteredResources { + aggregationBuckets[resource.Type]++ + } + }internal/infrastructure/opensearch/searcher.go (2)
97-121: Clarify error messages for count vs. aggregation failures.Both errors read “opensearch search failed,” which makes triage harder.
Apply this diff:
- countResponse, err := os.client.Count(ctx, os.index, parsedCount) + countResponse, err := os.client.Count(ctx, os.index, parsedCount) if err != nil { - return nil, fmt.Errorf("opensearch search failed: %w", err) + return nil, fmt.Errorf("opensearch count failed: %w", err) } @@ - aggregationResponse, err := os.client.AggregationSearch(ctx, os.index, parsedSearch) + aggregationResponse, err := os.client.AggregationSearch(ctx, os.index, parsedSearch) if err != nil { - return nil, fmt.Errorf("opensearch search failed: %w", err) + return nil, fmt.Errorf("opensearch aggregation failed: %w", err) }
248-266: Minor nits: variable name and log copy typo.
- err var name is misspelled.
- Log message repeats “created”.
Apply this diff:
- opensearchClient, errpensearchClient := opensearchapi.NewClient(opensearchapi.Config{ + opensearchClient, errOpenSearchClient := opensearchapi.NewClient(opensearchapi.Config{ @@ - if errpensearchClient != nil { - return nil, errors.NewServiceUnavailable("failed to create OpenSearch client", errpensearchClient) + if errOpenSearchClient != nil { + return nil, errors.NewServiceUnavailable("failed to create OpenSearch client", errOpenSearchClient) } - slog.InfoContext(ctx, "created OpenSearch client created successfully", + slog.InfoContext(ctx, "OpenSearch client created successfully", "url", config.URL, "index", config.Index, )internal/infrastructure/opensearch/searcher_test.go (1)
420-431: Add an assertion for Total to catch regressions.convertSearchResponse currently had a bug; asserting Total would have caught it.
Apply this diff:
- assertion.Equal(tc.expectedCount, len(result.Resources)) + assertion.Equal(tc.expectedCount, len(result.Resources)) + assertion.Equal(tc.response.Hits.Total.Value, result.Total)internal/service/resource_count.go (6)
79-92: Log message contains full aggregation; watch log volume.Non-blocking: dumping the full aggregation at debug may be noisy at scale, but fine under debug levels.
105-122: Avoid double-appending @user and fix preallocation; remove dead map.
- docCountMap is unused.
- Prealloc uses PageSize, but buckets are controlled by GroupBy/GroupBySize; use len(buckets).
- If bucket keys might already contain “@user:”, avoid appending again.
Apply this diff:
func (s *ResourceCount) BuildMessage(ctx context.Context, principal string, result *model.CountResult, aggregationCriteria model.SearchCriteria) []byte { - // Create a map to store the "doc_count" of each aggregation bucket. - docCountMap := make(map[string]uint64, aggregationCriteria.PageSize) - - // estimate the size of each line in the access check message - accessCheckMessage := make([]byte, 0, 80*aggregationCriteria.PageSize) + // Estimate buffer: ~64 bytes per line + accessCheckMessage := make([]byte, 0, 64*len(result.Aggregation.Buckets)) for _, bucket := range result.Aggregation.Buckets { - docCountMap[bucket.Key] = bucket.DocCount - accessCheckMessage = append(accessCheckMessage, bucket.Key...) - accessCheckMessage = append(accessCheckMessage, []byte("@user:")...) - accessCheckMessage = append(accessCheckMessage, []byte(principal)...) + key := bucket.Key + if !strings.Contains(key, "@user:") { + key += "@user:" + principal + } + accessCheckMessage = append(accessCheckMessage, key...) accessCheckMessage = append(accessCheckMessage, '\n') } return accessCheckMessage }Remember to add the strings import (see next comment).
124-165: Derive the access-check key consistently with BuildMessage.Align key construction to prevent mismatches if the bucket already contains “@user:”.
Apply this diff:
- // The bucket.Key already contains the full access check query including the principal - // e.g.: "committee:830513f8-0e77-4a48-a8e4-ede4c1a61f98#viewer@user:project_super_admin" - // The BuildMessage function appends "@user:" + principal to create the access check key - // So we need to use the same format here - accessCheckKey := bucket.Key + "@user:" + principal + // Build the same key used in BuildMessage + accessCheckKey := bucket.Key + if !strings.Contains(accessCheckKey, "@user:") { + accessCheckKey += "@user:" + principal + }
6-16: Add missing import for strings.Needed for the suggested key handling.
Apply this diff:
import ( "context" "fmt" "log/slog" "time" + "strings"
58-66: Minor: tighten error message.“search operation failed while executing query resources” is vague in a count path.
Apply this diff:
- return nil, fmt.Errorf("search operation failed: %w", err) + return nil, fmt.Errorf("count operation failed: %w", err)
147-161: Optional: guard against empty accessCheckMessage trim.Safe today due to prior len check, but a quick guard makes it future‑proof.
- // Trim trailing newline. - accessCheckMessage = accessCheckMessage[:len(accessCheckMessage)-1] + // Trim trailing newline. + if n := len(accessCheckMessage); n > 0 && accessCheckMessage[n-1] == '\n' { + accessCheckMessage = accessCheckMessage[:n-1] + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Jira integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
⛔ Files ignored due to path filters (17)
gen/http/cli/lfx_v2_query_service/cli.gois excluded by!**/gen/**gen/http/openapi.jsonis excluded by!**/gen/**gen/http/openapi.yamlis excluded by!**/gen/**gen/http/openapi3.jsonis excluded by!**/gen/**gen/http/openapi3.yamlis excluded by!**/gen/**gen/http/query_svc/client/cli.gois excluded by!**/gen/**gen/http/query_svc/client/client.gois excluded by!**/gen/**gen/http/query_svc/client/encode_decode.gois excluded by!**/gen/**gen/http/query_svc/client/paths.gois excluded by!**/gen/**gen/http/query_svc/client/types.gois excluded by!**/gen/**gen/http/query_svc/server/encode_decode.gois excluded by!**/gen/**gen/http/query_svc/server/paths.gois excluded by!**/gen/**gen/http/query_svc/server/server.gois excluded by!**/gen/**gen/http/query_svc/server/types.gois excluded by!**/gen/**gen/query_svc/client.gois excluded by!**/gen/**gen/query_svc/endpoints.gois excluded by!**/gen/**gen/query_svc/service.gois excluded by!**/gen/**
📒 Files selected for processing (20)
charts/lfx-v2-query-service/templates/ruleset.yaml(1 hunks)cmd/service/converters.go(1 hunks)cmd/service/service.go(3 hunks)cmd/service/service_test.go(3 hunks)design/query-svc.go(1 hunks)internal/domain/model/count.go(1 hunks)internal/domain/model/resource.go(1 hunks)internal/domain/model/search_criteria.go(2 hunks)internal/domain/port/searcher.go(1 hunks)internal/infrastructure/mock/access_control.go(4 hunks)internal/infrastructure/mock/resource_searcher.go(3 hunks)internal/infrastructure/mock/resource_searcher_test.go(1 hunks)internal/infrastructure/opensearch/client.go(2 hunks)internal/infrastructure/opensearch/models.go(1 hunks)internal/infrastructure/opensearch/searcher.go(5 hunks)internal/infrastructure/opensearch/searcher_test.go(5 hunks)internal/infrastructure/opensearch/template.go(3 hunks)internal/service/resource_count.go(1 hunks)internal/service/resource_count_test.go(1 hunks)pkg/constants/query.go(1 hunks)
🧰 Additional context used
📓 Path-based instructions (7)
internal/domain/model/**
📄 CodeRabbit inference engine (CLAUDE.md)
Place core business entities (e.g., Resource, SearchCriteria, AccessCheck) under internal/domain/model/
Files:
internal/domain/model/count.gointernal/domain/model/resource.gointernal/domain/model/search_criteria.go
internal/domain/port/**
📄 CodeRabbit inference engine (CLAUDE.md)
Place domain interfaces (ports) under internal/domain/port/
Files:
internal/domain/port/searcher.go
internal/infrastructure/opensearch/**
📄 CodeRabbit inference engine (CLAUDE.md)
Put OpenSearch implementations for resource search under internal/infrastructure/opensearch/
Files:
internal/infrastructure/opensearch/models.gointernal/infrastructure/opensearch/template.gointernal/infrastructure/opensearch/searcher_test.gointernal/infrastructure/opensearch/client.gointernal/infrastructure/opensearch/searcher.go
internal/infrastructure/mock/**
📄 CodeRabbit inference engine (CLAUDE.md)
Maintain mock implementations under internal/infrastructure/mock/
Files:
internal/infrastructure/mock/resource_searcher_test.gointernal/infrastructure/mock/access_control.gointernal/infrastructure/mock/resource_searcher.go
**/*_test.go
📄 CodeRabbit inference engine (CLAUDE.md)
Name Go test files with the *_test.go suffix and keep them alongside implementation files
Files:
internal/infrastructure/mock/resource_searcher_test.gointernal/infrastructure/opensearch/searcher_test.gointernal/service/resource_count_test.gocmd/service/service_test.go
internal/service/**
📄 CodeRabbit inference engine (CLAUDE.md)
Keep service-layer orchestration logic in internal/service/
Files:
internal/service/resource_count.gointernal/service/resource_count_test.go
design/**
📄 CodeRabbit inference engine (CLAUDE.md)
Keep Goa API design specifications in the design/ directory
Files:
design/query-svc.go
🧠 Learnings (3)
📚 Learning: 2025-09-10T17:40:14.123Z
Learnt from: CR
PR: linuxfoundation/lfx-v2-query-service#0
File: CLAUDE.md:0-0
Timestamp: 2025-09-10T17:40:14.123Z
Learning: Applies to internal/service/resource_search.go : Implement use case orchestration for resource search in internal/service/resource_search.go
Applied to files:
internal/domain/port/searcher.gointernal/infrastructure/mock/resource_searcher_test.gointernal/service/resource_count.go
📚 Learning: 2025-09-08T18:22:24.173Z
Learnt from: mauriciozanettisalomao
PR: linuxfoundation/lfx-v2-query-service#13
File: charts/lfx-v2-query-service/templates/httproute.yaml:16-19
Timestamp: 2025-09-08T18:22:24.173Z
Learning: For the lfx-v2-query-service HTTPRoute configuration, OPTIONS method rules should not be included - only GET method constraints are needed for the /query/orgs endpoint.
Applied to files:
charts/lfx-v2-query-service/templates/ruleset.yaml
📚 Learning: 2025-09-10T17:40:14.123Z
Learnt from: CR
PR: linuxfoundation/lfx-v2-query-service#0
File: CLAUDE.md:0-0
Timestamp: 2025-09-10T17:40:14.123Z
Learning: Applies to cmd/query_svc/query_svc.go : Keep the service implementation that connects Goa to domain logic in cmd/query_svc/query_svc.go
Applied to files:
cmd/service/service.gocmd/service/service_test.go
🧬 Code graph analysis (12)
internal/domain/model/count.go (1)
internal/infrastructure/opensearch/models.go (2)
AggregationBucket(25-28)TermsAggregation(31-35)
internal/domain/port/searcher.go (1)
internal/domain/model/search_criteria.go (2)
SearchCriteria(7-36)CountResult(51-60)
internal/infrastructure/mock/resource_searcher_test.go (2)
internal/domain/model/search_criteria.go (1)
SearchCriteria(7-36)internal/infrastructure/mock/resource_searcher.go (2)
NewMockResourceSearcher(24-136)NewResourceWithDefaults(392-436)
cmd/service/service.go (3)
internal/domain/port/searcher.go (1)
ResourceSearcher(15-24)internal/service/resource_count.go (2)
ResourceCountSearcher(21-27)NewResourceCount(180-185)gen/query_svc/service.go (2)
QueryResourcesCountPayload(110-123)QueryResourcesCountResult(127-135)
internal/domain/model/search_criteria.go (2)
internal/domain/model/count.go (1)
TermsAggregation(13-17)internal/infrastructure/opensearch/models.go (1)
TermsAggregation(31-35)
internal/infrastructure/mock/resource_searcher.go (3)
internal/domain/model/resource.go (1)
Resource(7-18)internal/domain/model/search_criteria.go (2)
CountResult(51-60)SearchCriteria(7-36)internal/domain/model/count.go (2)
AggregationBucket(7-10)TermsAggregation(13-17)
internal/service/resource_count.go (6)
internal/domain/model/search_criteria.go (2)
SearchCriteria(7-36)CountResult(51-60)internal/domain/port/searcher.go (1)
ResourceSearcher(15-24)internal/domain/port/access_control.go (1)
AccessControlChecker(16-25)pkg/constants/http.go (2)
PrincipalContextID(14-14)AnonymousCacheControlHeader(16-16)pkg/errors/client.go (1)
NewValidation(19-26)pkg/constants/access_control.go (2)
AnonymousPrincipal(10-10)AccessCheckSubject(8-8)
internal/infrastructure/opensearch/searcher_test.go (3)
internal/infrastructure/opensearch/models.go (2)
SearchResponse(15-18)CountResponse(20-22)internal/domain/model/search_criteria.go (1)
SearchCriteria(7-36)internal/infrastructure/opensearch/searcher.go (1)
OpenSearchSearcher(34-37)
internal/service/resource_count_test.go (6)
internal/domain/model/search_criteria.go (2)
SearchCriteria(7-36)CountResult(51-60)internal/infrastructure/mock/resource_searcher.go (2)
MockResourceSearcher(16-21)NewMockResourceSearcher(24-136)internal/infrastructure/mock/access_control.go (2)
MockAccessControlChecker(17-32)NewMockAccessControlChecker(149-157)pkg/constants/access_control.go (1)
AnonymousPrincipal(10-10)internal/service/resource_count.go (2)
NewResourceCount(180-185)ResourceCount(31-34)pkg/constants/http.go (1)
PrincipalContextID(14-14)
design/query-svc.go (1)
pkg/httpclient/client.go (1)
Response(31-35)
internal/infrastructure/opensearch/client.go (2)
internal/domain/model/count.go (1)
AggregationResponse(20-22)internal/infrastructure/opensearch/models.go (2)
AggregationResponse(38-40)CountResponse(20-22)
internal/infrastructure/opensearch/searcher.go (3)
internal/infrastructure/opensearch/models.go (4)
CountResponse(20-22)AggregationResponse(38-40)TermsAggregation(31-35)AggregationBucket(25-28)internal/domain/model/count.go (3)
AggregationResponse(20-22)TermsAggregation(13-17)AggregationBucket(7-10)internal/domain/model/search_criteria.go (1)
CountResult(51-60)
🔇 Additional comments (16)
internal/domain/model/resource.go (1)
32-33: New fields look good; verify source includes and response exposure.Confirm SourceIncludes for OpenSearch now include access_check_query and history_check_query, and ensure these internal query strings are not serialized back to external API responses.
internal/domain/model/count.go (1)
1-23: LGTM.Types mirror OpenSearch aggregation shapes and use unsigned counts.
charts/lfx-v2-query-service/templates/ruleset.yaml (1)
48-64: Route addition looks consistent with existing rules.Matches GET-only pattern and auth flow of /query/resources.
internal/infrastructure/opensearch/client.go (1)
116-128: Count path looks correctUsing Indices.Count with the query body and returning a thin DTO is appropriate. No issues spotted.
cmd/service/converters.go (1)
135-141: LGTM: domain→transport mapping for countCasts and field mapping look good.
internal/infrastructure/mock/access_control.go (1)
100-106: LGTM: IsReady override for error injectionAllows deterministic readiness testing.
cmd/service/service_test.go (1)
581-582: LGTM: wiring checkVerifying
resourceCountServicepresence guards against regressions in constructor wiring.internal/infrastructure/opensearch/models.go (1)
20-41: LGTM: count and aggregation DTOsStructs and JSON tags align with typical OpenSearch responses and the rest of the codebase.
cmd/service/service.go (1)
161-169: LGTM: constructor wires ResourceCount serviceConstructor now initializes
resourceCountService; matches tests and keeps deps cohesive.internal/infrastructure/mock/resource_searcher.go (3)
17-20: Mocks for count path look good.The additional fields give fine‑grained control over count responses and readiness in tests.
347-349: LGTM: controllable readiness.Returning the injected error makes readiness testing straightforward.
451-463: LGTM: helpful setters for tests.These keep tests simple and explicit.
internal/infrastructure/opensearch/searcher.go (1)
43-46: Interface expansion is appropriate.Adding Count and AggregationSearch cleanly separates count vs. aggregation concerns and keeps the client mockable.
internal/infrastructure/opensearch/searcher_test.go (2)
38-50: LGTM: mock client methods for count and aggregation.These enable thorough testing of the new paths.
683-813: Count + aggregation tests are solid; consider clarifying bucket key format.Your sample aggregation keys include “@user:test-user”. Service.BuildMessage appends “@user:” again; ensure real OpenSearch buckets do NOT already include the principal.
Would you confirm the actual group-by key (“access_check_query.keyword”) returns values without the principal? If not, we should align service logic accordingly.
internal/service/resource_count.go (1)
36-77: Happy path and anonymous cache-control look good.Deriving publicOnly from the principal and setting Cache-Control for anonymous users matches the API contract.
Minor improvements to the QueryResourcesCount endpoint: - Add proper HTTP error responses (InternalServerError, ServiceUnavailable) - Update OpenAPI specifications with complete error handling - Fix test expectations to match actual service behavior - Update aggregation search client implementation These changes ensure proper error handling and complete API documentation for the QueryResourcesCount endpoint. 🤖 Generated with [Claude Code](https://claude.ai/code) Signed-off-by: Andres Tobon <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
design/query-svc.go (1)
107-109: Align parent validation with query-resourcesAdd the same regex pattern used in query-resources to keep validation consistent.
- dsl.Attribute("parent", dsl.String, "Parent (for navigation; varies by object type)", func() { - dsl.Example("project:123") - }) + dsl.Attribute("parent", dsl.String, "Parent (for navigation; varies by object type)", func() { + dsl.Example("project:123") + dsl.Pattern(`^[a-zA-Z]+:[a-zA-Z0-9_-]+$`) + })internal/infrastructure/opensearch/client.go (1)
120-132: Count: check shard failures and make the internal Count int64Confirmed opensearch-go v4's IndicesCountResp exposes Count (int) and an _shards object with Total, Successful, Skipped, Failed.
- Add a shard-failure guard after the Count call and convert the SDK int to your model's int64:
countResponse, err := c.client.Indices.Count(ctx, &countRequest) if err != nil { return nil, fmt.Errorf("opensearch count failed: %w", err) } if countResponse.Shards.Failed > 0 { return nil, fmt.Errorf("opensearch count had %d shard failures", countResponse.Shards.Failed) } return &CountResponse{ Count: int64(countResponse.Count), }, nil
- Update model (internal/infrastructure/opensearch/models.go):
-type CountResponse struct { Count int `json:"count"` } +type CountResponse struct { Count int64 `json:"count"` }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Jira integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
⛔ Files ignored due to path filters (8)
gen/http/openapi.jsonis excluded by!**/gen/**gen/http/openapi.yamlis excluded by!**/gen/**gen/http/openapi3.jsonis excluded by!**/gen/**gen/http/openapi3.yamlis excluded by!**/gen/**gen/http/query_svc/client/encode_decode.gois excluded by!**/gen/**gen/http/query_svc/client/types.gois excluded by!**/gen/**gen/http/query_svc/server/encode_decode.gois excluded by!**/gen/**gen/http/query_svc/server/types.gois excluded by!**/gen/**
📒 Files selected for processing (3)
design/query-svc.go(1 hunks)internal/infrastructure/opensearch/client.go(2 hunks)internal/service/resource_count_test.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- internal/service/resource_count_test.go
🧰 Additional context used
📓 Path-based instructions (2)
internal/infrastructure/opensearch/**
📄 CodeRabbit inference engine (CLAUDE.md)
Put OpenSearch implementations for resource search under internal/infrastructure/opensearch/
Files:
internal/infrastructure/opensearch/client.go
design/**
📄 CodeRabbit inference engine (CLAUDE.md)
Keep Goa API design specifications in the design/ directory
Files:
design/query-svc.go
🧠 Learnings (1)
📚 Learning: 2025-09-22T23:47:55.493Z
Learnt from: andrest50
PR: linuxfoundation/lfx-v2-query-service#20
File: design/query-svc.go:119-130
Timestamp: 2025-09-22T23:47:55.493Z
Learning: In the LFX V2 Query Service, aggregation functionality exists for internal processing but is not meant to be returned from the API endpoints. The query-resources-count endpoint should only return count, has_more, and cache_control fields, not aggregation data.
Applied to files:
design/query-svc.go
🧬 Code graph analysis (2)
internal/infrastructure/opensearch/client.go (2)
internal/domain/model/count.go (1)
AggregationResponse(20-22)internal/infrastructure/opensearch/models.go (2)
AggregationResponse(38-40)CountResponse(20-22)
design/query-svc.go (1)
pkg/httpclient/client.go (1)
Response(31-35)
🔇 Additional comments (5)
design/query-svc.go (3)
119-130: Result shape looks correct (no aggregation exposed)Returning only count, has_more, and cache_control matches the clarified scope (aggregation used internally only). LGTM.
134-148: HTTP mapping is consistentRoute, params, and Cache-Control header mapping align with existing patterns. No changes needed.
132-132: Remove method-level BadRequest; reuse service-level errorsThis redefines BadRequest locally and diverges from query-resources. Drop it and keep the HTTP error responses referencing the standard error names.
- dsl.Error("BadRequest", dsl.ErrorResult, "Bad request")internal/infrastructure/opensearch/client.go (2)
9-9: LGTM: json import is appropriate for agg unmarshalling.
94-118: Harden AggregationSearch: set Size=0 and guard missing aggregations (prevents unnecessary hits and nil-unmarshal errors).
- Aggregation-only queries should set Size: 0 to avoid fetching hits.
- Unmarshalling when Aggregations is nil/"null" will error; guard explicitly.
Apply:
func (c *httpClient) AggregationSearch(ctx context.Context, index string, query []byte) (*AggregationResponse, error) { - searchRequest := opensearchapi.SearchReq{ - Indices: []string{index}, - Body: bytes.NewReader(query), - } + searchRequest := opensearchapi.SearchReq{ + Indices: []string{index}, + Body: bytes.NewReader(query), + Params: opensearchapi.SearchParams{ + // aggregation-only; don't fetch hits + Size: 0, + }, + } // Perform the search. searchResponse, err := c.client.Search(ctx, &searchRequest) if err != nil { return nil, fmt.Errorf("opensearch search failed: %w", err) } if searchResponse.Errors { return nil, fmt.Errorf("opensearch search returned errors") } + // Guard against missing/empty aggregations + if len(searchResponse.Aggregations) == 0 || string(searchResponse.Aggregations) == "null" { + return nil, fmt.Errorf("missing aggregations in search response") + } + // First, unmarshal the aggregations from raw JSON. var aggregations AggregationResponse if err := json.Unmarshal(searchResponse.Aggregations, &aggregations); err != nil { slog.ErrorContext(ctx, "failed to unmarshal aggregations", "error", err) return nil, fmt.Errorf("unrecoverable aggregation processing error: %w", err) } return &aggregations, nil }
internal/service/resource_count.go
Outdated
| return accessCheckMessage | ||
| } | ||
|
|
||
| func (s *ResourceCount) CheckAccess(ctx context.Context, principal string, result *model.CountResult, accessCheckMessage []byte) (uint64, error) { |
There was a problem hiding this comment.
Should we consider creating a ticket to refactor internal/service/resource_search.go - CheckAccees to perform the same validation based on AccessCheckQueryinstead of building the AccessCheckObject + AccessCheckRelation?
There was a problem hiding this comment.
We will want this, but I don't want to make this change yet because then the old resources would stop working for searches. I need to migrate/reindex the data in opensearch to include the new field then I can make this change. I'll make a ticket for it.
cmd/service/service.go
Outdated
| organizationService service.OrganizationSearcher | ||
| auth port.Authenticator | ||
| resourceService service.ResourceSearcher | ||
| resourceCountService service.ResourceCountSearcher |
There was a problem hiding this comment.
I was thinking of creating this abstraction based on the subject/datasource. Since the resource search count will almost always be an aggregate function over the resources, maybe we should just continue with the ResourceSearcher and add another function to it, something like QueryAggregateOrCountResource.
Note: feel free to go ahead without this - it’s just a matter of perspective.
There was a problem hiding this comment.
I think that's a good idea. I actually originally had it in the same service but then separated it because the BuildMessage and AccessCheck methods needed to be different. But I can just create new methods for them in the same service (e.g. BuildAggregationMessage).
Consolidate resource counting functionality into the existing ResourceSearch service interface for better architectural cohesion: - Remove separate ResourceCount service and merge into ResourceSearch - Move QueryResourcesCount, BuildCountMessage, and CheckCountAccess methods - Consolidate all resource-related operations in a single service - Update service layer to use unified ResourceSearch interface - Maintain all existing functionality and test coverage This simplifies the service architecture while preserving all features and maintaining clean separation of concerns. 🤖 Generated with [Claude Code](https://claude.ai/code) Signed-off-by: Andres Tobon <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (5)
cmd/service/service_test.go (1)
176-298: Strengthen HasMore assertions; verify value instead of non-nil.You only assert non-nil for HasMore. Capture the expected value in the table and assert equality to prevent regressions.
Apply this diff:
func TestQuerySvcsrvc_QueryResourcesCount(t *testing.T) { tests := []struct { name string payload *querysvc.QueryResourcesCountPayload setupMocks func(*mock.MockResourceSearcher, *mock.MockAccessControlChecker) expectedError bool expectedErrorType interface{} expectedCount uint64 + expectedHasMore bool }{ { name: "successful count query", payload: &querysvc.QueryResourcesCountPayload{ Version: "1", Type: stringPtr("project"), }, setupMocks: func(searcher *mock.MockResourceSearcher, accessChecker *mock.MockAccessControlChecker) { searcher.SetQueryResourcesCountResponse(&model.CountResult{ Count: 5, HasMore: false, }) accessChecker.DefaultResult = "allowed" }, expectedError: false, - expectedCount: 5, + expectedCount: 5, + expectedHasMore: false, }, { name: "successful count query with name filter", payload: &querysvc.QueryResourcesCountPayload{ Version: "1", Name: stringPtr("Test"), Type: stringPtr("committee"), }, setupMocks: func(searcher *mock.MockResourceSearcher, accessChecker *mock.MockAccessControlChecker) { searcher.SetQueryResourcesCountResponse(&model.CountResult{ Count: 2, HasMore: false, }) accessChecker.DefaultResult = "allowed" }, expectedError: false, - expectedCount: 2, + expectedCount: 2, + expectedHasMore: false, }, { name: "count query with tags", payload: &querysvc.QueryResourcesCountPayload{ Version: "1", Tags: []string{"active", "governance"}, }, setupMocks: func(searcher *mock.MockResourceSearcher, accessChecker *mock.MockAccessControlChecker) { searcher.SetQueryResourcesCountResponse(&model.CountResult{ Count: 10, HasMore: true, }) accessChecker.DefaultResult = "allowed" }, expectedError: false, - expectedCount: 10, + expectedCount: 10, + expectedHasMore: true, }, { name: "count query with parent filter", payload: &querysvc.QueryResourcesCountPayload{ Version: "1", Parent: stringPtr("project:123"), }, setupMocks: func(searcher *mock.MockResourceSearcher, accessChecker *mock.MockAccessControlChecker) { searcher.SetQueryResourcesCountResponse(&model.CountResult{ Count: 3, HasMore: false, }) accessChecker.DefaultResult = "allowed" }, expectedError: false, - expectedCount: 3, + expectedCount: 3, + expectedHasMore: false, }, { name: "count query with service error", payload: &querysvc.QueryResourcesCountPayload{ Version: "1", Type: stringPtr("invalid"), }, setupMocks: func(searcher *mock.MockResourceSearcher, accessChecker *mock.MockAccessControlChecker) { searcher.SetQueryResourcesCountError(fmt.Errorf("service error")) }, expectedError: true, expectedErrorType: &querysvc.InternalServerError{}, }, } for _, tc := range tests { t.Run(tc.name, func(t *testing.T) { // ... if tc.expectedError { // ... } else { assert.NoError(t, err) assert.NotNil(t, result) assert.Equal(t, tc.expectedCount, result.Count) - // HasMore is returned from the service - assert.NotNil(t, result.HasMore) + if result.HasMore != nil { + assert.Equal(t, tc.expectedHasMore, *result.HasMore) + } else { + t.Fatalf("expected HasMore to be non-nil") + } } }) } }internal/service/resource_search_test.go (2)
914-919: Assert anonymous cache-control header value, not just presence.Verify it equals the expected header to catch regressions.
Apply this diff:
if tc.expectedCacheControl { assertion.NotNil(result.CacheControl) - } else { + assertion.Equal(constants.AnonymousCacheControlHeader, *result.CacheControl) + } else { // For non-anonymous users, CacheControl might be nil // This depends on implementation }
955-960: Optionally verify trailing newline in count access message.Slightly stronger assertion to ensure each key is newline-delimited.
Example:
assertion.True(strings.HasSuffix(messageStr, "\n"))internal/service/resource_search.go (2)
25-27: Interface naming overlap with port.ResourceSearcher; consider renaming to avoid confusion.Local interface ResourceSearcher shadows the imported port.ResourceSearcher identifier, which increases cognitive load.
Suggestion: rename to SearchService or ResourceSearchUsecase.
281-299: Avoid negative-capacity risks and remove unused map in BuildCountMessage.Prealloc using actual bucket length and drop the unused docCountMap.
Apply this diff:
-func (s *ResourceSearch) BuildCountMessage(ctx context.Context, principal string, result *model.CountResult, aggregationCriteria model.SearchCriteria) []byte { - - // Create a map to store the "doc_count" of each aggregation bucket. - docCountMap := make(map[string]uint64, aggregationCriteria.PageSize) - - // estimate the size of each line in the access check message - accessCheckMessage := make([]byte, 0, 80*aggregationCriteria.PageSize) +func (s *ResourceSearch) BuildCountMessage(ctx context.Context, principal string, result *model.CountResult, aggregationCriteria model.SearchCriteria) []byte { + // Estimate buffer by number of buckets (~80 bytes per line) + bucketCount := len(result.Aggregation.Buckets) + accessCheckMessage := make([]byte, 0, 80*bucketCount) for _, bucket := range result.Aggregation.Buckets { - docCountMap[bucket.Key] = bucket.DocCount accessCheckMessage = append(accessCheckMessage, bucket.Key...) accessCheckMessage = append(accessCheckMessage, []byte("@user:")...) accessCheckMessage = append(accessCheckMessage, []byte(principal)...) accessCheckMessage = append(accessCheckMessage, '\n') } return accessCheckMessage }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Jira integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (4)
cmd/service/service.go(1 hunks)cmd/service/service_test.go(2 hunks)internal/service/resource_search.go(2 hunks)internal/service/resource_search_test.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- cmd/service/service.go
🧰 Additional context used
📓 Path-based instructions (3)
internal/service/**
📄 CodeRabbit inference engine (CLAUDE.md)
Keep service-layer orchestration logic in internal/service/
Files:
internal/service/resource_search.gointernal/service/resource_search_test.go
internal/service/resource_search.go
📄 CodeRabbit inference engine (CLAUDE.md)
Implement use case orchestration for resource search in internal/service/resource_search.go
Files:
internal/service/resource_search.go
**/*_test.go
📄 CodeRabbit inference engine (CLAUDE.md)
Name Go test files with the *_test.go suffix and keep them alongside implementation files
Files:
internal/service/resource_search_test.gocmd/service/service_test.go
🧠 Learnings (3)
📓 Common learnings
Learnt from: andrest50
PR: linuxfoundation/lfx-v2-query-service#20
File: design/query-svc.go:119-130
Timestamp: 2025-09-22T23:47:55.493Z
Learning: In the LFX V2 Query Service, aggregation functionality exists for internal processing but is not meant to be returned from the API endpoints. The query-resources-count endpoint should only return count, has_more, and cache_control fields, not aggregation data.
📚 Learning: 2025-09-10T17:40:14.123Z
Learnt from: CR
PR: linuxfoundation/lfx-v2-query-service#0
File: CLAUDE.md:0-0
Timestamp: 2025-09-10T17:40:14.123Z
Learning: Applies to internal/service/resource_search.go : Implement use case orchestration for resource search in internal/service/resource_search.go
Applied to files:
internal/service/resource_search.gointernal/service/resource_search_test.go
📚 Learning: 2025-09-10T17:40:14.123Z
Learnt from: CR
PR: linuxfoundation/lfx-v2-query-service#0
File: CLAUDE.md:0-0
Timestamp: 2025-09-10T17:40:14.123Z
Learning: Applies to cmd/query_svc/query_svc.go : Keep the service implementation that connects Goa to domain logic in cmd/query_svc/query_svc.go
Applied to files:
cmd/service/service_test.go
🧬 Code graph analysis (3)
internal/service/resource_search.go (4)
internal/domain/model/search_criteria.go (2)
SearchCriteria(7-36)CountResult(51-60)pkg/constants/http.go (2)
PrincipalContextID(14-14)AnonymousCacheControlHeader(16-16)pkg/errors/client.go (1)
NewValidation(19-26)pkg/constants/access_control.go (2)
AnonymousPrincipal(10-10)AccessCheckSubject(8-8)
internal/service/resource_search_test.go (7)
internal/domain/model/search_criteria.go (2)
SearchCriteria(7-36)CountResult(51-60)internal/infrastructure/mock/resource_searcher.go (2)
MockResourceSearcher(16-21)NewMockResourceSearcher(24-136)internal/infrastructure/mock/access_control.go (2)
MockAccessControlChecker(17-32)NewMockAccessControlChecker(149-157)pkg/constants/access_control.go (1)
AnonymousPrincipal(10-10)internal/infrastructure/opensearch/models.go (2)
TermsAggregation(31-35)AggregationBucket(25-28)internal/service/resource_search.go (2)
NewResourceSearch(353-358)ResourceSearch(34-37)pkg/constants/http.go (1)
PrincipalContextID(14-14)
cmd/service/service_test.go (7)
gen/query_svc/service.go (1)
QueryResourcesCountPayload(110-123)internal/infrastructure/mock/resource_searcher.go (2)
MockResourceSearcher(16-21)NewMockResourceSearcher(24-136)internal/infrastructure/mock/access_control.go (2)
MockAccessControlChecker(17-32)NewMockAccessControlChecker(149-157)internal/domain/model/search_criteria.go (1)
CountResult(51-60)internal/infrastructure/mock/organization_searcher.go (1)
NewMockOrganizationSearcher(23-84)internal/infrastructure/mock/auth.go (1)
NewMockAuthService(35-37)pkg/constants/http.go (1)
PrincipalContextID(14-14)
🔇 Additional comments (2)
internal/service/resource_search.go (2)
259-271: Confirm counting semantics (public + authorized private).Service adds authorized private count to the base Count. Ensure upstream returns a public-only Count for non-anonymous principals; otherwise this double-counts.
If upstream returns total (public+private), change this block to set Count to privateCount rather than add, or have the searcher return public-only base.
273-276: LGTM: HasMore computed from SumOtherDocCount.Correctly surfaces bucket overflow via HasMore.
| name: "successful count with authenticated user - public only", | ||
| countCriteria: model.SearchCriteria{ | ||
| ResourceType: stringPtr("project"), | ||
| PageSize: -1, | ||
| PublicOnly: true, | ||
| }, | ||
| aggregationCriteria: model.SearchCriteria{ | ||
| GroupBy: "access_check_query.keyword", | ||
| PageSize: 0, | ||
| PrivateOnly: true, | ||
| }, | ||
| principal: "user:test-user", | ||
| setupMocks: func(resourceSearcher *mock.MockResourceSearcher, accessChecker *mock.MockAccessControlChecker) { | ||
| resourceSearcher.SetQueryResourcesCountResponse(&model.CountResult{ | ||
| Count: 2, | ||
| Aggregation: model.TermsAggregation{ | ||
| Buckets: []model.AggregationBucket{ | ||
| {Key: "project:123#viewer", DocCount: 1}, | ||
| {Key: "project:456#contributor", DocCount: 2}, | ||
| }, | ||
| }, | ||
| HasMore: false, | ||
| }) | ||
| accessChecker.SetCheckAccessResponse(map[string]string{ | ||
| "project:123#viewer@user:test-user": "true", | ||
| "project:456#contributor@user:test-user": "false", | ||
| }) | ||
| }, | ||
| expectedError: false, | ||
| expectedCount: 2, | ||
| expectedCacheControl: false, | ||
| }, |
There was a problem hiding this comment.
🧩 Analysis chain
Fix expected counts: final count should be public + authorized private.
Service adds authorized private bucket counts to the public base. Update expectedCount accordingly to reflect actual semantics.
Apply this diff:
{
name: "successful count with authenticated user - public only",
countCriteria: model.SearchCriteria{
ResourceType: stringPtr("project"),
PageSize: -1,
PublicOnly: true,
},
aggregationCriteria: model.SearchCriteria{
GroupBy: "access_check_query.keyword",
PageSize: 0,
PrivateOnly: true,
},
principal: "user:test-user",
setupMocks: func(resourceSearcher *mock.MockResourceSearcher, accessChecker *mock.MockAccessControlChecker) {
resourceSearcher.SetQueryResourcesCountResponse(&model.CountResult{
- Count: 2,
+ Count: 2,
Aggregation: model.TermsAggregation{
Buckets: []model.AggregationBucket{
{Key: "project:123#viewer", DocCount: 1},
{Key: "project:456#contributor", DocCount: 2},
},
},
HasMore: false,
})
accessChecker.SetCheckAccessResponse(map[string]string{
"project:123#viewer@user:test-user": "true",
"project:456#contributor@user:test-user": "false",
})
},
expectedError: false,
- expectedCount: 2,
+ expectedCount: 3, // 2 public + 1 authorized private
expectedCacheControl: false,
},
{
name: "successful count with authenticated user - with private access",
countCriteria: model.SearchCriteria{
PageSize: -1,
PublicOnly: true,
},
aggregationCriteria: model.SearchCriteria{
GroupBy: "access_check_query.keyword",
PageSize: 0,
PrivateOnly: true,
},
principal: "user:admin",
setupMocks: func(resourceSearcher *mock.MockResourceSearcher, accessChecker *mock.MockAccessControlChecker) {
resourceSearcher.SetQueryResourcesCountResponse(&model.CountResult{
- Count: 5,
+ Count: 5,
Aggregation: model.TermsAggregation{
Buckets: []model.AggregationBucket{
{Key: "committee:789#member", DocCount: 3},
{Key: "project:101#viewer", DocCount: 2},
},
},
HasMore: false,
})
accessChecker.SetCheckAccessResponse(map[string]string{
"committee:789#member@user:admin": "true",
"project:101#viewer@user:admin": "true",
})
},
expectedError: false,
- expectedCount: 5,
+ expectedCount: 10, // 5 public + (3+2) authorized private
expectedCacheControl: false,
},Please confirm this aligns with the intended contract of QueryResourcesCount:
- Count from the searcher is public-only.
- Aggregation contains private buckets to be access-checked and added.
If not, we should adjust service logic instead of tests.
Also applies to: 816-846
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "---- Searching for relevant symbols ----"
rg -n --hidden "QueryResourcesCount|SetQueryResourcesCountResponse|CountResult|access_check_query" || true
echo
echo "---- Show test file snippet around the reported lines ----"
sed -n '760,860p' internal/service/resource_search_test.go || true
echo
echo "---- Show mock/resource searcher definitions ----"
rg -n --hidden "type .*ResourceSearcher|SetQueryResourcesCountResponse" || true
rg -n --hidden "MockResourceSearcher" || true
echo
echo "---- Search service logic that calls QueryResourcesCount / handles aggregation ----"
rg -n --hidden "QueryResourcesCount|QueryResources|access_check_query|PrivateOnly|PublicOnly" || trueLength of output: 150505
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "---- Show QueryResourcesCount implementation ----"
sed -n '1,360p' internal/service/resource_search.go || true
echo
echo "---- Show BuildCountMessage and CheckCountAccess functions ----"
sed -n '240,420p' internal/service/resource_search.go || true
echo
echo "---- Show MockAccessControlChecker methods used in tests ----"
sed -n '1,260p' internal/infrastructure/mock/access_control.go || trueLength of output: 24025
Fix expected counts: final count should be public + authorized private.
Confirmed: QueryResourcesCount uses the searcher's Count as the public base and adds authorized private aggregation bucket counts (via CheckCountAccess); update the tests' expectedCount accordingly.
{
name: "successful count with authenticated user - public only",
countCriteria: model.SearchCriteria{
ResourceType: stringPtr("project"),
PageSize: -1,
PublicOnly: true,
},
aggregationCriteria: model.SearchCriteria{
GroupBy: "access_check_query.keyword",
PageSize: 0,
PrivateOnly: true,
},
principal: "user:test-user",
setupMocks: func(resourceSearcher *mock.MockResourceSearcher, accessChecker *mock.MockAccessControlChecker) {
resourceSearcher.SetQueryResourcesCountResponse(&model.CountResult{
- Count: 2,
+ Count: 2,
Aggregation: model.TermsAggregation{
Buckets: []model.AggregationBucket{
{Key: "project:123#viewer", DocCount: 1},
{Key: "project:456#contributor", DocCount: 2},
},
},
HasMore: false,
})
accessChecker.SetCheckAccessResponse(map[string]string{
"project:123#viewer@user:test-user": "true",
"project:456#contributor@user:test-user": "false",
})
},
expectedError: false,
- expectedCount: 2,
+ expectedCount: 3, // 2 public + 1 authorized private
expectedCacheControl: false,
},
{
name: "successful count with authenticated user - with private access",
countCriteria: model.SearchCriteria{
PageSize: -1,
PublicOnly: true,
},
aggregationCriteria: model.SearchCriteria{
GroupBy: "access_check_query.keyword",
PageSize: 0,
PrivateOnly: true,
},
principal: "user:admin",
setupMocks: func(resourceSearcher *mock.MockResourceSearcher, accessChecker *mock.MockAccessControlChecker) {
resourceSearcher.SetQueryResourcesCountResponse(&model.CountResult{
- Count: 5,
+ Count: 5,
Aggregation: model.TermsAggregation{
Buckets: []model.AggregationBucket{
{Key: "committee:789#member", DocCount: 3},
{Key: "project:101#viewer", DocCount: 2},
},
},
HasMore: false,
})
accessChecker.SetCheckAccessResponse(map[string]string{
"committee:789#member@user:admin": "true",
"project:101#viewer@user:admin": "true",
})
},
expectedError: false,
- expectedCount: 5,
+ expectedCount: 10, // 5 public + (3+2) authorized private
expectedCacheControl: false,
},Also applies to: 816-846
🤖 Prompt for AI Agents
In internal/service/resource_search_test.go around lines 783-814 (and also apply
same fix to 816-846), the test's expectedCount is wrong: QueryResourcesCount
uses the searcher's Count as the public base and then adds counts from
aggregation buckets that CheckCountAccess returns authorized for. Update the
test expectations so expectedCount = Count (2) + sum of DocCount for buckets
where accessChecker returned "true" (here +1) -> expectedCount should be 3; make
the analogous adjustment for the second test block at 816-846 to reflect its
aggregation + access responses.
| if err != nil { | ||
| slog.ErrorContext(ctx, "search operation failed while executing query resources", | ||
| "error", err, | ||
| ) | ||
| return nil, fmt.Errorf("search operation failed: %w", err) | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick
Fix log message to reflect count path.
The error log mentions “query resources” instead of “query resources count”.
Apply this diff:
- slog.ErrorContext(ctx, "search operation failed while executing query resources",
+ slog.ErrorContext(ctx, "search operation failed while executing query resources count",
"error", err,
)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if err != nil { | |
| slog.ErrorContext(ctx, "search operation failed while executing query resources", | |
| "error", err, | |
| ) | |
| return nil, fmt.Errorf("search operation failed: %w", err) | |
| } | |
| if err != nil { | |
| slog.ErrorContext(ctx, "search operation failed while executing query resources count", | |
| "error", err, | |
| ) | |
| return nil, fmt.Errorf("search operation failed: %w", err) | |
| } |
🤖 Prompt for AI Agents
In internal/service/resource_search.go around lines 236 to 241, the
slog.ErrorContext call logs "search operation failed while executing query
resources" but this should reference the count path; change the log message
string to "search operation failed while executing query resources count" (or
similarly include "count") so the log accurately reflects the failed operation,
leaving the error handling/return as-is.
Summary
Implements a new QueryResourcesCount API endpoint that provides efficient resource counting with access control support.
Changes
GET /query/resources/countFeatures
JIRA Ticket
Resolves: https://linuxfoundation.atlassian.net/browse/LFXV2-456
Test Plan
Manual Testing
Note: The counts are based on the amount of data in one's local environment and dependent on the authenticated user.
Querying projects count:
Request:
Response:
Querying committee members count:
Request:
Unauthenticated response:
Authenticated response (with user who has viewer access on the committee):
It can be seen that the count is dependent on the user's permissions, so the count will only show resources that the user has access to.
Cross-type counts are supported as well, meaning one can get for example the count of all resources for a given project (e.g. meetings, committees):
Request:
Response:
🤖 Generated with Claude Code