diff --git a/CHANGELOG.md b/CHANGELOG.md index 866981b9..377ca10e 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -304,9 +304,22 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/). longer compiles; keyed literals are unaffected. The field is true when a provider adopted a commitment that already existed for the same idempotency token (#211) +- **Breaking (source):** `common.PurchaseResult` gained an `OfferingClass` field. + Downstream code that builds it with unkeyed composite literals no longer + compiles; keyed literals are unaffected. On an adopted re-drive it carries the + offering class AWS reports for the existing RI (the EC2 provider sets it in a + follow-up change); empty otherwise (#211). ### Fixed +- **`common`:** a `Recommendation`, and a `PurchaseResult` carrying one (with + no `Error`), now decodes from JSON when `details` holds an object. `details` + is decoded into the typed pointer for the recommendation's service through + `DecodeServiceDetailsFor`; absent `details` leave the field unchanged and + null sets it nil, as encoding/json does, and details for a service with no + typed details fail with `ErrUntypedServiceDetails`. Before this, + encoding/json could not decode an object into the `ServiceDetails` interface + (#211). - **Breaking (source):** `managedredis.ReservationsDetailsPager` and `(*managedredis.Client).SetReservationsPager` are removed; tests inject reservation inventory with `SetInventoryFactories` instead (#699). - **`azure` Redis:** `managedredis.GetExistingCommitments` now reads held reservations from the Reservations API (purchase date, quantity, location, state), so a Redis reservation bought in the last day reduces the matching recommendation instead of being invisible to the duplicate guard (#699). An incomplete live reservation row (missing SKU, location, quantity or purchase date, an unattributable scope, or an expiry falling today) now fails the lookup with an error instead of being skipped, as `azure` compute already does. - **`aws` RI recommendations:** a fractional Cost Explorer quantity now rounds diff --git a/pkg/common/service_details_codec.go b/pkg/common/service_details_codec.go index bffef24f..b2f83c48 100644 --- a/pkg/common/service_details_codec.go +++ b/pkg/common/service_details_codec.go @@ -172,3 +172,32 @@ func newDetailsForService(service string) (ServiceDetails, bool) { } return nil, false } + +// UnmarshalJSON decodes details into the typed pointer for the +// recommendation's service (DecodeServiceDetailsFor), so a Recommendation, and +// a PurchaseResult carrying one, round-trips through JSON. encoding/json cannot +// decode an object into the ServiceDetails interface on its own. As with +// encoding/json, absent details leave Details unchanged and null sets it nil. +func (r *Recommendation) UnmarshalJSON(data []byte) error { + type plain Recommendation + aux := struct { + *plain + Details json.RawMessage `json:"details,omitempty"` + }{plain: (*plain)(r)} + if err := json.Unmarshal(data, &aux); err != nil { + return err + } + if len(aux.Details) == 0 { + return nil + } + if bytes.Equal(aux.Details, jsonNullBytes) { + r.Details = nil + return nil + } + details, err := DecodeServiceDetailsFor(string(r.Service), aux.Details) + if err != nil { + return err + } + r.Details = details + return nil +} diff --git a/pkg/common/service_details_codec_test.go b/pkg/common/service_details_codec_test.go index 7786091d..f428855a 100644 --- a/pkg/common/service_details_codec_test.go +++ b/pkg/common/service_details_codec_test.go @@ -310,3 +310,66 @@ func TestDecodeServiceDetailsFor_EveryEmittedPair(t *testing.T) { }) } } + +func TestRecommendation_DetailsJSONRoundTrip(t *testing.T) { + t.Parallel() + for _, tt := range []struct { + name string + rec Recommendation + details ServiceDetails + }{ + {"ec2 compute", Recommendation{Service: ServiceEC2, Details: &ComputeDetails{InstanceType: "m5.large", Platform: "Linux/UNIX", Tenancy: "default", Scope: "Region"}}, &ComputeDetails{InstanceType: "m5.large", Platform: "Linux/UNIX", Tenancy: "default", Scope: "Region"}}, + {"value compute decodes to a pointer", Recommendation{Service: ServiceCompute, Details: ComputeDetails{InstanceType: "n2-standard-4"}}, &ComputeDetails{InstanceType: "n2-standard-4"}}, + {"rds database", Recommendation{Service: ServiceRDS, Details: &DatabaseDetails{Engine: "postgres", AZConfig: "multi-az", InstanceClass: "db.r6g.large"}}, &DatabaseDetails{Engine: "postgres", AZConfig: "multi-az", InstanceClass: "db.r6g.large"}}, + {"no details stays nil", Recommendation{Service: ServiceEC2}, nil}, + } { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + payload, err := json.Marshal(tt.rec) + require.NoError(t, err) + var got Recommendation + require.NoError(t, json.Unmarshal(payload, &got)) + assert.Equal(t, tt.details, got.Details) + assert.Equal(t, tt.rec.Service, got.Service) + }) + } + + t.Run("explicit null details stays nil", func(t *testing.T) { + t.Parallel() + var got Recommendation + require.NoError(t, json.Unmarshal([]byte(`{"service":"ec2","details":null}`), &got)) + assert.Nil(t, got.Details) + }) + + t.Run("decoding into a filled recommendation follows encoding/json", func(t *testing.T) { + t.Parallel() + prior := &ComputeDetails{InstanceType: "m5.large"} + absent := Recommendation{Service: ServiceEC2, Details: prior} + require.NoError(t, json.Unmarshal([]byte(`{"count":2}`), &absent)) + assert.Same(t, prior, absent.Details, "absent details leave Details unchanged") + null := Recommendation{Service: ServiceEC2, Details: prior} + require.NoError(t, json.Unmarshal([]byte(`{"details":null}`), &null)) + assert.Nil(t, null.Details, "null details set Details nil") + }) + + t.Run("details for a service with no typed details fail", func(t *testing.T) { + t.Parallel() + var got Recommendation + err := json.Unmarshal([]byte(`{"service":"no-such-service","details":{"x":1}}`), &got) + require.ErrorIs(t, err, ErrUntypedServiceDetails) + }) + + t.Run("purchase result with details keeps its offering class", func(t *testing.T) { + t.Parallel() + in := PurchaseResult{ + Success: true, CommitmentID: "ri-1", ExistingCommitment: true, OfferingClass: "convertible", + Recommendation: Recommendation{Service: ServiceEC2, Details: &ComputeDetails{InstanceType: "m5.large"}}, + } + payload, err := json.Marshal(in) + require.NoError(t, err) + var got PurchaseResult + require.NoError(t, json.Unmarshal(payload, &got)) + assert.Equal(t, "convertible", got.OfferingClass) + assert.Equal(t, &ComputeDetails{InstanceType: "m5.large"}, got.Recommendation.Details) + }) +} diff --git a/pkg/common/types.go b/pkg/common/types.go index 942f2f60..4a78ea65 100644 --- a/pkg/common/types.go +++ b/pkg/common/types.go @@ -390,10 +390,21 @@ type PurchaseResult struct { Error error `json:"error,omitempty"` // Cost is the total upfront charge for the whole purchase (unit price x count). - // Nil means unknown; a non-nil zero means no upfront charge. + // Nil means unknown; a non-nil zero means no upfront charge. On an adopted + // re-drive (ExistingCommitment) the original purchase's charge is not known: + // the AWS RI services (EC2, RDS, ElastiCache, MemoryDB, OpenSearch, Redshift) + // return nil. GCP Compute Engine reports 0 today and returns nil after a + // follow-up change for #211. Azure reports 0 for a monthly billing plan and + // nil for an upfront one, as on a fresh purchase. Cost *float64 `json:"cost"` - CommitmentID string `json:"commitment_id,omitempty"` + CommitmentID string `json:"commitment_id,omitempty"` + + // OfferingClass is, on an adopted re-drive, the offering class AWS reports + // for the existing commitment; empty otherwise. The EC2 provider sets it in + // a follow-up change for #211. + OfferingClass string `json:"offering_class,omitempty"` + Recommendation Recommendation `json:"recommendation"` Success bool `json:"success"` DryRun bool `json:"dry_run"` @@ -401,8 +412,8 @@ type PurchaseResult struct { // ExistingCommitment is true when the provider adopted a commitment that // already existed for the same idempotency token (a re-drive of an earlier // attempt) instead of creating a new one. Success stays true and - // CommitmentID names the adopted commitment. Cost semantics on adoption are - // unspecified until the provider changes land. False on a fresh purchase. + // CommitmentID names the adopted commitment. See Cost for its value on + // adoption. False on a fresh purchase. ExistingCommitment bool `json:"existing_commitment,omitempty"` } diff --git a/pkg/common/types_test.go b/pkg/common/types_test.go index 3df32a46..61c5a58e 100644 --- a/pkg/common/types_test.go +++ b/pkg/common/types_test.go @@ -45,6 +45,19 @@ func TestPurchaseResult_ExistingCommitmentJSON(t *testing.T) { assert.True(t, decoded.ExistingCommitment) } +func TestPurchaseResult_OfferingClassJSON(t *testing.T) { + empty, err := json.Marshal(PurchaseResult{Success: true, CommitmentID: "ri-1"}) + require.NoError(t, err) + assert.NotContains(t, string(empty), "offering_class") + + set, err := json.Marshal(PurchaseResult{Success: true, CommitmentID: "ri-1", ExistingCommitment: true, OfferingClass: "convertible"}) + require.NoError(t, err) + assert.Contains(t, string(set), `"offering_class":"convertible"`) + var decoded PurchaseResult + require.NoError(t, json.Unmarshal(set, &decoded)) + assert.Equal(t, "convertible", decoded.OfferingClass) +} + func TestRecommendation_CommitmentCurrencyJSONAndScaling(t *testing.T) { t.Parallel() for _, tt := range []struct {