Najczęstsze błędy w ASP.NET Core: gruby kontroler, .Result, encje w API, czasy życia w DI i try-catch

Przejrzałem w swojej pracy setki pull requestów juniorów i wciąż widzę w nich te same błędy. Nie literówki. Nie brakujące średniki. Tylko rzeczy znacznie groźniejsze: kod, który przechodzi code review, świetnie działa na demo... a potem kładzie aplikację na produkcji.

W tym artykule przejdziemy przez 5 antywzorców w ASP.NET Core, które spotykam dosłownie codziennie: gruby kontroler, .Result zamiast await, encje zwracane prosto z API, pomieszane czasy życia w Dependency Injection i try-catch w każdym endpoincie. Przy każdym schemat jest ten sam: zły kod (prosto z prawdziwych pull requestów), dlaczego boli (co się psuje i kiedy wybucha) i poprawka, czyli gotowy kod, który możesz wdrożyć jeszcze dziś. Ostatni, 5. antywzorzec zostaw sobie na deser, bo wygląda jak dobra praktyka. I właśnie dlatego jest najgroźniejszy.

Mała umowa na start: w trakcie czytania licz, ile z tych 5 antywzorców masz u siebie w projekcie. Jeśli na końcu wyjdzie Ci 5 na 5 - masz bingo, a ten tekst jest dokładnie dla Ciebie.

Wersje .NET: przykłady kodu są pisane pod ASP.NET Core w .NET 8 i nowszych (IExceptionHandler z ostatniego antywzorca jest dostępny właśnie od .NET 8). Same zasady są jednak uniwersalne i dotyczą każdej wersji ASP.NET Core, także starszych projektów.

Antywzorzec #1: gruby kontroler

Pierwszy antywzorzec to gruby kontroler, czyli akcja kontrolera, która robi wszystko. Cała logika biznesowa siedzi bezpośrednio w metodzie obsługującej żądanie HTTP. Spójrz na ten endpoint do składania zamówienia. Wygląda znajomo?

/* OrdersController.cs - ŹLE */
[HttpPost]
public async Task<IActionResult> CreateOrder(OrderRequest request)
{
    if (request.Items.Count == 0)
        return BadRequest("Pusty koszyk");           /* walidacja */

    decimal total = 0;
    foreach (var item in request.Items)              /* logika + baza */
    {
        var product = await _db.Products.FindAsync(item.ProductId);
        if (product.Stock < item.Quantity)
            return BadRequest("Brak towaru");
        total += product.Price * item.Quantity;
        product.Stock -= item.Quantity;
    }
    if (total > 500) total *= 0.95m;                 /* rabat 5% */

    _db.Orders.Add(new Order(request.Email, total));
    await _db.SaveChangesAsync();                    /* zapis */
    await _emailSender.SendAsync(request.Email, "Przyjęte!");
    return Ok();                                     /* + mail */
}

Walidacja koszyka. Pętla po produktach, w której sprawdzamy stany magazynowe i liczymy cenę. Rabat 5% przy zamówieniu powyżej 500 zł. Zapis do bazy. I na koniec jeszcze wysyłka maila. Wszystko w jednej metodzie kontrolera.

4 odpowiedzialności w 1 metodzie. Policzmy, co ten kontroler właściwie robi:

• Walidacja: pusty koszyk? brak towaru?
• Logika biznesowa: ceny, rabaty, stany magazynowe.
• Dostęp do bazy: FindAsync, SaveChanges.
• Wysyłka maili: SendAsync do klienta.

4 różne odpowiedzialności w jednej metodzie HTTP. Single Responsibility Principle mówi tu "nie". Głośno.

Dlaczego to boli? Z 3 powodów:

• Testy tylko przez HTTP. Żeby sprawdzić samą logikę rabatu, musisz stawiać cały pipeline HTTP z kontrolerem i bazą danych. Zamiast prostego testu jednostkowego dla jednego warunku piszesz test całego endpointu.

• Zero reużycia. Chcesz złożyć zamówienie z procesu w tle albo z innego endpointu? Zostaje kopiuj-wklej. I od tej chwili zasady rabatu żyją w 2 miejscach, które trzeba zmieniać razem.

• Plik puchnie. Dziś to 30 linii. Za pół roku 800 - i nikt nie chce tego pliku dotykać.

Poprawka: chudy kontroler. Kontroler robi dokładnie 2 rzeczy: przyjmuje żądanie i zwraca odpowiedź. 1 linijka wywołania serwisu i mapowanie wyniku na odpowiedź HTTP. Koniec.

/* OrdersController.cs - DOBRZE */
[HttpPost]
public async Task<IActionResult> CreateOrder(OrderRequest request)
{
    var result = await _orderService.CreateOrderAsync(request);

    return result.IsSuccess
        ? Ok(result.Value)
        : BadRequest(result.Error);
}

Przyjmij żądanie, wywołaj serwis, zwróć odpowiedź. Serwis zwraca prosty typ wyniku (sukces z wartością albo błąd), a kontroler tylko zamienia go na Ok albo BadRequest. Cała logika ląduje w serwisie - w osobnej klasie, z osobną odpowiedzialnością:

/* OrderService.cs - DOBRZE */
public class OrderService : IOrderService
{
    public async Task<Result<int>> CreateOrderAsync(
        OrderRequest request)
    {
        /* 1. walidacja */
        /* 2. ceny, rabaty, stany magazynowe */
        /* 3. zapis do bazy */
        /* 4. wysyłka maila */
        /* -> jedna klasa, jedna odpowiedzialność */
    }
}

Taki serwis testujesz zwykłym testem jednostkowym - bez HTTP i bez kontrolera. I wywołasz go, skąd tylko chcesz: z innego endpointu albo z procesu w tle.

Zasada #1. Kontroler przyjmuje żądanie i zwraca odpowiedź. Cała reszta to nie jego sprawa.

Antywzorzec #2: .Result zamiast await

Drugi antywzorzec to synchroniczne czekanie na asynchroniczny kod. Metoda jest asynchroniczna, ale ktoś wywołał ją synchronicznie - przez .Result. Warianty tego samego grzechu to .Wait() i .GetAwaiter().GetResult(). Najczęściej z komentarzem w stylu "działa, nie ruszać":

/* UsersController.cs - ŹLE */
[HttpGet("{id}")]
public IActionResult GetUser(int id)
{
    /* "działa, nie ruszać" */
    var user = _userService.GetUserAsync(id).Result;
    return Ok(user);
}

/* warianty tego samego grzechu: */
var data = LoadDataAsync().GetAwaiter().GetResult();
task.Wait();

"Działa u mnie" - na razie.

Co się dzieje pod maską: głodzenie puli wątków. Prześledźmy to krok po kroku:

1. Request przychodzi i dostaje wątek z puli.
2. .Result blokuje ten wątek - wątek czeka i nie robi kompletnie nic.
3. Przychodzą kolejne requesty i blokują kolejne wątki.
4. W końcu brakuje wolnych wątków, nowe requesty stoją w kolejce i zaczynają się timeouty.

To zjawisko nazywa się thread pool starvation, czyli głodzenie puli wątków.

Ten sam kod, 2 światy. I teraz najlepsze. U Ciebie, na środowisku deweloperskim, przy 1 użytkowniku wszystko śmiga i nie ma żadnych problemów. Na produkcji, przy 500 użytkownikach: timeouty, błędy 503, restarty - aplikacja klęka. To najgorszy rodzaj błędu: taki, którego nie widać na Twoim komputerze.

A w starym ASP.NET (na .NET Framework) i w aplikacjach desktopowych, np. w WPF, dochodzi bonus: klasyczny deadlock. Tam dalsza część metody po await czeka na powrót do wątku, który właśnie zablokowałeś przez .Result - i oba czekają na siebie w nieskończoność. ASP.NET Core nie ma takiego kontekstu synchronizacji, więc zamiast deadlocka kończy się "tylko" na głodzeniu puli. Co pod ruchem wcale nie jest lepszą wiadomością.

Poprawka: async all the way. Poprawka jest banalnie prosta: asynchronicznie przez całą ścieżkę wywołań. Metoda async, w środku await:

/* UsersController.cs - DOBRZE */
[HttpGet("{id}")]
public async Task<IActionResult> GetUser(int id)
{
    var user = await _userService.GetUserAsync(id);

    return Ok(user);
}

await zwalnia wątek - w czasie czekania na bazę ten sam wątek obsługuje inne requesty. Dlatego ASP.NET Core wytrzymuje duży ruch... o ile mu w tym nie przeszkodzisz.

Zasada #2. Asynchronicznie od kontrolera aż do bazy. Zero .Result, zero .Wait().

Antywzorzec #3: encje prosto z API

Trzeci antywzorzec to encje z bazy danych zwracane prosto z API, czyli w praktyce return Ok(encjaZBazy). Jedna linijka: pobierz użytkowników, zwróć użytkowników. Szybko, wygodnie...

/* UsersController.cs - ŹLE */
[HttpGet]
public async Task<IActionResult> GetUsers()
{
    var users = await _db.Users.ToListAsync();
    return Ok(users);            /* encja EF leci w świat */
}

public class User
{
    public int Id { get; set; }
    public string Email { get; set; }
    public string PasswordHash { get; set; }
    public bool IsAdmin { get; set; }
    public List<Order> Orders { get; set; }
}

...tylko spójrz, co siedzi w tej encji. Email. Hash hasła. Flaga admina. I kolekcja zamówień.

To poleciało do każdego klienta API. Domyślny serializer wysyła każdą publiczną właściwość encji, więc odpowiedź na GET /api/users wygląda tak:

/* GET /api/users -> 200 OK */
[
  {
    "id": 7,
    "email": "anna@firma.pl",
    "passwordHash": "AQAAAAIAAYagAAAAEC3f...",  /* hash hasła! */
    "isAdmin": true,                    /* każdy to widzi */
    "orders": [ ... ]
  }
]

Hash hasła. Flaga isAdmin. Do każdego klienta Twojego API. Gratulacje, właśnie zrobiłeś wyciek danych jedną linijką.

3 problemy (plus bonus). Zwracanie encji z API to nie jeden problem, tylko 3 naraz:

• Wyciek danych. Hash hasła i inne pola wewnętrzne wychodzą w odpowiedzi na świat.

• Cykle referencji. User ma Orders, Order ma Usera... i serializer się wykłada. System.Text.Json domyślnie rzuca wtedy wyjątkiem o wykrytym cyklu obiektów.

• API = schemat bazy. Kontrakt API jest przyspawany do schematu bazy. Zmieniasz kolumnę w bazie - psujesz wszystkich klientów API.

A jeśli przyjmujesz encje również w POST, dochodzi bonus: over-posting. Ktoś dośle Ci w żądaniu pole "isAdmin": true - i encja grzecznie je przyjmie.

Poprawka: DTO. Rekord z dokładnie tymi polami, które chcesz pokazać na zewnątrz. Select na zapytaniu i mapowanie gotowe:

/* UsersController.cs - DOBRZE */
public record UserDto(int Id, string Email);

[HttpGet]
public async Task<IActionResult> GetUsers()
{
    var users = await _db.Users
        .Select(u => new UserDto(u.Id, u.Email))
        .ToListAsync();

    return Ok(users);
}

3 linijki więcej, zero wycieków i pełna kontrola nad kontraktem API. Ten sam pomysł działa w drugą stronę: w POST przyjmuj osobny DTO tylko z tymi polami, które klient może ustawić, a over-posting nie ma się czego uchwycić.

Bonus: mniejszy SQL za darmo. Jest jeszcze korzyść, o której mało kto mówi. Select na IQueryable (czyli przed ToListAsync, a nie na gotowej liście w pamięci) sprawia, że Entity Framework pobiera z bazy tylko te 2 kolumny, a nie wszystkie kolumny encji:

/* bez .Select() - ŹLE */
SELECT [u].[Id], [u].[Email],
       [u].[PasswordHash],
       [u].[IsAdmin], ...
FROM [Users] AS [u]

/* z .Select(...) - DOBRZE */
SELECT [u].[Id], [u].[Email]
FROM [Users] AS [u]

Mniej danych z bazy, mniej pamięci, szybciej. Za darmo.

Zasada #3. Encje zostają w środku aplikacji. Na zewnątrz wychodzą tylko DTO.

Antywzorzec #4: złe czasy życia w DI

Czwarty antywzorzec to pomieszane czasy życia w Dependency Injection. Najpierw szybkie przypomnienie, 60 sekund:

• Transient - nowa instancja przy każdym wstrzyknięciu.
• Scoped - 1 instancja na 1 request HTTP.
• Singleton - 1 instancja na całe życie aplikacji.

Proste? Proste. Do czasu...

Scoped uwięziony w singletonie. Klasyk: serwis zarejestrowany jako singleton, który w konstruktorze bierze AppDbContext - a ten jest zarejestrowany jako scoped.

/* Program.cs - ŹLE */
builder.Services.AddSingleton<ReportService>();

/* ReportService.cs */
public class ReportService
{
    private readonly AppDbContext _db;      /* Scoped! */

    public ReportService(AppDbContext db)
    {
        _db = db;                           /* uwięziony na zawsze */
    }
}

Kompilator? Zero protestów. A bez walidacji scope'ów wszystko wygląda normalnie.

Captive dependency, czyli uwięziona zależność. Singleton żyje wiecznie. Więc DbContext, który miał żyć przez 1 request, też żyje wiecznie. Jeden, współdzielony przez wszystkie requesty naraz: request 1, request 2 i request 3 trafiają do tego samego ReportService, a ten cały czas trzyma ten sam AppDbContext.

Dlaczego to boli?

• Losowe wyjątki pod ruchem. DbContext nie jest thread-safe - 2 równoległe requesty i leci InvalidOperationException. Nie zawsze. Czasami. Najlepsze błędy do debugowania.

• Puchnący change tracker. Kontekst nigdy nie umiera, więc trzyma stare, zakeszowane dane, a zużycie pamięci ciągle rośnie.

• Development to nie produkcja. W typowym projekcie ASP.NET Core środowisko Development zwykle to wyłapie i rzuci wyjątkiem walidacji (odpowiada za to opcja ValidateScopes). Ale na produkcji ta walidacja jest domyślnie wyłączona.

Klasyczne miejsce, w którym to wypływa, to BackgroundService. On też jest singletonem, więc DbContext wstrzyknięty prosto do jego konstruktora od razu staje się uwięziony.

2 uczciwe wyjścia. Pierwsze: dopasuj czas życia, czyli zarejestruj serwis jako scoped:

/* Program.cs - opcja 1: dopasuj czas życia */
builder.Services.AddScoped<ReportService>();

Drugie: jeśli singleton naprawdę musi zostać singletonem (na przykład background service), nie wstrzykuj do niego DbContextu. Wstrzyknij IServiceScopeFactory i twórz scope ręcznie, na czas jednej operacji:

/* opcja 2: singleton MUSI zostać? Twórz scope ręcznie */
public class ReportService(IServiceScopeFactory factory)
{
    public async Task GenerateAsync()
    {
        using var scope = factory.CreateScope();
        var db = scope.ServiceProvider
                      .GetRequiredService<AppDbContext>();

        /* ... praca na świeżym, krótkim DbContext */
    }
}

Każde wywołanie GenerateAsync dostaje świeży, krótko żyjący DbContext, który znika razem ze swoim scope'em.

Zasada #4. Singleton nigdy nie trzyma zależności scoped. Masz wątpliwości? Bierz scoped.

Antywzorzec #5: try-catch wszędzie

I ostatni, najbardziej podstępny antywzorzec: try-catch wszędzie. Wygląda jak dbałość o jakość. Junior myśli: "zabezpieczam się". A tak naprawdę kopiuje ten sam blok do każdego endpointu: try, catch Exception, zwróć 500 z napisem "Coś poszło nie tak":

/* OrdersController.cs - ŹLE (i tak w 40 endpointach) */
[HttpGet("{id}")]
public async Task<IActionResult> GetOrder(int id)
{
    try
    {
        var order = await _orderService.GetAsync(id);
        return Ok(order);
    }
    catch (Exception ex)
    {
        return StatusCode(500, "Coś poszło nie tak");
    }
}

Defensywnie? Nie. Kopiuj-wklej. I to razy 40 endpointów w projekcie.

Wersja jeszcze gorsza: połknięty wyjątek. Czyli pusty catch:

/* PaymentsController.cs - ŹLE */
try
{
    await _paymentService.ChargeAsync(order);
}
catch (Exception)
{
    /* TODO: obsłużyć kiedyś */
}

Płatność się nie udała? Nikt się nie dowie. W logach cisza. Aplikacja "działa". To są dokładnie te bugi, których potem szukasz tygodniami.

Dlaczego to boli?

• 40 kopii kodu. Chcesz zmienić format błędu? Poprawiasz 40 plików.

• Debugowanie w ciemno. Połknięte wyjątki to szukanie po omacku: klient dzwoni, że nie działa, a w logach pusto.

• Wszystko kończy jako 500. Nawet zwykły błąd walidacji albo brak zasobu.

Poprawka: 1 handler na całą aplikację. Od .NET 8 masz do tego gotowy mechanizm: interfejs IExceptionHandler. 1 klasa na całą aplikację - logujesz wyjątek, zwracasz porządny ProblemDetails. Koniec.

/* GlobalExceptionHandler.cs - DOBRZE (.NET 8+) */
public class GlobalExceptionHandler(
    ILogger<GlobalExceptionHandler> logger) : IExceptionHandler
{
    public async ValueTask<bool> TryHandleAsync(
        HttpContext http, Exception ex, CancellationToken ct)
    {
        logger.LogError(ex, "Nieobsłużony wyjątek");

        http.Response.StatusCode = 500;
        await http.Response.WriteAsJsonAsync(new ProblemDetails
        {
            Status = 500,
            Title  = "Wystąpił błąd serwera"
        }, ct);

        return true;                 /* obsłużone */
    }
}

Zwrócenie true oznacza, że wyjątek został obsłużony. Gdybyś zwrócił false, ASP.NET Core przekaże go do kolejnego zarejestrowanego handlera - dzięki temu możesz mieć np. osobny handler dla konkretnego typu wyjątku. A błąd walidacji czy brak zasobu to zwykłe odpowiedzi (BadRequest, NotFound) zwracane przez kontroler, a nie wyjątki, więc nie muszą już kończyć jako 500.

Rejestracja to 3 linijki w Program.cs:

/* Program.cs - DOBRZE */
builder.Services.AddExceptionHandler<GlobalExceptionHandler>();
builder.Services.AddProblemDetails();

/* w pipeline: */
app.UseExceptionHandler();

Kiedy try-catch jest OK? Ważne: try-catch nie jest zakazany. Zostaje tam, gdzie umiesz na wyjątek zareagować:

• retry, czyli ponowienie operacji,
• fallback, czyli wariant zastępczy,
• konkretny komunikat dla użytkownika.

Całą resztę łapie globalny handler.

Zasada #5. Łap wyjątek tylko tam, gdzie wiesz, co z nim zrobić. Resztę loguj w jednym miejscu.

Ściąga: antywzorzec → poprawka

Wszystkie 5 antywzorców w jednej tabeli:

Podsumowanie

Wszystkie 5 antywzorców łączy jedno: przechodzą code review i działają na demo, a problemy pokazują dopiero pod prawdziwym ruchem albo po kilku miesiącach rozwoju projektu. 5 zasad na dziś:

1. Kontroler nie zawiera logiki - deleguje do serwisu.
2. Async all the way - zero .Result i .Wait().
3. Z API wychodzą DTO, nie encje.
4. Singleton nigdy nie trzyma scoped.
5. Jeden globalny handler zamiast try-catch wszędzie.

Ile trafień wyszło u Ciebie? Jeśli choć jedno, wiesz już, od czego zacząć najbliższy refactoring.

Jeśli ten artykuł był dla Ciebie wartościowy, mam coś więcej. Prowadzę społeczność, w której jest już ponad 10 000 programistów .NET. Raz w tygodniu wysyłam jednego maila: najważniejsze newsy ze świata .NET plus konkrety - antywzorce, praktyki i narzędzia, takie jak w tym tekście. Członkowie społeczności dostają też rabaty na moje kursy, których nie ma nigdzie indziej. Zero spamu - wypisujesz się jednym kliknięciem.

Wystarczy, że zostawisz swój adres tutaj: modestprogrammer.pl/vip. Powodzenia przy najbliższym code review :)