Add support for blueprints #1023

jamesls · 2018-12-22T01:46:10Z

This adds support for Flask style blueprints.
As part of this change, the internals app.py have been refactored
to remove duplication when registering resources. This should make
it easier to refactor the event sources into separate classes
in the future.

This builds on the work from @ewdurbin #651 (thanks!)

I still think this needs some discussion, and we'll need to figure out where it goes based on the outcome of #1019

This adds support for Flask style blueprints. As part of this change, the internals app.py have been refactored to remove duplication when registering resources. This should make it easier to refactor the event sources into separate classes in the future.

codecov-io · 2018-12-22T01:52:32Z

Codecov Report

Merging #1023 into master will decrease coverage by 0.02%.
The diff coverage is 96.18%.

@@            Coverage Diff             @@
##           master    #1023      +/-   ##
==========================================
- Coverage   95.47%   95.45%   -0.03%     
==========================================
  Files          27       27              
  Lines        4488     4528      +40     
  Branches      563      569       +6     
==========================================
+ Hits         4285     4322      +37     
- Misses        131      134       +3     
  Partials       72       72

Impacted Files	Coverage Δ
chalice/__init__.py	`100% <100%> (ø)`	⬆️
chalice/app.py	`96.5% <96.15%> (-0.28%)`	⬇️

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 7045036...53cfc47. Read the comment docs.

mholiv · 2018-12-24T16:11:13Z

Looking forward to this. :) 👍

kyleknap

Looks pretty good just had some comments/questions. A couple things that I am wondering if you have thought about are:

Is there a plan to do nested blueprints? I saw that in flask people were requesting it and I could see people wanting them in chalice as well.
I think blueprints could be really powerful mechanism for providing 3rd party chalice packages that people can pip install and then register the blueprints from the package to their application. Have you looked into that at all or tried it out? I may take some time playing around with this.

kyleknap · 2019-01-03T17:47:35Z

docs/source/topics/blueprints.rst

+        return {'foo': 'bar'}
+
+
+The ``__name__`` is used to denote the import path of the blueprint.  This must


Could you elaborate on why the import_name is a required parameter when it seems like most of the time users will be supplying __name__? When will the import_name not be __name__?

One reason is that it keeps the API the same as Flask, allowing people to port their code over a bit easier.

A second reason is that a single package can occasionally have multiple blueprints.

I was mainly more focused on why it is a required parameter as opposed to being an optional parameter that set the import_name to __name__ when not provided. That should still allow people to port it over from Flask pretty easily and set their own import_name if need be.

There wasn't a strong reason it was required other than to mimic the Flask API. I thought it would be good to make people aware of this parameter rather than to default it for them, but I don't have a strong reason why it needs to be required.

chalice/app.py

kyleknap · 2019-01-03T18:31:29Z

chalice/app.py

+
+    def _register_authorizer(self, name, handler_string, wrapped_handler,
+                             kwargs, **unused):
+        actual_kwargs = kwargs.copy()


This looks like we just copied over the logic from the previous implementation, but do we still need to do the kwargs copying and popping when we are the ones providing the kwargs to this helper method?

I don't think we should be relying on that. I'd try to avoid mutating input parameters where possible.

chalice/app.py

chalice/app.pyi

kyleknap · 2019-01-03T18:48:39Z

docs/source/topics/blueprints.rst

@@ -0,0 +1,198 @@
+Blueprints
+==========


Are we planning on adding API references as well for the Blueprints class? It might be useful at the very least to document the initializer and maybe have a link to the app decorator documentation for each decorator.

kyleknap · 2019-01-03T18:57:54Z

tests/unit/test_app.py

+    @foo.on_sqs_message('MyQueue')
+    def on_sqs(event):
+        pass
+


For this test, is there any way to make sure we update this test whenever we add a new decorator?

We could maybe verify that every public method on DecoratorAPI is also on the blueprint class? In practice I don't think this test is super useful. I originally added it when I was duplicating the decorators in the Blueprint class. Now this test would be verifying that we're subclassing from the DecoratorAPI.

jamesls · 2019-01-08T00:54:05Z

Is there a plan to do nested blueprints?

No, not right now.

I think blueprints could be really powerful mechanism for providing 3rd party chalice packages that people can pip install and then register the blueprints from the package to their application. Have you looked into that at all or tried it out?

Not specifically, but I AFAICT this should just work. You need to register_blueprint() on any blueprint. If you import from a separate package it should just work.

FWIW I don't I'd recommend blueprints for reusable components in its current form now. The big missing piece is a way to specify configuration of some sort. This was alluded to in #608 with "application parameters", but I think you'd need that in order to create reusable packages. It's also not really used that way in Flask (or at least its documentation says its not for reusable components).

jamesls · 2019-01-08T02:24:45Z

@kyleknap I think I've incorporated everything so far except for the import_name being optional. I don't feel too strongly about it so I don't mind making it an optional param if we want.

kyleknap · 2019-01-08T17:32:25Z

The updates look good. As to the import_name being optional, I don't feel too strongly about it either. I'm around a +0 on it being optional. I think I am leaning on the side of not making it required for the following reasons:

I have not thought of a use case where I would specify an import_name other than from porting over a flask app.
If you get the import_name wrong (i.e. not specify __name__), you will be able to chalice deploy the lambda function just fine, but when you go to invoke it, you will get something like:
```
$ chalice invoke -n wrong
Unable to import module 'wrong'
Error: Unhandled exception in Lambda function, details above.
```
So it could be confusing if you are not familiar with import_name and you mess it up.

jamesls · 2019-01-08T20:10:28Z

@kyleknap actually I don't think we can do this. The issue is that we need the import name of the module where the blueprint is created. We can't default that in the Blueprint class itself because it's defined in chalice/app.py. So if we had something like:

class Blueprint:
    def __init__(self):
        self._import_name = __name__

then __name__ will always be the __name__ for the chalice/app.py file (chalice.app). I think this is going to have to stay a required param. The only way to get the correct name is to use __name__ in the module where you create your blueprint.

kyleknap · 2019-01-08T21:14:07Z

Gotcha. Yeah that's a little unfortunate, but makes sense. Let's go ahead and keep it as required.

kyleknap

The code itself looks good. I think we can just push it to an branch on the chalice repository until we decide what to do about experimental features and close out this PR.

PR #1023 * feature-flags: Add feature opt in test through CLI layer Fix linting error for line spacing Update docs with a link to GH issues Move error message to constant Change link from rst to html Add documentation for feature flags Add support for experimental features

jamesls · 2019-01-30T23:07:29Z

Merged via #1057

Add support for blueprints

25a046b

This adds support for Flask style blueprints. As part of this change, the internals app.py have been refactored to remove duplication when registering resources. This should make it easier to refactor the event sources into separate classes in the future.

jamesls requested review from stealthycoin and kyleknap December 22, 2018 01:46

stealthycoin mentioned this pull request Dec 25, 2018

scheduler doesn't provide right route outside of app.py #1016

Open

kyleknap reviewed Jan 3, 2019

View reviewed changes

ewdurbin mentioned this pull request Jan 4, 2019

Hey, I just met you, and this is crazy, but lets put routes in a module... Blueprints maybe? #651

Closed

jamesls added 7 commits January 7, 2019 17:35

Update pyi file to be consistent with app.py

f04a78c

Map lambda context to blueprint

db39c52

Remove unused kwargs param

dea8ec9

Clarify comment on taking kwargs in the route decorator

2c50417

Add test to verify blueprint has all decorators

27b9011

Add API docs for blueprints

9cee7c3

Remove unused params in Blueprint

53cfc47

kyleknap approved these changes Jan 8, 2019

View reviewed changes

stealthycoin approved these changes Jan 10, 2019

View reviewed changes

jamesls mentioned this pull request Jan 30, 2019

Merge blueprints branch back to master #1057

Merged

jamesls closed this Jan 30, 2019

asellappen mentioned this pull request Sep 8, 2021

Adding Build script for chalice on UBI ppc64le/build-scripts#1019

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for blueprints #1023

Add support for blueprints #1023

jamesls commented Dec 22, 2018

codecov-io commented Dec 22, 2018 •

edited

Loading

mholiv commented Dec 24, 2018

kyleknap left a comment

kyleknap Jan 3, 2019

tedivm Jan 3, 2019

kyleknap Jan 3, 2019

jamesls Jan 8, 2019

kyleknap Jan 3, 2019

jamesls Jan 8, 2019

kyleknap Jan 3, 2019

kyleknap Jan 3, 2019

jamesls Jan 8, 2019

jamesls commented Jan 8, 2019

jamesls commented Jan 8, 2019

kyleknap commented Jan 8, 2019

jamesls commented Jan 8, 2019 •

edited

Loading

kyleknap commented Jan 8, 2019

kyleknap left a comment

jamesls commented Jan 30, 2019

		return {'foo': 'bar'}


		The ``__name__`` is used to denote the import path of the blueprint. This must

Add support for blueprints #1023

Add support for blueprints #1023

Conversation

jamesls commented Dec 22, 2018

codecov-io commented Dec 22, 2018 • edited Loading

Codecov Report

mholiv commented Dec 24, 2018

kyleknap 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jamesls commented Jan 8, 2019

jamesls commented Jan 8, 2019

kyleknap commented Jan 8, 2019

jamesls commented Jan 8, 2019 • edited Loading

kyleknap commented Jan 8, 2019

kyleknap left a comment

Choose a reason for hiding this comment

jamesls commented Jan 30, 2019

codecov-io commented Dec 22, 2018 •

edited

Loading

jamesls commented Jan 8, 2019 •

edited

Loading