Author Topic: Lets remove default limit of 25 from the API (Read 1999 times)

Michael McAndrew · November 19, 2012, 12:26:26 am

Hey there,

I guess the reasoning behind having a standard limit of 25 for the API is to stop people hammering the database, but I am now wondering if it is more trouble that it is worth, in a few (if not all cases).

If I want to get all members of a group with

Code: [Select]

civicrm_api('GroupContact', 'get', array('version'=>3, 'group_id' => $params['group_id']));
I only get 25. It is really annoying and not very cool to have to add an arbitrarily high number to option.limit with each call.

I thought I'd be clever and do a getcount call before hand but this also maxed out at 25.

Code: [Select]

civicrm_api('GroupContact', 'getcount', array('version'=>3, 'group_id' => $params['group_id']));
I'd suggest that we remove the 25 limit as a general rule and trust people that are using the API to develop work arounds on a case by case basis on the assumption that if they are clever enough to use an API, they should take responsibility for the load that they are putting on their server.

Also, if there were times in the past when this had caused problems, it would be good to know some details so we can work out ways to deal with them.

Cheers,
Michael

xavier · November 19, 2012, 12:53:18 am

Hi,

Trust in God but Tie Your Camel. Trust in developers, but tie Your queries. as my bedouin guide used to tell me.

Seriously, I think we absolutely need a limit, and that the api shouldn't have a default mode that potentially explode the db. Calling api.contact.get must not by default take your server down.

If you have no limit, it means unless you implement a paging mechanism it will fail (time out/memory) if it becomes big enough. and if you implement a paging, then you have an offset and... a limit.

However,
1) I think we should get a options.limit=0 or options.limit=false when you really want all of them (and therefore explicitly acknowledge you want it to explode in your face given the wrong use case)

2) getCount is by default implemented by doing a count(get()). If you plan to use it like you mention for some entities, please patch and implement a sql count() version instead

3) the api is accessible to "normal" civicrm users, not only dev (via the ajax api).

4) And in my experience, dev are not necessary the one hosting and having to deal with the extra load. It's way easier to get the dev scratching his head while writing the feature and adding the limit than having the server down and always very painful and time consuming investigations to know which bit is putting it on fire

5) might be good to add a "notice":"options.limit in action" in the result?

et voila

X+

Erik Hommel · November 19, 2012, 04:51:01 am

I Tie My Camel at All Times! Agree with X.

Michael McAndrew · November 20, 2012, 05:56:35 am

Hey there,

Quote

the api shouldn't have a default mode that potentially explode the db

as kurund would say 'yes and no'

i do agree but there are many UI elements that (on the wrong data set and hardware) allow you to explode the db via the UI. i.e. exploding the DB is something that depends not just on the API but on your hardware and dataset. Why punish people who want to write nice simple apis because other people write bad code? setting a global limit at 25 seems a bit weird and arbitrary to me.

Quote

the api is accessible to "normal" civicrm users, not only dev (via the ajax api).

Maybe we implement a safe mode with get limit of 25 that is used when the api is exposed to mere mortals.

Quote

in my experience, dev are not necessary the one hosting and having to deal with the extra load. It's way easier to get the dev scratching his head while writing the feature and adding the limit than having the server down and always very painful and time consuming investigations to know which bit is putting it on fire

in my experience, the dev (a.k.a. me adds option.limit=100000000 and explodes the database anyway (and then you do the painful time consuming investigation). hence this is not a good way to teach developers how to write not exploding code.

Quote

If you have no limit, it means unless you implement a paging mechanism it will fail (time out/memory) if it becomes big enough. and if you implement a paging, then you have an offset and... a limit.

ah yes - essentially this is what we have in the UI - an explicit paging mechanism (since if i look at a list of 25 contacts, i see the next link and it is obvious to me that there are a lot more contacts hidden on the next page. I think in the API we have an implicit / hidden paging mechanism and it would be nice to improve it. AFAIK there is no way to know to tell if you are looking at "ALL 25 records" or "THE FIRST 25 records".

Quote

might be good to add a "notice":"options.limit in action" in the result?

Maybe what we need is a 'more' link, i.e. an invitation and convenient way to get the next N results.

Some questions...

What API actions does this limit apply? Is it just get and get*?

And some suggestions (some of which you have already suggested

)

* we should implement option.limit=0 for all api calls that removes the limit
* we should consider allowing people to specify option.limit=0 (or -1) for all api calls where option.limit appies
* we should consider setting getcount to have an option.limit=0 by default. since surely the purpose of this is to know how many results you are looking at. I think so, but am willing to be persuaded otherwise.

might be nice to have an api object rather than an api function that allows you to set default params / options like

Code: [Select]

$api = new civicrm_api;
$api->setOption('limit', '0');
$api->setOption('version', '3');
$contacts = $api->action('Contact', 'get');
//oops, I exploded my DB

xavier · November 20, 2012, 12:44:36 pm

Quote from: Michael McAndrew on November 20, 2012, 05:56:35 am

i do agree but there are many UI elements that (on the wrong data set and hardware) allow you to explode the db via the UI. i.e. exploding the DB is something that depends not just on the API but on your hardware and dataset.

I'd rather suggest to fix the UI if needed rather than to brake the api to make them equal

Quote

Why punish people who want to write nice simple apis because other people write bad code?

By punish, you mean having to add an options.limit => max ? If this is what you call punish, we are quite ok, I think

Quote

setting a global limit at 25 seems a bit weird and arbitrary to me.

Completely arbitrary. I would support your motion to switch to 42.

Quote

Maybe we implement a safe mode with get limit of 25 that is used when the api is exposed to mere mortals.

I'd rather stick with the same (arbitrary) limit, no matter the channel. I'm sure the super heros venturing the php world will overcome the punishment

Quote

in my experience, the dev (a.k.a. me adds option.limit=100000000 and explodes the database anyway (and then you do the painful time consuming investigation). hence this is not a good way to teach developers how to write not exploding code.

Not sure how to communicate the risk better, at least by having an option.limit, it's easier to grep and, hopefully, gives a hint to the dev that might not be the best way to do it.

Quote

ah yes - essentially this is what we have in the UI - an explicit paging mechanism (since if i look at a list of 25 contacts, i see the next link and it is obvious to me that there are a lot more contacts hidden on the next page. I think in the API we have an implicit / hidden paging mechanism and it would be nice to improve it. AFAIK there is no way to know to tell if you are looking at "ALL 25 records" or "THE FIRST 25 records".

Good point. A cheap way could be that if limit is empty, it is set to 26, fetch 25 and return only the first 25. If there are 26, means that's the first 25 records, not all the 25 and we can return a count=25 AND a new more=yes return

Quote

Maybe what we need is a 'more' link, i.e. an invitation and convenient way to get the next N results.

not sure the link makes sense, it would be for php, rest, smarty and ajax

Quote

What API actions does this limit apply? Is it just get and get*?

My turn to quote kurund: it's applied where it's implemented.
Most of the get and get* have either implemented it specifically, or are using the basic generic api that has it implemented. If there is one that hasn't it implemented, it's a bug.

Quote

* we should implement option.limit=0 for all api calls that removes the limit
* we should consider allowing people to specify option.limit=0 (or -1) for all api calls where option.limit appies

option.limit=0 or =false sounds like a patch we would welcome

I don't think we ever limit who can set option.limit, nor that we check if the value isn't really high. so option.limit=0 should be allowed indeed.

Quote

* we should consider setting getcount to have an option.limit=0 by default. since surely the purpose of this is to know how many results you are looking at. I think so, but am willing to be persuaded otherwise.

getcount needs some love to avoid the default count(get(). Eileen probably has a better idea of why it wasn't done more efficiently (at the sql level as a select count(*)... ), but I don't think there is an easy way to do it.

Code: [Select]

$api = new civicrm_api;
$api->setOption('limit', '0');
$api->setOption('version', '3');
$contacts = $api->action('Contact', 'get');
//oops, I exploded my DB

What we have is

Code: [Select]

$api = new civicrm_api3();
$api->attr('option.limit', '0');
$api->Contact->get();
//or 
$api->Contact->get(array("option.limit"=>0));
//or
$api->attr('option.limit', '0')->get();
//oops, I exploded my DB, but at least you know why, and you asked for it

I think we are between consenting adults, if you want to do something, the api shouldn't stop you doing it, even if it's shoot in your own foot. What I think is good is that it isn't the default behaviour and you can be reasonably sure your foot will be safe by default.

X+

Eileen · November 20, 2012, 06:40:30 pm

getcount should be fixed on the contact get for 4.2 or 4.3 (can't remember which). Improving getcount isn't a big job - it's just one of so many jobs!

Note that the limit doesn't just protect you from your own bad code. For example the contribution_get api has a bad join (or more) on the option_value table which comes from the underlying query object. You don't want to increase your exposure to that

jcasharpe · November 27, 2012, 02:53:08 pm

Why not take the approach taken by mysql and return a fetch object with a second api call that called in a loop returns each element in turn? (see http://php.net/manual/en/function.mysql-fetch-object.php)

Alternatively provide a version of the api with a callback function which gets called with each api element in turn. Admittedly this doesn't cover all use cases but probably covers a large percentage of them. The underlying principal here is to abstract away at a level that doesn't expose the details of the protection of the size of the queries (after all different queries are going to have different memory footprints...). Also it'd be nice to be able to do 'batch' actions via the api, i.e. stuff that'd normally time out but can be pushed out into some asynchronous job queue to be executed. I don't want to have to implement that kind of logic by hand myself each time and it feels like something that should be done right once as doing it right is non-trivial. I guess the dedupe logic is an example of this?

Coleman Watts · November 27, 2012, 08:02:34 pm

Hey, let's sign jcasharpe up to be on the API team!

Erik Hommel · November 27, 2012, 11:18:47 pm

Good idea Coleman.....can you explain the warm beer concept?

xavier · November 28, 2012, 12:36:21 am

@jcasharpe,

I like the idea, probably better to build that on the oo version of the api then (in api/class.api.php) than on the civicrm_api function unless we can do that without having to change signature.

Want to start working on it?

This being said, the api is as well called from smarty rest and ajax. Unless we start implementing stuff like sockets, won't be possible to batch/page the results.

CiviCRM Community Forums (archive)

News:

Author Topic: Lets remove default limit of 25 from the API (Read 1999 times)

Michael McAndrew

Lets remove default limit of 25 from the API

xavier

Re: Lets remove default limit of 25 from the API

Erik Hommel

Re: Lets remove default limit of 25 from the API

Michael McAndrew

Re: Lets remove default limit of 25 from the API

xavier

Re: Lets remove default limit of 25 from the API

Eileen

Re: Lets remove default limit of 25 from the API

jcasharpe

Re: Lets remove default limit of 25 from the API

Coleman Watts

Re: Lets remove default limit of 25 from the API

Erik Hommel

Re: Lets remove default limit of 25 from the API

xavier

Re: Lets remove default limit of 25 from the API