refactor: replace native service with ITX proxy for GroupsIO operations#36
Open
refactor: replace native service with ITX proxy for GroupsIO operations#36
Conversation
Removes the NATS KV storage backend, direct Groups.io API client, committee sync, webhook processing, and mailing list sync in favour of a thin proxy that forwards all GroupsIO operations to the ITX API (https://api.prod.itx.linuxfoundation.org). Key changes: - New GOA design exposes 23 GroupsIO endpoints under /groupsio/* paths, accepting LFX v2 project/committee UUIDs in the API surface - New internal/infrastructure/proxy: Auth0 M2M OAuth2 HTTP client (RS256 private-key assertion, token caching via oauth2.ReuseTokenSource) - New internal/infrastructure/idmapper: NATS-based v1/v2 ID mapper (lfx.lookup_v1_mapping subject) with a no-op fallback for local dev - New internal/service/itx: service layer that maps v2 UUIDs to v1 SFIDs on requests and back on responses before/after each ITX call - New internal/domain: IDMapper interface, ITXGroupsioClient interface, DomainError types, and ITX request/response models - Regenerated gen/ via make apigen to match new design - Unit tests for all new packages: domain errors, no-op mapper, NATS mapper (validation + committee response parsing), proxy HTTP client (URL construction, error mapping, all endpoints), and ITX service layer (ID mapping logic, error propagation) Co-authored-by: Claude Sonnet 4.6 <[email protected]> Signed-off-by: Jordan Evans <[email protected]>
WalkthroughThe PR refactors the mailing-list API from a locally-implemented GroupsIO client to an ITX proxy-based architecture. It removes committee/mailing-list synchronization, eliminates the local GroupsIO client implementation, introduces ITX-backed domain services, adds ID mapping infrastructure, restructures API design from grpsio to groupsio naming with ID-based identifiers, and simplifies service composition. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant MailingListAPI as mailingListAPI<br/>(service)
participant ITXService as GroupsioServiceService<br/>(ITX)
participant IDMapper as IDMapper<br/>(NATS/NoOp)
participant ITXBackend as ITX Backend
Client->>MailingListAPI: CreateGroupsioService(ctx, payload)
MailingListAPI->>IDMapper: MapProjectV1ToV2(ctx, projectUID)
IDMapper-->>MailingListAPI: v2ProjectID
MailingListAPI->>ITXService: CreateService(ctx, mappedReq)
ITXService->>ITXBackend: HTTP POST /services
ITXBackend-->>ITXService: GroupsioService
ITXService-->>MailingListAPI: domain.Service
MailingListAPI->>MailingListAPI: convertService(domain.Service)
MailingListAPI-->>Client: GroupsioService response
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
📝 Coding Plan
Comment Tip You can customize the high-level summary generated by CodeRabbit.Configure the |
There was a problem hiding this comment.
Copilot reviewed 43 out of 90 changed files in this pull request and generated no comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. 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: 5
🧹 Nitpick comments (7)
internal/infrastructure/idmapper/nats_mapper.go (3)
36-57: Constructor uses plainfmt.Errorfinstead of domain error types.Lines 39 and 50 use
fmt.Errorffor errors, while the rest of the file usesdomain.NewValidationErroranddomain.NewUnavailableError. Consider using consistent error types throughout:♻️ Suggested improvement for consistency
func NewNATSMapper(cfg Config) (*NATSMapper, error) { if cfg.URL == "" { - return nil, fmt.Errorf("NATS URL is required") + return nil, domain.NewValidationError("NATS URL is required") } timeout := cfg.Timeout if timeout == 0 { timeout = defaultTimeout } // Connect to NATS server conn, err := nats.Connect(cfg.URL) if err != nil { - return nil, fmt.Errorf("failed to connect to NATS: %w", err) + return nil, domain.NewUnavailableError("failed to connect to NATS", err) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/infrastructure/idmapper/nats_mapper.go` around lines 36 - 57, Replace plain fmt.Errorf usages in NewNATSMapper with the domain error constructors used elsewhere: return a domain.NewValidationError when cfg.URL == "" (include similar message "NATS URL is required") and return a domain.NewUnavailableError wrapping the nats.Connect error when connection fails (preserve the wrapped error from nats.Connect). Update the imports if needed and keep semantics (timeout handling and returned *NATSMapper) unchanged.
134-141: Useerrors.Isfor error comparison.Line 137 compares errors using
==, which works for sentinel errors but is less idiomatic. Usingerrors.Ishandles wrapped errors and is the recommended approach.♻️ Suggested improvement
+ "errors" "fmt" "strings"func (m *NATSMapper) lookup(ctx context.Context, key string) (string, error) { msg, err := m.conn.RequestWithContext(ctx, lookupSubject, []byte(key)) if err != nil { - if err == context.DeadlineExceeded || err == nats.ErrTimeout { + if errors.Is(err, context.DeadlineExceeded) || errors.Is(err, nats.ErrTimeout) { return "", domain.NewUnavailableError("v1-sync-helper lookup timed out", err) } return "", domain.NewUnavailableError("failed to lookup ID mapping", err) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/infrastructure/idmapper/nats_mapper.go` around lines 134 - 141, In NATSMapper.lookup replace the equality check on err (currently `err == context.DeadlineExceeded || err == nats.ErrTimeout`) with errors.Is checks so wrapped errors are handled: use errors.Is(err, context.DeadlineExceeded) || errors.Is(err, nats.ErrTimeout) and ensure the package imports the standard "errors" package; keep the existing domain.NewUnavailableError return paths and behavior unchanged.
133-157: Consider adding structured logging for observability.The
lookupfunction handles several error conditions (timeout, upstream errors, not-found) but doesn't log any diagnostics. Adding structured logging withslogwould aid debugging in production.As per coding guidelines,
**/*.go: "Use structured logging with slog package throughout the application"♻️ Example logging addition
import ( "context" + "errors" "fmt" + "log/slog" "strings" "time"func (m *NATSMapper) lookup(ctx context.Context, key string) (string, error) { msg, err := m.conn.RequestWithContext(ctx, lookupSubject, []byte(key)) if err != nil { - if err == context.DeadlineExceeded || err == nats.ErrTimeout { + if errors.Is(err, context.DeadlineExceeded) || errors.Is(err, nats.ErrTimeout) { + slog.WarnContext(ctx, "v1-sync-helper lookup timed out", "key", key) return "", domain.NewUnavailableError("v1-sync-helper lookup timed out", err) } + slog.ErrorContext(ctx, "failed to lookup ID mapping", "key", key, "error", err) return "", domain.NewUnavailableError("failed to lookup ID mapping", err) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/infrastructure/idmapper/nats_mapper.go` around lines 133 - 157, The lookup method on NATSMapper should emit structured logs for each failure and notable path: add slog logging in NATSMapper.lookup to record the key and context when a RequestWithContext error occurs (including err and whether it was a timeout), when the response starts with "error: " (log upstream error message and raw response), when response is empty (log not-found for key), and optionally log a debug/info entry on successful lookup with the resolved ID; use slog with fields like "component":"NATSMapper", "key":key, "error":err, and "response" to aid observability and include these logs around the RequestWithContext call, the strings.HasPrefix(response, "error: ") branch, the empty response branch, and the successful return.internal/infrastructure/idmapper/nats_mapper_test.go (1)
23-30: Consider using a more reliable unreachable address.Using
localhost:14222with a comment that it's "unlikely to be in use" could cause flaky tests if the port happens to be occupied. Consider usingnats://invalid.test:4222(a reserved.testTLD that will never resolve) for more deterministic failure.♻️ Suggested improvement
func TestNewNATSMapper_UnreachableURL(t *testing.T) { _, err := NewNATSMapper(Config{ - URL: "nats://localhost:14222", // port unlikely to be in use + URL: "nats://unreachable.invalid:4222", // .invalid TLD guaranteed not to resolve Timeout: 100 * time.Millisecond, }) require.Error(t, err) assert.Contains(t, err.Error(), "failed to connect to NATS") }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/infrastructure/idmapper/nats_mapper_test.go` around lines 23 - 30, Replace the flaky unreachable URL in the TestNewNATSMapper_UnreachableURL test: update the Config passed to NewNATSMapper (in TestNewNATSMapper_UnreachableURL) to use a deterministic non-resolving host such as "nats://invalid.test:4222" instead of "nats://localhost:14222" so the call to NewNATSMapper(Config{URL: ..., Timeout: ...}) reliably fails and the require.Error/assert.Contains checks remain valid.internal/infrastructure/idmapper/noop_mapper_test.go (1)
46-51: Interface compliance test doesn't verify the interface.The comment states this test "verifies the NoOpMapper satisfies the domain.IDMapper interface at compile time," but
assert.NotNil(t, m)only checks that the constructor returns a non-nil value. It doesn't actually verify interface compliance.Consider adding an explicit compile-time interface check:
♻️ Suggested improvement
func TestNoOpMapper_ImplementsInterface(t *testing.T) { - // Verifies the NoOpMapper satisfies the domain.IDMapper interface at compile time - // via the type assertion in providers.go — this test documents the contract - m := NewNoOpMapper() - assert.NotNil(t, m) + // Compile-time interface compliance check + var _ domain.IDMapper = (*NoOpMapper)(nil) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/infrastructure/idmapper/noop_mapper_test.go` around lines 46 - 51, Update TestNoOpMapper_ImplementsInterface to perform an explicit compile-time interface assertion rather than only checking the constructor return; replace or augment the assert.NotNil(t, m) with a type assertion that assigns NewNoOpMapper() to a variable of type domain.IDMapper (or use a blank identifier var _ domain.IDMapper = (*NoOpMapper)(nil)) to ensure NoOpMapper implements domain.IDMapper at compile time, referencing the NewNoOpMapper constructor and the domain.IDMapper interface (and NoOpMapper type) so the test fails to compile if the interface contract changes.cmd/mailing-list-api/service/mailing_list_api.go (2)
59-62: Consider adding dependency health checks to readiness probe.The readiness probe returns a static "OK" without verifying that the ITX proxy backend is reachable. For a proxy service, this could cause traffic to be routed to an instance that cannot fulfill requests.
If the ITX client supports a health check or ping method, consider invoking it here. Otherwise, this implementation is acceptable for simpler deployments where Kubernetes probes are primarily for process liveness.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/mailing-list-api/service/mailing_list_api.go` around lines 59 - 62, The readiness probe Readyz on type mailingListAPI currently returns static "OK"; update it to perform a dependency health check by invoking the ITX client health/ping method (or other backend-check function) exposed on the mailingListAPI struct (e.g., s.itxClient.Health or s.pingITX) and return an error (and non-OK bytes) when that check fails so Kubernetes will mark the instance not ready; if no such method exists, add a simple ping method on mailingListAPI that exercises the ITX connection and call it from Readyz before returning "OK".
302-322: Consider adding structured logging for error observability.The error mapping logic is correct, but errors are silently mapped without any logging. For operational observability, consider adding a debug or warn-level log when mapping non-nil errors. This aids in troubleshooting without impacting the response.
As per coding guidelines, "Use structured logging with slog package throughout the application."
📊 Suggested improvement for logging
func mapDomainError(err error) error { if err == nil { return nil } + slog.Debug("mapping domain error", "error", err) var domErr *domain.DomainError if !errors.As(err, &domErr) { + slog.Warn("unexpected non-domain error", "error", err) return &mailinglist.InternalServerError{Message: err.Error()} }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/mailing-list-api/service/mailing_list_api.go` around lines 302 - 322, The mapDomainError function currently maps domain.DomainError values to API errors without logging; add structured slog logging at the start of mapDomainError for any non-nil err so operators can observe mappings (use slog.Debug or slog.Warn per severity). Specifically, when err is non-nil log the original error and, if errors.As(err, &domErr) succeeds, include domErr.Type and domErr.Message in the log fields; when errors.As fails, log the original err.Error() and indicate it will be mapped to mailinglist.InternalServerError. Use the slog package consistently and keep messages concise and structured.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@cmd/mailing-list-api/design/mailing_list.go`:
- Around line 435-440: The payload for the add-groupsio-member endpoint extends
GroupsioMemberRequestType but only marks "subgroup_id" as required, allowing an
empty create payload; update the dsl.Payload block (the one calling
Extend(GroupsioMemberRequestType) inside the add-groupsio-member method) to also
require the "email" attribute by adding it to the method-level required list
(e.g., add "email" to the dsl.Required call for this payload or include a second
dsl.Required("email") so the endpoint enforces email on create).
In `@cmd/mailing-list-api/main.go`:
- Around line 61-64: The code calls proxy.NewClient(itxConfig) which can panic
on missing private-key/auth; before invoking proxy.NewClient, validate the ITX
config returned by service.ITXProxyConfig() (check required fields such as
private key, auth token, endpoint or any bools the proxy client expects) and if
validation fails log a clear error via process or application logger and exit
cleanly (os.Exit(1)) instead of allowing a panic; alternatively wrap
proxy.NewClient(itxConfig) in a defer/recover block to convert any panic into a
logged error and controlled exit—update the initialization site that uses
service.ITXProxyConfig and proxy.NewClient accordingly.
In `@cmd/mailing-list-api/service/providers.go`:
- Around line 67-89: The IDMapper function currently falls back to
idmapper.NewNoOpMapper on missing NATS_URL or NewNATSMapper errors which
silently allows wrong pass-through IDs; instead make it fail fast: in IDMapper,
after checking ID_MAPPING_DISABLED, if NATS_URL is empty or
idmapper.NewNATSMapper returns an error, log the error with context (include the
env var and the error) and terminate startup (e.g. slog.Error/With(...).Fatal or
call os.Exit(1)) so the service does not run with idmapper.NewNoOpMapper; keep
idmapper.NewNoOpMapper only for the explicit ID_MAPPING_DISABLED=true branch.
Reference: IDMapper, idmapper.NewNATSMapper, idmapper.NewNoOpMapper,
ID_MAPPING_DISABLED and NATS_URL.
In `@internal/domain/errors.go`:
- Around line 19-65: The DomainError type and its constructors (DomainError,
NewValidationError, NewNotFoundError, NewConflictError, NewInternalError,
NewUnavailableError) should be removed and replaced with the shared pkg/errors
usage: change each constructor to call the corresponding pkg/errors factory
(e.g., pkg/errors.NewValidation / NewNotFound / NewConflict / NewUnexpected or
NewServiceUnavailable as appropriate) and return the pkg/errors error type;
update GetErrorType to detect and map the pkg/errors semantic types (use
errors.As against the pkg/errors types or their exported helpers) instead of
asserting *DomainError; ensure Unwrap/Error behavior relies on the pkg/errors
implementation so no custom DomainError remains.
In `@internal/infrastructure/idmapper/nats_mapper_test.go`:
- Around line 138-143: The test currently constructs NATSMapper directly and
doesn't exercise the defaulting logic in NewNATSMapper; change the test to call
NewNATSMapper with an option/config that sets Timeout: 0 (so NewNATSMapper runs
its defaulting code) and then assert that the returned mapper's timeout field
equals defaultTimeout; also keep an assertion that no error/panic occurred
during NewNATSMapper creation. Reference NewNATSMapper, NATSMapper, and
defaultTimeout when locating the code to change.
---
Nitpick comments:
In `@cmd/mailing-list-api/service/mailing_list_api.go`:
- Around line 59-62: The readiness probe Readyz on type mailingListAPI currently
returns static "OK"; update it to perform a dependency health check by invoking
the ITX client health/ping method (or other backend-check function) exposed on
the mailingListAPI struct (e.g., s.itxClient.Health or s.pingITX) and return an
error (and non-OK bytes) when that check fails so Kubernetes will mark the
instance not ready; if no such method exists, add a simple ping method on
mailingListAPI that exercises the ITX connection and call it from Readyz before
returning "OK".
- Around line 302-322: The mapDomainError function currently maps
domain.DomainError values to API errors without logging; add structured slog
logging at the start of mapDomainError for any non-nil err so operators can
observe mappings (use slog.Debug or slog.Warn per severity). Specifically, when
err is non-nil log the original error and, if errors.As(err, &domErr) succeeds,
include domErr.Type and domErr.Message in the log fields; when errors.As fails,
log the original err.Error() and indicate it will be mapped to
mailinglist.InternalServerError. Use the slog package consistently and keep
messages concise and structured.
In `@internal/infrastructure/idmapper/nats_mapper_test.go`:
- Around line 23-30: Replace the flaky unreachable URL in the
TestNewNATSMapper_UnreachableURL test: update the Config passed to NewNATSMapper
(in TestNewNATSMapper_UnreachableURL) to use a deterministic non-resolving host
such as "nats://invalid.test:4222" instead of "nats://localhost:14222" so the
call to NewNATSMapper(Config{URL: ..., Timeout: ...}) reliably fails and the
require.Error/assert.Contains checks remain valid.
In `@internal/infrastructure/idmapper/nats_mapper.go`:
- Around line 36-57: Replace plain fmt.Errorf usages in NewNATSMapper with the
domain error constructors used elsewhere: return a domain.NewValidationError
when cfg.URL == "" (include similar message "NATS URL is required") and return a
domain.NewUnavailableError wrapping the nats.Connect error when connection fails
(preserve the wrapped error from nats.Connect). Update the imports if needed and
keep semantics (timeout handling and returned *NATSMapper) unchanged.
- Around line 134-141: In NATSMapper.lookup replace the equality check on err
(currently `err == context.DeadlineExceeded || err == nats.ErrTimeout`) with
errors.Is checks so wrapped errors are handled: use errors.Is(err,
context.DeadlineExceeded) || errors.Is(err, nats.ErrTimeout) and ensure the
package imports the standard "errors" package; keep the existing
domain.NewUnavailableError return paths and behavior unchanged.
- Around line 133-157: The lookup method on NATSMapper should emit structured
logs for each failure and notable path: add slog logging in NATSMapper.lookup to
record the key and context when a RequestWithContext error occurs (including err
and whether it was a timeout), when the response starts with "error: " (log
upstream error message and raw response), when response is empty (log not-found
for key), and optionally log a debug/info entry on successful lookup with the
resolved ID; use slog with fields like "component":"NATSMapper", "key":key,
"error":err, and "response" to aid observability and include these logs around
the RequestWithContext call, the strings.HasPrefix(response, "error: ") branch,
the empty response branch, and the successful return.
In `@internal/infrastructure/idmapper/noop_mapper_test.go`:
- Around line 46-51: Update TestNoOpMapper_ImplementsInterface to perform an
explicit compile-time interface assertion rather than only checking the
constructor return; replace or augment the assert.NotNil(t, m) with a type
assertion that assigns NewNoOpMapper() to a variable of type domain.IDMapper (or
use a blank identifier var _ domain.IDMapper = (*NoOpMapper)(nil)) to ensure
NoOpMapper implements domain.IDMapper at compile time, referencing the
NewNoOpMapper constructor and the domain.IDMapper interface (and NoOpMapper
type) so the test fails to compile if the interface contract changes.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: a9aeb52f-a70d-4350-8a77-d9766128c0d5
⛔ Files ignored due to path filters (18)
gen/http/cli/mailing_list/cli.gois excluded by!**/gen/**gen/http/mailing_list/client/cli.gois excluded by!**/gen/**gen/http/mailing_list/client/client.gois excluded by!**/gen/**gen/http/mailing_list/client/encode_decode.gois excluded by!**/gen/**gen/http/mailing_list/client/paths.gois excluded by!**/gen/**gen/http/mailing_list/client/types.gois excluded by!**/gen/**gen/http/mailing_list/server/encode_decode.gois excluded by!**/gen/**gen/http/mailing_list/server/paths.gois excluded by!**/gen/**gen/http/mailing_list/server/server.gois excluded by!**/gen/**gen/http/mailing_list/server/types.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/mailing_list/client.gois excluded by!**/gen/**gen/mailing_list/endpoints.gois excluded by!**/gen/**gen/mailing_list/service.gois excluded by!**/gen/**go.sumis excluded by!**/*.sum
📒 Files selected for processing (72)
cmd/mailing-list-api/committee.gocmd/mailing-list-api/design/mailing_list.gocmd/mailing-list-api/design/type.gocmd/mailing-list-api/http.gocmd/mailing-list-api/mailing_list_sync.gocmd/mailing-list-api/main.gocmd/mailing-list-api/service/error.gocmd/mailing-list-api/service/grpsio_webhook_test.gocmd/mailing-list-api/service/mailing_list_api.gocmd/mailing-list-api/service/mailing_list_service.gocmd/mailing-list-api/service/mailing_list_validators_test.gocmd/mailing-list-api/service/providers.gocmd/mailing-list-api/service/service_payload_converters.gocmd/mailing-list-api/service/service_payload_converters_test.gocmd/mailing-list-api/service/service_response_converters.gocmd/mailing-list-api/service/service_response_converters_test.gocmd/mailing-list-api/service/service_validators.gocmd/mailing-list-api/service/service_validators_test.gogo.modinternal/domain/errors.gointernal/domain/errors_test.gointernal/domain/id_mapper.gointernal/domain/itx_proxy.gointernal/domain/models/itx_groupsio.gointernal/infrastructure/groupsio/client.gointernal/infrastructure/groupsio/config.gointernal/infrastructure/groupsio/errors.gointernal/infrastructure/groupsio/grpsio_webhook_validator.gointernal/infrastructure/groupsio/models.gointernal/infrastructure/idmapper/nats_mapper.gointernal/infrastructure/idmapper/nats_mapper_test.gointernal/infrastructure/idmapper/noop_mapper.gointernal/infrastructure/idmapper/noop_mapper_test.gointernal/infrastructure/mock/error_simulation_test.gointernal/infrastructure/mock/grpsio.gointernal/infrastructure/mock/grpsio_webhook_processor.gointernal/infrastructure/mock/grpsio_webhook_validator.gointernal/infrastructure/mock/message_publisher.gointernal/infrastructure/nats/client.gointernal/infrastructure/nats/messaging_publish.gointernal/infrastructure/nats/messaging_request.gointernal/infrastructure/nats/models.gointernal/infrastructure/nats/storage.gointernal/infrastructure/proxy/client.gointernal/infrastructure/proxy/client_test.gointernal/service/committee_sync_service.gointernal/service/committee_sync_service_test.gointernal/service/grpsio_mailing_list_reader.gointernal/service/grpsio_mailing_list_reader_test.gointernal/service/grpsio_mailing_list_writer.gointernal/service/grpsio_mailing_list_writer_test.gointernal/service/grpsio_member_reader.gointernal/service/grpsio_member_reader_test.gointernal/service/grpsio_member_writer.gointernal/service/grpsio_member_writer_test.gointernal/service/grpsio_reader.gointernal/service/grpsio_service_reader.gointernal/service/grpsio_service_reader_test.gointernal/service/grpsio_service_writer.gointernal/service/grpsio_service_writer_test.gointernal/service/grpsio_webhook_processor.gointernal/service/grpsio_webhook_processor_test.gointernal/service/grpsio_writer.gointernal/service/itx/groupsio_member.gointernal/service/itx/groupsio_member_test.gointernal/service/itx/groupsio_service.gointernal/service/itx/groupsio_service_test.gointernal/service/itx/groupsio_subgroup.gointernal/service/itx/groupsio_subgroup_test.gointernal/service/itx/mocks_test.gointernal/service/mailing_list_sync_service.gointernal/service/mailing_list_sync_service_test.go
💤 Files with no reviewable changes (17)
- cmd/mailing-list-api/http.go
- internal/infrastructure/mock/error_simulation_test.go
- internal/infrastructure/groupsio/grpsio_webhook_validator.go
- cmd/mailing-list-api/service/service_response_converters_test.go
- cmd/mailing-list-api/service/error.go
- cmd/mailing-list-api/service/mailing_list_validators_test.go
- cmd/mailing-list-api/service/grpsio_webhook_test.go
- internal/infrastructure/groupsio/errors.go
- internal/infrastructure/groupsio/config.go
- cmd/mailing-list-api/service/service_response_converters.go
- cmd/mailing-list-api/service/service_validators.go
- cmd/mailing-list-api/service/service_payload_converters_test.go
- cmd/mailing-list-api/service/service_payload_converters.go
- internal/infrastructure/groupsio/client.go
- cmd/mailing-list-api/service/service_validators_test.go
- cmd/mailing-list-api/service/mailing_list_service.go
- internal/infrastructure/groupsio/models.go
Comment on lines
+435
to
+440
| dsl.Payload(func() { | ||
| BearerTokenAttribute() | ||
| dsl.Attribute("subgroup_id", dsl.String, "Subgroup ID") | ||
| dsl.Extend(GroupsioMemberRequestType) | ||
| dsl.Required("subgroup_id") | ||
| dsl.Token("bearer_token", dsl.String) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the generated payload and validation for add-groupsio-member.
rg -n -C8 'type AddGroupsioMemberPayload struct|func \(.*AddGroupsioMemberPayload.*\) Validate\(' gen
# Expected: `subgroup_id` is enforced today; `email` is not.Repository: linuxfoundation/lfx-v2-mailing-list-service
Length of output: 1732
🏁 Script executed:
# Find the Validate method for AddGroupsioMemberPayload
rg -n -A30 'func \(p \*AddGroupsioMemberPayload\) Validate' genRepository: linuxfoundation/lfx-v2-mailing-list-service
Length of output: 69
🏁 Script executed:
# Confirm current state of the design file at lines 435-440
sed -n '430,445p' cmd/mailing-list-api/design/mailing_list.goRepository: linuxfoundation/lfx-v2-mailing-list-service
Length of output: 658
🏁 Script executed:
# Search for any Validate method in the generated code
rg -n 'func.*Validate' gen/mailing_list/Repository: linuxfoundation/lfx-v2-mailing-list-service
Length of output: 69
🏁 Script executed:
# Find GroupsioMemberRequestType definition in the design file
rg -n -B2 -A15 'var GroupsioMemberRequestType|GroupsioMemberRequestType\s*=' cmd/mailing-list-api/design/mailing_list.go | head -40Repository: linuxfoundation/lfx-v2-mailing-list-service
Length of output: 69
🏁 Script executed:
# Check if Goa enforces required fields at the payload struct level
rg -n 'email.*required|required.*email' gen/mailing_list/Repository: linuxfoundation/lfx-v2-mailing-list-service
Length of output: 69
🏁 Script executed:
# Search for GroupsioMemberRequestType definition
rg -n 'GroupsioMemberRequestType' cmd/mailing-list-api/design/mailing_list.goRepository: linuxfoundation/lfx-v2-mailing-list-service
Length of output: 176
🏁 Script executed:
# Get broader context around the definition
rg -n -B5 -A20 'GroupsioMemberRequestType.*func|func.*GroupsioMemberRequestType' cmd/mailing-list-api/design/Repository: linuxfoundation/lfx-v2-mailing-list-service
Length of output: 2206
🏁 Script executed:
# Check what's in the generated types.go to understand the payload structure
head -200 gen/mailing_list/types.go | tail -100Repository: linuxfoundation/lfx-v2-mailing-list-service
Length of output: 171
🏁 Script executed:
# Check if dsl.Required is used with multiple arguments elsewhere in design files
rg -n 'dsl\.Required\([^)]*,[^)]*\)' cmd/mailing-list-api/design/Repository: linuxfoundation/lfx-v2-mailing-list-service
Length of output: 617
Require email on the add-groupsio-member endpoint.
Since GroupsioMemberRequestType is shared with update, this method currently only requires subgroup_id, allowing an empty create payload to pass generated validation. Add email as a method-level requirement here.
💡 Proposed fix
dsl.Payload(func() {
BearerTokenAttribute()
dsl.Attribute("subgroup_id", dsl.String, "Subgroup ID")
dsl.Extend(GroupsioMemberRequestType)
- dsl.Required("subgroup_id")
+ dsl.Required("subgroup_id", "email")
dsl.Token("bearer_token", dsl.String)
})📝 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.
Suggested change
| dsl.Payload(func() { | |
| BearerTokenAttribute() | |
| dsl.Attribute("subgroup_id", dsl.String, "Subgroup ID") | |
| dsl.Extend(GroupsioMemberRequestType) | |
| dsl.Required("subgroup_id") | |
| dsl.Token("bearer_token", dsl.String) | |
| dsl.Payload(func() { | |
| BearerTokenAttribute() | |
| dsl.Attribute("subgroup_id", dsl.String, "Subgroup ID") | |
| dsl.Extend(GroupsioMemberRequestType) | |
| dsl.Required("subgroup_id", "email") | |
| dsl.Token("bearer_token", dsl.String) | |
| }) |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@cmd/mailing-list-api/design/mailing_list.go` around lines 435 - 440, The
payload for the add-groupsio-member endpoint extends GroupsioMemberRequestType
but only marks "subgroup_id" as required, allowing an empty create payload;
update the dsl.Payload block (the one calling Extend(GroupsioMemberRequestType)
inside the add-groupsio-member method) to also require the "email" attribute by
adding it to the method-level required list (e.g., add "email" to the
dsl.Required call for this payload or include a second dsl.Required("email") so
the endpoint enforces email on create).
Comment on lines
+61
to
64
| // Initialize ITX proxy client | ||
| itxConfig := service.ITXProxyConfig() | ||
| itxClient := proxy.NewClient(itxConfig) | ||
|
|
There was a problem hiding this comment.
Validate ITX config before calling the panic-based client constructor.
proxy.NewClient panics on missing private-key/auth setup, so a bad deploy currently crashes straight out of startup rather than emitting a controlled error and exiting cleanly.
💡 Proposed guard
// Initialize ITX proxy client
itxConfig := service.ITXProxyConfig()
+ if itxConfig.BaseURL == "" || itxConfig.ClientID == "" || itxConfig.PrivateKey == "" || itxConfig.Auth0Domain == "" || itxConfig.Audience == "" {
+ slog.ErrorContext(ctx, "missing required ITX proxy configuration")
+ os.Exit(1)
+ }
itxClient := proxy.NewClient(itxConfig)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@cmd/mailing-list-api/main.go` around lines 61 - 64, The code calls
proxy.NewClient(itxConfig) which can panic on missing private-key/auth; before
invoking proxy.NewClient, validate the ITX config returned by
service.ITXProxyConfig() (check required fields such as private key, auth token,
endpoint or any bools the proxy client expects) and if validation fails log a
clear error via process or application logger and exit cleanly (os.Exit(1))
instead of allowing a panic; alternatively wrap proxy.NewClient(itxConfig) in a
defer/recover block to convert any panic into a logged error and controlled
exit—update the initialization site that uses service.ITXProxyConfig and
proxy.NewClient accordingly.
Comment on lines
+67
to
+89
| func IDMapper(ctx context.Context) domain.IDMapper { | ||
| if os.Getenv("ID_MAPPING_DISABLED") == "true" { | ||
| slog.WarnContext(ctx, "ID mapping is DISABLED - using no-op mapper (IDs will pass through unchanged)") | ||
| return idmapper.NewNoOpMapper() | ||
| } | ||
|
|
||
| switch repoSource { | ||
| case "mock": | ||
| slog.InfoContext(ctx, "initializing mock grpsio service reader") | ||
| grpsIOReader = infrastructure.NewMockGrpsIOReader(mockRepository(ctx)) | ||
|
|
||
| case "nats": | ||
| slog.InfoContext(ctx, "initializing NATS service") | ||
| natsClient := natsStorage(ctx) | ||
| if natsClient == nil { | ||
| log.Fatalf("failed to initialize NATS client") | ||
| } | ||
| grpsIOReader = natsClient | ||
|
|
||
| default: | ||
| log.Fatalf("unsupported service reader implementation: %s", repoSource) | ||
| } | ||
|
|
||
| return grpsIOReader | ||
| } | ||
|
|
||
| // GrpsIOReaderWriter initializes the service reader writer implementation | ||
| func GrpsIOReaderWriter(ctx context.Context) port.GrpsIOReaderWriter { | ||
| var storage port.GrpsIOReaderWriter | ||
| // Repository implementation configuration | ||
| repoSource := os.Getenv("REPOSITORY_SOURCE") | ||
| if repoSource == "" { | ||
| repoSource = "nats" | ||
| natsURL := os.Getenv("NATS_URL") | ||
| if natsURL == "" { | ||
| slog.WarnContext(ctx, "NATS_URL not set, using no-op ID mapper") | ||
| return idmapper.NewNoOpMapper() | ||
| } | ||
|
|
||
| switch repoSource { | ||
| case "mock": | ||
| slog.InfoContext(ctx, "initializing mock grpsio storage reader writer") | ||
| storage = infrastructure.NewMockGrpsIOReaderWriter(mockRepository(ctx)) | ||
|
|
||
| case "nats": | ||
| slog.InfoContext(ctx, "initializing NATS service") | ||
| natsClient := natsStorage(ctx) | ||
| if natsClient == nil { | ||
| log.Fatalf("failed to initialize NATS client") | ||
| } | ||
| storage = natsClient | ||
|
|
||
| default: | ||
| log.Fatalf("unsupported service reader implementation: %s", repoSource) | ||
| } | ||
|
|
||
| return storage | ||
| } | ||
|
|
||
| // GrpsIOWriter initializes the service writer implementation | ||
| func GrpsIOWriter(ctx context.Context) port.GrpsIOWriter { | ||
| var grpsIOWriter port.GrpsIOWriter | ||
|
|
||
| // Repository implementation configuration | ||
| repoSource := os.Getenv("REPOSITORY_SOURCE") | ||
| if repoSource == "" { | ||
| repoSource = "nats" | ||
| } | ||
|
|
||
| switch repoSource { | ||
| case "mock": | ||
| slog.InfoContext(ctx, "initializing mock grpsio service writer") | ||
| grpsIOWriter = infrastructure.NewMockGrpsIOWriter(mockRepository(ctx)) | ||
|
|
||
| case "nats": | ||
| slog.InfoContext(ctx, "initializing NATS service writer") | ||
| natsClient := natsStorage(ctx) | ||
| if natsClient == nil { | ||
| log.Fatalf("failed to initialize NATS client") | ||
| } | ||
| grpsIOWriter = natsClient | ||
|
|
||
| default: | ||
| log.Fatalf("unsupported service writer implementation: %s", repoSource) | ||
| } | ||
|
|
||
| return grpsIOWriter | ||
| } | ||
|
|
||
| // GrpsIOMemberRepository initializes the member repository implementation | ||
| func GrpsIOMemberRepository(ctx context.Context) port.GrpsIOMemberRepository { | ||
| var memberRepository port.GrpsIOMemberRepository | ||
|
|
||
| // Repository implementation configuration | ||
| repoSource := os.Getenv("REPOSITORY_SOURCE") | ||
| if repoSource == "" { | ||
| repoSource = "nats" | ||
| } | ||
|
|
||
| switch repoSource { | ||
| case "mock": | ||
| slog.InfoContext(ctx, "initializing mock grpsio member repository") | ||
| memberRepository = infrastructure.NewMockGrpsIOMemberRepository(mockRepository(ctx)) | ||
|
|
||
| case "nats": | ||
| slog.InfoContext(ctx, "initializing NATS member repository") | ||
| natsClient := natsStorage(ctx) | ||
| if natsClient == nil { | ||
| log.Fatalf("failed to initialize NATS client") | ||
| } | ||
| memberRepository = natsClient | ||
|
|
||
| default: | ||
| log.Fatalf("unsupported member repository implementation: %s", repoSource) | ||
| } | ||
|
|
||
| return memberRepository | ||
| } | ||
|
|
||
| // MessagePublisher initializes the service publisher implementation | ||
| func MessagePublisher(ctx context.Context) port.MessagePublisher { | ||
| var publisher port.MessagePublisher | ||
|
|
||
| // Repository implementation configuration | ||
| repoSource := os.Getenv("REPOSITORY_SOURCE") | ||
| if repoSource == "" { | ||
| repoSource = "nats" | ||
| } | ||
|
|
||
| switch repoSource { | ||
| case "mock": | ||
| slog.InfoContext(ctx, "initializing mock service publisher") | ||
| publisher = infrastructure.NewMockMessagePublisher() | ||
|
|
||
| case "nats": | ||
| slog.InfoContext(ctx, "initializing NATS service publisher") | ||
| natsPublisher := natsPublisher(ctx) | ||
| if natsPublisher == nil { | ||
| log.Fatalf("failed to initialize NATS publisher") | ||
| } | ||
| publisher = natsPublisher | ||
|
|
||
| default: | ||
| log.Fatalf("unsupported service publisher implementation: %s", repoSource) | ||
| } | ||
|
|
||
| return publisher | ||
| } | ||
|
|
||
| // GroupsIOClient initializes the GroupsIO client with singleton pattern | ||
| func GroupsIOClient(ctx context.Context) groupsio.ClientInterface { | ||
| groupsIOClientOnce.Do(func() { | ||
| var client groupsio.ClientInterface | ||
|
|
||
| // Repository implementation configuration | ||
| source := os.Getenv("GROUPSIO_SOURCE") | ||
| if source == "" { | ||
| source = "groupsio" // Default to production GroupsIO client | ||
| } | ||
|
|
||
| switch source { | ||
| case "mock": | ||
| slog.InfoContext(ctx, "initializing mock groupsio client") | ||
| client = infrastructure.NewMockGroupsIOClient() | ||
|
|
||
| case "groupsio": | ||
| slog.InfoContext(ctx, "initializing groupsio client") | ||
| config := groupsio.NewConfigFromEnv() | ||
|
|
||
| var err error | ||
| client, err = groupsio.NewClient(config) | ||
| if err != nil { | ||
| log.Fatalf("failed to initialize GroupsIO client - missing required configuration: %v", err) | ||
| } | ||
| slog.InfoContext(ctx, "groupsio client initialized successfully") | ||
|
|
||
| default: | ||
| log.Fatalf("unsupported groupsio client implementation: %s", source) | ||
| } | ||
|
|
||
| groupsIOClient = client | ||
| natsMapper, err := idmapper.NewNATSMapper(idmapper.Config{ | ||
| URL: natsURL, | ||
| Timeout: 5 * time.Second, | ||
| }) | ||
| if err != nil { | ||
| slog.With("error", err).WarnContext(ctx, "Failed to initialize NATS ID mapper, falling back to no-op mapper") | ||
| return idmapper.NewNoOpMapper() | ||
| } | ||
|
|
||
| return groupsIOClient | ||
| } | ||
|
|
||
| // GrpsIOReaderOrchestrator initializes the service reader orchestrator | ||
| func GrpsIOReaderOrchestrator(ctx context.Context) service.GrpsIOReader { | ||
| grpsIOReader := GrpsIOReader(ctx) | ||
|
|
||
| return service.NewGrpsIOReaderOrchestrator( | ||
| service.WithGrpsIOReader(grpsIOReader), | ||
| ) | ||
| slog.InfoContext(ctx, "ID mapping enabled - using NATS mapper for v1/v2 ID conversions") | ||
| return natsMapper |
There was a problem hiding this comment.
Don't silently downgrade to the no-op mapper when NATS is unavailable.
ID_MAPPING_DISABLED=true already gives you an explicit local-dev escape hatch. The NATS_URL == "" and NewNATSMapper failure branches still start the service with pass-through IDs even though the ITX-facing models use v1 SFIDs, so project/committee-based calls will be proxied with the wrong identifiers instead of failing fast.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@cmd/mailing-list-api/service/providers.go` around lines 67 - 89, The IDMapper
function currently falls back to idmapper.NewNoOpMapper on missing NATS_URL or
NewNATSMapper errors which silently allows wrong pass-through IDs; instead make
it fail fast: in IDMapper, after checking ID_MAPPING_DISABLED, if NATS_URL is
empty or idmapper.NewNATSMapper returns an error, log the error with context
(include the env var and the error) and terminate startup (e.g.
slog.Error/With(...).Fatal or call os.Exit(1)) so the service does not run with
idmapper.NewNoOpMapper; keep idmapper.NewNoOpMapper only for the explicit
ID_MAPPING_DISABLED=true branch. Reference: IDMapper, idmapper.NewNATSMapper,
idmapper.NewNoOpMapper, ID_MAPPING_DISABLED and NATS_URL.
Comment on lines
+19
to
+65
| // DomainError represents an error with semantic type information | ||
| type DomainError struct { | ||
| Type ErrorType | ||
| Message string | ||
| Err error // underlying error for wrapping | ||
| } | ||
|
|
||
| func (e *DomainError) Error() string { | ||
| if e.Err != nil { | ||
| return e.Message + ": " + e.Err.Error() | ||
| } | ||
| return e.Message | ||
| } | ||
|
|
||
| func (e *DomainError) Unwrap() error { | ||
| return e.Err | ||
| } | ||
|
|
||
| // GetErrorType returns the semantic type of an error | ||
| func GetErrorType(err error) ErrorType { | ||
| var domainErr *DomainError | ||
| if errors.As(err, &domainErr) { | ||
| return domainErr.Type | ||
| } | ||
| return ErrorTypeInternal // default fallback | ||
| } | ||
|
|
||
| // Error constructors for different types | ||
| func NewValidationError(message string, err ...error) *DomainError { | ||
| return &DomainError{Type: ErrorTypeValidation, Message: message, Err: errors.Join(err...)} | ||
| } | ||
|
|
||
| func NewNotFoundError(message string, err ...error) *DomainError { | ||
| return &DomainError{Type: ErrorTypeNotFound, Message: message, Err: errors.Join(err...)} | ||
| } | ||
|
|
||
| func NewConflictError(message string, err ...error) *DomainError { | ||
| return &DomainError{Type: ErrorTypeConflict, Message: message, Err: errors.Join(err...)} | ||
| } | ||
|
|
||
| func NewInternalError(message string, err ...error) *DomainError { | ||
| return &DomainError{Type: ErrorTypeInternal, Message: message, Err: errors.Join(err...)} | ||
| } | ||
|
|
||
| func NewUnavailableError(message string, err ...error) *DomainError { | ||
| return &DomainError{Type: ErrorTypeUnavailable, Message: message, Err: errors.Join(err...)} | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Reuse the shared pkg/errors package instead of introducing DomainError.
This adds a second error taxonomy to the codebase, so downstream layers now need to translate both pkg/errors and internal/domain wrappers. Please model these semantic cases on top of the existing shared error package instead of adding a parallel set of constructors here.
As per coding guidelines, "**/*.go: Use custom error types from pkg/errors/ (NewServiceUnavailable, NewUnexpected) for error handling".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@internal/domain/errors.go` around lines 19 - 65, The DomainError type and its
constructors (DomainError, NewValidationError, NewNotFoundError,
NewConflictError, NewInternalError, NewUnavailableError) should be removed and
replaced with the shared pkg/errors usage: change each constructor to call the
corresponding pkg/errors factory (e.g., pkg/errors.NewValidation / NewNotFound /
NewConflict / NewUnexpected or NewServiceUnavailable as appropriate) and return
the pkg/errors error type; update GetErrorType to detect and map the pkg/errors
semantic types (use errors.As against the pkg/errors types or their exported
helpers) instead of asserting *DomainError; ensure Unwrap/Error behavior relies
on the pkg/errors implementation so no custom DomainError remains.
Comment on lines
+138
to
+143
| func TestNATSMapper_DefaultTimeout(t *testing.T) { | ||
| // Verify that zero timeout uses the default | ||
| m := &NATSMapper{conn: nil, timeout: 0} | ||
| // A zero timeout would be unusual but shouldn't panic at construction | ||
| assert.Equal(t, time.Duration(0), m.timeout) | ||
| } |
There was a problem hiding this comment.
Test doesn't verify default timeout behavior.
This test bypasses NewNATSMapper and directly constructs a NATSMapper struct with timeout: 0. Per the implementation in nats_mapper.go (lines 42-45), the default timeout is applied in NewNATSMapper, not in the struct itself.
The test verifies that a zero timeout doesn't panic, but the comment suggests it should verify default timeout behavior. To actually test the default, you'd need to call NewNATSMapper with Timeout: 0 and verify m.timeout == defaultTimeout.
🔧 Suggested fix
func TestNATSMapper_DefaultTimeout(t *testing.T) {
- // Verify that zero timeout uses the default
- m := &NATSMapper{conn: nil, timeout: 0}
- // A zero timeout would be unusual but shouldn't panic at construction
- assert.Equal(t, time.Duration(0), m.timeout)
+ // Note: Default timeout is applied in NewNATSMapper, which requires a valid NATS URL.
+ // This test documents that directly constructing NATSMapper with zero timeout
+ // does not panic, but doesn't test the default timeout fallback in NewNATSMapper.
+ m := &NATSMapper{conn: nil, timeout: 0}
+ assert.Equal(t, time.Duration(0), m.timeout)
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@internal/infrastructure/idmapper/nats_mapper_test.go` around lines 138 - 143,
The test currently constructs NATSMapper directly and doesn't exercise the
defaulting logic in NewNATSMapper; change the test to call NewNATSMapper with an
option/config that sets Timeout: 0 (so NewNATSMapper runs its defaulting code)
and then assert that the returned mapper's timeout field equals defaultTimeout;
also keep an assertion that no error/panic occurred during NewNATSMapper
creation. Reference NewNATSMapper, NATSMapper, and defaultTimeout when locating
the code to change.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Removes the NATS KV storage backend, direct Groups.io API client, committee sync, webhook processing, and mailing list sync in favour of a thin proxy that forwards all GroupsIO operations to the ITX API (https://api.prod.itx.linuxfoundation.org).
Key changes: