Cannot use "limit" and "offset" functionnality

[poutsjr]

It's impossible to change the limit and offset value in
com.taskadapter.redmineapi.internal.Transport.getObjectsList(Class, Collection)

if it's defined in params, it will be added twice so the url will be :
/issues.json?project_id=my_project&offset=0&limit=5&limit=25&offset=0

If the parameter (limit or offset) is already defined, it would not be added

[alexeyOnGitHub]

we should fix this in the next release.
@poutsjr , do you have a fix ready?

[alexeyOnGitHub]

I don't see a fix for this. have to remove from the next release (1.24).

[a11n]

I have investigated the code so far.

I've one question concerning the usage of offset and totalObjectsFoundOnServer in Transport.getObjectsList().

Is the reason behind dividing list queries into chunks of size objectsPerPage performance or stability?

In other words, is there any reason against just one query?

[alexeyOnGitHub]

yes, the simple reason is that Redmine would only give you one page at a time. the number of items on that page is defined in Redmine server settings.

[a11n]

Sorry for my late reply, but I had to get familiar with the Redmine REST API first ;-)

I tested a lot and finally found a working solution. However, I want to discuss some aspects regarding the general interpretation of the getIssues()method with you first.

The questions I stumbled upon was:
"What does an API user expect from a method like getIssues(Map<String, String> pParameters)?"

My interpretation:
Since the documentation exactly states @param pParameters the http parameters key/value pairs to append to the rest api request I would expect that this methods wraps up the Redmine API call and returns the original result directly to me.

This implicates, if Redmine limits the returned number of whatever entities I request, that's totally fine by me, since that's what I'd expect from this method.
If we stick with this interpretation, there would be no big deal for handling "limit" and "offset" since we could pass all params directly to the Transport layer.

The aforementioned problem IMHO exists, because the method does more than it actually promises. It uses certain params (in this case offsetand limit) for control purposes.

So, dealing with this in one method will result into "conditional code" (=if method-parameter has some value, do something, if method-parameter has another value, do something else), which should always be avoided

In summary, what I would suggest is to somehow explicitly split above mentioned purposes into two separated methods:

1. fetch me all the entities (even breaking server configuration boundaries)
2. fetch me the entities which match the criteria specified in "params" (if offset and limit are not defined, how ever, server will use its defaults anyways.)

In conclusion, I see an issue in overloading the "params" parameter by using certain params for control purposes. Let's split it up by keeping the current approach to fetch all (just add a remark in the javadoc) and introduce a new method, which just passes in whatever parameter the API user want to have control over.

What do you thing?

[alexeyOnGitHub]

I will take a look soon (probably tomorrow)...

[alexeyOnGitHub]

I see how this could be misleading. the original API usage was "load all objects at once" and fetching individual pages was never a goal.
you can use setObjectsPerPage() method on RedmineManager, but this only affects the internal page size used for loading issues. clients will only see the final result with all objects anyway.

note: loading all object pages before returning to client is the same for all kinds of objects - Issue, User, Group, Project, ....

[alexeyOnGitHub]

one option here would be modifying getObjectsList() method in Transport class to respect "limit" and "offset" parameters. just need to check how those parameters will interfere with setObjectsPerPage() and how that will affect existing clients.

[alexeyOnGitHub]

how about the solution in pull request #188 ? I do not use that getIssues( map of params) myself, so it is hard to design that part of the API without having real use-cases...

[a11n]

For me the proposed solution in #188 is exactly what I was looking for.

I am now able to control the returned list by myself.

+1 for going ahead and merge the PR :-P

[alexeyOnGitHub]

would you need an additional method that gets a map of parameters, but takes care of paging itself? something like

getIssuesAllPages(Map of http params)

[a11n]

Well, currently not.

But in regards to downwards compatibility it is worth to think about to keep the current method and add the "no paging" version as a new method. This will provide both features without breaking the current API.

So I think it's good to have both in place, but I currently only need the "no paging" one :-)

[alexeyOnGitHub]

Backward compatibility is important, of course. But in this case this method is totally misleading as couple people complained. I believe fixing this bug is more important than maintaining compatibility.

[alexeyOnGitHub]

merged the pull request solving this issue.
BREAKING CHANGE: getIssues(params map) does NOT handle paging anymore, you can/must use your own limit&offset http params.

[atnak]

I'm in the process of updating from 1.25 to 2.6.0 and came across this thread.

Looking at this as an outsider, there now seems to be a glaring gap in the use case where the old getIssues(Map<String, String>) used to be. As it stands, I was initially left wondering if the new "recommended" way of using the API was to somehow use getIssues(String, Integer, Include...). (By dynamically registering a query perhaps.)

Now, I'm wondering if I'm really required to roll out my own getIssuesAllPages() in all my projects, or if I'm just missing something obvious. Although it's not too difficult, the implementation for getIssuesAllPages() doesn't seem[1] that simple that it should be forced on users to reconstruct. (Especially when many just don't want to think about paging.)

Lastly, I'm baffled as to whether or not my implementation is atomic. What happens when records are inserted or deleted from lower offset pages during the loop? Can this happen? I have no idea because I don't know design surrounding offset, limit and what the server-side is doing.

[1]

public List<Issue> getIssuesAllPages(IssueManager manager, Map<String, String> criteria) {
    Map<String, String> params = new LinkedHashMap<>(criteria);
    params.put("limit", String.valueOf(Integer.MAX_VALUE));

    final List<Issue> result = new ArrayList<>();

    for (;;) {
        List<Issue> found = manager.getIssues(params);
        if (found.isEmpty()) {
            return result;
        }
        result.addAll(found);
        params.put("offset", String.valueOf(result.size()));
    }
}