Avoid mutating _filter dict for subsequent calls (#229)

* Avoid mutating `_filter` dict for subsequent calls

* Check type of `_filter['labels']` instead of creating new object

* Unit test for filters with multiple labels

* Unit test fixes

* Avoid mutating config object

* Use deepcopy and test multiple labels for PRs as well

---------

Co-authored-by: Ralph Bean <[email protected]>

sync2jira/upstream_issue.py

@@ -19,6 +19,7 @@
 
 import logging
 from urllib.parse import urlencode
+from copy import deepcopy
 
 import requests
 from github import Github
@@ -247,13 +248,17 @@ def github_issues(upstream, config):
         .get('github', {})\
         .get(upstream, {})
 
-    if 'labels' in _filter:
-        # We have to flatten the labels list to a comma-separated string
-        _filter['labels'] = ','.join(_filter['labels'])
-
     url = 'https://api.github.com/repos/%s/issues' % upstream
     if _filter:
-        url += '?' + urlencode(_filter)
+        labels = _filter.get('labels')
+        if isinstance(labels, list):
+            # We have to flatten the labels list to a comma-separated string,
+            # so make a copy to avoid mutating the config object
+            url_filter = deepcopy(_filter)
+            url_filter['labels'] = ','.join(labels)
+        else:
+            url_filter = _filter  # Use the existing filter, unmodified
+        url += '?' + urlencode(url_filter)
 
     issues = get_all_github_data(url, headers)
 

sync2jira/upstream_pr.py

@@ -18,6 +18,7 @@
 # Authors:  Ralph Bean <[email protected]>
 
 import logging
+from copy import deepcopy
 
 try:
     from urllib.parse import urlencode  # py3
@@ -142,14 +143,18 @@ def github_prs(upstream, config):
         .get('github', {}) \
         .get(upstream, {})
 
-    if 'labels' in _filter:
-        # We have to flatten the labels list to a comma-separated string
-        _filter['labels'] = ','.join(_filter['labels'])
-
     # Build our URL
     url = 'https://api.github.com/repos/%s/pulls' % upstream
     if _filter:
-        url += '?' + urlencode(_filter)
+        labels = _filter.get('labels')
+        if isinstance(labels, list):
+            # We have to flatten the labels list to a comma-separated string,
+            # so make a copy to avoid mutating the config object
+            url_filter = deepcopy(_filter)
+            url_filter['labels'] = ','.join(labels)
+        else:
+            url_filter = _filter  # Use the existing filter, unmodified
+        url += '?' + urlencode(url_filter)
 
     # Get our issues using helper functions
     prs = u_issue.get_all_github_data(url, headers)

tests/test_upstream_issue.py

@@ -1,6 +1,7 @@
 import unittest
 import unittest.mock as mock
 from unittest.mock import MagicMock
+from copy import deepcopy
 
 import sync2jira.upstream_issue as u
 
@@ -396,6 +397,60 @@ def test_github_issues_no_token(self,
         self.mock_github_repo.get_issue.assert_not_called()
         self.mock_github_issue.get_comments.assert_not_called()
 
+    @mock.patch('sync2jira.intermediary.Issue.from_github')
+    @mock.patch(PATH + 'Github')
+    @mock.patch(PATH + 'get_all_github_data')
+    def test_filter_multiple_labels(self,
+                                    mock_get_all_github_data,
+                                    mock_github,
+                                    mock_issue_from_github):
+        """
+        This function tests 'github_issues' function with a filter including multiple labels
+        """
+        # Set up return values
+        self.mock_config['sync2jira']['filters']['github']['org/repo']['labels'].extend(['another_tag', 'and_another'])
+        mock_github.return_value =  self.mock_github_client
+        mock_issue_from_github.return_value = 'Successful Call!'
+        # We mutate the issue object so we need to pass a copy here
+        mock_get_all_github_data.return_value = [deepcopy(self.mock_github_issue_raw)]
+
+        # Call the function
+        list(u.github_issues(
+            upstream='org/repo',
+            config=self.mock_config
+        ))
+
+        # Assert that the labels filter is correct
+        self.assertIn(
+            'labels=custom_tag%2Canother_tag%2Cand_another',
+            mock_get_all_github_data.call_args[0][0]
+        )
+        # Assert the config value was not mutated
+        self.assertEqual(
+            self.mock_config['sync2jira']['filters']['github']['org/repo']['labels'],
+            ['custom_tag','another_tag','and_another']
+        )
+
+        # Restore the return value to the original object
+        mock_get_all_github_data.return_value = [deepcopy(self.mock_github_issue_raw)]
+
+        # Call the function again to ensure consistency for subsequent calls
+        list(u.github_issues(
+            upstream='org/repo',
+            config=self.mock_config
+        ))
+
+        # Assert that the labels filter is correct
+        self.assertIn(
+            'labels=custom_tag%2Canother_tag%2Cand_another',
+            mock_get_all_github_data.call_args[0][0]
+        )
+         # Assert the config value was not mutated
+        self.assertEqual(
+            self.mock_config['sync2jira']['filters']['github']['org/repo']['labels'],
+            ['custom_tag','another_tag','and_another']
+        )
+
     @mock.patch(PATH + 'Github')
     @mock.patch('sync2jira.intermediary.Issue.from_github')
     def test_handle_github_message_not_in_mapped(self,

tests/test_upstream_pr.py

@@ -1,6 +1,7 @@
 import unittest
 import unittest.mock as mock
 from unittest.mock import MagicMock
+from copy import deepcopy
 
 import sync2jira.upstream_pr as u
 
@@ -201,3 +202,57 @@ def test_github_issues(self,
         self.mock_github_repo.get_pull.assert_called_with(number='1234')
         self.mock_github_pr.get_issue_comments.assert_any_call()
         self.assertEqual(response[0], 'Successful Call!')
+
+    @mock.patch('sync2jira.intermediary.PR.from_github')
+    @mock.patch(PATH + 'Github')
+    @mock.patch(PATH + 'u_issue.get_all_github_data')
+    def test_filter_multiple_labels(self,
+                                    mock_get_all_github_data,
+                                    mock_github,
+                                    mock_issue_from_github):
+        """
+        This function tests 'github_issues' function with a filter including multiple labels
+        """
+        # Set up return values
+        self.mock_config['sync2jira']['filters']['github']['org/repo']['labels'].extend(['another_tag', 'and_another'])
+        mock_github.return_value =  self.mock_github_client
+        mock_issue_from_github.return_value = 'Successful Call!'
+        # We mutate the issue object so we need to pass a copy here
+        mock_get_all_github_data.return_value = [deepcopy(self.mock_github_issue_raw)]
+
+        # Call the function
+        list(u.github_prs(
+            upstream='org/repo',
+            config=self.mock_config
+        ))
+
+        # Assert that the labels filter is correct
+        self.assertIn(
+            'labels=custom_tag%2Canother_tag%2Cand_another',
+            mock_get_all_github_data.call_args[0][0]
+        )
+        # Assert the config value was not mutated
+        self.assertEqual(
+            self.mock_config['sync2jira']['filters']['github']['org/repo']['labels'],
+            ['custom_tag','another_tag','and_another']
+        )
+
+        # Restore the return value to the original object
+        mock_get_all_github_data.return_value = [deepcopy(self.mock_github_issue_raw)]
+
+        # Call the function again to ensure consistency for subsequent calls
+        list(u.github_prs(
+            upstream='org/repo',
+            config=self.mock_config
+        ))
+
+        # Assert that the labels filter is correct
+        self.assertIn(
+            'labels=custom_tag%2Canother_tag%2Cand_another',
+            mock_get_all_github_data.call_args[0][0]
+        )
+         # Assert the config value was not mutated
+        self.assertEqual(
+            self.mock_config['sync2jira']['filters']['github']['org/repo']['labels'],
+            ['custom_tag','another_tag','and_another']
+        )