Repository: EdinburghGenomics/pyclarity-lims

Issue:
Lims _get_instances() returns empty array when start_index is set.
An empty array is always returned when retrieving a list of entities using the `get_*` methods of the`Lims` class. For example:

```
samples = l.get_samples(start_index=500)
# samples == []
```

The problem is in the [_get_instances()](https://github.com/EdinburghGenomics/pyclarity-lims/blob/master/pyclarity_lims/lims.py#L563-L586) method. The response is only parsed when `start_index` is `None`.  

Discussion:

Patch:
diff --git a/pyclarity_lims/lims.py b/pyclarity_lims/lims.py
index c00b1a1..532b315 100644
--- a/pyclarity_lims/lims.py
+++ b/pyclarity_lims/lims.py
@@ -210,7 +210,8 @@ class Lims(object):
         root = ElementTree.fromstring(response.content)
         return root
 
-    def get_udfs(self, name=None, attach_to_name=None, attach_to_category=None, start_index=None, add_info=False):
+    def get_udfs(self, name=None, attach_to_name=None, attach_to_category=None, start_index=None, nb_pages=-1,
+                 add_info=False):
         """Get a list of udfs, filtered by keyword arguments.
 
         :param name: name of udf
@@ -218,7 +219,9 @@ class Lims(object):
             Sample, Project, Container, or the name of a process.
         :param attach_to_category: If 'attach_to_name' is the name of a process, such as 'CaliperGX QC (DNA)',
                                    then you need to set attach_to_category='ProcessType'. Must not be provided otherwise.
-        :param start_index: Page to retrieve; all if None.
+        :param start_index: first element to retrieve; start at first element if None.
+        :param nb_pages: number of page to iterate over. The page size is 500 by default unless configured otherwise
+                        in your LIMS. 0 or negative numbers returns all pages.
         :param add_info: Change the return type to a tuple where the first element is normal return and
                          the second is a dict of additional information provided in the query.
         """
@@ -226,21 +229,23 @@ class Lims(object):
                                   attach_to_name=attach_to_name,
                                   attach_to_category=attach_to_category,
                                   start_index=start_index)
-        return self._get_instances(Udfconfig, add_info=add_info, params=params)
+        return self._get_instances(Udfconfig, add_info=add_info, nb_pages=nb_pages, params=params)
 
-    def get_reagent_types(self, name=None, start_index=None):
+    def get_reagent_types(self, name=None, start_index=None, nb_pages=-1):
         """
         Get a list of reagent types, filtered by keyword arguments.
 
         :param name: Reagent type name, or list of names.
-        :param start_index: Page to retrieve; all if None.
+        :param start_index: first element to retrieve; start at first element if None.
+        :param nb_pages: number of page to iterate over. The page size is 500 by default unless configured otherwise
+                        in your LIMS. 0 or negative numbers returns all pages.
         """
         params = self._get_params(name=name,
                                   start_index=start_index)
-        return self._get_instances(ReagentType, params=params)
+        return self._get_instances(ReagentType, nb_pages=nb_pages, params=params)
 
     def get_labs(self, name=None, last_modified=None,
-                 udf=dict(), udtname=None, udt=dict(), start_index=None, add_info=False):
+                 udf=dict(), udtname=None, udt=dict(), start_index=None, nb_pages=-1, add_info=False):
         """Get a list of labs, filtered by keyword arguments.
 
         :param name: Lab name, or list of names.
@@ -249,7 +254,9 @@ class Lims(object):
         :param udtname: UDT name, or list of names.
         :param udt: dictionary of UDT UDFs with 'UDTNAME.UDFNAME[OPERATOR]' as keys
                     and a string or list of strings as value.
-        :param start_index: Page to retrieve; all if None.
+        :param start_index: first element to retrieve; start at first element if None.
+        :param nb_pages: number of page to iterate over. The page size is 500 by default unless configured otherwise
+                        in your LIMS. 0 or negative numbers returns all pages.
         :param add_info: Change the return type to a tuple where the first element is normal return and
                          the second is a dict of additional information provided in the query.
         """
@@ -257,11 +264,11 @@ class Lims(object):
                                   last_modified=last_modified,
                                   start_index=start_index)
         params.update(self._get_params_udf(udf=udf, udtname=udtname, udt=udt))
-        return self._get_instances(Lab, add_info=add_info, params=params)
+        return self._get_instances(Lab, add_info=add_info, nb_pages=nb_pages, params=params)
 
     def get_researchers(self, firstname=None, lastname=None, username=None,
                         last_modified=None,
-                        udf=dict(), udtname=None, udt=dict(), start_index=None,
+                        udf=dict(), udtname=None, udt=dict(), start_index=None, nb_pages=-1,
                         add_info=False):
         """Get a list of researchers, filtered by keyword arguments.
 
@@ -273,7 +280,9 @@ class Lims(object):
         :param udtname: UDT name, or list of names.
         :param udt: dictionary of UDT UDFs with 'UDTNAME.UDFNAME[OPERATOR]' as keys
                     and a string or list of strings as value.
-        :param start_index: Page to retrieve; all if None.
+        :param start_index: first element to retrieve; start at first element if None.
+        :param nb_pages: number of page to iterate over. The page size is 500 by default unless configured otherwise
+                        in your LIMS. 0 or negative numbers returns all pages.
         :param add_info: Change the return type to a tuple where the first element is normal return and
                          the second is a dict of additional information provided in the query.
 
@@ -284,10 +293,10 @@ class Lims(object):
                                   last_modified=last_modified,
                                   start_index=start_index)
         params.update(self._get_params_udf(udf=udf, udtname=udtname, udt=udt))
-        return self._get_instances(Researcher, add_info=add_info, params=params)
+        return self._get_instances(Researcher, add_info=add_info, nb_pages=nb_pages, params=params)
 
     def get_projects(self, name=None, open_date=None, last_modified=None,
-                     udf=dict(), udtname=None, udt=dict(), start_index=None,
+                     udf=dict(), udtname=None, udt=dict(), start_index=None, nb_pages=-1,
                      add_info=False):
         """Get a list of projects, filtered by keyword arguments.
 
@@ -298,7 +307,9 @@ class Lims(object):
         :param udtname: UDT name, or list of names.
         :param udt: dictionary of UDT UDFs with 'UDTNAME.UDFNAME[OPERATOR]' as keys
                     and a string or list of strings as value.
-        :param start_index: Page to retrieve; all if None.
+        :param start_index: first element to retrieve; start at first element if None.
+        :param nb_pages: number of page to iterate over. The page size is 500 by default unless configured otherwise
+                        in your LIMS. 0 or negative numbers returns all pages.
         :param add_info: Change the return type to a tuple where the first element is normal return and
                          the second is a dict of additional information provided in the query.
 
@@ -308,14 +319,16 @@ class Lims(object):
                                   last_modified=last_modified,
                                   start_index=start_index)
         params.update(self._get_params_udf(udf=udf, udtname=udtname, udt=udt))
-        return self._get_instances(Project, add_info=add_info, params=params)
+        return self._get_instances(Project, add_info=add_info, nb_pages=nb_pages, params=params)
 
     def get_sample_number(self, name=None, projectname=None, projectlimsid=None,
-                          udf=dict(), udtname=None, udt=dict(), start_index=None):
+                          udf=dict(), udtname=None, udt=dict(), start_index=None, nb_pages=-1):
         """
         Gets the number of samples matching the query without fetching every
         sample, so it should be faster than len(get_samples())
         """
+        # TODO: I doubt that this make any difference in terms of speed since the only thing it save is the Sample
+        # construction. We should test and a replace with len(get_samples())
         params = self._get_params(name=name,
                                   projectname=projectname,
                                   projectlimsid=projectlimsid,
@@ -331,7 +344,7 @@ class Lims(object):
         return total
 
     def get_samples(self, name=None, projectname=None, projectlimsid=None,
-                    udf=dict(), udtname=None, udt=dict(), start_index=None):
+                    udf=dict(), udtname=None, udt=dict(), start_index=None, nb_pages=-1):
         """Get a list of samples, filtered by keyword arguments.
 
         :param name: Sample name, or list of names.
@@ -341,21 +354,22 @@ class Lims(object):
         :param udtname: UDT name, or list of names.
         :param udt: dictionary of UDT UDFs with 'UDTNAME.UDFNAME[OPERATOR]' as keys
                     and a string or list of strings as value.
-        :param start_index: Page to retrieve; all if None.
-
+        :param start_index: first element to retrieve; start at first element if None.
+        :param nb_pages: number of page to iterate over. The page size is 500 by default unless configured otherwise
+                        in your LIMS. 0 or negative numbers returns all pages.
         """
         params = self._get_params(name=name,
                                   projectname=projectname,
                                   projectlimsid=projectlimsid,
                                   start_index=start_index)
         params.update(self._get_params_udf(udf=udf, udtname=udtname, udt=udt))
-        return self._get_instances(Sample, params=params)
+        return self._get_instances(Sample, nb_pages=nb_pages, params=params)
 
     def get_artifacts(self, name=None, type=None, process_type=None,
                       artifact_flag_name=None, working_flag=None, qc_flag=None,
                       sample_name=None, samplelimsid=None, artifactgroup=None, containername=None,
                       containerlimsid=None, reagent_label=None,
-                      udf=dict(), udtname=None, udt=dict(), start_index=None,
+                      udf=dict(), udtname=None, udt=dict(), start_index=None, nb_pages=-1,
                       resolve=False):
         """Get a list of artifacts, filtered by keyword arguments.
 
@@ -375,9 +389,10 @@ class Lims(object):
         :param udtname: UDT name, or list of names.
         :param udt: dictionary of UDT UDFs with 'UDTNAME.UDFNAME[OPERATOR]' as keys
                     and a string or list of strings as value.
-        :param start_index: Page to retrieve; all if None.
+        :param start_index: first element to retrieve; start at first element if None.
+        :param nb_pages: number of page to iterate over. The page size is 500 by default unless configured otherwise
+                        in your LIMS. 0 or negative numbers returns all pages.
         :param resolve: Send a batch query to the lims to get the content of all artifacts retrieved
-
         """
         params = self._get_params(name=name,
                                   type=type,
@@ -394,13 +409,13 @@ class Lims(object):
                                   start_index=start_index)
         params.update(self._get_params_udf(udf=udf, udtname=udtname, udt=udt))
         if resolve:
-            return self.get_batch(self._get_instances(Artifact, params=params))
+            return self.get_batch(self._get_instances(Artifact, nb_pages=nb_pages, params=params))
         else:
-            return self._get_instances(Artifact, params=params)
+            return self._get_instances(Artifact, nb_pages=nb_pages, params=params)
 
     def get_containers(self, name=None, type=None,
                        state=None, last_modified=None,
-                       udf=dict(), udtname=None, udt=dict(), start_index=None,
+                       udf=dict(), udtname=None, udt=dict(), start_index=None, nb_pages=-1,
                        add_info=False):
         """Get a list of containers, filtered by keyword arguments.
 
@@ -412,10 +427,11 @@ class Lims(object):
         :param udtname: UDT name, or list of names.
         :param udt: dictionary of UDT UDFs with 'UDTNAME.UDFNAME[OPERATOR]' as keys
                     and a string or list of strings as value.
-        :param start_index: Page to retrieve; all if None.
+        :param start_index: first element to retrieve; start at first element if None.
+        :param nb_pages: number of page to iterate over. The page size is 500 by default unless configured otherwise
+                        in your LIMS. 0 or negative numbers returns all pages.
         :param add_info: Change the return type to a tuple where the first element is normal return and
                          the second is a dict of additional information provided in the query.
-
         """
         params = self._get_params(name=name,
                                   type=type,
@@ -423,24 +439,25 @@ class Lims(object):
                                   last_modified=last_modified,
                                   start_index=start_index)
         params.update(self._get_params_udf(udf=udf, udtname=udtname, udt=udt))
-        return self._get_instances(Container, add_info=add_info, params=params)
+        return self._get_instances(Container, add_info=add_info, nb_pages=nb_pages, params=params)
 
-    def get_container_types(self, name=None, start_index=None, add_info=False):
+    def get_container_types(self, name=None, start_index=None, nb_pages=-1, add_info=False):
         """Get a list of container types, filtered by keyword arguments.
 
         :param name: name of the container type or list of names.
-        :param start_index: Page to retrieve; all if None.
+        :param start_index: first element to retrieve; start at first element if None.
+        :param nb_pages: number of page to iterate over. The page size is 500 by default unless configured otherwise
+                        in your LIMS. 0 or negative numbers returns all pages.
         :param add_info: Change the return type to a tuple where the first element is normal return and
                          the second is a dict of additional information provided in the query.
-
         """
         params = self._get_params(name=name, start_index=start_index)
-        return self._get_instances(Containertype, add_info=add_info, params=params)
+        return self._get_instances(Containertype, add_info=add_info, nb_pages=nb_pages, params=params)
 
     def get_processes(self, last_modified=None, type=None,
                       inputartifactlimsid=None,
                       techfirstname=None, techlastname=None, projectname=None,
-                      udf=dict(), udtname=None, udt=dict(), start_index=None):
+                      udf=dict(), udtname=None, udt=dict(), start_index=None, nb_pages=-1):
         """Get a list of processes, filtered by keyword arguments.
 
         :param last_modified: Since the given ISO format datetime.
@@ -453,7 +470,9 @@ class Lims(object):
         :param techfirstname: First name of researcher, or list of.
         :param techlastname: Last name of researcher, or list of.
         :param projectname: Name of project, or list of.
-        :param start_index: Page to retrieve; all if None.
+        :param start_index: first element to retrieve; start at first element if None.
+        :param nb_pages: number of page to iterate over. The page size is 500 by default unless configured otherwise
+                        in your LIMS. 0 or negative numbers returns all pages.
         """
         params = self._get_params(last_modified=last_modified,
                                   type=type,
@@ -463,7 +482,7 @@ class Lims(object):
                                   projectname=projectname,
                                   start_index=start_index)
         params.update(self._get_params_udf(udf=udf, udtname=udtname, udt=udt))
-        return self._get_instances(Process, params=params)
+        return self._get_instances(Process, nb_pages=nb_pages, params=params)
 
     def get_workflows(self, name=None, add_info=False):
         """
@@ -513,32 +532,35 @@ class Lims(object):
         params = self._get_params(name=name)
         return self._get_instances(Protocol, add_info=add_info, params=params)
 
-    def get_reagent_kits(self, name=None, start_index=None, add_info=False):
+    def get_reagent_kits(self, name=None, start_index=None, nb_pages=-1, add_info=False):
         """Get a list of reagent kits, filtered by keyword arguments.
 
         :param name: reagent kit  name, or list of names.
-        :param start_index: Page to retrieve; all if None.
+        :param start_index: first element to retrieve; start at first element if None.
+        :param nb_pages: number of page to iterate over. The page size is 500 by default unless configured otherwise
+                        in your LIMS. 0 or negative numbers returns all pages.
         :param add_info: Change the return type to a tuple where the first element is normal return and
                          the second is a dict of additional information provided in the query.
 
         """
         params = self._get_params(name=name,
                                   start_index=start_index)
-        return self._get_instances(ReagentKit, add_info=add_info, params=params)
+        return self._get_instances(ReagentKit, add_info=add_info, nb_pages=nb_pages, params=params)
 
     def get_reagent_lots(self, name=None, kitname=None, number=None,
-                         start_index=None):
+                         start_index=None, nb_pages=-1):
         """Get a list of reagent lots, filtered by keyword arguments.
 
         :param name: reagent kit  name, or list of names.
         :param kitname: name of the kit this lots belong to
         :param number: lot number or list of lot number
-        :param start_index: Page to retrieve; all if None.
-
+        :param start_index: first element to retrieve; start at first element if None.
+        :param nb_pages: number of page to iterate over. The page size is 500 by default unless configured otherwise
+                        in your LIMS. 0 or negative numbers returns all pages.
         """
         params = self._get_params(name=name, kitname=kitname, number=number,
                                   start_index=start_index)
-        return self._get_instances(ReagentLot, params=params)
+        return self._get_instances(ReagentLot, nb_pages=nb_pages, params=params)
 
     def _get_params(self, **kwargs):
         """Convert keyword arguments to a kwargs dictionary."""
@@ -560,14 +582,15 @@ class Lims(object):
             result["udt.%s" % key] = value
         return result
 
-    def _get_instances(self, klass, add_info=None, params=dict()):
+    def _get_instances(self, klass, add_info=None, nb_pages=-1, params=dict()):
         results = []
         additionnal_info_dicts = []
         tag = klass._TAG
         if tag is None:
             tag = klass.__name__.lower()
         root = self.get(self.get_uri(klass._URI), params=params)
-        while params.get('start-index') is None:  # Loop over all pages.
+        while root:  # Loop over all requested pages.
+            nb_pages -= 1
             for node in root.findall(tag):
                 results.append(klass(self, uri=node.attrib['uri']))
                 info_dict = {}
@@ -577,9 +600,10 @@ class Lims(object):
                     info_dict[subnode.tag] = subnode.text
                 additionnal_info_dicts.append(info_dict)
             node = root.find('next-page')
-            if node is None:
-                break
-            root = self.get(node.attrib['uri'], params=params)
+            if node is None or nb_pages == 0:
+                root = None
+            else:
+                root = self.get(node.attrib['uri'], params=params)
         if add_info:
             return results, additionnal_info_dicts
         else:


Test patch:
diff --git a/tests/test_lims.py b/tests/test_lims.py
index 58fe1a7..0bd29c5 100644
--- a/tests/test_lims.py
+++ b/tests/test_lims.py
@@ -1,5 +1,7 @@
 from unittest import TestCase
 from requests.exceptions import HTTPError
+
+from pyclarity_lims.entities import Sample
 from pyclarity_lims.lims import Lims
 try:
     callable(1)
@@ -143,3 +145,63 @@ class TestLims(TestCase):
         assert lims.get_file_contents(id='an_id', encoding='utf-16', crlf=True) == 'some data\n'
         assert lims.request_session.get.return_value.encoding == 'utf-16'
         lims.request_session.get.assert_called_with(exp_url, auth=(self.username, self.password), timeout=16)
+
+    def test_get_instances(self):
+        lims = Lims(self.url, username=self.username, password=self.password)
+        sample_xml_template = """<?xml version="1.0" encoding="UTF-8" standalone="yes"?>
+        <smp:samples xmlns:smp="http://pyclarity_lims.com/ri/sample">
+            <sample uri="{url}/api/v2/samples/{s1}" limsid="{s1}"/>
+            <sample uri="{url}/api/v2/samples/{s2}" limsid="{s2}"/>
+            {next_page}
+        </smp:samples>
+        """
+        sample_xml1 = sample_xml_template.format(
+            s1='sample1', s2='sample2',
+            url=self.url,
+            next_page='<next-page uri="{url}/api/v2/samples?start-index=3"/>'.format(url=self.url)
+        )
+        sample_xml2 = sample_xml_template.format(
+            s1='sample3', s2='sample4',
+            url=self.url,
+            next_page='<next-page uri="{url}/api/v2/samples?start-index=5"/>'.format(url=self.url)
+        )
+        sample_xml3 = sample_xml_template.format(
+            s1='sample5', s2='sample6',
+            url=self.url,
+            next_page=''
+        )
+        get_returns = [
+            Mock(content=sample_xml1, status_code=200),
+            Mock(content=sample_xml2, status_code=200),
+            Mock(content=sample_xml3, status_code=200)
+        ]
+
+        with patch('requests.Session.get', side_effect=get_returns) as mget:
+            samples = lims._get_instances(Sample, nb_pages=2, params={'projectname': 'p1'})
+            assert len(samples) == 4
+            assert mget.call_count == 2
+            mget.assert_any_call(
+                'http://testgenologics.com:4040/api/v2/samples',
+                auth=('test', 'password'),
+                headers={'accept': 'application/xml'},
+                params={'projectname': 'p1'},
+                timeout=16
+            )
+            mget.assert_called_with(
+                'http://testgenologics.com:4040/api/v2/samples?start-index=3',
+                auth=('test', 'password'),
+                headers={'accept': 'application/xml'},
+                params={'projectname': 'p1'},
+                timeout=16
+            )
+
+        with patch('requests.Session.get', side_effect=get_returns) as mget:
+            samples = lims._get_instances(Sample, nb_pages=0)
+            assert len(samples) == 6
+            assert mget.call_count == 3
+
+        with patch('requests.Session.get', side_effect=get_returns) as mget:
+            samples = lims._get_instances(Sample, nb_pages=-1)
+            assert len(samples) == 6
+            assert mget.call_count == 3
+