Failing at API dev

@Solarion ,i got distracted earlier but I also see you used fromBody, that should not be necessary for post/put, those by default read the body to parse into a object. It's used more for when you want to pass a body object for a get ie: complex search parameters that would make route and query params ugly or difficult.

Post /put are generally object calls ie complex object/DTO's are submitted vs parameters ( query or route)
 
Kosmik said:
@Solarion ,i got distracted earlier but I also see you used fromBody, that should not be necessary for post/put, those by default read the body to parse into a object. It's used more for when you want to pass a body object for a get ie: complex search parameters that would make route and query params ugly or difficult.

Post /put are generally object calls ie complex object/DTO's are submitted vs parameters ( query or route)
Click to expand...

That is actually very helpful thank-you. I am still just a beginner here so trying to well, get a structured methodology going of what to use where and what not to use where. This what you have said has cleared up some confusion regarding some of my end points which I thought demanded a body in addition to a query parameter, and this led to sometimes the body only have 1 field. Knew it was wrong but couldn't figure out why. Going back to it now.
 
Solarion said:
Oh yeah. The reason I did that is because whether it returns a success or a fail, it still returns as nothing in my MVC Service.

I'll explain. In the below, say I get a success or fail from the API, this section here reflects that response message



However on the next line, it's completely lost. It returns as empty.
Click to expand...
Well, you have a response object. This will include the status code for the request. And it's quite easy to interrogate that status code within the frontend to trigger a different flow there.

Before getting into status codes, lets take a step back and consider what the endpoint is to do.

This endpoint should remove a role(s) from a user's profile, provided that the user-role combination exists.

If you can remove a user's role with this endpoint, then you're done. That the request succeeded implies that the role(s) has been removed. So a response body seems redundant. Therefore, simply respond with an OK() (or NoContent()).

If, for some reason, you cannot remove some role from the user's profile when consuming this endpoint, that's not OK() ;) Some (business rule or other) error precluded role removal. For these occurrences, respond with an appropriate status code to specify the problem. Here's a non-exhaustive list of errors and a corresponding (and appropriate) status code:

user doesn't exist404
user-role combination doesn't exist404 (or 422 perhaps if you prefer completely separate status codes. note 404 is more appropriate imo)
user performing request doesn't have rights to perform role removal on target user403
Db (or AD/whatever supporting upstream service) unavailable503

Feel free to add response bodies if you'd want to provide extra information to the frontend.

JavaScript: 
// some crappy JS that could be riddled with errors
this.http
  .post('http://thecatapi.com/path/to/role/removal', body, opts)
  .then(this.interrogateResponse)
  .then(this.handleSuccessfulResponse.bind(this))
  .catch(this.handleErrorResponse.bind(this));

private interrogateResponse(res: Response) {
   if (res.statusCode > 399) {
       throw res;
   }
}

private handleErrorResponse(res: Response) {
    switch (res.statusCode) {
        case 400: //handle BadRequest
        case 404: //handle NotFound
        //...etc.
        default: //handle everything else
    }
}
 
Please can someone look at this for me. Looking for an outsider perspective/quick code review. Unless my eyes are playing tricks with me, it all looks wrong; the return types, the value checks. Sometimes we go down a road and then go down the wrong road!

C#: 
// GET api/v1/users/{userId}
[HttpGet(ApiRoutes.Users.FindById)]
public async Task<IActionResult> GetUser(string userId)
{
    try
    {
        var user = await _userManager.FindByIdAsync(userId);

        if(user == null)
        {
            return NotFound(new UserRequestResult
            {
                Succeeded = true,
                User = null
            });
        }

        return Ok(new UserRequestResult
        {
                Succeeded = true,
                User = user
        });
    }
    catch (Exception ex)
    {
        return BadRequest(new UserRequestResult
        {
            Errors = new[] { ex.ToString() }
        });
    }
}

C#: 
public class UserRequestResult
{
    public User User { get; set; }
    public bool Succeeded { get; set; }
    public IEnumerable<string> Errors { get; set; }
}

IF RESULT

C#: 
{
  "user": {
    "firstName": "Tom",
    "lastName": "Smith",
    "remunerationRate": 35,
    "id": "65f9c18b-eb67-4b08-a3d1-8nw1f0219df6",
    "userName": "[email protected]",
    "normalizedUserName": "[email protected]",
    "email": "[email protected]",
    "normalizedEmail": "[email protected]",
    "emailConfirmed": true,
    "phoneNumber": "0044 34 5675 342",
  },
  "succeeded": true,
  "errors": null
    }

IF EMPTY

C#: 
{
  "user": null,
  "succeeded": true,
  "errors": null
}
 
I'd limit the object but a 404 and 200 are fine to return , the person is adding a wrapped object. Had similar as folks ( some devs ) would also have a hissy fit when just returning a 404 as it was deemed a web failure vs data failure so they used to complain that the api was failing.

Both ways are actually correct. An array of errors is nice if its a servicing api BUT there is also a pool of thought that one should be brief. Too much explanation allows for manipulation.

What are your thoughts?

Exception is dodgy though. Bad request should only be used for a malformed submission.
 
Thanks Kosmik. I'm about to drive 30km's but will respond when I get home. Good thoughts on the Exception yeah.
 
Kosmik said:
I'd limit the object but a 404 and 200 are fine to return , the person is adding a wrapped object. Had similar as folks ( some devs ) would also have a hissy fit when just returning a 404 as it was deemed a web failure vs data failure so they used to complain that the api was failing.

Both ways are actually correct. An array of errors is nice if its a servicing api BUT there is also a pool of thought that one should be brief. Too much explanation allows for manipulation.

What are your thoughts?

Exception is dodgy though. Bad request should only be used for a malformed submission.
Click to expand...

Thanks Kosmik. Alrighty I have modified the Exception a little. Not to say it's better but maybe a little better.

Going to work on limiting the User information returned now.

C#: 
// GET api/v1/users/{userId}
[HttpGet(ApiRoutes.Users.FindById)]
public async Task<IActionResult> GetUser(string userId)
{
    try
    {
        var user = await _userManager.FindByIdAsync(userId);

        if(user == null)
        {
            return NotFound(new UserRequestResult
            {
                Succeeded = true,
                User = null
            });
        }

        return Ok(new UserRequestResult
        {
                Succeeded = true,
                User = user
        });
    }
    catch (Exception ex)
    {
        _logger.LogError(ex, "Error in AccountController - GetUser Method"); //Serilog

        return Json(new UserRequestResult
        {
            Errors = new[] { "An unexpected error has occurred on the server.  Please contact the administrator." }
        });
    }
}

To generate an error I simply Paused SQL Server. The logged response is quite immense as you can imagine. This is the returned response.

C#: 
{
  "user": null,
  "succeeded": false,
  "errors": [
    "An unexpected error has occurred on the server.  Please contact the administrator."
  ]
}
 
not a great fan of these wrappers TBH

if you are doing something like GraphQL for example, where every response is a 200, then having a wrapper and success field makes sense.

but a 200 succeeds by definition. there is no need to tell the client in the body that it succeeded.

same with a failure. if it fails, obviously the user is null, and obviously the succeeded is false.

if you MUST use a wrapper object, use something like `data` instead of `user`

will make your API easier to use on the client side
 
_kabal_ said:
not a great fan of these wrappers TBH

if you are doing something like GraphQL for example, where every response is a 200, then having a wrapper and success field makes sense.

but a 200 succeeds by definition. there is no need to tell the client in the body that it succeeded.

same with a failure. if it fails, obviously the user is null, and obviously the succeeded is false.

if you MUST use a wrapper object, use something like `data` instead of `user`

will make your API easier to use on the client side
Click to expand...

I think the philosophy I'm going with by using wrapper classes is that no matter what the outcome on the API side, there will always be feedback. If there are no records you will not be presented with just a 404 null but the following:

C#: 
{
  "user": null,
  "succeeded": true,
  "Message": "No record found."
}

If there is a record found then

C#: 
{
  "user": {
    "firstName": "John",
    "lastName": "Smith",
    "userName": "[email protected]",
    "emailConfirmed": true,
    "phoneNumber": "072 343 3434"
  },
  "succeeded": true,
  "Message": null
}

And if the server has shat itself then

C#: 
{
  "Succeeded": false,
  "Message": "An unexpected error occurred.  Please try again later."
}
 
Last edited:
This is already part of the http REST “spec”

200 = success
404 = not found
500 = unexpected error

There is definitely room to have an error class/object that returned expected errors. This object most likely contains an error code and some friendly text. These are business errors.

javascript clients like fetch and axios already handle this natively too - either in the catch block when using “await”, or in the catch handler when using promises.

When in doubt, look about.
docs.github.com

GitHub REST API documentation - GitHub Docs

Create integrations, retrieve data, and automate your workflows with the GitHub REST API.
docs.github.com docs.github.com

Stripe API reference – curl

Complete reference documentation for the Stripe API. Includes code snippets and examples for our Python, Java, PHP, Node.js, Go, Ruby, and .NET libraries.
stripe.com stripe.com
These are just 2 large, documented api’s off the top of my head

At the end of the day if you API is predicable and consistent, then that is a fine solution
 
Thanks kabal I will check those links out.

That is essentially what I'm aiming for is consistency. It will always return a similar format as above.
 
Just a suggestion, where are you [anonymous] [authorized] decorators on the API methods.

I always return ok(object) if query is successful, return badReqest(something nice to say to user) or

if in a try catch exception return a log entry for debugging, then badReqest (message" (normally a nice user message, we use a custom localizer for language)

If you oneday whant to expose these to say a mobile app or Vue app, electron or whatever learn about CORS and preflighting

Don't rely on front end validation, validate your model fields with the [required] decoration on your get/set propeties then in in the controller action first thing use this If(validate Model) do work Else " return an bad request and possibly a message "first name is required"

Backend dev high on Ritalin and coffee at 4 am tells you don't trust your. Forms validation.
 
Last edited:
Backend should always validate, particularly api. My philosophy is to always assume in anything coming in, is suspect and prone to manipulation.

I agree with @_kabal_ ,the rest spec has all defined response codes to calls, the only one that is ever really questionable is the 404 as it confuses people with resource availability vs network failure.

But I certainly don't like using the bare minimum of just ok, not found and bad request. Read the rest spec, understand 200,201,204 differances and where to use them. Same with errors, 4xx is client, 5xx is server, you do a disservice by not using it correctly and staging in a bad request.

With wrappers, the last of that implementation I used was very similar but for the main returned object I would use a dynamic type as a property called data. That way the wrapper was independant of my other models but able to be used for them all. Keep in mind, while flexible, dynamic means the objects type is only validated on run time of its execution, it is not precompiled like a known type or even var which is defined during build.
 
Kosmik said:
Ha! Hit me up via dm for coffee sometime if you keen :p
Click to expand...

You too also Durbs lol here I was thinking the only mybroadband oke still left here :laugh:

Yeah lets.

Oh fyi I'm doing interview prep too for an overseas job interview tomorrow. Holland nogal. We'll see.
 
Solarion said:
You too also Durbs lol here I was thinking the only mybroadband oke still left here :laugh:

Yeah lets.

Oh fyi I'm doing interview prep too for an overseas job interview tomorrow. Holland nogal. We'll see.
Click to expand...

Kosmik said:
Lol you do know @ToxicBunny is one of us too :D
Click to expand...
I keep saying we should do a meet, but alas. This is Durban, laziness is unmeasurable
 
Afon Kulikov said:
I keep saying we should do a meet, but alas. This is Durban, laziness is unmeasurable
Click to expand...

Yeah we're all bliksems lazy, and driving more than 2kms is just a hack :P

But same as @Kosmik , hit me up via PM for a coffee sometime.

I'm not all into the Dev space, but I can over complicate your life with Infrastructure architecture and governance/compliance crap if you want to have your head implode :D
 
You must log in or register to reply here.
Back
Top
Sign up to the MyBroadband newsletter
X