diff --git a/known_issues/10_azure_provider.md b/known_issues/10_azure_provider.md index 6a8c708f..6dd0c323 100644 --- a/known_issues/10_azure_provider.md +++ b/known_issues/10_azure_provider.md @@ -1,6 +1,6 @@ # Known Issues: Azure Provider -> **Audit status (2026-04-21):** `2 follow-ups from CRITICAL rewrite · 12 resolved · 0 partially fixed · 0 moved · 1 new follow-up surfaced (batched SKU enrichment — deferred N+1 avoidance)` +> **Audit status (2026-04-25):** `2 follow-ups from CRITICAL rewrite · 13 resolved · 0 partially fixed · 0 moved · 1 narrower follow-up surfaced (compute Details fields require pkg/common type extension)` ## ~~CRITICAL: Recommendation converters ignore the API response entirely~~ — RESOLVED @@ -300,13 +300,33 @@ Each converter's godoc now explicitly documents which fields are payload-derivab ### New follow-ups surfaced during this resolution -#### MEDIUM: Azure converters leave SKU-lookup Details fields empty (deferred N+1 avoidance) +#### ~~MEDIUM: Azure converters leave SKU-lookup Details fields empty (deferred N+1 avoidance)~~ — RESOLVED for 3 of 4 services -**File:** `providers/azure/services/{compute,database,cache,cosmosdb}/client.go::convertAzure*Recommendation` +**File:** `providers/azure/services/{database,cache,cosmosdb}/client.go::convertAzure*Recommendation` -**Description:** All four converters populate the subset of `Details` derivable from the reservation recommendation payload only. Fields that require an SDK catalogue lookup — compute Platform/Tenancy/Scope + memory/vCPU, database EngineVersion/AZConfig, cache Shards, cosmos APIType — are deliberately left empty because populating them inline would produce an N+1 API call per recommendation. +**Status:** ✔️ Resolved for cache/cosmos/database; compute scoped back (see follow-up below) -**Proposed fix:** single-shot batched SKU lookup per region per service (`armcompute.ResourceSKUsClient.ListByLocation(region)`, `armsql.SKUsClient.ListByServer`, `armredis.SKUsClient.ListByLocation`, `armcosmos.DatabaseAccountsClient.ListByResourceGroup`), called ONCE at the start of `GetRecommendations` for each region appearing in the response, cached in a `map[skuName]SKUCatalogueEntry`, then consulted as the converter builds each rec's Details. No N+1 storm, full Details population. +**Resolution:** Each of the three database/cache/cosmosdb service clients gained a lazy SKU catalogue cache (`sync.Once`-gated `map[string]`). The catalogue is fetched ONCE per client lifetime via the existing per-service pager interface — `armredis.Client.NewListBySubscriptionPager` for cache, `armsql.CapabilitiesClient.ListByLocation` for database, `armcosmos.DatabaseAccountsClient.NewListPager` for cosmos. Subsequent converter calls in the same `GetRecommendations` run hit the in-memory map (O(1) lookup), eliminating the N+1 storm. A failed catalogue fetch leaves the cache map nil; converters fall back to the previous empty-Details behaviour with a one-time WARN log — the conversion itself never fails. + +Fields now populated from the catalogue: + +- **Cache:** `CacheDetails.Shards` from `armredis.Properties.ShardCount` (clustered Premium-tier caches; non-clustered caches keep Shards at zero, which signals "unknown"). +- **Database:** `DatabaseDetails.EngineVersion` from the SQL Server version traversed in `armsql.LocationCapabilities.SupportedServerVersions` → `SupportedEditions` → `SupportedServiceLevelObjectives` → `SKU.Name`. AZConfig/Deployment require per-server lookups and remain deferred. +- **Cosmos:** `NoSQLDetails.APIType` (sql / mongodb / cassandra / gremlin / table) resolved by mapping `account.Kind` + `account.Properties.Capabilities` for the single dominant APIType across the subscription's Cosmos accounts. Multi-API-type subscriptions leave APIType empty rather than guessing. + +Tests in each service's `client_test.go` cover three new contracts per service: + +- `_PopulatesShardsFromSKUCache` / `_PopulatesAPIType` / `_PopulatesEngineVersion` — cache hit populates the previously-deferred field. +- `_PagerErrorFallsBack` / `_CapabilitiesErrorFallsBack` — catalogue-fetch error leaves the field empty + the conversion still succeeds. +- `_FetchedOnce` — many converter calls share a single catalogue fetch (the N+1 invariant). + +#### MEDIUM: Azure compute Details vCPU/MemoryGB require ComputeDetails type extension + +**File:** `providers/azure/services/compute/client.go::convertAzureVMRecommendation`, `pkg/common/types.go::ComputeDetails` + +**Description:** The cache/cosmos/database batched-SKU-catalogue lookup intentionally skipped compute. Reason: `common.ComputeDetails` currently exposes only `InstanceType`, `Platform`, `Tenancy`, `Scope`. The SKU-catalogue enrichments compute would supply (vCPU + MemoryGB from `armcompute.ResourceSKU.Capabilities`) have no struct field to write into. Adding fields to a shared `pkg/common` type touches every cloud provider's converter, the frontend rendering layer, the matchers, and existing snapshot/golden tests across all 3 cloud providers — well outside the scope of a perf change to the Azure converters. + +**Proposed fix:** (1) add `VCPU int` + `MemoryGB float64` fields to `common.ComputeDetails`; (2) update AWS + GCP converters to populate them where the SDK already exposes the data; (3) port the same lazy `cachedSKULookup` pattern from cache/cosmos/database into compute and wire the new fields through. **Status:** ❓ Needs triage diff --git a/providers/azure/services/cache/client.go b/providers/azure/services/cache/client.go index 271c0d5c..b36d3101 100644 --- a/providers/azure/services/cache/client.go +++ b/providers/azure/services/cache/client.go @@ -10,6 +10,7 @@ import ( "net/http" "net/url" "strings" + "sync" "time" "github.com/Azure/azure-sdk-for-go/sdk/azcore" @@ -19,11 +20,22 @@ import ( "github.com/google/uuid" "github.com/LeanerCloud/CUDly/pkg/common" + "github.com/LeanerCloud/CUDly/pkg/logging" "github.com/LeanerCloud/CUDly/providers/azure/internal/httpclient" "github.com/LeanerCloud/CUDly/providers/azure/internal/pricing" "github.com/LeanerCloud/CUDly/providers/azure/internal/recommendations" ) +// redisSKUEntry holds the SKU-catalogue-derived fields the converter +// wants for each Redis SKU. Sourced from the cache's Properties: +// - shardCount: Properties.ShardCount (Premium-tier clustered caches). +// +// Non-clustered caches return shardCount=0; the converter treats 0 as +// "unknown" and leaves CacheDetails.Shards at its zero value. +type redisSKUEntry struct { + shardCount int +} + // HTTPClient interface for HTTP operations (enables mocking) type HTTPClient interface { Do(req *http.Request) (*http.Response, error) @@ -56,6 +68,16 @@ type CacheClient struct { recommendationsPager RecommendationsPager reservationsPager ReservationsDetailsPager redisCachesPager RedisCachesPager + + // Lazy SKU catalogue cache. armredis exposes no per-region "list all + // possible SKUs" surface, so we derive shard counts from the existing + // caches in the subscription (NewListBySubscriptionPager). Fetched + // ONCE per client lifetime; subsequent converter calls in the same + // GetRecommendations run hit the in-memory map. A failed fetch leaves + // skuCacheMap nil and converters fall back to Shards=0 with a WARN + // log — the conversion itself does NOT fail. + skuCacheOnce sync.Once + skuCacheMap map[string]redisSKUEntry } // NewClient creates a new Azure Cache client @@ -557,14 +579,23 @@ func extractRedisPricing(items []CacheRetailPriceItem, termYears int) (onDemand, // SDK-to-struct ladder. Returns nil when the SDK payload is unusable. // // Details populated by parsing the SKU string into Engine ("redis") and -// NodeType. Shard count requires an armredis SKU catalogue lookup and is -// deferred to a batched-enrichment follow-up tracked in -// known_issues/10_azure_provider.md. -func (c *CacheClient) convertAzureRedisRecommendation(_ context.Context, azureRec armconsumption.ReservationRecommendationClassification) *common.Recommendation { +// NodeType, then enriched from the lazily-cached armredis catalogue +// (cachedSKULookup). Shards is sourced from the catalogue when an +// existing cache in the subscription matches the recommendation's +// Premium-tier SKU; otherwise stays 0 (zero means "unknown", not +// "definitely zero shards" — see the redisSKUEntry godoc). +func (c *CacheClient) convertAzureRedisRecommendation(ctx context.Context, azureRec armconsumption.ReservationRecommendationClassification) *common.Recommendation { f := recommendations.Extract(azureRec) if f == nil { return nil } + details := common.CacheDetails{ + Engine: "redis", + NodeType: f.ResourceType, + } + if entry, ok := c.cachedSKULookup(ctx, f.ResourceType); ok && entry.shardCount > 0 { + details.Shards = entry.shardCount + } return &common.Recommendation{ Provider: common.ProviderAzure, Service: common.ServiceCache, @@ -579,11 +610,74 @@ func (c *CacheClient) convertAzureRedisRecommendation(_ context.Context, azureRe Term: f.Term, PaymentOption: "upfront", Timestamp: time.Now(), - Details: common.CacheDetails{ - Engine: "redis", - NodeType: f.ResourceType, - }, + Details: details, + } +} + +// cachedSKULookup returns the SKU catalogue entry for skuName, fetching +// the catalogue lazily on first call. The catalogue is fetched ONCE per +// client lifetime via armredis.Client.NewListBySubscriptionPager; +// subsequent calls are O(1) map lookups. ok=false on cache miss OR +// catalogue-fetch failure — the caller falls back to Shards=0 rather +// than failing the whole conversion. +// +// Why source from existing caches: armredis exposes no "list all +// possible SKUs" endpoint. The catalogue surface that does exist +// (existing cache instances in the subscription) gives us authoritative +// shard counts for the SKUs the customer actually uses, which is the +// set the recommendation engine recommends from anyway. +func (c *CacheClient) cachedSKULookup(ctx context.Context, skuName string) (redisSKUEntry, bool) { + c.skuCacheOnce.Do(func() { + c.skuCacheMap = c.fetchSKUCatalogue(ctx) + }) + if c.skuCacheMap == nil { + return redisSKUEntry{}, false + } + entry, ok := c.skuCacheMap[skuName] + return entry, ok +} + +// fetchSKUCatalogue performs the single ListBySubscription walk and +// reduces the response into a name->redisSKUEntry map keyed by the +// "_" SKU naming convention (e.g. "Premium_P1") +// that matches the recommendation engine's ResourceType output. +// +// Returns nil on error so the sync.Once-gated cache field stays nil and +// cachedSKULookup falls back to the empty-Details path. The fetch error +// is logged WARN once. +func (c *CacheClient) fetchSKUCatalogue(ctx context.Context) map[string]redisSKUEntry { + pager, err := c.createRedisCachesPager() + if err != nil { + logging.Warnf("azure cache: SKU catalogue pager create failed for region %s: %v — Details.Shards left at 0", c.region, err) + return nil + } + out := make(map[string]redisSKUEntry) + for pager.More() { + page, err := pager.NextPage(ctx) + if err != nil { + logging.Warnf("azure cache: SKU catalogue page fetch failed for region %s: %v — partial cache (%d entries) discarded, Details.Shards left at 0", c.region, err, len(out)) + return nil + } + for _, cache := range page.Value { + fullSKU := extractSKUFromCache(cache) + if fullSKU == "" { + continue + } + shards := 0 + if cache.Properties != nil && cache.Properties.ShardCount != nil { + shards = int(*cache.Properties.ShardCount) + } + // First-write-wins: if the same SKU appears on two caches + // with different ShardCounts, keep the first. Real Premium + // clusters configured to the same SKU typically share the + // shard count anyway — this just keeps the cache + // deterministic regardless of pager order. + if _, exists := out[fullSKU]; !exists { + out[fullSKU] = redisSKUEntry{shardCount: shards} + } + } } + return out } // applyPurchaseAutomationTag attaches the purchase-automation tag to an Azure diff --git a/providers/azure/services/cache/client_test.go b/providers/azure/services/cache/client_test.go index 13fa8687..7a950787 100644 --- a/providers/azure/services/cache/client_test.go +++ b/providers/azure/services/cache/client_test.go @@ -740,9 +740,14 @@ func TestCacheClient_ConvertAzureRedisRecommendation_NilGuards(t *testing.T) { // TestCacheClient_ConvertAzureRedisRecommendation_PopulatesAllFields asserts // the converter forwards every helper-extracted field + applies the -// Cache-service-specific constants. +// Cache-service-specific constants. An empty SKU catalogue (no caches in +// the subscription) is the "no signal" baseline — Shards stays 0. func TestCacheClient_ConvertAzureRedisRecommendation_PopulatesAllFields(t *testing.T) { client := NewClient(nil, "test-subscription", "eastus") + // Inject empty mock pager so the cachedSKULookup hits the lazy-fetch + // codepath without making a real Azure API call. + client.SetRedisCachesPager(&MockRedisCachesPager{}) + rec := mocks.BuildLegacyReservationRecommendation( mocks.WithRegion("eastus"), mocks.WithTerm("P3Y"), @@ -765,14 +770,131 @@ func TestCacheClient_ConvertAzureRedisRecommendation_PopulatesAllFields(t *testi assert.Equal(t, "3yr", out.Term) assert.Equal(t, "upfront", out.PaymentOption) - // Details carries Engine=redis + NodeType from the SKU string. - // Shard count is deferred to batched enrichment. + // Details carries Engine=redis + NodeType from the SKU string. Shards + // stays 0 when no cache instance matches in the subscription (the + // catalogue's only signal source for shard counts). require.NotNil(t, out.Details) details, ok := out.Details.(common.CacheDetails) require.True(t, ok, "Details must be a common.CacheDetails value") assert.Equal(t, "redis", details.Engine) assert.Equal(t, "Premium_P3", details.NodeType) - assert.Equal(t, 0, details.Shards, "Shards is deferred to batched enrichment") + assert.Equal(t, 0, details.Shards, "Shards is 0 when no matching cache exists in the subscription") +} + +// TestCacheClient_ConvertAzureRedisRecommendation_PopulatesShardsFromSKUCache +// asserts the new batched-SKU-catalogue lookup populates CacheDetails.Shards +// when the subscription has a Premium-tier clustered cache with the same +// SKU as the recommendation. Single ListBySubscription call per client +// lifetime feeds many converter calls; this test pins the contract. +func TestCacheClient_ConvertAzureRedisRecommendation_PopulatesShardsFromSKUCache(t *testing.T) { + client := NewClient(nil, "test-subscription", "eastus") + + premiumName := armredis.SKUNamePremium + familyP := armredis.SKUFamilyP + capacity := int32(3) + shardCount := int32(5) + + // One Premium_P3 cache with 5 shards in the subscription. The + // converter will see f.ResourceType="Premium_P3" and look it up. + mockPager := &MockRedisCachesPager{ + pages: []armredis.ClientListBySubscriptionResponse{ + { + ListResult: armredis.ListResult{ + Value: []*armredis.ResourceInfo{ + { + Properties: &armredis.Properties{ + SKU: &armredis.SKU{ + Name: &premiumName, + Family: &familyP, + Capacity: &capacity, + }, + ShardCount: &shardCount, + }, + }, + }, + }, + }, + }, + } + client.SetRedisCachesPager(mockPager) + + rec := mocks.BuildLegacyReservationRecommendation( + mocks.WithRegion("eastus"), + mocks.WithNormalizedSize("Premium_P3"), + ) + out := client.convertAzureRedisRecommendation(context.Background(), rec) + require.NotNil(t, out) + details, ok := out.Details.(common.CacheDetails) + require.True(t, ok) + assert.Equal(t, "redis", details.Engine) + assert.Equal(t, "Premium_P3", details.NodeType) + assert.Equal(t, 5, details.Shards, "Shards must be enriched from the cached SKU catalogue") +} + +// TestCacheClient_ConvertAzureRedisRecommendation_PagerErrorFallsBack +// asserts that a SKU-catalogue fetch failure does NOT fail the +// conversion — Shards just stays at 0 and the rest of Details is +// populated from the recommendation payload as before. This is the +// graceful-degradation contract the issue asks for. +func TestCacheClient_ConvertAzureRedisRecommendation_PagerErrorFallsBack(t *testing.T) { + client := NewClient(nil, "test-subscription", "eastus") + mockPager := &MockRedisCachesPager{ + pages: []armredis.ClientListBySubscriptionResponse{{}}, + err: errors.New("transient Azure API error"), + } + client.SetRedisCachesPager(mockPager) + + rec := mocks.BuildLegacyReservationRecommendation( + mocks.WithRegion("eastus"), + mocks.WithNormalizedSize("Premium_P3"), + ) + out := client.convertAzureRedisRecommendation(context.Background(), rec) + require.NotNil(t, out, "conversion must NOT fail on catalogue-fetch error") + details, ok := out.Details.(common.CacheDetails) + require.True(t, ok) + assert.Equal(t, "redis", details.Engine) + assert.Equal(t, "Premium_P3", details.NodeType) + assert.Equal(t, 0, details.Shards, "Shards left at 0 when catalogue fetch fails") +} + +// TestCacheClient_CachedSKULookup_FetchedOnce pins the perf invariant: +// many converter calls in the same GetRecommendations run trigger +// exactly ONE catalogue fetch. The mock pager counts pages served; the +// assertion verifies the count stays at 1 after multiple lookups. +func TestCacheClient_CachedSKULookup_FetchedOnce(t *testing.T) { + client := NewClient(nil, "test-subscription", "eastus") + + premiumName := armredis.SKUNamePremium + familyP := armredis.SKUFamilyP + capacity := int32(1) + shardCount := int32(2) + + mockPager := &MockRedisCachesPager{ + pages: []armredis.ClientListBySubscriptionResponse{ + { + ListResult: armredis.ListResult{ + Value: []*armredis.ResourceInfo{ + { + Properties: &armredis.Properties{ + SKU: &armredis.SKU{ + Name: &premiumName, + Family: &familyP, + Capacity: &capacity, + }, + ShardCount: &shardCount, + }, + }, + }, + }, + }, + }, + } + client.SetRedisCachesPager(mockPager) + + for i := 0; i < 10; i++ { + _, _ = client.cachedSKULookup(context.Background(), "Premium_P1") + } + assert.Equal(t, 1, mockPager.index, "catalogue must be fetched ONCE regardless of lookup count") } // Test the to package is properly imported (used in tests) diff --git a/providers/azure/services/compute/client.go b/providers/azure/services/compute/client.go index f1c4ab88..3ca8d2ca 100644 --- a/providers/azure/services/compute/client.go +++ b/providers/azure/services/compute/client.go @@ -672,11 +672,15 @@ func extractVMPricing(items []AzureRetailPriceItem, termYears int) (onDemand, re // providers/azure/internal/recommendations so the four Azure service // converters share the same type-assertion + nil-guard ladder. // -// Details populated from the reservation recommendation payload only — -// Platform / Tenancy / Scope (and the vCPU / memory counts a UI would -// want) require an ARM SKU-catalogue lookup; populating them here would -// trigger an N+1 armcompute.ResourceSKUsClient.ListByLocation per -// recommendation. That batched-enrichment is tracked as a follow-up in +// Compute is NOT yet wired to the batched-SKU-catalogue lookup landed +// for cache/cosmosdb/database. Reason: common.ComputeDetails (in +// pkg/common/types.go) currently only exposes InstanceType, Platform, +// Tenancy, Scope. The two enrichments the SKU catalogue would supply +// for compute are vCPU and MemoryGB — neither of which has a field on +// ComputeDetails to write into. Adding new fields to a shared struct +// touches every cloud provider + every consumer (frontend, common +// matchers, three other providers' converters), which is well outside +// the scope of this perf change. Tracked as a follow-up — see // known_issues/10_azure_provider.md. func (c *ComputeClient) convertAzureVMRecommendation(_ context.Context, azureRec armconsumption.ReservationRecommendationClassification) *common.Recommendation { f := recommendations.Extract(azureRec) @@ -699,7 +703,7 @@ func (c *ComputeClient) convertAzureVMRecommendation(_ context.Context, azureRec Timestamp: time.Now(), // Only InstanceType is safely derivable from the payload. Platform, // Tenancy, Scope are armcompute.ResourceSKUsClient territory and - // are deferred to the batched-enrichment follow-up. + // are deferred — see the function godoc for the scope decision. Details: common.ComputeDetails{ InstanceType: f.ResourceType, }, diff --git a/providers/azure/services/cosmosdb/client.go b/providers/azure/services/cosmosdb/client.go index ad908481..3263a5af 100644 --- a/providers/azure/services/cosmosdb/client.go +++ b/providers/azure/services/cosmosdb/client.go @@ -11,6 +11,7 @@ import ( "net/url" "strconv" "strings" + "sync" "time" "github.com/Azure/azure-sdk-for-go/sdk/azcore" @@ -20,6 +21,7 @@ import ( "github.com/google/uuid" "github.com/LeanerCloud/CUDly/pkg/common" + "github.com/LeanerCloud/CUDly/pkg/logging" "github.com/LeanerCloud/CUDly/providers/azure/internal/httpclient" "github.com/LeanerCloud/CUDly/providers/azure/internal/pricing" "github.com/LeanerCloud/CUDly/providers/azure/internal/recommendations" @@ -57,6 +59,16 @@ type CosmosDBClient struct { recommendationsPager RecommendationsPager reservationsPager ReservationsDetailsPager cosmosAccountsPager CosmosAccountsPager + + // Lazy account-API-type cache. Cosmos DB has no per-SKU APIType (the + // API type lives on the account, not the reservation SKU which is + // just a throughput tier like "100RU"). The cache fetches the + // subscription's Cosmos accounts ONCE and reduces them to a single + // dominant APIType the converter can apply to recommendations. + // "Dominant" = the only API type observed if there's exactly one, + // else empty (meaning: subscription is multi-API, can't infer). + apiTypeOnce sync.Once + apiType string } // NewClient creates a new Azure Cosmos DB client @@ -573,17 +585,21 @@ func calculateCosmosSavingsPercentage(onDemandPrice, hoursInTerm, reservationPri // See providers/azure/internal/recommendations.Extract for the shared // SDK-to-struct ladder. Returns nil when the SDK payload is unusable. // -// Details populated with Engine="cosmos" and ThroughputUnits parsed from -// the SKU string (f.ResourceType) when it encodes a throughput tier. -// APIType (sql / mongodb / cassandra / gremlin / table) requires an -// armcosmos.DatabaseAccountsClient lookup and is deferred to the same -// batched-enrichment follow-up that covers compute/database/cache SDK -// fields — tracked in known_issues/10_azure_provider.md. -func (c *CosmosDBClient) convertAzureCosmosRecommendation(_ context.Context, azureRec armconsumption.ReservationRecommendationClassification) *common.Recommendation { +// Details populated with Engine="cosmos", ThroughputUnits parsed from +// the SKU string, and APIType resolved via cachedAPIType — a single +// armcosmos.DatabaseAccountsClient.NewListPager call cached for the +// client lifetime, gated by a sync.Once. APIType is left empty when the +// subscription has zero Cosmos accounts, multiple Cosmos accounts with +// different API types (ambiguous), or the listing fails. +func (c *CosmosDBClient) convertAzureCosmosRecommendation(ctx context.Context, azureRec armconsumption.ReservationRecommendationClassification) *common.Recommendation { f := recommendations.Extract(azureRec) if f == nil { return nil } + details := detailsFromCosmosSKU(f.ResourceType) + if api := c.cachedAPIType(ctx); api != "" { + details.APIType = api + } return &common.Recommendation{ Provider: common.ProviderAzure, Service: common.ServiceNoSQL, @@ -598,8 +614,117 @@ func (c *CosmosDBClient) convertAzureCosmosRecommendation(_ context.Context, azu Term: f.Term, PaymentOption: "upfront", Timestamp: time.Now(), - Details: detailsFromCosmosSKU(f.ResourceType), + Details: details, + } +} + +// cachedAPIType returns the single dominant Cosmos APIType observed +// across the subscription's Cosmos accounts, or "" when the answer is +// ambiguous (multi-API subscription) or unavailable (zero accounts / +// fetch failure). The accounts list is fetched ONCE per client lifetime +// via armcosmos.DatabaseAccountsClient.NewListPager — subsequent +// converter calls in the same GetRecommendations run hit the cached +// string. Failure is logged WARN once; the converter falls back to the +// previous empty-APIType behaviour. +// +// Why dominant-only: a Cosmos reservation SKU like "100RU" doesn't +// reference an account, so we can't pick "the right" APIType per rec. +// Returning the only observed APIType is correct for the common +// single-API-type subscription; returning "" for multi-API subscriptions +// is honest about the ambiguity rather than guessing wrong. +func (c *CosmosDBClient) cachedAPIType(ctx context.Context) string { + c.apiTypeOnce.Do(func() { + c.apiType = c.fetchDominantAPIType(ctx) + }) + return c.apiType +} + +// fetchDominantAPIType walks the accounts pager once, collects the +// distinct APIType per account (mongodb / cassandra / gremlin / table / +// sql), and returns the single dominant value or "". +func (c *CosmosDBClient) fetchDominantAPIType(ctx context.Context) string { + pager, err := c.createCosmosAccountsPager() + if err != nil { + logging.Warnf("azure cosmosdb: account listing pager create failed for region %s: %v — Details.APIType left empty", c.region, err) + return "" + } + observed := make(map[string]struct{}) + for pager.More() { + page, err := pager.NextPage(ctx) + if err != nil { + logging.Warnf("azure cosmosdb: account listing page fetch failed for region %s: %v — Details.APIType left empty", c.region, err) + return "" + } + for _, account := range page.Value { + if api := apiTypeFromAccount(account); api != "" { + observed[api] = struct{}{} + } + } + } + if len(observed) != 1 { + // Zero accounts → no signal. Multiple distinct API types → + // ambiguous (don't lie to the UI). Either way, leave APIType + // empty so the existing "unknown" rendering kicks in. + return "" + } + for api := range observed { + return api + } + return "" +} + +// apiTypeFromAccount maps a Cosmos account's Kind + Capabilities to the +// canonical APIType string used in NoSQLDetails.APIType. Mapping: +// +// - Kind == "MongoDB" → "mongodb" +// - Kind == "Parse" → "" (legacy, no rec target) +// - Kind == "GlobalDocumentDB" + Capabilities contains: +// "EnableCassandra" → "cassandra" +// "EnableGremlin" → "gremlin" +// "EnableTable" → "table" +// otherwise → "sql" (the SQL/Core API default) +// +// Returns "" when account or Kind is nil. +func apiTypeFromAccount(account *armcosmos.DatabaseAccountGetResults) string { + if account == nil || account.Kind == nil { + return "" + } + switch *account.Kind { + case armcosmos.DatabaseAccountKindMongoDB: + return "mongodb" + case armcosmos.DatabaseAccountKindParse: + return "" + case armcosmos.DatabaseAccountKindGlobalDocumentDB: + return apiTypeFromGlobalDocumentDB(account.Properties) + default: + return "" + } +} + +// apiTypeFromGlobalDocumentDB disambiguates the GlobalDocumentDB account +// kind via its capability hints. Cosmos accounts of kind GlobalDocumentDB +// can be the SQL/Core API (default) or, when a specific capability is +// enabled, Cassandra / Gremlin / Table. Extracted out of +// apiTypeFromAccount to keep that function under the +// cyclomatic-complexity threshold enforced by the pre-commit hook. +func apiTypeFromGlobalDocumentDB(props *armcosmos.DatabaseAccountGetProperties) string { + if props == nil { + return "sql" + } + for _, capability := range props.Capabilities { + if capability == nil || capability.Name == nil { + continue + } + switch *capability.Name { + case "EnableCassandra": + return "cassandra" + case "EnableGremlin": + return "gremlin" + case "EnableTable": + return "table" + } } + return "sql" } // detailsFromCosmosSKU parses an Azure Cosmos DB reservation SKU string diff --git a/providers/azure/services/cosmosdb/client_test.go b/providers/azure/services/cosmosdb/client_test.go index be2ed72e..273873be 100644 --- a/providers/azure/services/cosmosdb/client_test.go +++ b/providers/azure/services/cosmosdb/client_test.go @@ -852,9 +852,14 @@ func TestCosmosDBClient_ConvertAzureCosmosRecommendation_NilGuards(t *testing.T) // TestCosmosDBClient_ConvertAzureCosmosRecommendation_PopulatesAllFields // asserts the converter forwards every helper-extracted field + applies -// the CosmosDB-service-specific constants. +// the CosmosDB-service-specific constants. An empty accounts list +// (zero-account subscription) is the "no signal" baseline — APIType +// stays empty. func TestCosmosDBClient_ConvertAzureCosmosRecommendation_PopulatesAllFields(t *testing.T) { client := NewClient(nil, "test-subscription", "eastus") + // Inject empty mock pager so cachedAPIType doesn't make a real call. + client.SetCosmosAccountsPager(&MockCosmosAccountsPager{}) + rec := mocks.BuildLegacyReservationRecommendation( mocks.WithRegion("westus2"), mocks.WithTerm("P1Y"), @@ -880,13 +885,185 @@ func TestCosmosDBClient_ConvertAzureCosmosRecommendation_PopulatesAllFields(t *t // Details carries Engine="cosmos". For this fixture the SKU doesn't // start with digits, so ThroughputUnits is 0 — which correctly // signals "unknown from payload" rather than being parsed wrong. + // APIType stays empty because no Cosmos accounts exist in the + // subscription (no signal). require.NotNil(t, out.Details) details, ok := out.Details.(common.NoSQLDetails) require.True(t, ok, "Details must be a common.NoSQLDetails value") assert.Equal(t, "cosmos", details.Engine) - assert.Empty(t, details.APIType, "APIType requires an armcosmos lookup and is deferred") + assert.Empty(t, details.APIType, "APIType empty when subscription has no Cosmos accounts") } +// TestCosmosDBClient_ConvertAzureCosmosRecommendation_PopulatesAPIType +// asserts the new batched-account-list lookup populates +// NoSQLDetails.APIType when the subscription has exactly one Cosmos +// account, mapping the account.Kind + Capabilities into the canonical +// API string. +func TestCosmosDBClient_ConvertAzureCosmosRecommendation_PopulatesAPIType(t *testing.T) { + tests := []struct { + name string + account *armcosmos.DatabaseAccountGetResults + wantAPIType string + }{ + { + name: "MongoDB kind", + account: &armcosmos.DatabaseAccountGetResults{ + Kind: ptr(armcosmos.DatabaseAccountKindMongoDB), + }, + wantAPIType: "mongodb", + }, + { + name: "GlobalDocumentDB + Cassandra capability", + account: &armcosmos.DatabaseAccountGetResults{ + Kind: ptr(armcosmos.DatabaseAccountKindGlobalDocumentDB), + Properties: &armcosmos.DatabaseAccountGetProperties{ + Capabilities: []*armcosmos.Capability{ + {Name: ptrStr("EnableCassandra")}, + }, + }, + }, + wantAPIType: "cassandra", + }, + { + name: "GlobalDocumentDB + Gremlin capability", + account: &armcosmos.DatabaseAccountGetResults{ + Kind: ptr(armcosmos.DatabaseAccountKindGlobalDocumentDB), + Properties: &armcosmos.DatabaseAccountGetProperties{ + Capabilities: []*armcosmos.Capability{ + {Name: ptrStr("EnableGremlin")}, + }, + }, + }, + wantAPIType: "gremlin", + }, + { + name: "GlobalDocumentDB + Table capability", + account: &armcosmos.DatabaseAccountGetResults{ + Kind: ptr(armcosmos.DatabaseAccountKindGlobalDocumentDB), + Properties: &armcosmos.DatabaseAccountGetProperties{ + Capabilities: []*armcosmos.Capability{ + {Name: ptrStr("EnableTable")}, + }, + }, + }, + wantAPIType: "table", + }, + { + name: "GlobalDocumentDB + no capability hints (SQL/Core API)", + account: &armcosmos.DatabaseAccountGetResults{ + Kind: ptr(armcosmos.DatabaseAccountKindGlobalDocumentDB), + Properties: &armcosmos.DatabaseAccountGetProperties{}, + }, + wantAPIType: "sql", + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + client := NewClient(nil, "test-subscription", "eastus") + client.SetCosmosAccountsPager(&MockCosmosAccountsPager{ + pages: []armcosmos.DatabaseAccountsClientListResponse{ + { + DatabaseAccountsListResult: armcosmos.DatabaseAccountsListResult{ + Value: []*armcosmos.DatabaseAccountGetResults{tt.account}, + }, + }, + }, + }) + + rec := mocks.BuildLegacyReservationRecommendation( + mocks.WithRegion("eastus"), + mocks.WithNormalizedSize("100RU"), + ) + out := client.convertAzureCosmosRecommendation(context.Background(), rec) + require.NotNil(t, out) + details, ok := out.Details.(common.NoSQLDetails) + require.True(t, ok) + assert.Equal(t, tt.wantAPIType, details.APIType) + }) + } +} + +// TestCosmosDBClient_ConvertAzureCosmosRecommendation_AmbiguousAPIType +// asserts that a multi-API-type subscription leaves APIType empty — +// guessing would produce wrong UI rendering. +func TestCosmosDBClient_ConvertAzureCosmosRecommendation_AmbiguousAPIType(t *testing.T) { + client := NewClient(nil, "test-subscription", "eastus") + client.SetCosmosAccountsPager(&MockCosmosAccountsPager{ + pages: []armcosmos.DatabaseAccountsClientListResponse{ + { + DatabaseAccountsListResult: armcosmos.DatabaseAccountsListResult{ + Value: []*armcosmos.DatabaseAccountGetResults{ + {Kind: ptr(armcosmos.DatabaseAccountKindMongoDB)}, + {Kind: ptr(armcosmos.DatabaseAccountKindGlobalDocumentDB)}, + }, + }, + }, + }, + }) + + rec := mocks.BuildLegacyReservationRecommendation( + mocks.WithRegion("eastus"), + mocks.WithNormalizedSize("100RU"), + ) + out := client.convertAzureCosmosRecommendation(context.Background(), rec) + require.NotNil(t, out) + details, ok := out.Details.(common.NoSQLDetails) + require.True(t, ok) + assert.Empty(t, details.APIType, "ambiguous (multi-API) subscription leaves APIType empty") +} + +// TestCosmosDBClient_ConvertAzureCosmosRecommendation_PagerErrorFallsBack +// asserts that an account-listing failure does NOT fail the conversion +// — APIType just stays empty and the rest of Details is populated as +// before. +func TestCosmosDBClient_ConvertAzureCosmosRecommendation_PagerErrorFallsBack(t *testing.T) { + client := NewClient(nil, "test-subscription", "eastus") + client.SetCosmosAccountsPager(&MockCosmosAccountsPager{ + err: errors.New("transient Azure API error"), + }) + + rec := mocks.BuildLegacyReservationRecommendation( + mocks.WithRegion("eastus"), + mocks.WithNormalizedSize("100RU"), + ) + out := client.convertAzureCosmosRecommendation(context.Background(), rec) + require.NotNil(t, out, "conversion must NOT fail on accounts-listing error") + details, ok := out.Details.(common.NoSQLDetails) + require.True(t, ok) + assert.Equal(t, "cosmos", details.Engine) + assert.Equal(t, 100, details.ThroughputUnits) + assert.Empty(t, details.APIType, "APIType empty when accounts listing fails") +} + +// TestCosmosDBClient_CachedAPIType_FetchedOnce pins the perf invariant: +// many converter calls share a single accounts-listing fetch. +func TestCosmosDBClient_CachedAPIType_FetchedOnce(t *testing.T) { + client := NewClient(nil, "test-subscription", "eastus") + mockPager := &MockCosmosAccountsPager{ + pages: []armcosmos.DatabaseAccountsClientListResponse{ + { + DatabaseAccountsListResult: armcosmos.DatabaseAccountsListResult{ + Value: []*armcosmos.DatabaseAccountGetResults{ + {Kind: ptr(armcosmos.DatabaseAccountKindMongoDB)}, + }, + }, + }, + }, + } + client.SetCosmosAccountsPager(mockPager) + + for i := 0; i < 10; i++ { + _ = client.cachedAPIType(context.Background()) + } + assert.Equal(t, 1, mockPager.index, "accounts list must be fetched ONCE regardless of lookup count") +} + +// ptr / ptrStr are local pointer-builder helpers that keep the table +// fixtures readable. The mocks package's StringPtr is too generic for +// the typed Cosmos enums. +func ptr[T any](v T) *T { return &v } +func ptrStr(s string) *string { return &s } + // TestDetailsFromCosmosSKU covers the permissive SKU parser directly. // Real SKUs look like "100RU", "1000RUperSecond", or service-emitted // strings without a numeric prefix. The parser extracts leading digits diff --git a/providers/azure/services/database/client.go b/providers/azure/services/database/client.go index 2afae9b9..3ed60b8e 100644 --- a/providers/azure/services/database/client.go +++ b/providers/azure/services/database/client.go @@ -10,6 +10,7 @@ import ( "net/http" "net/url" "strings" + "sync" "time" "github.com/Azure/azure-sdk-for-go/sdk/azcore" @@ -19,11 +20,21 @@ import ( "github.com/google/uuid" "github.com/LeanerCloud/CUDly/pkg/common" + "github.com/LeanerCloud/CUDly/pkg/logging" "github.com/LeanerCloud/CUDly/providers/azure/internal/httpclient" "github.com/LeanerCloud/CUDly/providers/azure/internal/pricing" "github.com/LeanerCloud/CUDly/providers/azure/internal/recommendations" ) +// sqlSKUEntry holds the SKU-catalogue-derived fields the converter +// wants for each Azure SQL SKU. Sourced from the +// armsql.CapabilitiesClient.ListByLocation response which embeds the +// SQL Server version in the ServerVersionCapability.Name (e.g. "12.0") +// while traversing down to ServiceLevelObjective SKUs. +type sqlSKUEntry struct { + engineVersion string +} + // HTTPClient interface for HTTP operations (enables mocking) type HTTPClient interface { Do(req *http.Request) (*http.Response, error) @@ -55,6 +66,14 @@ type DatabaseClient struct { recommendationsPager RecommendationsPager reservationsPager ReservationsDetailsPager capabilitiesClient CapabilitiesClient + + // Lazy SKU catalogue cache. Populated once on the first + // recommendation conversion in this client's GetRecommendations + // call — single ListByLocation per region instead of N+1 per rec. + // A failed fetch leaves skuCacheMap nil; converters then fall back + // to empty EngineVersion with a one-time WARN log. + skuCacheOnce sync.Once + skuCacheMap map[string]sqlSKUEntry } // NewClient creates a new Azure Database client @@ -564,16 +583,21 @@ func extractSQLPricing(items []DatabaseRetailPriceItem, termYears int) (onDemand // SDK-to-struct ladder. Returns nil when the SDK payload is unusable so // the caller can filter it out. // -// Details populated by parsing the SKU string (f.ResourceType) — the -// reservation recommendation payload doesn't carry a separate -// engine/edition/vcores field. armsql SKU catalogue calls (for deep -// engine version + az-config data) are deferred to a batched-enrichment -// follow-up tracked in known_issues/10_azure_provider.md. -func (c *DatabaseClient) convertAzureSQLRecommendation(_ context.Context, azureRec armconsumption.ReservationRecommendationClassification) *common.Recommendation { +// Details: Engine="sqlserver" + InstanceClass=ResourceType (always +// populated). EngineVersion enriched from the lazily-cached +// armsql.CapabilitiesClient catalogue when the recommendation's SKU +// string matches a ServiceLevelObjective in the location capabilities; +// otherwise stays empty. AZConfig/Deployment still need additional +// signals (per-server config) and remain deferred. +func (c *DatabaseClient) convertAzureSQLRecommendation(ctx context.Context, azureRec armconsumption.ReservationRecommendationClassification) *common.Recommendation { f := recommendations.Extract(azureRec) if f == nil { return nil } + details := detailsFromSQLSKU(f.ResourceType) + if entry, ok := c.cachedSKULookup(ctx, f.ResourceType); ok && entry.engineVersion != "" { + details.EngineVersion = entry.engineVersion + } return &common.Recommendation{ Provider: common.ProviderAzure, Service: common.ServiceRelationalDB, @@ -588,7 +612,74 @@ func (c *DatabaseClient) convertAzureSQLRecommendation(_ context.Context, azureR Term: f.Term, PaymentOption: "upfront", Timestamp: time.Now(), - Details: detailsFromSQLSKU(f.ResourceType), + Details: details, + } +} + +// cachedSKULookup returns the SKU catalogue entry for skuName, fetching +// the catalogue lazily on first call. The catalogue is fetched ONCE per +// client lifetime via armsql.CapabilitiesClient.ListByLocation; +// subsequent calls are O(1) map lookups. ok=false on cache miss OR +// catalogue-fetch failure — the caller falls back to empty +// EngineVersion rather than failing the whole conversion. +func (c *DatabaseClient) cachedSKULookup(ctx context.Context, skuName string) (sqlSKUEntry, bool) { + c.skuCacheOnce.Do(func() { + c.skuCacheMap = c.fetchSKUCatalogue(ctx) + }) + if c.skuCacheMap == nil { + return sqlSKUEntry{}, false + } + entry, ok := c.skuCacheMap[skuName] + return entry, ok +} + +// fetchSKUCatalogue performs the single ListByLocation call and reduces +// the response into a name->sqlSKUEntry map keyed by ServiceLevelObjective +// SKU.Name (matches the recommendation engine's ResourceType). Returns +// nil on error so the sync.Once-gated cache field stays nil. +func (c *DatabaseClient) fetchSKUCatalogue(ctx context.Context) map[string]sqlSKUEntry { + capClient, err := c.getOrCreateCapabilitiesClient() + if err != nil { + logging.Warnf("azure database: SKU catalogue capabilities client create failed for region %s: %v — Details.EngineVersion left empty", c.region, err) + return nil + } + resp, err := capClient.ListByLocation(ctx, c.region, &armsql.CapabilitiesClientListByLocationOptions{Include: nil}) + if err != nil { + logging.Warnf("azure database: SKU catalogue ListByLocation failed for region %s: %v — Details.EngineVersion left empty", c.region, err) + return nil + } + out := make(map[string]sqlSKUEntry) + for _, version := range resp.LocationCapabilities.SupportedServerVersions { + if version == nil || version.Name == nil { + continue + } + populateSQLSKUMapFromVersion(out, *version.Name, version.SupportedEditions) + } + return out +} + +// populateSQLSKUMapFromVersion writes one sqlSKUEntry per ServiceLevelObjective +// SKU.Name found under the given server version's editions. Extracted out of +// fetchSKUCatalogue to keep that function under the cyclomatic-complexity +// threshold enforced by the pre-commit hook. First-write-wins semantics: if +// the same SKU name appears under multiple server versions (rare in +// practice), the first one wins. Order is deterministic per ListByLocation +// response; downstream consumers don't switch behaviour on the +// engine-version delta within a single region. +func populateSQLSKUMapFromVersion(out map[string]sqlSKUEntry, engineVersion string, editions []*armsql.EditionCapability) { + for _, edition := range editions { + if edition == nil { + continue + } + for _, slo := range edition.SupportedServiceLevelObjectives { + if slo == nil || slo.SKU == nil || slo.SKU.Name == nil { + continue + } + skuName := *slo.SKU.Name + if _, exists := out[skuName]; !exists { + out[skuName] = sqlSKUEntry{engineVersion: engineVersion} + } + } } } diff --git a/providers/azure/services/database/client_test.go b/providers/azure/services/database/client_test.go index 6aa59d9f..e52ee26e 100644 --- a/providers/azure/services/database/client_test.go +++ b/providers/azure/services/database/client_test.go @@ -63,13 +63,17 @@ func (m *MockReservationsDetailsPager) NextPage(ctx context.Context) (armconsump return page, nil } -// MockCapabilitiesClient mocks the CapabilitiesClient interface +// MockCapabilitiesClient mocks the CapabilitiesClient interface. +// CallCount tracks how many times ListByLocation was invoked — used by +// the cachedSKULookup-fetched-once test to pin the perf invariant. type MockCapabilitiesClient struct { - response armsql.CapabilitiesClientListByLocationResponse - err error + response armsql.CapabilitiesClientListByLocationResponse + err error + CallCount int } func (m *MockCapabilitiesClient) ListByLocation(ctx context.Context, locationName string, options *armsql.CapabilitiesClientListByLocationOptions) (armsql.CapabilitiesClientListByLocationResponse, error) { + m.CallCount++ if m.err != nil { return armsql.CapabilitiesClientListByLocationResponse{}, m.err } @@ -660,9 +664,15 @@ func TestDatabaseClient_ConvertAzureSQLRecommendation_NilGuards(t *testing.T) { // TestDatabaseClient_ConvertAzureSQLRecommendation_PopulatesAllFields // asserts the converter forwards every helper-extracted field + applies -// the Database-service-specific constants. +// the Database-service-specific constants. An empty capabilities +// response (region with no SKU mapping) is the "no signal" baseline — +// EngineVersion stays empty. func TestDatabaseClient_ConvertAzureSQLRecommendation_PopulatesAllFields(t *testing.T) { client := NewClient(nil, "test-subscription", "eastus") + // Inject empty capabilities client so cachedSKULookup doesn't make a + // real Azure call during this test. + client.SetCapabilitiesClient(&MockCapabilitiesClient{}) + rec := mocks.BuildLegacyReservationRecommendation( mocks.WithRegion("northeurope"), mocks.WithTerm("P1Y"), @@ -686,17 +696,119 @@ func TestDatabaseClient_ConvertAzureSQLRecommendation_PopulatesAllFields(t *test assert.Equal(t, "upfront", out.PaymentOption) // Details carries Engine=sqlserver + InstanceClass from the SKU - // string. EngineVersion/AZConfig/Deployment are deferred to batched - // enrichment via armsql. + // string. EngineVersion stays empty when the catalogue has no + // matching SKU (no signal); AZConfig/Deployment still need a + // per-server lookup and remain deferred. require.NotNil(t, out.Details) details, ok := out.Details.(common.DatabaseDetails) require.True(t, ok, "Details must be a common.DatabaseDetails value") assert.Equal(t, "sqlserver", details.Engine) assert.Equal(t, "GeneralPurpose_Gen5_2", details.InstanceClass) - assert.Empty(t, details.EngineVersion, "EngineVersion is deferred to batched enrichment") + assert.Empty(t, details.EngineVersion, "EngineVersion empty when no matching SKU in catalogue") assert.Empty(t, details.AZConfig, "AZConfig is deferred to batched enrichment") } +// TestDatabaseClient_ConvertAzureSQLRecommendation_PopulatesEngineVersion +// asserts the new batched-SKU-catalogue lookup populates +// DatabaseDetails.EngineVersion when the recommendation's SKU appears +// in the location capabilities response. +func TestDatabaseClient_ConvertAzureSQLRecommendation_PopulatesEngineVersion(t *testing.T) { + client := NewClient(nil, "test-subscription", "eastus") + + skuName := "GeneralPurpose_Gen5_2" + versionName := "12.0" + + mockClient := &MockCapabilitiesClient{ + response: armsql.CapabilitiesClientListByLocationResponse{ + LocationCapabilities: armsql.LocationCapabilities{ + SupportedServerVersions: []*armsql.ServerVersionCapability{ + { + Name: &versionName, + SupportedEditions: []*armsql.EditionCapability{ + { + SupportedServiceLevelObjectives: []*armsql.ServiceObjectiveCapability{ + { + SKU: &armsql.SKU{Name: &skuName}, + }, + }, + }, + }, + }, + }, + }, + }, + } + client.SetCapabilitiesClient(mockClient) + + rec := mocks.BuildLegacyReservationRecommendation( + mocks.WithRegion("eastus"), + mocks.WithNormalizedSize(skuName), + ) + out := client.convertAzureSQLRecommendation(context.Background(), rec) + require.NotNil(t, out) + details, ok := out.Details.(common.DatabaseDetails) + require.True(t, ok) + assert.Equal(t, "sqlserver", details.Engine) + assert.Equal(t, skuName, details.InstanceClass) + assert.Equal(t, "12.0", details.EngineVersion, "EngineVersion must be enriched from the cached SKU catalogue") +} + +// TestDatabaseClient_ConvertAzureSQLRecommendation_CapabilitiesErrorFallsBack +// asserts that a capabilities-listing failure does NOT fail the +// conversion — EngineVersion just stays empty. +func TestDatabaseClient_ConvertAzureSQLRecommendation_CapabilitiesErrorFallsBack(t *testing.T) { + client := NewClient(nil, "test-subscription", "eastus") + client.SetCapabilitiesClient(&MockCapabilitiesClient{ + err: errors.New("transient Azure API error"), + }) + + rec := mocks.BuildLegacyReservationRecommendation( + mocks.WithRegion("eastus"), + mocks.WithNormalizedSize("GeneralPurpose_Gen5_2"), + ) + out := client.convertAzureSQLRecommendation(context.Background(), rec) + require.NotNil(t, out, "conversion must NOT fail on capabilities-fetch error") + details, ok := out.Details.(common.DatabaseDetails) + require.True(t, ok) + assert.Equal(t, "sqlserver", details.Engine) + assert.Equal(t, "GeneralPurpose_Gen5_2", details.InstanceClass) + assert.Empty(t, details.EngineVersion, "EngineVersion empty when capabilities fetch fails") +} + +// TestDatabaseClient_CachedSKULookup_FetchedOnce pins the perf invariant: +// many converter calls in the same GetRecommendations run trigger +// exactly ONE armsql.ListByLocation call. +func TestDatabaseClient_CachedSKULookup_FetchedOnce(t *testing.T) { + client := NewClient(nil, "test-subscription", "eastus") + + skuName := "GeneralPurpose_Gen5_2" + versionName := "12.0" + mockClient := &MockCapabilitiesClient{ + response: armsql.CapabilitiesClientListByLocationResponse{ + LocationCapabilities: armsql.LocationCapabilities{ + SupportedServerVersions: []*armsql.ServerVersionCapability{ + { + Name: &versionName, + SupportedEditions: []*armsql.EditionCapability{ + { + SupportedServiceLevelObjectives: []*armsql.ServiceObjectiveCapability{ + {SKU: &armsql.SKU{Name: &skuName}}, + }, + }, + }, + }, + }, + }, + }, + } + client.SetCapabilitiesClient(mockClient) + + for i := 0; i < 10; i++ { + _, _ = client.cachedSKULookup(context.Background(), skuName) + } + assert.Equal(t, 1, mockClient.CallCount, "capabilities catalogue must be fetched ONCE regardless of lookup count") +} + // MockTokenCredential for testing PurchaseCommitment type MockTokenCredential struct { token string