From 70736d8473a56394194afc4f545bbf6b44406183 Mon Sep 17 00:00:00 2001 From: Marco Belladelli Date: Tue, 19 Nov 2024 15:54:08 +0100 Subject: [PATCH] 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 --- sync2jira/upstream_issue.py | 15 ++++++---- sync2jira/upstream_pr.py | 15 ++++++---- tests/test_upstream_issue.py | 55 ++++++++++++++++++++++++++++++++++++ tests/test_upstream_pr.py | 55 ++++++++++++++++++++++++++++++++++++ 4 files changed, 130 insertions(+), 10 deletions(-) diff --git a/sync2jira/upstream_issue.py b/sync2jira/upstream_issue.py index 843354e1..4fd9f9d7 100644 --- a/sync2jira/upstream_issue.py +++ b/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) diff --git a/sync2jira/upstream_pr.py b/sync2jira/upstream_pr.py index c84cdd70..686807f6 100644 --- a/sync2jira/upstream_pr.py +++ b/sync2jira/upstream_pr.py @@ -18,6 +18,7 @@ # Authors: Ralph Bean 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) diff --git a/tests/test_upstream_issue.py b/tests/test_upstream_issue.py index b72d673f..a3c22023 100644 --- a/tests/test_upstream_issue.py +++ b/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, diff --git a/tests/test_upstream_pr.py b/tests/test_upstream_pr.py index 0524a998..c2cff593 100644 --- a/tests/test_upstream_pr.py +++ b/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'] + )