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

# REST API Review — Azure-AI-RAG-CSharp-Semantic-Kernel-Functions

| Field | Value |
|---|---|
| Project | Azure-AI-RAG-CSharp-Semantic-Kernel-Functions |
| Steward | REST API Steward |
| Run Date | 2026-03-21 |
| Critical | 3 |
| Notable | 7 |
| Minor | 4 |
| Info | 2 |
| Total | 16 |

---

## 1. API Surface Summary

Two controllers are present in `src/ChatAPI/Controllers/`:

| Method | Route | Controller | Action |
|---|---|---|---|
| POST | `/chat` | `ChatController` | Submit a chat message and receive an AI-generated response |
| GET | `/session` | `SessionController` | Generate and return a new session ID |

No minimal-API endpoints (MapGet/MapPost) were found. The API surface is small — two endpoints total.

---

## 2. HTTP Verb Assessment

**POST /chat** — Verb is correct for a non-idempotent, stateful operation that writes to CosmosDB (chat history is persisted). No issues with verb choice.

**GET /session** — This is a 🔴 **state-mutating GET**. Despite using `[HttpGet]`, the intent is to generate a fresh session identity. In isolation, the endpoint only returns a new GUID (no write), but the contract implies clients will immediately begin writing under that session ID. More critically, the endpoint is consumed by the frontend as a session-creation step, not a pure read. Returning a new resource (a session) on GET violates REST semantics. The correct verb is POST — `POST /sessions` — to signal resource creation.

---

## 3. Status Code Assessment

**POST /chat** — Returns `200 OK` with the AI response body. Because this operation persists data to CosmosDB (chat messages) on every invocation, the response should communicate outcome more clearly. `200 OK` is tolerable for a conversational exchange, but there is a deeper problem: the return type is `Task<string>` rather than `IActionResult`. This means the action always returns 200 regardless of error — there is no path for 400, 404, 422, or 500 to be returned explicitly from the controller. Any exception thrown propagates as an unhandled 500 with the default ASP.NET error page or JSON problem details (depending on middleware) but the controller itself provides no typed error handling.

**GET /session** — Returns `200 OK` when semantically this is creating a resource. Should return `201 Created`. The `Ok(new { session_id = ... })` call does not set a `Location` header.

---

## 4. URL Convention Assessment

| Endpoint | Current Route | Issue |
|---|---|---|
| `ChatController` | `/chat` | Singular noun; should be `/chats` or semantically `/chat/messages`. Route template is `[Route("[controller]")]` which resolves to `/chat` (controller name minus suffix). |
| `SessionController` | `/session` | Singular noun; should be `/sessions`. Route is hardcoded as `[Route("session")]`. |

Neither route is prefixed with `/api/`. REST convention places API endpoints under `/api/` to distinguish them from other middleware (health checks, static files, etc.).

No `/api/v1/` versioning prefix is present on either route.

The `.http` sample file uses the route `/PostChatRequest/` — this is incorrect and refers to the `Name` parameter on `[HttpPost(Name = "PostChatRequest")]`, not a route. The actual POST endpoint is `/chat`. This is a developer documentation error but also suggests the `Name` parameter is being confused with a route.

---

## 5. Request/Response Contract Assessment

**ChatRequest DTO**

```csharp
public class ChatRequest
{
    public string Input { get; set; }
    public string SessionId { get; set; }
}
```

Issues:
- Both `Input` and `SessionId` are non-nullable `string` in a project with `<Nullable>enable</Nullable>`. Without `= null!` assignments or `[Required]` attributes, these properties are implicitly nullable strings that will compile with warnings and may receive null at runtime with no validation guard.
- No `[Required]` annotation on either field.
- No `[MaxLength]` on `Input`. A user can submit an arbitrarily large string that will be forwarded to Azure OpenAI, potentially causing excessive API costs or request failures.
- `ChatRequest` is defined inside `ChatController.cs` rather than in a dedicated `Requests/` or `Models/` folder. This couples the DTO to the controller file.

**ChatMessage (data entity used as persistence model)**

The `ChatMessage` class in `ChatHistoryData.cs` uses lowercase property names (`id`, `sessionid`, `message`, `role`) mixing with a PascalCase `Timestamp`. This inconsistency is a data contract issue but is internal — it is not exposed directly through the API response.

**GET /session response**

```csharp
return Ok(new { session_id = sessionId });
```

The response uses an anonymous type with snake_case (`session_id`). The POST /chat request body uses PascalCase (`Input`, `SessionId`). This inconsistency means the client must handle two different casing conventions within the same API. Global `JsonSerializerOptions` camelCase serialization should normalize this, but no explicit options are configured in `Program.cs`.

**POST /chat response**

`ChatService.GetResponseAsync` returns:

```csharp
return JsonSerializer.Serialize(new { resp });
```

This double-encodes the response: the controller returns a `string` (already serialized JSON), and ASP.NET Core then serializes that string again as a JSON string literal. The client receives a JSON-encoded string containing JSON, not a JSON object. This is a contract defect that will require the client to double-parse (`JSON.parse(JSON.parse(response))`).

**Product entity**

`Product.cs` exposes CosmosDB internal fields (`Rid`, `Self`, `Etag`, `Attachments`, `Paths`) that appear to be CosmosDB system properties. These are not response DTOs — they are used by the Semantic Kernel plugin — but their shape leaks infrastructure details into the domain model.

---

## 6. Input Validation Assessment

**POST /chat**

- `Input`: No `[Required]`, no `[MaxLength]`, no null guard. An empty or null `Input` will be forwarded to the AI service.
- `SessionId`: No `[Required]`, no format validation (should be a non-empty string, ideally a GUID). A null or empty `SessionId` will cause CosmosDB query failures at the data layer.
- `[ApiController]` is present on `ChatController`, so automatic 400 responses for model validation failures are enabled — but since neither property is annotated with `[Required]`, no validation will fire even though both fields are logically mandatory.

**GET /session**

No input parameters. No validation concerns.

---

## 7. Error Response Format Assessment

No global exception-handling middleware is configured in `Program.cs`. ASP.NET Core 8 provides a default problem details response for unhandled exceptions in Development mode, but this is not explicitly configured.

`app.UseExceptionHandler()` is absent. `app.UseProblemDetails()` or equivalent is absent.

`ChatController.Post` returns `Task<string>` (not `IActionResult`). Any unhandled exception from `chatService.GetResponseAsync` will propagate as a 500. In Development mode, ASP.NET Core's developer exception page may return an HTML error page rather than JSON, violating content negotiation. In Production mode, the exception page may expose internal messages.

No consistent error response envelope (e.g., `{ "error": "...", "traceId": "..." }` or RFC 7807 `ProblemDetails`) is established. `SessionController` returns `Ok(new { session_id })` directly without any error path.

---

## 8. API Versioning Assessment

No API versioning strategy is implemented:

- No URL path versioning (`/api/v1/`).
- No `Asp.Versioning.Mvc` package reference.
- No `[ApiVersion]` attributes.
- No version negotiation via headers or query parameters.

This API is currently undifferentiated by version. If the request or response contract changes, all existing clients break with no migration path.

---

## 9. Pagination Assessment

Neither endpoint returns a collection in the traditional paginated sense:

- `POST /chat` returns a single AI response string.
- `GET /session` returns a single session ID.

No paginated collection endpoints exist. However, chat history retrieval from CosmosDB (`GetMessagesBySessionIdAsync`) returns all messages for a session without any limit. This is an internal data-access method, not an API endpoint, so it falls outside the direct REST contract scope — but if a chat history endpoint were ever added, it would need pagination.

No pagination concerns apply to the current public API surface.

---

## 10. Documentation Assessment

**Swagger/OpenAPI** is configured:

```csharp
builder.Services.AddEndpointsApiExplorer();
builder.Services.AddSwaggerGen();
```

And Swagger UI is unconditionally enabled (the `if (app.Environment.IsDevelopment())` guard was commented out):

```csharp
// if (app.Environment.IsDevelopment())
// {
app.UseSwagger();
app.UseSwaggerUI();
// }
```

This means Swagger UI and the raw OpenAPI JSON are publicly accessible in all environments including Production.

**Endpoint annotations** are absent:

- No `[ProducesResponseType]` on either controller action.
- No `[Produces("application/json")]` attribute.
- No XML doc comments (`///`) on controller actions.
- The Swagger-generated spec will show the `200` default only, with no documented error codes.

The `.http` file (`ChatAPI.http`) contains an incorrect example (`GET /PostChatRequest/`) that does not correspond to any valid route.

---

## 11. Findings

| Severity | ID | Title | File |
|---|---|---|---|
| 🔴 Critical | REST-VERB-001 | State-creating GET: `/session` endpoint uses GET for session creation | `Controllers/SessionController.cs` |
| 🔴 Critical | REST-CONTRACT-001 | Double-JSON serialization in POST /chat response — client receives string-in-string | `Services/ChatService.cs` |
| 🔴 Critical | REST-VALIDATION-001 | No input validation on POST /chat — unbounded `Input` string with no null or length guard | `Controllers/ChatController.cs` |
| 🟡 Notable | REST-STATUS-001 | POST /chat returns raw `string` not `IActionResult` — no typed error status codes possible | `Controllers/ChatController.cs` |
| 🟡 Notable | REST-STATUS-002 | GET /session returns `200 OK` instead of `201 Created` for resource creation | `Controllers/SessionController.cs` |
| 🟡 Notable | REST-ERROR-001 | No global exception-handling middleware — unhandled exceptions may return HTML or expose details | `Program.cs` |
| 🟡 Notable | REST-VERSION-001 | No API versioning strategy — no version prefix, no versioning package, no `[ApiVersion]` | (project-wide) |
| 🟡 Notable | REST-CONTRACT-002 | Mixed JSON casing conventions — snake_case in session response, PascalCase in chat request body | `Controllers/SessionController.cs`, `Controllers/ChatController.cs` |
| 🟡 Notable | REST-VALIDATION-002 | `SessionId` field has no `[Required]` or format validation — null/empty causes CosmosDB failures | `Controllers/ChatController.cs` |
| 🟡 Notable | REST-DOCS-001 | Swagger UI enabled unconditionally in all environments including Production | `Program.cs` |
| 🟢 Minor | REST-URL-001 | Singular route nouns — `/chat` and `/session` should be `/chats` and `/sessions` | `Controllers/ChatController.cs`, `Controllers/SessionController.cs` |
| 🟢 Minor | REST-URL-002 | No `/api/` prefix on routes — API endpoints not distinguished from other middleware paths | (project-wide) |
| 🟢 Minor | REST-DOCS-002 | No `[ProducesResponseType]` annotations on any endpoint — Swagger spec only shows default 200 | `Controllers/ChatController.cs`, `Controllers/SessionController.cs` |
| 🟢 Minor | REST-CONTRACT-003 | `ChatRequest` DTO defined inside controller file — should be in a dedicated models folder | `Controllers/ChatController.cs` |
| ℹ️ Info | REST-INFO-001 | `[ApiController]` is present on both controllers — automatic model validation is enabled | (positive finding) |
| ℹ️ Info | REST-INFO-002 | `.http` sample file uses incorrect route `/PostChatRequest/` — does not match actual endpoint `/chat` | `ChatAPI.http` |

---

## 12. Recommended Improvements

| Finding | Recommended Action | Priority |
|---|---|---|
| REST-VERB-001 | Change `GET /session` to `POST /sessions`. Return `201 Created` with a `Location` header pointing to the new session resource. | High |
| REST-CONTRACT-001 | In `ChatService.GetResponseAsync`, return the object directly (not a serialized string). Change `ChatController.Post` return type to `IActionResult` and return `Ok(result)`. | High |
| REST-VALIDATION-001 | Add `[Required]` and `[MaxLength(4000)]` to `ChatRequest.Input`. Add a null guard in the controller. Configure `JsonSerializerOptions` camelCase globally. | High |
| REST-STATUS-001 | Change `ChatController.Post` return type to `Task<IActionResult>`. Return `Ok(result)` on success, `BadRequest(...)` on validation failure, and let global middleware handle 500s. | High |
| REST-STATUS-002 | In `SessionController.GetSession` (once converted to POST), return `CreatedAtAction(...)` with a `201` status and a `Location` header. | Medium |
| REST-ERROR-001 | Add `app.UseExceptionHandler("/error")` or `app.UseProblemDetails()` in `Program.cs`. Ensure all error responses use RFC 7807 `ProblemDetails` format consistently. | High |
| REST-VERSION-001 | Add `Asp.Versioning.Mvc` NuGet package. Apply `[ApiVersion("1.0")]` to controllers. Prefix routes with `/api/v{version}/`. | Medium |
| REST-CONTRACT-002 | Configure `builder.Services.AddControllers().AddJsonOptions(o => o.JsonSerializerOptions.PropertyNamingPolicy = JsonNamingPolicy.CamelCase)` to enforce consistent camelCase across all responses. | Medium |
| REST-VALIDATION-002 | Add `[Required]` to `ChatRequest.SessionId`. Consider adding a `[RegularExpression]` pattern to enforce GUID format. | High |
| REST-DOCS-001 | Restore the `if (app.Environment.IsDevelopment())` guard around `app.UseSwagger()` / `app.UseSwaggerUI()`. If Swagger is intentionally public, document this decision. | Medium |
| REST-URL-001 | Rename routes to use plural nouns: `[Route("chats")]` and `[Route("sessions")]`. | Low |
| REST-URL-002 | Add `/api/` prefix: `[Route("api/[controller]")]` on both controllers. | Low |
| REST-DOCS-002 | Add `[ProducesResponseType(typeof(...), StatusCodes.Status200OK)]` and `[ProducesResponseType(StatusCodes.Status400BadRequest)]` to each action. Add `///` XML summaries. | Low |
| REST-CONTRACT-003 | Move `ChatRequest` to a `Models/` or `Requests/` folder as a dedicated file. Annotate properties with `[Required]`, `[MaxLength]`, and proper nullability. | Low |
| REST-INFO-002 | Correct `ChatAPI.http` to use `POST http://localhost:8080/chat` with a JSON body `{ "input": "...", "sessionId": "..." }`. | Low |

---

## Summary

The API surface is minimal — two endpoints — but both exhibit foundational issues that would cause real client-compatibility and reliability problems:

1. The response from `POST /chat` is double-serialized, meaning every client must parse a JSON string containing JSON. This is a breaking contract defect.
2. `GET /session` violates HTTP semantics by using a safe method for resource creation.
3. There is no input validation on the primary endpoint, meaning the AI service can receive null or unbounded input strings.
4. No API versioning exists, and Swagger is exposed in all environments.

Addressing the three critical findings (REST-CONTRACT-001, REST-VERB-001, REST-VALIDATION-001) should be the immediate priority before this API handles any production traffic.

---

*This review is based on static analysis of source files as of 2026-03-21. It does not reflect runtime behavior, deployment configuration, or dynamic security analysis. Generated by the REST API Steward (rest-steward).*