feat: add lifecycle states for config-only resources#2081
Merged
Conversation
…tions Add DataSourceStatus, ConnectionStatus, and RouteStatus enums with ACTIVE/DEPRECATED states. Add DeprecateDataSource, DeprecateConnection, and DeprecateRoute RPCs for config-only resource lifecycle management.
Add status (VARCHAR DEFAULT 'ACTIVE') and deprecated_at columns to data_source, provider_connections, and instruction_routes tables with CHECK constraints and tenant-scoped indexes.
Add ACTIVE/DEPRECATED status to DataSource domain model with Deprecate() method, update persistence layer for status column, and add DeprecateDataSource gRPC handler with idempotent deprecation semantics.
Add ACTIVE/DEPRECATED status to ProviderConnection and InstructionRoute domain models with Deprecate() methods. Update persistence entities, repositories, and gRPC handlers with DeprecateConnection and DeprecateRoute endpoints. Update test DDL for new columns.
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds deprecation lifecycle support across market-information and operational-gateway: new status enums and deprecated timestamps, proto RPCs for deprecation, domain deprecate methods/errors, repository deprecate operations, persistence schema/migrations, mappers, and service RPC handlers. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant API as gRPC Service
participant Domain
participant Repo
participant DB
Client->>API: Deprecate*Request(identifier)
API->>Repo: FindBy...(identifier)
Repo->>DB: SELECT ... WHERE identifier=?
DB-->>Repo: row (status=ACTIVE)
Repo-->>API: entity -> domain
API->>Domain: call Deprecate()
Domain-->>API: success / ErrNotActive
API->>Repo: Deprecate(identifier) (txn)
Repo->>DB: UPDATE ... SET status='DEPRECATED', deprecated_at=NOW() WHERE ... AND status='ACTIVE'
DB-->>Repo: result (rows affected)
Repo-->>API: success
API->>Repo: FindBy...(identifier)
Repo->>DB: SELECT ...
DB-->>Repo: row (status=DEPRECATED)
Repo-->>API: updated entity/domain
API-->>Client: Deprecate*Response(updated resource proto)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes 🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
Update generated TypeScript protobuf files for new lifecycle status enums and deprecation RPCs added to market-information and operational-gateway protos.
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
Claude Code ReviewCommit: `c3eaa1d` | CI: running (all checks pending except migration checksums: pass) SummaryWell-structured PR adding ACTIVE/DEPRECATED lifecycle states to three config-only resources across market-information and operational-gateway services. The operational-gateway implementation is clean — GORM-based persistence correctly includes both `status` and `deprecated_at` in entity mappings and upsert clauses. Domain models with idempotent `Deprecate()` methods are well-designed. However, the market-information service has two correctness bugs in the persistence/service layers that will cause production failures after migration. These were flagged in the previous review and remain unresolved. Risk Assessment
Findings
Bot Review NotesCodeRabbit on `market_information.proto:466` — Valid minor concern. Proto doc says "Returns FAILED_PRECONDITION if data source is not in ACTIVE status" but the repo `Deprecate()` method returns success (nil) when already DEPRECATED. The idempotent behavior is correct; the proto doc just needs to say "Returns FAILED_PRECONDITION if data source is not in ACTIVE or DEPRECATED status" or explicitly document idempotency. CodeRabbit on `setup_test.go:103` and `:158` — Valid minor concern. Test DDL for `provider_connections` and `instruction_routes` omits the `CHECK (status IN ('ACTIVE', 'DEPRECATED'))` constraints present in production migrations. Tests won't catch invalid status values at the DB layer. Worth adding for parity but not a blocker. CodeRabbit on `schema.sql:21` — Marginal. Missing `idx_data_source_status` index in test schema. Indexes don't affect correctness in tests. Not actionable. Questions for the Author
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
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 (3)
services/market-information/adapters/persistence/source_repository.go (2)
37-67:⚠️ Potential issue | 🟠 Major
deprecated_atnever reaches the database inSave.
DataSourceToEntitynow carriesentity.DeprecatedAt, but this upsert never inserts or binds that column. Any laterSaveof a deprecated source will drop its timestamp back toNULL, so the new field is not round-trippable.💾 Include `deprecated_at` in the upsert payload
query := ` INSERT INTO data_source ( - id, code, name, description, trust_level, status, - created_at, created_by, updated_at, updated_by, version + id, code, name, description, trust_level, status, deprecated_at, + created_at, created_by, updated_at, updated_by, version ) VALUES ( - $1, $2, $3, $4, $5, $6, - $7, $8, $9, $10, 1 + $1, $2, $3, $4, $5, $6, $7, + $8, $9, $10, $11, 1 ) ON CONFLICT (code) DO UPDATE SET name = EXCLUDED.name, description = EXCLUDED.description, trust_level = EXCLUDED.trust_level, @@ _, err := tx.Exec(ctx, query, entity.ID, entity.Code, entity.Name, entity.Description, entity.TrustLevel, entity.Status, + entity.DeprecatedAt, entity.CreatedAt, userID, entity.UpdatedAt, userID, )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@services/market-information/adapters/persistence/source_repository.go` around lines 37 - 67, The upsert in Save (in source_repository.go) omits entity.DeprecatedAt from the INSERT payload so deprecated_at never gets persisted; update the SQL INSERT column list and VALUES placeholders to include deprecated_at, and add entity.DeprecatedAt to the tx.Exec argument list (adjusting parameter order to match the new placeholder index) so DataSourceToEntity's DeprecatedAt is persisted and round-trips correctly (the ON CONFLICT already assigns deprecated_at = EXCLUDED.deprecated_at).
233-249:⚠️ Potential issue | 🟠 MajorHonor
activeOnlyin both list queries.
api/proto/meridian/market_information/v1/market_information.protosaysactive_onlyshould return only active data sources, but these queries still filter onlydeleted_at. After this change,List(..., true, ...)will keep returningstatus='DEPRECATED'rows.📋 Thread `activeOnly` into the SQL builder
-func (r *SourceRepository) List(ctx context.Context, _ bool, pageSize int, pageToken string) ([]domain.DataSource, string, error) { +func (r *SourceRepository) List(ctx context.Context, activeOnly bool, pageSize int, pageToken string) ([]domain.DataSource, string, error) { @@ - query, args := buildSourceListQuery(cursorTime, cursorID, pageSize) + query, args := buildSourceListQuery(activeOnly, cursorTime, cursorID, pageSize) @@ -func buildSourceListQuery(cursorTime time.Time, cursorID uuid.UUID, pageSize int) (string, []interface{}) { +func buildSourceListQuery(activeOnly bool, cursorTime time.Time, cursorID uuid.UUID, pageSize int) (string, []interface{}) { + whereClause := "WHERE deleted_at IS NULL" + if activeOnly { + whereClause += " AND status = 'ACTIVE'" + } + if cursorTime.IsZero() { return ` SELECT id, code, name, description, trust_level, status, created_at, updated_at, deprecated_at, version FROM data_source - WHERE deleted_at IS NULL + ` + whereClause + ` ORDER BY date_trunc('second', created_at) DESC, id DESC LIMIT $1`, []interface{}{pageSize + 1} } return ` SELECT id, code, name, description, trust_level, status, created_at, updated_at, deprecated_at, version FROM data_source - WHERE deleted_at IS NULL + ` + whereClause + ` AND ( date_trunc('second', created_at) < $1 OR (date_trunc('second', created_at) = $1 AND id < $2) )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@services/market-information/adapters/persistence/source_repository.go` around lines 233 - 249, The list SQL currently only filters deleted_at but ignores the activeOnly flag; update the two query branches in source_repository.go (the no-cursor and cursor-returning queries built by the list function) to add "AND status = 'ACTIVE'" when the activeOnly boolean is true, thread activeOnly into the SQL builder so the WHERE clause is conditionally extended, and adjust the parameter list/order (and the $n positional placeholders) to include no extra params for the simple branch or to include activeOnly only if used; ensure the queries still use cursorTime, cursorID, and pageSize + 1 variables (and update $1/$2/$3 indices accordingly) when building the cursor query.services/market-information/adapters/persistence/mappers.go (1)
113-130:⚠️ Potential issue | 🔴 Critical
Statusis now required, but existing builder call sites still omit it.
DataSourceToEntitynow always persistss.Status(). Inservices/market-information/service/source_service.go, the builders starting at Line 83 and Line 143 still never callWithStatus(...). That makesUpdateDataSourcewritestatus=""and hitchk_data_source_status, andDeactivateDataSourcenow returnsDATA_SOURCE_STATUS_UNSPECIFIED.🔧 Preserve lifecycle fields when rebuilding a source
builder := domain.NewDataSourceBuilder(). WithID(existing.ID()). WithCode(existing.Code()). WithSourceType(existing.SourceType()). WithIsActive(existing.IsActive()). + WithStatus(existing.Status()). WithCreatedAt(existing.CreatedAt()). WithUpdatedAt(time.Now()) +if existing.DeprecatedAt() != nil { + builder.WithDeprecatedAt(existing.DeprecatedAt()) +}Apply the same preservation in
DeactivateDataSource.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@services/market-information/adapters/persistence/mappers.go` around lines 113 - 130, The DataSourceToEntity mapper now always writes s.Status(), but the builder usages in services/market-information/service/source_service.go (used by UpdateDataSource and DeactivateDataSource) don’t set Status, so updates write an empty status and validations fail; fix the builders to preserve lifecycle fields when rebuilding sources by passing the existing entity's status into the builder (call WithStatus(existing.Status()) or the domain object's Status()) and also preserve other lifecycle fields the mapper expects (Description, DeprecatedAt, CreatedAt, UpdatedAt, Version) when constructing the new domain/DataSource before calling DataSourceToEntity; apply the same preservation logic in DeactivateDataSource so it sets Status (and DeprecatedAt) correctly instead of leaving it unspecified.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@api/proto/meridian/market_information/v1/market_information.proto`:
- Around line 457-466: Update the DeprecateDataSource RPC comment to document
idempotency: state that calling DeprecateDataSource transitions a data source
from ACTIVE to DEPRECATED, returns NOT_FOUND if missing, and is idempotent — it
returns success when the source is already DEPRECATED (instead of
FAILED_PRECONDITION); only return FAILED_PRECONDITION for statuses that are
neither ACTIVE nor DEPRECATED. Reference the RPC name DeprecateDataSource in the
proto and the persistence behavior in
services/market-information/adapters/persistence/source_repository.go so
generated OpenAPI/SDK docs reflect the actual behavior.
In `@services/market-information/domain/repository.go`:
- Around line 170-174: The mockSourceRepository needs to implement the new
Deprecate(ctx context.Context, code string) error method to satisfy the
SourceRepository interface; add a Deprecate method on mockSourceRepository with
the same signature, returning nil or the appropriate test error (e.g.,
ErrDataSourceNotFound / ErrDataSourceNotActive) based on the mock's configured
state so existing compile-time assertions and tests continue to pass; ensure the
method uses the mock's existing fields/behaviors (the same patterns as other
mock methods like Create/Update) so tests can simulate success and failure
cases.
In `@services/operational-gateway/adapters/persistence/setup_test.go`:
- Around line 157-158: The test DDL for the instruction_routes table is missing
the CHECK constraint on the status column; update the test schema in
setup_test.go so the instruction_routes definition includes the same constraint
as production (e.g., status VARCHAR(20) NOT NULL DEFAULT 'ACTIVE' CHECK (status
IN ('ACTIVE','DEPRECATED'))), ensuring the status column and its default remain
unchanged but add the CHECK clause to enforce valid values during tests.
- Around line 102-103: Test DDL for table provider_connections is missing the
CHECK constraint on the status column; update the DDL in setup_test.go where
provider_connections is defined to add the same constraint as production (e.g.,
"CHECK (status IN ('ACTIVE','DEPRECATED'))") on the status VARCHAR(20) NOT NULL
DEFAULT 'ACTIVE' column so tests enforce the same valid values as the migration.
---
Outside diff comments:
In `@services/market-information/adapters/persistence/mappers.go`:
- Around line 113-130: The DataSourceToEntity mapper now always writes
s.Status(), but the builder usages in
services/market-information/service/source_service.go (used by UpdateDataSource
and DeactivateDataSource) don’t set Status, so updates write an empty status and
validations fail; fix the builders to preserve lifecycle fields when rebuilding
sources by passing the existing entity's status into the builder (call
WithStatus(existing.Status()) or the domain object's Status()) and also preserve
other lifecycle fields the mapper expects (Description, DeprecatedAt, CreatedAt,
UpdatedAt, Version) when constructing the new domain/DataSource before calling
DataSourceToEntity; apply the same preservation logic in DeactivateDataSource so
it sets Status (and DeprecatedAt) correctly instead of leaving it unspecified.
In `@services/market-information/adapters/persistence/source_repository.go`:
- Around line 37-67: The upsert in Save (in source_repository.go) omits
entity.DeprecatedAt from the INSERT payload so deprecated_at never gets
persisted; update the SQL INSERT column list and VALUES placeholders to include
deprecated_at, and add entity.DeprecatedAt to the tx.Exec argument list
(adjusting parameter order to match the new placeholder index) so
DataSourceToEntity's DeprecatedAt is persisted and round-trips correctly (the ON
CONFLICT already assigns deprecated_at = EXCLUDED.deprecated_at).
- Around line 233-249: The list SQL currently only filters deleted_at but
ignores the activeOnly flag; update the two query branches in
source_repository.go (the no-cursor and cursor-returning queries built by the
list function) to add "AND status = 'ACTIVE'" when the activeOnly boolean is
true, thread activeOnly into the SQL builder so the WHERE clause is
conditionally extended, and adjust the parameter list/order (and the $n
positional placeholders) to include no extra params for the simple branch or to
include activeOnly only if used; ensure the queries still use cursorTime,
cursorID, and pageSize + 1 variables (and update $1/$2/$3 indices accordingly)
when building the cursor query.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: f496463d-2978-4a2c-9d8a-b294ac2ddfbc
⛔ Files ignored due to path filters (8)
api/proto/meridian/market_information/v1/market_information.pb.gois excluded by!**/*.pb.go,!**/*.pb.goapi/proto/meridian/market_information/v1/market_information_grpc.pb.gois excluded by!**/*.pb.go,!**/*.pb.go,!**/*_grpc.pb.goapi/proto/meridian/operational_gateway/v1/operational_gateway.pb.gois excluded by!**/*.pb.go,!**/*.pb.goapi/proto/meridian/operational_gateway/v1/operational_gateway_grpc.pb.gois excluded by!**/*.pb.go,!**/*.pb.go,!**/*_grpc.pb.gofrontend/src/api/gen/meridian/market_information/v1/market_information_pb.tsis excluded by!**/gen/**frontend/src/api/gen/meridian/operational_gateway/v1/operational_gateway_pb.tsis excluded by!**/gen/**services/market-information/migrations/atlas.sumis excluded by!**/*.sumservices/operational-gateway/migrations/atlas.sumis excluded by!**/*.sum
📒 Files selected for processing (21)
api/proto/meridian/market_information/v1/market_information.protoapi/proto/meridian/operational_gateway/v1/operational_gateway.protoservices/market-information/adapters/persistence/entities.goservices/market-information/adapters/persistence/mappers.goservices/market-information/adapters/persistence/source_repository.goservices/market-information/domain/data_source.goservices/market-information/domain/repository.goservices/market-information/migrations/20260401000001_add_data_source_status.sqlservices/market-information/service/source_service.goservices/operational-gateway/adapters/persistence/connection_entity.goservices/operational-gateway/adapters/persistence/connection_repository.goservices/operational-gateway/adapters/persistence/mappers.goservices/operational-gateway/adapters/persistence/route_entity.goservices/operational-gateway/adapters/persistence/route_repository.goservices/operational-gateway/adapters/persistence/setup_test.goservices/operational-gateway/domain/instruction_route.goservices/operational-gateway/domain/provider_connection.goservices/operational-gateway/migrations/20260401000001_add_connection_route_status.sqlservices/operational-gateway/service/grpc_connection_service.goservices/operational-gateway/service/grpc_mappers.goservices/operational-gateway/service/route_server.go
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
The mock did not implement the new Deprecate interface method, causing golangci-lint typecheck failures.
The generated MarketInformationServiceClient interface gained the new DeprecateDataSource RPC method, requiring mock implementations to match.
Replace string literals with domain type constants in parseDataSetStatus and parseDataSourceStatus to avoid repeated string occurrences.
Lifecycle deprecation is a standard operation for config-only resources, alongside Create/Find/Update/Delete.
The market-information test helper schema.sql was missing the new lifecycle columns, causing integration test failures.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@services/market-information/adapters/persistence/testhelpers/schema.sql`:
- Around line 16-21: The test schema is missing the index for data_source.status
causing drift; add the same index as production by creating
idx_data_source_status on the data_source table for the status column (match the
index name used in migrations: idx_data_source_status) so the test DDL mirrors
services/market-information/migrations/20260401000001_add_data_source_status.sql;
update the schema definition near the data_source CREATE TABLE block to include
the index creation for status.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 366f457d-abb7-4881-91c9-83f24fe28809
📒 Files selected for processing (2)
services/market-information/adapters/persistence/testhelpers/schema.sqltests/architecture/naming_test.go
The createSchemaDataSourceTable function for multi-tenant test schemas was missing the new columns, causing column mismatch when copying data sources from the public schema via SELECT *.
There was a problem hiding this comment.
Two critical correctness bugs remain in market-information:
- source_repository.go:51 - deprecated_at missing from INSERT (see inline)
- source_service.go:89-105 (not in diff) - UpdateDataSource builder omits WithStatus() and WithDeprecatedAt(). Zero-value DataSourceStatus is empty string, violating the new CHECK constraint. Every UpdateDataSource call will fail post-migration. Fix: add WithStatus(existing.Status()) and WithDeprecatedAt(existing.DeprecatedAt()) to the builder chain.
See summary comment for full details including bot thread assessments.
Critical fixes: - Add deprecated_at to INSERT column list in Save (was missing, causing EXCLUDED.deprecated_at to always be NULL) - Preserve status and deprecated_at in UpdateDataSource and DeactivateDataSource builders (zero-value status violated CHECK constraint) - Add CHECK constraints to operational-gateway test DDL for parity - Add idx_data_source_status to test schema to match production - Update proto comment to document idempotent deprecation behavior
Use WithDeprecatedAt directly in builder chain instead of conditional block, reducing function size below 60-line threshold.
…ifest-apply--10--lifecycle-states
bjcoombs
dismissed stale reviews from claude[bot] and coderabbitai[bot]
Stale bot review - all findings addressed
bjcoombs
dismissed stale reviews from claude[bot], claude[bot], claude[bot], coderabbitai[bot], claude[bot], and claude[bot]
Stale bot review - all findings addressed
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
1 participant
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.
Summary
Details
Config-only resources (DataSource, ProviderConnection, InstructionRoute) previously had no lifecycle management. This PR adds proper ACTIVE/DEPRECATED states to enable deprecation instead of deletion, supporting PRD-057's convergent manifest apply semantics.
Design decisions:
is_activeboolean derives from newstatusfieldhealth_status(operational) remains separate fromstatus(lifecycle)Changes per service:
Market Information:
Operational Gateway:
Test plan