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

Pass UserCtx to fabric's all_docs from mango query #627

Merged
merged 4 commits into from Jul 13, 2017

Conversation

Projects
None yet
5 participants
@eiri
Member

eiri commented Jun 30, 2017

Overview

We don't pass UserCtx in fabric:all_docs/4 call in mango's view query which doesn't matter most of the times, except when we need to check admin credentials to return documents as in case of the queries against _usersdatabase. As a result we are getting back null as a body of the documents and crash.

Testing recommendations

Steps described in #535 shouldn't crash with case_clause, but should return a proper document.

GitHub issue number

Fixes #535

Checklist

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

@eiri eiri requested review from davisp and tonysun83 Jun 30, 2017

@eiri eiri added bug mango labels Jun 30, 2017

@tonysun83

This comment has been minimized.

Show comment
Hide comment
@tonysun83

tonysun83 Jul 3, 2017

Contributor

The only thing that confuses me here a bit is that the original bug poster mentioned he created a index on id and name field. The error for fabric:all_docs call is initiated when there's no user-defined index, and we have to use the _all_docs index to retrieve results. It would be nice to have a python test that tries to query the _users db with and without an index just to be safe.

Contributor

tonysun83 commented Jul 3, 2017

The only thing that confuses me here a bit is that the original bug poster mentioned he created a index on id and name field. The error for fabric:all_docs call is initiated when there's no user-defined index, and we have to use the _all_docs index to retrieve results. It would be nice to have a python test that tries to query the _users db with and without an index just to be safe.

@wohali

This comment has been minimized.

Show comment
Hide comment
@wohali

wohali Jul 3, 2017

Member

@tonysun83 Ideally it'd be a eunit or a javascript test. Apache CouchDB has no python test framework at present.

Member

wohali commented Jul 3, 2017

@tonysun83 Ideally it'd be a eunit or a javascript test. Apache CouchDB has no python test framework at present.

@garrensmith

This comment has been minimized.

Show comment
Hide comment
@garrensmith
Member

garrensmith commented Jul 3, 2017

@wohali the mango test suite is a python test suite https://github.com/apache/couchdb/tree/master/src/mango/test

@eiri

This comment has been minimized.

Show comment
Hide comment
@eiri

eiri Jul 3, 2017

Member

@tonysun83 I've followed the steps in the original issue and got the described case_clause error, so I take it that actually there were no index created.

Sure, I'll add the notetests, but as @wohali mentioned, that mango's python test ran neither by travis or jenkins, so that's not awfully useful.

Member

eiri commented Jul 3, 2017

@tonysun83 I've followed the steps in the original issue and got the described case_clause error, so I take it that actually there were no index created.

Sure, I'll add the notetests, but as @wohali mentioned, that mango's python test ran neither by travis or jenkins, so that's not awfully useful.

@tonysun83

This comment has been minimized.

Show comment
Hide comment
@tonysun83

tonysun83 Jul 3, 2017

Contributor

I think we wanted to convert the mango test framework over to eunit at some point. It's become such a clunky thing. I guess we could also convert the entire thing over to js as well.

@eiri: I read the first parts of the bug report. Now I see with an index created and posted by @wohali

[error] 2017-05-22T23:22:03.877980Z node1@127.0.0.1 <0.17275.0> 81bbcf9c62 req_err(3408688270) badrecord : vacc
    [<<"fabric:query_view/6 L343">>,<<"mango_cursor_view:execute/3 L98">>,<<"mango_httpd:handle_find_req/2 L185">>,<<"mango_httpd:handle_req/2 L37">>,<<"chttpd:process_request/1 L295">>,<<"chttpd:handle_request_int/1 L231">>,<<"mochiweb_http:headers/6 L122">>,<<"proc_lib:init_p_do_apply/3 L237">>]

This is interesting because when it's users_db in query_view we're doing:

case fabric_util:is_users_db(Db) of
    true ->
        Req = Acc0#vacc.req,
        FakeDb = fabric_util:fake_db(Db, [{user_ctx, Req#httpd.user_ctx}]),
        couch_users_db:after_doc_read(DDoc, FakeDb);
Contributor

tonysun83 commented Jul 3, 2017

I think we wanted to convert the mango test framework over to eunit at some point. It's become such a clunky thing. I guess we could also convert the entire thing over to js as well.

@eiri: I read the first parts of the bug report. Now I see with an index created and posted by @wohali

[error] 2017-05-22T23:22:03.877980Z node1@127.0.0.1 <0.17275.0> 81bbcf9c62 req_err(3408688270) badrecord : vacc
    [<<"fabric:query_view/6 L343">>,<<"mango_cursor_view:execute/3 L98">>,<<"mango_httpd:handle_find_req/2 L185">>,<<"mango_httpd:handle_req/2 L37">>,<<"chttpd:process_request/1 L295">>,<<"chttpd:handle_request_int/1 L231">>,<<"mochiweb_http:headers/6 L122">>,<<"proc_lib:init_p_do_apply/3 L237">>]

This is interesting because when it's users_db in query_view we're doing:

case fabric_util:is_users_db(Db) of
    true ->
        Req = Acc0#vacc.req,
        FakeDb = fabric_util:fake_db(Db, [{user_ctx, Req#httpd.user_ctx}]),
        couch_users_db:after_doc_read(DDoc, FakeDb);
@tonysun83

This comment has been minimized.

Show comment
Hide comment
@tonysun83

tonysun83 Jul 3, 2017

Contributor

Also, we would probably want to check what happens if users created a mango text index as well.

Contributor

tonysun83 commented Jul 3, 2017

Also, we would probably want to check what happens if users created a mango text index as well.

@eiri

This comment has been minimized.

Show comment
Hide comment
@eiri

eiri Jul 3, 2017

Member

I think we wanted to convert the mango test framework over to eunit at some point.

Conceptually they are closer to our javascript suite, but I agree, it's better to decide on a single test framework, should we stick with existing js suite or port everything to something more user-friendly and stable.

Member

eiri commented Jul 3, 2017

I think we wanted to convert the mango test framework over to eunit at some point.

Conceptually they are closer to our javascript suite, but I agree, it's better to decide on a single test framework, should we stick with existing js suite or port everything to something more user-friendly and stable.

@eiri

This comment has been minimized.

Show comment
Hide comment
@eiri

eiri Jul 5, 2017

Member

This turned out to be more complicated, two-fold issue:

  1. fabric:query_view/6 always expects Acc to be #vacc record and we are passing #cursor from mango there, so when an index presented it is indeed crashing with badrecord. Different issue than what's fixed here.

  2. We don't pass user context to db from fabric_view_map at all, so including_docs not working on _users db even without mango, on honest views:

$ echo '{"name": "demo", "password": "apple", "roles": [], "type": "user"}' | http put :15984/_users/org.couchdb.user:demo
{
    "id": "org.couchdb.user:demo",
    "ok": true,
    "rev": "1-16edca99a7b08fc65cdc963f40c9fa9d"
}


$ echo '{"views": {"names": {"map": "function(doc) { emit(doc.name); }"}}}' | http put :15984/_users/_design/users 
{
    "id": "_design/users",
    "ok": true,
    "rev": "1-fb4b161306a6da60301f313cc827704c"
}

$ http :15984/_users/_design/users/_view/names include_docs==true
{
    "offset": 0,
    "rows": [
        {
            "doc": null,
            "id": "org.couchdb.user:demo",
            "key": "demo",
            "value": null
        }
    ],
    "total_rows": 1
}

That "doc" shouldn't be null.

So I'm going to fix that in separate PR, amend this one to work for mango indexes and make it depended on the former.

Member

eiri commented Jul 5, 2017

This turned out to be more complicated, two-fold issue:

  1. fabric:query_view/6 always expects Acc to be #vacc record and we are passing #cursor from mango there, so when an index presented it is indeed crashing with badrecord. Different issue than what's fixed here.

  2. We don't pass user context to db from fabric_view_map at all, so including_docs not working on _users db even without mango, on honest views:

$ echo '{"name": "demo", "password": "apple", "roles": [], "type": "user"}' | http put :15984/_users/org.couchdb.user:demo
{
    "id": "org.couchdb.user:demo",
    "ok": true,
    "rev": "1-16edca99a7b08fc65cdc963f40c9fa9d"
}


$ echo '{"views": {"names": {"map": "function(doc) { emit(doc.name); }"}}}' | http put :15984/_users/_design/users 
{
    "id": "_design/users",
    "ok": true,
    "rev": "1-fb4b161306a6da60301f313cc827704c"
}

$ http :15984/_users/_design/users/_view/names include_docs==true
{
    "offset": 0,
    "rows": [
        {
            "doc": null,
            "id": "org.couchdb.user:demo",
            "key": "demo",
            "value": null
        }
    ],
    "total_rows": 1
}

That "doc" shouldn't be null.

So I'm going to fix that in separate PR, amend this one to work for mango indexes and make it depended on the former.

@eiri

This comment has been minimized.

Show comment
Hide comment
@eiri

eiri Jul 5, 2017

Member

Depends on #645

Member

eiri commented Jul 5, 2017

Depends on #645

@nickva nickva self-assigned this Jul 7, 2017

@tonysun83

This comment has been minimized.

Show comment
Hide comment
@tonysun83

tonysun83 Jul 10, 2017

Contributor

@eiri @nickva : The source looks good to me. The tests also pass for me. If there's a better pythonic way to combine the two new test files(one tests _all_docs, the other tests user created indexes) such that we don't need two separate test files that would good. It's not a big deal though.

Contributor

tonysun83 commented Jul 10, 2017

@eiri @nickva : The source looks good to me. The tests also pass for me. If there's a better pythonic way to combine the two new test files(one tests _all_docs, the other tests user created indexes) such that we don't need two separate test files that would good. It's not a big deal though.

@nickva

This comment has been minimized.

Show comment
Hide comment
@nickva

nickva Jul 10, 2017

Contributor

@tonysun83 good point!

Should be doable. If we had parameterized tests like py.test framework has, then index = True | False could be a parameter. Maybe will just make the tests a mixin and have two test classes that uses the mixin - one with an index one without.

Contributor

nickva commented Jul 10, 2017

@tonysun83 good point!

Should be doable. If we had parameterized tests like py.test framework has, then index = True | False could be a parameter. Maybe will just make the tests a mixin and have two test classes that uses the mixin - one with an index one without.

@nickva

This comment has been minimized.

Show comment
Hide comment
@nickva

nickva Jul 12, 2017

Contributor

@tonysun83 take another look.

Since most tests are the same I put the default ones in a base test class. Then the index version overrides those methods and where it creates an index it does that first then call the superclass test method.

Contributor

nickva commented Jul 12, 2017

@tonysun83 take another look.

Since most tests are the same I put the default ones in a base test class. Then the index version overrides those methods and where it creates an index it does that first then call the superclass test method.

@tonysun83

This comment has been minimized.

Show comment
Hide comment
@tonysun83

tonysun83 Jul 13, 2017

Contributor

+1

Contributor

tonysun83 commented Jul 13, 2017

+1

@nickva nickva merged commit 779f00e into apache:master Jul 13, 2017

1 check was pending

continuous-integration/travis-ci/pr The Travis CI build is in progress
Details

@nickva nickva deleted the cloudant:set-user_ctx-in-mango_view branch Jul 13, 2017

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