Merge pull request #120 from ServiceNow/scratch/address-link-failure

fix: tests and update

Author: vetsin

docs/user/advanced.rst

@@ -132,3 +132,7 @@ The following can improve performance:
 * Increase (or decrease) the default :ref:`batch_size` for GlideRecord
 * According to `KB0534905 <https://support.servicenow.com/kb_view.do?sysparm_article=KB0534905>`_ try disabling display values if they are not required via `gr.display_value = False`
 * Try setting a query :ref:`limit` if you do not need all results
+
+2. Why am I consuming so much memory?
+
+By default, GlideRecord can be 'rewound' in that it stores all previously fetched records in memory. This can be disabled by setting :ref:`rewind` to False on GlideRecord

poetry.lock

@@ -365,7 +365,14 @@ def _transform_response(self, req: requests.PreparedRequest, serviced_request) -
 
         return response
 
-    def execute(self):
+    def execute(self, attempt=0):
+        if attempt > 2:
+            # just give up and tell em we tried
+            for h in self.__hooks:
+                self.__hooks[h](None)
+            self.__hooks = {}
+            self.__requests = []
+            self.__stored_requests = {}
         bid = self._next_id()
         body = {
             'batch_request_id': bid,
@@ -374,20 +381,18 @@ def execute(self):
         r = self.session.post(self._batch_target(), json=body)
         self._validate_response(r)
         data = r.json()
-        try:
-            assert str(bid) == data['batch_request_id'], f"How did we get a response id different from {bid}"
-
-            for response in data['serviced_requests'] + data['unserviced_requests']:
-                response_id = response['id']
-                assert response_id in self.__hooks, f"Somehow has no hook for {response_id}"
-                assert response_id in self.__stored_requests, f"Somehow we did not store request for {response_id}"
-                self.__hooks[response['id']](self._transform_response(self.__stored_requests[response_id], response))
-
-            return bid
-        finally:
-            self.__stored_requests = {}
-            self.__requests = []
-            self.__hooks = {}
+        assert str(bid) == data['batch_request_id'], f"How did we get a response id different from {bid}"
+
+        for response in data['serviced_requests']:
+            response_id = response['id']
+            assert response_id in self.__hooks, f"Somehow has no hook for {response_id}"
+            assert response_id in self.__stored_requests, f"Somehow we did not store request for {response_id}"
+            self.__hooks[response['id']](self._transform_response(self.__stored_requests.pop(response_id), response))
+            del self.__hooks[response_id]
+            self.__requests = list(filter(lambda x: x['id'] != response_id, self.__requests))
+
+        if len(data['unserviced_requests']) > 0:
+            self.execute(attempt=attempt+1)
 
     def get(self, record: GlideRecord, sys_id: str, hook: Callable) -> None:
         params = self._set_params(record)

pysnc/client.py

@@ -706,7 +706,7 @@ def delete_multiple(self) -> bool:
         allRecordsWereDeleted = True
         def handle(response):
             nonlocal  allRecordsWereDeleted
-            if response.status_code != 204:
+            if response is None or response.status_code != 204:
                 allRecordsWereDeleted = False
 
         for e in self:
@@ -716,12 +716,17 @@ def handle(response):
 
     def update_multiple(self, custom_handler=None) -> bool:
         """
-        Updates multiple records at once
+        Updates multiple records at once. A ``custom_handler`` of the form ``def handle(response: requests.Response | None)`` can be passed in,
+        which may be useful if you wish to handle errors in a specific way. Note that if a custom_handler is used this
+        method will always return ``True``
+
+
+        :return: ``True`` on success, ``False`` if any records failed. If custom_handler is specified, always returns ``True``
         """
         updated = True
         def handle(response):
             nonlocal updated
-            if response.status_code != 200:
+            if response is None or response.status_code != 200:
                 updated = False
 
         for e in self:
@@ -810,7 +815,7 @@ def set_display_value(self, field, value):
 
     def set_link(self, field, value):
         """
-        Set the link for a field.
+        Set the link for a field, it is however preferable to to `gr.field.set_link(value)`.
 
         :param str field: The field
         :param value: The Value
@@ -823,9 +828,9 @@ def set_link(self, field, value):
         else:
             c[field].set_link(value)
 
-    def get_link(self, no_stack=False) -> str:
+    def get_link(self, no_stack: bool=False) -> str:
         """
-        Generate a full URL to the current record. sys_id will be null if there is no current record.
+        Generate a full URL to the current record. sys_id will be -1 if there is no current record.
 
         :param bool no_stack: Default ``False``, adds ``&sysparm_stack=<table>_list.do?sysparm_query=active=true`` to the URL
         :param bool list: Default ``False``, if ``True`` then provide a link to the record set, not the current record
@@ -837,7 +842,8 @@ def get_link(self, no_stack=False) -> str:
         stack = '&sysparm_stack=%s_list.do?sysparm_query=active=true' % self.__table
         if no_stack:
             stack = ''
-        id = self.sys_id if obj else 'null'
+        id = self.sys_id if obj else None
+        id = id or '-1'
         return "{}/{}.do?sys_id={}{}".format(ins, self.__table, id, stack)
 
     def get_link_list(self) -> Optional[str]:
@@ -1052,6 +1058,7 @@ def serialize(self, display_value=False, fields=None, fmt=None, changes_only=Fal
         :param list fields: Fields to serialize. Defaults to all fields.
         :param str fmt: None or ``pandas``. Defaults to None
         :param changes_only: Do we want to serialize only the fields we've modified?
+        :param exclude_reference_link: Do we want to exclude the reference link? default is True
         :return: dict representation
         """
         if fmt == 'pandas':

pysnc/record.py

@@ -30,11 +30,11 @@ def test_link_query(self):
         gr.query()
         link = gr.get_link(no_stack=True)
         print(link)
-        self.assertTrue(link.endswith('sys_user.do?sys_id=null'))
+        self.assertTrue(link.endswith('sys_user.do?sys_id=-1'))
         self.assertTrue(gr.next())
         link = gr.get_link(no_stack=True)
         print(link)
-        self.assertFalse(link.endswith('sys_user.do?sys_id=null'))
+        self.assertFalse(link.endswith('sys_user.do?sys_id=-1'))
         client.session.close()
 
     def test_link_list(self):

test/test_snc_api.py

@@ -254,5 +254,74 @@ def custom_handle(response):
         tgr.delete_multiple()
         client.session.close()
 
+    def test_multi_update_with_failures(self):
+        client = ServiceNowClient(self.c.server, self.c.credentials)
+        br = client.GlideRecord('sys_script')
+
+        # in order to test the failure case, let's insert a BR that will reject specific values
+        br.add_query('name', 'test_multi_update_with_failures')
+        br.query()
+        if not br.next():
+            br.initialize()
+            br.name = 'test_multi_update_with_failures'
+            br.collection = 'problem'
+            br.active = True
+            br.when = 'before'
+            br.order = 100
+            br.action_insert = True
+            br.action_update = True
+            br.abort_action = True
+            br.add_message = True
+            br.message = 'rejected by test_multi_update_with_failures br'
+            br.filter_condition = 'short_descriptionLIKEEICAR^ORdescriptionLIKEEICAR^EQ'
+            br.insert()
+
+
+        gr = client.GlideRecord('problem')
+        gr.add_query('short_description', 'LIKE', 'BUNKZZ')
+        gr.query()
+        self.assertTrue(gr.delete_multiple()) # try to make sure weh ave none first
+        gr.query()
+        self.assertEqual(len(gr), 0, 'should have had none left')
+
 
+        total_count = 10
+        # insert five bunk records
+        # TODO: insert multiple
+        for i in range(total_count):
+            gr = client.GlideRecord('problem')
+            gr.initialize()
+            gr.short_description = f"Unit Test - BUNKZZ Multi update {i}"
+            assert gr.insert(), "Failed to insert a record"
+
+        gr = client.GlideRecord('problem')
+        gr.add_query('short_description', 'LIKE', 'BUNKZZ')
+        gr.query()
+        self.assertEqual(len(gr), total_count)
+
+        i = 0
+        # half append
+        print(f"for i < {total_count//2}")
+        while i < (total_count//2) and gr.next():
+            gr.short_description = gr.short_description + ' -- APPENDEDZZ'
+            i += 1
+        # half error
+        while gr.next():
+            gr.short_description = gr.short_description + ' -- EICAR'
+
+        self.assertFalse(gr.update_multiple())
+        # make sure we cleaned up as expected
+        self.assertEqual(gr._client.batch_api._BatchAPI__hooks, {})
+        self.assertEqual(gr._client.batch_api._BatchAPI__stored_requests, {})
+        self.assertEqual(gr._client.batch_api._BatchAPI__requests, [])
+
+        tgr = client.GlideRecord('problem')
+        tgr.add_query('short_description', 'LIKE', 'APPENDEDZZ')
+        tgr.query()
+        self.assertEqual(len(tgr), total_count//2)
+
+        tgr = client.GlideRecord('problem')
+        tgr.add_query('short_description', 'LIKE', 'EICAR')
+        tgr.query()
+        self.assertEqual(len(tgr), 0)
 

test/test_snc_api_write.py

@@ -168,9 +168,13 @@ def test_serialize_reference_link(self):
         data = gr.serialize(exclude_reference_link=False)
         self.assertIsNotNone(data)
         self.assertEqual(gr.get_value('reffield'), 'my reference')
-        self.assertEqual(gr.get_link('reffield'), 'https://dev00000.service-now.com/api/now/table/sys___/abcde12345')
+        self.assertTrue(gr.get_link(True).endswith('/some_table.do?sys_id=-1'), f"was {gr.get_link()}")
+        self.assertEqual(gr.reffield.get_link(), 'https://dev00000.service-now.com/api/now/table/sys___/abcde12345')
         self.assertEqual(gr.serialize(exclude_reference_link=False), {'reffield':{'value': 'my reference','link':'https://dev00000.service-now.com/api/now/table/sys___/abcde12345'}})
         self.assertEqual(data, {'reffield':{'value': 'my reference','link':'https://dev00000.service-now.com/api/now/table/sys___/abcde12345'}})
+
+        gr.reffield.set_link('https://dev00000.service-now.com/api/now/table/sys___/xyz789')
+        self.assertEqual(gr.reffield.get_link(), 'https://dev00000.service-now.com/api/now/table/sys___/xyz789')
         client.session.close()
 
     def test_serialize_reference_link_all(self):
@@ -181,12 +185,23 @@ def test_serialize_reference_link_all(self):
         gr.set_link('reffield', 'https://dev00000.service-now.com/api/now/table/sys___/abcde12345')
         gr.set_display_value('reffield', 'my reference display')
 
-        data = gr.serialize(exclude_reference_link=False)
-        self.assertIsNotNone(data)
         self.assertEqual(gr.get_value('reffield'), 'my reference')
-        self.assertEqual(gr.get_link('reffield'), 'https://dev00000.service-now.com/api/now/table/sys___/abcde12345')
-        self.assertEqual(gr.serialize(exclude_reference_link=False), {'reffield':{'value': 'my reference','display_value': 'my reference display', 'link':'https://dev00000.service-now.com/api/now/table/sys___/abcde12345'}})
-        self.assertEqual(data, {'reffield':{'value': 'my reference','display_value': 'my reference display', 'link':'https://dev00000.service-now.com/api/now/table/sys___/abcde12345'}})
+        self.assertEqual(gr.get_display_value('reffield'), 'my reference display')
+        self.assertEqual(gr.reffield.get_link(), 'https://dev00000.service-now.com/api/now/table/sys___/abcde12345')
+
+        self.assertEqual(gr.serialize(), {'reffield':'my reference'})
+        self.assertEqual(
+            gr.serialize(exclude_reference_link=False),
+            {'reffield':{'value': 'my reference','link':'https://dev00000.service-now.com/api/now/table/sys___/abcde12345'}}
+        )
+        self.assertEqual(
+                gr.serialize(display_value=True, exclude_reference_link=False),
+            {'reffield':{'display_value': 'my reference display', 'link':'https://dev00000.service-now.com/api/now/table/sys___/abcde12345'}}
+        )
+        self.assertEqual(
+                gr.serialize(display_value='both', exclude_reference_link=False),
+                {'reffield':{'value': 'my reference','display_value': 'my reference display', 'link':'https://dev00000.service-now.com/api/now/table/sys___/abcde12345'}}
+        )
         client.session.close()
 
     def test_str(self):