agoda-com / agoda-com/AgodaAnalyzers

New Analyzer Rule: Detect Usage of Generated Client Models in Controller Responses (AG0050)

Aperta
#212 1 commento 0 reazioni 0 assegnatari Vedi su GitHub
bug
Lingua principale
C#
Stelle
25
Fork
15
Metriche di merge delle PR
Nessuna PR unita negli ultimi 30g

Descrizione

# New Analyzer Rule: Detect Usage of Generated Client Models in Controller Responses

## 🎯 **Objective**
Create a new Roslyn analyzer rule to prevent tight coupling between data/dependency layer models and API response models by detecting when generated client models are used directly in controller action responses.

## 📋 **Background & Motivation**
Currently, there's a risk of tight coupling when developers use generated client models (from HTTP clients or GraphQL clients) directly in controller responses. This creates several issues:

- **Tight Coupling**: API responses become directly dependent on external service contracts
- **Breaking Changes**: Changes in external services can break our API responses unexpectedly
- **Poor Separation of Concerns**: Blurs the boundary between data access and API presentation layers
- **Maintenance Issues**: Makes it harder to evolve APIs independently

## 🔍 **Detection Strategy**
The rule should identify when models from generated client assemblies are used anywhere in controller actions by:

1. **Assembly Name Detection Possible due to naming convention at Agoda**:
- HTTP generated clients: Assembly names ending with `.Client`
- GraphQL generated clients: Assembly names ending with `.Graphql`

2. **Complete Object Tree Walking**:
- **Response Analysis**: Analyze the complete object graph of controller action return types
- **Parameter Analysis**: Analyze all input parameters to controller actions
- **Deep Property Traversal**: Recursively examine all properties, nested objects, collections, and generic type parameters
- **Multi-Level Detection**: Detect generated client models at any depth within the object structure

## 📝 **Detailed Requirements**

### **Scope**
- **Target**: Controller action methods (classes inheriting from `Controller` or `ControllerBase`)
- **Analysis Depth**: Full object tree traversal of both return types AND input parameters
- **Property Traversal**: Deep analysis of all properties within response/parameter classes
- **Coverage**: Both synchronous and asynchronous action methods

### **Detection Logic**
```csharp
// ✅ GOOD - Using dedicated response models with clean properties
public ActionResult GetUser(int id)
{
return Ok(new UserResponse { Name = "John", Age = 30 });
}

public class UserResponse
{
public string Name { get; set; }
public int Age { get; set; }
}

// ❌ BAD - Using generated client model directly in return type
public ActionResult GetUser(int id)
{
return Ok(generatedClientModel);
}

// ❌ BAD - Generated client model as property in response class
public ActionResult GetUser(int id)
{
return Ok(new UserResponse());
}

public class UserResponse
{
public UserService.Client.UserDto User { get; set; } // ← VIOLATION HERE
public string AdditionalInfo { get; set; }
}

// ❌ BAD - Generated client model nested deeper in response structure
public ActionResult GetUser(int id)
{
return Ok(new ApiWrapper());
}

public class ApiWrapper
{
public UserData Data { get; set; }
}

public class UserData
{
public UserService.Client.UserDto Details { get; set; } // ← VIOLATION HERE
public List Orders { get; set; } // ← VIOLATION HERE
}

// ❌ BAD - Generated client model in input parameters
public ActionResult UpdateUser(UserService.Client.UserDto userInput) // ← VIOLATION HERE
{
return Ok(new UserResponse());
}

// ❌ BAD - Generated client model in complex input parameter
public ActionResult UpdateUser(UpdateRequest request) // ← Contains violation
{
return Ok(new UserResponse());
}

public class UpdateRequest
{
public UserService.Client.UserDto UserData { get; set; } // ← VIOLATION HERE
public string Reason { get; set; }
}

// ❌ BAD - Generated model in collection within response property
public ActionResult GetOrderSummary()
{
return Ok(new OrderSummary());
}

public class OrderSummary
{
public List RecentOrders { get; set; } // ← VIOLATION HERE
public decimal TotalAmount { get; set; }
}
```

### **Assembly/Namespace Detection**
- **Primary**: Use `Type.Assembly.FullName` or `Type.Assembly.GetName().Name`
- **Fallback**: Use `Type.Namespace` if assembly name detection fails
- **Patterns to Match**:
- `*.Client` (case-insensitive)
- `*.Graphql` (case-insensitive)

## 🛠 **Implementation Approach**

### **Analyzer Structure**
```csharp
[DiagnosticAnalyzer(LanguageNames.CSharp)]
public class GeneratedClientModelInResponseAnalyzer : DiagnosticAnalyzer
{
public static readonly DiagnosticDescriptor Rule = new DiagnosticDescriptor(
"AG001", // Rule ID
"Generated client model used in controller response",
"Controller action '{0}' returns generated client model '{1}' from assembly '{2}'",
"Design",
DiagnosticSeverity.Warning, // or Error based on preference
isEnabledByDefault: true
);
}
```

### **Key Components**
1. **Controller Detection**: Identify controller classes and action methods
2. **Return Type Analysis**: Extract and analyze return types from action methods
3. **Parameter Type Analysis**: Extract and analyze all input parameter types from action methods
4. **Deep Type Tree Walker**: Recursively examine all types, properties, and nested structures
5. **Property Traversal**: Walk through all properties of classes to detect violations at any depth
6. **Assembly Pattern Matcher**: Check if types come from generated client assemblies
7. **Diagnostic Reporter**: Report violations with detailed context including the exact property path

### **Type Tree Walking Algorithm**
```csharp
private bool ContainsGeneratedClientModel(ITypeSymbol type, HashSet visited)
{
if (visited.Contains(type)) return false;
visited.Add(type);

// Check current type
if (IsGeneratedClientModel(type)) return true;

// Check generic type arguments
if (type is INamedTypeSymbol namedType)
{
foreach (var typeArg in namedType.TypeArguments)
{
if (ContainsGeneratedClientModel(typeArg, visited)) return true;
}
}

// Check properties
foreach (var member in type.GetMembers().OfType())
{
if (ContainsGeneratedClientModel(member.Type, visited)) return true;
}

return false;
}
```

## ✅ **Acceptance Criteria**

### **Must Detect**
- [ ] Direct usage of generated client models as return types
- [ ] Generated client models nested in generic types (`ActionResult`, `Task`, etc.)
- [ ] Generated client models in collection types (`List`, `IEnumerable`, etc.)
- [ ] Generated client models in custom response wrapper classes
- [ ] **Generated client models as properties within response classes (any depth)**
- [ ] **Generated client models in input parameters (direct and nested)**
- [ ] **Generated client models as properties within parameter classes (any depth)**
- [ ] Generated client models in deeply nested object structures
- [ ] Both HTTP client models (*.Client assemblies) and GraphQL models (*.Graphql assemblies)
- [ ] Generated client models in collections within properties (`List` as a property)

### **Must Handle**
- [ ] Async action methods (`Task>`)
- [ ] Various ActionResult types (`Ok()`, `BadRequest()`, `NotFound()`, etc.)
- [ ] Complex generic nested structures
- [ ] Circular references in object graphs (prevent infinite loops)
- [ ] Nullable reference types
- [ ] Abstract base controllers

### **Should Not Flag**
- [ ] Generated client models used in private methods that are not controller actions
- [ ] Generated client models used in non-controller classes
- [ ] Legitimate response/parameter models that happen to have similar naming patterns but aren't from generated assemblies
- [ ] Generated client models used internally within business logic (not in controller signatures)

## 🧪 **Test Cases**

### **Positive Cases (Should Trigger Rule)**
```csharp
// Direct usage in return type
public ActionResult GetUser() { }

// Nested in ActionResult
public async Task> GetOrder() { }

// In collections
public ActionResult> GetProducts() { }

// CRITICAL: Generated client model as property in response class
public ActionResult GetUser() { }
public class UserResponse
{
public UserService.Client.UserDto User { get; set; } // ← MUST DETECT
}

// CRITICAL: Multiple levels deep in response properties
public ActionResult GetData() { }
public class ApiWrapper
{
public DataContainer Data { get; set; }
}
public class DataContainer
{
public UserService.Client.UserDto UserInfo { get; set; } // ← MUST DETECT
public List Orders { get; set; } // ← MUST DETECT
}

// CRITICAL: Generated client models in input parameters
public ActionResult UpdateUser(UserService.Client.UserDto input) { } // ← MUST DETECT

// CRITICAL: Generated client models in parameter properties
public ActionResult UpdateUser(UpdateRequest request) { } // ← MUST DETECT
public class UpdateRequest
{
public UserService.Client.UserDto UserData { get; set; } // ← MUST DETECT
public string Reason { get; set; }
}

// Deep nesting in both parameters and responses
public ActionResult>> GetPagedUsers(
SearchRequest searchParams) { } // ← Both should be detected
```

### **Negative Cases (Should Not Trigger Rule)**
```csharp
// Dedicated response models with clean properties
public ActionResult GetUser() { }
public class UserResponse
{
public string Name { get; set; }
public int Age { get; set; }
// No generated client models in properties
}

// Dedicated input models with clean properties
public ActionResult UpdateUser(UpdateUserRequest request) { }
public class UpdateUserRequest
{
public string Name { get; set; }
public int Age { get; set; }
// No generated client models in properties
}

// Non-controller classes (business logic)
public class UserService
{
public UserService.Client.UserDto GetUser() { } // OK - not a controller
private void ProcessUser(UserService.Client.UserDto user) { } // OK - not a controller action
}

// Models with similar naming but from legitimate assemblies
public ActionResult GetUser() { }
public class UserResponse
{
public MyApp.Client.UserDto User { get; set; } // OK - not generated (different assembly pattern)
}
```

## 🏷 **Metadata**
- **Rule ID**: `AG0050` (or next available ID in sequence)
- **Category**: Design / Architecture
- **Severity**: Warning (configurable to Error if needed)
- **Title**: "Generated client model used in controller action"
- **Description**: "Controller actions should not use generated client models in parameters or return types (including nested properties) to avoid tight coupling"

Note: Dont forget to write the unit tests first

Guida per i contributori

Apri la guida per i contributori

Valutazione

Questa issue non è ancora stata valutata.

Ricevi le nuove issue nella tua casella

Un breve riepilogo di issue GitHub adatte ai principianti.