feat(robot-server): add formatted json responses and error handling #5208

iansolano · 2020-03-11T16:41:55Z

Adds formatted json responses and error handling in FastAPI framework

overview

changelog

review requests

In the robot-server project:

run OT_API_FF_useFastApi=TRUE make dev

You should be able to visit /docs route on localhost. This will allow you to review the swagger interactive docs. The two endpoints these changes affect are the placeholder endpoints /items and /items/{item_id}

Make sure 200 responses are returned as expected
Test error responses when JSON is invalid structure
Test error responses when there is a model validation error

Please add comments around any additions you would like to see, this is a first pass at solidifying some structure based on the JSON:API spec but we can change and add as we see fit.

Still todo:

Need to fixup types!
Add an endpoint that returns multiple resources
Determine how the meta object will be formatted
Add a convenience method for formatting link responses

iansolano · 2020-03-11T16:44:52Z

robot-server/robot_server/service/models/json_api/errors.py

+    if isinstance(exception, StarletteHTTPException):
+        request_error = {
+            'status': status_code,
+            'detail': exception.detail,
+            'title': 'Bad Request'
+        }
+        error_response = ErrorResponse(errors=[request_error])
+        return filter_none(error_response.dict())


This is annoying b/c the error comes back differently if it's a HTTPException, I can probably clean this up a bit (feels a bit redundant)

My preference would be a different function for each Exception type.

iansolano · 2020-03-11T16:45:51Z

robot-server/robot_server/service/routers/item.py

+        # NOTE(isk: 3/10/20): mock DB / robot response
+        item = Item(name="apple", quantity=10, price=1.20)
+        data = ItemResponse.resource_object(id=item_id, attributes=item)
+        return ItemResponse(data=data, links={"self": f'/items/{item_id}'})


I should add a helper method in ItemResponse to format links similar to resource_object class method.

sfoster1

Couple small typos but I like the model factories. Is the idea to use this on everything?

sfoster1 · 2020-03-11T16:55:03Z

robot-server/robot_server/service/models/json_api/errors.py

+    title: Optional[str]
+    detail: Optional[str]
+    source: Optional[ErrorSource]
+    meta: Optional[dict]


here and elsewhere, we should use typing.Dict in type definitions - it's the typing generic version of dictionaries

sfoster1 · 2020-03-11T17:09:59Z

robot-server/robot_server/service/models/json_api/response.py

+    def dict(
+        self,
+        *,
+        serlialize_none: bool = False,


robot-server/robot_server/service/models/item.py

amitlissack · 2020-03-11T19:29:26Z

robot-server/robot_server/service/models/json_api/errors.py

+
+def transform_to_json_api_errors(status_code, exception) -> dict:
+    if isinstance(exception, StarletteHTTPException):
+        request_error = {


You can also construct this using an Error object rather than a dict.

amitlissack · 2020-03-11T19:30:17Z

robot-server/robot_server/service/models/json_api/errors.py

+    if isinstance(exception, StarletteHTTPException):
+        request_error = {
+            'status': status_code,
+            'detail': exception.detail,
+            'title': 'Bad Request'
+        }
+        error_response = ErrorResponse(errors=[request_error])
+        return filter_none(error_response.dict())


My preference would be a different function for each Exception type.

amitlissack · 2020-03-11T20:03:26Z

robot-server/robot_server/service/models/json_api/factory.py

+from .request import JsonApiRequest, RequestModel
+from .response import JsonApiResponse, ResponseModel
+
+def JsonApiModel(


functions should use snake case.

amitlissack · 2020-03-12T14:57:54Z

robot-server/robot_server/service/models/json_api/errors.py

+            'title': 'Bad Request'
+        }
+        error_response = ErrorResponse(errors=[request_error])
+        return filter_none(error_response.dict())


As you saw, you can direct fastapi to omit default or unset values in handlers. Looks like you can also tell pydantic to do that when converting to dict. So you might not need the filter functions.

https://pydantic-docs.helpmanual.io/usage/exporting_models/

nice! this was what i found yesterday (https://fastapi.tiangolo.com/tutorial/response-model/#use-the-response_model_exclude_unset-parameter) so it looks like we can get rid of this filter_none stuff entirely and TBH it's a bit more explicit in the handlers

iansolano · 2020-03-12T18:44:42Z

@sfoster1 yeah, the idea would be to use this to format our api responses going forward (new endpoints).

codecov · 2020-03-12T20:31:58Z

Codecov Report

Merging #5208 into edge will increase coverage by 0.16%.
The diff coverage is 95.80%.

@@            Coverage Diff             @@
##             edge    #5208      +/-   ##
==========================================
+ Coverage   61.17%   61.34%   +0.16%     
==========================================
  Files        1032     1039       +7     
  Lines       29416    29559     +143     
==========================================
+ Hits        17995    18132     +137     
- Misses      11421    11427       +6

Impacted Files	Coverage Δ
robot-server/robot_server/service/routers/item.py	`78.57% <78.57%> (ø)`
robot-server/robot_server/service/main.py	`100.00% <100.00%> (ø)`
robot-server/robot_server/service/models/item.py	`100.00% <100.00%> (ø)`
...ver/robot_server/service/models/json_api/errors.py	`100.00% <100.00%> (ø)`
...er/robot_server/service/models/json_api/factory.py	`100.00% <100.00%> (ø)`
...er/robot_server/service/models/json_api/request.py	`100.00% <100.00%> (ø)`
...t_server/service/models/json_api/resource_links.py	`100.00% <100.00%> (ø)`
...r/robot_server/service/models/json_api/response.py	`100.00% <100.00%> (ø)`
... and 5 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 91178f0...0b1fb67. Read the comment docs.

amitlissack · 2020-03-13T18:48:29Z

robot-server/robot_server/service/models/json_api/errors.py

+
+
+class ErrorSource(BaseModel):
+    pointer: Optional[str]


Not that it's essential for this PR, but adding documentation for these attributes will be great for the docs UI:

my_field: str = Field(..., description="This is my field")

ah, yeah - thanks i'll add now

amitlissack · 2020-03-13T18:50:45Z

robot-server/robot_server/service/models/json_api/errors.py

+    errors: List[Error]
+
+
+def transform_http_exception_to_json_api_errors(exception) -> Dict:


Nit: I would return an ErrorResponse object from these two functions. Only converting it to Dict when it needs to go over the wire. It's a minor point, but it's about habit. The habit of using clearly defined types rather than dicts and tuples and so on.

makes a lot of sense

amitlissack · 2020-03-13T18:51:31Z

robot-server/robot_server/service/models/json_api/request.py

+from typing import Generic, TypeVar, Optional, Any, Type
+from typing_extensions import Literal
+from pydantic.generics import GenericModel
+


I know I'm old fashioned, but I like comments. This is somewhat complicated and comments explain what these types are would be great.

robot-server/robot_server/service/models/json_api/response.py

amitlissack · 2020-03-13T18:58:30Z

robot-server/tests/service/routers/test_item.py

+from robot_server.service.main import app
+from tests.service.helpers import ItemData
+
+client = TestClient(app)


There's nothing necessarily wrong with this, but our python code base tends to use pytest and their fixtures.

In fact, if you rebase of of edge, you can just use the fixture that's already set up called api_client. That way your tests could look like:

def test_get_item(api_client): response = api_client.get(....)

amitlissack · 2020-03-13T19:00:11Z

robot-server/tests/service/models/json_api/test_response.py

+from robot_server.service.models.json_api.response import JsonApiResponse
+from tests.service.helpers import ItemModel, ItemData
+
+class TestJsonApiResponse:


I don't see a need for there to be a class here. Can't the tests just all be global functions? That would more jive with the rest of our code base.

they sure can. yup, makes sense to be consistent was just trying to keep nicely organized, I didn't realize that we don't do this elsewhere. my bad

amitlissack · 2020-03-13T19:01:44Z

This is a great piece of work!

A lot of style comments. Pythonisms etc. And just asking for more comments.

amitlissack

Woot!

mcous · 2020-03-16T19:18:11Z

robot-server/robot_server/service/main.py

@@ -28,6 +36,8 @@
                   tags=["settings"])
 app.include_router(router=deck_calibration.router,
                   tags=["deckCalibration"])
+app.include_router(router=item.router,


Could use a TODO comment noting that this should be removed

mcous · 2020-03-16T19:50:15Z

robot-server/robot_server/service/models/json_api/errors.py

+                          " meta-information about the error.")
+
+
+class ErrorResponse(BaseModel):


This doesn't match what's in our documentation at https://coda.io/d/HTTP-API-v2_dS1dJrSYETc/Error-Responses_suowf#_luSMh. Can you make sure the docs are up to date?

yeah, for sure. I will update this accordingly. thanks for pointing out

mcous · 2020-03-16T19:51:59Z

robot-server/robot_server/service/main.py

+    exception: RequestValidationError
+) -> JSONResponse:
+    errors = transform_validation_error_to_json_api_errors(
+        HTTP_422_UNPROCESSABLE_ENTITY, exception


I'm curious about the rational behind using what appears to be a WebDAV extension code (422) instead of an old-fashioned 400

fastapi and json:api spec both return validation errors as 422. The main difference between 400 is that the syntax is correct in the request. See below (https://tools.ietf.org/html/rfc4918#section-11.2):

The 422 (Unprocessable Entity) status code means the server understands the content type of the request entity (hence a 415(Unsupported Media Type) status code is inappropriate), and the syntax of the request entity is correct (thus a 400 (Bad Request) status code is inappropriate) but was unable to process the contained instructions.

mcous · 2020-03-16T20:32:19Z

robot-server/robot_server/service/routers/item.py

+
+
+@router.get("/items/{item_id}",
+            description="Get an individual item by it's ID",


sfoster1

One minor nitpick but if it passes tests I'm ok with it. A lot of fun type manipulation, I like it. You might want to get some solid documentation going somewhere to onboard people who haven't really used runtime type handling.

sfoster1 · 2020-03-17T14:13:34Z

robot-server/robot_server/service/models/json_api/response.py

+
+# Note(isk: 3/13/20): returns response based on whether
+# the data object is a list or not
+def JsonApiResponse(


let's keep to snake case or (if you must) camelCase for functions please

ah, sorry - i meant to change that. thanks for catching

Add formatted json responses and error handling in FastAPI framework closes #4636

iansolano added ready for review robot-svcs Falls under the purview of the Robot Services squad (formerly CPX, Core Platform Experience). robot server Affects the `robot-server` project labels Mar 11, 2020

iansolano requested a review from a team as a code owner March 11, 2020 16:41

iansolano commented Mar 11, 2020

View reviewed changes

sfoster1 requested changes Mar 11, 2020

View reviewed changes

amitlissack reviewed Mar 11, 2020

View reviewed changes

amitlissack reviewed Mar 12, 2020

View reviewed changes

iansolano requested a review from a team as a code owner March 12, 2020 19:11

mcous changed the title ~~feat(robot-server): adds formatted json responses and error handling~~ feat(robot-server): add formatted json responses and error handling Mar 12, 2020

amitlissack reviewed Mar 13, 2020

View reviewed changes

robot-server/robot_server/service/models/json_api/response.py Show resolved Hide resolved

amitlissack reviewed Mar 13, 2020

View reviewed changes

iansolano force-pushed the robot-server_json-response-patterns branch 3 times, most recently from a8794cf to bf814b3 Compare March 16, 2020 14:51

amitlissack approved these changes Mar 16, 2020

View reviewed changes

iansolano requested a review from sfoster1 March 16, 2020 15:36

mcous reviewed Mar 16, 2020

View reviewed changes

sfoster1 approved these changes Mar 17, 2020

View reviewed changes

iansolano added 2 commits March 18, 2020 12:05

feat(robot-server): add formatted json responses and error handling

473bd28

Add formatted json responses and error handling in FastAPI framework closes #4636

fixup: address PR comments

0b1fb67

iansolano force-pushed the robot-server_json-response-patterns branch from 821b170 to 0b1fb67 Compare March 18, 2020 16:06

iansolano merged commit 25e9298 into edge Mar 18, 2020

sfoster1 mentioned this pull request Mar 24, 2020

bug: robot server fails to start on robot with httpapiv2 because it relies on typing_extensions #5275

Closed

mcous deleted the robot-server_json-response-patterns branch May 6, 2020 23:36

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(robot-server): add formatted json responses and error handling #5208

feat(robot-server): add formatted json responses and error handling #5208

iansolano commented Mar 11, 2020 •

edited

Loading

iansolano Mar 11, 2020

amitlissack Mar 11, 2020

iansolano Mar 11, 2020 •

edited

Loading

sfoster1 left a comment

sfoster1 Mar 11, 2020

sfoster1 Mar 11, 2020

amitlissack Mar 11, 2020

amitlissack Mar 11, 2020

amitlissack Mar 11, 2020

amitlissack Mar 12, 2020

iansolano Mar 12, 2020

iansolano commented Mar 12, 2020

codecov bot commented Mar 12, 2020 •

edited

Loading

amitlissack Mar 13, 2020

iansolano Mar 13, 2020

amitlissack Mar 13, 2020

iansolano Mar 13, 2020

amitlissack Mar 13, 2020

amitlissack Mar 13, 2020

amitlissack Mar 13, 2020

iansolano Mar 13, 2020

amitlissack commented Mar 13, 2020

amitlissack left a comment

mcous Mar 16, 2020

mcous Mar 16, 2020

iansolano Mar 17, 2020

mcous Mar 16, 2020

iansolano Mar 17, 2020

mcous Mar 16, 2020

sfoster1 left a comment

sfoster1 Mar 17, 2020

iansolano Mar 17, 2020

		errors: List[Error]


		def transform_http_exception_to_json_api_errors(exception) -> Dict:

		" meta-information about the error.")


		class ErrorResponse(BaseModel):



		@router.get("/items/{item_id}",
		description="Get an individual item by it's ID",

feat(robot-server): add formatted json responses and error handling #5208

feat(robot-server): add formatted json responses and error handling #5208

Conversation

iansolano commented Mar 11, 2020 • edited Loading

overview

changelog

review requests

Choose a reason for hiding this comment

Choose a reason for hiding this comment

iansolano Mar 11, 2020 • edited Loading

Choose a reason for hiding this comment

sfoster1 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

iansolano commented Mar 12, 2020

codecov bot commented Mar 12, 2020 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

amitlissack commented Mar 13, 2020

amitlissack left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sfoster1 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

iansolano commented Mar 11, 2020 •

edited

Loading

iansolano Mar 11, 2020 •

edited

Loading

codecov bot commented Mar 12, 2020 •

edited

Loading