C#•4d ago

Trying to understand proper error handling

I am trying to write my code to properly handle errors and I am getting so hyper fixated on it that I can't tell whether I'm making good decisions or not. I have rewritten the way my code base handles errors several times and I'm not sure what the right approach is anymore. I originally started using my own implementation of the result pattern, then realized I have this issue where the status code wasn't correct when returning errors, so I said maybe my implementation isn't that good so I looked for a library and settled on ErrorOr, but soon realized its the same issue, I dont understand how you can tell the "result pattern" that this error is a 401, 500, or 400 etc so I gutted that out and started writting my own custom exceptions and made a middleware that handles all the exceptions but now Im reading that you shouldn't throw errors when you now how to handle them and that throwing excessive erros will cause performance issues.

84 Replies

CydoOP•4d ago

// Method using exceptions.
    public async Task<string> Login(LoginRequestDto requestDto)
    {
        var user = await _unitOfWork.User.GetAsync(u => u.Email == requestDto.Email);
        if (user == null)
            throw new NotFoundException("User not found");

        if (!await _userManager.CheckPasswordAsync(user, requestDto.Password))
            throw new ValidationException("email", "Invalid username or password");

        var sessionId = $"SESS{Guid.NewGuid()}";
        var refreshToken = _tokenService.CreateRefreshToken();

        await _sessionService.CreateSession(user.Id, sessionId, refreshToken);
        _tokenService.CreateAccessToken(user.Id, sessionId);

        return "Login successful";
    }

// Method using exceptions.
    public async Task<string> Login(LoginRequestDto requestDto)
    {
        var user = await _unitOfWork.User.GetAsync(u => u.Email == requestDto.Email);
        if (user == null)
            throw new NotFoundException("User not found");

        if (!await _userManager.CheckPasswordAsync(user, requestDto.Password))
            throw new ValidationException("email", "Invalid username or password");

        var sessionId = $"SESS{Guid.NewGuid()}";
        var refreshToken = _tokenService.CreateRefreshToken();

        await _sessionService.CreateSession(user.Id, sessionId, refreshToken);
        _tokenService.CreateAccessToken(user.Id, sessionId);

        return "Login successful";
    }

// Using my own custom result pattern
    public async Task<Result> Login(LoginRequestDto requestDto)
    {
        var user = await _unitOfWork.User.GetAsync(u => u.Email == requestDto.Email);
        if (user == null || !await _userManager.CheckPasswordAsync(user, requestDto.Password))
            return Result.Failure("email", new[] { "Invalid username or password" });

        var sessionId = $"SESS{Guid.NewGuid()}";
        var refreshToken = _tokenService.CreateRefreshToken();

        await _sessionService.CreateSession(user.Id, sessionId, refreshToken);

        _tokenService.CreateAccessToken(user.Id, sessionId);
        return Result.Success("Login successful");
    }

// Using my own custom result pattern
    public async Task<Result> Login(LoginRequestDto requestDto)
    {
        var user = await _unitOfWork.User.GetAsync(u => u.Email == requestDto.Email);
        if (user == null || !await _userManager.CheckPasswordAsync(user, requestDto.Password))
            return Result.Failure("email", new[] { "Invalid username or password" });

        var sessionId = $"SESS{Guid.NewGuid()}";
        var refreshToken = _tokenService.CreateRefreshToken();

        await _sessionService.CreateSession(user.Id, sessionId, refreshToken);

        _tokenService.CreateAccessToken(user.Id, sessionId);
        return Result.Success("Login successful");
    }

ero•4d ago

don't you just use IActionResult in asp and return Ok() or Unauthorized() or BadRequest() or whatever?

CydoOP•4d ago

Well I do use those, for instance in my globall exception handler, but i dnt understand how uw ould use those with something like the result pattern, like yes I can do it with one but if the result pattern doesnt have a status code how can u do like a switch case to match ur api response's to be wrapped in the correct one of those methods?

ero•4d ago

i guess i'm questioning why you need results at all instead of just using ActionResult from asp?

CydoOP•4d ago

isnt the result pattern and action result compeltely different things?

ero•4d ago

maybe? depends on the implementation of either. i'm just wondering why you feel the need to do this instead of using what's there?

CydoOP•4d ago

I am using IActionResult and those different methods inside my controller, but not in my service methods Im trying to handle errors in the service method and then send a formatted api response in my controller whether something error or it was successful After reading all the ErrorOr documentation and chatgpting i think I understand what i need to do now

ero•4d ago

knowing that it's in a service makes more sense. anyway you can just have your result type contain the error type? what's the issue there?

CydoOP•4d ago

if u handle the error in the service method, i didnt understand how u can tell the controller to assign the correct status code cause error returns all errors in a results.Errrors list so theres like no http status code or anything

ero•4d ago

:uhh: is there like something stopping you from adding the status code to the error list or

CydoOP•4d ago

from my understanding, in the ErrorOr library u cant but maybe i just add my own custom codes

ero•4d ago

oh, the Result type is from another library, not your own i mean a result type is really easy to write, you can just do it yourself really depends on how complicated you need it to be, but the most basic implementation you can think of might be

readonly record struct Result(
  int StatusCode);

readonly record struct Result(
  int StatusCode);

CydoOP•4d ago

Yeah I think im gonna try to write my own, i dont need anthing complex, i just want to be able to map errors to certain httpstatuscodes, and be able to take in either a string error message or a dictionary for validation errors that should be mapped to specific form fields, like password: "Password must be 8 characters" @ero I created my own Result class, prob overengineered this but maybe if you read the code you will have a better understanding of what I was trying to do and then could tell me if what I'm doing is stupid or not 😅 https://gist.github.com/CydoEntis/61d06312eacdc372e927b2b79d0dc358

shua•4d ago

what on earth are you doing? throw the exception that occurs by using throw or let it naturally throw and catch that exception and return the status code (Unauth, Forbidden.. etc) and in the payload return ex.Message (if this is sensitive). Also by the way, you should rename Login to LoginAsync() since it's an async method. It will be confusing later on A quick example though, instead of mapping to some crazy class DTO with a custom message, just create a custom exception for your specific scnearios (IFF it makes sense) For example

if (user is null)
  throw UserNotFoundException("..");

if (user is null)
  throw UserNotFoundException("..");

CydoOP•4d ago

I am using the result pattern instead of throwing exceptions, since u shouldnt throw exceptions for things like user == null or string.IsNullOrEmpty(userId)

shua•4d ago

Then you can, in the base custom exception message, customize it how you want and pass it in

since u shouldnt throw exceptions for things like user == null or string.IsNullOrEmpty(userId)

what??

CydoOP•4d ago

this is how my original iteration of this application worked

shua•4d ago

What do you mean by that exactly? "Shouldn't throw exceptions for things like null "? It's quite the opposite!

CydoOP•4d ago

Ok for example, in another non refactored section of my code base, i do the exact thing you suggest

    public async Task<IActionResult> ChangePassword([FromBody] ChangePasswordRequestDto requestDto)
    {
        var userId = User.FindFirstValue(ClaimTypes.NameIdentifier);

        if (string.IsNullOrEmpty(userId))
            throw new UnauthorizedAccessException("User does not have permission");


        var result = await _authService.ChangePasswordAsync(userId, requestDto);
        return Ok(ApiResponse<object>.SuccessResponse(result));
    }

    public async Task<IActionResult> ChangePassword([FromBody] ChangePasswordRequestDto requestDto)
    {
        var userId = User.FindFirstValue(ClaimTypes.NameIdentifier);

        if (string.IsNullOrEmpty(userId))
            throw new UnauthorizedAccessException("User does not have permission");


        var result = await _authService.ChangePasswordAsync(userId, requestDto);
        return Ok(ApiResponse<object>.SuccessResponse(result));
    }

shua•4d ago

You don't have to even check if user is null you could just do ArgumentException.ThrowIfNullOrEmpty https://learn.microsoft.com/en-us/dotnet/api/system.argumentexception.throwifnullorempty?view=net-9.0

ArgumentException.ThrowIfNullOrEmpty(String, String) Method (System)

Throws an exception if argument is null or empty.

shua•4d ago

There is absolutely nothing wrong with this. What's the issue? That's what you're supposed to do.

CydoOP•4d ago

But i read that exceptions should only be thrown under unexpected or exceptional conditions, so since I know that there can be an error if user is null, intstead of throwing it, i should just handle it, in my case using the result pattern

shua•4d ago

Where did you read that, that is not true at all.

CydoOP•4d ago

There was no issue, i just kept reading deeper and deeper and found this, that made me think okay maybe throwing exceptions isn't the best approach. It also stated, that throwing exceptions is a performance expensive task, so they shouldnt be thrown in cases where you can just handle them.

shua•4d ago

An exception should be thrown when anything could go wrong. That is inclusive of null or empty strings or anything that doesn't satisfy getting to the next step Can you link me this article? That is not accurate Also, think about it, if you are throwing an exception, the performance is irrelevant LOL the program will blow

CydoOP•4d ago

I dont have the article on hand, this was a few days ago, I was stumped on how to refactor this app to use this result pattern thing this entire time lol like here's an example of another method that isn't refactored

    public async Task<string> RefreshTokens()
    {
        var refreshToken = _cookieService.Get("QB-REFRESH-TOKEN");
        if (string.IsNullOrEmpty(refreshToken))
            throw new NotFoundException("Refresh token not found");


        var session = await _unitOfWork.Session.GetAsync(s => s.RefreshToken == refreshToken);
        if (session == null)
            throw new NotFoundException("Session not found");

        if (_tokenService.IsRefreshTokenValid(session, refreshToken))
            throw new InvalidTokenException("Refresh token expired");

        var user = await _unitOfWork.User.GetAsync(u => u.Id == session.UserId);
        if (user == null)
            throw new NotFoundException("User not found");

        var accessToken = _tokenService.CreateAccessToken(user.Id, session.SessionId);
        var refreshTokenModel = _tokenService.CreateRefreshToken();
        session.RefreshToken = refreshTokenModel.Token;
        session.RefreshTokenExpiry = refreshTokenModel.Expiry;

        await _unitOfWork.Session.UpdateAsync(session);
        return "Tokens refreshed successfully";
    }

    public async Task<string> RefreshTokens()
    {
        var refreshToken = _cookieService.Get("QB-REFRESH-TOKEN");
        if (string.IsNullOrEmpty(refreshToken))
            throw new NotFoundException("Refresh token not found");


        var session = await _unitOfWork.Session.GetAsync(s => s.RefreshToken == refreshToken);
        if (session == null)
            throw new NotFoundException("Session not found");

        if (_tokenService.IsRefreshTokenValid(session, refreshToken))
            throw new InvalidTokenException("Refresh token expired");

        var user = await _unitOfWork.User.GetAsync(u => u.Id == session.UserId);
        if (user == null)
            throw new NotFoundException("User not found");

        var accessToken = _tokenService.CreateAccessToken(user.Id, session.SessionId);
        var refreshTokenModel = _tokenService.CreateRefreshToken();
        session.RefreshToken = refreshTokenModel.Token;
        session.RefreshTokenExpiry = refreshTokenModel.Expiry;

        await _unitOfWork.Session.UpdateAsync(session);
        return "Tokens refreshed successfully";
    }

there's nothing wrong with this?

shua•4d ago

Yeah, I would not read into that. Throwing exceptions is not performance expensive, and even if it was (it wasn't) it wouldn't matter since that's the whole point of throwing looking.. 1 sec.

ero•4d ago

especially for something like a web api, throwing is kinda bad and pointless the app isn't supposed to "blow up"

shua•4d ago

Nothing wrong with that but I would use this https://learn.microsoft.com/en-us/dotnet/api/system.argumentexception.throwifnullorempty?view=net-9.0

ArgumentException.ThrowIfNullOrEmpty(String, String) Method (System)

Throws an exception if argument is null or empty.

ero•4d ago

it must keep running

shua•4d ago

????

CydoOP•4d ago

instead of writing my own custom error exceptions?

shua•4d ago

Throwing is important if something bad is happening and you need to stop that instance. VERY important lol

ero•4d ago

you're not gonna have your entire server shut down just because 1 user entered the wrong data in an api request

shua•4d ago

It's just not that different from the generic one , but you could

ero•4d ago

throwing is just the wrong thing here

shua•4d ago

Who said anything about shutting the entire server down?

CydoOP•4d ago

Lol, see this is where i get overwhelmed and confused, one side says throw errors other side says you shouldn't

shua•4d ago

Throwing errors is how you stop your application from processing something that has a problem. You should always throw when nec, and always LOG errors. Suppressing is almost always a bad idea UNLESS, you have a poison queue or a very huge process where you dont want to throw because you dont want to crash the for loop or whataever it is

ero•4d ago

how is throwing useful here, if you're just gonna catch the exception one layer up? it's useless

shua•4d ago

Because he said he was returning a 401 It's a web API right?

CydoOP•4d ago

yeah a web api

shua•4d ago

& It's not useless at all, it's good for logging or returning non-sensitive stack trace info to the caller

CydoOP•4d ago

Its too long to post it here, but i have a global exception handler middleware that catches the errors and formats them into an ApiError, so i im not catching errors all over the place. Also while its on my mind, is there a simpler approach then making my own ApiResponse? so all the api responses follow the same format?

shua•4d ago

How are you doing it now? I would imagine just making a class to represent the ApiResponse

CydoOP•4d ago

I basically made a class called ApiResponse, that has these properties

    public bool Success { get; set; }
    public T Data { get; set; }
    public ErrorDetail ErrorDetail { get; set; }

    public bool Success { get; set; }
    public T Data { get; set; }
    public ErrorDetail ErrorDetail { get; set; }

and then based on the Exception i fill out that api response and return it wrapped in like a BadRequest() or an Ok() etc

shua•4d ago

Looks good to me, you could have overloaded constructors when you need to pass in diff stuff or massage the info differently If you're using Web API I think they have a HttpRequestMessage class, and in Azure Functions its HttpResponseMessage (I think)

CydoOP•4d ago

Sweet, are there any simpler ways to handle formatting different errors? like for example some of my methods return just a straight up string message for an error and other's for like forms return a dictionary where the key is the field and value is the message Never can find any resources that go over how to create api responses that explain how to structure ur errors for things like that Because in my front end i like to catch the error in my submit method and then map it directly to the input so it shows under the input,

shua•4d ago

I think it's kind of up to you unless you pull in a custom nuget that does that sort of thing. You could do something like a IApiResponse CustomResponse: IApiResponse and have multiple classes , or something like that or differently formatted ones.. not really sure tbh, ctor overloads seem to be the way to go Can you give me an example where 2 would be entirely different responses?

CydoOP•4d ago

Let me see if i have an example if not i can write one up. Ok so say you have these two different exceptions

if (user == null)
    throw new UserNotFoundException("User does not exist.");

if (!await _userManager.CheckPasswordAsync(user, requestDto.Password))
    throw new ValidationException("password", "Invalid password");

if (user == null)
    throw new UserNotFoundException("User does not exist.");

if (!await _userManager.CheckPasswordAsync(user, requestDto.Password))
    throw new ValidationException("password", "Invalid password");

the first one would send back a json response like this

{
  success: false,
  data: null,
  error: "User does not exist",
}

{
  success: false,
  data: null,
  error: "User does not exist",
}

but the 2nd one would return like this

{
  success: false,
  data: null,
  error: [
    {"password": "Invalid Password"}
  ]
}

{
  success: false,
  data: null,
  error: [
    {"password": "Invalid Password"}
  ]
}

I don't exactly understand how you can have that error field be of different types but the same key if that makes sense.

shua•4d ago

Prob a dictionary yeah the way to go, have you ever used params keyword? It might be useful

CydoOP•4d ago

I saw this when i was asking Chatgpt for help lol

shua•4d ago

haha yeah I mean, I would say the easiest thing is a dictionary<string,string> or a Dictionary<IType, string>

CydoOP•4d ago

what about for the errors that arent neccessarily needed in a dictionary?

shua•4d ago

you know where the string is the message, and IType is some custom type (if u even need to go that deep) You could categorize them as Generic so for example you will always have a dictionary but "generic": "Some message"

CydoOP•4d ago

thats what i was doing in the first ever iteration of this app, but then like Fluent Validation, was sending errors exactly how i wanted and im like i want that lol

shua•4d ago

:Shrug: im just spitballing oh yeah FluentValidation

CydoOP•4d ago

this app has been literally complete for 2 months and i keep going back trying to make it perfect and its killing me xD been learning C# and .NET for like 140 days now and ive almost made myself rage quit this ecosystem 😂

shua•4d ago

nothing wrong with that that will make you a great programmer refactors never stop!

CydoOP•4d ago

Yeah, u right, just wanna work on something else but i refuse till i really understand wtf im doing

shua•4d ago

whats link to ur repo? or is it private

CydoOP•4d ago

its private, but it could def be public, the apps gonna go on my portfolio if i ever finish it and figure out how to deploy a .net app lol not at my computer but tomorrow, i can make it public and post the link here, it could prob use some hard code review, so i can see where to make improvements tbh

Anton•4d ago

@Cydo I had the same problem with the codes. Ended up solving it by mapping the error code at a later point (I think you did the same). The problem is that it's not going to be documented properly unless you make your possible errors known declaratively, so I also had to make a swagger extension and decorate the possible errors on each action. And after that I decided to work through having the possible errors be implied, which means having the errors sort of propagate declaratively through the whole pipeline. I have developed some ideas, but haven't solved it completely yet. There are so many things that you'd think should be solved that there isn't a good solution to, with complete attention to detail and maximum code / configuration reuse. People just don't care enough

CydoOP•3d ago

Yeah when I was writing the one implementation, it always seemed like I had to go back and add one more thing that I didnt think of at the time and it was getting more complex and overwhelming each iteration. I'm currently going back and just switching to throw exceptions now, my goal has changed from making this thing perfect using all these "design patterns" to just functioning deployed app.

Anton•3d ago

throwing exceptions doesn't solve the problem with documentation though In fact, it doesn't really solve anything it's just less handling code overall and it's easier to just delegate the handling to something else, whatever does the handling, without using an abstraction or injecting a dependency

CydoOP•3d ago

@shua Turns out the repo was public this entire time lol, Im still trying to refactor everything back to using exceptions instead of my shotty Result pattern so its a bit of a messge in there, but i refactored at least the auth, and party service if u wanted to take a look and potentionally vomit https://github.com/CydoEntis/collabparty-backend

Anton•3d ago

I mean you can't do better with the regular tools If you need to do validation after you've bound the model The thing I do different is a single action per controller And I would only do interfaces for the services if I have tests for them otherwise they're just a pointless abstraction

CydoOP•3d ago

wdym a single action per controller? I wanna add testing, just havent learned it yet, its on the list tho lol

Joschi•2d ago

Some people only have one endpoint in each controller class when using mvc. It is often done to reduce the dependencies in each controller. Regarding your problems above with exceptions and results. The advantage of an Result implementation is, that you are forced to handle errors. And you are aware that errors could happen in a method. Unless it is documented or you read the methods source, you cannot know if and which exceptions it may throw. In APIs this will result in 500 responses, which is often undesired. What we currently do is to map our different Error types to the corresponding api error. So a NotFoundError would return a 404. Having it well documented in an OpenApi schema, without manually defining the possible errors that could happen at this specific endpoint is really hard. Probably even impossible in MVC. Mainly because everything is boiled down to IActionResult. And its hard to know at compile time which actual types could maybe occour. But honestly not that important unless you develop an API meant for public consumption.

CydoOP•2d ago

The one method per controller seems insane to me, but having only been learning .NET for 144 days I'll leave that to my inexperience lol. So overall, its probably better to use a result pattern and handle those errors then to use exceptions since its harder for outside users of an API to understand why/when an exception will be thrown? But I also thought you can decorate ur endpoints in ur controller with [ProducesResponseType(typeof(ErrorResponse), StatusCodes.Status404NotFound)] and that will document ur API's possible responses in Swagger? Or do you mean that even if it's documented like that, it doesnt actually tell you why/what will cause those exceptions to be thrown regardless?

Anton•2d ago

It is technically possible, a ready solution just hasn't been made yet it being documented is important in all cases imo you wanna generate your frontend client You do the same for the services if you have any so they only handle one thing Different methods in a service may have different dependencies So it's a way to only resolve the dependencies that you need to do the concrete thing in an ideal world there would be no controller at all just a handler class that lets it generate an endpoint by convention or if you have something more concrete, by sharing the wrapping logic around it anyway, ignore my ramblings if you're just starting it's coming from thinking about it a lot and trying things The problem with annotations is that they are not connected to the actual source code They can lie

CydoOP•2d ago

Is that what you do for minimal api's?

Anton•2d ago

And the compiler won't detect that No, minimal API's are the same thing, just without a class

CydoOP•2d ago

Haha fair, I come from mainly frontend development and my previous job was wordpress so using C#/.NET compared to react and just vanilla php is a different beast

Anton•2d ago

You still need a model for the json body and you need to include any parameters in the url That should be more dynamic so you don't have to repeat that across many endpoints

CydoOP•2d ago

Gotcha, I thought i saw somewhere that when u use minimal api, you make like action specific files so like POST would just be a file containting a post method for a specific route etc.

Anton•2d ago

You can't share this structure with the systems we have right now I mean yeah they are easier to generalize, because you don't have to have the controller classes, you can create things in a loop just as well But it's not nearly advanced enough as is

CydoOP•2d ago

Gotcha my next plan after what im currently working on is to learn minimal api, is that a good idea or should I just focus on improving with MVC and such, aiming for a job in the .NET ecosystem lol

Anton•2d ago

I mean digging deeper is not mistake Thier basic idea is that the metadata model for an action is the delegate While MVC has some heavier metadata structures And MVC has lots of legacy

CydoOP•2d ago

Yeah, sometimes it just feels like I'm doing way more in MVC to get a simple api going then I should really need to, so I figured switching to using minimal api would let me bring my ideas to life much quicker, i've read minimal api is a lot closer to express which is/was the backend i am most use to using, except I have moved away from even touching Js/Ts for anything outside the frontend lol

Anton•2d ago

mvc has more steps in the pipeline out of the box, in minimal APIs you have to add them manually

CydoOP•2d ago

Did not know that, thought it was all just the same stuff with out controllers tbh.

Joschi•2d ago

Would you elaborate how? Because that would be something really interesting to build. To me it sounds like you should give minimal apis with the REPR pattern a try. Like FastEndpoints. It also has strongly typed endpoint methods with union responses, which will end up in the OpenApi schema. Not too familiar with generation, because we work with blazor and just share our dtos. I only used Kiota (a few years ago) and that doesn't really care about your defined errors. It will just throw exceptions on non success in any case.

Anton•2d ago

My idea is basically that you make a pipeline where you combine multiple steps together like middlewares, and where each step declares its possible errors. The errors can be either specified manually, or source generated from the result type of a method that does the step. I made a source gen that generates the result type from what types or values you return in the method. When you build the pipeline by combining the steps, you can get the error set of the whole pipeline by just looking at what each step returns. This way it's easier to combine unrelated logic without mentioning the possible errors of the other system explicitly (you just specify your own errors). All this is complemented with a precindition system, where you can check if the context a step depends on has been specified at an earlier step. Once again, without running or testing the whole pipeline. Then you can map the errors and check at startup if all the errors have been handled there. Sure, the kind of problem is that you have to trust what errors each step declares, but it's more localized so it's easier not to mess up. Additionally, because each step is modular, you won't have to repeat the logic anywhere. Including the context passing logic. I haven't developed this idea fully, but I'll get back to it in some time Since the errors and the whole context are known at startup, you can inject that info from the pipeline into OpenAPI descriptors So you get perfect information without decorating the action method at all, it's propagated automatically

Gaming

Programming

Trying to understand proper error handling

Did you find this page helpful?