Incorrect response codes for invalid HTTP calls

[virkt25]

Description / Steps to reproduce / Feature proposal

From #1206

• In examples/todo make a GET request to /todo/id using API Explorer. id shouldn't have a value (undefined).
• You get back a 500 - Server Error BUT it should've been a 400 (Bad Request) since the request is missing the id. Same goes for an invalid id value such as -10, ten, etc.

Current Behavior

• Invalid Response Code

Expected Behavior

• Correct HTTP Response Code as per http://www.restapitutorial.com/httpstatuscodes.html

Acceptance Criteria

• Fix response codes as described above, see Incorrect response codes for invalid HTTP calls #1469 (comment)
• Appropriate test cases

See Reporting Issues for more tips on writing good issues

[marioestradarosa]

Also, it should be 404 , resource not found, when /model/{id} and id is a valid one but was not found in the DB. In any case I think if you try to map database operation errors with http status code will be a complicated task. For instance 404 - not found , 201 when you created a record and so on.

As a client developer I might be must interested in the response message for regular DB operations. So if I sent an operation i would do:

http.get<clientModel[]>(MyResourcreURL).suscribe( (result : clientModel) =>{} , error=>{} ); 

So I think that no matter the http satatus code, I would get to the error=> {} code right?, to show my users the appropriate message. Currently juggler is placing acceptable messages automatically for certain crud operations, the problem is that they are not being sent back to the client app thru the response object.

So, talking in the database operation arena, the errors can vary a lot, and it would be a daunting task to try match everything to http status codes.

• Couldn't acquire the lock to update the record
• We tried to update the record, but it no longer exists
• Can not remove the record, it has references everywhere (key constraints)
• That column in your SQL table, no longer exists etc.

However, currently in our lb4 app, we are sending two properties that don not conflict with status or statusCode, to send in our custom operations an appropriate error code and message, but for CRUD operations I think it is ok, once the missing message is fixed :-)

[virkt25]

Thanks for the great feedback @marioestradarosa !! You are correct in that it should be a 404 and yes we definitely need to do some work around passing errors back from the DataBase as we head towards GA -- but I also think we should try and map some (not all) of the database errors to HTTP Status Codes because some users use that to display an error message in the front end since showing DB errors can be a bit confusing.

[marioestradarosa]

@virkt25 this is more or less :-)

DB Basic Operations

Create

• Successfully returns 201 + body { Record } + location header field + /{id} to locate it
• Already Exits returns 409 + body { statusCode: 409 , message: ‘Resource with ID xx already exists, couldn't create’ }
• General returns 500 + body { statusCode:500, message: ‘juggler original message’ }

Read

• Successfully returns 200 + body { Record }
• Not found returns 404 + body { statusCode: 404 , message: ‘Resource with ID xx not found’ }
• General returns 500 + body { statusCode:500, message: ‘juggler original message’ }

Update

• Successfully returns 200 + body { Record }
• Not found returns 404 + body { statusCode: 404 , message: ‘Resource with ID xx not found, couldn't update' }
• General returns 500 + body { statusCode:500, message: ‘juggler original message’ }

Delete

• Successfully returns 200 + body { Record }
• Not found returns 404 + body { statusCode: 404 , message: ‘Resource with ID xx not found, couldn’t delete' }
• General returns 500 + body { statusCode:500, message: ‘juggler original message’ }

Notes 
1- { Record } is the full record, and behaves similar to the current insert operation in that it returns the defined record in the model.
2- statusCode is evaluated internally today and that’s the number placed in the http status header
3- 201 In the insert should also include A Location Header field with /newlycreatedID appended

[hacksparrow]

Looks like it is fixed for undefined and string id ("ten", "haha" etc.) cases in the latest master. Still 500s for invalid numerical IDs.

[virkt25]

Wonder if the fix happened because of the coercion and validation story.

[bajtos]

Wonder if the fix happened because of the coercion and validation story.

Almost certainly yes :)