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 bug with registration of identical views with same 'accept'. #404

Merged
merged 2 commits into from
Jan 19, 2012

Conversation

dnouri
Copy link
Member

@dnouri dnouri commented Jan 17, 2012

In Kotti I register two views for HTTPForbidden:

config.add_view(
    forbidden_redirect,
    context=HTTPForbidden,
    accept='text/html',
    )

config.add_view(
    forbidden_view,
    context=HTTPForbidden,
    )

With this configuration, and due to the bug, it's not possible to override the first registration (the one with the accept). The following call will work (given autocommit=True or similar) but will fail to override the existing view; the result is that both views are registered in the MultiView but the first is sometimes preferred (both will have the same 'order'):

config.add_view(
    another_forbidden_view,
    context=HTTPForbidden,
    accept='text/html',
    )

Note: the fix looks very similar to the overwriting a couple lines above. However, I've added the check for order == s, since tests (TestMultiView.test_add) expect views with same 'accept' but different 'order' to live alongside each other.

    if phash == h and order == s:
        subset[i] = (order, view, phash)
        return

@mmerickel
Copy link
Member

Out of curiosity does this solve your problem?

config.add_view(
    forbidden_redirect,
    context=HTTPForbidden,
    accept='text/html',
    )

config.add_view(
    forbidden_view,
    context=HTTPForbidden,
    accept='*/*'
    )

It seems to me the actual bug here is that for some reason you're able to add the view with the same accept twice without committing between (in test_add_views_with_accept_multiview_replaces_existing).

@dnouri
Copy link
Member Author

dnouri commented Jan 17, 2012

Using accept='*/*' as you suggest produces a PredicateMismatch when I then try the view lookup.

Note that adding a view with the same accept twice is not possible when autocommit=False; it will correctly generate a configuration conflict. (That's contrary to your last statement, if I understood you correctly.)

@mmerickel
Copy link
Member

Yeah misread the unit test, didn't notice the autocommit=True.

@dnouri
Copy link
Member Author

dnouri commented Jan 17, 2012

I've updated the changeset to:

  • remove the changes I originally made to TestMultiView.test_add, and only change that existing test so much to make it work
  • add another test (test_add_with_phash_override_accept) that shows the bug more clearly than the previous one did

@mcdonc mcdonc merged commit f4b7fd8 into Pylons:master Jan 19, 2012
@mcdonc
Copy link
Member

mcdonc commented Jan 19, 2012

Thanks Daniel!

mcdonc added a commit that referenced this pull request Jan 20, 2012
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants