Skip to content
This repository has been archived by the owner on May 25, 2021. It is now read-only.

Make Query Limit Results Configurable #56

Open
wants to merge 4 commits into
base: master
Choose a base branch
from
Open

Make Query Limit Results Configurable #56

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
92575f4
make query limit results configurable
Sep 5, 2016
0c1b4e3
use macro for default query limit
Sep 5, 2016
460615d
change default limit name
Sep 6, 2016
a31f613
make admin configuration limit higher priority than user limit
Sep 6, 2016
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
4 changes: 3 additions & 1 deletion include/couch_mrview.hrl
View file
Open in desktop
Expand Up @@ -10,6 +10,8 @@
% License for the specific language governing permissions and limitations under
% the License.

-define(DEFAULT_LIMIT, 16#10000000).

-record(mrst, {
sig=nil,
fd=nil,
Expand Down Expand Up @@ -73,7 +75,7 @@
keys,

direction = fwd,
limit = 16#10000000,
limit = ?DEFAULT_LIMIT,
skip = 0,
group_level = 0,
group = undefined,
Expand Down
12 changes: 9 additions & 3 deletions src/couch_mrview_http.erl
View file
Open in desktop
Expand Up @@ -469,10 +469,16 @@ parse_params(Props, Keys, #mrargs{}=Args0, Options) ->
IsDecoded = lists:member(decoded, Options),
% group_level set to undefined to detect if explicitly set by user
Args1 = Args0#mrargs{keys=Keys, group=undefined, group_level=undefined},
lists:foldl(fun({K, V}, Acc) ->
Args2 = lists:foldl(fun({K, V}, Acc) ->
parse_param(K, V, Acc, IsDecoded)
end, Args1, Props).

end, Args1, Props),
Limit = Args2#mrargs.limit,
case config:get_integer("couch_db", " default_query_limit", -1) of
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this should be max_query_limit, right?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

max_query_limit confusing. Is it max value I can put into limit query parameter?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But section name couch_db is something strange. We don't have any of such.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I just mean it needs to match with the setting in the test, but yes, that's the point, right? An optional forced upper value of ?limit=X.

good catch of section name, it should be couch_mrview imo.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see. My point was about thin difference between default_query_limit and max_query_limit. First one assumes that you can specify any value instead to override the default, even greater than. The second implies (at least for me) that I cannot beat that value in config file, so ?limit=100 with max_query_limit=10 will still return 10 records back.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hm..I should read the code probably, lol. The min(Limit, ConfigDefault1) is quite specific on the intentions. So everything is correct here, you're right.

ConfigDefault0 when ConfigDefault0 < 1 ->
Args2;
ConfigDefault1 ->
Args2#mrargs{limit=min(Limit, ConfigDefault1)}
end.

parse_param(Key, Val, Args, IsDecoded) when is_binary(Key) ->
parse_param(binary_to_list(Key), Val, Args, IsDecoded);
Expand Down
15 changes: 15 additions & 0 deletions test/couch_mrview_all_docs_tests.erl
View file
Open in desktop
Expand Up @@ -107,6 +107,21 @@ should_query_with_limit_and_skip(Db) ->
]},
?_assertEqual(Expect, Result).

should_query_with_config_limit_and_skip(Db) ->
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this test is never run.

/Users/robertnewson/Source/asf/couchdb/src/couch_mrview/test/couch_mrview_all_docs_tests.erl:110: Warning: function should_query_with_config_limit_and_skip/1 is unused

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(neither is /Users/robertnewson/Source/asf/couchdb/src/couch_mrview/test/couch_mrview_map_views_tests.erl:97: Warning: function should_map_with_config_limit/1 is unused)

This is cause we need to mention them explicitly above, they're not auto-detected

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

my suspicion is your new test fails, because of the bug mentioned.

Copy link
Author

@tonysun83 tonysun83 Sep 6, 2016 •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I will fix the tests. I agree with @kxepal : it should be default_query_limit rather than max_query_limit. I just need to change it in the test case and also remove the whitespace in the quotes. if there are no objections, I will go ahead and use that name, cc @rnewson.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@tonysun83 Wait. We just figured out that default_query_limit doesn't reflects the behaviour.

ok = config:set("couch_db", "max_query_limit", 3),
Result = run_query(Db, [
{start_key, <<"2">>},
{limit, 6},
{skip, 3}
]),
Expect = {ok, [
{meta, [{total, 11}, {offset, 5}]},
mk_row(<<"5">>, <<"1-aaac5d460fd40f9286e57b9bf12e23d2">>),
mk_row(<<"6">>, <<"1-aca21c2e7bc5f8951424fcfc5d1209d8">>),
mk_row(<<"7">>, <<"1-4374aeec17590d82f16e70f318116ad9">>)
]},
?_assertEqual(Expect, Result).

should_query_with_include_docs(Db) ->
Result = run_query(Db, [
{start_key, <<"8">>},
Expand Down
9 changes: 9 additions & 0 deletions test/couch_mrview_map_views_tests.erl
View file
Open in desktop
Expand Up @@ -94,6 +94,15 @@ should_map_with_limit_and_skip(Db) ->
]},
?_assertEqual(Expect, Result).

should_map_with_config_limit(Db) ->
ok = config:set("couch_db", "max_query_limit", 1),
Result = run_query(Db, []),
Expect = {ok, [
{meta, [{total, 1}, {offset, 0}]},
{row, [{id, <<"1">>}, {key, 1}, {value, 1}]}
]},
?_assertEqual(Expect, Result).

should_map_with_include_docs(Db) ->
Result = run_query(Db, [
{start_key, 8},
Expand Down