Most bad-code examples in tutorials are contrived - obviously wrong in a way real code never actually looks. This post takes a different approach: a complete, realistic Product CRUD API, built with thirteen anti-patterns that genuinely show up in codebases. Each endpoint is walked through one at a time - the bad version, exactly what's wrong with it and why, then the good version, right next to each other, so nothing needs to be hunted down elsewhere on the page.
The scope: a Product entity, five CRUD endpoints - GetAll, GetById, Create, Update, Delete.
Shared Setup: Program.cs
Bad version:
var builder = WebApplication.CreateBuilder(args);
builder.Services.AddControllers();
builder.Services.AddSingleton<ProductsController>();
var app = builder.Build();
app.MapControllers();
app.Run();
// Inside ProductsController itself:
private readonly string _connectionString =
"Server=prod-sql-01;Database=Products;User Id=sa;Password=Passw0rd123!;";
Issues:
Hardcoded connection string. A literal password is committed directly into source code. Anyone with repository access, including, eventually, anyone the repository is ever accidentally exposed to, has the production database password in plain text.
The fix: load the connection string from configuration, appsettings.json locally, environment variables or Key Vault in production, never as a literal string in a class.Incorrect service lifetime, AddSingleton for a controller. Manually registering the controller itself with AddSingleton means exactly one instance of ProductsController is created and shared across every single request for the entire lifetime of the application, rather than the framework creating a fresh instance per request the normal way. This becomes genuinely dangerous the moment that controller, or anything it depends on, holds any per-request state, and it gets worse once a Scoped dependency like a DbContext enters the picture, since DbContext is explicitly not thread-safe and is designed to live for exactly one request. A Singleton capturing a Scoped dependency is a well-known anti-pattern called a "captive dependency" - the Scoped service effectively gets trapped inside the Singleton's lifetime, silently becoming a de facto singleton itself, shared across requests and threads it was never designed to be shared across.
The fix: never manually register a controller at all, AddControllers() already handles controller instantiation correctly per request, and give each dependency the lifetime that actually matches how it's meant to be used, which for something wrapping a DbContext means Scoped.
Good version:
var builder = WebApplication.CreateBuilder(args);
builder.Services.AddDbContext<ProductDbContext>(options =>
options.UseSqlServer(
builder.Configuration.GetConnectionString("ProductsDb")));
builder.Services.AddScoped<IProductService, ProductService>();
builder.Services.AddControllers();
var app = builder.Build();
app.MapControllers();
app.Run();
The connection string now comes from configuration. IProductService is registered as Scoped, not Singleton, one instance per HTTP request, matching the lifetime of the DbContext it wraps, and avoiding the captive dependency problem entirely. The controller itself is never manually registered at all; AddControllers() handles that correctly on its own.
Shared Setup: The Product Entity
public class Product
{
public int Id { get; set; }
public string Name { get; set; }
public decimal Price { get; set; }
public int StockQuantity { get; set; }
public string InternalSupplierNotes { get; set; }
public decimal CostPrice { get; set; }
}
Identical in both versions - the entity itself isn't the problem. InternalSupplierNotes and CostPrice are genuinely internal fields, what the business actually pays a supplier, never meant for a customer-facing API to expose. What differs between versions is whether this exact class ever gets returned to a caller directly, covered in the GetById section below.
Endpoint 1: GetAll
Bad version:
[HttpGet]
public IActionResult GetAll()
{
var products = GetAllProductsAsync().Result;
return Ok(products);
}
private async Task<List<Product>> GetAllProductsAsync()
{
var products = new List<Product>();
using var connection = new SqlConnection(_connectionString);
await connection.OpenAsync();
var command = new SqlCommand("SELECT * FROM Products", connection);
var reader = await command.ExecuteReaderAsync();
while (await reader.ReadAsync())
{
products.Add(new Product
{
Id = reader.GetInt32(0),
Name = reader.GetString(1),
Price = reader.GetDecimal(2)
});
}
return products;
}
Issues:
Missing await, .Result blocks the thread. GetAllProductsAsync().Result synchronously blocks the calling thread while waiting for the async method to complete, defeating the entire purpose of async and await, since the thread sits idle instead of being released back to the pool. In certain synchronization contexts, classic ASP.NET, WPF, WinForms, this specific pattern can cause a genuine deadlock.
The fix: use await all the way up the call chain, never call .Result or .Wait() on an async method.Undisposed SqlCommand and SqlDataReader. Both command and reader implement IDisposable, but only connection gets a using statement here. SqlDataReader in particular keeps the underlying connection in a busy state until it's explicitly closed, so under real load, with many concurrent requests each doing this, connections return to the pool later than they need to, risking pool exhaustion faster than necessary. The outer using on connection happens to clean these up indirectly when it disposes, which is what keeps this specific method from actively breaking right now, but relying on that indirect cleanup rather than disposing each IDisposable explicitly is fragile, and won't hold up if the method returns early or the pattern changes later.
The fix: wrap every IDisposable in its own using statement, not just the outermost one.
Good version:
[HttpGet]
public async Task<ActionResult<List<ProductDto>>> GetAll()
{
var products = await _productService.GetAllAsync();
return Ok(products);
}
// Inside ProductService:
public async Task<List<ProductDto>> GetAllAsync()
{
return await _context.Products
.Select(p => new ProductDto
{
Id = p.Id,
Name = p.Name,
Price = p.Price,
StockQuantity = p.StockQuantity
})
.ToListAsync();
}
Genuinely awaited from the controller all the way through the service call. EF Core's LINQ query is also parameterized automatically and projects directly to ProductDto - a preview of the DTO pattern the next endpoint introduces properly. There's also no manual connection, command, or reader to dispose at all here - EF Core's DbContext and its query execution manage the underlying ADO.NET resources internally, which is one more reason the disposal issue from the bad version simply can't occur in this version.
Endpoint 2: GetById
Bad version:
[HttpGet("{id}")]
public IActionResult GetById(int id)
{
using var connection = new SqlConnection(_connectionString);
connection.Open();
var query = "SELECT * FROM Products WHERE Id = " + id;
var command = new SqlCommand(query, connection);
var reader = command.ExecuteReader();
if (!reader.Read())
{
return Ok(null);
}
var product = new Product
{
Id = reader.GetInt32(0),
Name = reader.GetString(1),
Price = reader.GetDecimal(2)
};
return Ok(product);
}
Issues:
SQL injection. The id parameter is concatenated directly into the query string. A request to /api/products/1;DROP TABLE Products-- executes exactly that.
The fix: never build SQL by concatenating user input, use a parameterized query, or an ORM like EF Core that parameterizes automatically.Ok(null) instead of NotFound(). When the product doesn't exist, this returns a 200 OK with an empty body. The caller can't distinguish "found nothing" from "succeeded, and the answer happens to be nothing," both look identical on the wire.
The fix: return NotFound(), a genuine 404, so the status code itself carries the information.No DTO, full entity exposed. The returned product is the raw entity, including InternalSupplierNotes and CostPrice, fields that should never reach an API caller.
The fix: map to a ProductDto containing only the fields meant to be public.
Good version:
public class ProductDto
{
public int Id { get; set; }
public string Name { get; set; }
public decimal Price { get; set; }
public int StockQuantity { get; set; }
}
[HttpGet("{id}")]
public async Task<ActionResult<ProductDto>> GetById(int id)
{
var product = await _productService.GetByIdAsync(id);
if (product == null)
return NotFound();
return Ok(product);
}
// Inside ProductService:
public async Task<ProductDto?> GetByIdAsync(int id)
{
var product = await _context.Products.FindAsync(id);
// FindAsync uses the primary key safely - parameterized
// automatically, no string concatenation anywhere
if (product == null)
return null;
// Returning null here is fine - it's the CONTROLLER's
// job to translate this into the correct HTTP response,
// not the service's
return new ProductDto
{
Id = product.Id,
Name = product.Name,
Price = product.Price,
StockQuantity = product.StockQuantity
};
}
All three issues fixed together: FindAsync makes SQL injection structurally impossible, NotFound() makes the response honest, and ProductDto makes leaking internal fields impossible rather than something to remember not to do.
Endpoint 3: Create
Bad version:
[HttpPost]
public IActionResult Create(Product product)
{
using var connection = new SqlConnection(_connectionString);
connection.Open();
var query = $"INSERT INTO Products (Name, Price) " +
$"VALUES ('{product.Name}', {product.Price})";
var command = new SqlCommand(query, connection);
command.ExecuteNonQuery();
_logger.LogInformation(
"Product created by user {Email} with auth token {Token}: {Name}",
currentUser.Email, currentUser.AuthToken, product.Name);
return Ok(product);
}
Issues:
No validation. A request with an empty Name, a negative Price, or missing fields is accepted exactly as-is.
The fix: add validation attributes to a dedicated input DTO, paired with ApiController's automatic model validation.SQL injection, again. Same pattern as GetById, product.Name concatenated directly, and this version breaks entirely if the name contains a single quote.
The fix: same as before, EF Core, parameterized automatically.Wrong status code. Returns 200 OK for a successful creation.
The fix: 201 Created, with a Location header pointing to the new resource, since 200 doesn't communicate "a new thing was created."Sensitive data logged in plain text. The user's email is reasonable to log, but the auth token is a genuine secret, and it's now sitting in plain text inside Application Insights or Log Analytics, visible to anyone with read access to logs, often retained for weeks or months. A leaked or overly-permissioned log query becomes a credential leak.
The fix: never log secrets, tokens, passwords, or other sensitive fields, log an identifier (a user ID) instead of the credential itself, and if a field must be referenced for debugging, mask or redact it before it reaches the logger.
Good version:
public class CreateProductDto
{
[Required, MaxLength(200)]
public string Name { get; set; }
[Range(0.01, 1000000)]
public decimal Price { get; set; }
[Range(0, int.MaxValue)]
public int StockQuantity { get; set; }
}
[HttpPost]
public async Task<ActionResult<ProductDto>> Create(CreateProductDto dto)
{
// No manual validation check needed - [ApiController]
// automatically returns 400 Bad Request if the DTO's
// validation attributes aren't satisfied, before this
// method body even runs
var created = await _productService.CreateAsync(dto);
_logger.LogInformation(
"Product {ProductId} created by user {UserId}",
created.Id, currentUser.Id);
// Logs an IDENTIFIER, never a credential or token -
// enough to trace who did what, without exposing
// anything an attacker or an over-permissioned log
// reader could actually use
return CreatedAtAction(
nameof(GetById),
new { id = created.Id },
created);
}
// Inside ProductService:
public async Task<ProductDto> CreateAsync(CreateProductDto dto)
{
var product = new Product
{
Name = dto.Name,
Price = dto.Price,
StockQuantity = dto.StockQuantity
};
_context.Products.Add(product);
await _context.SaveChangesAsync();
return new ProductDto
{
Id = product.Id,
Name = product.Name,
Price = product.Price,
StockQuantity = product.StockQuantity
};
}
CreateProductDto's Required, MaxLength, and Range attributes are checked automatically by ApiController. CreatedAtAction returns the correct 201 with a Location header built from the GetById action. The log statement now records a user ID, not a credential, and EF Core removes the SQL injection path entirely.
Endpoint 4: Update
Bad version:
[HttpPut("{id}")]
public IActionResult Update(int id, Product product)
{
try
{
using var connection = new SqlConnection(_connectionString);
connection.Open();
var query = $"UPDATE Products SET Name = '{product.Name}', " +
$"Price = {product.Price} WHERE Id = {id}";
var command = new SqlCommand(query, connection);
command.ExecuteNonQuery();
}
catch (Exception ex)
{
return Ok(new { error = ex.ToString() });
}
return Ok();
}
Issues:
Swallowed exception behavior masked as a response. The catch block doesn't silently discard the exception here, but it does something almost as problematic in a different way, it converts a failure into a 200 OK response body, meaning the HTTP status code itself claims success while the actual content says otherwise. Callers checking only the status code, which is the normal way to check for success, will treat this as a successful update.
The fix: return an actual error status code, and don't rely on the response body alone to communicate failure.SQL injection, again. Same string-concatenation pattern as the previous two endpoints.
Full exception details leaked to the caller. ex.ToString() returns the complete exception message and full stack trace, including internal file paths, method names, and sometimes literal fragments of the failed SQL query itself, all sent directly to whoever called this endpoint. This is genuinely useful reconnaissance for an attacker probing the API, and it's an accidental disclosure of internal implementation detail even to a well-meaning caller.
The fix: log the full exception detail internally, server-side, and return only a generic, safe error message to the caller.
Good version:
[HttpPut("{id}")]
public async Task<IActionResult> Update(int id, CreateProductDto dto)
{
var success = await _productService.UpdateAsync(id, dto);
if (!success)
return NotFound();
return NoContent();
}
// Inside ProductService:
public async Task<bool> UpdateAsync(int id, CreateProductDto dto)
{
var product = await _context.Products.FindAsync(id);
if (product == null)
return false;
product.Name = dto.Name;
product.Price = dto.Price;
product.StockQuantity = dto.StockQuantity;
try
{
await _context.SaveChangesAsync();
return true;
}
catch (DbUpdateException ex)
{
// The FULL exception, including stack trace, is
// logged INTERNALLY here - visible to the team via
// Application Insights, never sent to the caller
_logger.LogError(ex, "Failed to update product {Id}", id);
return false;
}
}
The exception is now caught narrowly, DbUpdateException, not the broad Exception base class, logged internally in full detail with _logger.LogError, and the method returns false so the controller responds with a correct NotFound or, in a fuller version, a generic 500 with a safe message, never the raw exception content itself.
Endpoint 5: Delete
Bad version:
[HttpDelete("{id}")]
public IActionResult Delete(int id)
{
using var connection = new SqlConnection(_connectionString);
connection.Open();
var query = "DELETE FROM Products WHERE Id = " + id;
var command = new SqlCommand(query, connection);
command.ExecuteNonQuery();
return Ok();
}
Issues:
SQL injection, again. The same concatenation pattern as every other endpoint above.
Wrong status code. Returns 200 OK with no body for a successful delete. The fix: 204 No Content, the correct response when an operation succeeds and there's genuinely nothing further to return.
Good version:
[HttpDelete("{id}")]
public async Task<IActionResult> Delete(int id)
{
var success = await _productService.DeleteAsync(id);
if (!success)
return NotFound();
return NoContent();
}
// Inside ProductService:
public async Task<bool> DeleteAsync(int id)
{
var product = await _context.Products.FindAsync(id);
if (product == null)
return false;
_context.Products.Remove(product);
await _context.SaveChangesAsync();
return true;
}
The Complete Good-Version Service Interface
Pulling every service method from above together into the one interface the controller actually depends on:
public interface IProductService
{
Task<List<ProductDto>> GetAllAsync();
Task<ProductDto?> GetByIdAsync(int id);
Task<ProductDto> CreateAsync(CreateProductDto dto);
Task<bool> UpdateAsync(int id, CreateProductDto dto);
Task<bool> DeleteAsync(int id);
}
This is the layer that never existed in the bad version at all, the bad controller talked directly to SqlConnection. Having this interface is also what makes the controller genuinely unit-testable with a mock, something effectively impossible against the bad version's design.
Attribute Reference: What's Doing the Actual Work
ApiController enables automatic model validation, checking a DTO's validation attributes and returning 400 automatically if they fail, before the action method body runs. Present in both versions, but only doing real work in the good one, since the bad version's Product parameter has no validation attributes for it to check.
Route with the controller token sets the base route, "api/products," identical in both versions.
HttpGet, the id-scoped HttpGet, HttpPost, the id-scoped HttpPut, and the id-scoped HttpDelete map each method to its HTTP verb and route, the routing itself was never the problem in the bad version.
Required, MaxLength, and Range on CreateProductDto are declarative validation rules, good version only, paired with ApiController to reject invalid requests automatically, with zero manual if-checks.
Every Issue, At a Glance
Hardcoded connection string, in Program.cs and setup.
Bad: literal password committed in source code.
Good: loaded from configuration or Key Vault.Incorrect service lifetime, AddSingleton for a controller, in Program.cs and setup.
Bad: the controller manually registered as Singleton, one shared instance forever, risking a captive dependency once a Scoped DbContext enters the picture.
Good: the controller is never manually registered at all, and IProductService is registered as Scoped, matching the DbContext's own lifetime.Missing await / .Result, in GetAll.
Bad: .Result blocks the thread synchronously.
Good: await released the thread properly, all the way through.Undisposed SqlCommand and SqlDataReader, in GetAll.
Bad: only connection wrapped in a using, command and reader never explicitly disposed, relying on indirect cleanup that isn't reliable.
Good: EF Core manages the underlying ADO.NET resources internally, so there's no manual disposal to get wrong.SQL injection, appearing in GetById, Create, Update, and Delete.
Bad: string-concatenated raw SQL.
Good: EF Core LINQ or FindAsync, parameterized automatically.Ok(null) instead of NotFound(), in GetById.
Bad: 200 OK with a null body when not found.
Good: NotFound(), a genuine 404.No DTOs, full entity exposed, in GetById.
Bad: internal fields like CostPrice and supplier notes exposed.
Good: ProductDto excludes them entirely, structurally impossible to leak.No validation, in Create.
Bad: any data accepted as-is.
Good: Required, MaxLength, and Range attributes plus ApiController auto-reject invalid requests.Wrong status codes, in Create and Delete.
Bad: 200 OK for create and delete.
Good: 201 Created with a Location header for create, 204 No Content for delete.Sensitive data logged in plain text, in Create.
Bad: an auth token logged directly, sitting in plain text in Application Insights.
Good: only a user ID logged, never a credential or token.Swallowed exception behavior masked as a response, in Update.
Bad: a failure converted into a 200 OK response body, status code claims success while content says otherwise.
Good: an honest result returned, false propagated up so the controller can respond correctly.Full exception details leaked to the caller, in Update.
Bad: ex.ToString(), including the full stack trace, returned directly in the API response.
Good: the full exception logged internally only, a generic safe message, if any, returned to the caller.No service layer, structural, underneath all endpoints.
Bad: controller does raw data access directly.
Good: IProductService and ProductService, separated, testable, the natural home for every other fix.
Key Lessons
Every anti-pattern here is realistic, these are mistakes that genuinely appear in real production code, not exaggerated examples built just to be wrong.
The missing service layer is structurally connected to most of the other issues, a controller doing raw data access directly removes the natural place validation, parameterization, and error handling would otherwise live.
A DTO isn't just extra code, it's what makes leaking internal fields structurally impossible, rather than something a developer has to remember not to do.
Status codes are information, not decoration, Ok(null), 200 for a create, and 200 for a delete each communicate something genuinely false or ambiguous to the caller.
Logging and error responses are both genuine attack surfaces, not just debugging conveniences, a logged token or a leaked stack trace can hand an attacker exactly what they need, even when the code otherwise "works."
A swallowed exception, or one disguised as a success response, is worse than a crash, a crash is visible immediately, a silently hidden failure looks like success and can go unnoticed for a long time.
Summary
Every issue in the bad version compiles, runs, and looks like reasonable code at a glance, which is exactly why these patterns survive in real codebases far longer than obviously broken code ever would. Rebuilding the same five endpoints with a real service layer, correct dependency lifetimes, disciplined resource disposal, DTOs, parameterized queries, genuine async, proper validation, correct status codes, and careful logging doesn't just fix thirteen individual bugs, it changes the shape of the code so that several of those bugs become structurally difficult to reintroduce by accident.
More from TechStack Blog: C# / .NET: https://www.techstackblog.com/category.html?cat=csharp
CS Fundamentals: https://www.techstackblog.com/category.html?cat=cs-fundamentals
