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 Mango error handling #804

Merged
merged 3 commits into from Sep 12, 2017

Conversation

Projects
None yet
3 participants
@willholley
Member

willholley commented Sep 11, 2017

Overview

Fixes a regression where a 500 status code was returned when no index is available to service a _find query because the sort order does not match any available indexes. This was actually a general issue with _find errors, introduced when index selection moved from mango_cursor.erl to mango_idx.erl in apache/couchdb-mango@01252f9.

In a separate commit, I converted the index selection tests to use unittest assertion functions and addressed an issue with a duplicate test name.

Testing recommendations

Changes covered by unit tests

GitHub issue number

Related Pull Requests

Checklist

  • Code is written and works correctly;
  • Changes are covered by tests;
  • Documentation reflects the changes;

@willholley willholley changed the title from Mango invalid sort to Fix Mango error handling Sep 11, 2017

@iilyak

iilyak approved these changes Sep 11, 2017

+1. Tests pass locally

nosetests test/05-index-selection-test.py
..S..S..SSSSS
----------------------------------------------------------------------
Ran 13 tests in 5.010s
@eiri

This comment has been minimized.

Show comment
Hide comment
@eiri

eiri Sep 12, 2017

Member

What's the reasoning for complete removal of module name match? As far as I can see no_usable_index errors still thrown from a single and not multiple modules and as a matter of fact {no_usable_index, selector_unsupported} kept in mango_cursor and doesn't require changes in error module

Member

eiri commented Sep 12, 2017

What's the reasoning for complete removal of module name match? As far as I can see no_usable_index errors still thrown from a single and not multiple modules and as a matter of fact {no_usable_index, selector_unsupported} kept in mango_cursor and doesn't require changes in error module

@willholley

This comment has been minimized.

Show comment
Hide comment
@willholley

willholley Sep 12, 2017

Member

@eiri I didn't see the benefit of scoping those error message conversions to a single module; we're not likely to change the status code or message based on the module that raised the error and it makes the error conversion more brittle (as illustrated by the change that led to this regression). Arguably this logic applies to all of mango_error.erl - perhaps this PR should stick to the existing convention and use a separate PR to discuss whether to remove the module scope from error conversion?

Member

willholley commented Sep 12, 2017

@eiri I didn't see the benefit of scoping those error message conversions to a single module; we're not likely to change the status code or message based on the module that raised the error and it makes the error conversion more brittle (as illustrated by the change that led to this regression). Arguably this logic applies to all of mango_error.erl - perhaps this PR should stick to the existing convention and use a separate PR to discuss whether to remove the module scope from error conversion?

@eiri

This comment has been minimized.

Show comment
Hide comment
@eiri

eiri Sep 12, 2017

Member

I don't know for sure, but my guess that was a kind of a rudimentary traceback that got lost in a refactoring.

Yes, as I see it you here mixing fixes for index selection refactoring with mango_error's signature changes, which I'm not against of, but would prefer to be done in more organized fashion, rather than the scope creep. If that's all right with you let's do that in separate PR for sake of accountability and simplified revert, if that'd be necessary.

Member

eiri commented Sep 12, 2017

I don't know for sure, but my guess that was a kind of a rudimentary traceback that got lost in a refactoring.

Yes, as I see it you here mixing fixes for index selection refactoring with mango_error's signature changes, which I'm not against of, but would prefer to be done in more organized fashion, rather than the scope creep. If that's all right with you let's do that in separate PR for sake of accountability and simplified revert, if that'd be necessary.

willholley added some commits Sep 11, 2017

Use unittest assertions in mango index tests
The assertion functions inherited from unittest
provide clearer errors when tests fail - use these
in preference to plain assert.
Return 400 when no index can fulfil a sort
Fixes a regression where a 500 status code was returned when
no index is available to service a _find query because the
sort order does not match any available indexes.
@willholley

This comment has been minimized.

Show comment
Hide comment
@willholley

willholley Sep 12, 2017

Member

@eiri Interesting - I wonder why the logging was removed. In any case, I've reverted to the existing pattern for now and we can consider the error signature changes as a separate PR.

Member

willholley commented Sep 12, 2017

@eiri Interesting - I wonder why the logging was removed. In any case, I've reverted to the existing pattern for now and we can consider the error signature changes as a separate PR.

@eiri

This comment has been minimized.

Show comment
Hide comment
@eiri

eiri Sep 12, 2017

Member

Judging by minimalistic description of commit where it was done - in heat of refactoring :)

Cool, thank you for the change! +1

Member

eiri commented Sep 12, 2017

Judging by minimalistic description of commit where it was done - in heat of refactoring :)

Cool, thank you for the change! +1

@willholley willholley merged commit cf00dc2 into apache:master Sep 12, 2017

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@willholley willholley deleted the willholley:mango_invalid_sort branch Sep 12, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment