ASP.NET MVC / APIs - DELETE method not returning correct response

Question

I have a DELETE and PUT method which contain the same code to get the list of Ids from the database and check to see if they exist before performing the action:

try
{
    var ids =
        (await _testTableTbRepository.GetAllAsync())
        .Select(_ => new TestTableTbModel { Id = _.Id }).OfType<int>().ToList();

    if (!ids.Contains(model.Id))
    {
        return StatusCode(400, "Id: " + model.Id + " does not exist");
    }

    await _testTableTbRepository.DeleteAsync(model.Id);
    return Ok(model.Id);
}
catch (Exception e)
{
    return StatusCode(500, e);
}

My thought is that the conversion of Ids to integers is not working as it should, so it keeps getting caught at the if-statement and returning a 400 status. Does anyone see anything wrong with this? Is there a better way to write this?

I am new to this. While testing these methods I found that they would still return a 200 Status code, which is acting as if it is ok. — tb517
– tb517, Commented Jan 23, 2018 at 15:41
a 400 is a bad request. it denotes something when wrong with the request. i.e. model binder couldn't map the params. if you just can't find an entity, use a 404 'not found.' — Fran
– Fran, Commented Jan 23, 2018 at 15:43
also don't do a GetAllAsync() when you are searching for a record. That will bring back the entire table and you will be searching in memory instead of letting the db query the record. — Fran
– Fran, Commented Jan 23, 2018 at 15:44

Hey24sheep · Accepted Answer · 2018-01-23 15:48:15Z

2

Why all this hassle? Let DB Help you Use DBException instead.

This way you wont load a huge db in your memory and save one giant call plus all that select and contain checks.

try
{
    await _testTableTbRepository.DeleteAsync(model.Id);
    return Ok(model.Id);
}
catch(DbException e)
{
   return StatusCode(404, "Id: " + model.Id + " does not exist");
}
catch (Exception e)
{
    return StatusCode(500, e);
}

Thanks @Fran for pointing out 400 -> 404

edited Jan 23, 2018 at 15:48

answered Jan 23, 2018 at 15:40

Hey24sheep

1,2127 silver badges16 bronze badges

Sign up to request clarification or add additional context in comments.

5 Comments

tb517 Over a year ago

I didn't know it could be done this way, thank you. Although, I am getting "_testTableTbRepository does not exist in the current context"

Hey24sheep Over a year ago

@tb517 Then that means you do not have it in your code. Are you sure it is injected or defined? was it working before?

Hey24sheep Over a year ago

@tb517 I have updated my answer, made it better, try now.

tb517 Over a year ago

Yeah it was defined...I actually was in the process of changing it to the code you just edited. No errors in the code.

Hey24sheep Over a year ago

You are welcome, I am glad it helped you :) Don't forget to upvote and accept it as answer to close your question.

Fran · Accepted Answer · 2018-01-23 20:01:45Z

0

I'm not sure how _testTableRepository is implemented, but I making the assumption that it's just wrapping EF functions.

var exists = await _testTableRepository.FindAsync(model.Id);

if (!exists)
 return NotFound();

_testTableRepository.Delete(Model.Id);

return Ok(model.Id);

Don't bother with the try catch block. Most exceptions will automatically be converted to a 500 response code. If that's all your try/catch is doing, just let the framework handle it.

It's also easy enough to check by id for the record you want. Like I said about i'm making the assumption that your repository is just forwarding on the call to an EF DbContext, which will have a FindAsync() method on it.

edited Jan 23, 2018 at 20:01

answered Jan 23, 2018 at 17:51

Fran

6,5801 gold badge26 silver badges37 bronze badges

2 Comments

tb517 Over a year ago

There is a not a FindAsync() method. Should one be implemented in the repository? What is the best way to go about implementing that, if so? Sorry, this was not part of our training, so I'm trying to understand it. Thanks.

Fran Over a year ago

Since you don't provide the implementation of your repo in your question, I'm making the assumption your repository implementation reflects whatever it's wrapping and i'm taking a guess that's it's EF. EF has a FindAsync on it's DbSet. most ORM's will have a method to get an entity by id.

Collectives™ on Stack Overflow

ASP.NET MVC / APIs - DELETE method not returning correct response

2 Answers 2

5 Comments

2 Comments

Your Answer

Hot Network Questions

Collectives™ on Stack Overflow

2 Answers 2

5 Comments

2 Comments

Your Answer

Sign up or log in

Post as a guest

Related