DS-3543 shows pagination bug with withdrawn items

[abollini]

This PR provides the alternative fix agreed in #2120

The first commit extends the current IT about items pagination to show the bug
https://travis-ci.org/4Science/DSpace/builds/524414847
the second one is the fix

[KevinVdV]

The total from the "findAll()" rest call can still differ from the from the amount of items that are actually returned.

The query that we execute to retrieve all items right now is:

SELECT uuid FROM item WHERE in_archive=true OR withdrawn=true; (OK)

But the query to retrieve the total count is

SELECT COUNT(*) FROM item;

When there are items in the workflow / workspace the total count will be higher. I think we need a countTotalUnfiltered() method.

[abollini]

good catch @KevinVdV , thanks to check that. I will fix that change the IT to identify this issue (it should be sufficient to add some workspaceitem to the test env). I will ping you once done

[abollini]

@KevinVdV after further consideration, for consistency reason, I think we should introduce an additional findAllUnfilteredAndInprogress method that really return all the items that should be exposed over the REST endpoint. The inprogress items indeed are exposed on the items endpoint if you know the uuid (and linked from the workspaceitem / workflowitem).
Probably, as I can see usual use cases where we are only interested to "final" items, we should add support for some search methods for them. Do you agree?

[benbosman]

@abollini I would assume that if e.g. /rest/api/core/items/517f13ab-3105-42d3-bf55-271876fb726e exists, it would be exposed in /rest/api/core/items even if it's in the submission. I wouldn't see a reason for GET /rest/api/core/items to filter items even they can be requested when entering the exact ID.

On the other hand, the GET /rest/api/core/items should perhaps not even exist at all. It's an unsorted list of all items, and it has risks of being incorrectly used (e.g. for harvesting the repository), but no good use cases. It also can't be safely used to iterate all items because it will change if items are created/deleted while paging. I think the best use case for it is currently the ITs, but they can be replaced with direct calls to the ItemService.

I must admit that these methods can also get quite confusing when we have:

• findAll: which is in fact not all, but only archived
• findAllUnfiltered: which is in fact still filtered, but includes archived or withdrawn
• countTotal: which is the real total, and makes sense
• countNotArchivedItems: which omits the withdrawn items as well

Although a cleanup of all these names is out of scope for this PR, I would prefer to either:

• Make sure GET /rest/api/core/items contains all items (there's no service for this yet)
• Remove GET /rest/api/core/items (but that would probably require quite some work)

[abollini]

thanks @benbosman for your feedback, I will proceed with the option

Make sure GET /rest/api/core/items contains all items (there's no service for this yet)

The items endpoint is just for administrators and it is convenient for the Integration Tests for sure as "double check" maybe there are maintenance use case that we don't see yet. I will also create a PR to update the contract to make this explicit

[dimitrispie]

thanks @benbosman for your feedback, I will proceed with the option

Make sure GET /rest/api/core/items contains all items (there's no service for this yet)

The items endpoint is just for administrators and it is convenient for the Integration Tests for sure as "double check" maybe there are maintenance use case that we don't see yet. I will also create a PR to update the contract to make this explicit

Have you considered the performance of such request, i.e. returning all items?

[tdonohue]

This may have been resolved (in a different manner) by #2580, as withdrawn & private items are no longer returned in default searches/counts...they are now a separate search configuration which is only accessible to Admins.