fix: Conventional Commit parser filters files based on Librarian flow (#2433)

Fixes: #2403

Generation PR is based on files changed in the source repo (googleapis)
and NOT language repo. This should filter for files changed that match
the the paths for each API in a library.

Release PR is based on files changed in the language repo and NOT source
repo. This should filter for files changed that match the the paths for
a library's `source_root`.

## Changes
- Introduce a parameter for generation / release flows to define their
own files filtering logic (apiPaths vs sourceRoot)

---------

Signed-off-by: Lawrence Qiu <[email protected]>
Co-authored-by: Joe Wang <[email protected]>

Author: lqiu96

internal/librarian/commit_version_analyzer.go

@@ -28,20 +28,39 @@ import (
 
 const defaultTagFormat = "{id}-{version}"
 
-// GetConventionalCommitsSinceLastRelease returns all conventional commits for the given library since the
-// version specified in the state file.
-func GetConventionalCommitsSinceLastRelease(repo gitrepo.Repository, library *config.LibraryState) ([]*conventionalcommits.ConventionalCommit, error) {
+// getConventionalCommitsSinceLastRelease returns all conventional commits for the given library since the
+// version specified in the state file. The repo should be the language repo.
+func getConventionalCommitsSinceLastRelease(repo gitrepo.Repository, library *config.LibraryState) ([]*conventionalcommits.ConventionalCommit, error) {
 	tag := formatTag(library.TagFormat, library.ID, library.Version)
 	commits, err := repo.GetCommitsForPathsSinceTag(library.SourceRoots, tag)
 	if err != nil {
 		return nil, fmt.Errorf("failed to get commits for library %s: %w", library.ID, err)
 	}
 
-	return convertToConventionalCommits(repo, library, commits)
+	// checks that if the files in the commit are in the sources root. The release
+	// changes are in the language repo and NOT in the source repo.
+	shouldIncludeFiles := func(files []string) bool {
+		return shouldIncludeForRelease(files, library.SourceRoots, library.ReleaseExcludePaths)
+	}
+
+	return convertToConventionalCommits(repo, library, commits, shouldIncludeFiles)
+}
+
+// shouldIncludeForRelease determines if a commit should be included in a release.
+// It returns true if there is at least one file in the commit that is under a source_root
+// and not under a release_exclude_path.
+func shouldIncludeForRelease(files, sourceRoots, excludePaths []string) bool {
+	for _, file := range files {
+		if isUnderAnyPath(file, sourceRoots) && !isUnderAnyPath(file, excludePaths) {
+			return true
+		}
+	}
+	return false
 }
 
 // getConventionalCommitsSinceLastGeneration returns all conventional commits for
-// all API paths in given library since the last generation.
+// all API paths in given library since the last generation. The repo input should
+// be the googleapis source repo.
 func getConventionalCommitsSinceLastGeneration(repo gitrepo.Repository, library *config.LibraryState, lastGenCommit string) ([]*conventionalcommits.ConventionalCommit, error) {
 	if lastGenCommit == "" {
 		slog.Info("the last generation commit is empty, skip fetching conventional commits", "library", library.ID)
@@ -58,17 +77,38 @@ func getConventionalCommitsSinceLastGeneration(repo gitrepo.Repository, library
 		return nil, fmt.Errorf("failed to get commits for library %s at commit %s: %w", library.ID, lastGenCommit, err)
 	}
 
-	return convertToConventionalCommits(repo, library, commits)
+	// checks that the files in the commit are in the api paths for the source repo.
+	// The generation change is for changes in the source repo and NOT the language repo.
+	shouldIncludeFiles := func(files []string) bool {
+		return shouldIncludeForGeneration(files, apiPaths)
+	}
+
+	return convertToConventionalCommits(repo, library, commits, shouldIncludeFiles)
 }
 
-func convertToConventionalCommits(repo gitrepo.Repository, library *config.LibraryState, commits []*gitrepo.Commit) ([]*conventionalcommits.ConventionalCommit, error) {
+// shouldIncludeForGeneration determines if a commit should be included in generation.
+// It returns true if there is at least one file in the commit that is under the
+// library's API(s) path (a library could have multiple APIs).
+func shouldIncludeForGeneration(files, apiPaths []string) bool {
+	for _, file := range files {
+		if isUnderAnyPath(file, apiPaths) {
+			return true
+		}
+	}
+	return false
+}
+
+// convertToConventionalCommits converts a list of commits in a git repo into a list
+// of conventional commits. The filesFilter parameter is custom filter out non-matching
+// files depending on a generation or a release change.
+func convertToConventionalCommits(repo gitrepo.Repository, library *config.LibraryState, commits []*gitrepo.Commit, filesFilter func(files []string) bool) ([]*conventionalcommits.ConventionalCommit, error) {
 	var conventionalCommits []*conventionalcommits.ConventionalCommit
 	for _, commit := range commits {
 		files, err := repo.ChangedFilesInCommit(commit.Hash.String())
 		if err != nil {
 			return nil, fmt.Errorf("failed to get changed files for commit %s: %w", commit.Hash.String(), err)
 		}
-		if !shouldInclude(files, library.SourceRoots, library.ReleaseExcludePaths) {
+		if !filesFilter(files) {
 			continue
 		}
 		parsedCommits, err := conventionalcommits.ParseCommits(commit, library.ID)
@@ -97,18 +137,6 @@ func isUnderAnyPath(file string, paths []string) bool {
 	return false
 }
 
-// shouldInclude determines if a commit should be included in a release.
-// It returns true if there is at least one file in the commit that is under a source_root
-// and not under a release_exclude_path.
-func shouldInclude(files, sourceRoots, excludePaths []string) bool {
-	for _, file := range files {
-		if isUnderAnyPath(file, sourceRoots) && !isUnderAnyPath(file, excludePaths) {
-			return true
-		}
-	}
-	return false
-}
-
 // formatTag returns the git tag for a given library version.
 func formatTag(tagFormat string, libraryID string, version string) string {
 	if tagFormat == "" {

internal/librarian/commit_version_analyzer_test.go

@@ -27,7 +27,7 @@ import (
 	"github.com/googleapis/librarian/internal/semver"
 )
 
-func TestShouldInclude(t *testing.T) {
+func TestShouldIncludeForRelease(t *testing.T) {
 	t.Parallel()
 	for _, test := range []struct {
 		name         string
@@ -135,9 +135,45 @@ func TestShouldInclude(t *testing.T) {
 	} {
 		t.Run(test.name, func(t *testing.T) {
 			t.Parallel()
-			got := shouldInclude(test.files, test.sourceRoots, test.excludePaths)
+			got := shouldIncludeForRelease(test.files, test.sourceRoots, test.excludePaths)
 			if got != test.want {
-				t.Errorf("shouldInclude(%v, %v, %v) = %v, want %v", test.files, test.sourceRoots, test.excludePaths, got, test.want)
+				t.Errorf("shouldIncludeForRelease(%v, %v, %v) = %v, want %v", test.files, test.sourceRoots, test.excludePaths, got, test.want)
+			}
+		})
+	}
+}
+
+func TestShouldIncludeForGeneration(t *testing.T) {
+	t.Parallel()
+	for _, test := range []struct {
+		name     string
+		files    []string
+		apiPaths []string
+		want     bool
+	}{
+		{
+			name:     "all_files_in_apiPaths",
+			files:    []string{"a/b/c.proto"},
+			apiPaths: []string{"a"},
+			want:     true,
+		},
+		{
+			name:     "some_files_in_apiPaths",
+			files:    []string{"a/b/c.proto", "e/f/g.proto"},
+			apiPaths: []string{"a"},
+			want:     true,
+		},
+		{
+			name:     "no_files_in_apiPaths",
+			files:    []string{"a/b/c.proto"},
+			apiPaths: []string{"b"},
+			want:     false,
+		},
+	} {
+		t.Run(test.name, func(t *testing.T) {
+			got := shouldIncludeForGeneration(test.files, test.apiPaths)
+			if got != test.want {
+				t.Errorf("shouldIncludeForGeneration(%v, %v) = %v, want %v", test.files, test.apiPaths, got, test.want)
 			}
 		})
 	}
@@ -220,7 +256,7 @@ func TestGetConventionalCommitsSinceLastRelease(t *testing.T) {
 		wantErrPhrase string
 	}{
 		{
-			name: "get_commits_for_foo",
+			name: "found_matching_commits_for_foo",
 			repo: repoWithCommits,
 			library: &config.LibraryState{
 				ID:                  "foo",
@@ -246,6 +282,34 @@ func TestGetConventionalCommitsSinceLastRelease(t *testing.T) {
 				},
 			},
 		},
+		{
+			name: "no_matching_commits_for_foo",
+			repo: repoWithCommits,
+			library: &config.LibraryState{
+				ID:          "foo",
+				Version:     "1.0.0",
+				TagFormat:   "{id}-v{version}",
+				SourceRoots: []string{"no_matching_dir"},
+			},
+		},
+		{
+			name: "apiPaths_has_no_impact_on_release",
+			repo: repoWithCommits,
+			library: &config.LibraryState{
+				ID:          "foo",
+				Version:     "1.0.0",
+				TagFormat:   "{id}-v{version}",
+				SourceRoots: []string{"no_matching_dir"}, // For release, only this is considered
+				APIs: []*config.API{
+					{
+						Path: "foo",
+					},
+					{
+						Path: "bar",
+					},
+				},
+			},
+		},
 		{
 			name: "GetCommitsForPathsSinceTag error",
 			repo: &MockRepository{
@@ -281,7 +345,7 @@ func TestGetConventionalCommitsSinceLastRelease(t *testing.T) {
 		},
 	} {
 		t.Run(test.name, func(t *testing.T) {
-			got, err := GetConventionalCommitsSinceLastRelease(test.repo, test.library)
+			got, err := getConventionalCommitsSinceLastRelease(test.repo, test.library)
 			if test.wantErr {
 				if err == nil {
 					t.Fatal("GetConventionalCommitsSinceLastRelease() should have failed")
@@ -301,6 +365,108 @@ func TestGetConventionalCommitsSinceLastRelease(t *testing.T) {
 	}
 }
 
+func TestGetConventionalCommitsSinceLastGeneration(t *testing.T) {
+	t.Parallel()
+	for _, test := range []struct {
+		name          string
+		repo          gitrepo.Repository
+		library       *config.LibraryState
+		want          []*conventionalcommits.ConventionalCommit
+		wantErr       bool
+		wantErrPhrase string
+	}{
+		{
+			name: "found_matching_file_changes_for_foo",
+			library: &config.LibraryState{
+				ID: "foo",
+				APIs: []*config.API{
+					{
+						Path: "foo",
+					},
+				},
+			},
+			repo: &MockRepository{
+				GetCommitsForPathsSinceLastGenByCommit: map[string][]*gitrepo.Commit{
+					"1234": {
+						{Message: "feat(foo): a feature"},
+					},
+				},
+				ChangedFilesInCommitValue: []string{"foo/a.proto"},
+			},
+			want: []*conventionalcommits.ConventionalCommit{
+				{
+					Type:      "feat",
+					Scope:     "foo",
+					Subject:   "a feature",
+					LibraryID: "foo",
+					Footers:   map[string]string{},
+				},
+			},
+		},
+		{
+			name: "no_matching_file_changes_for_foo",
+			library: &config.LibraryState{
+				ID: "foo",
+				APIs: []*config.API{
+					{
+						Path: "foo",
+					},
+				},
+			},
+			repo: &MockRepository{
+				GetCommitsForPathsSinceLastGenByCommit: map[string][]*gitrepo.Commit{
+					"1234": {
+						{Message: "feat(baz): a feature"},
+					},
+				},
+				ChangedFilesInCommitValue: []string{"baz/a.proto", "baz/b.proto", "bar/a.proto"}, // file changed is not in foo/*
+			},
+		},
+		{
+			name: "sources_root_has_no_impact",
+			library: &config.LibraryState{
+				ID: "foo",
+				APIs: []*config.API{
+					{
+						Path: "foo", // For generation, only this is considered
+					},
+				},
+				SourceRoots: []string{
+					"baz/",
+					"bar/",
+				},
+			},
+			repo: &MockRepository{
+				GetCommitsForPathsSinceLastGenByCommit: map[string][]*gitrepo.Commit{
+					"1234": {
+						{Message: "feat(baz): a feature"},
+					},
+				},
+				ChangedFilesInCommitValue: []string{"baz/a.proto", "baz/b.proto", "bar/a.proto"}, // file changed is not in foo/*
+			},
+		},
+	} {
+		t.Run(test.name, func(t *testing.T) {
+			got, err := getConventionalCommitsSinceLastGeneration(test.repo, test.library, "1234")
+			if test.wantErr {
+				if err == nil {
+					t.Fatal("getConventionalCommitsSinceLastGeneration() should have failed")
+				}
+				if !strings.Contains(err.Error(), test.wantErrPhrase) {
+					t.Errorf("GetConventionalCommitsSinceLastRelease() returned error %q, want to contain %q", err.Error(), test.wantErrPhrase)
+				}
+				return
+			}
+			if err != nil {
+				t.Fatalf("GetConventionalCommitsSinceLastRelease() failed: %v", err)
+			}
+			if diff := cmp.Diff(test.want, got, cmpopts.IgnoreFields(conventionalcommits.ConventionalCommit{}, "SHA", "CommitHash", "Body", "IsBreaking", "When")); diff != "" {
+				t.Errorf("GetConventionalCommitsSinceLastRelease() mismatch (-want +got):\n%s", diff)
+			}
+		})
+	}
+}
+
 func TestGetHighestChange(t *testing.T) {
 	t.Parallel()
 	for _, test := range []struct {

internal/librarian/release_init.go

@@ -196,7 +196,7 @@ func (r *initRunner) runInitCommand(ctx context.Context, outputDir string) error
 // processLibrary wrapper to process the library for release. Helps retrieve latest commits
 // since the last release and passing the changes to updateLibrary.
 func (r *initRunner) processLibrary(library *config.LibraryState) error {
-	commits, err := GetConventionalCommitsSinceLastRelease(r.repo, library)
+	commits, err := getConventionalCommitsSinceLastRelease(r.repo, library)
 	if err != nil {
 		return fmt.Errorf("failed to fetch conventional commits for library, %s: %w", library.ID, err)
 	}