[← Back to Reviews Index](../Stewards%20Reviews%20Index.md)

# .NET Best Practices Review — Azure-AI-RAG-CSharp-Semantic-Kernel-Functions

| Field | Value |
|---|---|
| **Project** | Azure-AI-RAG-CSharp-Semantic-Kernel-Functions |
| **Steward** | .NET Best Practices Steward |
| **Run Date** | 2026-03-21 |
| **Target** | `src/ChatAPI` (C# / ASP.NET Core) |
| **Summary** | 🔴 2 Critical · 🟡 5 Notable · 🟢 6 Minor · ℹ️ 2 Info |

---

## 1. Code Quality Overview

| Attribute | Value |
|---|---|
| **Target Framework** | `net8.0` |
| **Nullable Reference Types** | `<Nullable>enable</Nullable>` — enabled |
| **Implicit Usings** | Enabled |
| **Language Version** | C# 12 (default for .NET 8) |
| **Project Structure** | Single `ChatAPI` ASP.NET Core web project |
| **C# Files Audited** | 9 (Controllers: 2, Data: 4, Plugins: 2, Services: 1) |

The project is a small, focused RAG (Retrieval-Augmented Generation) chat API built on Semantic Kernel and Azure OpenAI. The codebase is compact. Primary constructors are adopted in several classes, and the project targets .NET 8, which is a supported LTS release. `<Nullable>enable</Nullable>` is present in the project file — a positive baseline. However, several files do not honour this setting (nullable annotations are missing from public APIs and models), and there are notable DI lifetime, async, and exception-handling concerns.

---

## 2. Async/Await Correctness

**Posture: Acceptable — one notable anti-pattern.**

All public async methods return `Task` or `Task<T>`. No `async void` methods were found. No `.Result` or `.Wait()` blocking calls are present.

One notable async issue exists in `AISearchDataPlugin.ResourceLookup`:

```csharp
var embeddingTask = _embedding.GenerateEmbeddingAsync(question);
var embedding = await embeddingTask;
string embeddingString = JsonSerializer.Serialize(embedding);  // result never used
```

The intermediate variable `embeddingTask` is created and immediately awaited — offering no concurrency benefit. The `embeddingString` local variable is serialized but never returned or used anywhere, making it dead code.

No `await` inside loops where `Task.WhenAll` would be more appropriate was found in the async paths. The startup `PopulateData` uses `Task.WhenAll` correctly, though only one task is actually passed (the second is commented out).

---

## 3. Memory Management

**Posture: Acceptable with one minor concern.**

`CosmosClient`, `SearchClient`, and `SearchIndexClient` are registered as singletons and never directly instantiated outside DI — this is correct. No raw `HttpClient` instantiations were found; SDK clients abstract HTTP internally.

`MemoryStream` is allocated in a loop in `GenerateProductInfo.PopulateCosmosAsync` without a `using` statement:

```csharp
var stream = new MemoryStream(Encoding.UTF8.GetBytes(productContent));
await container.Container.CreateItemStreamAsync(stream, new PartitionKey(product.id));
```

`MemoryStream` implements `IDisposable`. While GC will eventually collect it, the idiomatic and correct pattern is to wrap it in a `using` statement, especially in a loop.

The `FeedIterator<int>` (the `using (var iterator = ...)` block in `GenerateProductInfo`) is correctly disposed via a `using` statement. However, other `FeedIterator` usages in `ChatHistoryData.GetMessagesBySessionIdAsync` and `ProductData.GetProductByNameAsync` do not dispose the iterator after use — these are `IDisposable` resources.

---

## 4. DI Lifetime Management

**Posture: Critical issue — scoped services captured in a singleton.**

The most significant architectural issue in this codebase is the DI lifetime mismatch in `Program.cs`:

```csharp
builder.Services.AddSingleton<GenerateProductInfo>();   // singleton
builder.Services.AddScoped<ProductData>();              // scoped
builder.Services.AddScoped<ChatHistoryData>();          // scoped
builder.Services.AddScoped<AISearchData>();             // scoped
builder.Services.AddScoped<ChatService>();              // scoped
```

`GenerateProductInfo` is registered as singleton and takes `CosmosClient` and `IConfiguration` — both safe for singleton lifetime. This is fine on its own.

However, `ChatService` is registered as **scoped** and receives `ChatHistory` which is registered as a **singleton**:

```csharp
builder.Services.AddSingleton(serviceProvider => new ChatHistory(systemMessage: @"..."));
```

`ChatHistory` is a mutable object from Semantic Kernel that accumulates conversation state. Registering it as a singleton means **all requests share the same `ChatHistory` instance** — messages from one user's session will be seen by all other users. This is a critical multi-tenancy / data isolation bug.

Additionally, `AISearchDataPlugin` and `ProductDataPlugin` are added to the Semantic Kernel plugin collection via:

```csharp
builder.Services.AddKernel().Plugins.AddFromType<AISearchDataPlugin>("troubleshoot");
builder.Services.AddKernel().Plugins.AddFromType<ProductDataPlugin>("azure_products_services");
```

These plugins resolve `AISearchData` and `ProductData` (both scoped) from a singleton `Kernel`. The Semantic Kernel DI integration may or may not create a new scope for plugin resolution — this warrants verification to ensure scoped services are not being captured into the singleton kernel lifetime.

---

## 5. Nullable Reference Types

**Posture: Nullable is enabled but inconsistently applied.**

While `<Nullable>enable</Nullable>` is set, multiple public-facing model properties are not annotated:

- `ChatMessage` (in `ChatHistoryData.cs`): all string properties (`id`, `sessionid`, `message`, `role`) lack nullability annotations. With nullable enabled, these are implicitly non-nullable reference types but are never initialized in the constructor — they will cause CS8618 warnings.
- `Product` (in `Product.cs`): same pattern — `Id`, `Name`, `Description`, `Rid`, `Self`, `Etag`, `Attachments`, `Paths` are uninitialized non-nullable properties.
- `ChatRequest` (in `ChatController.cs`): `Input` and `SessionId` are non-nullable but have no initializers and no constructor guarantees.

`GetProductByNameAsync` in `ProductData.cs` returns `null` at line 60 for the "not found" path, but the return type is declared as `Task<Dictionary<string, object>>` (non-nullable). This is a nullable annotation violation — the method should return `Task<Dictionary<string, object>?>`.

The null-forgiving operator (`!`) is used in several places in `Program.cs` for configuration values (e.g., `builder.Configuration["AZURE_OPENAI_DEPLOYMENT"]!`). These are generally acceptable in startup code where a missing value would cause immediate failure, but they mask the absence of null-guard logic or `GetRequiredSection` alternatives.

---

## 6. Exception Handling

**Posture: Notable issues — exceptions logged at wrong level; broad catch in startup.**

`GenerateProductInfo.PopulateCosmosAsync` catches `Exception` broadly at the outermost level and logs it without re-throwing:

```csharp
catch (Exception ex)
{
    _logger.LogError(ex, "Error populating Cosmos");
}
```

This silently swallows startup failures. If Cosmos population fails, the application continues to serve requests with an empty database, which may cause incorrect or missing responses without any surfaced error to the caller. The startup helper should propagate the exception so the host fails fast.

In `AISearchDataPlugin.ResourceLookup` and `ProductDataPlugin.GetAzureProductDetailsById`, exceptions are caught, logged with `LogInformation` (not `LogError`), and re-thrown:

```csharp
catch (Exception ex)
{
    _logger.LogInformation(ex, "Error retrieving aisearch data: {question}", question);
    throw;
}
```

Using `LogInformation` for exception logging is incorrect — errors should use `LogError` or `LogWarning`. `LogInformation` is for normal operational events; it will cause errors to be invisible in dashboards filtered to warning/error level.

The re-throw pattern (`throw`) is correct — no `throw ex` anti-pattern found. `ProductData` catches `CosmosException` with a status code filter — this is appropriate and specific.

---

## 7. LINQ and Collections

**Posture: Minor issues.**

In `ProductData.GetProductByNameAsync`:

```csharp
if (response.Resource.Count() > 0)
```

`Count() > 0` on an `IEnumerable<T>` forces full enumeration. `Any()` is the idiomatic and more efficient alternative.

The same method calls `.First()` immediately after the `Count() > 0` check, which causes a second enumeration of the same `IEnumerable`. Calling `.FirstOrDefault()` directly without the count check would be cleaner, or materializing with `.ToList()` first.

No multiple-enumeration issues were found in the main `GetMessagesBySessionIdAsync` — it appends directly to a `List<T>`.

---

## 8. Modern C# Features

**Posture: Partially adopted; some inconsistencies.**

Primary constructors are used in `ProductData`, `ChatHistoryData`, `GenerateProductInfo`, `ChatService`, `ChatController`, and `SessionController` — good adoption.

However, `AISearchDataPlugin` and `ProductDataPlugin` use explicit constructors with manual field assignment instead of primary constructors, inconsistent with the rest of the codebase.

`ChatMessage` and `Product` are plain classes with mutable properties. Given they are data-carrier types with no behavior, they are candidates for `record` types, which would provide value equality, immutability, and concise syntax.

`SessionController` is a conventional class while `ChatController` uses `sealed` and a primary constructor — minor inconsistency.

String interpolation is used appropriately in log messages — no `string.Format` anti-patterns found.

The `if (x != null)` pattern is used implicitly but no clear instances of `is not null` refactoring opportunities were identified as prominent issues.

---

## 9. Findings

| Severity | ID | Title | File |
|---|---|---|---|
| 🔴 | DNET-DI-001 | Singleton `ChatHistory` shared across all requests — multi-tenancy data leak | `src/ChatAPI/Program.cs` |
| 🔴 | DNET-EXCEPT-001 | Startup exception swallowed silently in `PopulateCosmosAsync` | `src/ChatAPI/Data/sample_data/products/GenerateProductInfo.cs` |
| 🟡 | DNET-NULL-001 | Non-nullable return type on `GetProductByNameAsync` despite returning `null` | `src/ChatAPI/Data/ProductData.cs` |
| 🟡 | DNET-NULL-002 | `ChatMessage` model properties uninitialized — nullable annotation mismatch | `src/ChatAPI/Data/ChatHistoryData.cs` |
| 🟡 | DNET-NULL-003 | `Product` model properties uninitialized — nullable annotation mismatch | `src/ChatAPI/Data/Product.cs` |
| 🟡 | DNET-EXCEPT-002 | Exceptions logged at `LogInformation` level in plugin catch blocks | `src/ChatAPI/Plugins/AISearchDataPlugin.cs`, `ProductDataPlugin.cs` |
| 🟡 | DNET-NULL-004 | `ChatRequest` model properties uninitialized — nullable annotation mismatch | `src/ChatAPI/Controllers/ChatController.cs` |
| 🟢 | DNET-DISPOSE-001 | `FeedIterator` not disposed in `GetMessagesBySessionIdAsync` and `GetProductByNameAsync` | `src/ChatAPI/Data/ChatHistoryData.cs`, `ProductData.cs` |
| 🟢 | DNET-DISPOSE-002 | `MemoryStream` not disposed in loop in `PopulateCosmosAsync` | `src/ChatAPI/Data/sample_data/products/GenerateProductInfo.cs` |
| 🟢 | DNET-LINQ-001 | `Count() > 0` used instead of `Any()` | `src/ChatAPI/Data/ProductData.cs` |
| 🟢 | DNET-ASYNC-001 | Dead variable `embeddingString` computed and discarded in `ResourceLookup` | `src/ChatAPI/Plugins/AISearchDataPlugin.cs` |
| 🟢 | DNET-MODERN-001 | `AISearchDataPlugin` and `ProductDataPlugin` use explicit constructors inconsistently with rest of codebase | `src/ChatAPI/Plugins/AISearchDataPlugin.cs`, `ProductDataPlugin.cs` |
| ℹ️ | DNET-INFO-001 | Nullable reference types enabled and `<ImplicitUsings>` enabled — good baseline | `src/ChatAPI/ChatAPI.csproj` |
| ℹ️ | DNET-INFO-002 | Primary constructors adopted across most classes — modern C# practice applied | Multiple files |

---

## 10. Recommended Improvements

| Finding | Recommended Action | Priority |
|---|---|---|
| DNET-DI-001 | Remove the singleton `ChatHistory` registration. Scope `ChatHistory` to the request (or better: instantiate it per call in `ChatService.GetResponseAsync` and load from Cosmos, rather than holding it as injected state). | High |
| DNET-EXCEPT-001 | Remove the try/catch in `PopulateCosmosAsync` (or re-throw after logging) so startup failures surface and prevent the host from entering a degraded state. | High |
| DNET-NULL-001 | Change return type of `GetProductByNameAsync` to `Task<Dictionary<string, object>?>` and update all callers to handle the null case. | Medium |
| DNET-NULL-002 | Add `= default!` initializers or initialize in constructor, or better — convert `ChatMessage` to a `record` with required init properties, or add nullable annotations to the string properties. | Medium |
| DNET-NULL-003 | Same as DNET-NULL-002 — convert `Product` (in `Product.cs`) to a `record` or add proper nullability annotations. Note: there are two `Product` class definitions (`Product.cs` and `GenerateProductInfo.cs`) — consolidate these. | Medium |
| DNET-EXCEPT-002 | Replace `_logger.LogInformation(ex, ...)` with `_logger.LogError(ex, ...)` in all plugin catch blocks. | Medium |
| DNET-NULL-004 | Add nullable annotations or required modifiers to `ChatRequest.Input` and `ChatRequest.SessionId`. Consider using a `record` type. | Medium |
| DNET-DISPOSE-001 | Wrap `FeedIterator` results in `using` statements in `GetMessagesBySessionIdAsync` and `GetProductByNameAsync` to ensure deterministic disposal. | Low |
| DNET-DISPOSE-002 | Wrap `MemoryStream` in a `using` statement inside the `foreach` loop in `PopulateCosmosAsync`. | Low |
| DNET-LINQ-001 | Replace `Count() > 0` with `Any()` in `GetProductByNameAsync`, and replace the two-step `Count + First` with a single `FirstOrDefault` call. | Low |
| DNET-ASYNC-001 | Remove the unused `embeddingTask` intermediate variable (just `await` directly) and remove the dead `embeddingString` variable. | Low |
| DNET-MODERN-001 | Refactor `AISearchDataPlugin` and `ProductDataPlugin` to use primary constructors for consistency. | Low |

---

## Notes

- **Two `Product` class definitions exist**: one in `Data/Product.cs` and one in `Data/sample_data/products/GenerateProductInfo.cs`. Both are in the `ChatAPI.Data` namespace (or no namespace for `Product.cs`). This will cause a compile-time duplicate type conflict. The definitions are slightly different (`Product.cs` has more properties). This should be consolidated into a single class.
- **Swagger always enabled**: Swagger is unconditionally enabled (the `if (app.Environment.IsDevelopment())` block is commented out). This is noted here as an observation; the Security Steward owns API exposure findings.
- **CORS `AllowAll` policy**: noted for cross-cutting purposes; ownership belongs to the Security Steward.

---

*This review is based on static analysis of source files as of 2026-03-21. It reflects code-level observations and does not represent runtime profiling, load testing, or dynamic analysis results.*

*Generated by: .NET Best Practices Steward (`dotnet-bestpractises-steward.md`)*