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

Fix issue #22 (rule lookup does not count inheritance) #32

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Fix issue #22 (rule lookup does not count inheritance) #32

wants to merge 1 commit into from

Conversation

ivan-kleshnin
Copy link

Fix issue #22

The issue is with cache strictly bound to the class object.
Rule lookup doesn't count inheritance correctly.

Then, if you have

class BaseView(FlaskView):
    @route('/delete/<id>')
    def delete(self, id=None):
        pass

class PostsView(BaseView):
    @route('test')  # any route to provide new cache obj instance
    def test(self): # and break behavior
        pass

It generates

Map(...
    <Rule '/posts/<id>' (OPTIONS, DELETE) ...>,
)

While it should generate

Map(...
 <Rule '/delete/<id>' (HEAD, OPTIONS, GET) ...>
)

Diff looks a bit scary but basically i added just 3 lines to deal with inheritance.
This fix passes all tests.

I fixed this fragment

if hasattr(cls, "_rule_cache") and name in cls._rule_cache:
    for idx, cached_rule in enumerate(cls._rule_cache[name]):
        ...
elif name in special_methods:
    ...

by wrapping in .mro() lookup loop:

for _cls in cls.mro()[:-1]: # -1 to skip `object` lookup
    if hasattr(_cls, "_rule_cache") and name in _cls._rule_cache:
        for idx, cached_rule in enumerate(_cls._rule_cache[name]):
            ...
        break
else: # no break
    if name in special_methods: # elif -> if
        ... 

I'm not 100% sure about solution. Flask-Classy is kinda tricky ;)
I'm sure even less about git though...

@apiguy
Copy link
Owner

apiguy commented Jun 12, 2013

This is great. Let me play with it just a bit and add some tests specifically for it.

@apiguy
Copy link
Owner

apiguy commented Jun 14, 2013

After the Python 3 changes (pretty substantial changes were needed) this will need to be modified. I think the general idea is still the right way to do it though. I'll spend some more time on this tomorrow and hopefully get it out.

@ivan-kleshnin
Copy link
Author

Yep, thank you.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants