aws-powertools / powertools-lambda-python

A developer toolkit to implement Serverless best practices and increase developer velocity.
https://docs.powertools.aws.dev/lambda/python/latest/
MIT No Attribution
2.84k stars 390 forks source link

Docs: Syntax for routing rules doesn't seem to be documented anywhere #2962

Open johnmreynolds opened 1 year ago

johnmreynolds commented 1 year ago

What were you searching in the docs?

I've been trying to find any documentation on how the routing rules work, but even in the API reference it's just documented as being a 'str'.

https://docs.powertools.aws.dev/lambda/python/latest/api/event_handler/index.html#aws_lambda_powertools.event_handler.ApiGatewayResolver.route

From the tutorials and source code, it looks like this should be a regex, but there is no indication of what regex syntax this follows, or how variable capture works with decorated functions.

For example, the tutorials include things like @app.get("/subscriptions/") which seems to be a curious syntax, and I can't find any documentation on this. Also, it doesn't seem to work for subfolders which is weird.

Is this related to an existing documentation section?

https://docs.powertools.aws.dev/lambda/python/latest/api/event_handler/index.html#aws_lambda_powertools.event_handler.ApiGatewayResolver.route

How can we improve?

Document how the routing rules work somewhere.

Got a suggestion in mind?

No response

Acknowledgment

boring-cyborg[bot] commented 1 year ago

Thanks for opening your first issue here! We'll come back to you as soon as we can. In the meantime, check out the #python channel on our Powertools for AWS Lambda Discord: Invite link

leandrodamascena commented 1 year ago

Hello @johnmreynolds! Thanks for bringing up this important point about the documentation. If it was difficult for you to find clear information in our documentation, it may be difficult for others too and we need to fix it.

Let me split this issue to make it easier to define the next steps.

1 - Method signature: In the informed method signature, the rule parameter is str because it is a string where in most of cases developers can add static or dynamic paths.

In some special cases, developers can add a regex to catch all routes, or create more advanced dynamic routes

I think the confusion here is that even the signature method is a string, behind the hood we compile a regex to make it more flexible. In this method we have some regex examples, but I agree that is hard to find.

2 - Subfolders:

For example, the tutorials include things like @app.get("/subscriptions/") which seems to be a curious syntax, and I can't find any documentation on this. Also, it doesn't seem to work for subfolders which is weird.

Can you please elaborate a little more on that? I didn't understand the point of subfolders. We support a path like /powertools/test/hello, is that what you mean?

3 - Next steps: Do you have any suggestions that we can add to our documentation to make it clearer? Maybe adding a specific section on regex and some examples? Adding docstring to route method to explain parameters? If you have any ideas or even want to submit a PR to improve our documentation please do so, we love it when we can improve our documentation to make it easier for us to communicate with our customers.

I'm adding the "Pending customer" status because we want to hear from you before proceeding.

Thanks

johnmreynolds commented 1 year ago

Thanks.

On the topic of subfolders, github mangled my comment which included angle brackets, so my example made no sense.

What I meant is like in your example: @app.get("/powertools/<customer_id>")

where customer_id captures the path, it seems not to work if what would be captured for customer_id contains a slash, so is a subfolder of /powertools/. This is probably sensible, and I think is covered by the examples in the source you pointed at (implied by the word boundary match) but I was just noting I couldn't find this documented anywhere.

On how to improve the documentation, I think yes it could do with small additions of:

1) just a short section on what can be passed to the rule, which I think can be one of a) regex, b) a dynamic path with angle brackets or c) a static path (which is a special case of regex).

2) How dynamic path matching works (if I'm reading the comment you linked correctly it's just a word boundary match).

3) How the captured parts of the path get passed to the decorated function. From the example it looks clear this happens as an extra parameter for dynamic paths, but I couldn't see any documentation that this also happens for regex captures (I'm assuming it does - I've just not tested it).

leandrodamascena commented 1 year ago

Hello @johnmreynolds! Thanks for adding your ideas to improve the documentation.

1 - just a short section on what can be passed to the rule, which I think can be one of a) regex, b) a dynamic path with angle brackets or c) a static path (which is a special case of regex).

Except the regex case, where we don't have a detailed section, the other things like dynamic and static path we already have in our documentation. However, I think that since this is split into multiple sections of our documentation, it might be good to create a new section called "Working with Rules" or something similar to make it clearer and explain these gaps.

2 - How dynamic path matching works (if I'm reading the comment you linked correctly it's just a word boundary match).

It can be covered in this new section.

3 - How the captured parts of the path get passed to the decorated function. From the example it looks clear this happens as an extra parameter for dynamic paths, but I couldn't see any documentation that this also happens for regex captures (I'm assuming it does - I've just not tested it).

Same for the previous question.

Do you want to send a PR so we can work together? It would be great to have your contribution to improve the documentation of a widely used utility in Powertools. This PR can be a draft, in case you don't have all the information and I'll help you with whatever you need.

Thanks

leandrodamascena commented 1 year ago

Adding backlog status until we start working on it.

dmarra commented 1 year ago

I want to share my experience (originally from discussion #2976 ):

In the documentation, it is stated in the section about catch-all routes the following:

We choose the most explicit registered route that matches an incoming event.

I understood this to mean that in the case where a more specific match is found, that it will pick the more explicit match over the other. For instance:

@app.get(".+")
def catch_all():
    pass

@app.get("/my/resource/<resource_id>")
def more_explicit():
    pass

In this case, I would think from the docs that a request to /my/resource/12345 would invoke more_explicit, but in practice I see the opposite happen (catch_all is invoked for everything). In fact, it does this no matter what order the methods are added in.

This seems like a bug honestly. But, I can see the difficulty in determining which rules are more explicit than others. How exactly does one determine that reliably from a regex?

I suggest adding the ability to explicitly define the priority of a rule against others. Something like:

@app.get(".+", priority=2)
def catch_all():
    pass

@app.get("/my/resource/<resource_id>", priority=1)
def more_explicit():
    pass

Perhaps with the following behavior:

  1. priority is in ascending order (with 0 being highest for instance)
  2. rules of equal priority are analyzed in the order they appear in the code
  3. not setting a priority defaults to the lowest possible priority (in the example above, not setting priority in a new rule would effectively make it priority=3

Basically, let the user determine which is more important than the other instead of trying to infer it. This way, at least the user has a way out if there is a bug in the ordering

leandrodamascena commented 1 year ago

Hello everyone! Thank you for all your contributions and discoveries. I'm adding this issue in the current iteration (26th Aug to 8th Sept) to work on 1) improving the documentation, 2) seeing if we have any bugs we need to fix, and 3) checking if we can improve the developer experience in any way.

I will keep you posted on updates.

jaysoffian commented 1 year ago
  1. I'll add another few points. I found this not to work:
@app.get("/job/<name>/builds")
def  get_job_builds(name):
    pass

I was not able to make this type of rule work. I kept getting 404 back indicating it wasn't matching.

  1. There is no shortcut for handling HEAD requests. I guess you have to use @app.route("...", method=["HEAD"])

  2. It took a while to find that the original event dict is available as app.current_event.data. More documentation on the event router is needed. (I was able to find it in the API reference, but that's the sort of thing where you kind of have to know where to look to have any hope of finding it.)

  3. I think the tutorial could be more succinct by getting straight to the features that Powertools offers instead of walking folks through building those features themselves. e.g. the entire https://docs.powertools.aws.dev/lambda/python/latest/tutorial/#creating-our-own-router section isn't needed. The whole point of Powertools is that I don't need to write my own router.

In general, Powertools provides a lot of "magic" which is great, except I had to keep going to the source code to figure out how everything worked. In the end I found the cognitive overhead too much and replaced it with my own router which better matched my simpler use case.

Thank you for this excellent tool and I'll revisit using it once I have enough code to justify needing all the power it provides.

heitorlessa commented 1 year ago

hey @jaysoffian, appreciate you taking the time to share what's been challenging -- while I couldn't reproduce the 404 issue (picture) and you are working on a custom router, please allow me to provide additional context for whoever is reading next.

  1. I'll add another few points. I found this not to work:

As long as your API Gateway is configured with the correct routing to Lambda, it is a HTTP GET request, there's no reason for that function not to be called. Despite the incorrect return (pass, I'd guess as a demo), it should return a HTTP 200 with a null body.

For troubleshooting, we have a debug mode that prints out more details about the routing logic, doesn't sanitize exceptions, and whatnot that you might need in Dev mode: https://docs.powertools.aws.dev/lambda/python/latest/core/event_handler/api_gateway/#debug-mode

  1. There is no shortcut for handling HEAD requests. I guess you have to use @app.route("...", method=["HEAD"].

That's right. We'd be more than happy to add a HEAD shortcut, it's the first time we hear from a customer and it's a no brainer to get it done.

  1. It took a whole to find that the original event dict is available as app.current_event.data

That's on us. The original data is available under app.current_event.raw_event. Despite autocompletion and the explicit name, we've got work to do here to surface this more.

In today's release for Middleware support, I've added an example to cover that. Still, we can add a note under this section.

  1. I think the tutorial could be more succinct.

Appreciate your feedback. The reason we did it like that is for non-developers to get a sense of what's before and after. We've had feedback from customers before that they couldn't understand the need for Powertools, and this was a response to that.

As for your feedback, one area we'd like to invest is in more specific tutorials. That way, we can cover a broader range of areas and yet meet the diverse persona that use Powertools for AWS today.

If you've got ideas for Tutorials, please don't hesitate in creating a documentation issue. If you'd like to contribute, we're also more than happy to mentor tech writing too!

To close, we're listening and always have! The minute we stop it's when we lose our identity and tenets on working backwards from the community.

We're more than happy to jump on a call with anyone to troubleshoot when needed (bandwidth permits) - please join us on Discord with 502 other people too 🙏🏻

image