Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

enable view function annotation based type detection #100

Merged
merged 2 commits into from
Jan 15, 2021

Conversation

yoursvivek
Copy link
Contributor

The current implementation of spectree uses arguments to validate method for declaration of types for query params, request body, cookies and headers and in turn adds a context attribute to request object for all three framework implementations like following:

# flask view implementation
@app.route("/api/user/<name>", methods=["POST"])
@api.validate(
    query=Query,
    json=JSON,
    cookies=Cookies,
    resp=Response(HTTP_200=Resp, HTTP_401=None),
    tags=["api", "test"],
    after=api_after_handler,
)
def user_score(name):
    score = [randint(0, request.context.json.limit) for _ in range(5)]
    score.sort(reverse=request.context.query.order)
    assert request.context.cookies.pub == "abcdefg"
    assert request.cookies["pub"] == "abcdefg"
    return jsonify(name=request.context.json.name, score=score)

While this is good, but static linters like mypy complain about context attribute missing from flask request object. If view functions itself are annotated with their type and we additionally inject kwargs in view, linters are happy and long request.context.query.order access is now just query.order. Following is an example of annotated view function:

# flask view implementation with annotation
@app.route("/api/user_annotated/<name>", methods=["POST"])
@api.validate(
    resp=Response(HTTP_200=Resp, HTTP_401=None),
    tags=["api", "test"],
    after=api_after_handler,
)
def user_score_annotated(
    name,
    query: Query,
    json: JSON,
    cookies: Cookies
):
    score = [randint(0, json.limit) for _ in range(5)]
    score.sort(reverse=query.order)
    assert cookies.pub == "abcdefg"
    assert request.cookies["pub"] == "abcdefg"
    return jsonify(name=json.name, score=score)

Current implementation is an opt-in feature disabled by default. To opt-in simple instantiate SpecTree instance with an additional parameter like following:

api = SpecTree("flask", annotations=True)

This feature doesn't change anything if not opted in. Once opted it, user can have a nice API.

I'm willing to discuss the merits, demerits of this approach. It's much closer to FaskAPI usage and since we are already using pydantic and annotations inside spectree, why stop there.

All tests are in place.

@yoursvivek yoursvivek force-pushed the annotation branch 2 times, most recently from aef28f6 to 7ad03b2 Compare January 7, 2021 16:49
@kemingy kemingy added the enhancement New feature or request label Jan 8, 2021
@yedpodtrzitko
Copy link
Collaborator

static linters like mypy complain about context attribute missing from flask request object

if this is done just to make mypy pass, how about acknowledge the request object isn't vanilla flask.Request and add custom Request class instead, something like this:

# anywhere, eg. spectree/plugins/flask_plugin.py
+ from flask import Request, request as request_

+ class SpecTreeRequest(Request):
+     context: Context

+ request: SpecTreeRequest = request_
# myapp.py
- from flask import abort
+ from spectree.plugins.flask_plugin import request

# this will now pass without mypy error
payload = request.context.json

@kemingy
Copy link
Member

kemingy commented Jan 11, 2021

if this is done just to make mypy pass, how about acknowledge the request object isn't vanilla flask.Request and add custom Request class instead, something like this:

This works well but it's limited to flask.

Personally, I think linter is very useful (although Python linter still has a long way to go). Meanwhile, FastAPI is quite popular. I think a lot of people will accept this annotation approach.

One possible problem is that users should avoid using these keywords as path variables.

One possible benefit is that it may make it easy to implement #96.

@yoursvivek
Copy link
Contributor Author

if this is done just to make mypy pass, how about acknowledge the request object isn't vanilla flask.Request and add custom Request class instead, something like this:

This is not just about making mypy happy. I find annotation based approach much cleaner for developers. Once a developer has access to an object instance and it's type by virtue of function signature, he can focus on his code right away instead of trying to decipher which all decorators have injected some additional attributes somewhere in an otherwise common object.

One possible problem is that users should avoid using these keywords as path variables.

During development, concern about having same keyword in path variables also came to my mind. I don't have a concrete answer to this. For now, if I were in that position, I'll pass them in params to validate in those select function where I can't rename path params for any particular reason (part of public api documentation etc.).

Another approach would be to provide option to alias params name for query/json/headers/cookies through validate method.

app.route("/search/{query}/")
api.validate(..., alias={"query": "q", "json": "body"})
def viewfunc(query: str, q: QueryModel, body: BodyModel):
   ...

@yedpodtrzitko
Copy link
Collaborator

I find annotation based approach much cleaner for developers.

I'm not opposing that, I like the benefits type annotations provide too, but from your comment it seemed like the motivation for this is just to make mypy pass.

developer has access to an object instance and it's type by virtue of function signature

That's sure useful, but I'm not sure if the tradeoff of inflating each function with 4 parameters is worth it, considering some of them are barely used (how many views really use cookies or headers?) So how about passing there just the Context object (which encapsulate all the data structures as it is now), similarly as Django is passing request to its views? This could solve all the problems:

  • no property injected into the request object (so mypy is happy)

  • you can have almost direct access (with only 1 level of nesting) to all the data structures through a function's parameter

  • function signature is not inflated by 4 parameters, so there's lower probability of name clashing

  • it's more future-proof than having 4 parameters. If anything changes in the Context, you still have the same 1 parameter, and it can facade the changes internally (maybe it can be even lazily evaluated?). If there would be any change within the 4 parameters, like name change, or adding yet another parameter, you'd need to change all function signatures retroactively.

...in fact I can imagine passing context into the views could be the default (only) way how to do that, even at the price of backward incompatibility. Having this as a config option is sure non-intrusive, but it increases the complexity of the code and something something Zen of Python one way how to do things.

@kemingy
Copy link
Member

kemingy commented Jan 12, 2021

During development, concern about having same keyword in path variables also came to my mind. I don't have a concrete answer to this. For now, if I were in that position, I'll pass them in params to validate in those select function where I can't rename path params for any particular reason (part of public api documentation etc.).

Thanks for your answer. You are right, users can still use the validate function if there are some conflicts. And I think that's enough.

@yoursvivek
Copy link
Contributor Author

yoursvivek commented Jan 12, 2021

..., but I'm not sure if the tradeoff of inflating each function with 4 parameters is worth it, considering some of them are barely used (how many views really use cookies or headers?)

Actually the PR requires you to only put annotations for what you want, not all four.

So if you only want to parse body your view would be:

@route('/keys')
@api.validate(resp=Response("HTTP_200"))
def viewfunc(json: BodyModel):
    return json.keys()

So the rest of objections are moot. I'm of the believe that libraries should absorb the complexity and provide clean contract. Also this is a public library, I can't propose something that breaks entire existing api.

Also if you look at FastAPI or rocket (from rust world) you'll see that they've taken things farther and values inside body/query etc are unpacked into view like:

def search(
    q: str = Query(..., description="query string"),
    page: int = Query(0, description="pagination page number"),
    limit: int = Query(10, description="results per page"),
):
    ...

@kemingy
Copy link
Member

kemingy commented Jan 13, 2021

LGTM. Can you add some documents to the README?

@yoursvivek
Copy link
Contributor Author

yoursvivek commented Jan 14, 2021

Examples and description added in README.

@kemingy
Copy link
Member

kemingy commented Jan 15, 2021

Thanks.

@kemingy kemingy merged commit fccd8c5 into 0b01001001:master Jan 15, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants