Improve "basics" docs #152

twosigmajab · 2020-01-18T17:47:10Z

Document that 3-tuples can also be returned to include custom headers.
Improve "Todo" example based on how such an app would actually be written:
- Combine TodoSchema and CreateTodoSchema, so that TodoSchema actually includes the description (not just the id), and make id a dump_only field, so that there's no need for a separate schema for the create_todo endpoint's request_body_schema.
- Update create_todo to actually use request_body_schema.
- Demonstrate (1.) by including a Location header in create_todo's 201 Created response, in compliance with the HTTP spec ("...the [new resource's] location being... the content of the Location header").

I actually implemented a real Todo list example app with Flask-Rebar using Flask-SQLAlchemy, which these changes are based on. Figured I'd finally try to contribute back some best practices I'm following that Flask-Rebar's docs aren't currently demonstrating.

airstandley

Nice! Thanks for taking the time to update the docs ❤️

airstandley · 2020-01-30T18:50:50Z

docs/quickstart/basics.rst

@@ -159,20 +156,25 @@ The ``response_body_schema`` (previously ``marshal_schema``) argument of ``Handl

 In the case of a ``marshmallow.Schema``, that schema is used to ``dump`` the return value of the handler function.

-In the case of a dictionary mapping integers to ``marshmallow.Schemas``, the integers are interpreted as status codes, and the handler function must return a tuple of ``(response_body, status_code)``:
+In the case of a dictionary mapping integers to ``marshmallow.Schemas``, the integers are interpreted as status codes, and the handler function must return a tuple like ``(response_body, status_code)``,
+or like ``(response_body, status_code, headers)`` to also include custom headers:


Nitpick: I could be wrong, but I believe that we still assume a non-tuple response has a 200 status_code. If that is the case we might want to also document that as a valid response here?

Pushed another revision based on this comment. Ended up making what I think are a few other nice improvements to these docs, which I'll call out separately. Thanks for the suggestion!

Sytten · 2020-01-31T01:22:39Z

docs/quickstart/basics.rst

@@ -19,7 +19,8 @@ Let's take a look at a very basic API using Flask-Rebar:


 class TodoSchema(ResponseSchema):
- id = fields.Integer()
+ id = fields.Integer(dump_only=True)
+ description = fields.String(required=True)


I personally like having different schema for create and get, but this pattern is also nice so I am not sure what we should recommend to beginners.

Check out the latest revision (which actually includes a little more explanation in the docs) and let me know what you think.

When the data for the same type of object is for some reason very different in request bodies compared to response bodies, it makes sense to use different schemas. But in the simpler case (which is maybe more common?) where the only difference is that some fields should only be read-only by users, since they're managed entirely by the database or application, then I think using a single Schema with those fields marked as dump_only is a perfect fit. Common examples in my experience are autogenerated ids and automatically maintained updated_at and created_at timestamps, which can all be maintained by a SQL database automatically.

FWIW, this is the pattern I use in the class I've been teaching to beginners and it's been working really well. (For the past year I've been teaching a monthly class at Two Sigma on how to build REST APIs with Flask-Rebar, and by the time they leave the class, students have built a full working CRUD app to manage (you guessed it) their Todo list. 🤣)

Definitely much better with the new comments, I am not sure about the form (explanation before/after code vs inline comments) but that can be improved later.

twosigmajab · 2020-01-31T05:03:36Z

My pleasure, @airstandley! Note, all Travis jobs passed except one, but that looks like an ephemeral networking failure during the pip install (specifically pip._vendor.urllib3.exceptions.ProtocolError: ("Connection broken: ConnectionResetError(104, 'Connection reset by peer')"). It might be worth prefixing calls that require the network with a good old travis_retry (depending on what happens with #160:).

Sytten · 2020-01-31T05:14:05Z

docs/quickstart/basics.rst



-``RequestSchema`` is an extension of ``marshmallow.Schema`` that throws an internal server error if an object is missing a required field. It's usage is optional - a normal Marshmallow schema will also work.
-
 This request schema is passed to ``request_body_schema``, and the handler will now call `marshmallow.Schema.load <http://marshmallow.readthedocs.io/en/latest/api_reference.html#marshmallow.Schema.load>`_ on the request body decoded as JSON. A 400 error with a descriptive error will be returned if validation fails.


RequestSchema is still important for marshmallow 2 if I am not mistaken, otherwise additional fields are not rejected. What I think we should do in V2 is a single Schema class that includes the RequestSchema and ResponseSchema, but I will open an issue for that.

See #152 (comment)

twosigmajab · 2020-01-31T05:15:49Z

docs/quickstart/basics.rst

 from flask import Flask
 from flask_rebar import Rebar
- from flask_rebar import ResponseSchema


FWIW, I've never used ResponseSchema because I've always been the one in full control of my data, and have automated tests that make sure the data I keep in my database stays consistent (validation-wise) with the data my API promises to return. In the interest of keeping this example as basic as possible, while still being as useful as possible, I thought it's better to save Rebar's special RequestSchema/ResponseSchema for a later section, in return for demonstrating the other stuff I added here.

I tend to agree on that one but I think we should move toward #161

twosigmajab · 2020-01-31T05:16:55Z

docs/quickstart/basics.rst

 )
 def get_todo(id):
- ...
- return {'id': id}
+ todo = _get_todo_or_404(id) # Some helper function that queries our database.


Shout out to https://flask-sqlalchemy.palletsprojects.com/en/2.x/api/#flask_sqlalchemy.BaseQuery.first_or_404

twosigmajab · 2020-01-31T05:18:47Z

docs/quickstart/basics.rst


 app = Flask(__name__)
 rebar.init_app(app)

- if __name__ == '__main__':
- app.run()


(app.run() has been deprecated/discouraged for several years, and it's just as easy to show best practice here instead, so I fixed that too while I was in here.)

twosigmajab · 2020-01-31T05:39:23Z

docs/quickstart/basics.rst

@@ -60,38 +78,49 @@ Request Body Validation

 .. code-block:: python

- from flask_rebar import RequestSchema


(FWIW, I haven't yet hit a case where I've wanted Rebar's RequestSchema; in all my use cases so far, I've preferred "being liberal in what I accept and conservative in what I send", at least as far as unrecognized fields are concerned.)

I put it everywhere since I prefer a break fast strategy with a REST API. For example, if for some reason you have a breaking change in the API, you don't want to have clients sending requests thinking that all went well and it applied some changes but in reality it didn't (and no alert since its a 200).

(To be clear, I'm by no means saying it's always wrong to fail hard for unrecognized fields, just that it's definitely not always right. Maybe more citation needed, so just did a quick search and immediately turned up something interesting: https://opensource.zalando.com/restful-api-guidelines/#109 – this topic is deep!)

I really like this website, thanks for sharing. I think we should try to encourage some of it's principles while allowing divergences because they are quite strict and hard to meet for a lot of people.

So looking with fresh eyes today... That API Guidelines link I posted last night is saying that the Robustness Principle applies on the client side, but on the server side, it doesn't!:

Unknown input fields in payload or URL should not be ignored; servers should provide error feedback to clients via an HTTP 400 response code.

This is not what I thought was best practice, so I'll definitely be doing more research on this! If this API Guidelines doc is correct, then it's quite a validation of flask_rebar's RequestSchema, and I'll correct my understanding, my code, and my teaching accordingly!

In the meantime, the current revision of this PR currently is still removing the use of RequestSchema for the example request_body_schemas. That means going from conforming to these guidelines to not conforming to them. I really don't want to be responsible for, in the process of trying to improve the docs' demonstration of best practices, actually making them worse in this particular way.

At the same time, I'm very keen to see #161 happen, because I do think that having two different schemas (request and response) for the same type of object is not always necessary, and when it isn't, as in this Todo example, it adds unnecessary complexity to our docs. Shall we wait for #161 to be resolved so we can incorporate the outcome into these changes before we merge them?

Thank you @Sytten for prompting this!

We can wait, I will try to prioritize this weekend of monday. Though I am not sure if we want to only do that in V2 or backport it to V1.

twosigmajab · 2020-01-31T05:39:47Z

docs/quickstart/basics.rst

-
-
- class CreateTodoSchema(RequestSchema):
- description = fields.String(required=True)


(Now reusing TodoSchema for both requests and responses)

airstandley · 2020-02-03T20:14:53Z

@twosigmajab This is fantastic! It's a huge improvement.

However I'm against removing all references to RequestSchema/ResponseSchema; I'm completely pro adding documentation that discusses different philosophies on API design and when Schema may be more appropriate than RequestSchema/ResponseSchema, but for our basic 'QuickStart' guide I am strongly in favor of suggesting RequestSchema/ResponseSchema.

airstandley · 2020-02-03T20:20:24Z

docs/quickstart/basics.rst

+
+ export FLASK_APP=todos.py
+ flask run
+


I'm wondering if we should split this into two sections, "Creating a Flask App with Rebar" and "Registering a Handler"? As is this portion on running the app feels out of place and I think makes the lines that follow a bit confusing.

gtmanfred · 2022-10-25T14:04:47Z

Is this still needed? can we make sure it is up to date for flask-rebar v2.0?

Otherwise, i would like to close it to start clearing out the backlog.

jab · 2022-10-25T17:41:34Z

I think there are still some good improvements here that would be beneficial to merge, but I no longer work at Two Sigma, nor with Flask-Rebar, and at this point I would have to hand this off to the maintainers (or another external contributor). Hopefully someone is still interested in making these improvements.

twosigmajab requested a review from a team as a code owner January 18, 2020 17:47

ghost assigned PratikPramanik Jan 18, 2020

airstandley approved these changes Jan 30, 2020

View reviewed changes

Sytten reviewed Jan 31, 2020

View reviewed changes

twosigmajab force-pushed the docs branch from 020b970 to 17c0a3d Compare January 31, 2020 04:44

Sytten reviewed Jan 31, 2020

View reviewed changes

Sytten mentioned this pull request Jan 31, 2020

Refactor RequestSchema and ResponseSchema into a single Schema #161

Open

twosigmajab force-pushed the docs branch from 17c0a3d to 290990f Compare January 31, 2020 05:38

twosigmajab commented Jan 31, 2020

View reviewed changes

Improve "basics" docs

71565fc

twosigmajab force-pushed the docs branch from 290990f to 71565fc Compare January 31, 2020 15:58

Merge branch 'master' into docs

e40c766

airstandley self-requested a review February 3, 2020 20:15

ghost assigned airstandley Feb 3, 2020

airstandley reviewed Feb 3, 2020

View reviewed changes

Sytten mentioned this pull request Feb 8, 2020

Feature/strict schema #169

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improve "basics" docs #152

Improve "basics" docs #152

twosigmajab commented Jan 18, 2020 •

edited

airstandley left a comment •

edited

airstandley Jan 30, 2020

twosigmajab Jan 31, 2020

Sytten Jan 31, 2020

twosigmajab Jan 31, 2020

Sytten Jan 31, 2020

twosigmajab commented Jan 31, 2020 •

edited

Sytten Jan 31, 2020

twosigmajab Jan 31, 2020

twosigmajab Jan 31, 2020

Sytten Jan 31, 2020

twosigmajab Jan 31, 2020

twosigmajab Jan 31, 2020

twosigmajab Jan 31, 2020

Sytten Jan 31, 2020

twosigmajab Jan 31, 2020 •

edited

Sytten Jan 31, 2020

twosigmajab Jan 31, 2020

Sytten Feb 1, 2020

twosigmajab Jan 31, 2020

airstandley commented Feb 3, 2020 •

edited

airstandley Feb 3, 2020

gtmanfred commented Oct 25, 2022

jab commented Oct 25, 2022



		``RequestSchema`` is an extension of ``marshmallow.Schema`` that throws an internal server error if an object is missing a required field. It's usage is optional - a normal Marshmallow schema will also work.

		This request schema is passed to ``request_body_schema``, and the handler will now call `marshmallow.Schema.load <http://marshmallow.readthedocs.io/en/latest/api_reference.html#marshmallow.Schema.load>`_ on the request body decoded as JSON. A 400 error with a descriptive error will be returned if validation fails.

		@@ -60,38 +78,49 @@ Request Body Validation

		.. code-block:: python

		from flask_rebar import RequestSchema



		class CreateTodoSchema(RequestSchema):
		description = fields.String(required=True)

Improve "basics" docs #152

Are you sure you want to change the base?

Improve "basics" docs #152

Conversation

twosigmajab commented Jan 18, 2020 • edited

airstandley left a comment • edited

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

twosigmajab commented Jan 31, 2020 • edited

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

twosigmajab Jan 31, 2020 • edited

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

airstandley commented Feb 3, 2020 • edited

Choose a reason for hiding this comment

gtmanfred commented Oct 25, 2022

jab commented Oct 25, 2022

twosigmajab commented Jan 18, 2020 •

edited

airstandley left a comment •

edited

twosigmajab commented Jan 31, 2020 •

edited

twosigmajab Jan 31, 2020 •

edited

airstandley commented Feb 3, 2020 •

edited