agoda-com / agoda-com/AgodaAnalyzers

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

Đang mở
#212 1 bình luận 0 reaction 0 người được giao Xem trên GitHub
bug
Ngôn ngữ chính
C#
Star
25
Fork
15
Chỉ số merge pull request
Không có pull request nào được merge trong 30 ngày

Mô tả

# 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

Hướng dẫn đóng góp

Mở hướng dẫn đóng góp

Đánh giá

Issue này chưa được đánh giá.

Nhận issue mới trong hộp thư của bạn

Bản tóm tắt ngắn những issue GitHub phù hợp với người mới.