[LFX-V2-199] Query Service: org search endpoint implementation#15
Conversation
- Introduced a new method `query-orgs` to locate organizations by name or domain. - Updated OpenAPI specifications to include the new endpoint and its parameters. - Added necessary request and response types for the query-orgs functionality. - Enhanced CLI to support querying organizations. - Updated server and client code to handle the new endpoint. Jira Ticket: https://linuxfoundation.atlassian.net/browse/LFXV2-199 Generated with [Cursor](https://cursor.com/) Signed-off-by: Mauricio Zanetti Salomao <mauriciozanetti86@gmail.com>
…bilities - Moved organization search logic into a dedicated service layer. - Introduced new interfaces for organization searching and resource searching. - Implemented organization searcher and resource searcher with mock implementations for testing. - Updated main application logic to utilize the new service structure. - Enhanced error handling and logging for search operations. Jira Ticket: https://linuxfoundation.atlassian.net/browse/LFXV2-199 Generated with [Cursor](https://cursor.com/) Signed-off-by: Mauricio Zanetti Salomao <mauriciozanetti86@gmail.com>
…type - Introduced NotFound error handling in the query service to return appropriate responses when resources are not found. - Updated OpenAPI specifications to include NotFound error responses for relevant endpoints. - Enhanced error wrapping in the service layer to improve clarity and consistency in error reporting. - Updated client and server code to handle new NotFound error type and responses. Jira Ticket: https://linuxfoundation.atlassian.net/browse/LFXV2-199 Generated with [Cursor](https://cursor.com/) Signed-off-by: Mauricio Zanetti Salomao <mauriciozanetti86@gmail.com>
|
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. WalkthroughRenames Use Case Layer to Service Layer in docs. Adds /query/orgs API, domain models, ports, and implementations. Integrates Clearbit client with config, HTTP client with retries, and mock searcher. Refactors cmd wiring to use service providers, adds JWT setup, NotFound error, and readiness handling. Updates Helm chart version. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor User
participant API as Query Service API
participant Auth as JWT Auth
participant AC as Access Control
participant OrgSvc as Organization Search Service
participant Impl as OrganizationSearcher (Clearbit/Mock)
note over API,OrgSvc: New endpoint: GET /query/orgs
User->>API: HTTP GET /query/orgs (Authorization, name/domain)
API->>Auth: Validate JWT
Auth-->>API: OK / Error
alt Auth error
API-->>User: 400/401 BadRequest
else Auth OK
API->>AC: Check access
AC-->>API: Allowed / Denied
alt Access denied
API-->>User: 403/400 Error
else Allowed
API->>OrgSvc: QueryOrganizations(criteria)
OrgSvc->>Impl: QueryOrganizations(criteria)
alt Found
Impl-->>OrgSvc: Organization
OrgSvc-->>API: Organization
API-->>User: 200 Organization
else Not found
Impl-->>OrgSvc: NotFound error
OrgSvc-->>API: NotFound error
API-->>User: 404 NotFound
else Service error
Impl-->>OrgSvc: 5xx/Unexpected
OrgSvc-->>API: Error
API-->>User: 503/500
end
end
end
sequenceDiagram
autonumber
actor Proc as Process
participant Main as cmd/main
participant Prov as service/providers
participant Svc as cmd/service.NewQuerySvc
participant Res as internal/service.ResourceSearch
participant Org as internal/service.OrganizationSearch
note over Main,Org: Startup wiring moved to service providers
Proc->>Main: Start
Main->>Prov: SearcherImpl(ctx)
Prov-->>Main: port.ResourceSearcher
Main->>Prov: AccessControlCheckerImpl(ctx)
Prov-->>Main: port.AccessControlChecker
Main->>Prov: OrganizationSearcherImpl(ctx)
Prov-->>Main: port.OrganizationSearcher
Main->>Svc: NewQuerySvc(res, ac, org)
Svc->>Res: NewResourceSearch(res, ac)
Svc->>Org: NewOrganizationSearch(org)
Svc-->>Main: querysvc.Service
Main-->>Proc: Serve
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Possibly related PRs
Pre-merge checks (3 passed, 2 warnings)❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
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. ✨ Finishing Touches
🧪 Generate unit tests
Comment |
There was a problem hiding this comment.
Pull Request Overview
This pull request implements a new organization search endpoint (/query/orgs) with Clearbit API integration while consolidating service initialization code into a new cmd/service package. The implementation follows clean architecture principles and includes comprehensive test coverage for both the service layer and mock implementations.
- Adds organization search functionality with support for name and domain-based queries
- Introduces Clearbit API integration as the primary data source for organization information
- Refactors service initialization from
cmd/main.gointo a structuredcmd/servicepackage - Adds generic HTTP client package with retry logic and error handling
- Updates package structure from
usecasetoservicefor better naming consistency
Reviewed Changes
Copilot reviewed 45 out of 46 changed files in this pull request and generated 12 comments.
Show a summary per file
| File | Description |
|---|---|
pkg/httpclient/ |
New generic HTTP client with retry logic and configurable timeouts |
pkg/errors/client.go |
Adds NotFound error type for organization search operations |
internal/service/organization_search.go |
New service layer for organization search business logic |
internal/infrastructure/clearbit/ |
Clearbit API client implementation with domain model conversion |
internal/infrastructure/mock/organization_searcher.go |
Mock implementation for testing with fictional company data |
gen/ |
Generated code for new query-orgs endpoint with validation and error handling |
cmd/service/ |
Consolidated service initialization and dependency injection |
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: 20
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (7)
README.md (4)
22-24: Directory structure out-of-date with refactor (missing cmd/service).Docs still list only cmd/query_svc; the PR adds cmd/service for wiring/providers.
Apply:
├── cmd/ # Services (main packages) -│ └── query_svc/ # Query service implementation +│ ├── query_svc/ # Goa-generated service entrypoints +│ └── service/ # Service wiring, providers, JWT setup
65-69: Dependency Injection location no longer accurate.DI moved into cmd/service/providers.go (with main delegating to it).
Apply:
-Dependency injection is performed in `cmd/main.go`, where the concrete implementations for resource search and access control are selected based on configuration and then injected into the service constructor. +Dependency injection is performed in `cmd/service/providers.go` (wiring search, access control, and organization search). `cmd/main.go` initializes logging/flags and calls the service wiring.
149-169: Document the new organization search endpoint.Add usage for /query/orgs to match the implemented API.
Apply:
### API Usage The service exposes a RESTful API through the Goa framework:GET /query/resources?name=committee&type=committee&v=1
+GET /query/orgs?v=1&domain=linuxfoundation.org
+GET /query/orgs?v=1&name=Google
+GET /query/orgs?v=1&name=Google&domain=google.com**Parameters:** - - `name`: Resource name or alias (supports typeahead search) + - `name`: Resource name or alias (supports typeahead search). For orgs: organization name. - `type`: Resource type to filter by - `parent`: Parent resource for hierarchical queries - `tags`: Array of tags to filter by - `sort`: Sort order (name_asc, name_desc, updated_asc, updated_desc) - `page_token`: Pagination token - `v`: API version (required) + - `domain`: For orgs, registrable domain (e.g., example.com). Validated by regex.
121-142: Add missing JWT, Clearbit, and ORG_SEARCH_SOURCE env var docsUnder “Available Environment Variables,” add:
JWT Configuration:
JWKS_URLAUDIENCEJWT_SIGNATURE_ALGORITHMJWT_AUTH_DISABLED_MOCK_LOCAL_PRINCIPALClearbit Configuration:
CLEARBIT_CREDENTIALCLEARBIT_BASE_URLCLEARBIT_MAX_RETRIESCLEARBIT_RETRY_DELAYCLEARBIT_TIMEOUTSearch Configuration:
ORG_SEARCH_SOURCE: Choose between “mock” or “clearbit” (default: “clearbit”)Remove any mention of
JWT_ISSUER—no such variable is referenced in code.internal/service/resource_search.go (1)
93-97: Avoid logging sensitive access-check payloads (PII leakage).The logs include the full access-check message with the principal ID. Prefer logging sizes/counts, not payload contents.
Apply this diff to redact payloads:
@@ - slog.ErrorContext(ctx, "access control check failed", - "error", errCheckAccess, - "message", string(messageCheckAccess), - ) + slog.ErrorContext(ctx, "access control check failed", + "error", errCheckAccess, + "message_len", len(messageCheckAccess), + ) @@ - slog.DebugContext(ctx, "performing access control checks", - "message", string(accessCheckMessage), - ) + slog.DebugContext(ctx, "performing access control checks", + "payload_bytes", len(accessCheckMessage), + )Also applies to: 173-176
cmd/service/error.go (1)
6-12: Don’t shadow stdlib errors; handle context cancellations/timeouts explicitly.Aliasing project errors avoids confusion and lets you use errors.Is for context errors.
Apply this diff:
@@ -import ( - "context" - "log/slog" - - querysvc "github.com/linuxfoundation/lfx-v2-query-service/gen/query_svc" - "github.com/linuxfoundation/lfx-v2-query-service/pkg/errors" -) +import ( + "context" + stderrors "errors" + "log/slog" + + querysvc "github.com/linuxfoundation/lfx-v2-query-service/gen/query_svc" + qerrors "github.com/linuxfoundation/lfx-v2-query-service/pkg/errors" +) @@ func wrapError(ctx context.Context, err error) error { - f := func(err error) error { + // Treat canceled/timeout requests as a 503 instead of 500. + if stderrors.Is(err, context.DeadlineExceeded) || stderrors.Is(err, context.Canceled) { + slog.ErrorContext(ctx, "request canceled/timeout", "error", err) + return &querysvc.ServiceUnavailableError{Message: err.Error()} + } + + f := func(err error) error { switch e := err.(type) { - case errors.Validation: + case qerrors.Validation: return &querysvc.BadRequestError{ Message: e.Error(), } - case errors.NotFound: + case qerrors.NotFound: return &querysvc.NotFoundError{ Message: e.Error(), } - case errors.ServiceUnavailable: + case qerrors.ServiceUnavailable: return &querysvc.ServiceUnavailableError{ Message: e.Error(), } default: return &querysvc.InternalServerError{ Message: e.Error(), } } }Also applies to: 16-35
cmd/main.go (1)
73-74: Only log payloads when debug is enabled (avoid PII leakage).
debug.LogPayloads() should be gated by -d; unguarded it may leak JWTs/payloads and bloat logs.- querySvcEndpoints.Use(debug.LogPayloads()) + if *dbgF { + querySvcEndpoints.Use(debug.LogPayloads()) + }
🧹 Nitpick comments (35)
internal/domain/model/search_criteria.go (1)
44-50: Clarify domain vs URL semantics in the domain model.The comment says “domain or website URL,” but infra (Clearbit) typically expects a registrable domain. Recommend documenting the invariant that Domain is normalized (lowercased, scheme/paths stripped) before constructing this model to keep domain-layer semantics clean.
Apply this doc tweak:
type OrganizationSearchCriteria struct { // Organization name Name *string - // Organization domain or website URL - Domain *string + // Organization domain (e.g., "example.com"). + // Note: Upstream layers may accept URLs; they must normalize to a registrable + // domain (lowercased, no scheme/path) before populating this field. + Domain *string }cmd/service/jwt.go (3)
84-95: Support multiple audiences via comma-separated env var.Many issuers emit multiple aud values. Allow “AUDIENCE=a1,a2”.
Apply:
- audience := os.Getenv("AUDIENCE") - if audience == "" { - audience = defaultAudience - } + audienceEnv := os.Getenv("AUDIENCE") + if audienceEnv == "" { + audienceEnv = defaultAudience + } + var audiences []string + for _, a := range strings.Split(audienceEnv, ",") { + if s := strings.TrimSpace(a); s != "" { + audiences = append(audiences, s) + } + } jwtValidator, err = validator.New( provider.KeyFunc, signatureAlgorithm, - issuer.String(), - []string{audience}, + issuer.String(), + audiences, validator.WithCustomClaims(customClaims), validator.WithAllowedClockSkew(5*time.Second), )
68-77: Allow overriding the expected issuer via env.Hardcoding “heimdall” limits deploy flexibility. Add JWT_ISSUER override (still use JWKS_URL for keys).
Apply:
- var issuer *url.URL - issuer, err = url.Parse(defaultIssuer) + iss := os.Getenv("JWT_ISSUER") + if iss == "" { + iss = defaultIssuer + } + var issuer *url.URL + issuer, err = url.Parse(iss) if err != nil { // This shouldn't happen; a bare hostname is a valid URL. slog.ErrorContext(ctx, "failed to parse issuer URL", - "issuer", defaultIssuer, + "issuer", iss, "error", err, ) log.Fatalf("failed to parse issuer URL: %v", err) }
149-156: Avoid shadowing package-level customClaims var.Local variable name collides with the package-level factory, reducing readability.
Apply:
- customClaims, ok := claims.CustomClaims.(*HeimdallClaims) + hc, ok := claims.CustomClaims.(*HeimdallClaims) if !ok { // This should never happen. return "", errors.New("failed to get custom authorization claims") } - return customClaims.Principal, nil + return hc.Principal, nilCLAUDE.md (2)
67-70: Doc consistency: reflect new DI location.
Also update the “Dependency Injection” bullet to point to cmd/service/providers.go instead of cmd/main.go.
96-96: Rename step to “Service orchestration” and fix path in step 2.
The flow still references cmd/query_svc. Suggest:-3. Use case orchestration (`internal/service/resource_search.go`) +3. Service orchestration (`internal/service/resource_search.go`)Additionally, adjust step 2 (not shown in the hunk) to:
2. Service layer (`cmd/service/service.go`)And in “Configuration”, add Clearbit vars for the new endpoint:
- CLEARBIT_API_KEY: API key for Clearbit Company API (required for org search) - CLEARBIT_BASE_URL (optional, default: https://company.clearbit.com)pkg/errors/client.go (1)
28-46: Add helper for error matching (optional).
Provide an IsNotFound helper to standardize checks across layers.type NotFound struct { base } // Error returns the error message for NotFound. func (v NotFound) Error() string { return v.error() } // NewNotFound creates a new NotFound error with the provided message. func NewNotFound(message string, err ...error) NotFound { return NotFound{ base: base{ message: message, err: errors.Join(err...), }, } } + +// IsNotFound reports whether err is a NotFound error in the chain. +func IsNotFound(err error) bool { + var nf NotFound + return errors.As(err, &nf) +}pkg/httpclient/config.go (1)
25-33: Config defaults LGTM; consider reuse to avoid duplication.
Clearbit’s config mirrors these fields; consider embedding httpclient.Config into internal/infrastructure/clearbit.Config to keep defaults/behavior in one place.design/types.go (1)
83-101: Tighten example semantics for industry vs sector.
Align examples with Clearbit taxonomy (sector broader than industry).- dsl.Attribute("industry", dsl.String, "Organization industry classification", func() { - dsl.Example("Non-Profit") - }) - dsl.Attribute("sector", dsl.String, "Business sector classification", func() { - dsl.Example("Technology") - }) + dsl.Attribute("industry", dsl.String, "Organization industry classification", func() { + dsl.Example("Software") + }) + dsl.Attribute("sector", dsl.String, "Business sector classification", func() { + dsl.Example("Technology") + })internal/infrastructure/clearbit/models.go (2)
12-28: Make optional fields omitempty.
Prevents emitting empty strings for absent values.- LegalName string `json:"legalName"` + LegalName string `json:"legalName,omitempty"` @@ - Description string `json:"description"` + Description string `json:"description,omitempty"`
49-58: Add constants for Clearbit error types (optional).
Useful for mapping to NotFound/rate-limit without magic strings.type ClearbitErrorResponse struct { Error *ClearbitError `json:"error,omitempty"` } // ClearbitError represents error details type ClearbitError struct { Type string `json:"type"` Message string `json:"message"` } + +// Known Clearbit error types. +const ( + ErrTypeUnknownRecord = "unknown_record" + ErrTypeRateLimit = "rate_limit" +)internal/infrastructure/clearbit/config.go (3)
33-41: Reuse defaultBaseURL in DefaultConfig.
Avoids divergence if the constant changes.func DefaultConfig() Config { return Config{ - BaseURL: "https://company.clearbit.com", + BaseURL: defaultBaseURL, Timeout: 30 * time.Second, MaxRetries: 3, RetryDelay: 1 * time.Second, } }
55-66: Align NewConfig defaults with DefaultConfig.
Timeout default is 10s here vs 30s in DefaultConfig; pick one (recommend 30s).- if timeout == "" { - timeout = "10s" - } + if timeout == "" { + timeout = "30s" + } @@ - if maxRetries <= 0 { + if maxRetries <= 0 { maxRetries = 3 }
15-31: Consider embedding httpclient.Config.
To reduce duplication and enable RetryBackoff/jitter from pkg/httpclient.internal/domain/port/searcher.go (1)
23-32: Interface addition is clean; avoid cross-package name ambiguity.Service also defines an OrganizationSearcher interface; consider renaming the service-layer interface to OrganizationSearchService to reduce confusion in call sites and tests.
internal/service/organization_search_test.go (1)
405-427: Canceled context test: clarify expected behavior.Test asserts no error on canceled context due to mock behavior. Consider marking with a note or TODO that real implementations should respect context cancellation and likely return an error.
pkg/httpclient/client_test.go (3)
39-49: Assert header passthrough in GET success test.
You set a custom header but don’t verify it reached the server.Apply:
server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { if r.Method != "GET" { t.Errorf("Expected GET request, got %s", r.Method) } + if r.Header.Get("Custom-Header") != "custom-value" { + t.Errorf("Expected Custom-Header 'custom-value', got '%s'", r.Header.Get("Custom-Header")) + } w.WriteHeader(http.StatusOK)Also applies to: 62-69
6-14: Import errors for proper unwrapping.
Needed by the 404 test fix below.import ( + "errors" "context" "io"
179-189: Also assert Content-Type header in POST test.
Since the client sets it, verify it on the server.server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { if r.Method != "POST" { t.Errorf("Expected POST request, got %s", r.Method) } + if ct := r.Header.Get("Content-Type"); ct != "application/json" { + t.Errorf("Expected Content-Type 'application/json', got '%s'", ct) + } + body, _ := io.ReadAll(r.Body)internal/service/organization_search.go (2)
14-23: Avoid name collision with domain port interface.
service.OrganizationSearcher duplicates the name of port.OrganizationSearcher and can confuse readers. Consider renaming to OrganizationService or OrganizationSearchService.
31-47: Tighten error log message.
Current message is verbose; simplify for consistency.- slog.ErrorContext(ctx, "organization search operation failed while executing query organizations", + slog.ErrorContext(ctx, "organization search failed", "error", err, )cmd/service/providers.go (2)
8-9: Avoid log.Fatalf in providers; standardize on slog and return errors to caller for testability.Using log.Fatalf() here makes these providers hard to unit test and abruptly terminates the process from inside a wiring package. Prefer returning (T, error) and handling process exit in main, or at least standardize logging to slog and exit explicitly.
Example (apply pattern to each fatal site shown in the ranges):
-import "log" +// import "log" // remove - resourceSearcher, err = opensearch.NewSearcher(ctx, opensearchConfig) - if err != nil { - log.Fatalf("failed to initialize OpenSearch searcher: %v", err) - } + resourceSearcher, err = opensearch.NewSearcher(ctx, opensearchConfig) + if err != nil { + slog.ErrorContext(ctx, "failed to initialize OpenSearch searcher", "error", err) + os.Exit(1) + }If you’re open to changing signatures, I can provide a follow-up diff to return errors instead of exiting here.
Also applies to: 60-67, 96-99, 105-108, 114-117, 134-141, 189-190, 198-201, 203-205
92-118: Be lenient on malformed env values; fall back to defaults instead of exiting.For NATS durations/ints, prefer defaulting with a warning rather than fatal-exiting on bad input.
- natsTimeoutDuration, err := time.ParseDuration(natsTimeout) - if err != nil { - log.Fatalf("invalid NATS timeout duration: %v", err) - } + natsTimeoutDuration, err := time.ParseDuration(natsTimeout) + if err != nil { + slog.WarnContext(ctx, "invalid NATS timeout duration; using default 10s", "value", natsTimeout, "error", err) + natsTimeoutDuration = 10 * time.Second + } ... - natsMaxReconnectInt, err := strconv.Atoi(natsMaxReconnect) - if err != nil { - log.Fatalf("invalid NATS max reconnect value %s: %v", natsMaxReconnect, err) - } + natsMaxReconnectInt, err := strconv.Atoi(natsMaxReconnect) + if err != nil { + slog.WarnContext(ctx, "invalid NATS max reconnect; using default 3", "value", natsMaxReconnect, "error", err) + natsMaxReconnectInt = 3 + } ... - natsReconnectWaitDuration, err := time.ParseDuration(natsReconnectWait) - if err != nil { - log.Fatalf("invalid NATS reconnect wait duration %s : %v", natsReconnectWait, err) - } + natsReconnectWaitDuration, err := time.ParseDuration(natsReconnectWait) + if err != nil { + slog.WarnContext(ctx, "invalid NATS reconnect wait; using default 2s", "value", natsReconnectWait, "error", err) + natsReconnectWaitDuration = 2 * time.Second + }cmd/service/converters.go (1)
29-42: Provide an explicit default sort when p.Sort is empty or unrecognized.Currently SortBy/Order can remain empty, deferring defaulting downstream. Set a clear default to keep behavior consistent.
switch p.Sort { case "name_asc": criteria.SortBy = "sort_name" criteria.SortOrder = "asc" case "name_desc": criteria.SortBy = "sort_name" criteria.SortOrder = "desc" case "updated_asc": criteria.SortBy = "updated_at" criteria.SortOrder = "asc" case "updated_desc": criteria.SortBy = "updated_at" criteria.SortOrder = "desc" + default: + criteria.SortBy = "updated_at" + criteria.SortOrder = "desc" }If the API spec dictates a different default, adjust accordingly.
internal/infrastructure/clearbit/client.go (3)
59-76: Handle 202 Accepted and 422 Invalid Domain explicitly; improve error mapping.The API can return 202 (queued) with empty body and 422 for invalid domains. Unmarshal on 202 will fail. Check StatusCode on success path and map these to domain errors. Also map 401 more clearly.
resp, err := c.httpClient.Request(ctx, http.MethodGet, url, nil, headers) if err != nil { // Handle specific Clearbit API errors if httpErr, ok := err.(*httpclient.RetryableError); ok { switch httpErr.StatusCode { + case http.StatusUnauthorized: + return nil, errors.NewUnexpected("clearbit authentication failed", err) case http.StatusNotFound: return nil, errors.NewNotFound("company not found") default: return nil, errors.NewUnexpected("unexpected error", err) } } return nil, errors.NewUnexpected("request failed", err) } +// Successful HTTP but non-200 +switch resp.StatusCode { +case http.StatusOK: + // continue +case http.StatusAccepted: + return nil, errors.NewNotFound("company lookup queued; try again later") +case http.StatusUnprocessableEntity: + return nil, errors.NewNotFound("invalid domain") +default: + return nil, errors.NewUnexpected("unexpected status from Clearbit", fmt.Errorf("status %d", resp.StatusCode)) +} var company ClearbitCompany if err := json.Unmarshal(resp.Body, &company); err != nil { return nil, errors.NewUnexpected("failed to decode response", err) }Status behavior per Clearbit docs. (dashboard.clearbit.com)
54-54: Minor: avoid naming paramurlto reduce confusion with net/url import.Rename to
endpointto improve readability.-func (c *Client) makeRequest(ctx context.Context, url string) (*ClearbitCompany, error) { +func (c *Client) makeRequest(ctx context.Context, endpoint string) (*ClearbitCompany, error) { - resp, err := c.httpClient.Request(ctx, http.MethodGet, url, nil, headers) + resp, err := c.httpClient.Request(ctx, http.MethodGet, endpoint, nil, headers)
88-95: Use HEAD for readiness to reduce payload.A HEAD to BaseURL is sufficient and cheaper.
- resp, err := c.httpClient.Request(ctx, http.MethodGet, c.config.BaseURL, nil, nil) + resp, err := c.httpClient.Request(ctx, http.MethodHead, c.config.BaseURL, nil, nil)cmd/service/service.go (1)
111-121: Guard against nil dependencies or document invariantsNewQuerySvc assumes non-nil resourceSearcher, accessControlChecker, and organizationSearcher. If any are nil, later calls will panic. Either validate here (panic with clear message) or ensure providers enforce non-nil.
func NewQuerySvc(resourceSearcher port.ResourceSearcher, accessControlChecker port.AccessControlChecker, organizationSearcher port.OrganizationSearcher, ) querysvc.Service { + if resourceSearcher == nil { + panic("NewQuerySvc: resourceSearcher is nil") + } + if accessControlChecker == nil { + panic("NewQuerySvc: accessControlChecker is nil") + } + if organizationSearcher == nil { + panic("NewQuerySvc: organizationSearcher is nil") + } resourceService := service.NewResourceSearch(resourceSearcher, accessControlChecker) organizationService := service.NewOrganizationSearch(organizationSearcher) return &querySvcsrvc{ resourceService: resourceService, organizationService: organizationService, } }pkg/httpclient/client.go (1)
46-80: Optional: add jitter to backoff to reduce retries synchronizationAdd ±20% jitter to delay.
- delay := c.config.RetryDelay + delay := c.config.RetryDelay if c.config.RetryBackoff { delay = time.Duration(int64(delay) * int64(1<<(attempt-1))) } + // jitter + j := time.Duration(int64(delay) / 5) // 20% + delay += time.Duration((time.Now().UnixNano()%int64(j))*int64(1)) - j/2internal/infrastructure/clearbit/searcher.go (2)
50-56: Guard enrichment when domain is emptyIf name search returns a record without Domain, the subsequent domain lookup will use an empty string.
- if clearbitCompany != nil { - clearbitCompany, err = s.client.FindCompanyByDomain(ctx, clearbitCompany.Domain) + if clearbitCompany != nil && clearbitCompany.Domain != "" { + clearbitCompany, err = s.client.FindCompanyByDomain(ctx, clearbitCompany.Domain) if err == nil { slog.DebugContext(ctx, "found organization by domain", "name", clearbitCompany.Name) } }
58-70: Avoid logging a nil errorWhen no criteria are provided, err may be nil; log only non-nil errors.
- if clearbitCompany == nil { - slog.ErrorContext(ctx, "error searching organization", "error", err) + if clearbitCompany == nil { + if err != nil { + slog.ErrorContext(ctx, "error searching organization", "error", err) + } if criteria.Name != nil && criteria.Domain != nil { return nil, errors.NewNotFound(fmt.Sprintf("organization not found with name '%s' or domain '%s'", *criteria.Name, *criteria.Domain), err) }internal/infrastructure/mock/organization_searcher.go (4)
88-91: Log values, not pointers.Deref criteria so logs show strings instead of pointer addresses.
- slog.DebugContext(ctx, "executing mock organization search", - "name", criteria.Name, - "domain", criteria.Domain, - ) + nameVal, domainVal := "", "" + if criteria.Name != nil { + nameVal = *criteria.Name + } + if criteria.Domain != nil { + domainVal = *criteria.Domain + } + slog.DebugContext(ctx, "executing mock organization search", + "name", nameVal, + "domain", domainVal, + )
13-13: Avoid shadowing stdlib “errors” with import name.Alias the project errors package for clarity.
- "github.com/linuxfoundation/lfx-v2-query-service/pkg/errors" + apperrors "github.com/linuxfoundation/lfx-v2-query-service/pkg/errors"- return nil, errors.NewNotFound(fmt.Sprintf("organization not found with name '%s' or domain '%s'", *criteria.Name, *criteria.Domain)) + return nil, apperrors.NewNotFound(fmt.Sprintf("organization not found with name '%s' or domain '%s'", *criteria.Name, *criteria.Domain)) - return nil, errors.NewNotFound(fmt.Sprintf("organization not found with name '%s'", *criteria.Name)) + return nil, apperrors.NewNotFound(fmt.Sprintf("organization not found with name '%s'", *criteria.Name)) - return nil, errors.NewNotFound(fmt.Sprintf("organization not found with domain '%s'", *criteria.Domain)) + return nil, apperrors.NewNotFound(fmt.Sprintf("organization not found with domain '%s'", *criteria.Domain)) - return nil, errors.NewValidation("no search criteria provided") + return nil, apperrors.NewValidation("no search criteria provided")Also applies to: 117-124
170-172: Return a copy to prevent external mutation of internal slice.Safer for tests that iterate/modify the returned slice.
func (m *MockOrganizationSearcher) GetAllOrganizations() []model.Organization { - return m.organizations + out := make([]model.Organization, len(m.organizations)) + copy(out, m.organizations) + return out }
115-122: Confirm intended semantics when both name and domain are provided.Current logic matches on name OR domain (name wins if both match different orgs). If the API expects AND semantics, adjust accordingly and add a test for the mismatch case.
Happy to provide a patch either way and add the test.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- 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 (28)
CLAUDE.md(3 hunks)README.md(2 hunks)cmd/main.go(3 hunks)cmd/service/converters.go(1 hunks)cmd/service/error.go(2 hunks)cmd/service/jwt.go(1 hunks)cmd/service/providers.go(1 hunks)cmd/service/service.go(3 hunks)design/query-svc.go(3 hunks)design/types.go(2 hunks)internal/domain/model/organization.go(1 hunks)internal/domain/model/search_criteria.go(1 hunks)internal/domain/port/searcher.go(2 hunks)internal/infrastructure/clearbit/client.go(1 hunks)internal/infrastructure/clearbit/config.go(1 hunks)internal/infrastructure/clearbit/models.go(1 hunks)internal/infrastructure/clearbit/searcher.go(1 hunks)internal/infrastructure/clearbit/searcher_test.go(1 hunks)internal/infrastructure/mock/organization_searcher.go(1 hunks)internal/infrastructure/mock/organization_searcher_test.go(1 hunks)internal/service/organization_search.go(1 hunks)internal/service/organization_search_test.go(1 hunks)internal/service/resource_search.go(1 hunks)internal/service/resource_search_test.go(2 hunks)pkg/errors/client.go(1 hunks)pkg/httpclient/client.go(1 hunks)pkg/httpclient/client_test.go(1 hunks)pkg/httpclient/config.go(1 hunks)
🧰 Additional context used
📓 Path-based instructions (7)
cmd/**
📄 CodeRabbit inference engine (CLAUDE.md)
Service implementation connecting Goa to domain logic should be placed in
cmd/
Files:
cmd/service/converters.gocmd/service/providers.gocmd/service/jwt.gocmd/service/error.gocmd/main.gocmd/service/service.go
internal/domain/model/**
📄 CodeRabbit inference engine (CLAUDE.md)
Core business entities should be defined in
internal/domain/model/
Files:
internal/domain/model/organization.gointernal/domain/model/search_criteria.go
**/*_test.go
📄 CodeRabbit inference engine (CLAUDE.md)
**/*_test.go: Test files should follow the*_test.gonaming pattern and be located alongside implementation files
Unit tests should use mock implementations
Integration tests can switch between real and mock implementations
Files:
internal/infrastructure/mock/organization_searcher_test.gointernal/infrastructure/clearbit/searcher_test.gointernal/service/resource_search_test.gopkg/httpclient/client_test.gointernal/service/organization_search_test.go
internal/infrastructure/mock/**
📄 CodeRabbit inference engine (CLAUDE.md)
Mock implementations for testing should be placed in
internal/infrastructure/mock/
Files:
internal/infrastructure/mock/organization_searcher_test.gointernal/infrastructure/mock/organization_searcher.go
internal/domain/port/**
📄 CodeRabbit inference engine (CLAUDE.md)
Domain interfaces (ports) should be defined in
internal/domain/port/
Files:
internal/domain/port/searcher.go
design/**
📄 CodeRabbit inference engine (CLAUDE.md)
design/**: After design changes in thedesign/directory, always runmake apigento regenerate API code
Design specifications for the API should be placed in thedesign/directory
Files:
design/types.godesign/query-svc.go
cmd/main.go
📄 CodeRabbit inference engine (CLAUDE.md)
Concrete implementations should be injected in
cmd/main.gousing dependency injection
Files:
cmd/main.go
🧠 Learnings (6)
📚 Learning: 2025-08-04T22:31:51.082Z
Learnt from: CR
PR: linuxfoundation/lfx-v2-query-service#0
File: CLAUDE.md:0-0
Timestamp: 2025-08-04T22:31:51.082Z
Learning: Applies to internal/usecase/** : Business logic orchestration should be implemented in the use case layer (`internal/usecase/`)
Applied to files:
README.mdCLAUDE.md
📚 Learning: 2025-08-04T22:31:51.082Z
Learnt from: CR
PR: linuxfoundation/lfx-v2-query-service#0
File: CLAUDE.md:0-0
Timestamp: 2025-08-04T22:31:51.082Z
Learning: Follow clean architecture principles with clear separation of concerns between domain, use case, infrastructure, and presentation layers
Applied to files:
README.mdCLAUDE.md
📚 Learning: 2025-08-04T22:31:51.082Z
Learnt from: CR
PR: linuxfoundation/lfx-v2-query-service#0
File: CLAUDE.md:0-0
Timestamp: 2025-08-04T22:31:51.082Z
Learning: Applies to internal/domain/model/** : Core business entities should be defined in `internal/domain/model/`
Applied to files:
README.mdCLAUDE.md
📚 Learning: 2025-08-04T22:31:51.082Z
Learnt from: CR
PR: linuxfoundation/lfx-v2-query-service#0
File: CLAUDE.md:0-0
Timestamp: 2025-08-04T22:31:51.082Z
Learning: Applies to internal/infrastructure/opensearch/** : OpenSearch implementation for resource search should be placed in `internal/infrastructure/opensearch/`
Applied to files:
README.md
📚 Learning: 2025-08-04T22:31:51.082Z
Learnt from: CR
PR: linuxfoundation/lfx-v2-query-service#0
File: CLAUDE.md:0-0
Timestamp: 2025-08-04T22:31:51.082Z
Learning: Applies to **/*_test.go : Unit tests should use mock implementations
Applied to files:
internal/infrastructure/mock/organization_searcher_test.gointernal/service/resource_search_test.go
📚 Learning: 2025-08-04T22:31:51.082Z
Learnt from: CR
PR: linuxfoundation/lfx-v2-query-service#0
File: CLAUDE.md:0-0
Timestamp: 2025-08-04T22:31:51.082Z
Learning: Applies to **/*_test.go : Integration tests can switch between real and mock implementations
Applied to files:
internal/infrastructure/mock/organization_searcher_test.gointernal/service/resource_search_test.go
🧬 Code graph analysis (19)
internal/service/organization_search.go (3)
internal/domain/model/search_criteria.go (1)
OrganizationSearchCriteria(45-50)internal/domain/model/organization.go (1)
Organization(7-18)internal/domain/port/searcher.go (1)
OrganizationSearcher(26-32)
cmd/service/converters.go (5)
gen/query_svc/service.go (3)
QueryResourcesPayload(96-113)QueryResourcesResult(117-124)QueryOrgsPayload(83-92)internal/domain/model/search_criteria.go (3)
SearchCriteria(7-30)SearchResult(33-42)OrganizationSearchCriteria(45-50)pkg/constants/query.go (1)
DefaultPageSize(9-9)pkg/paging/codec.go (1)
DecodePageToken(22-59)pkg/global/token.go (1)
PageTokenSecret(20-35)
internal/infrastructure/mock/organization_searcher_test.go (3)
internal/infrastructure/mock/organization_searcher.go (1)
NewMockOrganizationSearcher(23-84)internal/domain/model/search_criteria.go (1)
OrganizationSearchCriteria(45-50)internal/domain/model/organization.go (1)
Organization(7-18)
pkg/httpclient/client.go (3)
internal/infrastructure/clearbit/config.go (1)
Config(16-31)pkg/httpclient/config.go (1)
Config(11-23)internal/infrastructure/clearbit/client.go (2)
Client(18-21)NewClient(102-114)
cmd/service/providers.go (9)
internal/domain/port/searcher.go (2)
ResourceSearcher(15-21)OrganizationSearcher(26-32)internal/infrastructure/mock/resource_searcher.go (1)
NewMockResourceSearcher(21-133)internal/infrastructure/clearbit/config.go (2)
Config(16-31)NewConfig(44-82)internal/infrastructure/opensearch/searcher.go (1)
NewSearcher(160-199)internal/domain/port/access_control.go (1)
AccessControlChecker(16-25)internal/infrastructure/mock/access_control.go (1)
NewMockAccessControlChecker(132-140)internal/infrastructure/nats/access_control.go (1)
NewAccessControlChecker(72-85)internal/infrastructure/clearbit/searcher.go (2)
OrganizationSearcher(17-19)NewOrganizationSearcher(124-141)internal/infrastructure/mock/organization_searcher.go (1)
NewMockOrganizationSearcher(23-84)
internal/domain/port/searcher.go (2)
internal/domain/model/search_criteria.go (1)
OrganizationSearchCriteria(45-50)internal/domain/model/organization.go (1)
Organization(7-18)
internal/infrastructure/clearbit/searcher_test.go (3)
internal/infrastructure/clearbit/config.go (2)
Config(16-31)DefaultConfig(34-41)internal/infrastructure/clearbit/searcher.go (2)
NewOrganizationSearcher(124-141)OrganizationSearcher(17-19)internal/infrastructure/clearbit/models.go (3)
ClearbitCompany(7-28)ClearbitCategory(36-41)ClearbitMetrics(44-47)
pkg/httpclient/config.go (1)
internal/infrastructure/clearbit/config.go (2)
DefaultConfig(34-41)Config(16-31)
cmd/service/error.go (3)
pkg/errors/client.go (1)
NotFound(29-31)design/types.go (1)
NotFoundError(60-65)gen/query_svc/service.go (1)
NotFoundError(62-65)
internal/service/resource_search_test.go (2)
internal/infrastructure/mock/resource_searcher.go (1)
MockResourceSearcher(16-18)internal/infrastructure/mock/access_control.go (1)
MockAccessControlChecker(17-28)
internal/infrastructure/clearbit/searcher.go (5)
internal/infrastructure/clearbit/client.go (2)
Client(18-21)NewClient(102-114)internal/domain/model/search_criteria.go (1)
OrganizationSearchCriteria(45-50)internal/infrastructure/clearbit/models.go (1)
ClearbitCompany(7-28)pkg/errors/client.go (1)
NewNotFound(39-46)internal/infrastructure/clearbit/config.go (1)
Config(16-31)
pkg/httpclient/client_test.go (2)
pkg/httpclient/config.go (2)
Config(11-23)DefaultConfig(26-33)pkg/httpclient/client.go (3)
NewClient(156-163)Request(22-27)RetryableError(37-40)
internal/infrastructure/clearbit/config.go (1)
pkg/httpclient/config.go (2)
DefaultConfig(26-33)Config(11-23)
internal/service/organization_search_test.go (4)
internal/domain/model/search_criteria.go (1)
OrganizationSearchCriteria(45-50)internal/infrastructure/mock/organization_searcher.go (2)
MockOrganizationSearcher(18-20)NewMockOrganizationSearcher(23-84)pkg/errors/client.go (2)
NotFound(29-31)Validation(9-11)internal/service/organization_search.go (3)
NewOrganizationSearch(65-69)OrganizationSearch(27-29)OrganizationSearcher(17-23)
internal/infrastructure/clearbit/client.go (6)
internal/infrastructure/clearbit/config.go (1)
Config(16-31)pkg/httpclient/config.go (1)
Config(11-23)pkg/httpclient/client.go (3)
Client(16-19)RetryableError(37-40)NewClient(156-163)internal/infrastructure/clearbit/models.go (1)
ClearbitCompany(7-28)pkg/errors/client.go (1)
NewNotFound(39-46)pkg/errors/server.go (1)
NewUnexpected(19-26)
design/query-svc.go (2)
design/types.go (2)
NotFoundError(60-65)Organization(83-101)gen/query_svc/service.go (2)
NotFoundError(62-65)Organization(68-79)
internal/infrastructure/mock/organization_searcher.go (5)
design/types.go (1)
Organization(83-101)internal/domain/model/organization.go (1)
Organization(7-18)gen/query_svc/service.go (1)
Organization(68-79)internal/domain/model/search_criteria.go (1)
OrganizationSearchCriteria(45-50)pkg/errors/client.go (2)
NewNotFound(39-46)NewValidation(19-26)
cmd/main.go (4)
cmd/service/providers.go (3)
SearcherImpl(22-71)AccessControlCheckerImpl(74-144)OrganizationSearcherImpl(147-208)gen/query_svc/service.go (1)
Service(18-28)cmd/service/service.go (1)
NewQuerySvc(111-121)cmd/service/jwt.go (1)
SetupJWTAuth(53-102)
cmd/service/service.go (4)
internal/service/resource_search.go (2)
ResourceSearcher(21-27)NewResourceSearch(222-227)internal/service/organization_search.go (2)
OrganizationSearcher(17-23)NewOrganizationSearch(65-69)gen/query_svc/service.go (3)
QueryOrgsPayload(83-92)Organization(68-79)Service(18-28)design/types.go (1)
Organization(83-101)
🔇 Additional comments (27)
internal/domain/model/search_criteria.go (1)
45-50: LGTM on modeling (pointer fields for tri-state).Pointer semantics for Name/Domain to distinguish “unset” vs “empty” align with validation upstream.
cmd/service/jwt.go (2)
4-4: Package move looks good.Namespace shift to service is consistent with the refactor.
118-121: Defensively strip “Bearer ” prefix in ParsePrincipalParsePrincipal is only called in cmd/service/service.go (line 30); to handle cases where upstream passes the full
Authorizationheader, prepend:func ParsePrincipal(ctx context.Context, token string) (string, error) { + // Strip raw "Bearer " prefix if present. + if strings.HasPrefix(strings.ToLower(strings.TrimSpace(token)), "bearer ") { + token = strings.TrimSpace(token)[7:] + } // existing logic…CLAUDE.md (1)
44-45: Test path update looks good.
Switching TestResourceSearch to ./internal/service/ matches the layer rename.internal/infrastructure/clearbit/searcher_test.go (1)
67-86: Confirm sector mapping intent.
Test expects Sector from SubIndustry. If domain “sector” should reflect Clearbit Sector, update mapping; otherwise consider renaming the domain field to avoid confusion.internal/service/resource_search.go (1)
4-4: Package rename to service: LGTMMatches the architectural shift from usecase → service.
cmd/service/error.go (1)
22-25: 404 mapping added: good addition.NotFound now maps cleanly to the generated NotFoundError.
internal/service/resource_search_test.go (1)
638-639: Test updates for type name: LGTMThe expectations now correctly assert the service-layer type.
Also applies to: 646-647
internal/domain/model/organization.go (1)
6-18: New Organization model: LGTMFields and JSON tags align with the API shape used in design/types.go.
internal/service/organization_search_test.go (1)
16-158: Comprehensive happy-path and error-path coverage: nice work.Covers name/domain queries, case-insensitivity, not-found, and constructor/IsReady behavior.
design/query-svc.go (1)
23-23: Adding NotFound to service errors: LGTMEnables proper 404 mapping end-to-end.
internal/infrastructure/mock/organization_searcher_test.go (1)
1-306: Mock tests are thorough and self-contained: LGTMGood coverage across helpers, case-insensitive matching, and dataset management.
pkg/httpclient/client_test.go (2)
16-35: Constructor test looks solid.
Covers config propagation and http.Client timeout correctly.
217-232: Default config test looks correct.
Values match DefaultConfig.cmd/main.go (2)
58-61: Good DI: providers centralized in service layer.
Complies with cmd/** wiring guideline.
67-67: Correctly injects new organizationSearcher dependency.
Aligns with updated constructor.internal/service/organization_search.go (1)
56-69: Constructor and readiness passthrough look good.
Simple, clear delegation.cmd/service/providers.go (1)
45-69: LGTM on provider selection and sane defaults.Source selection via env with sensible defaults, plus mock backends for local dev, is clear and practical.
Also applies to: 119-144, 160-207
cmd/service/converters.go (2)
79-86: LGTM: clean mapping to domain org criteria.Straight pass-through is correct and keeps the converter minimal.
88-97: LGTM: org response mapping.Pointers for optionality are fine; structure matches the RPC type expectations.
internal/infrastructure/clearbit/client.go (1)
101-114: LGTM: httpclient wiring with retry/backoff looks good.Construction respects timeout/retry config and enables exponential backoff.
cmd/service/service.go (1)
50-65: Resolved: converter methods verified
Both payloadToCriteria (line 18) and domainResultToResponse (line 61) are defined in cmd/service/converters.go with receiver (s *querySvcsrvc).internal/infrastructure/clearbit/searcher.go (3)
22-40: Behavior OK; fallback flow is sensibleSearch by domain first, then by name with enrichment. Good balance of accuracy vs. resilience.
124-141: Init path is solidAPI key validation, readiness probe on startup, and structured logging look good.
22-26: Confirm domain normalization upstream
I didn’t find any URL→domain normalization logic in the converters or Clearbit client—ensure payload website URLs are stripped to bare domains before calling FindCompanyByDomain to avoid Clearbit 404s.internal/infrastructure/mock/organization_searcher.go (2)
4-14: Good placement and package structure.Mock impl lives under internal/infrastructure/mock as per guidelines; package/imports look conventional.
22-84: Seed data is sensible for tests.Reserved TLDs and varied fields make this mock safe and useful.
- Added Clearbit API client and configuration for organization searching. - Implemented organization searcher using Clearbit's Company API to find organizations by name or domain. - Enhanced error handling for API responses and added logging for search operations. - Updated service provider to utilize Clearbit for organization search functionality. - Included tests for the new Clearbit integration and organization searcher. Jira Ticket: https://linuxfoundation.atlassian.net/browse/LFXV2-199 Generated with [Cursor](https://cursor.com/) Signed-off-by: Mauricio Zanetti Salomao <mauriciozanetti86@gmail.com>
…eferences - Renamed `internal/usecase/` to `internal/service/` to better reflect the architecture. - Updated documentation and code references to align with the new service layer naming. - Added new service layer implementation for organization searching and resource searching. - Introduced new test cases for organization search functionality. Jira Ticket: https://linuxfoundation.atlassian.net/browse/LFXV2-199 Generated with [Cursor](https://cursor.com/) Signed-off-by: Mauricio Zanetti Salomao <mauriciozanetti86@gmail.com>
- Added environment variables for application configuration in values.yaml. - Introduced service account configuration in values.yaml and created serviceaccount.yaml template. - Refactored deployment.yaml to utilize new environment variable structure and service account settings. Jira Ticket: https://linuxfoundation.atlassian.net/browse/LFXV2-196 Signed-off-by: Mauricio Zanetti Salomao <mauriciozanetti86@gmail.com>
- Changed environment variable from `CLEARBIT_API_KEY` to `CLEARBIT_CREDENTIAL`. - Enhanced error handling in the Clearbit client to use `http.StatusNotFound` for 404 errors. - Implemented a readiness check for the Clearbit API to verify its availability. - Set a default base URL for the Clearbit API in the configuration. - Updated tests to ensure proper error handling and response writing. Jira Ticket: https://linuxfoundation.atlassian.net/browse/LFXV2-199 Generated with [Cursor](https://cursor.com/) Signed-off-by: Mauricio Zanetti Salomao <mauriciozanetti86@gmail.com>
mauriciozanettisalomao
force-pushed
the
feat/lfxv2-199-org-search-endpoint
branch
from
785b781 to
f253456
Compare
There was a problem hiding this comment.
Actionable comments posted: 6
♻️ Duplicate comments (8)
cmd/service/service.go (2)
76-78: Ensure payload normalization happens here (domain cleanup, lowercasing).Confirm payloadToOrganizationCriteria strips scheme/paths, trims “www.”, lowercases, and handles empty/invalid inputs gracefully. This prevents leaking non-normalized values to the Clearbit client.
91-99: Readyz should verify both resource and organization backends.Currently only resource readiness is checked; outages in org search/clearbit would still report OK.
func (s *querySvcsrvc) Readyz(ctx context.Context) (res []byte, err error) { - errIsReady := s.resourceService.IsReady(ctx) - if errIsReady != nil { - slog.ErrorContext(ctx, "querySvc.readyz failed", "error", errIsReady) - return nil, wrapError(ctx, errIsReady) - } + if err := s.resourceService.IsReady(ctx); err != nil { + slog.ErrorContext(ctx, "querySvc.readyz resource not ready", "error", err) + return nil, wrapError(ctx, err) + } + if err := s.organizationService.IsReady(ctx); err != nil { + slog.ErrorContext(ctx, "querySvc.readyz org search not ready", "error", err) + return nil, wrapError(ctx, err) + } return []byte("OK\n"), nil }pkg/httpclient/client.go (3)
6-13: Add missing imports to support robust retries (body rewind, typed error checks).Required by the fixes below.
Apply:
import ( "context" "fmt" "io" + "bytes" + "errors" "net/http" - "strings" + "net" "time" )
46-79: Retries may resend empty/corrupted bodies and final error message is unclear; rewind body per attempt and keep last response.Buffer/rewind the body for each attempt; keep lastResp; fix error text.
func (c *Client) Do(ctx context.Context, req Request) (*Response, error) { - var lastErr error + var ( + lastErr error + lastResp *Response + ) + + // Prepare reusable body (if any) + var bodyBytes []byte + var seeker io.ReadSeeker + if req.Body != nil { + if rs, ok := req.Body.(io.ReadSeeker); ok { + seeker = rs + _, _ = seeker.Seek(0, io.SeekStart) + } else { + var err error + bodyBytes, err = io.ReadAll(req.Body) + if err != nil { + return nil, fmt.Errorf("failed to buffer request body: %w", err) + } + } + } for attempt := 0; attempt <= c.config.MaxRetries; attempt++ { if attempt > 0 { // Calculate delay with optional exponential backoff delay := c.config.RetryDelay if c.config.RetryBackoff { delay = time.Duration(int64(delay) * int64(1<<(attempt-1))) } select { case <-ctx.Done(): return nil, ctx.Err() case <-time.After(delay): } } + // Rewind/rebuild body for this attempt + if seeker != nil { + _, _ = seeker.Seek(0, io.SeekStart) + req.Body = seeker + } else if bodyBytes != nil { + req.Body = bytes.NewReader(bodyBytes) + } + response, err := c.doRequest(ctx, req) if err == nil { return response, nil } - lastErr = err + lastErr = err + lastResp = response // Don't retry on certain errors if !c.shouldRetry(err) { break } } - return nil, fmt.Errorf("request failed %w", lastErr) + return lastResp, fmt.Errorf("request failed: %w", lastErr) }
125-142: Make retry policy type-safe (net.Error/context) and include 408; avoid retrying on cancellations.Current string matching is brittle.
func (c *Client) shouldRetry(err error) bool { if err == nil { return false } // Check if it's a retryable error if retryableErr, ok := err.(*RetryableError); ok { - // Retry on server errors and rate limiting - return retryableErr.StatusCode >= 500 || retryableErr.StatusCode == 429 + // Retry on server errors, 429, and 408 Request Timeout + return retryableErr.StatusCode >= 500 || retryableErr.StatusCode == 429 || retryableErr.StatusCode == 408 } - // Retry on network-related errors - errStr := strings.ToLower(err.Error()) - return strings.Contains(errStr, "timeout") || - strings.contains(errStr, "connection") || - strings.contains(errStr, "network") + // Respect context signals + if errors.Is(err, context.Canceled) { + return false + } + if errors.Is(err, context.DeadlineExceeded) { + return true + } + // Retry on temporary/timeouts from net.Error + var ne net.Error + if errors.As(err, &ne) { + return ne.Timeout() || ne.Temporary() + } + return false }internal/infrastructure/clearbit/client.go (1)
55-57: Fix auth: Clearbit Company API uses HTTP Basic, not Bearer.Bearer will 401. Send Authorization: Basic base64(API_KEY:).
func (c *Client) makeRequest(ctx context.Context, url string) (*ClearbitCompany, error) { - headers := map[string]string{ - "Authorization": fmt.Sprintf("Bearer %s", c.config.APIKey), - } + auth := base64.StdEncoding.EncodeToString([]byte(c.config.APIKey + ":")) + headers := map[string]string{ + "Authorization": "Basic " + auth, + }Add import:
import ( "context" "encoding/json" + "encoding/base64" "fmt" "net/http" "net/url"internal/infrastructure/clearbit/searcher.go (2)
91-104: Industry/Sector mapping: align with semantic preference.Prefer Industry→Industry; Sector→Sector with sensible fallbacks.
// Map industry information if company.Category != nil { - if company.Category.Industry != "" { - org.Industry = company.Category.Industry - } else if company.Category.Sector != "" { - org.Industry = company.Category.Sector - } - - if company.Category.SubIndustry != "" { - org.Sector = company.Category.SubIndustry - } else if company.Category.IndustryGroup != "" { - org.Sector = company.Category.IndustryGroup - } + // Industry: prefer Industry, then IndustryGroup, then Sector + if v := company.Category.Industry; v != "" { + org.Industry = v + } else if v := company.Category.IndustryGroup; v != "" { + org.Industry = v + } else if v := company.Category.Sector; v != "" { + org.Industry = v + } + // Sector: prefer Sector, then IndustryGroup, then Industry + if v := company.Category.Sector; v != "" { + org.Sector = v + } else if v := company.Category.IndustryGroup; v != "" { + org.Sector = v + } else if v := company.Category.Industry; v != "" { + org.Sector = v + } }
42-56: Avoid redundant/invalid enrichment calls and propagate unexpected errors.Only enrich when domain present and different; bubble up non-NotFound errors.
// If domain search failed or wasn't provided, try name search if clearbitCompany == nil && criteria.Name != nil { slog.DebugContext(ctx, "searching by name", "name", *criteria.Name) clearbitCompany, err = s.client.FindCompanyByName(ctx, *criteria.Name) - if err == nil { + if err == nil { slog.DebugContext(ctx, "found organization by name", "name", clearbitCompany.Name) - } - // search by domain again to enrich the organization - if clearbitCompany != nil { - clearbitCompany, err = s.client.FindCompanyByDomain(ctx, clearbitCompany.Domain) - if err == nil { - slog.DebugContext(ctx, "found organization by domain", "name", clearbitCompany.Name) - } + } else if _, ok := err.(errors.NotFound); !ok { + return nil, err + } + // Enrich via domain only if we have a domain and it differs from the requested one + if clearbitCompany != nil && clearbitCompany.Domain != "" && + (criteria.Domain == nil || clearbitCompany.Domain != *criteria.Domain) { + if enriched, errEnrich := s.client.FindCompanyByDomain(ctx, clearbitCompany.Domain); errEnrich == nil && enriched != nil { + slog.DebugContext(ctx, "found organization by domain (enriched)", "name", enriched.Name) + clearbitCompany = enriched + } } }
🧹 Nitpick comments (11)
cmd/service/service.go (2)
71-74: Fix slog fields: currently logs pointer addresses, not values.p.Name and p.Domain are *string; slog will print pointer addresses. Deref safely before logging.
Apply:
- slog.DebugContext(ctx, "querySvc.query-orgs", - "name", p.Name, - "domain", p.Domain, - ) + var name, domain string + if p.Name != nil { + name = *p.Name + } + if p.Domain != nil { + domain = *p.Domain + } + slog.DebugContext(ctx, "querySvc.query-orgs", + "name", name, + "domain", domain, + )
80-83: Log the failure before mapping the error.Mirror QueryResources’ pattern; helps triage Clearbit-side issues.
- result, errQueryOrgs := s.organizationService.QueryOrganizations(ctx, criteria) - if errQueryOrgs != nil { - return nil, wrapError(ctx, errQueryOrgs) - } + result, errQueryOrgs := s.organizationService.QueryOrganizations(ctx, criteria) + if errQueryOrgs != nil { + slog.ErrorContext(ctx, "querySvc.query-orgs failed", "error", errQueryOrgs) + return nil, wrapError(ctx, errQueryOrgs) + }pkg/httpclient/client.go (1)
46-79: Optional: honor Retry-After for 429/503 to compute delay.Use lastResp.Headers.Get("Retry-After") when present to override/backoff delay.
internal/infrastructure/clearbit/client.go (3)
61-71: Preserve Clearbit error semantics and expose useful messages.Consider mapping 401/403 to a clearer error (e.g., “invalid API key”), and 422 to a client/validation error if your error package has one.
26-34: Safer URL building.Avoid double slashes and manual fmt.Sprintf; prefer url.JoinPath.
- u, err := url.Parse(fmt.Sprintf("%s/v1/domains/find", c.config.BaseURL)) + uStr, err := url.JoinPath(c.config.BaseURL, "v1", "domains", "find") + if err != nil { + return nil, fmt.Errorf("failed to join URL: %w", err) + } + u, err := url.Parse(uStr)Apply similarly for companies find:
- u, err := url.Parse(fmt.Sprintf("%s/v2/companies/find", c.config.BaseURL)) + uStr, err := url.JoinPath(c.config.BaseURL, "v2", "companies", "find") + if err != nil { + return nil, fmt.Errorf("failed to join URL: %w", err) + } + u, err := url.Parse(uStr)Also applies to: 40-49
82-99: Optional: use HEAD for readiness.HEAD to BaseURL reduces bytes; acceptable to keep GET if you want banner text.
charts/lfx-v2-query-service/templates/serviceaccount.yaml (2)
7-15: Adopt standard Kubernetes/Helm labelsUse recommended labels for better tooling and ownership tracking.
metadata: name: {{ .Values.serviceAccount.name | default .Chart.Name }} namespace: {{ .Release.Namespace }} labels: - app: {{ .Chart.Name }} + app: {{ .Chart.Name }} + app.kubernetes.io/name: {{ .Chart.Name }} + app.kubernetes.io/instance: {{ .Release.Name }} + app.kubernetes.io/managed-by: {{ .Release.Service }} + app.kubernetes.io/version: {{ .Chart.AppVersion }} + app.kubernetes.io/part-of: lfx-platform
16-16: Default token automount to falseMore secure default; only enable where needed.
-automountServiceAccountToken: {{ .Values.serviceAccount.automountServiceAccountToken }} +automountServiceAccountToken: {{ .Values.serviceAccount.automountServiceAccountToken | default false }}charts/lfx-v2-query-service/values.yaml (3)
24-25: Deduplicate JWT/JWKS config
JWT_SIGNATURE_ALGORITHMandjwks.urlduplicatejwt.signatureAlgorithmandjwks.url. Keep a single source of truth to avoid drift (either env or structured values, not both).Also applies to: 97-101
54-64: Harden service account defaultsConsider not creating a dedicated SA by default in dev, or at least disable token automount by default.
serviceAccount: # create specifies whether a service account should be created - create: true + create: true # name is the name of the service account to use. # If not set and create is true, a name is generated using the fullname template name: "" # annotations to add to the service account annotations: {} # automountServiceAccountToken is a boolean to determine if the service account token should be automatically mounted - automountServiceAccountToken: true + automountServiceAccountToken: false
18-23: Reduce config duplication via values already presentOPENSEARCH_URL and NATS_URL duplicate the values under
opensearch.urlandnats.url. Consider rendering env vars from those sections to avoid drift.Also applies to: 66-75
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- Jira integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (21)
CLAUDE.md(3 hunks)README.md(2 hunks)charts/lfx-v2-query-service/Chart.yaml(1 hunks)charts/lfx-v2-query-service/templates/deployment.yaml(1 hunks)charts/lfx-v2-query-service/templates/serviceaccount.yaml(1 hunks)charts/lfx-v2-query-service/values.yaml(2 hunks)cmd/service/converters.go(1 hunks)cmd/service/providers.go(1 hunks)cmd/service/service.go(3 hunks)internal/infrastructure/clearbit/client.go(1 hunks)internal/infrastructure/clearbit/config.go(1 hunks)internal/infrastructure/clearbit/models.go(1 hunks)internal/infrastructure/clearbit/searcher.go(1 hunks)internal/infrastructure/clearbit/searcher_test.go(1 hunks)internal/service/organization_search.go(1 hunks)internal/service/organization_search_test.go(1 hunks)internal/service/resource_search.go(1 hunks)internal/service/resource_search_test.go(2 hunks)pkg/httpclient/client.go(1 hunks)pkg/httpclient/client_test.go(1 hunks)pkg/httpclient/config.go(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- charts/lfx-v2-query-service/Chart.yaml
🚧 Files skipped from review as they are similar to previous changes (12)
- internal/service/resource_search.go
- internal/service/resource_search_test.go
- cmd/service/providers.go
- CLAUDE.md
- pkg/httpclient/config.go
- internal/service/organization_search.go
- internal/infrastructure/clearbit/searcher_test.go
- internal/infrastructure/clearbit/config.go
- README.md
- cmd/service/converters.go
- internal/service/organization_search_test.go
- pkg/httpclient/client_test.go
🧰 Additional context used
📓 Path-based instructions (1)
cmd/**
📄 CodeRabbit inference engine (CLAUDE.md)
Service implementation connecting Goa to domain logic should be placed in
cmd/
Files:
cmd/service/service.go
🧬 Code graph analysis (4)
pkg/httpclient/client.go (3)
internal/infrastructure/clearbit/config.go (1)
Config(16-31)pkg/httpclient/config.go (1)
Config(11-23)internal/infrastructure/clearbit/client.go (2)
Client(18-21)NewClient(102-114)
internal/infrastructure/clearbit/client.go (6)
internal/infrastructure/clearbit/config.go (1)
Config(16-31)pkg/httpclient/config.go (1)
Config(11-23)pkg/httpclient/client.go (4)
Client(16-19)Request(22-27)RetryableError(37-40)NewClient(156-163)internal/infrastructure/clearbit/models.go (1)
ClearbitCompany(7-28)pkg/errors/client.go (1)
NewNotFound(39-46)pkg/errors/server.go (1)
NewUnexpected(19-26)
cmd/service/service.go (4)
internal/service/resource_search.go (2)
ResourceSearcher(21-27)NewResourceSearch(222-227)internal/service/organization_search.go (2)
OrganizationSearcher(17-23)NewOrganizationSearch(65-69)gen/query_svc/service.go (3)
QueryOrgsPayload(83-92)Organization(68-79)Service(18-28)design/types.go (1)
Organization(83-101)
internal/infrastructure/clearbit/searcher.go (5)
internal/infrastructure/clearbit/client.go (2)
Client(18-21)NewClient(102-114)internal/domain/model/search_criteria.go (1)
OrganizationSearchCriteria(45-50)internal/infrastructure/clearbit/models.go (1)
ClearbitCompany(7-28)pkg/errors/client.go (1)
NewNotFound(39-46)internal/infrastructure/clearbit/config.go (1)
Config(16-31)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: MegaLinter
🔇 Additional comments (8)
cmd/service/service.go (5)
4-4: Package placement aligns with guidelines.Service entrypoint wired under cmd/service as required.
12-12: Correct layer dependency.Switch to internal/service is appropriate for orchestration vs. port/domain.
21-23: Adding organizationService is the right DI shape.Clean split between resource and org searchers looks good.
85-87: Guard against nil result to avoid downstream panics.If the org service returns (nil, nil) for some not-found or empty-domain cases, domainOrganizationToResponse must be nil-safe or handle early here.
Would you like me to add a nil check here or make domainOrganizationToResponse accept a nil input safely?
111-120: Constructor wiring LGTM.Resource and organization services are correctly constructed and injected.
internal/infrastructure/clearbit/models.go (1)
6-28: Models look correct and aligned with Clearbit payloads.No issues found.
internal/infrastructure/clearbit/searcher.go (2)
106-113: Employees mapping: keep numeric when range missing.Current logic is fine; just noting ordering is correct (range preferred, else exact).
123-141: Constructor behavior LGTM (API key check + readiness).Good guardrails; no changes needed.
- Updated resource handling in the domainResultToResponse function to create local copies of loop variables, preventing potential issues with variable addresses. - Enhanced tests for the Clearbit organization searcher by implementing a local test server, allowing for offline testing and improved reliability. - Added checks for organization domain presence before querying, ensuring more robust error handling in the search process. - Adjusted organization search logging to use local variables for clarity. Reviewed with [GitHub Copilot](https://github.com/features/copilot) Jira Ticket: https://linuxfoundation.atlassian.net/browse/LFXV2-199 Signed-off-by: Mauricio Zanetti Salomao <mauriciozanetti86@gmail.com>
- Updated version in Chart.yaml to reflect the new release. Jira Ticket: https://linuxfoundation.atlassian.net/browse/LFXV2-199 Signed-off-by: Mauricio Zanetti Salomao <mauriciozanetti86@gmail.com>
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (3)
internal/service/organization_search.go (1)
48-57: Nil-safe result logging LGTM; consider logging a found flag and consistent keys.This avoids the prior nil deref risk. Add a found boolean and align key names for easier querying.
- slog.DebugContext(ctx, "organization search completed", - "organization_name", orgName, - "organization_domain", orgDomain, - ) + slog.DebugContext(ctx, "organization search completed", + "found", result != nil, + "result_name", orgName, + "result_domain", orgDomain, + )pkg/httpclient/client_test.go (2)
153-159: Align MaxRetries with “retry attempts” semantics (use 2, not 3).For “2 failures + 1 success” you need 2 retries. Using 3 is harmless but confusing.
config := Config{ Timeout: 5 * time.Second, - MaxRetries: 3, + MaxRetries: 2, RetryDelay: 10 * time.Millisecond, // Short delay for testing RetryBackoff: false, }
103-124: Use errors.As instead of fabricating RetryableError (prevents masked failures).The current test creates a fake RetryableError, which can hide real issues. Assert by unwrapping with errors.As.
Apply this diff:
- // The error might be wrapped, so we need to check the underlying error - var retryableErr *RetryableError - found := false - if re, ok := err.(*RetryableError); ok { - retryableErr = re - found = true - } else { - // Check if it's a wrapped error - t.Logf("Error type: %T, Error: %v", err, err) - // For now, just check that we got an error - the wrapping behavior might be different - found = true - // Create a mock retryableErr for the rest of the test - retryableErr = &RetryableError{StatusCode: 404} - } - - if !found { - t.Fatalf("Expected RetryableError or wrapped error, got %T", err) - } - - if retryableErr.StatusCode != http.StatusNotFound { - t.Errorf("Expected status code 404, got %d", retryableErr.StatusCode) - } + var re *RetryableError + if errors.As(err, &re) { + if re.StatusCode != http.StatusNotFound { + t.Errorf("Expected status code 404, got %d", re.StatusCode) + } + } else { + t.Fatalf("Expected RetryableError, got %T: %v", err, err) + }And add the missing import (see next comment).
🧹 Nitpick comments (9)
internal/service/organization_search.go (6)
14-24: Avoid duplicating the port interface; rename or remove the service-local twin.This interface mirrors port.OrganizationSearcher and the identical name causes cognitive load. Either return the concrete type from the constructor or rename this interface to avoid confusion.
-// OrganizationSearcher defines the interface for organization search operations +// OrganizationSearchService defines the interface for organization search operations ... -type OrganizationSearcher interface { +type OrganizationSearchService interface { // QueryOrganizations searches for organizations based on the provided criteria QueryOrganizations(ctx context.Context, criteria model.OrganizationSearchCriteria) (*model.Organization, error) // IsReady checks if the search service is ready IsReady(ctx context.Context) error } @@ -// NewOrganizationSearch creates a new OrganizationSearch instance -func NewOrganizationSearch(organizationSearcher port.OrganizationSearcher) OrganizationSearcher { +// NewOrganizationSearch creates a new OrganizationSearch instance +func NewOrganizationSearch(organizationSearcher port.OrganizationSearcher) OrganizationSearchService { return &OrganizationSearch{ organizationSearcher: organizationSearcher, } }Also applies to: 71-75
31-32: Doc says “with business logic validation” but none is implemented here.Either implement validation here or update the comment to reflect pass-through behavior (validation happens at API layer).
34-37: Deref pointers in logs to avoid pointer addresses.criteria.Name/Domain are *string and will log as pointers; log their values.
- slog.DebugContext(ctx, "starting organization search", - "name", criteria.Name, - "domain", criteria.Domain, - ) + var name, domain string + if criteria.Name != nil { + name = *criteria.Name + } + if criteria.Domain != nil { + domain = *criteria.Domain + } + slog.DebugContext(ctx, "starting organization search", + "name", name, + "domain", domain, + )
39-46: Include criteria in error logs for traceability.Add the (deref’d) criteria to the error log so failures are diagnosable.
- if err != nil { - slog.ErrorContext(ctx, "organization search operation failed while executing query organizations", - "error", err, - ) + if err != nil { + slog.ErrorContext(ctx, "organization search failed", + "error", err, + "name", name, + "domain", domain, + ) return nil, err }
62-68: Simplify IsReady.Minor readability nit: inline the return.
-func (s *OrganizationSearch) IsReady(ctx context.Context) error { - if err := s.organizationSearcher.IsReady(ctx); err != nil { - return err - } - return nil -} +func (s *OrganizationSearch) IsReady(ctx context.Context) error { + return s.organizationSearcher.IsReady(ctx) +}
71-75: Guard against nil dependency in constructor.Defensive check prevents late panics if a nil searcher is injected.
func NewOrganizationSearch(organizationSearcher port.OrganizationSearcher) OrganizationSearcher { - return &OrganizationSearch{ + if organizationSearcher == nil { + panic("NewOrganizationSearch: organizationSearcher is nil") + } + return &OrganizationSearch{ organizationSearcher: organizationSearcher, } }pkg/httpclient/client_test.go (3)
39-49: Also assert request headers are forwarded.server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { if r.Method != "GET" { t.Errorf("Expected GET request, got %s", r.Method) } + if got := r.Header.Get("Custom-Header"); got != "custom-value" { + t.Errorf("Expected header Custom-Header=custom-value, got %q", got) + } w.WriteHeader(http.StatusOK)
81-90: Ensure 404 is not retried.Guard against accidental retries on client errors by asserting single attempt.
func TestClient_Get_NotFound(t *testing.T) { - // Create a test server that returns 404 - server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + // Create a test server that returns 404 + callCount := 0 + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + callCount++ w.WriteHeader(http.StatusNotFound) _, err := w.Write([]byte(`{"error": "not found"}`)) if err != nil { t.Errorf("Expected no error, got %v", err) } })) @@ _, err := client.Request(ctx, "GET", server.URL, nil, nil) // Should return response with error if err == nil { t.Fatal("Expected error for 404 status, got none") } + if callCount != 1 { + t.Errorf("Expected 1 call (no retry on 404), got %d", callCount) + }Also applies to: 96-102
177-195: Assert Content-Type header on POST.server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { if r.Method != "POST" { t.Errorf("Expected POST request, got %s", r.Method) } + if ct := r.Header.Get("Content-Type"); ct != "application/json" { + t.Errorf("Expected Content-Type application/json, got %q", ct) + } body, _ := io.ReadAll(r.Body)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- Jira integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (7)
charts/lfx-v2-query-service/Chart.yaml(1 hunks)cmd/service/converters.go(1 hunks)internal/infrastructure/clearbit/searcher.go(1 hunks)internal/infrastructure/clearbit/searcher_test.go(1 hunks)internal/service/organization_search.go(1 hunks)pkg/httpclient/client_test.go(1 hunks)pkg/httpclient/config.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (5)
- internal/infrastructure/clearbit/searcher.go
- cmd/service/converters.go
- pkg/httpclient/config.go
- charts/lfx-v2-query-service/Chart.yaml
- internal/infrastructure/clearbit/searcher_test.go
🧰 Additional context used
📓 Path-based instructions (1)
**/*_test.go
📄 CodeRabbit inference engine (CLAUDE.md)
**/*_test.go: Test files should follow the*_test.gonaming pattern and be located alongside implementation files
Unit tests should use mock implementations
Integration tests can switch between real and mock implementations
Files:
pkg/httpclient/client_test.go
🧬 Code graph analysis (2)
internal/service/organization_search.go (3)
internal/domain/model/search_criteria.go (1)
OrganizationSearchCriteria(45-50)internal/domain/model/organization.go (1)
Organization(7-18)internal/domain/port/searcher.go (1)
OrganizationSearcher(26-32)
pkg/httpclient/client_test.go (2)
pkg/httpclient/config.go (2)
Config(11-23)DefaultConfig(26-33)pkg/httpclient/client.go (3)
NewClient(156-163)Request(22-27)RetryableError(37-40)
🔇 Additional comments (1)
pkg/httpclient/client_test.go (1)
217-232: Defaults coverage looks good.
| func (s *querySvcsrvc) payloadToOrganizationCriteria(ctx context.Context, p *querysvc.QueryOrgsPayload) model.OrganizationSearchCriteria { | ||
| criteria := model.OrganizationSearchCriteria{ | ||
| Name: p.Name, | ||
| Domain: p.Domain, |
There was a problem hiding this comment.
I think we can normalize the domain before sending to clearbit. Not sure if clearbit handles it
func normalizeDomain(input string) string {
// Remove protocol
input = strings.TrimPrefix(input, "https://")
input = strings.TrimPrefix(input, "http://")
// Remove www prefix
input = strings.TrimPrefix(input, "www.")
// Parse to extract host
if u, err := url.Parse("http://" + input); err == nil {
return strings.ToLower(u.Host)
}
return strings.ToLower(input)
}
There was a problem hiding this comment.
For now, the protocol is not allowed (RegExp on the GOA side). Should we be more flexible and accept the protocol?
400 Bad Request
....
"message": "domain must match the regexp \"^[a-zA-Z0-9][a-zA-Z0-9-_.]*[a-zA-Z0-9]*\\\\.[a-zA-Z]{2,}$\" but got value \"https://www.linuxfoundation.org/\"",
Also, Clearbit handles wwww
GET /query/orgs?v=1&domain=www.linuxfoundation.org
HTTP/1.1 200 OK
Content-Type: application/json
X-Request-Id: 5ad5138c-9332-4646-b634-7676b56696c7
Date: Wed, 10 Sep 2025 14:39:21 GMT
Content-Length: 176
{"name":"The Linux Foundation","domain":"linuxfoundation.org","industry":"Internet Software \u0026 Services","sector":"Internet Software \u0026 Services","employees":"51-250"}
There was a problem hiding this comment.
I like the use of an httpclient package in case we need clients for other APIs.
There was a problem hiding this comment.
Nice reorganizing of some of the code from the long main.go file. It's cleaner like this.
There was a problem hiding this comment.
We might want to document the clearbit API usage and setup in the README at some point. Not needed as part of this PR but something for soon.
There was a problem hiding this comment.
You're right! I'll add to my TODO list. Thanks!
- Introduced Clearbit integration in the README, detailing organization search capabilities using the Clearbit Company API. - Updated environment variables for Clearbit configuration, including API credentials and settings. - Enhanced documentation on Clearbit API setup, usage, and error handling. Jira Ticket: https://linuxfoundation.atlassian.net/browse/LFXV2-199 Generated with [Cursor](https://cursor.com/) Signed-off-by: Mauricio Zanetti Salomao <mauriciozanetti86@gmail.com>
mauriciozanettisalomao
dismissed stale reviews from andrest50 and prabodhcs
via
e7cecee
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 (4)
README.md (4)
23-24: Directory tree is outdated; include cmd/service package.Reflect the new service wiring location in the tree to prevent confusion.
├── cmd/ # Services (main packages) -│ └── query_svc/ # Query service implementation +│ ├── query_svc/ # Query service entrypoint +│ └── service/ # Service providers/wiring (DI, clients, config)
69-73: Dependency Injection location is outdated.DI moved out of cmd/main.go into cmd/service providers; update this section to point to the new wiring.
-Dependency injection is performed in `cmd/main.go`, where the concrete implementations for resource search and access control are selected based on configuration and then injected into the service constructor. +Dependency injection is centralized in `cmd/service/` (e.g., `providers.go`), which wires concrete implementations (search, org search, access control, Clearbit/OpenSearch/NATS clients) based on environment configuration and injects them into service constructors.
171-188: Add /query/orgs endpoint to API Usage.Document the new org search route, parameters, and auth to avoid discoverability gaps.
### API Usage @@ -The service exposes a RESTful API through the Goa framework: +The service exposes a RESTful API through the Goa framework: + +Organization search: +``` +GET /query/orgs?v=1&domain=linuxfoundation.org +Authorization: Bearer <JWT> +``` + +Parameters: +- `v` (required): API version (enum: "1") +- `domain` (optional): Company domain (regex-validated) +- `name` (optional): Company name (min length enforced) + +Responses: +- 200: `{ "name", "domain", "industry", "sector", "employees" }` +- 400: Validation/authentication errors +- 404: Organization not found (including Clearbit `unknown_record`) + Resource search:GET /query/resources?name=committee&type=committee&v=1
281-293: Update extension steps to reference new DI location.The steps still say “add config to main.go” and “update DI switch statement.” Point contributors to cmd/service providers instead.
-3. Add configuration options to `main.go` -4. Update the dependency injection switch statement +3. Add configuration options and wiring in `cmd/service/providers.go` +4. Extend the provider selection (SEARCH_SOURCE / ORG_SEARCH_SOURCE / ACCESS_CONTROL_SOURCE) in `cmd/service/`
♻️ Duplicate comments (4)
pkg/httpclient/client_test.go (3)
130-175: Align MaxRetries with “retry attempts” semantics.
Config.MaxRetries is the number of retries (initial + MaxRetries attempts). For “2 failures + 1 success” (3 total), set MaxRetries to 2. Keep the 3-call assertion.config := Config{ Timeout: 5 * time.Second, - MaxRetries: 3, + MaxRetries: 2, RetryDelay: 10 * time.Millisecond, // Short delay for testing RetryBackoff: false, }
6-14: Add missing import for errors.As.
Needed for the 404 test refactor below.import ( + "errors" "context" "io" "net/http" "net/http/httptest" "strings" "testing" "time" )
81-128: Use errors.As to unwrap RetryableError and assert no retries on 404.
Avoid fabricating errors and ensure 404s aren’t retried.func TestClient_Get_NotFound(t *testing.T) { - // Create a test server that returns 404 - server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { - w.WriteHeader(http.StatusNotFound) - _, err := w.Write([]byte(`{"error": "not found"}`)) - if err != nil { - t.Errorf("Expected no error, got %v", err) - } - })) + callCount := 0 + // Create a test server that returns 404 + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + callCount++ + w.WriteHeader(http.StatusNotFound) + if _, err := w.Write([]byte(`{"error": "not found"}`)); err != nil { + t.Errorf("Expected no error, got %v", err) + } + })) defer server.Close() config := DefaultConfig() client := NewClient(config) ctx := context.Background() _, err := client.Request(ctx, "GET", server.URL, nil, nil) // Should return response with error if err == nil { t.Fatal("Expected error for 404 status, got none") } - // The error might be wrapped, so we need to check the underlying error - var retryableErr *RetryableError - found := false - if re, ok := err.(*RetryableError); ok { - retryableErr = re - found = true - } else { - // Check if it's a wrapped error - t.Logf("Error type: %T, Error: %v", err, err) - // For now, just check that we got an error - the wrapping behavior might be different - found = true - // Create a mock retryableErr for the rest of the test - retryableErr = &RetryableError{StatusCode: 404} - } - - if !found { - t.Fatalf("Expected RetryableError or wrapped error, got %T", err) - } - - if retryableErr.StatusCode != http.StatusNotFound { - t.Errorf("Expected status code 404, got %d", retryableErr.StatusCode) - } - - // Note: The response might be nil when the error is wrapped - // This is acceptable behavior for the HTTP client + var re *RetryableError + if !errors.As(err, &re) { + t.Fatalf("Expected RetryableError, got %T: %v", err, err) + } + if re.StatusCode != http.StatusNotFound { + t.Errorf("Expected status code 404, got %d", re.StatusCode) + } + // 404 should not be retried. + if callCount != 1 { + t.Errorf("Expected 1 call (no retries) for 404, got %d", callCount) + } }README.md (1)
209-239: Clearbit setup docs: nicely added, resolves prior request.This addresses the earlier ask to document Clearbit usage and setup.
🧹 Nitpick comments (6)
pkg/httpclient/client_test.go (3)
37-49: Assert the custom request header is propagated.
You set Custom-Header but don’t verify it.server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { if r.Method != "GET" { t.Errorf("Expected GET request, got %s", r.Method) } + if got := r.Header.Get("Custom-Header"); got != "custom-value" { + t.Errorf("Expected Custom-Header=custom-value, got %q", got) + } w.WriteHeader(http.StatusOK)
177-195: Also assert Content-Type header for POST.
Verifies header propagation for requests with bodies.server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { if r.Method != "POST" { t.Errorf("Expected POST request, got %s", r.Method) } + if ct := r.Header.Get("Content-Type"); ct != "application/json" { + t.Errorf("Expected Content-Type=application/json, got %q", ct) + } body, _ := io.ReadAll(r.Body)
16-35: Run tests in parallel.
These tests are isolated; parallelizing speeds up CI.func TestNewClient(t *testing.T) { + t.Parallel() @@ func TestClient_Get_Success(t *testing.T) { + t.Parallel() @@ func TestClient_Get_NotFound(t *testing.T) { + t.Parallel() @@ func TestClient_Retry_ServerError(t *testing.T) { + t.Parallel() @@ func TestClient_Post(t *testing.T) { + t.Parallel() @@ func TestDefaultConfig(t *testing.T) { + t.Parallel()Also applies to: 37-79, 81-128, 130-175, 177-215, 217-232
cmd/service/converters.go (2)
51-54: Avoid duplicate page-token debug logs (codec already logs encoded/decoded)paging.DecodePageToken emits both the encoded token and normalized search_after; this second debug block duplicates it and adds noise. Safe to remove.
- slog.DebugContext(ctx, "decoded page token", - "page_token", *criteria.PageToken, - "decoded", pageToken, - )Note: per retrieved learning, PageToken isn’t sensitive in this codebase; this suggestion is purely for log hygiene. [used_learnings]
92-100: Guard against nil org to prevent panicsIf callers ever pass nil (e.g., conditional paths around 404 handling), this will panic. Cheap defensive check:
func (s *querySvcsrvc) domainOrganizationToResponse(org *model.Organization) *querysvc.Organization { + if org == nil { + return nil + } return &querysvc.Organization{ Name: &org.Name, Domain: &org.Domain, Industry: &org.Industry, Sector: &org.Sector, Employees: &org.Employees, } }README.md (1)
65-68: Add Clearbit limitations and behavior notes.Document known behaviors to set expectations for consumers (from PR notes).
#### Clearbit Implementation - -The Clearbit implementation provides organization search capabilities using the Clearbit Company API. It includes a client for API communication, searcher for organization queries, and configuration management for API credentials and settings. +The Clearbit implementation provides organization search capabilities using the Clearbit Company API. It includes a client for API communication, searcher for organization queries, and configuration management for API credentials and settings. + +Known limitations: +- Name-only searches may return `unknown_record` from Clearbit and surface as HTTP 404. +- No internal caching is performed; all lookups hit Clearbit directly. +- Availability and latency depend on Clearbit; respect rate limits.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- Jira integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (8)
README.md(7 hunks)charts/lfx-v2-query-service/Chart.yaml(1 hunks)cmd/service/converters.go(1 hunks)internal/infrastructure/clearbit/searcher.go(1 hunks)internal/infrastructure/clearbit/searcher_test.go(1 hunks)internal/service/organization_search.go(1 hunks)pkg/httpclient/client_test.go(1 hunks)pkg/httpclient/config.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (5)
- internal/infrastructure/clearbit/searcher_test.go
- internal/service/organization_search.go
- charts/lfx-v2-query-service/Chart.yaml
- internal/infrastructure/clearbit/searcher.go
- pkg/httpclient/config.go
🧰 Additional context used
📓 Path-based instructions (2)
cmd/**
📄 CodeRabbit inference engine (CLAUDE.md)
Service implementation connecting Goa to domain logic should be placed in
cmd/
Files:
cmd/service/converters.go
**/*_test.go
📄 CodeRabbit inference engine (CLAUDE.md)
**/*_test.go: Test files should follow the*_test.gonaming pattern and be located alongside implementation files
Unit tests should use mock implementations
Integration tests can switch between real and mock implementations
Files:
pkg/httpclient/client_test.go
🧠 Learnings (7)
📚 Learning: 2025-09-09T12:48:02.892Z
Learnt from: mauriciozanettisalomao
PR: linuxfoundation/lfx-v2-query-service#15
File: cmd/service/converters.go:51-54
Timestamp: 2025-09-09T12:48:02.892Z
Learning: In the lfx-v2-query-service codebase, PageToken used for pagination does not contain sensitive data and can be safely logged for debugging purposes.
Applied to files:
cmd/service/converters.go
📚 Learning: 2025-09-09T12:53:56.738Z
Learnt from: mauriciozanettisalomao
PR: linuxfoundation/lfx-v2-query-service#15
File: cmd/service/service.go:68-88
Timestamp: 2025-09-09T12:53:56.738Z
Learning: In the LFX V2 Query Service, domain validation for the QueryOrgs endpoint is handled by the GOA framework using the regex pattern `^[a-zA-Z0-9][a-zA-Z0-9-_.]*[a-zA-Z0-9]*\.[a-zA-Z]{2,}$`. This ensures only properly formatted domain names (without URL schemes or paths) reach the service layer, eliminating the need for additional domain normalization in the converter methods.
Applied to files:
cmd/service/converters.go
📚 Learning: 2025-08-04T22:31:51.082Z
Learnt from: CR
PR: linuxfoundation/lfx-v2-query-service#0
File: CLAUDE.md:0-0
Timestamp: 2025-08-04T22:31:51.082Z
Learning: Applies to internal/usecase/** : Business logic orchestration should be implemented in the use case layer (`internal/usecase/`)
Applied to files:
README.md
📚 Learning: 2025-08-04T22:31:51.082Z
Learnt from: CR
PR: linuxfoundation/lfx-v2-query-service#0
File: CLAUDE.md:0-0
Timestamp: 2025-08-04T22:31:51.082Z
Learning: Follow clean architecture principles with clear separation of concerns between domain, use case, infrastructure, and presentation layers
Applied to files:
README.md
📚 Learning: 2025-08-04T22:31:51.082Z
Learnt from: CR
PR: linuxfoundation/lfx-v2-query-service#0
File: CLAUDE.md:0-0
Timestamp: 2025-08-04T22:31:51.082Z
Learning: Applies to internal/domain/model/** : Core business entities should be defined in `internal/domain/model/`
Applied to files:
README.md
📚 Learning: 2025-08-04T22:31:51.082Z
Learnt from: CR
PR: linuxfoundation/lfx-v2-query-service#0
File: CLAUDE.md:0-0
Timestamp: 2025-08-04T22:31:51.082Z
Learning: Applies to internal/infrastructure/opensearch/** : OpenSearch implementation for resource search should be placed in `internal/infrastructure/opensearch/`
Applied to files:
README.md
📚 Learning: 2025-08-04T22:31:51.082Z
Learnt from: CR
PR: linuxfoundation/lfx-v2-query-service#0
File: CLAUDE.md:0-0
Timestamp: 2025-08-04T22:31:51.082Z
Learning: Applies to cmd/main.go : Concrete implementations should be injected in `cmd/main.go` using dependency injection
Applied to files:
README.md
🧬 Code graph analysis (2)
cmd/service/converters.go (5)
gen/query_svc/service.go (3)
QueryResourcesPayload(96-113)QueryResourcesResult(117-124)QueryOrgsPayload(83-92)internal/domain/model/search_criteria.go (3)
SearchCriteria(7-30)SearchResult(33-42)OrganizationSearchCriteria(45-50)pkg/constants/query.go (1)
DefaultPageSize(9-9)pkg/paging/codec.go (1)
DecodePageToken(22-59)pkg/global/token.go (1)
PageTokenSecret(20-35)
pkg/httpclient/client_test.go (3)
pkg/httpclient/config.go (2)
Config(11-23)DefaultConfig(26-33)internal/infrastructure/clearbit/client.go (1)
NewClient(102-114)pkg/httpclient/client.go (3)
NewClient(156-163)Request(22-27)RetryableError(37-40)
🔇 Additional comments (6)
pkg/httpclient/client_test.go (1)
216-232: Defaults test looks correct.
Matches pkg/httpclient/config.go.cmd/service/converters.go (3)
68-77: Good fix for loop-variable address aliasingCreating locals for Type and ID before taking addresses avoids the classic range-variable pointer bug. LGTM.
82-89: Organization criteria mapping is appropriately minimalDirectly passing Name and Domain aligns with GOA-side validation (regex) and avoids redundant normalization here. LGTM. [used_learnings]
25-42: Remove default override suggestion
Upstream decoding always sets p.Sort to one of the four supported literals (defaulting toname_asc), and theswitchhandles each case—so the initialSortBy: p.Sortassignment is safe. Ignore the prior recommendation to default toupdated_at/desc.Likely an incorrect or invalid review comment.
README.md (2)
50-54: Service Layer section reads well.Accurately describes orchestration responsibilities after the rename.
137-140: ORG_SEARCH_SOURCE variable addition LGTM.Defaults and choices are clear.
| │ │ ├── model/ # Domain models and entities | ||
| │ │ └── port/ # Domain interfaces/ports | ||
| │ ├── usecase/ # Business logic/use cases layer | ||
| │ ├── service/ # Business logic/use cases layer |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Rename to Service layer looks good; verify no lingering "usecase" references.
The directory rename aligns with the refactor. Please confirm there are no stale references to internal/usecase across docs and code.
🏁 Script executed:
#!/bin/bash
# Find lingering references to the old "usecase" layer
rg -nP -C2 '\binternal/usecase\b|\bUse\s*Case\b'Length of output: 0
🏁 Script executed:
#!/bin/bash
# Check for any lingering “usecase” references across code and docs
rg -inP -C2 '\busecase\b|use[\s_-]*case\b' .Length of output: 363
Update stale “Use case” reference in CLAUDE.md
- CLAUDE.md:96 still reads “Use case orchestration”; rename to “Service orchestration” (or “Service layer orchestration”) to match the updated directory naming.
🤖 Prompt for AI Agents
In README.md around line 28, the documentation still refers to "Use case
orchestration" which is now stale; update the reference in CLAUDE.md line 96 to
read "Service orchestration" (or "Service layer orchestration") to match the
directory name change, and ensure any other occurrences in README.md or
CLAUDE.md are renamed consistently.
| #### With Production Services | ||
|
|
||
| ```bash | ||
| # Using OpenSearch and NATS (production-like setup) | ||
| # production-like setup | ||
| SEARCH_SOURCE=opensearch \ | ||
| ORG_SEARCH_SOURCE=clearbit \ | ||
| ACCESS_CONTROL_SOURCE=nats \ | ||
| OPENSEARCH_URL={{placeholder}} \ | ||
| OPENSEARCH_INDEX=resources \ | ||
| NATS_URL{{placeholder}} \ | ||
| CLEARBIT_CREDENTIAL=your_clearbit_api_key \ | ||
| CLEARBIT_BASE_URL=https://company.clearbit.com \ | ||
| CLEARBIT_TIMEOUT=30s \ | ||
| CLEARBIT_MAX_RETRIES=5 \ | ||
| CLEARBIT_RETRY_DELAY=2s \ | ||
| go run cmd/main.go | ||
| ``` |
There was a problem hiding this comment.
Fix NATS_URL typo in production example.
Missing '=' will break copy/paste.
OPENSEARCH_INDEX=resources \
-NATS_URL{{placeholder}} \
+NATS_URL={{placeholder}} \
CLEARBIT_CREDENTIAL=your_clearbit_api_key \
CLEARBIT_BASE_URL=https://company.clearbit.com \
CLEARBIT_TIMEOUT=30s \
CLEARBIT_MAX_RETRIES=5 \
CLEARBIT_RETRY_DELAY=2s \📝 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.
| #### With Production Services | |
| ```bash | |
| # Using OpenSearch and NATS (production-like setup) | |
| # production-like setup | |
| SEARCH_SOURCE=opensearch \ | |
| ORG_SEARCH_SOURCE=clearbit \ | |
| ACCESS_CONTROL_SOURCE=nats \ | |
| OPENSEARCH_URL={{placeholder}} \ | |
| OPENSEARCH_INDEX=resources \ | |
| NATS_URL{{placeholder}} \ | |
| CLEARBIT_CREDENTIAL=your_clearbit_api_key \ | |
| CLEARBIT_BASE_URL=https://company.clearbit.com \ | |
| CLEARBIT_TIMEOUT=30s \ | |
| CLEARBIT_MAX_RETRIES=5 \ | |
| CLEARBIT_RETRY_DELAY=2s \ | |
| go run cmd/main.go | |
| ``` | |
| OPENSEARCH_INDEX=resources \ | |
| NATS_URL={{placeholder}} \ | |
| CLEARBIT_CREDENTIAL=your_clearbit_api_key \ |
🤖 Prompt for AI Agents
In README.md around lines 113 to 129, the production example has a typo in the
NATS URL environment variable ("NATS_URL{{placeholder}}" missing the '=').
Update that line to include the equals sign so it reads
"NATS_URL={{placeholder}}" to ensure the env var is set correctly for copy/paste
usage.
| **Clearbit Configuration:** | ||
|
|
||
| - `CLEARBIT_CREDENTIAL`: Clearbit API key (required for organization search) | ||
| - `CLEARBIT_BASE_URL`: Clearbit API base URL (default: `https://company.clearbit.com`) | ||
| - `CLEARBIT_TIMEOUT`: HTTP client timeout for API requests (default: "10s") | ||
| - `CLEARBIT_MAX_RETRIES`: Maximum number of retry attempts for failed requests (default: "3") | ||
| - `CLEARBIT_RETRY_DELAY`: Delay between retry attempts (default: "1s") | ||
|
|
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Verify Clearbit default values match code.
Ensure docs and internal/infrastructure/clearbit/config.go are consistent for timeout, retries, and delay.
🏁 Script executed:
#!/bin/bash
# Confirm documented defaults vs. code
rg -nP -C2 '(TIMEOUT|RETRY|DELAY|CREDENTIAL|BASE_URL)' internal/infrastructure/clearbit/config.goLength of output: 0
🏁 Script executed:
#!/bin/bash
# Locate Clearbit config file(s)
fd config.go -t f | grep -i clearbit
# Inspect default values in the located file(s)
fd config.go -t f | grep -i clearbit | xargs -r rg -nP -C2 'Default.*|Base_URL|Timeout|Retries|Delay'Length of output: 1305
Update Clearbit timeout default in README. Change CLEARBIT_TIMEOUT default from “10s” to “30s” in README.md (currently lines ~157–164) to match DefaultConfig() in internal/infrastructure/clearbit/config.go:37. All other defaults (Base URL, MaxRetries, RetryDelay) already align.
🤖 Prompt for AI Agents
In README.md around lines 157 to 164, the documented default for
CLEARBIT_TIMEOUT is incorrect ("10s"); update it to "30s" so it matches the
DefaultConfig() in internal/infrastructure/clearbit/config.go (around line 37).
Change the bullet for CLEARBIT_TIMEOUT to show default: "30s" and ensure no
other formatting or wording changes are made.
|
|
||
| The Clearbit integration includes robust error handling: | ||
|
|
||
| - **404 Not Found**: Returns appropriate "organization not found" errors | ||
| - **Rate Limiting**: Automatic retry with exponential backoff | ||
| - **Network Issues**: Configurable retry attempts with delays | ||
| - **API Errors**: Proper error propagation with context |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Confirm retry strategy; align wording if not exponential backoff.
Docs promise “Automatic retry with exponential backoff.” Verify implementation matches (e.g., backoff algorithm and jitter). If linear or fixed, adjust wording.
🏁 Script executed:
#!/bin/bash
# Inspect Clearbit client for retry/backoff strategy
rg -nP -C3 '(retry|backoff|exponential|jitter)' internal/infrastructure/clearbit/Length of output: 3119
🏁 Script executed:
#!/bin/bash
# Inspect Clearbit client for backoff logic by finding sleep calls
rg -nP -C3 'Sleep' internal/infrastructure/clearbit/Length of output: 0
#!/bin/bash
# Locate and display the Clearbit searcher implementation to inspect retry/backoff logic
fd -e go searcher.go internal/infrastructure/clearbit
sed -n '1,200p' internal/infrastructure/clearbit/searcher.goUpdate documentation to reflect fixed retry delays
The implementation uses a constant RetryDelay (default 1 s) across MaxRetries (default 3) with no exponential backoff or jitter. In README.md replace “Automatic retry with exponential backoff” with wording like “Automatic retry with configurable fixed delays.”
🤖 Prompt for AI Agents
In README.md around lines 254 to 260, the documentation incorrectly claims the
Clearbit integration uses exponential backoff for retries; update the phrasing
to state that retries use configurable fixed delays (default RetryDelay = 1s)
across MaxRetries (default 3) and do not include exponential backoff or jitter.
Replace the bullet "Rate Limiting: Automatic retry with exponential backoff"
with wording such as "Rate Limiting: Automatic retry with configurable fixed
delays" and, if desired, add a short parenthetical to mention defaults
(RetryDelay = 1s, MaxRetries = 3).
Overview
This pull request refactors the codebase to consolidate service initialization and orchestration logic, moving it from
cmd/main.goandcmd/query_svcinto a newcmd/servicepackage. It also updates documentation and naming to reflect this change, introduces support for organization search via Clearbit, and improves error handling. The changes help clarify the separation of responsibilities according to clean architecture principles and make the codebase easier to maintain and extend.🔴 CLEARBIT API DEPENDENCY: This endpoint is completely dependent on the Clearbit Company API for all organization data. The service does not maintain its own organization database or provide suggested company search functionality.
Summary
📋 Test Results Summary
🔍 Detailed Test Cases
✅ Successful Scenarios
1. Domain-based Organization Lookup
Response (200 OK):
{ "name": "The Linux Foundation", "domain": "linuxfoundation.org", "industry": "Internet Software & Services", "sector": "Internet Software & Services", "employees": "51-250" }2. Large Organization Lookup
Response (200 OK):
{ "name": "Google", "domain": "google.com", "industry": "Internet Software & Services", "sector": "Internet", "employees": "100K+" }3. Combined Parameters
Response (200 OK): Same as domain-only lookup
4. Name-only Search
Response (404 Not Found):
{ "message": "organization not found with name 'Linux Foundation': request failed: request failed HTTP 404: {\n \"error\": {\n \"type\": \"unknown_record\",\n \"message\": \"Unknown company.\"\n }\n}" }Note: This indicates dependency on external Clearbit API for name-based searches.
🚫 Error Handling
5. Invalid Domain Format
Response (400 Bad Request):
{ "name": "invalid_pattern", "message": "domain must match the regexp \"^[a-zA-Z0-9][a-zA-Z0-9-_.]*[a-zA-Z0-9]*\\\\.[a-zA-Z]{2,}$\" but got value \"invalid-domain\"" }6. Missing Required Parameters
GET /query/orgs?name=testResponse (400 Bad Request):
{ "name": "missing_field", "message": "\"version\" is missing from query string; value of version must be one of \"1\" but got value \"\"" }7. Authentication Failure
Response (400 Bad Request):
{ "name": "missing_field", "message": "\"bearer_token\" is missing from header" }🔧 API Specification Compliance
Parameters
v(version): Required, validates enum ["1"]name: Optional, minimum length validationdomain: Optional, regex pattern validationResponse Formats
Organization Object Schema
{ "name": "string", "domain": "string", "industry": "string", "sector": "string", "employees": "string" }🎯 Key Findings