diff --git a/release_notes_generator/data/miner.py b/release_notes_generator/data/miner.py index 8dccd4c6..79f1e1e1 100644 --- a/release_notes_generator/data/miner.py +++ b/release_notes_generator/data/miner.py @@ -26,7 +26,7 @@ from typing import Optional, Callable import semver -from github import Github, GithubException +from github import Github, GithubException, UnknownObjectException from github.GitRelease import GitRelease from github.Issue import Issue from github.PullRequest import PullRequest @@ -41,10 +41,13 @@ from release_notes_generator.model.record.pull_request_record import PullRequestRecord from release_notes_generator.utils.decorators import safe_call_decorator from release_notes_generator.utils.github_rate_limiter import GithubRateLimiter -from release_notes_generator.utils.record_utils import get_id, parse_issue_id +from release_notes_generator.utils.record_utils import get_id, parse_issue_id, get_commit_subject -_PR_NUMBER_RE = re.compile(r"\(#(\d+)\)|Merge pull request #(\d+)") +# GitHub PR references: merge/squash markers and bare #N from rebase-merge commits +_PR_MERGE_ARTIFACT_RE = re.compile(r"\(#(\d+)\)|Merge pull request #(\d+)") +_PR_NUMBER_RE = re.compile(r"\(#(\d+)\)|Merge pull request #(\d+)|^#(\d+)\b") _COMPARE_COMMITS_MAX_RESULTS = 10_000 +_MAX_DIRECT_COMMIT_PR_LOOKUPS = 200 # Prevent API call explosion from large commit batches logger = logging.getLogger(__name__) @@ -56,10 +59,7 @@ class DataMiner: def __init__(self, github_instance: Github, rate_limiter: GithubRateLimiter): self.github_instance = github_instance - # dev note: PyGithub paginated results are lazy - the HTTP requests happen while iterating, - # not on the initial call. Call sites that need a list must materialize it (e.g. via - # `self._safe_call(lambda: list(x.get_foo()))()`) inside the safe-call, otherwise pagination - # errors are raised outside of it and go uncaught. + # Lazy pagination errors must be caught inside safe_call, not propagated self._safe_call = safe_call_decorator(rate_limiter) def mine_data(self) -> MinedData: @@ -147,37 +147,74 @@ def _handle_compare_mode(self, repo: Repository, data: MinedData) -> None: data.compare_commit_shas = {c.sha for c in compare_commits} data.commits = {c: data.home_repository for c in compare_commits} pr_numbers = self._extract_pr_numbers_from_commits(compare_commits) + logger.debug("Compare mode: PR number(s) extracted from commit subjects: %s", sorted(pr_numbers)) pulls: dict[PullRequest, Repository] = {} pr_commit_shas: set[str] = set() for number in sorted(pr_numbers): - pr = self._safe_call(repo.get_pull)(number) - if pr is not None: - # Store each PR with its source repository for downstream filtering and processing. - # In compare mode, all PRs come from home_repository; cross-repo is handled elsewhere. - pulls[pr] = data.home_repository - # dev note: pull.get_commits() returns all commits GitHub associates with the PR, - # including sync-merge commits (base branch merged back into the PR branch) whose - # messages don't match _PR_NUMBER_RE. Excluding them by SHA (rather than by message - # pattern) prevents them being misclassified as stand-alone "direct commits". - # merge_commit_sha is added separately: for a rebase-merge, get_commits() still - # reports the pre-rebase SHAs, not the new SHA(s) landed on the base branch. - pr_commits = self._safe_call(lambda p=pr: list(p.get_commits()))() - if pr_commits is not None: - pr_commit_shas.update(c.sha for c in pr_commits) - if pr.merge_commit_sha: - pr_commit_shas.add(pr.merge_commit_sha) - data.pull_requests = pulls + pr = self._safe_call(lambda n=number: self._get_pull_ignoring_not_found(repo, n))() + if pr is None: + logger.debug("Compare mode: PR #%d could not be fetched; skipping.", number) + continue + self._register_pr_commit_shas(pr, pulls, pr_commit_shas, data.home_repository) # Only include commits that aren't already accounted for by a PR # (commits identified by PR, or belonging to a PR's commit list, are redundant with the PR itself) commits_without_pr: dict[GithubCommit, Repository] = {} for commit in compare_commits: + subject = get_commit_subject(commit) if commit.sha in pr_commit_shas: + logger.debug("Compare mode: commit %s ('%s') excluded, matched PR commit SHA.", commit.sha, subject) + continue + has_pr_ref = bool(_PR_MERGE_ARTIFACT_RE.search(subject)) + if has_pr_ref: + logger.debug("Compare mode: commit %s ('%s') excluded, subject references a PR.", commit.sha, subject) continue - subject = commit.commit.message.splitlines()[0] if commit.commit.message else "" - has_pr_ref = bool(_PR_NUMBER_RE.search(subject)) - if not has_pr_ref: - commits_without_pr[commit] = data.home_repository + commits_without_pr[commit] = data.home_repository + + # Fallback: query GitHub's commit→PR association for candidates without explicit PR refs + if len(commits_without_pr) > _MAX_DIRECT_COMMIT_PR_LOOKUPS: + logger.debug( + "Compare mode: %d commit(s) without a detected PR exceed the fallback lookup cap of %d; " + "skipping commit -> PR association fallback, some may still belong to a PR.", + len(commits_without_pr), + _MAX_DIRECT_COMMIT_PR_LOOKUPS, + ) + else: + registered_pr_numbers = {p.number for p in pulls} + for commit in list(commits_without_pr): + associated_prs = self._safe_call(lambda c=commit: list(c.get_pulls()))() + for pr in associated_prs or []: + # Simple PR objects need lazy-loading via _safe_call to access `merged` attribute + is_merged = self._safe_call(lambda p=pr: p.merged)() + if not is_merged or pr.number in registered_pr_numbers: + continue + logger.debug( + "Compare mode: commit %s is associated with merged PR #%d not found via commit subjects.", + commit.sha, + pr.number, + ) + self._register_pr_commit_shas(pr, pulls, pr_commit_shas, data.home_repository) + registered_pr_numbers.add(pr.number) + if commit.sha in pr_commit_shas: + subject = get_commit_subject(commit) + logger.debug( + "Compare mode: commit %s ('%s') excluded, matched PR commit SHA via association fallback.", + commit.sha, + subject, + ) + del commits_without_pr[commit] + + for commit in commits_without_pr: + subject = get_commit_subject(commit) + logger.debug("Compare mode: commit %s ('%s') classified as direct commit.", commit.sha, subject) + + data.pull_requests = pulls + sorted_pr_commit_shas = sorted(pr_commit_shas) + logger.debug( + "Compare mode: total %d unique PR-associated commit SHA(s): %s", + len(sorted_pr_commit_shas), + sorted_pr_commit_shas, + ) data.commits = commits_without_pr logger.info( @@ -186,6 +223,50 @@ def _handle_compare_mode(self, repo: Repository, data: MinedData) -> None: len(data.pull_requests), ) + def _register_pr_commit_shas( + self, + pr: PullRequest, + pulls: dict[PullRequest, Repository], + pr_commit_shas: set[str], + home_repository: Repository, + ) -> None: + """ + Store `pr` alongside its home repository and add all commit SHAs GitHub associates with it. + + Notes: + - Uses `get_commits()` plus `merge_commit_sha` to cover both regular and rebase-merge SHAs. + - Sync-merge commits (base branch merged back into PR branch) are captured by SHA, + preventing misclassification as "direct commits". + - For rebase-merge, `get_commits()` returns pre-rebase SHAs; `merge_commit_sha` has the + new SHA(s) that landed on the base branch. + - In compare mode, all PRs come from home_repository; cross-repo handled elsewhere. + """ + pulls[pr] = home_repository + pr_commits = self._safe_call(lambda p=pr: list(p.get_commits()))() + pr_commit_sha_list = [c.sha for c in pr_commits] if pr_commits is not None else [] + pr_commit_shas.update(pr_commit_sha_list) + if pr.merge_commit_sha: + pr_commit_shas.add(pr.merge_commit_sha) + logger.debug( + "Compare mode: PR #%d has %d commit(s) via get_commits(): %s, merge_commit_sha=%s.", + pr.number, + len(pr_commit_sha_list), + pr_commit_sha_list, + pr.merge_commit_sha, + ) + + @staticmethod + def _get_pull_ignoring_not_found(repo: Repository, number: int) -> Optional[PullRequest]: + """ + Fetch a PR by number, treating "not found" as an expected outcome (bare `#N` commit + references are as likely to point at an issue as at a PR) rather than an error worth a + full traceback in the logs. + """ + try: + return repo.get_pull(number) + except UnknownObjectException: + return None + def _validate_tag_exists(self, repo: Repository, tag: str) -> None: try: repo.get_git_ref(f"tags/{tag}") @@ -598,9 +679,9 @@ def _extract_pr_numbers_from_commits(commits: list[GithubCommit]) -> set[int]: """ pr_numbers: set[int] = set() for commit in commits: - subject = commit.commit.message.splitlines()[0] if commit.commit.message else "" + subject = get_commit_subject(commit) for match in _PR_NUMBER_RE.finditer(subject): - number_str = match.group(1) or match.group(2) + number_str = match.group(1) or match.group(2) or match.group(3) pr_numbers.add(int(number_str)) return pr_numbers diff --git a/release_notes_generator/record/factory/default_record_factory.py b/release_notes_generator/record/factory/default_record_factory.py index 1231a431..cb5a6c01 100644 --- a/release_notes_generator/record/factory/default_record_factory.py +++ b/release_notes_generator/record/factory/default_record_factory.py @@ -40,7 +40,7 @@ from release_notes_generator.utils.github_rate_limiter import GithubRateLimiter from release_notes_generator.utils.pull_request_utils import get_issues_for_pr, extract_issue_numbers_from_body -from release_notes_generator.utils.record_utils import get_id, parse_issue_id +from release_notes_generator.utils.record_utils import get_id, parse_issue_id, get_commit_subject logger = logging.getLogger(__name__) @@ -92,6 +92,8 @@ def generate(self, data: MinedData) -> dict[str, Record]: logger.info("Registering direct commits to records...") for commit, repo in data.commits.items(): if commit.sha not in self.__registered_commits: + subject = get_commit_subject(commit) + logger.debug("Direct commit registered: %s ('%s')", commit.sha, subject) self._records[get_id(commit, repo)] = CommitRecord(commit) # dev note: now we have all PRs and commits registered to issues or as stand-alone records @@ -136,6 +138,14 @@ def _register_pull_and_its_commits_to_issue( pr_commit_shas.add(pull.merge_commit_sha) related_commits = [c for c in data.commits if c.sha in pr_commit_shas] self.__registered_commits.update(c.sha for c in related_commits) + related_commit_shas = [c.sha for c in related_commits] + logger.debug( + "PR #%d: %d commit SHA(s) via get_commits() + merge_commit_sha, %d matched against mined commits: %s", + pull.number, + len(pr_commit_shas), + len(related_commits), + related_commit_shas, + ) pr_repo = target_repository if target_repository is not None else data.home_repository diff --git a/release_notes_generator/utils/record_utils.py b/release_notes_generator/utils/record_utils.py index f65dcd15..9038d051 100644 --- a/release_notes_generator/utils/record_utils.py +++ b/release_notes_generator/utils/record_utils.py @@ -228,3 +228,16 @@ def placeholder(key: str) -> str: result = re.sub(placeholder(key), str(value), result, flags=re.IGNORECASE) return re.sub(r"\s+", " ", result).strip() + + +def get_commit_subject(commit: Commit) -> str: + """ + Extract the first line (subject) of a commit message. + + Parameters: + commit: A GitHub commit object. + + Returns: + The first line of the commit message, or empty string if message is None. + """ + return commit.commit.message.splitlines()[0] if commit.commit.message else "" diff --git a/tests/unit/release_notes_generator/data/test_miner.py b/tests/unit/release_notes_generator/data/test_miner.py index 71007d7c..6d0b8386 100644 --- a/tests/unit/release_notes_generator/data/test_miner.py +++ b/tests/unit/release_notes_generator/data/test_miner.py @@ -20,7 +20,7 @@ from datetime import datetime from typing import Optional -from github import Github, GithubException +from github import Github, GithubException, UnknownObjectException from github.Commit import Commit from github.GitRelease import GitRelease from github.Issue import Issue @@ -523,6 +523,28 @@ def test_fetch_prs_for_fetched_cross_issues(mocker, mock_repo): warn_mock.assert_called_once() +# --- _get_pull_ignoring_not_found --- + + +def test_get_pull_ignoring_not_found_returns_none_on_404(mocker, mock_repo): + """A bare `#N` commit reference is as likely to point at an issue as at a PR; a 404 for it is + expected and must not surface as an error-level traceback.""" + mock_repo.get_pull.side_effect = UnknownObjectException(404, {"message": "Not Found"}, None) + error_mock = mocker.patch("release_notes_generator.data.miner.logger.error") + + result = DataMiner._get_pull_ignoring_not_found(mock_repo, 1384) + + assert result is None + error_mock.assert_not_called() + + +def test_get_pull_ignoring_not_found_propagates_other_errors(mock_repo): + mock_repo.get_pull.side_effect = GithubException(403, {"message": "rate limited"}, None) + + with pytest.raises(GithubException): + DataMiner._get_pull_ignoring_not_found(mock_repo, 1384) + + # --- _extract_pr_numbers_from_commits --- @@ -570,6 +592,19 @@ def test_extract_pr_numbers_multiline_message(mocker): assert DataMiner._extract_pr_numbers_from_commits([commit]) == set() +def test_extract_pr_numbers_leading_bare_hash_format(mocker): + commit = mocker.Mock() + commit.commit.message = "#1403 Investigate and fix omd dockerfile certificate issue" + assert DataMiner._extract_pr_numbers_from_commits([commit]) == {1403} + + +def test_extract_pr_numbers_leading_bare_hash_not_matched_mid_subject(mocker): + commit = mocker.Mock() + commit.commit.message = "Feature/#1366 prebuild kerberos dependencies" + assert DataMiner._extract_pr_numbers_from_commits([commit]) == set() + + + # --- mine_data compare mode --- @@ -602,6 +637,7 @@ def _make_compare_miner(mocker, mock_repo, *, from_tag="v2.6.3", to_tag="v2.6.4" else: default_pr = mocker.Mock(spec=PullRequest) default_pr.get_commits.return_value = [] + default_pr.merge_commit_sha = None mock_repo.get_pull.return_value = default_pr github_mock = mocker.Mock(spec=Github) @@ -632,6 +668,7 @@ def test_mine_data_compare_mode_fetches_prs_by_number(mocker, mock_repo): pr_mock = mocker.Mock(spec=PullRequest) pr_mock.number = 42 pr_mock.get_commits.return_value = [] + pr_mock.merge_commit_sha = None miner = _make_compare_miner(mocker, mock_repo, compare_commits=[commit_mock], get_pull_side_effect=lambda n: pr_mock if n == 42 else None) @@ -650,9 +687,11 @@ def test_mine_data_compare_mode_multiple_prs(mocker, mock_repo): pr10 = mocker.Mock(spec=PullRequest) pr10.number = 10 pr10.get_commits.return_value = [] + pr10.merge_commit_sha = None pr20 = mocker.Mock(spec=PullRequest) pr20.number = 20 pr20.get_commits.return_value = [] + pr20.merge_commit_sha = None miner = _make_compare_miner(mocker, mock_repo, compare_commits=[c1, c2], get_pull_side_effect=lambda n: pr10 if n == 10 else pr20) @@ -694,10 +733,28 @@ def test_mine_data_compare_mode_skips_none_prs(mocker, mock_repo): assert data.pull_requests == {} +def test_mine_data_compare_mode_bare_hash_ref_to_unresolved_pr_stays_direct_commit(mocker, mock_repo): + """A bare leading "#N" is a common convention for referencing an issue, not proof the commit + belongs to a real merged PR. If #N can't be resolved to a merged PR, the commit must remain a + direct commit rather than silently vanishing from the release notes.""" + commit_mock = mocker.Mock() + commit_mock.sha = "dead450" + commit_mock.commit.message = "#450 Fix typo in docs" + commit_mock.get_pulls.return_value = [] + + miner = _make_compare_miner(mocker, mock_repo, compare_commits=[commit_mock], + get_pull_side_effect=lambda _: None) + data = miner.mine_data() + + assert data.pull_requests == {} + assert "dead450" in {c.sha for c in data.commits} + + def test_mine_data_compare_mode_no_pr_numbers_in_message(mocker, mock_repo): commit_mock = mocker.Mock() commit_mock.sha = "bumpsha" commit_mock.commit.message = "Bump version to 2.6.4" + commit_mock.get_pulls.return_value = [] miner = _make_compare_miner(mocker, mock_repo, compare_commits=[commit_mock]) data = miner.mine_data() @@ -723,6 +780,7 @@ def test_mine_data_compare_mode_excludes_sync_merge_commit_belonging_to_pr( pr7 = mocker.Mock(spec=PullRequest) pr7.number = 7 pr7.get_commits.return_value = [sync_merge_commit, squash_commit] + pr7.merge_commit_sha = None miner = _make_compare_miner( mocker, @@ -736,6 +794,104 @@ def test_mine_data_compare_mode_excludes_sync_merge_commit_belonging_to_pr( assert data.commits == {} +def test_mine_data_compare_mode_finds_pr_via_commit_association_fallback( + mocker: MockerFixture, mock_repo: Repository +) -> None: + """A merged PR's commit message may never reference the PR number at all (e.g. "Set project + version to 1.8.0"). Such commits must still be excluded via the commit -> PRs association + fallback, not left as misclassified direct commits (issue #337 follow-up).""" + commit_mock = mocker.Mock() + commit_mock.sha = "versionbumpsha" + commit_mock.commit.message = "Set project version to 1.8.0" + + pr1430 = mocker.Mock(spec=PullRequest) + pr1430.number = 1430 + pr1430.merged = True + pr1430.merge_commit_sha = "mergeshaxyz" + pr1430.get_commits.return_value = [commit_mock] + commit_mock.get_pulls.return_value = [pr1430] + + miner = _make_compare_miner(mocker, mock_repo, compare_commits=[commit_mock]) + data = miner.mine_data() + + assert pr1430 in data.pull_requests + assert data.commits == {} + + +def test_mine_data_compare_mode_ignores_unmerged_pr_from_association_fallback( + mocker: MockerFixture, mock_repo: Repository +) -> None: + """An open (not-yet-merged) PR returned by the commit association endpoint must not suppress + a direct commit.""" + commit_mock = mocker.Mock() + commit_mock.sha = "directsha" + commit_mock.commit.message = "Quick fix" + + open_pr = mocker.Mock(spec=PullRequest) + open_pr.number = 55 + open_pr.merged = False + commit_mock.get_pulls.return_value = [open_pr] + + miner = _make_compare_miner(mocker, mock_repo, compare_commits=[commit_mock]) + data = miner.mine_data() + + assert "directsha" in {c.sha for c in data.commits} + + +def test_mine_data_compare_mode_association_fallback_dedupes_by_pr_number( + mocker: MockerFixture, mock_repo: Repository +) -> None: + """The commit -> PRs association endpoint can return a fresh PullRequest instance for a PR already + discovered (e.g. via another commit's association lookup), distinct by object identity from that + prior instance even though it's the same real PR. Dedup must be by PR number, not object identity, + or the same PR ends up registered twice in data.pull_requests.""" + commit_a = mocker.Mock() + commit_a.sha = "shaA" + commit_a.commit.message = "Part of a PR but message doesn't say so (A)" + + commit_b = mocker.Mock() + commit_b.sha = "shaB" + commit_b.commit.message = "Part of a PR but message doesn't say so (B)" + + pr99_via_a = mocker.Mock(spec=PullRequest) + pr99_via_a.number = 99 + pr99_via_a.merged = True + pr99_via_a.merge_commit_sha = None + pr99_via_a.get_commits.return_value = [commit_a, commit_b] + commit_a.get_pulls.return_value = [pr99_via_a] + + pr99_via_b = mocker.Mock(spec=PullRequest) + pr99_via_b.number = 99 + pr99_via_b.merged = True + pr99_via_b.merge_commit_sha = None + pr99_via_b.get_commits.return_value = [commit_a, commit_b] + commit_b.get_pulls.return_value = [pr99_via_b] + + miner = _make_compare_miner(mocker, mock_repo, compare_commits=[commit_a, commit_b]) + data = miner.mine_data() + + assert len(data.pull_requests) == 1 + assert data.commits == {} + + +def test_mine_data_compare_mode_skips_association_fallback_above_cap(mocker, mock_repo): + """When too many commits lack a detected PR, the per-commit association fallback is skipped + rather than issuing one API call per commit.""" + commits = [] + for i in range(201): + commit_mock = mocker.Mock() + commit_mock.sha = f"sha{i}" + commit_mock.commit.message = "Direct change" + commits.append(commit_mock) + + miner = _make_compare_miner(mocker, mock_repo, compare_commits=commits) + data = miner.mine_data() + + assert len(data.commits) == 201 + for commit_mock in commits: + commit_mock.get_pulls.assert_not_called() + + def test_mine_data_compare_mode_warns_on_total_commits_overflow(mocker, mock_repo): """Test that a warning is logged when the compare API returns more commits than it can retrieve (over 10,000).""" commit_mock = mocker.Mock() diff --git a/tests/unit/release_notes_generator/record/factory/test_default_record_factory.py b/tests/unit/release_notes_generator/record/factory/test_default_record_factory.py index d6a23be6..757dd968 100644 --- a/tests/unit/release_notes_generator/record/factory/test_default_record_factory.py +++ b/tests/unit/release_notes_generator/record/factory/test_default_record_factory.py @@ -198,6 +198,7 @@ def test_generate_with_issues_and_pulls_and_commits(mocker, mock_repo): data.pull_requests = {pr1: mock_repo} commit3 = mocker.Mock(spec=Commit) commit3.sha = "ghi789" + commit3.commit.message = "Direct commit ghi789" commit3.repository = mock_repo data.commits = {commit1: mock_repo, commit2: mock_repo, commit3: mock_repo} @@ -282,6 +283,7 @@ def test_generate_with_issues_and_pulls_and_commits_with_skip_labels(mocker, moc commit3 = mocker.Mock(spec=Commit) commit3.sha = "ghi789" + commit3.commit.message = "Direct commit ghi789" commit3.repository.full_name = "org/repo" data = MinedData(mock_repo)