feat(internal/surfer): add robust Method Identification (#3571)

This PR implements robust AIP-compliant method identification logic and
refactors the associated utility functions and tests to align with
project conventions and ensure reliable gcloud surface generation.

### Key Changes

1.  **Robust Identification Logic (`utils/method.go`):**
- Renamed `GetVerb` to `GetCommandName` to more accurately reflect its
purpose (providing the gcloud command name used for file naming and CLI
invocation).
- Updated `IsCreate`, `IsGet`, `IsList`, `IsUpdate`, and `IsDelete` to
accept `*api.Method` and use a combination of AIP standard prefixes and
HTTP verbs for more reliable detection.
- Implemented custom verb extraction for non-standard methods as per
AIP-136 (e.g., extracting `exportData` from the HTTP path).
- Ensured all returned command names use `snake_case` to maintain
consistency with `gcloud` file naming conventions.

2.  **Model Safety (`internal/sidekick/api/model.go`):**
- Added nil checks for `m.OutputType` in `AIPStandardGetInfo` to prevent
panics during model analysis, particularly in testing environments with
partial mocks.

3.  **Codebase Integration:**
- Updated all call sites in `builder.go` and `generate.go` to use the
new `GetCommandName` signature and robust predicates.

### Impact

- **Reliability:** The generator can now correctly identify custom
methods and standard methods even when naming conventions are slightly
varied, as long as the HTTP annotations are present.
-   **Stability:** Fixed a latent panic in the core API model logic.
- **Maintainability:** Cleaned up "mutation soup" in method
identification and brought test code up to project standards.

Fixes #3362

Author: quirogas

internal/sidekick/api/model.go

@@ -391,7 +391,7 @@ type AIPStandardGetInfo struct {
 // a get operation as defined by AIP-131, if the method is such an operation.
 func (m *Method) AIPStandardGetInfo() *AIPStandardGetInfo {
 	// A get operation is always a simple operation that returns a resource.
-	if !m.IsSimple() || m.InputType == nil || m.ReturnsEmpty || m.OutputType.Resource == nil {
+	if !m.IsSimple() || m.InputType == nil || m.ReturnsEmpty || m.OutputType == nil || m.OutputType.Resource == nil {
 		return nil
 	}
 

internal/surfer/gcloud/builder.go

@@ -128,7 +128,7 @@ func shouldSkipParam(field *api.Field, method *api.Method) bool {
 	}
 
 	// For Update commands, fields marked as IMMUTABLE cannot be changed and should be hidden.
-	if utils.IsUpdate(method.Name) && slices.Contains(field.Behavior, api.FIELD_BEHAVIOR_IMMUTABLE) {
+	if utils.IsUpdate(method) && slices.Contains(field.Behavior, api.FIELD_BEHAVIOR_IMMUTABLE) {
 		return true
 	}
 
@@ -226,7 +226,7 @@ func newParam(field *api.Field, apiField string, overrides *Config, model *api.A
 	}
 
 	// For Update commands, maps and repeated fields are often clearable.
-	if utils.IsUpdate(method.Name) && param.Repeated {
+	if utils.IsUpdate(method) && param.Repeated {
 		param.Clearable = true
 	}
 
@@ -267,7 +267,7 @@ func newPrimaryResourceParam(field *api.Field, method *api.Method, model *api.AP
 
 	// We generate a helpful help text based on whether the command is a `Create` command or not.
 	helpText := fmt.Sprintf("The %s to create.", resourceName)
-	if !strings.HasPrefix(method.Name, "Create") {
+	if !utils.IsCreate(method) {
 		helpText = fmt.Sprintf("The %s to operate on.", resourceName)
 	}
 
@@ -286,7 +286,7 @@ func newPrimaryResourceParam(field *api.Field, method *api.Method, model *api.AP
 		},
 	}
 
-	if utils.IsCreate(method.Name) {
+	if utils.IsCreate(method) {
 		param.RequestIDField = strcase.ToLowerCamel(field.Name)
 	}
 

internal/surfer/gcloud/generate.go

@@ -162,11 +162,11 @@ func generateResourceCommands(collectionID string, methods []*api.Method, baseDi
 
 	// We iterate through each method associated with this resource.
 	for _, method := range methods {
-		// We map the API method name to a standard gcloud command verb.
+		// We map the API method to a standard gcloud command name.
 		// Example: `CreateInstance` -> "create"
-		verb, err := utils.GetVerb(method.Name)
+		verb, err := utils.GetCommandName(method)
 		if err != nil {
-			// Continue to the next method if we can't determine a verb,
+			// Continue to the next method if we can't determine a command name,
 			// logging the issue might be useful here in the future.
 			continue
 		}

internal/surfer/gcloud/utils/method.go

@@ -18,53 +18,110 @@ import (
 	"fmt"
 	"strings"
 
+	"github.com/googleapis/librarian/internal/sidekick/api"
 	"github.com/iancoleman/strcase"
 )
 
-// GetVerb maps an API method name to a standard gcloud command verb.
-func GetVerb(methodName string) (string, error) {
-	if methodName == "" {
-		return "", fmt.Errorf("method name cannot be empty")
+// GetCommandName maps an API method to a standard gcloud command name (in snake_case).
+// This name is typically used for the command's file name.
+func GetCommandName(method *api.Method) (string, error) {
+	if method == nil {
+		return "", fmt.Errorf("method cannot be nil")
 	}
 	switch {
-	case IsGet(methodName):
+	case IsGet(method):
 		return "describe", nil
-	case IsList(methodName):
+	case IsList(method):
 		return "list", nil
-	case IsCreate(methodName):
+	case IsCreate(method):
 		return "create", nil
-	case IsUpdate(methodName):
+	case IsUpdate(method):
 		return "update", nil
-	case IsDelete(methodName):
+	case IsDelete(method):
 		return "delete", nil
 	default:
-		// For non-standard methods, we just use the snake_case version of the method name.
-		return strcase.ToSnake(methodName), nil
+		// For custom methods (AIP-136), we try to extract the custom verb from the HTTP path.
+		// The custom verb is the part after the colon (e.g., .../instances/*:exportData).
+		if method.PathInfo != nil && len(method.PathInfo.Bindings) > 0 {
+			binding := method.PathInfo.Bindings[0]
+			if binding.PathTemplate != nil && binding.PathTemplate.Verb != nil {
+				return strcase.ToSnake(*binding.PathTemplate.Verb), nil
+			}
+		}
+		// Fallback: use the method name converted to snake_case.
+		return strcase.ToSnake(method.Name), nil
 	}
 }
 
-// IsCreate determines if the method is a Create method.
-// TODO(https://github.com/googleapis/librarian/issues/3362): implement a robust AIP-compliant method idetification.
-func IsCreate(methodName string) bool {
-	return strings.HasPrefix(methodName, "Create")
+// IsCreate determines if the method is a standard Create method (AIP-133).
+func IsCreate(m *api.Method) bool {
+	if !strings.HasPrefix(m.Name, "Create") {
+		return false
+	}
+	if verb := getHTTPVerb(m); verb != "" {
+		return verb == "POST"
+	}
+	return true
 }
 
-// IsGet determines if the method is a Get method.
-func IsGet(methodName string) bool {
-	return strings.HasPrefix(methodName, "Get")
+// IsGet determines if the method is a standard Get method (AIP-131).
+func IsGet(m *api.Method) bool {
+	// Use sidekick's robust AIP check if available.
+	if m.AIPStandardGetInfo() != nil {
+		return true
+	}
+	// Fallback heuristic
+	if !strings.HasPrefix(m.Name, "Get") {
+		return false
+	}
+	if verb := getHTTPVerb(m); verb != "" {
+		return verb == "GET"
+	}
+	return true
 }
 
-// IsList determines if the method is a List method.
-func IsList(methodName string) bool {
-	return strings.HasPrefix(methodName, "List")
+// IsList determines if the method is a standard List method (AIP-132).
+func IsList(m *api.Method) bool {
+	if !strings.HasPrefix(m.Name, "List") {
+		return false
+	}
+	if verb := getHTTPVerb(m); verb != "" {
+		return verb == "GET"
+	}
+	return true
 }
 
-// IsUpdate determines if the method is a Update method.
-func IsUpdate(methodName string) bool {
-	return strings.HasPrefix(methodName, "Update")
+// IsUpdate determines if the method is a standard Update method (AIP-134).
+func IsUpdate(m *api.Method) bool {
+	if !strings.HasPrefix(m.Name, "Update") {
+		return false
+	}
+	if verb := getHTTPVerb(m); verb != "" {
+		return verb == "PATCH" || verb == "PUT"
+	}
+	return true
 }
 
-// IsDelete determines if the method is a Delete method.
-func IsDelete(methodName string) bool {
-	return strings.HasPrefix(methodName, "Delete")
+// IsDelete determines if the method is a standard Delete method (AIP-135).
+func IsDelete(m *api.Method) bool {
+	// Use sidekick's robust AIP check if available.
+	if m.AIPStandardDeleteInfo() != nil {
+		return true
+	}
+	// Fallback heuristic
+	if !strings.HasPrefix(m.Name, "Delete") {
+		return false
+	}
+	if verb := getHTTPVerb(m); verb != "" {
+		return verb == "DELETE"
+	}
+	return true
+}
+
+// getHTTPVerb returns the HTTP verb from the primary binding, or an empty string if not available.
+func getHTTPVerb(m *api.Method) string {
+	if m.PathInfo != nil && len(m.PathInfo.Bindings) > 0 {
+		return m.PathInfo.Bindings[0].Verb
+	}
+	return ""
 }