From 852fdf44beaefee0965d7e720c996aefd099a5a5 Mon Sep 17 00:00:00 2001 From: MegaportPhilipBrowne Date: Fri, 10 Jul 2026 13:00:27 -0700 Subject: [PATCH 01/11] ESD-1647: Close input-mode divergences across buy and update commands Bring interactive, flag, and JSON input modes to parity across eight commands so each validates, resolves, and gates inputs the same way regardless of mode. - nat gateway: reject empty tag keys on flag paths (already done on JSON) - managed account: validate required fields in the JSON path - vxc buy: resolve partner-port UIDs in the JSON path like the flags path - mve update: gate the name update on an explicit flag/JSON change, not a non-empty check - ix update: require at least one field before issuing the update - vxc update prompts: reject an update where nothing changed or the new name is empty - nat gateway update: keep the current promo code and service level reference when left blank, instead of relying on server-side omitempty - partners search: normalize the interactive output-format input (case, whitespace, unsupported values falling back to table) --- internal/commands/ix/ix_inputs.go | 6 ++ internal/commands/ix/ix_test.go | 14 ++-- .../managed_account/managed_account_inputs.go | 11 ++- .../managed_account_inputs_test.go | 18 ++--- internal/commands/mve/mve_inputs.go | 16 +++- internal/commands/mve/mve_inputs_test.go | 14 ++++ .../nat_gateway/nat_gateway_actions.go | 6 ++ .../nat_gateway_additional_test.go | 25 ++++++ .../nat_gateway/nat_gateway_inputs.go | 13 ++- .../commands/partners/partners_actions.go | 9 ++- .../partners/partners_actions_test.go | 80 +++++++++++++++++-- internal/commands/vxc/vxc_actions.go | 4 +- internal/commands/vxc/vxc_inputs.go | 51 ++++++++---- internal/commands/vxc/vxc_inputs_test.go | 43 +++++++++- internal/commands/vxc/vxc_prompts.go | 22 +++++ internal/commands/vxc/vxc_prompts_test.go | 37 ++++----- 16 files changed, 299 insertions(+), 70 deletions(-) diff --git a/internal/commands/ix/ix_inputs.go b/internal/commands/ix/ix_inputs.go index c44cecf3..df11b0b8 100644 --- a/internal/commands/ix/ix_inputs.go +++ b/internal/commands/ix/ix_inputs.go @@ -183,5 +183,11 @@ func buildUpdateIXRequestFromJSON(jsonStr, jsonFile string) (*megaport.UpdateIXR } } + if req.Name == nil && req.RateLimit == nil && req.CostCentre == nil && req.VLAN == nil && + req.MACAddress == nil && req.ASN == nil && req.Password == nil && req.PublicGraph == nil && + req.ReverseDns == nil && req.AEndProductUid == nil && req.Shutdown == nil { + return nil, exitcodes.NewUsageError(fmt.Errorf("at least one field must be updated")) + } + return req, nil } diff --git a/internal/commands/ix/ix_test.go b/internal/commands/ix/ix_test.go index c4e853f3..88e1ae62 100644 --- a/internal/commands/ix/ix_test.go +++ b/internal/commands/ix/ix_test.go @@ -1134,12 +1134,14 @@ func TestBuildUpdateIXRequestFromJSON(t *testing.T) { expectedError: "failed to parse JSON", }, { - name: "empty JSON object", - jsonStr: `{}`, - validate: func(t *testing.T, req *megaport.UpdateIXRequest) { - assert.Nil(t, req.Name) - assert.Nil(t, req.RateLimit) - }, + name: "empty JSON object", + jsonStr: `{}`, + expectedError: "at least one field must be updated", + }, + { + name: "misspelled key matches no known field", + jsonStr: `{"nam":"Updated IX"}`, + expectedError: "at least one field must be updated", }, { name: "valid JSON file", diff --git a/internal/commands/managed_account/managed_account_inputs.go b/internal/commands/managed_account/managed_account_inputs.go index 601f4731..2f25f17f 100644 --- a/internal/commands/managed_account/managed_account_inputs.go +++ b/internal/commands/managed_account/managed_account_inputs.go @@ -38,7 +38,16 @@ func buildManagedAccountRequestFromFlags(cmd *cobra.Command) (*megaport.ManagedA } func buildManagedAccountRequestFromJSON(jsonStr, jsonFile string) (*megaport.ManagedAccountRequest, error) { - return parseManagedAccountRequestJSON(jsonStr, jsonFile) + req, err := parseManagedAccountRequestJSON(jsonStr, jsonFile) + if err != nil { + return nil, err + } + + if req.AccountName == "" || req.AccountRef == "" { + return nil, exitcodes.NewUsageError(fmt.Errorf("account-name and account-ref are required")) + } + + return req, nil } // buildUpdateManagedAccountRequestFromFlags seeds the request from the current diff --git a/internal/commands/managed_account/managed_account_inputs_test.go b/internal/commands/managed_account/managed_account_inputs_test.go index cccbb4d9..5f0e7584 100644 --- a/internal/commands/managed_account/managed_account_inputs_test.go +++ b/internal/commands/managed_account/managed_account_inputs_test.go @@ -148,12 +148,9 @@ func TestBuildManagedAccountRequestFromJSON(t *testing.T) { }, }, { - name: "valid JSON string with partial fields", - jsonStr: `{"accountName":"Partial Account"}`, - validate: func(t *testing.T, req *megaport.ManagedAccountRequest) { - assert.Equal(t, "Partial Account", req.AccountName) - assert.Equal(t, "", req.AccountRef) - }, + name: "JSON string missing account-ref", + jsonStr: `{"accountName":"Partial Account"}`, + expectedError: "account-name and account-ref are required", }, { name: "invalid JSON syntax", @@ -161,12 +158,9 @@ func TestBuildManagedAccountRequestFromJSON(t *testing.T) { expectedError: "failed to parse JSON", }, { - name: "empty JSON object", - jsonStr: `{}`, - validate: func(t *testing.T, req *megaport.ManagedAccountRequest) { - assert.Equal(t, "", req.AccountName) - assert.Equal(t, "", req.AccountRef) - }, + name: "empty JSON object", + jsonStr: `{}`, + expectedError: "account-name and account-ref are required", }, { name: "valid JSON file", diff --git a/internal/commands/mve/mve_inputs.go b/internal/commands/mve/mve_inputs.go index 2a9dced7..a8769457 100644 --- a/internal/commands/mve/mve_inputs.go +++ b/internal/commands/mve/mve_inputs.go @@ -685,9 +685,15 @@ func processJSONUpdateMVEInput(jsonStr, jsonFilePath, mveUID string) (*megaport. MVEID: mveUID, } + // Gate on presence, not a non-empty value, so an explicit "name": "" is + // rejected deterministically instead of being silently treated the same + // as the key being absent, matching the flag path. if name, present, err := utils.JSONString(jsonData, "name"); err != nil { return nil, false, err - } else if present && name != "" { + } else if present { + if name == "" { + return nil, false, fmt.Errorf("name cannot be empty") + } req.Name = name } @@ -748,7 +754,13 @@ func processFlagUpdateMVEInput(cmd *cobra.Command, mveUID string) (*megaport.Mod MVEID: mveUID, } - if name != "" { + // Gate on Changed, not a non-empty value, so an explicit --name "" is + // rejected deterministically instead of being silently treated the same + // as the flag not being passed at all. + if cmd.Flags().Changed("name") { + if name == "" { + return nil, false, fmt.Errorf("name cannot be empty") + } req.Name = name } diff --git a/internal/commands/mve/mve_inputs_test.go b/internal/commands/mve/mve_inputs_test.go index 31891352..b2fd8c63 100644 --- a/internal/commands/mve/mve_inputs_test.go +++ b/internal/commands/mve/mve_inputs_test.go @@ -630,6 +630,20 @@ func TestProcessFlagUpdateMVEInput(t *testing.T) { } } +func TestProcessFlagUpdateMVEInput_ExplicitEmptyNameRejected(t *testing.T) { + cmd := createTestCmd() + require.NoError(t, cmd.Flags().Set("name", "")) + _, _, err := processFlagUpdateMVEInput(cmd, "mve-123") + require.Error(t, err) + assert.Contains(t, err.Error(), "name cannot be empty") +} + +func TestProcessJSONUpdateMVEInput_ExplicitEmptyNameRejected(t *testing.T) { + _, _, err := processJSONUpdateMVEInput(`{"name":""}`, "", "mve-123") + require.Error(t, err) + assert.Contains(t, err.Error(), "name cannot be empty") +} + func TestProcessFlagUpdateMVEInput_CostCentreProvided(t *testing.T) { t.Run("flag not set reports not provided", func(t *testing.T) { cmd := createTestCmd() diff --git a/internal/commands/nat_gateway/nat_gateway_actions.go b/internal/commands/nat_gateway/nat_gateway_actions.go index f3e9c235..13240b81 100644 --- a/internal/commands/nat_gateway/nat_gateway_actions.go +++ b/internal/commands/nat_gateway/nat_gateway_actions.go @@ -363,6 +363,12 @@ func mergeUpdateDefaults(req *megaport.UpdateNATGatewayRequest, original *megapo if req.Term == 0 { req.Term = original.Term } + if req.PromoCode == "" { + req.PromoCode = original.PromoCode + } + if req.ServiceLevelReference == "" { + req.ServiceLevelReference = original.ServiceLevelReference + } if !explicit.SessionCount && req.Config.SessionCount == 0 { req.Config.SessionCount = original.Config.SessionCount } diff --git a/internal/commands/nat_gateway/nat_gateway_additional_test.go b/internal/commands/nat_gateway/nat_gateway_additional_test.go index 4ff4bb44..d2b0e2e3 100644 --- a/internal/commands/nat_gateway/nat_gateway_additional_test.go +++ b/internal/commands/nat_gateway/nat_gateway_additional_test.go @@ -615,6 +615,31 @@ func TestMergeUpdateDefaultsExplicitBools(t *testing.T) { }) } +func TestMergeUpdateDefaultsPromoCodeAndServiceLevelReference(t *testing.T) { + original := &megaport.NATGateway{ + ProductName: "Original", + LocationID: 1, + Speed: 1000, + Term: 12, + PromoCode: "ORIGINAL-PROMO", + ServiceLevelReference: "ORIGINAL-SLR", + } + + t.Run("blank promo code and service level reference keep current value", func(t *testing.T) { + req := &megaport.UpdateNATGatewayRequest{ProductUID: "uid"} + mergeUpdateDefaults(req, original, updateExplicitFields{}) + assert.Equal(t, "ORIGINAL-PROMO", req.PromoCode, "blank promo code must inherit from original") + assert.Equal(t, "ORIGINAL-SLR", req.ServiceLevelReference, "blank service level reference must inherit from original") + }) + + t.Run("provided promo code and service level reference are not overridden", func(t *testing.T) { + req := &megaport.UpdateNATGatewayRequest{ProductUID: "uid", PromoCode: "NEW-PROMO", ServiceLevelReference: "NEW-SLR"} + mergeUpdateDefaults(req, original, updateExplicitFields{}) + assert.Equal(t, "NEW-PROMO", req.PromoCode) + assert.Equal(t, "NEW-SLR", req.ServiceLevelReference) + }) +} + func TestProcessFlagCreateNATGatewayInput(t *testing.T) { cmd := newTestCmd("create") require.NoError(t, cmd.Flags().Set("name", "Flag GW")) diff --git a/internal/commands/nat_gateway/nat_gateway_inputs.go b/internal/commands/nat_gateway/nat_gateway_inputs.go index 23aa69cb..4d8897b0 100644 --- a/internal/commands/nat_gateway/nat_gateway_inputs.go +++ b/internal/commands/nat_gateway/nat_gateway_inputs.go @@ -95,6 +95,9 @@ func processFlagCreateNATGatewayInput(cmd *cobra.Command) (*megaport.CreateNATGa if err := json.Unmarshal(tagData, &tagsMap); err != nil { return nil, fmt.Errorf("failed to parse resource tags JSON: %w", err) } + if err := utils.RejectEmptyTagKeys(tagsMap); err != nil { + return nil, err + } for k, v := range tagsMap { resourceTags = append(resourceTags, megaport.ResourceTag{Key: k, Value: v}) } @@ -132,9 +135,10 @@ func processFlagCreateNATGatewayInput(cmd *cobra.Command) (*megaport.CreateNATGa // BGPShutdownDefault *bool) get a corresponding entry here because their zero // value is ambiguous. Fields with genuinely invalid zero values // (LocationID, Speed, Term — all must be positive) do not need tracking -// because 0 can only mean "omitted". Fields with omitempty on the SDK request -// (PromoCode, ServiceLevelReference, ResourceTags) are safe to leave as plain -// strings/slices because the API ignores empty/nil values for those. +// because 0 can only mean "omitted". PromoCode and ServiceLevelReference are +// likewise plain strings; mergeUpdateDefaults inherits them from the original +// resource when left blank rather than relying on server-side omitempty +// behavior. type updateExplicitFields struct { AutoRenewTerm bool // was autoRenewTerm present in input? BGPShutdownDefault bool // was bgpShutdownDefault present in input? @@ -259,6 +263,9 @@ func processFlagUpdateNATGatewayInput(cmd *cobra.Command, uid string) (*megaport if err := json.Unmarshal(tagData, &tagsMap); err != nil { return nil, fmt.Errorf("failed to parse resource tags JSON: %w", err) } + if err := utils.RejectEmptyTagKeys(tagsMap); err != nil { + return nil, err + } for k, v := range tagsMap { resourceTags = append(resourceTags, megaport.ResourceTag{Key: k, Value: v}) } diff --git a/internal/commands/partners/partners_actions.go b/internal/commands/partners/partners_actions.go index 47e21f06..81816c01 100644 --- a/internal/commands/partners/partners_actions.go +++ b/internal/commands/partners/partners_actions.go @@ -2,6 +2,7 @@ package partners import ( "fmt" + "strings" "github.com/megaport/megaport-cli/internal/base/output" "github.com/megaport/megaport-cli/internal/commands/config" @@ -106,15 +107,17 @@ func FindPartners(cmd *cobra.Command, args []string, noColor bool) error { return err } - format, err := utils.Prompt("Output format [table/json] (default: table): ", noColor) + format, err := utils.Prompt("Output format [table/json/csv/xml] (default: table): ", noColor) if err != nil { output.PrintError("Failed to get output format: %v", noColor, err) return err } + normalizedFormat := strings.ToLower(strings.TrimSpace(format)) selectedFormat := "table" - if format == "json" { - selectedFormat = "json" + switch normalizedFormat { + case "json", "csv", "xml": + selectedFormat = normalizedFormat } filteredPartners := filterPartners(partners, productName, connectType, companyName, locationID, diversityZone) diff --git a/internal/commands/partners/partners_actions_test.go b/internal/commands/partners/partners_actions_test.go index fa03a206..7ff5bc40 100644 --- a/internal/commands/partners/partners_actions_test.go +++ b/internal/commands/partners/partners_actions_test.go @@ -29,11 +29,12 @@ func TestFindPartners(t *testing.T) { }() tests := []struct { - name string - prompts []string - expectedError string - setupMock func(*testing.T, *MockPartnerService) - expectedCount int + name string + prompts []string + expectedError string + setupMock func(*testing.T, *MockPartnerService) + expectedCount int + expectedFormat string }{ { name: "successful search with all filters", @@ -129,6 +130,70 @@ func TestFindPartners(t *testing.T) { }, expectedCount: 0, }, + { + name: "uppercase format is normalized", + prompts: []string{ + "", "", "", "", "", + "JSON", + }, + expectedError: "", + setupMock: func(t *testing.T, m *MockPartnerService) { + m.listPartnersResponse = []*megaport.PartnerMegaport{ + {ProductName: "Test Product", ConnectType: "AWS", CompanyName: "Amazon", LocationId: 123, DiversityZone: "blue"}, + } + m.listPartnersErr = nil + }, + expectedCount: 1, + expectedFormat: "json", + }, + { + name: "format with surrounding whitespace is normalized", + prompts: []string{ + "", "", "", "", "", + " table ", + }, + expectedError: "", + setupMock: func(t *testing.T, m *MockPartnerService) { + m.listPartnersResponse = []*megaport.PartnerMegaport{ + {ProductName: "Test Product", ConnectType: "AWS", CompanyName: "Amazon", LocationId: 123, DiversityZone: "blue"}, + } + m.listPartnersErr = nil + }, + expectedCount: 1, + expectedFormat: "table", + }, + { + name: "csv format is honored", + prompts: []string{ + "", "", "", "", "", + "csv", + }, + expectedError: "", + setupMock: func(t *testing.T, m *MockPartnerService) { + m.listPartnersResponse = []*megaport.PartnerMegaport{ + {ProductName: "Test Product", ConnectType: "AWS", CompanyName: "Amazon", LocationId: 123, DiversityZone: "blue"}, + } + m.listPartnersErr = nil + }, + expectedCount: 1, + expectedFormat: "csv", + }, + { + name: "unrecognized format falls back to table", + prompts: []string{ + "", "", "", "", "", + "yaml", + }, + expectedError: "", + setupMock: func(t *testing.T, m *MockPartnerService) { + m.listPartnersResponse = []*megaport.PartnerMegaport{ + {ProductName: "Test Product", ConnectType: "AWS", CompanyName: "Amazon", LocationId: 123, DiversityZone: "blue"}, + } + m.listPartnersErr = nil + }, + expectedCount: 1, + expectedFormat: "table", + }, } for _, tt := range tests { @@ -158,8 +223,10 @@ func TestFindPartners(t *testing.T) { }) var capturedPartners []*megaport.PartnerMegaport + var capturedFormat string printPartnersFunc = func(partners []*megaport.PartnerMegaport, format string, noColor bool) error { capturedPartners = partners + capturedFormat = format return nil } @@ -173,6 +240,9 @@ func TestFindPartners(t *testing.T) { assert.NoError(t, err) assert.Equal(t, len(tt.prompts), promptIndex, "not all prompts were used") assert.Equal(t, tt.expectedCount, len(capturedPartners), "incorrect number of filtered partners") + if tt.expectedFormat != "" { + assert.Equal(t, tt.expectedFormat, capturedFormat, "output format not normalized as expected") + } } }) } diff --git a/internal/commands/vxc/vxc_actions.go b/internal/commands/vxc/vxc_actions.go index 981a8be1..02dae62e 100644 --- a/internal/commands/vxc/vxc_actions.go +++ b/internal/commands/vxc/vxc_actions.go @@ -253,7 +253,9 @@ func buildVXCRequest(cmd *cobra.Command, ctx context.Context, client *megaport.C cmd.Flags().Changed("a-end-vlan") || cmd.Flags().Changed("b-end-uid") || cmd.Flags().Changed("b-end-vlan") }, - FromJSON: buildVXCRequestFromJSON, + FromJSON: func(jsonStr, jsonFile string) (*megaport.BuyVXCRequest, error) { + return buildVXCRequestFromJSON(jsonStr, jsonFile, ctx, client.VXCService) + }, FromFlags: func() (*megaport.BuyVXCRequest, error) { return buildVXCRequestFromFlags(cmd, ctx, client.VXCService) }, diff --git a/internal/commands/vxc/vxc_inputs.go b/internal/commands/vxc/vxc_inputs.go index 63d2f59a..415376e1 100644 --- a/internal/commands/vxc/vxc_inputs.go +++ b/internal/commands/vxc/vxc_inputs.go @@ -212,7 +212,7 @@ func parseVXCEndpointConfig(endConfigRaw map[string]interface{}, endLabel string return config, nil } -func buildVXCRequestFromJSON(jsonStr string, jsonFilePath string) (*megaport.BuyVXCRequest, error) { +func buildVXCRequestFromJSON(jsonStr string, jsonFilePath string, ctx context.Context, svc megaport.VXCService) (*megaport.BuyVXCRequest, error) { if jsonStr == "" && jsonFilePath == "" { return nil, exitcodes.NewUsageError(fmt.Errorf("either json or json-file must be provided")) } @@ -228,17 +228,40 @@ func buildVXCRequestFromJSON(jsonStr string, jsonFilePath string) (*megaport.Buy return nil, exitcodes.NewUsageError(fmt.Errorf("failed to parse JSON: %w", err)) } + // Parse A-End configuration early: its partner config, if present, can + // resolve a missing top-level portUid the same way flags mode does. + var aEndConfig megaport.VXCOrderEndpointConfiguration + if aEndConfigRaw, present, err := utils.JSONObject(rawData, "aEndConfiguration"); err != nil { + return nil, exitcodes.NewUsageError(err) + } else if present { + aEndConfig, err = parseVXCEndpointConfig(aEndConfigRaw, "A-End") + if err != nil { + return nil, err + } + } + portUID, present, err := utils.JSONString(rawData, "portUid") if err != nil { return nil, exitcodes.NewUsageError(err) } if !present { - return nil, exitcodes.NewUsageError(validation.NewValidationError("portUid", "", "Port UID is required")) + if aEndConfig.PartnerConfig == nil { + return nil, exitcodes.NewUsageError(validation.NewValidationError("portUid", "", "Port UID is required")) + } + uid, err := resolvePartnerPortUID(ctx, svc, aEndConfig.PartnerConfig) + if err != nil { + return nil, fmt.Errorf("failed to look up A-End Partner Port: %w", err) + } + if uid == "" { + return nil, exitcodes.NewUsageError(validation.NewValidationError("portUid", "", "Port UID is required")) + } + portUID = uid } // Create the base request req := &megaport.BuyVXCRequest{ - PortUID: portUID, + PortUID: portUID, + AEndConfiguration: aEndConfig, } // Set simple fields @@ -301,18 +324,9 @@ func buildVXCRequestFromJSON(jsonStr string, jsonFilePath string) (*megaport.Buy req.ResourceTags = tags } - // Handle A-End configuration - if aEndConfigRaw, present, err := utils.JSONObject(rawData, "aEndConfiguration"); err != nil { - return nil, exitcodes.NewUsageError(err) - } else if present { - aEndConfig, err := parseVXCEndpointConfig(aEndConfigRaw, "A-End") - if err != nil { - return nil, err - } - req.AEndConfiguration = aEndConfig - } - - // Handle B-End configuration + // Handle B-End configuration. Resolve a missing productUID from the + // partner config the same way flags mode does, so the same logical + // purchase succeeds through either input mode. if bEndConfigRaw, present, err := utils.JSONObject(rawData, "bEndConfiguration"); err != nil { return nil, exitcodes.NewUsageError(err) } else if present { @@ -320,6 +334,13 @@ func buildVXCRequestFromJSON(jsonStr string, jsonFilePath string) (*megaport.Buy if err != nil { return nil, err } + if bEndConfig.ProductUID == "" && bEndConfig.PartnerConfig != nil { + uid, err := resolvePartnerPortUID(ctx, svc, bEndConfig.PartnerConfig) + if err != nil { + return nil, fmt.Errorf("failed to look up B-End Partner Port: %w", err) + } + bEndConfig.ProductUID = uid + } req.BEndConfiguration = bEndConfig } diff --git a/internal/commands/vxc/vxc_inputs_test.go b/internal/commands/vxc/vxc_inputs_test.go index 0f45ea69..622b0537 100644 --- a/internal/commands/vxc/vxc_inputs_test.go +++ b/internal/commands/vxc/vxc_inputs_test.go @@ -1288,7 +1288,7 @@ func TestBuildVXCRequestFromJSON(t *testing.T) { for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - result, err := buildVXCRequestFromJSON(tt.jsonStr, tt.jsonFilePath) + result, err := buildVXCRequestFromJSON(tt.jsonStr, tt.jsonFilePath, context.Background(), &MockVXCService{}) if tt.expectedError != "" { assert.Error(t, err) assert.Contains(t, err.Error(), tt.expectedError) @@ -1310,7 +1310,7 @@ func TestBuildVXCRequestFromJSON_RejectsEmptyTagKey(t *testing.T) { const payload = `{"portUid":"port-1","vxcName":"Test VXC","rateLimit":1000,"term":12,"resourceTags":{"":"x"},"bEndConfiguration":{"productUID":"port-2"}}` t.Run("via json", func(t *testing.T) { - _, err := buildVXCRequestFromJSON(payload, "") + _, err := buildVXCRequestFromJSON(payload, "", context.Background(), &MockVXCService{}) require.Error(t, err) assert.Contains(t, err.Error(), "tag key must not be empty") @@ -1327,7 +1327,7 @@ func TestBuildVXCRequestFromJSON_RejectsEmptyTagKey(t *testing.T) { require.NoError(t, err) require.NoError(t, tmp.Close()) - _, err = buildVXCRequestFromJSON("", tmp.Name()) + _, err = buildVXCRequestFromJSON("", tmp.Name(), context.Background(), &MockVXCService{}) require.Error(t, err) assert.Contains(t, err.Error(), "tag key must not be empty") @@ -1342,7 +1342,7 @@ func TestBuildVXCRequestFromJSON_EndpointConfigMalformedField(t *testing.T) { // top-level field checks in buildVXCRequestFromJSON. payload := `{"portUid":"port-1","rateLimit":1000,"term":12,"aEndConfiguration":{"vlan":"not-a-number"}}` - _, err := buildVXCRequestFromJSON(payload, "") + _, err := buildVXCRequestFromJSON(payload, "", context.Background(), &MockVXCService{}) require.Error(t, err) assert.Contains(t, err.Error(), "A-End") @@ -1351,6 +1351,41 @@ func TestBuildVXCRequestFromJSON_EndpointConfigMalformedField(t *testing.T) { assert.Equal(t, exitcodes.Usage, cliErr.Code) } +func TestBuildVXCRequestFromJSON_PartnerPortResolution(t *testing.T) { + t.Run("A-End portUid resolved from partner config when omitted", func(t *testing.T) { + payload := `{"vxcName":"Test VXC","rateLimit":1000,"term":12,"aEndConfiguration":{"partnerConfig":{"connectType":"AZURE","serviceKey":"azure-key"}},"bEndConfiguration":{"productUID":"port-2"}}` + svc := &MockVXCService{ + LookupPartnerPortsResponse: &megaport.LookupPartnerPortsResponse{ProductUID: "resolved-a-end"}, + } + + req, err := buildVXCRequestFromJSON(payload, "", context.Background(), svc) + require.NoError(t, err) + assert.Equal(t, "resolved-a-end", req.PortUID) + }) + + t.Run("A-End portUid lookup failure surfaces as error", func(t *testing.T) { + payload := `{"vxcName":"Test VXC","rateLimit":1000,"term":12,"aEndConfiguration":{"partnerConfig":{"connectType":"AZURE","serviceKey":"azure-key"}},"bEndConfiguration":{"productUID":"port-2"}}` + svc := &MockVXCService{ + LookupPartnerPortsError: fmt.Errorf("lookup failed"), + } + + _, err := buildVXCRequestFromJSON(payload, "", context.Background(), svc) + require.Error(t, err) + assert.Contains(t, err.Error(), "failed to look up A-End Partner Port") + }) + + t.Run("B-End productUID resolved from partner config when omitted", func(t *testing.T) { + payload := `{"portUid":"port-1","vxcName":"Test VXC","rateLimit":1000,"term":12,"bEndConfiguration":{"partnerConfig":{"connectType":"GOOGLE","pairingKey":"google-key"}}}` + svc := &MockVXCService{ + LookupPartnerPortsResponse: &megaport.LookupPartnerPortsResponse{ProductUID: "resolved-b-end"}, + } + + req, err := buildVXCRequestFromJSON(payload, "", context.Background(), svc) + require.NoError(t, err) + assert.Equal(t, "resolved-b-end", req.BEndConfiguration.ProductUID) + }) +} + func TestBuildUpdateVXCRequestFromJSON_PartnerConfigs(t *testing.T) { tests := []struct { name string diff --git a/internal/commands/vxc/vxc_prompts.go b/internal/commands/vxc/vxc_prompts.go index 2c0059fd..914b0bf9 100644 --- a/internal/commands/vxc/vxc_prompts.go +++ b/internal/commands/vxc/vxc_prompts.go @@ -245,6 +245,8 @@ var buildUpdateVXCRequestFromPrompt = func(ctx context.Context, client *megaport return nil, fmt.Errorf("failed to fetch VXC details: %w", err) } + fieldsUpdated := false + fmt.Fprintf(os.Stderr, "Current name: %s\n", vxc.Name) updateName, err := utils.ResourcePrompt("vxc", "Update name? (yes/no): ", noColor) if err != nil { @@ -255,7 +257,11 @@ var buildUpdateVXCRequestFromPrompt = func(ctx context.Context, client *megaport if err != nil { return nil, err } + if name == "" { + return nil, fmt.Errorf("name cannot be empty") + } req.Name = &name + fieldsUpdated = true } fmt.Fprintf(os.Stderr, "Current rate limit: %d Mbps\n", vxc.RateLimit) @@ -276,6 +282,7 @@ var buildUpdateVXCRequestFromPrompt = func(ctx context.Context, client *megaport return nil, err } req.RateLimit = &rateLimit + fieldsUpdated = true } fmt.Fprintf(os.Stderr, "Current term: %d months\n", vxc.ContractTermMonths) @@ -297,6 +304,7 @@ var buildUpdateVXCRequestFromPrompt = func(ctx context.Context, client *megaport fmt.Sprintf("must be 0, or one of: %v", validation.ValidContractTerms)) } req.Term = &term + fieldsUpdated = true } fmt.Fprintf(os.Stderr, "Current cost centre: %s\n", vxc.CostCentre) @@ -310,6 +318,7 @@ var buildUpdateVXCRequestFromPrompt = func(ctx context.Context, client *megaport return nil, err } req.CostCentre = &costCentre + fieldsUpdated = true } shutdownStatus := "No" @@ -328,6 +337,7 @@ var buildUpdateVXCRequestFromPrompt = func(ctx context.Context, client *megaport } shutdown := strings.ToLower(shutdownStr) == "yes" req.Shutdown = &shutdown + fieldsUpdated = true } fmt.Fprintf(os.Stderr, "Current A-End VLAN: %d\n", vxc.AEndConfiguration.VLAN) @@ -348,6 +358,7 @@ var buildUpdateVXCRequestFromPrompt = func(ctx context.Context, client *megaport return nil, err } req.AEndVLAN = &aEndVLAN + fieldsUpdated = true } fmt.Fprintf(os.Stderr, "Current B-End VLAN: %d\n", vxc.BEndConfiguration.VLAN) @@ -368,6 +379,7 @@ var buildUpdateVXCRequestFromPrompt = func(ctx context.Context, client *megaport return nil, err } req.BEndVLAN = &bEndVLAN + fieldsUpdated = true } innerVLANAEnd := 0 @@ -394,6 +406,7 @@ var buildUpdateVXCRequestFromPrompt = func(ctx context.Context, client *megaport } req.AEndInnerVLAN = &aEndInnerVLAN + fieldsUpdated = true } innerVLANBEnd := 0 @@ -420,6 +433,7 @@ var buildUpdateVXCRequestFromPrompt = func(ctx context.Context, client *megaport } req.BEndInnerVLAN = &bEndInnerVLAN + fieldsUpdated = true } fmt.Fprintf(os.Stderr, "Current A-End UID: %s\n", vxc.AEndConfiguration.UID) @@ -433,6 +447,7 @@ var buildUpdateVXCRequestFromPrompt = func(ctx context.Context, client *megaport return nil, err } req.AEndProductUID = &aEndUID + fieldsUpdated = true } fmt.Fprintf(os.Stderr, "Current B-End UID: %s\n", vxc.BEndConfiguration.UID) @@ -446,6 +461,7 @@ var buildUpdateVXCRequestFromPrompt = func(ctx context.Context, client *megaport return nil, err } req.BEndProductUID = &bEndUID + fieldsUpdated = true } wantsAEndPartnerConfig, err := utils.ResourcePrompt("vxc", "Do you want to configure an A-End VRouter partner configuration? (yes/no): ", noColor) @@ -459,6 +475,7 @@ var buildUpdateVXCRequestFromPrompt = func(ctx context.Context, client *megaport return nil, err } req.AEndPartnerConfig = aEndPartnerConfig + fieldsUpdated = true } wantsBEndPartnerConfig, err := utils.ResourcePrompt("vxc", "Do you want to configure a B-End VRouter partner configuration? (yes/no): ", noColor) @@ -472,6 +489,11 @@ var buildUpdateVXCRequestFromPrompt = func(ctx context.Context, client *megaport return nil, err } req.BEndPartnerConfig = bEndPartnerConfig + fieldsUpdated = true + } + + if !fieldsUpdated { + return nil, fmt.Errorf("at least one field must be updated") } return req, nil diff --git a/internal/commands/vxc/vxc_prompts_test.go b/internal/commands/vxc/vxc_prompts_test.go index 424e191a..4ab1f6b6 100644 --- a/internal/commands/vxc/vxc_prompts_test.go +++ b/internal/commands/vxc/vxc_prompts_test.go @@ -9,6 +9,7 @@ import ( "github.com/megaport/megaport-cli/internal/utils" megaport "github.com/megaport/megaportgo" "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" ) func mockPrompts(responses []string) func() { @@ -932,9 +933,10 @@ func TestBuildUpdateVXCRequestFromPrompt(t *testing.T) { } tests := []struct { - name string - responses []string - verify func(t *testing.T, req *megaport.UpdateVXCRequest) + name string + responses []string + expectedError string + verify func(t *testing.T, req *megaport.UpdateVXCRequest) }{ { name: "skip all", @@ -953,21 +955,7 @@ func TestBuildUpdateVXCRequestFromPrompt(t *testing.T) { "no", // A-End VRouter config "no", // B-End VRouter config }, - verify: func(t *testing.T, req *megaport.UpdateVXCRequest) { - assert.Nil(t, req.Name) - assert.Nil(t, req.RateLimit) - assert.Nil(t, req.Term) - assert.Nil(t, req.CostCentre) - assert.Nil(t, req.Shutdown) - assert.Nil(t, req.AEndVLAN) - assert.Nil(t, req.BEndVLAN) - assert.Nil(t, req.AEndInnerVLAN) - assert.Nil(t, req.BEndInnerVLAN) - assert.Nil(t, req.AEndProductUID) - assert.Nil(t, req.BEndProductUID) - assert.True(t, req.WaitForUpdate) - // WaitForTime is set by the caller (UpdateVXC), not the prompt builder - }, + expectedError: "at least one field must be updated", }, { name: "update name and rate limit", @@ -1035,6 +1023,14 @@ func TestBuildUpdateVXCRequestFromPrompt(t *testing.T) { assert.Equal(t, -1, vrouterCfg.Interfaces[0].VLAN) }, }, + { + name: "empty name rejected", + responses: []string{ + "yes", // update name + "", // new name (empty) + }, + expectedError: "name cannot be empty", + }, } for _, tc := range tests { @@ -1048,6 +1044,11 @@ func TestBuildUpdateVXCRequestFromPrompt(t *testing.T) { mockClient := &megaport.Client{VXCService: mockSvc} req, err := buildUpdateVXCRequestFromPrompt(context.Background(), mockClient, "vxc-uid-123", true) + if tc.expectedError != "" { + require.Error(t, err) + assert.Contains(t, err.Error(), tc.expectedError) + return + } assert.NoError(t, err) assert.NotNil(t, req) tc.verify(t, req) From 50a053ccf410b68f70014a80307a1a3a183e55d8 Mon Sep 17 00:00:00 2001 From: MegaportPhilipBrowne Date: Fri, 10 Jul 2026 13:09:02 -0700 Subject: [PATCH 02/11] ESD-1647: require B-End productUID when partner lookup yields none, add coverage Copilot review on PR #536 flagged that JSON-mode buy could send an empty B-End productUID when the partner config type doesn't support UID lookup (e.g. AWS), since ValidateVXCRequest only requires it when partnerConfig is nil. Match flags mode, which always requires a resolved UID. Also fills in patch coverage gaps flagged by Codecov: empty tag key rejection on both NAT gateway flag paths, the remaining VXC update prompt fields, and the new VXC JSON partner-port resolution branches. --- .../nat_gateway/nat_gateway_inputs_test.go | 20 +++++++ internal/commands/vxc/vxc_inputs.go | 3 + internal/commands/vxc/vxc_inputs_test.go | 27 +++++++++ internal/commands/vxc/vxc_prompts_test.go | 57 +++++++++++++++++++ 4 files changed, 107 insertions(+) diff --git a/internal/commands/nat_gateway/nat_gateway_inputs_test.go b/internal/commands/nat_gateway/nat_gateway_inputs_test.go index 91b5c527..d60b9bba 100644 --- a/internal/commands/nat_gateway/nat_gateway_inputs_test.go +++ b/internal/commands/nat_gateway/nat_gateway_inputs_test.go @@ -199,6 +199,18 @@ func TestProcessFlagCreateNATGatewayInput_ValidResourceTags(t *testing.T) { assert.Len(t, req.ResourceTags, 2) } +func TestProcessFlagCreateNATGatewayInput_RejectsEmptyTagKey(t *testing.T) { + cmd := newCreateFlagsCmd() + require.NoError(t, cmd.Flags().Set("name", "Tag GW")) + require.NoError(t, cmd.Flags().Set("term", "12")) + require.NoError(t, cmd.Flags().Set("speed", "1000")) + require.NoError(t, cmd.Flags().Set("location-id", "1")) + require.NoError(t, cmd.Flags().Set("resource-tags", `{"":"x"}`)) + + _, err := processFlagCreateNATGatewayInput(cmd) + assert.Error(t, err) +} + // ---- processJSONUpdateNATGatewayInput ---- func TestProcessJSONUpdateNATGatewayInput_RejectsEmptyTagKey(t *testing.T) { @@ -364,3 +376,11 @@ func TestProcessFlagUpdateNATGatewayInput_InvalidResourceTags(t *testing.T) { _, err := processFlagUpdateNATGatewayInput(cmd, "uid-bad") assert.Error(t, err) } + +func TestProcessFlagUpdateNATGatewayInput_RejectsEmptyTagKey(t *testing.T) { + cmd := newUpdateFlagsCmd() + require.NoError(t, cmd.Flags().Set("resource-tags", `{"":"x"}`)) + + _, err := processFlagUpdateNATGatewayInput(cmd, "uid-empty-tag") + assert.Error(t, err) +} diff --git a/internal/commands/vxc/vxc_inputs.go b/internal/commands/vxc/vxc_inputs.go index 415376e1..de5689fb 100644 --- a/internal/commands/vxc/vxc_inputs.go +++ b/internal/commands/vxc/vxc_inputs.go @@ -339,6 +339,9 @@ func buildVXCRequestFromJSON(jsonStr string, jsonFilePath string, ctx context.Co if err != nil { return nil, fmt.Errorf("failed to look up B-End Partner Port: %w", err) } + if uid == "" { + return nil, exitcodes.NewUsageError(fmt.Errorf("bEndConfiguration.productUID was neither provided nor could be looked up")) + } bEndConfig.ProductUID = uid } req.BEndConfiguration = bEndConfig diff --git a/internal/commands/vxc/vxc_inputs_test.go b/internal/commands/vxc/vxc_inputs_test.go index 622b0537..ab46d316 100644 --- a/internal/commands/vxc/vxc_inputs_test.go +++ b/internal/commands/vxc/vxc_inputs_test.go @@ -1384,6 +1384,33 @@ func TestBuildVXCRequestFromJSON_PartnerPortResolution(t *testing.T) { require.NoError(t, err) assert.Equal(t, "resolved-b-end", req.BEndConfiguration.ProductUID) }) + + t.Run("B-End productUID lookup failure surfaces as error", func(t *testing.T) { + payload := `{"portUid":"port-1","vxcName":"Test VXC","rateLimit":1000,"term":12,"bEndConfiguration":{"partnerConfig":{"connectType":"GOOGLE","pairingKey":"google-key"}}}` + svc := &MockVXCService{ + LookupPartnerPortsError: fmt.Errorf("lookup failed"), + } + + _, err := buildVXCRequestFromJSON(payload, "", context.Background(), svc) + require.Error(t, err) + assert.Contains(t, err.Error(), "failed to look up B-End Partner Port") + }) + + t.Run("A-End partner config that resolves to no UID still requires portUid", func(t *testing.T) { + payload := `{"vxcName":"Test VXC","rateLimit":1000,"term":12,"aEndConfiguration":{"partnerConfig":{"connectType":"AWS","ownerAccount":"123"}},"bEndConfiguration":{"productUID":"port-2"}}` + + _, err := buildVXCRequestFromJSON(payload, "", context.Background(), &MockVXCService{}) + require.Error(t, err) + assert.Contains(t, err.Error(), "Port UID is required") + }) + + t.Run("B-End partner config that resolves to no UID still requires productUID", func(t *testing.T) { + payload := `{"portUid":"port-1","vxcName":"Test VXC","rateLimit":1000,"term":12,"bEndConfiguration":{"partnerConfig":{"connectType":"AWS","ownerAccount":"123"}}}` + + _, err := buildVXCRequestFromJSON(payload, "", context.Background(), &MockVXCService{}) + require.Error(t, err) + assert.Contains(t, err.Error(), "productUID was neither provided nor could be looked up") + }) } func TestBuildUpdateVXCRequestFromJSON_PartnerConfigs(t *testing.T) { diff --git a/internal/commands/vxc/vxc_prompts_test.go b/internal/commands/vxc/vxc_prompts_test.go index 4ab1f6b6..cd19fb51 100644 --- a/internal/commands/vxc/vxc_prompts_test.go +++ b/internal/commands/vxc/vxc_prompts_test.go @@ -1031,6 +1031,63 @@ func TestBuildUpdateVXCRequestFromPrompt(t *testing.T) { }, expectedError: "name cannot be empty", }, + { + name: "update remaining fields", + responses: []string{ + "no", // update name + "no", // update rate limit + "yes", // update term + "24", // new term + "yes", // update cost centre + "New CC", // new cost centre + "yes", // update shutdown + "yes", // shut down the VXC + "yes", // update A-End VLAN + "101", // new A-End VLAN + "yes", // update B-End VLAN + "201", // new B-End VLAN + "yes", // update A-End inner VLAN + "301", // new A-End inner VLAN + "yes", // update B-End inner VLAN + "401", // new B-End inner VLAN + "yes", // update A-End product UID + "new-a-uid", // new A-End product UID + "yes", // update B-End product UID + "new-b-uid", // new B-End product UID + "no", // A-End VRouter config + "yes", // B-End VRouter config + "1", // number of interfaces + "", // VLAN (untagged) + "", // interface type (default subInterface) + "no", // add IP addresses + "no", // add IP routes + "no", // add NAT IPs + "no", // configure BFD + "no", // configure BGP + }, + verify: func(t *testing.T, req *megaport.UpdateVXCRequest) { + assert.NotNil(t, req.Term) + assert.Equal(t, 24, *req.Term) + assert.NotNil(t, req.CostCentre) + assert.Equal(t, "New CC", *req.CostCentre) + assert.NotNil(t, req.Shutdown) + assert.True(t, *req.Shutdown) + assert.NotNil(t, req.AEndVLAN) + assert.Equal(t, 101, *req.AEndVLAN) + assert.NotNil(t, req.BEndVLAN) + assert.Equal(t, 201, *req.BEndVLAN) + assert.NotNil(t, req.AEndInnerVLAN) + assert.Equal(t, 301, *req.AEndInnerVLAN) + assert.NotNil(t, req.BEndInnerVLAN) + assert.Equal(t, 401, *req.BEndInnerVLAN) + assert.NotNil(t, req.AEndProductUID) + assert.Equal(t, "new-a-uid", *req.AEndProductUID) + assert.NotNil(t, req.BEndProductUID) + assert.Equal(t, "new-b-uid", *req.BEndProductUID) + assert.Nil(t, req.AEndPartnerConfig) + assert.NotNil(t, req.BEndPartnerConfig) + }, + }, } for _, tc := range tests { From 6e25d4322ec9938df3a6c96a86841f771431ffa3 Mon Sep 17 00:00:00 2001 From: MegaportPhilipBrowne Date: Fri, 10 Jul 2026 13:16:42 -0700 Subject: [PATCH 03/11] ESD-1647: reference JSON keys in managed-account JSON validation error Copilot review on PR #536 noted the JSON-mode required-fields error used flag names (account-name/account-ref) instead of the JSON keys (accountName/accountRef), which is confusing when the input is a JSON body or file rather than flags. --- internal/commands/managed_account/managed_account_inputs.go | 2 +- .../commands/managed_account/managed_account_inputs_test.go | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/internal/commands/managed_account/managed_account_inputs.go b/internal/commands/managed_account/managed_account_inputs.go index 2f25f17f..86b3b6c8 100644 --- a/internal/commands/managed_account/managed_account_inputs.go +++ b/internal/commands/managed_account/managed_account_inputs.go @@ -44,7 +44,7 @@ func buildManagedAccountRequestFromJSON(jsonStr, jsonFile string) (*megaport.Man } if req.AccountName == "" || req.AccountRef == "" { - return nil, exitcodes.NewUsageError(fmt.Errorf("account-name and account-ref are required")) + return nil, exitcodes.NewUsageError(fmt.Errorf("accountName and accountRef are required")) } return req, nil diff --git a/internal/commands/managed_account/managed_account_inputs_test.go b/internal/commands/managed_account/managed_account_inputs_test.go index 5f0e7584..144fde9d 100644 --- a/internal/commands/managed_account/managed_account_inputs_test.go +++ b/internal/commands/managed_account/managed_account_inputs_test.go @@ -150,7 +150,7 @@ func TestBuildManagedAccountRequestFromJSON(t *testing.T) { { name: "JSON string missing account-ref", jsonStr: `{"accountName":"Partial Account"}`, - expectedError: "account-name and account-ref are required", + expectedError: "accountName and accountRef are required", }, { name: "invalid JSON syntax", @@ -160,7 +160,7 @@ func TestBuildManagedAccountRequestFromJSON(t *testing.T) { { name: "empty JSON object", jsonStr: `{}`, - expectedError: "account-name and account-ref are required", + expectedError: "accountName and accountRef are required", }, { name: "valid JSON file", From baf4d6b0a687f8968b7f07fbcbef2a50e89d3dcb Mon Sep 17 00:00:00 2001 From: MegaportPhilipBrowne Date: Fri, 10 Jul 2026 13:26:14 -0700 Subject: [PATCH 04/11] ESD-1647: validate VXC buy JSON format before A-End partner-port lookup Copilot review on PR #536 noted that resolvePartnerPortUID could run before vxcName/rateLimit/term were even parsed, wasting an API call on malformed JSON that would fail validation anyway. Move the rateLimit/ term whole-number checks (and vxcName parsing) ahead of the lookup so malformed JSON fails fast without the network round-trip. --- internal/commands/vxc/vxc_inputs.go | 51 ++++++++++++------------ internal/commands/vxc/vxc_inputs_test.go | 11 +++++ 2 files changed, 37 insertions(+), 25 deletions(-) diff --git a/internal/commands/vxc/vxc_inputs.go b/internal/commands/vxc/vxc_inputs.go index de5689fb..d9009a51 100644 --- a/internal/commands/vxc/vxc_inputs.go +++ b/internal/commands/vxc/vxc_inputs.go @@ -240,6 +240,29 @@ func buildVXCRequestFromJSON(jsonStr string, jsonFilePath string, ctx context.Co } } + // Parse and format-check vxcName/rateLimit/term before any partner-port + // lookup, so malformed JSON fails fast without an API round-trip. + vxcName, _, err := utils.JSONString(rawData, "vxcName") + if err != nil { + return nil, exitcodes.NewUsageError(err) + } + + rateLimit, _, err := utils.JSONNumber(rawData, "rateLimit") + if err != nil { + return nil, exitcodes.NewUsageError(err) + } + if rateLimit != math.Trunc(rateLimit) { + return nil, exitcodes.NewUsageError(fmt.Errorf("rateLimit must be a whole number, got %v", rateLimit)) + } + + term, _, err := utils.JSONNumber(rawData, "term") + if err != nil { + return nil, exitcodes.NewUsageError(err) + } + if term != math.Trunc(term) { + return nil, exitcodes.NewUsageError(fmt.Errorf("term must be a whole number, got %v", term)) + } + portUID, present, err := utils.JSONString(rawData, "portUid") if err != nil { return nil, exitcodes.NewUsageError(err) @@ -262,31 +285,9 @@ func buildVXCRequestFromJSON(jsonStr string, jsonFilePath string, ctx context.Co req := &megaport.BuyVXCRequest{ PortUID: portUID, AEndConfiguration: aEndConfig, - } - - // Set simple fields - if vxcName, present, err := utils.JSONString(rawData, "vxcName"); err != nil { - return nil, exitcodes.NewUsageError(err) - } else if present { - req.VXCName = vxcName - } - - if rateLimit, present, err := utils.JSONNumber(rawData, "rateLimit"); err != nil { - return nil, exitcodes.NewUsageError(err) - } else if present { - if rateLimit != math.Trunc(rateLimit) { - return nil, exitcodes.NewUsageError(fmt.Errorf("rateLimit must be a whole number, got %v", rateLimit)) - } - req.RateLimit = int(rateLimit) - } - - if term, present, err := utils.JSONNumber(rawData, "term"); err != nil { - return nil, exitcodes.NewUsageError(err) - } else if present { - if term != math.Trunc(term) { - return nil, exitcodes.NewUsageError(fmt.Errorf("term must be a whole number, got %v", term)) - } - req.Term = int(term) + VXCName: vxcName, + RateLimit: int(rateLimit), + Term: int(term), } if shutdown, present, err := utils.JSONBool(rawData, "shutdown"); err != nil { diff --git a/internal/commands/vxc/vxc_inputs_test.go b/internal/commands/vxc/vxc_inputs_test.go index ab46d316..7b153002 100644 --- a/internal/commands/vxc/vxc_inputs_test.go +++ b/internal/commands/vxc/vxc_inputs_test.go @@ -1374,6 +1374,17 @@ func TestBuildVXCRequestFromJSON_PartnerPortResolution(t *testing.T) { assert.Contains(t, err.Error(), "failed to look up A-End Partner Port") }) + t.Run("malformed rateLimit fails before A-End partner-port lookup", func(t *testing.T) { + payload := `{"vxcName":"Test VXC","rateLimit":1000.5,"term":12,"aEndConfiguration":{"partnerConfig":{"connectType":"AZURE","serviceKey":"azure-key"}},"bEndConfiguration":{"productUID":"port-2"}}` + svc := &MockVXCService{ + LookupPartnerPortsError: fmt.Errorf("lookup should not have been called"), + } + + _, err := buildVXCRequestFromJSON(payload, "", context.Background(), svc) + require.Error(t, err) + assert.Contains(t, err.Error(), "rateLimit must be a whole number") + }) + t.Run("B-End productUID resolved from partner config when omitted", func(t *testing.T) { payload := `{"portUid":"port-1","vxcName":"Test VXC","rateLimit":1000,"term":12,"bEndConfiguration":{"partnerConfig":{"connectType":"GOOGLE","pairingKey":"google-key"}}}` svc := &MockVXCService{ From 0add7bff5a7cdc450fdfbf858157dbbdd87afe7b Mon Sep 17 00:00:00 2001 From: MegaportPhilipBrowne Date: Fri, 10 Jul 2026 13:35:55 -0700 Subject: [PATCH 05/11] ESD-1647: enforce required VXC buy fields before partner-port lookups Copilot review on PR #536 pointed out that vxcName/rateLimit/term presence wasn't enforced before resolvePartnerPortUID, so a JSON body missing these fields but relying on a partner config for portUid or productUID would still trigger a network lookup and only fail later in ValidateVXCRequest. Added a requireBuyFields check called immediately before both the A-End and B-End lookups. --- internal/commands/vxc/vxc_inputs.go | 22 ++++++++++++++++++++++ internal/commands/vxc/vxc_inputs_test.go | 22 ++++++++++++++++++++++ 2 files changed, 44 insertions(+) diff --git a/internal/commands/vxc/vxc_inputs.go b/internal/commands/vxc/vxc_inputs.go index d9009a51..144446ea 100644 --- a/internal/commands/vxc/vxc_inputs.go +++ b/internal/commands/vxc/vxc_inputs.go @@ -263,6 +263,22 @@ func buildVXCRequestFromJSON(jsonStr string, jsonFilePath string, ctx context.Co return nil, exitcodes.NewUsageError(fmt.Errorf("term must be a whole number, got %v", term)) } + // Required fields the buy request can't do without, checked before any + // partner-port lookup so a partial JSON body fails fast rather than + // spending an API round-trip on a request that will fail validation anyway. + requireBuyFields := func() error { + if vxcName == "" { + return exitcodes.NewUsageError(validation.NewValidationError("VXC name", vxcName, "cannot be empty")) + } + if err := validation.ValidateRateLimit(int(rateLimit)); err != nil { + return exitcodes.NewUsageError(err) + } + if err := validation.ValidateContractTerm(int(term)); err != nil { + return exitcodes.NewUsageError(err) + } + return nil + } + portUID, present, err := utils.JSONString(rawData, "portUid") if err != nil { return nil, exitcodes.NewUsageError(err) @@ -271,6 +287,9 @@ func buildVXCRequestFromJSON(jsonStr string, jsonFilePath string, ctx context.Co if aEndConfig.PartnerConfig == nil { return nil, exitcodes.NewUsageError(validation.NewValidationError("portUid", "", "Port UID is required")) } + if err := requireBuyFields(); err != nil { + return nil, err + } uid, err := resolvePartnerPortUID(ctx, svc, aEndConfig.PartnerConfig) if err != nil { return nil, fmt.Errorf("failed to look up A-End Partner Port: %w", err) @@ -336,6 +355,9 @@ func buildVXCRequestFromJSON(jsonStr string, jsonFilePath string, ctx context.Co return nil, err } if bEndConfig.ProductUID == "" && bEndConfig.PartnerConfig != nil { + if err := requireBuyFields(); err != nil { + return nil, err + } uid, err := resolvePartnerPortUID(ctx, svc, bEndConfig.PartnerConfig) if err != nil { return nil, fmt.Errorf("failed to look up B-End Partner Port: %w", err) diff --git a/internal/commands/vxc/vxc_inputs_test.go b/internal/commands/vxc/vxc_inputs_test.go index 7b153002..ca1c8b65 100644 --- a/internal/commands/vxc/vxc_inputs_test.go +++ b/internal/commands/vxc/vxc_inputs_test.go @@ -1385,6 +1385,28 @@ func TestBuildVXCRequestFromJSON_PartnerPortResolution(t *testing.T) { assert.Contains(t, err.Error(), "rateLimit must be a whole number") }) + t.Run("missing vxcName fails before A-End partner-port lookup", func(t *testing.T) { + payload := `{"rateLimit":1000,"term":12,"aEndConfiguration":{"partnerConfig":{"connectType":"AZURE","serviceKey":"azure-key"}},"bEndConfiguration":{"productUID":"port-2"}}` + svc := &MockVXCService{ + LookupPartnerPortsError: fmt.Errorf("lookup should not have been called"), + } + + _, err := buildVXCRequestFromJSON(payload, "", context.Background(), svc) + require.Error(t, err) + assert.Contains(t, err.Error(), "VXC name") + }) + + t.Run("missing vxcName fails before B-End partner-port lookup", func(t *testing.T) { + payload := `{"portUid":"port-1","rateLimit":1000,"term":12,"bEndConfiguration":{"partnerConfig":{"connectType":"GOOGLE","pairingKey":"google-key"}}}` + svc := &MockVXCService{ + LookupPartnerPortsError: fmt.Errorf("lookup should not have been called"), + } + + _, err := buildVXCRequestFromJSON(payload, "", context.Background(), svc) + require.Error(t, err) + assert.Contains(t, err.Error(), "VXC name") + }) + t.Run("B-End productUID resolved from partner config when omitted", func(t *testing.T) { payload := `{"portUid":"port-1","vxcName":"Test VXC","rateLimit":1000,"term":12,"bEndConfiguration":{"partnerConfig":{"connectType":"GOOGLE","pairingKey":"google-key"}}}` svc := &MockVXCService{ From 73d9b6595569523f5d3a665639b56d9e9e6c42ef Mon Sep 17 00:00:00 2001 From: MegaportPhilipBrowne Date: Fri, 10 Jul 2026 13:56:14 -0700 Subject: [PATCH 06/11] fix(exit-codes): classify empty-name and empty-tag-key errors as usage errors nat_gateway's flags create/update paths and its JSON update path returned plain errors for an empty resource tag key, exiting general (1) instead of usage, unlike the JSON create path and every other resource's tag validation. mve's update paths and vxc's update prompt returned a plain error for an explicitly empty name instead of a validation.ValidationError, so they also exited general instead of usage, unlike the vxc buy prompt's equivalent check. --- internal/commands/mve/mve_inputs.go | 4 ++-- internal/commands/mve/mve_inputs_test.go | 4 ++-- .../commands/nat_gateway/nat_gateway_inputs.go | 6 +++--- .../nat_gateway/nat_gateway_inputs_test.go | 16 ++++++++++++++-- internal/commands/vxc/vxc_prompts.go | 2 +- internal/commands/vxc/vxc_prompts_test.go | 2 +- 6 files changed, 23 insertions(+), 11 deletions(-) diff --git a/internal/commands/mve/mve_inputs.go b/internal/commands/mve/mve_inputs.go index a8769457..aa8e6e6f 100644 --- a/internal/commands/mve/mve_inputs.go +++ b/internal/commands/mve/mve_inputs.go @@ -692,7 +692,7 @@ func processJSONUpdateMVEInput(jsonStr, jsonFilePath, mveUID string) (*megaport. return nil, false, err } else if present { if name == "" { - return nil, false, fmt.Errorf("name cannot be empty") + return nil, false, validation.NewValidationError("name", name, "cannot be empty") } req.Name = name } @@ -759,7 +759,7 @@ func processFlagUpdateMVEInput(cmd *cobra.Command, mveUID string) (*megaport.Mod // as the flag not being passed at all. if cmd.Flags().Changed("name") { if name == "" { - return nil, false, fmt.Errorf("name cannot be empty") + return nil, false, validation.NewValidationError("name", name, "cannot be empty") } req.Name = name } diff --git a/internal/commands/mve/mve_inputs_test.go b/internal/commands/mve/mve_inputs_test.go index b2fd8c63..9088fffe 100644 --- a/internal/commands/mve/mve_inputs_test.go +++ b/internal/commands/mve/mve_inputs_test.go @@ -635,13 +635,13 @@ func TestProcessFlagUpdateMVEInput_ExplicitEmptyNameRejected(t *testing.T) { require.NoError(t, cmd.Flags().Set("name", "")) _, _, err := processFlagUpdateMVEInput(cmd, "mve-123") require.Error(t, err) - assert.Contains(t, err.Error(), "name cannot be empty") + assert.Contains(t, err.Error(), "cannot be empty") } func TestProcessJSONUpdateMVEInput_ExplicitEmptyNameRejected(t *testing.T) { _, _, err := processJSONUpdateMVEInput(`{"name":""}`, "", "mve-123") require.Error(t, err) - assert.Contains(t, err.Error(), "name cannot be empty") + assert.Contains(t, err.Error(), "cannot be empty") } func TestProcessFlagUpdateMVEInput_CostCentreProvided(t *testing.T) { diff --git a/internal/commands/nat_gateway/nat_gateway_inputs.go b/internal/commands/nat_gateway/nat_gateway_inputs.go index 4d8897b0..a225ce6d 100644 --- a/internal/commands/nat_gateway/nat_gateway_inputs.go +++ b/internal/commands/nat_gateway/nat_gateway_inputs.go @@ -96,7 +96,7 @@ func processFlagCreateNATGatewayInput(cmd *cobra.Command) (*megaport.CreateNATGa return nil, fmt.Errorf("failed to parse resource tags JSON: %w", err) } if err := utils.RejectEmptyTagKeys(tagsMap); err != nil { - return nil, err + return nil, exitcodes.NewUsageError(err) } for k, v := range tagsMap { resourceTags = append(resourceTags, megaport.ResourceTag{Key: k, Value: v}) @@ -179,7 +179,7 @@ func processJSONUpdateNATGatewayInput(jsonStr, jsonFile, uid string) (*megaport. } if err := utils.RejectEmptyTagKeys(raw.ResourceTags); err != nil { - return nil, updateExplicitFields{}, err + return nil, updateExplicitFields{}, exitcodes.NewUsageError(err) } explicit := updateExplicitFields{ @@ -264,7 +264,7 @@ func processFlagUpdateNATGatewayInput(cmd *cobra.Command, uid string) (*megaport return nil, fmt.Errorf("failed to parse resource tags JSON: %w", err) } if err := utils.RejectEmptyTagKeys(tagsMap); err != nil { - return nil, err + return nil, exitcodes.NewUsageError(err) } for k, v := range tagsMap { resourceTags = append(resourceTags, megaport.ResourceTag{Key: k, Value: v}) diff --git a/internal/commands/nat_gateway/nat_gateway_inputs_test.go b/internal/commands/nat_gateway/nat_gateway_inputs_test.go index d60b9bba..c64f0e3b 100644 --- a/internal/commands/nat_gateway/nat_gateway_inputs_test.go +++ b/internal/commands/nat_gateway/nat_gateway_inputs_test.go @@ -208,7 +208,11 @@ func TestProcessFlagCreateNATGatewayInput_RejectsEmptyTagKey(t *testing.T) { require.NoError(t, cmd.Flags().Set("resource-tags", `{"":"x"}`)) _, err := processFlagCreateNATGatewayInput(cmd) - assert.Error(t, err) + require.Error(t, err) + + var cliErr *exitcodes.CLIError + require.True(t, errors.As(err, &cliErr)) + assert.Equal(t, exitcodes.Usage, cliErr.Code) } // ---- processJSONUpdateNATGatewayInput ---- @@ -218,6 +222,10 @@ func TestProcessJSONUpdateNATGatewayInput_RejectsEmptyTagKey(t *testing.T) { `{"name":"GW","resourceTags":{"":"x"}}`, "", "uid-empty-tag") require.Error(t, err) assert.Contains(t, err.Error(), "tag key must not be empty") + + var cliErr *exitcodes.CLIError + require.True(t, errors.As(err, &cliErr)) + assert.Equal(t, exitcodes.Usage, cliErr.Code) } func TestProcessJSONUpdateNATGatewayInput_ValidResourceTags(t *testing.T) { @@ -382,5 +390,9 @@ func TestProcessFlagUpdateNATGatewayInput_RejectsEmptyTagKey(t *testing.T) { require.NoError(t, cmd.Flags().Set("resource-tags", `{"":"x"}`)) _, err := processFlagUpdateNATGatewayInput(cmd, "uid-empty-tag") - assert.Error(t, err) + require.Error(t, err) + + var cliErr *exitcodes.CLIError + require.True(t, errors.As(err, &cliErr)) + assert.Equal(t, exitcodes.Usage, cliErr.Code) } diff --git a/internal/commands/vxc/vxc_prompts.go b/internal/commands/vxc/vxc_prompts.go index 914b0bf9..c1c9aef7 100644 --- a/internal/commands/vxc/vxc_prompts.go +++ b/internal/commands/vxc/vxc_prompts.go @@ -258,7 +258,7 @@ var buildUpdateVXCRequestFromPrompt = func(ctx context.Context, client *megaport return nil, err } if name == "" { - return nil, fmt.Errorf("name cannot be empty") + return nil, validation.NewValidationError("VXC name", name, "cannot be empty") } req.Name = &name fieldsUpdated = true diff --git a/internal/commands/vxc/vxc_prompts_test.go b/internal/commands/vxc/vxc_prompts_test.go index cd19fb51..d5580f7e 100644 --- a/internal/commands/vxc/vxc_prompts_test.go +++ b/internal/commands/vxc/vxc_prompts_test.go @@ -1029,7 +1029,7 @@ func TestBuildUpdateVXCRequestFromPrompt(t *testing.T) { "yes", // update name "", // new name (empty) }, - expectedError: "name cannot be empty", + expectedError: "cannot be empty", }, { name: "update remaining fields", From 660d250f936d1ebf56fe1c7b4d91c9245f303ab8 Mon Sep 17 00:00:00 2001 From: MegaportPhilipBrowne Date: Fri, 10 Jul 2026 14:06:23 -0700 Subject: [PATCH 07/11] fix(ix): classify update JSON file-read and parse errors as usage errors buildUpdateIXRequestFromJSON returned utils.ReadJSONInput and json.Unmarshal errors unwrapped, so a missing --json-file got classified as an API error (the "failed to read" pattern) instead of usage, unlike buildIXRequestFromJSON. --- internal/commands/ix/ix_inputs.go | 4 ++-- internal/commands/ix/ix_inputs_test.go | 15 +++++++++++++++ 2 files changed, 17 insertions(+), 2 deletions(-) diff --git a/internal/commands/ix/ix_inputs.go b/internal/commands/ix/ix_inputs.go index df11b0b8..38fb6838 100644 --- a/internal/commands/ix/ix_inputs.go +++ b/internal/commands/ix/ix_inputs.go @@ -154,12 +154,12 @@ func buildUpdateIXRequestFromFlags(cmd *cobra.Command) (*megaport.UpdateIXReques func buildUpdateIXRequestFromJSON(jsonStr, jsonFile string) (*megaport.UpdateIXRequest, error) { jsonData, err := utils.ReadJSONInput(jsonStr, jsonFile) if err != nil { - return nil, err + return nil, exitcodes.NewUsageError(err) } req := &megaport.UpdateIXRequest{} if err := json.Unmarshal(jsonData, req); err != nil { - return nil, fmt.Errorf("failed to parse JSON: %w", err) + return nil, exitcodes.NewUsageError(fmt.Errorf("failed to parse JSON: %w", err)) } if req.ASN != nil { diff --git a/internal/commands/ix/ix_inputs_test.go b/internal/commands/ix/ix_inputs_test.go index bb4ae181..588b0fcb 100644 --- a/internal/commands/ix/ix_inputs_test.go +++ b/internal/commands/ix/ix_inputs_test.go @@ -29,6 +29,21 @@ func TestBuildUpdateIXRequestFromJSON_BothEmpty(t *testing.T) { assert.Error(t, err) assert.Contains(t, err.Error(), "failed to parse JSON") assert.Nil(t, req) + + var cliErr *exitcodes.CLIError + require.True(t, errors.As(err, &cliErr)) + assert.Equal(t, exitcodes.Usage, cliErr.Code) +} + +func TestBuildUpdateIXRequestFromJSON_FileNotFound(t *testing.T) { + req, err := buildUpdateIXRequestFromJSON("", filepath.Join(t.TempDir(), "missing.json")) + assert.Error(t, err) + assert.Contains(t, err.Error(), "failed to read JSON file") + assert.Nil(t, req) + + var cliErr *exitcodes.CLIError + require.True(t, errors.As(err, &cliErr)) + assert.Equal(t, exitcodes.Usage, cliErr.Code) } func TestBuildIXRequestFromJSON_AllFields(t *testing.T) { From 12d4dcdb7ef2eaa44cd4ccf2923e1d320cf62f62 Mon Sep 17 00:00:00 2001 From: MegaportPhilipBrowne Date: Fri, 10 Jul 2026 14:18:07 -0700 Subject: [PATCH 08/11] fix(nat-gateway): wrap resource-tags read/parse errors as usage errors "failed to parse resource tags JSON" doesn't match classifyError's exact "failed to parse JSON" usage pattern, so it fell through to the broader API-error pattern and returned the wrong exit code. Wrap the read and unmarshal errors explicitly, same as the empty-tag-key check right below them. --- internal/commands/nat_gateway/nat_gateway_inputs.go | 8 ++++---- .../commands/nat_gateway/nat_gateway_inputs_test.go | 12 ++++++++++-- 2 files changed, 14 insertions(+), 6 deletions(-) diff --git a/internal/commands/nat_gateway/nat_gateway_inputs.go b/internal/commands/nat_gateway/nat_gateway_inputs.go index a225ce6d..485d8bd3 100644 --- a/internal/commands/nat_gateway/nat_gateway_inputs.go +++ b/internal/commands/nat_gateway/nat_gateway_inputs.go @@ -89,11 +89,11 @@ func processFlagCreateNATGatewayInput(cmd *cobra.Command) (*megaport.CreateNATGa if resourceTagsStr != "" || resourceTagsFile != "" { tagData, err := utils.ReadJSONInput(resourceTagsStr, resourceTagsFile) if err != nil { - return nil, fmt.Errorf("failed to read resource tags: %w", err) + return nil, exitcodes.NewUsageError(fmt.Errorf("failed to read resource tags: %w", err)) } var tagsMap map[string]string if err := json.Unmarshal(tagData, &tagsMap); err != nil { - return nil, fmt.Errorf("failed to parse resource tags JSON: %w", err) + return nil, exitcodes.NewUsageError(fmt.Errorf("failed to parse resource tags JSON: %w", err)) } if err := utils.RejectEmptyTagKeys(tagsMap); err != nil { return nil, exitcodes.NewUsageError(err) @@ -257,11 +257,11 @@ func processFlagUpdateNATGatewayInput(cmd *cobra.Command, uid string) (*megaport if resourceTagsStr != "" || resourceTagsFile != "" { tagData, err := utils.ReadJSONInput(resourceTagsStr, resourceTagsFile) if err != nil { - return nil, fmt.Errorf("failed to read resource tags: %w", err) + return nil, exitcodes.NewUsageError(fmt.Errorf("failed to read resource tags: %w", err)) } var tagsMap map[string]string if err := json.Unmarshal(tagData, &tagsMap); err != nil { - return nil, fmt.Errorf("failed to parse resource tags JSON: %w", err) + return nil, exitcodes.NewUsageError(fmt.Errorf("failed to parse resource tags JSON: %w", err)) } if err := utils.RejectEmptyTagKeys(tagsMap); err != nil { return nil, exitcodes.NewUsageError(err) diff --git a/internal/commands/nat_gateway/nat_gateway_inputs_test.go b/internal/commands/nat_gateway/nat_gateway_inputs_test.go index c64f0e3b..2dedbe56 100644 --- a/internal/commands/nat_gateway/nat_gateway_inputs_test.go +++ b/internal/commands/nat_gateway/nat_gateway_inputs_test.go @@ -183,7 +183,11 @@ func TestProcessFlagCreateNATGatewayInput_InvalidResourceTagsJSON(t *testing.T) require.NoError(t, cmd.Flags().Set("resource-tags", `{invalid}`)) _, err := processFlagCreateNATGatewayInput(cmd) - assert.Error(t, err) + require.Error(t, err) + + var cliErr *exitcodes.CLIError + require.True(t, errors.As(err, &cliErr)) + assert.Equal(t, exitcodes.Usage, cliErr.Code) } func TestProcessFlagCreateNATGatewayInput_ValidResourceTags(t *testing.T) { @@ -382,7 +386,11 @@ func TestProcessFlagUpdateNATGatewayInput_InvalidResourceTags(t *testing.T) { require.NoError(t, cmd.Flags().Set("resource-tags", `{invalid}`)) _, err := processFlagUpdateNATGatewayInput(cmd, "uid-bad") - assert.Error(t, err) + require.Error(t, err) + + var cliErr *exitcodes.CLIError + require.True(t, errors.As(err, &cliErr)) + assert.Equal(t, exitcodes.Usage, cliErr.Code) } func TestProcessFlagUpdateNATGatewayInput_RejectsEmptyTagKey(t *testing.T) { From 19e467fca14e61955529d0175dd21e6af6de0d52 Mon Sep 17 00:00:00 2001 From: MegaportPhilipBrowne Date: Mon, 13 Jul 2026 07:35:02 -0700 Subject: [PATCH 09/11] fix(managed-account): require accountName/accountRef in flags path buildManagedAccountRequestFromFlags built the request with no validation while buildManagedAccountRequestFromJSON rejected empty accountName/accountRef, letting managed-account buy --account-name Foo (blank --account-ref) ship to the API. Add the same check to the flags builder. --- .../managed_account/managed_account_inputs.go | 4 ++ .../managed_account_inputs_test.go | 41 +++++++++++-------- 2 files changed, 27 insertions(+), 18 deletions(-) diff --git a/internal/commands/managed_account/managed_account_inputs.go b/internal/commands/managed_account/managed_account_inputs.go index 86b3b6c8..a9e4e8a9 100644 --- a/internal/commands/managed_account/managed_account_inputs.go +++ b/internal/commands/managed_account/managed_account_inputs.go @@ -29,6 +29,10 @@ func buildManagedAccountRequestFromFlags(cmd *cobra.Command) (*megaport.ManagedA accountName, _ := cmd.Flags().GetString("account-name") accountRef, _ := cmd.Flags().GetString("account-ref") + if accountName == "" || accountRef == "" { + return nil, exitcodes.NewUsageError(fmt.Errorf("accountName and accountRef are required")) + } + req := &megaport.ManagedAccountRequest{ AccountName: accountName, AccountRef: accountRef, diff --git a/internal/commands/managed_account/managed_account_inputs_test.go b/internal/commands/managed_account/managed_account_inputs_test.go index 144fde9d..1c88c16b 100644 --- a/internal/commands/managed_account/managed_account_inputs_test.go +++ b/internal/commands/managed_account/managed_account_inputs_test.go @@ -90,9 +90,10 @@ func TestParseManagedAccountRequestJSON(t *testing.T) { func TestBuildManagedAccountRequestFromFlags(t *testing.T) { tests := []struct { - name string - flags map[string]string - validate func(t *testing.T, req *megaport.ManagedAccountRequest) + name string + flags map[string]string + expectedError string + validate func(t *testing.T, req *megaport.ManagedAccountRequest) }{ { name: "both flags provided", @@ -103,29 +104,33 @@ func TestBuildManagedAccountRequestFromFlags(t *testing.T) { }, }, { - name: "name only", - flags: map[string]string{"account-name": "Test Account"}, - validate: func(t *testing.T, req *megaport.ManagedAccountRequest) { - assert.Equal(t, "Test Account", req.AccountName) - assert.Equal(t, "", req.AccountRef) - }, + name: "name only", + flags: map[string]string{"account-name": "Test Account"}, + expectedError: "accountName and accountRef are required", }, { - name: "no flags (defaults)", - flags: map[string]string{}, - validate: func(t *testing.T, req *megaport.ManagedAccountRequest) { - assert.Equal(t, "", req.AccountName) - assert.Equal(t, "", req.AccountRef) - }, + name: "no flags (defaults)", + flags: map[string]string{}, + expectedError: "accountName and accountRef are required", }, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { req, err := buildManagedAccountRequestFromFlags(newManagedAccountCmd(tt.flags)) - assert.NoError(t, err) - assert.NotNil(t, req) - tt.validate(t, req) + + if tt.expectedError != "" { + assert.Error(t, err) + assert.Contains(t, err.Error(), tt.expectedError) + + var cliErr *exitcodes.CLIError + require.True(t, errors.As(err, &cliErr)) + assert.Equal(t, exitcodes.Usage, cliErr.Code) + } else { + assert.NoError(t, err) + assert.NotNil(t, req) + tt.validate(t, req) + } }) } } From b684176557d519fe43810fc2ccc268b1dd605220 Mon Sep 17 00:00:00 2001 From: MegaportPhilipBrowne Date: Wed, 22 Jul 2026 06:56:48 -0700 Subject: [PATCH 10/11] ESD-1647: Fix NAT Gateway promo/SLR clear and tighten input-mode parity - NAT Gateway update: track explicit promoCode/serviceLevelReference across flags, JSON, and prompts so a deliberate clear ("") isn't reverted to the original value by mergeUpdateDefaults (both carry omitempty in the SDK). - IX update: reject an explicit empty name on flags and JSON paths. - VXC buy: return a Usage exit code (not General) for an empty name from flags. - Add exit-code assertions for the IX "at least one field" and VXC B-End productUID usage errors. --- internal/commands/ix/ix_inputs.go | 6 +++ internal/commands/ix/ix_inputs_test.go | 34 ++++++++++++++ .../nat_gateway/nat_gateway_actions.go | 10 ++++- .../nat_gateway_additional_test.go | 29 ++++++++++++ .../nat_gateway/nat_gateway_inputs.go | 45 ++++++++++++------- .../nat_gateway/nat_gateway_prompts.go | 2 + internal/commands/vxc/vxc_inputs.go | 2 +- internal/commands/vxc/vxc_inputs_test.go | 4 ++ 8 files changed, 113 insertions(+), 19 deletions(-) diff --git a/internal/commands/ix/ix_inputs.go b/internal/commands/ix/ix_inputs.go index 38fb6838..1ee43326 100644 --- a/internal/commands/ix/ix_inputs.go +++ b/internal/commands/ix/ix_inputs.go @@ -83,6 +83,9 @@ func buildUpdateIXRequestFromFlags(cmd *cobra.Command) (*megaport.UpdateIXReques if cmd.Flags().Changed("name") { name, _ := cmd.Flags().GetString("name") + if name == "" { + return nil, validation.NewValidationError("name", name, "cannot be empty") + } req.Name = &name } @@ -162,6 +165,9 @@ func buildUpdateIXRequestFromJSON(jsonStr, jsonFile string) (*megaport.UpdateIXR return nil, exitcodes.NewUsageError(fmt.Errorf("failed to parse JSON: %w", err)) } + if req.Name != nil && *req.Name == "" { + return nil, validation.NewValidationError("name", "", "cannot be empty") + } if req.ASN != nil { if err := validation.ValidateASN(*req.ASN); err != nil { return nil, err diff --git a/internal/commands/ix/ix_inputs_test.go b/internal/commands/ix/ix_inputs_test.go index 588b0fcb..72b51f7a 100644 --- a/internal/commands/ix/ix_inputs_test.go +++ b/internal/commands/ix/ix_inputs_test.go @@ -7,6 +7,7 @@ import ( "testing" "github.com/megaport/megaport-cli/internal/base/exitcodes" + "github.com/megaport/megaport-cli/internal/validation" megaport "github.com/megaport/megaportgo" "github.com/spf13/cobra" "github.com/stretchr/testify/assert" @@ -134,6 +135,39 @@ func TestBuildUpdateIXRequestFromJSON_PointerFields(t *testing.T) { } } +func TestBuildUpdateIXRequestFromJSON_AtLeastOneFieldExitCode(t *testing.T) { + req, err := buildUpdateIXRequestFromJSON(`{}`, "") + assert.Nil(t, req) + assert.Contains(t, err.Error(), "at least one field must be updated") + + var cliErr *exitcodes.CLIError + require.True(t, errors.As(err, &cliErr)) + assert.Equal(t, exitcodes.Usage, cliErr.Code) +} + +func TestBuildUpdateIXRequestFromJSON_EmptyNameRejected(t *testing.T) { + req, err := buildUpdateIXRequestFromJSON(`{"name":""}`, "") + assert.Nil(t, req) + + var valErr *validation.ValidationError + require.True(t, errors.As(err, &valErr)) + assert.Equal(t, "name", valErr.Field) +} + +func TestBuildUpdateIXRequestFromFlags_EmptyNameRejected(t *testing.T) { + cmd := &cobra.Command{Use: "test"} + cmd.Flags().String("name", "", "") + require.NoError(t, cmd.Flags().Set("name", "")) + cmd.Flags().Lookup("name").Changed = true + + req, err := buildUpdateIXRequestFromFlags(cmd) + assert.Nil(t, req) + + var valErr *validation.ValidationError + require.True(t, errors.As(err, &valErr)) + assert.Equal(t, "name", valErr.Field) +} + func TestBuildUpdateIXRequestFromFlags_NoopWhenUnchanged(t *testing.T) { cmd := &cobra.Command{Use: "test"} cmd.Flags().String("name", "", "") diff --git a/internal/commands/nat_gateway/nat_gateway_actions.go b/internal/commands/nat_gateway/nat_gateway_actions.go index 13240b81..5e40c7ec 100644 --- a/internal/commands/nat_gateway/nat_gateway_actions.go +++ b/internal/commands/nat_gateway/nat_gateway_actions.go @@ -298,6 +298,8 @@ func UpdateNATGateway(cmd *cobra.Command, args []string, noColor bool) error { explicit.AutoRenewTerm = cmd.Flags().Changed("auto-renew") explicit.SessionCount = cmd.Flags().Changed("session-count") explicit.DiversityZone = cmd.Flags().Changed("diversity-zone") + explicit.PromoCode = cmd.Flags().Changed("promo-code") + explicit.ServiceLevelReference = cmd.Flags().Changed("service-level-reference") // ASN and BGPShutdownDefault have no flag path; never explicit in flag mode. } else if interactive { req, explicit, err = promptForUpdateNATGatewayDetails(uid, noColor) @@ -363,10 +365,14 @@ func mergeUpdateDefaults(req *megaport.UpdateNATGatewayRequest, original *megapo if req.Term == 0 { req.Term = original.Term } - if req.PromoCode == "" { + // PromoCode and ServiceLevelReference carry omitempty, so an explicit "" is + // dropped from the wire and the server clears the field. Only inherit the + // original when the caller didn't provide the field, so a deliberate clear + // isn't overwritten with the stale value. + if !explicit.PromoCode && req.PromoCode == "" { req.PromoCode = original.PromoCode } - if req.ServiceLevelReference == "" { + if !explicit.ServiceLevelReference && req.ServiceLevelReference == "" { req.ServiceLevelReference = original.ServiceLevelReference } if !explicit.SessionCount && req.Config.SessionCount == 0 { diff --git a/internal/commands/nat_gateway/nat_gateway_additional_test.go b/internal/commands/nat_gateway/nat_gateway_additional_test.go index d2b0e2e3..77f03674 100644 --- a/internal/commands/nat_gateway/nat_gateway_additional_test.go +++ b/internal/commands/nat_gateway/nat_gateway_additional_test.go @@ -555,6 +555,28 @@ func TestProcessJSONUpdateNATGatewayInputBoolPresence(t *testing.T) { assert.True(t, explicit.DiversityZone, "explicit empty string must be tracked so mergeUpdateDefaults does not override it") assert.Equal(t, "", req.Config.DiversityZone) }) + t.Run("promoCode absent returns explicit=false", func(t *testing.T) { + _, explicit, err := processJSONUpdateNATGatewayInput(`{"name":"GW"}`, "", "uid") + assert.NoError(t, err) + assert.False(t, explicit.PromoCode) + }) + t.Run("promoCode present empty returns explicit=true", func(t *testing.T) { + req, explicit, err := processJSONUpdateNATGatewayInput(`{"promoCode":""}`, "", "uid") + assert.NoError(t, err) + assert.True(t, explicit.PromoCode, "explicit empty string must be tracked so mergeUpdateDefaults does not override it") + assert.Equal(t, "", req.PromoCode) + }) + t.Run("serviceLevelReference absent returns explicit=false", func(t *testing.T) { + _, explicit, err := processJSONUpdateNATGatewayInput(`{"name":"GW"}`, "", "uid") + assert.NoError(t, err) + assert.False(t, explicit.ServiceLevelReference) + }) + t.Run("serviceLevelReference present empty returns explicit=true", func(t *testing.T) { + req, explicit, err := processJSONUpdateNATGatewayInput(`{"serviceLevelReference":""}`, "", "uid") + assert.NoError(t, err) + assert.True(t, explicit.ServiceLevelReference, "explicit empty string must be tracked so mergeUpdateDefaults does not override it") + assert.Equal(t, "", req.ServiceLevelReference) + }) } func TestMergeUpdateDefaultsExplicitBools(t *testing.T) { @@ -638,6 +660,13 @@ func TestMergeUpdateDefaultsPromoCodeAndServiceLevelReference(t *testing.T) { assert.Equal(t, "NEW-PROMO", req.PromoCode) assert.Equal(t, "NEW-SLR", req.ServiceLevelReference) }) + + t.Run("explicit blank promo code and service level reference are not overridden", func(t *testing.T) { + req := &megaport.UpdateNATGatewayRequest{ProductUID: "uid"} + mergeUpdateDefaults(req, original, updateExplicitFields{PromoCode: true, ServiceLevelReference: true}) + assert.Empty(t, req.PromoCode, "explicitly cleared promo code must not inherit from original") + assert.Empty(t, req.ServiceLevelReference, "explicitly cleared service level reference must not inherit from original") + }) } func TestProcessFlagCreateNATGatewayInput(t *testing.T) { diff --git a/internal/commands/nat_gateway/nat_gateway_inputs.go b/internal/commands/nat_gateway/nat_gateway_inputs.go index 485d8bd3..15e3d376 100644 --- a/internal/commands/nat_gateway/nat_gateway_inputs.go +++ b/internal/commands/nat_gateway/nat_gateway_inputs.go @@ -140,11 +140,13 @@ func processFlagCreateNATGatewayInput(cmd *cobra.Command) (*megaport.CreateNATGa // resource when left blank rather than relying on server-side omitempty // behavior. type updateExplicitFields struct { - AutoRenewTerm bool // was autoRenewTerm present in input? - BGPShutdownDefault bool // was bgpShutdownDefault present in input? - ASN bool // was asn present in JSON input? (no flag path for ASN) - SessionCount bool // was sessionCount present in JSON input? - DiversityZone bool // was diversityZone present in JSON input? + AutoRenewTerm bool // was autoRenewTerm present in input? + BGPShutdownDefault bool // was bgpShutdownDefault present in input? + ASN bool // was asn present in JSON input? (no flag path for ASN) + SessionCount bool // was sessionCount present in JSON input? + DiversityZone bool // was diversityZone present in JSON input? + PromoCode bool // was promoCode present in input? + ServiceLevelReference bool // was serviceLevelReference present in input? } // processJSONUpdateNATGatewayInput parses a JSON update request and returns the @@ -152,8 +154,10 @@ type updateExplicitFields struct { // zero value could be a valid intentional value (bool, int for ASN/SessionCount, // string for DiversityZone) use pointer types in the raw struct so nil means // "not provided" and mergeUpdateDefaults will inherit from the original resource. -// Fields with genuinely invalid zero values (LocationID, Speed, Term) or omitempty -// in the SDK request (PromoCode, ServiceLevelReference) use plain types. +// Fields with genuinely invalid zero values (LocationID, Speed, Term) use plain +// types. PromoCode and ServiceLevelReference use pointer types too: they carry +// omitempty in the SDK request, so an explicit "" is dropped from the wire and +// the server clears the field, but only if we don't inherit the original first. func processJSONUpdateNATGatewayInput(jsonStr, jsonFile, uid string) (*megaport.UpdateNATGatewayRequest, updateExplicitFields, error) { jsonData, err := utils.ReadJSONInput(jsonStr, jsonFile) if err != nil { @@ -170,8 +174,8 @@ func processJSONUpdateNATGatewayInput(jsonStr, jsonFile, uid string) (*megaport. ASN *int `json:"asn"` BGPShutdownDefault *bool `json:"bgpShutdownDefault"` AutoRenewTerm *bool `json:"autoRenewTerm"` - PromoCode string `json:"promoCode"` - ServiceLevelReference string `json:"serviceLevelReference"` + PromoCode *string `json:"promoCode"` + ServiceLevelReference *string `json:"serviceLevelReference"` ResourceTags map[string]string `json:"resourceTags"` } if err := json.Unmarshal(jsonData, &raw); err != nil { @@ -183,11 +187,13 @@ func processJSONUpdateNATGatewayInput(jsonStr, jsonFile, uid string) (*megaport. } explicit := updateExplicitFields{ - AutoRenewTerm: raw.AutoRenewTerm != nil, - BGPShutdownDefault: raw.BGPShutdownDefault != nil, - ASN: raw.ASN != nil, - SessionCount: raw.SessionCount != nil, - DiversityZone: raw.DiversityZone != nil, + AutoRenewTerm: raw.AutoRenewTerm != nil, + BGPShutdownDefault: raw.BGPShutdownDefault != nil, + ASN: raw.ASN != nil, + SessionCount: raw.SessionCount != nil, + DiversityZone: raw.DiversityZone != nil, + PromoCode: raw.PromoCode != nil, + ServiceLevelReference: raw.ServiceLevelReference != nil, } var autoRenew, bgpShutdown bool @@ -208,6 +214,13 @@ func processJSONUpdateNATGatewayInput(jsonStr, jsonFile, uid string) (*megaport. if raw.DiversityZone != nil { diversityZone = *raw.DiversityZone } + var promoCode, serviceLevelRef string + if raw.PromoCode != nil { + promoCode = *raw.PromoCode + } + if raw.ServiceLevelReference != nil { + serviceLevelRef = *raw.ServiceLevelReference + } req := &megaport.UpdateNATGatewayRequest{ ProductUID: uid, @@ -216,8 +229,8 @@ func processJSONUpdateNATGatewayInput(jsonStr, jsonFile, uid string) (*megaport. Speed: raw.Speed, Term: raw.Term, AutoRenewTerm: autoRenew, - PromoCode: raw.PromoCode, - ServiceLevelReference: raw.ServiceLevelReference, + PromoCode: promoCode, + ServiceLevelReference: serviceLevelRef, Config: megaport.NATGatewayNetworkConfig{ ASN: asn, BGPShutdownDefault: bgpShutdown, diff --git a/internal/commands/nat_gateway/nat_gateway_prompts.go b/internal/commands/nat_gateway/nat_gateway_prompts.go index c612cb3d..7637805d 100644 --- a/internal/commands/nat_gateway/nat_gateway_prompts.go +++ b/internal/commands/nat_gateway/nat_gateway_prompts.go @@ -216,12 +216,14 @@ func promptForUpdateNATGatewayDetails(uid string, noColor bool) (*megaport.Updat return nil, explicit, err } req.PromoCode = strings.TrimSpace(promoCode) + explicit.PromoCode = req.PromoCode != "" serviceLevelRef, err := utils.ResourcePrompt("nat-gateway", "Service level reference (leave empty to keep current): ", noColor) if err != nil { return nil, explicit, err } req.ServiceLevelReference = strings.TrimSpace(serviceLevelRef) + explicit.ServiceLevelReference = req.ServiceLevelReference != "" tagsMap, err := utils.ResourceTagsPrompt(noColor) if err != nil { diff --git a/internal/commands/vxc/vxc_inputs.go b/internal/commands/vxc/vxc_inputs.go index 144446ea..8ffea98a 100644 --- a/internal/commands/vxc/vxc_inputs.go +++ b/internal/commands/vxc/vxc_inputs.go @@ -19,7 +19,7 @@ var buildVXCRequestFromFlags = func(cmd *cobra.Command, ctx context.Context, svc name, _ := cmd.Flags().GetString("name") if name == "" { - return nil, fmt.Errorf("name is required") + return nil, exitcodes.NewUsageError(validation.NewValidationError("VXC name", name, "cannot be empty")) } rateLimit, _ := cmd.Flags().GetInt("rate-limit") diff --git a/internal/commands/vxc/vxc_inputs_test.go b/internal/commands/vxc/vxc_inputs_test.go index 8b338d7a..83653404 100644 --- a/internal/commands/vxc/vxc_inputs_test.go +++ b/internal/commands/vxc/vxc_inputs_test.go @@ -1444,6 +1444,10 @@ func TestBuildVXCRequestFromJSON_PartnerPortResolution(t *testing.T) { _, err := buildVXCRequestFromJSON(payload, "", context.Background(), &MockVXCService{}) require.Error(t, err) assert.Contains(t, err.Error(), "productUID was neither provided nor could be looked up") + + var cliErr *exitcodes.CLIError + require.True(t, errors.As(err, &cliErr), "expected a *exitcodes.CLIError, got %T: %v", err, err) + assert.Equal(t, exitcodes.Usage, cliErr.Code) }) } From 5aaeb3d4f6106074b0f27d56f5c5fd7c52d4ddb4 Mon Sep 17 00:00:00 2001 From: MegaportPhilipBrowne Date: Mon, 27 Jul 2026 16:22:50 -0700 Subject: [PATCH 11/11] ESD-1647: Update one field in the VXC shutdown-status display test The test came in from main (#525) answering "no" to every prompt, which this branch's new "at least one field must be updated" gate now rejects. The status line prints before the prompts, so changing the name keeps the request valid without affecting what the test checks. --- internal/commands/vxc/vxc_prompts_test.go | 27 ++++++++++++----------- 1 file changed, 14 insertions(+), 13 deletions(-) diff --git a/internal/commands/vxc/vxc_prompts_test.go b/internal/commands/vxc/vxc_prompts_test.go index c6d9f29b..c733f172 100644 --- a/internal/commands/vxc/vxc_prompts_test.go +++ b/internal/commands/vxc/vxc_prompts_test.go @@ -1171,19 +1171,20 @@ func TestBuildUpdateVXCRequestFromPrompt(t *testing.T) { func TestBuildUpdateVXCRequestFromPromptShutdownStatusDisplay(t *testing.T) { responses := []string{ - "no", // update name - "no", // update rate limit - "no", // update term - "no", // update cost centre - "no", // update shutdown - "no", // update A-End VLAN - "no", // update B-End VLAN - "no", // update A-End inner VLAN - "no", // update B-End inner VLAN - "no", // update A-End UID - "no", // update B-End UID - "no", // A-End VRouter config - "no", // B-End VRouter config + "yes", // update name + "New VXC Name", // new name + "no", // update rate limit + "no", // update term + "no", // update cost centre + "no", // update shutdown + "no", // update A-End VLAN + "no", // update B-End VLAN + "no", // update A-End inner VLAN + "no", // update B-End inner VLAN + "no", // update A-End UID + "no", // update B-End UID + "no", // A-End VRouter config + "no", // B-End VRouter config } tests := []struct {