Repository navigation
refactor(common): make DatabaseDetails/CacheDetails pointer-only - #1525
Conversation
cristim
added
priority/p3
|
Note Currently processing new changes in this PR. This may take a few minutes, please wait... ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (21)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
Adversarial review of #1525's pointer-only collapse found the invariant ("every producer constructs DatabaseDetails/CacheDetails as a pointer") was violated in providers/aws/services/memorydb/client_test.go, which built value-typed common.CacheDetails{} literals for Recommendation.Details. Value receivers on CacheDetails let this compile silently (both T and *T satisfy ServiceDetails), so nothing caught the mismatch at build time. The memorydb client doesn't currently type-switch on Details, so this was inert rather than a live panic/misdispatch, but it violated the invariant the refactor exists to establish and would silently break the moment any consumer started asserting *CacheDetails. Switch all ten fixtures to the pointer form to match every other producer in the codebase.
688d7ed to
9d6a694
Compare
|
@coderabbitai full review |
✅ Action performedFull review finished. Your included review limit is currently reached under our Fair Usage Limits Policy. Your recent PR review activity is in the 95th percentile or higher among CodeRabbit users, so adaptive limits apply. This review may still proceed through usage-based billing if eligible. Your next included review will be available in 46 minutes. |
Adversarial review of the pointer-only collapseReviewed this PR specifically for the failure mode a value-to-pointer change on a Verification performedSwept every module (root,
Findings fixed1. Comment names GCP as the only value-typed 2. 3. MemoryDB test fixtures still built value-typed 4. Typed-nil Not reachable from today's producers, but the sibling helpers already guard exactly Dismissed with justification
Verification
Note: CodeRabbit hit its fair-usage limit before reviewing this PR at all, so this is a @coderabbitai full review |
|
✅ Action performedFull review finished. Your included review limit is currently reached under our Fair Usage Limits Policy. Your recent PR review activity is in the 95th percentile or higher among CodeRabbit users, so adaptive limits apply. This review may still proceed through usage-based billing if eligible. Your next included review will be available in 39 minutes. |
Azure's SQL and Cache reservation converters were the only ServiceDetails producers in the codebase that populated Recommendation.Details with a value struct instead of a pointer. Every other producer (AWS, the CSV loader, the JSON codec used on the purchase-execution round trip) already constructs a pointer, so this was pure inconsistency rather than an intentional variant. Align Azure with the rest of the codebase so DatabaseDetails/ CacheDetails are always pointers regardless of provider or code path. Consumers still handle both forms defensively; that cleanup follows in the next commit.
…cases Every producer of Recommendation.Details now constructs DatabaseDetails/ CacheDetails as a pointer (previous commit), matching what the JSON codec has always reconstructed on the purchase-execution round trip. The value-typed cases in the type switches that dispatched on Details were the only thing papering over the old inconsistency; they are now unreachable. Simplify the switches in getEngineFromRecommendation (cmd/helpers.go), extractEngineLabel (cmd/main.go), extractDeployment/extractEngine (cmd/multi_service_csv.go), adjustRecommendationForExcludedVersions (cmd/multi_service_engine_versions.go), extractEngine (internal/scheduler/scheduler.go), EngineFromDetails (pkg/common/engine.go), and rdsEngineDeploymentFromRec (providers/aws/recommendations/coverage.go) to only handle the pointer form. Two switches collapsed to a single case; rewrote those as plain type assertions per gocritic's singleCaseSwitch. Also corrects stale comments on a few of these functions that claimed "the CSV-loader path constructs values" -- the CSV loader has constructed pointers for a while; the real (now-fixed) source of value-typed Details was the Azure SQL/Cache converters. ComputeDetails is untouched: the GCP compute-engine client still constructs it as a value, so consumers that handle ComputeDetails keep accepting both forms.
…tails Update test fixtures across cmd and internal/scheduler to construct DatabaseDetails/CacheDetails as pointers, matching the new pointer-only invariant. Removes the "value type" test cases in TestGetEngineFromRecommendation, TestExtractDeployment, and TestExtractEngine that existed solely to cover the now-removed value-typed dispatch branches; the pointer-typed cases already in each table give equivalent coverage of the real (only) code path.
Adversarial review of #1525's pointer-only collapse found the invariant ("every producer constructs DatabaseDetails/CacheDetails as a pointer") was violated in providers/aws/services/memorydb/client_test.go, which built value-typed common.CacheDetails{} literals for Recommendation.Details. Value receivers on CacheDetails let this compile silently (both T and *T satisfy ServiceDetails), so nothing caught the mismatch at build time. The memorydb client doesn't currently type-switch on Details, so this was inert rather than a live panic/misdispatch, but it violated the invariant the refactor exists to establish and would silently break the moment any consumer started asserting *CacheDetails. Switch all ten fixtures to the pointer form to match every other producer in the codebase.
The `details == nil` check on ServiceDetails only catches an untyped nil; a (*DatabaseDetails)(nil) or (*CacheDetails)(nil) stored in the interface is non-nil at that check and reaches the type switch, where a bare field read would panic. Every dispatch site already guards each case with its own nil check, but the guard read as plausible dead code without a comment explaining why, and none of the call sites had a regression test pinning the behavior. Add TestEngineFromDetails (new, common package had no test for this function), TestExtractEngine in scheduler, and typed-nil cases to the existing TestGetEngineFromRecommendation / TestExtractEngine (cmd) / TestAdjustRecommendationForExcludedVersions_TypedNilDetails suites. Also correct the ComputeDetails doc comment: the Azure compute client constructs it as a value alongside the GCP compute-engine client, not GCP alone.
9d6a694 to
1ab2ca1
Compare
|
Rebased onto
Re-verified locally on the rebased tree: Note for the record: CodeRabbit has not yet produced a substantive review of this PR. Its walkthrough comment still carries the @coderabbitai full review |
|
✅ Action performedFull review finished. Your included review limit is currently reached under our Fair Usage Limits Policy. Your recent PR review activity is in the 95th percentile or higher among CodeRabbit users, so adaptive limits apply. This review may still proceed through usage-based billing if eligible. Your next included review will be available in 2 seconds. |
|
Caution Failed to replace (edit) comment. This is likely due to insufficient permissions or the comment being deleted. Error details |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
internal/scheduler/scheduler.go (1)
1247-1269: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚖️ Poor tradeoffSplit these oversized Go files before extending them further.
internal/scheduler/scheduler.go#L1247-L1269: extract focused scheduler helpers into an existing appropriate package/file.internal/scheduler/scheduler_test.go#L2346-L2380: move focused helper tests into dedicated existing test files.cmd/multi_service_csv_test.go#L526-L541: split CSV extraction tests by behavior.cmd/main_test.go#L417-L419: split purchase-ID tests into focused test files.cmd/multi_service_coverage_test.go#L742-L745: split coverage/engine-version tests by concern.As per coding guidelines, “keep files under 500 lines.”
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/scheduler/scheduler.go` around lines 1247 - 1269, Split the oversized files into focused existing packages or test files, keeping each file under 500 lines: move extractEngine and related scheduler helpers from internal/scheduler/scheduler.go:1247-1269; move the focused helper tests from internal/scheduler/scheduler_test.go:2346-2380; separate CSV extraction behavior tests in cmd/multi_service_csv_test.go:526-541; move purchase-ID tests from cmd/main_test.go:417-419 into focused test files; and separate coverage tests from engine-version tests in cmd/multi_service_coverage_test.go:742-745, preserving behavior and updating package-local references as needed.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@internal/scheduler/scheduler.go`:
- Around line 1247-1269: Split the oversized files into focused existing
packages or test files, keeping each file under 500 lines: move extractEngine
and related scheduler helpers from internal/scheduler/scheduler.go:1247-1269;
move the focused helper tests from
internal/scheduler/scheduler_test.go:2346-2380; separate CSV extraction behavior
tests in cmd/multi_service_csv_test.go:526-541; move purchase-ID tests from
cmd/main_test.go:417-419 into focused test files; and separate coverage tests
from engine-version tests in cmd/multi_service_coverage_test.go:742-745,
preserving behavior and updating package-local references as needed.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 3d73d217-7670-4415-9642-42984e3d810e
📒 Files selected for processing (21)
cmd/helpers.gocmd/helpers_test.gocmd/main.gocmd/main_test.gocmd/multi_service_coverage_test.gocmd/multi_service_csv.gocmd/multi_service_csv_test.gocmd/multi_service_engine_versions.gocmd/multi_service_engine_versions_test.gocmd/multi_service_test.gointernal/scheduler/scheduler.gointernal/scheduler/scheduler_test.gopkg/common/engine.gopkg/common/engine_test.gopkg/common/service_details_codec.goproviders/aws/recommendations/coverage.goproviders/aws/services/memorydb/client_test.goproviders/azure/services/cache/client.goproviders/azure/services/cache/client_test.goproviders/azure/services/database/client.goproviders/azure/services/database/client_test.go
* fix(azure): construct DatabaseDetails/CacheDetails as pointers Azure's SQL and Cache reservation converters were the only ServiceDetails producers in the codebase that populated Recommendation.Details with a value struct instead of a pointer. Every other producer (AWS, the CSV loader, the JSON codec used on the purchase-execution round trip) already constructs a pointer, so this was pure inconsistency rather than an intentional variant. Align Azure with the rest of the codebase so DatabaseDetails/ CacheDetails are always pointers regardless of provider or code path. Consumers still handle both forms defensively; that cleanup follows in the next commit. * refactor(common): drop dead value-typed DatabaseDetails/CacheDetails cases Every producer of Recommendation.Details now constructs DatabaseDetails/ CacheDetails as a pointer (previous commit), matching what the JSON codec has always reconstructed on the purchase-execution round trip. The value-typed cases in the type switches that dispatched on Details were the only thing papering over the old inconsistency; they are now unreachable. Simplify the switches in getEngineFromRecommendation (cmd/helpers.go), extractEngineLabel (cmd/main.go), extractDeployment/extractEngine (cmd/multi_service_csv.go), adjustRecommendationForExcludedVersions (cmd/multi_service_engine_versions.go), extractEngine (internal/scheduler/scheduler.go), EngineFromDetails (pkg/common/engine.go), and rdsEngineDeploymentFromRec (providers/aws/recommendations/coverage.go) to only handle the pointer form. Two switches collapsed to a single case; rewrote those as plain type assertions per gocritic's singleCaseSwitch. Also corrects stale comments on a few of these functions that claimed "the CSV-loader path constructs values" -- the CSV loader has constructed pointers for a while; the real (now-fixed) source of value-typed Details was the Azure SQL/Cache converters. ComputeDetails is untouched: the GCP compute-engine client still constructs it as a value, so consumers that handle ComputeDetails keep accepting both forms. * test(common): update fixtures to pointer-only DatabaseDetails/CacheDetails Update test fixtures across cmd and internal/scheduler to construct DatabaseDetails/CacheDetails as pointers, matching the new pointer-only invariant. Removes the "value type" test cases in TestGetEngineFromRecommendation, TestExtractDeployment, and TestExtractEngine that existed solely to cover the now-removed value-typed dispatch branches; the pointer-typed cases already in each table give equivalent coverage of the real (only) code path. * fix(aws): construct memorydb test fixtures' CacheDetails as pointers Adversarial review of #1525's pointer-only collapse found the invariant ("every producer constructs DatabaseDetails/CacheDetails as a pointer") was violated in providers/aws/services/memorydb/client_test.go, which built value-typed common.CacheDetails{} literals for Recommendation.Details. Value receivers on CacheDetails let this compile silently (both T and *T satisfy ServiceDetails), so nothing caught the mismatch at build time. The memorydb client doesn't currently type-switch on Details, so this was inert rather than a live panic/misdispatch, but it violated the invariant the refactor exists to establish and would silently break the moment any consumer started asserting *CacheDetails. Switch all ten fixtures to the pointer form to match every other producer in the codebase. * test(common): pin typed-nil safety regression bar for Details dispatch The `details == nil` check on ServiceDetails only catches an untyped nil; a (*DatabaseDetails)(nil) or (*CacheDetails)(nil) stored in the interface is non-nil at that check and reaches the type switch, where a bare field read would panic. Every dispatch site already guards each case with its own nil check, but the guard read as plausible dead code without a comment explaining why, and none of the call sites had a regression test pinning the behavior. Add TestEngineFromDetails (new, common package had no test for this function), TestExtractEngine in scheduler, and typed-nil cases to the existing TestGetEngineFromRecommendation / TestExtractEngine (cmd) / TestAdjustRecommendationForExcludedVersions_TypedNilDetails suites. Also correct the ComputeDetails doc comment: the Azure compute client constructs it as a value alongside the GCP compute-engine client, not GCP alone.
1 participant
Summary
Revives the valuable part of a stale, unshipped refactor found in worktree
agent-a2b3490bc2d3f0c08(~1 month old, never landed). That worktree bundled three ideas; after assessing each against currentmain, only one turned out to be a genuine win. This PR implements only that part, built fresh against currentmain(not built from the stale diff).What's in scope:
common.DatabaseDetails/common.CacheDetailswere the onlyServiceDetailsproducers in the codebase constructed inconsistently -- Azure's SQL/Redis reservation converters built them as plain values, while AWS, the CSV loader, and the JSON codec (used on the purchase-execution DB round trip) always built pointers. That inconsistency forced 8 separate consumer functions (incmd/,internal/scheduler,providers/aws/recommendations,pkg/common) to defensively type-switch on bothTand*Tfor the same two types.*DatabaseDetails/*CacheDetails.ComputeDetailsis intentionally untouched -- GCP's compute-engine client still constructs it as a value, so its dual-handling stays.What was in the stale worktree but is NOT in this PR (assessed as not worth reviving)
provider.ProviderConfig->provider.Configrename: real (revive stutter) finding, but ~96 occurrences across 18 files for a purely cosmetic rename. Also discovered while assessing this:golangci-lintin CI only lints the root Go module --pkg/,providers/aws,providers/azure,providers/gcp(separatego.workmodules) are never scanned by the "Lint Code" job, so this stutter finding isn't even CI-enforced today. High blast radius for a change with no functional or CI-visible payoff; declined.WriteAuditRecord(record AuditRecord)->(record *AuditRecord):AuditRecordis ~280 bytes, well under the project's deliberate 1024-bytegocritichugeParamthreshold (see.golangci.ymlcomment). No lint fires on it today and passing it by value is idiomatic Go at that size. Declined as zero-value churn.Follow-ups surfaced but out of scope here (not fixed, flagging per project convention)
ComputeDetails(GCP constructs a value; everyone else a pointer), with the same 2-branch dual-handling incmd/multi_service_csv.goandcmd/main.go. Could be collapsed the same way in a follow-up.cmd/helpers.go'sgetEngineFromRecommendation/normalizeEngineNameis a near-duplicate ofpkg/common/engine.go'sEngineFromDetails/NormalizeEngineName(independent of this refactor).golangci-lintstep only covers the root module;pkg/,providers/aws,providers/azure,providers/gcpcarry real pre-existing lint debt (godot, misspell, a couple ofrevivestutter findings, gosec is separately covered) that's currently invisible to the "Lint Code" job.Test plan
go build ./...clean in root,pkg/,providers/aws,providers/azure,providers/gcpmodulesgo vet ./...clean in all of the abovego test ./...green at the repo root (includescmd,internal/scheduler,internal/purchase, etc.)go test ./...green inpkg/,providers/aws,providers/azuregolangci-lint run --timeout=10m(pinned to CI'sv2.10.1) clean at repo root, matching the exact CI invocationgofmt -lclean on all touched files.golangci.ymlunchangedSummary by CodeRabbit
Bug Fixes
Tests