flask-classful

flask-classful copied to clipboard

Views can have a route_base, which can contain parameters (e.g. route_base = /foo/<bar>). Such parameters are given as arguments to the methods of the view. When generating routing rules for the method of such a view, the argument(s) of the method corresponding to the parameter(s) of the route_base should be ignored for the purpose of generating routing rules.

This is currently done by discovering such parameters when parsing route_base, and adding the discovered parameters to a base_args list bound directly to the class. However, the class argument is declared at the level of FlaskView. I realized that when subclassing it multiple times with route_base containing parameters, the list containing the parameters is shared by all subclasses. Thus, previously discovered parameters have an influence on all subsequently registered view methods and cause erroneous routing rules being generated, for example with the following:

class FooDeepView(FlaskView):
    route_base = "/foo/<bar>/quz"
    def get(self, bar, baz):
        ...

class FooShallowView(FlaskView):
    route_base = "/foo"
    def get(self, bar):
        ...
        
FooDeepView.register(app)
FooShallowView.register(app)

Here, bar in FooShallowView would be ignored due to bar being in base_args after registering FooDeepView. The expected GET /foo/<bar> -> FooShallowView.get routing rule would be incorrectly generated as GET /foo -> FooShallowView.get since bar is ignored. Then, GET /foo/whatever ends up with a 404 since no routing rule correspond to this path.

It's relatively straightforward to fix though, by removing base_args at the level of the class, and simply returning the discovered parameters along with the route_base in get_route_base and appending the returned parameters to the ignored arguments. base_args is not used elsewhere and is not directly used by users.

I also adjusted test_base_args to focus on simply testing the behavior of route_base with parameters.

You removed 11 tests in favour of 1 new one. Are you sure the same functionality is covered (presumably partly by other tests)?

I had removed the old tests in this file because it wasn't clear what was being tested, introduced unnecessary dependencies in the tests, and some were testing the same behavior. But the test I introduced was too narrow indeed. I've added:

• A test on whether a route_base with no args would have some base_args (it shouldn't)
• A test on the detection of base_args for the purpose of ignoring them during route generation
• A test on the values of multiple args in a route_base, combined with a method arg.
• A test on the issue that occurred in my case, i.e. contamination of base_args to subsequently registered views
• A test on the expected (current) error where an arg in the route_base but missing in the method arguments will cause a TypeError at call time.

Hi @superseed, thanks for contribution!

It would seem to me to be better to split this into multiple PRs:

• adding your new behavior + tests for it
• refactoring current test suite
• adding missing tests you discovered.

I realize it's more work, but it would in my opinion allow clearer discussion about these parts, and faster decisions on what to merge.

Do you agree?

I needed to modify existing tests due to my changes, they tested using the internal base_args list directly rather than using route_base. I thought that if I was going to refactor the tests, I could try to make them clearer in the process.