refactor(internal/librarian): remove contentLoader (#1698)

The release code will also need to use some of this functionality and to
make the code easier to call/read I refactored out the idea of a content
loader that I believe was added to help coverage. I am fine taking a
couple points of a coverage hit to make the code, in my opinion, more
readable.

Updates: #1009

Author: codyoss

internal/librarian/generate.go

@@ -16,7 +16,6 @@ package librarian
 
 import (
 	"context"
-	"encoding/json"
 	"errors"
 	"fmt"
 	"io/fs"
@@ -274,9 +273,6 @@ func (r *generateRunner) runGenerateCommand(ctx context.Context, libraryID, outp
 
 	// Read the library state from the response.
 	if _, err := readLibraryState(
-		func(data []byte, libraryState *config.LibraryState) error {
-			return json.Unmarshal(data, libraryState)
-		},
 		filepath.Join(generateRequest.RepoDir, config.LibrarianDir, config.GenerateResponse)); err != nil {
 		return "", err
 	}
@@ -334,10 +330,8 @@ func (r *generateRunner) runBuildCommand(ctx context.Context, libraryID string)
 
 	// Read the library state from the response.
 	_, err := readLibraryState(
-		func(data []byte, libraryState *config.LibraryState) error {
-			return json.Unmarshal(data, libraryState)
-		},
-		filepath.Join(buildRequest.RepoDir, config.LibrarianDir, config.BuildResponse))
+		filepath.Join(buildRequest.RepoDir, config.LibrarianDir, config.BuildResponse),
+	)
 
 	return err
 }
@@ -529,9 +523,6 @@ func (r *generateRunner) runConfigureCommand(ctx context.Context) (string, error
 
 	if err := populateServiceConfigIfEmpty(
 		r.state,
-		func(file string) ([]byte, error) {
-			return os.ReadFile(file)
-		},
 		r.cfg.APISource); err != nil {
 		return "", err
 	}
@@ -550,10 +541,8 @@ func (r *generateRunner) runConfigureCommand(ctx context.Context) (string, error
 
 	// Read the new library state from the response.
 	libraryState, err := readLibraryState(
-		func(data []byte, libraryState *config.LibraryState) error {
-			return json.Unmarshal(data, libraryState)
-		},
-		filepath.Join(r.repo.GetDir(), config.LibrarianDir, config.ConfigureResponse))
+		filepath.Join(r.repo.GetDir(), config.LibrarianDir, config.ConfigureResponse),
+	)
 	if err != nil {
 		return "", err
 	}

internal/librarian/state.go

@@ -15,6 +15,7 @@
 package librarian
 
 import (
+	"encoding/json"
 	"errors"
 	"fmt"
 	"log/slog"
@@ -36,35 +37,25 @@ const (
 
 // Utility functions for saving and loading pipeline state and config from various places.
 
-func loadRepoState(languageRepo *gitrepo.LocalRepository, source string) (*config.LibrarianState, error) {
-	if languageRepo == nil {
+func loadRepoState(repo *gitrepo.LocalRepository, source string) (*config.LibrarianState, error) {
+	if repo == nil {
+		slog.Info("repo is nil, skipping state loading")
 		return nil, nil
 	}
-	state, err := loadLibrarianState(languageRepo, source)
-	if err != nil {
-		return nil, err
-	}
-	return state, nil
-}
-
-func loadLibrarianState(languageRepo *gitrepo.LocalRepository, source string) (*config.LibrarianState, error) {
-	if languageRepo == nil {
-		return nil, nil
-	}
-	path := filepath.Join(languageRepo.Dir, config.LibrarianDir, pipelineStateFile)
-	return parseLibrarianState(func(file string) ([]byte, error) { return os.ReadFile(file) }, path, source)
+	path := filepath.Join(repo.Dir, config.LibrarianDir, pipelineStateFile)
+	return parseLibrarianState(path, source)
 }
 
-func parseLibrarianState(contentLoader func(file string) ([]byte, error), path, source string) (*config.LibrarianState, error) {
-	bytes, err := contentLoader(path)
+func parseLibrarianState(path, source string) (*config.LibrarianState, error) {
+	bytes, err := os.ReadFile(path)
 	if err != nil {
 		return nil, err
 	}
 	var s config.LibrarianState
 	if err := yaml.Unmarshal(bytes, &s); err != nil {
 		return nil, fmt.Errorf("unmarshaling librarian state: %w", err)
 	}
-	if err := populateServiceConfigIfEmpty(&s, contentLoader, source); err != nil {
+	if err := populateServiceConfigIfEmpty(&s, source); err != nil {
 		return nil, fmt.Errorf("populating service config: %w", err)
 	}
 	if err := s.Validate(); err != nil {
@@ -73,15 +64,19 @@ func parseLibrarianState(contentLoader func(file string) ([]byte, error), path,
 	return &s, nil
 }
 
-func populateServiceConfigIfEmpty(state *config.LibrarianState, contentLoader func(file string) ([]byte, error), source string) error {
+func populateServiceConfigIfEmpty(state *config.LibrarianState, source string) error {
+	if source == "" {
+		slog.Info("source not specified, skipping service config population")
+		return nil
+	}
 	for i, library := range state.Libraries {
 		for j, api := range library.APIs {
 			if api.ServiceConfig != "" {
 				// Do not change API if the service config has already been set.
 				continue
 			}
 			apiPath := filepath.Join(source, api.Path)
-			serviceConfig, err := findServiceConfigIn(contentLoader, apiPath)
+			serviceConfig, err := findServiceConfigIn(apiPath)
 			if err != nil {
 				return err
 			}
@@ -100,7 +95,7 @@ func populateServiceConfigIfEmpty(state *config.LibrarianState, contentLoader fu
 // 1. the file ends with `.yaml` and it is a valid yaml file.
 //
 // 2. the file contains `type: google.api.Service`.
-func findServiceConfigIn(contentLoader func(file string) ([]byte, error), path string) (string, error) {
+func findServiceConfigIn(path string) (string, error) {
 	entries, err := os.ReadDir(path)
 	if err != nil {
 		return "", fmt.Errorf("failed to read dir %q: %w", path, err)
@@ -110,7 +105,7 @@ func findServiceConfigIn(contentLoader func(file string) ([]byte, error), path s
 		if !strings.HasSuffix(entry.Name(), ".yaml") {
 			continue
 		}
-		bytes, err := contentLoader(filepath.Join(path, entry.Name()))
+		bytes, err := os.ReadFile(filepath.Join(path, entry.Name()))
 		if err != nil {
 			return "", err
 		}
@@ -138,7 +133,7 @@ func saveLibrarianState(repoDir string, state *config.LibrarianState) error {
 // readLibraryState reads the library state from a container response.
 //
 // The response file is removed afterwards.
-func readLibraryState(contentLoader func(data []byte, state *config.LibraryState) error, jsonFilePath string) (*config.LibraryState, error) {
+func readLibraryState(jsonFilePath string) (*config.LibraryState, error) {
 	data, err := os.ReadFile(jsonFilePath)
 	defer func() {
 		if err := os.Remove(jsonFilePath); err != nil {
@@ -149,9 +144,9 @@ func readLibraryState(contentLoader func(data []byte, state *config.LibraryState
 		return nil, fmt.Errorf("failed to read response file, path: %s, error: %w", jsonFilePath, err)
 	}
 
-	libraryState := &config.LibraryState{}
+	var libraryState *config.LibraryState
 
-	if err := contentLoader(data, libraryState); err != nil {
+	if err := json.Unmarshal(data, &libraryState); err != nil {
 		return nil, fmt.Errorf("failed to load file, %s, to state: %w", jsonFilePath, err)
 	}
 

internal/librarian/state_test.go

@@ -15,7 +15,6 @@
 package librarian
 
 import (
-	"encoding/json"
 	"errors"
 	"fmt"
 	"io"
@@ -96,10 +95,11 @@ libraries:
 		},
 	} {
 		t.Run(test.name, func(t *testing.T) {
-			contentLoader := func(path string) ([]byte, error) {
-				return []byte(path), nil
+			path := filepath.Join(t.TempDir(), "state.yaml")
+			if err := os.WriteFile(path, []byte(test.content), 0644); err != nil {
+				t.Fatalf("os.WriteFile() failed: %v", err)
 			}
-			got, err := parseLibrarianState(contentLoader, test.content, test.source)
+			got, err := parseLibrarianState(path, test.source)
 			if (err != nil) != test.wantErr {
 				t.Errorf("parseLibrarianState() error = %v, wantErr %v", err, test.wantErr)
 				return
@@ -113,59 +113,43 @@ libraries:
 
 func TestFindServiceConfigIn(t *testing.T) {
 	for _, test := range []struct {
-		name          string
-		contentLoader func(file string) ([]byte, error)
-		path          string
-		want          string
-		wantErr       bool
+		name    string
+		path    string
+		want    string
+		wantErr bool
 	}{
 		{
 			name: "find a service config",
-			contentLoader: func(file string) ([]byte, error) {
-				return os.ReadFile(file)
-			},
 			path: filepath.Join("..", "..", "testdata", "find_a_service_config"),
 			want: "service_config.yaml",
 		},
 		{
-			name: "non existed source path",
-			contentLoader: func(file string) ([]byte, error) {
-				return os.ReadFile(file)
-			},
+			name:    "non existed source path",
 			path:    filepath.Join("..", "..", "testdata", "non-existed-path"),
 			want:    "",
 			wantErr: true,
 		},
 		{
-			name: "non service config in a source path",
-			contentLoader: func(file string) ([]byte, error) {
-				return os.ReadFile(file)
-			},
+			name:    "non service config in a source path",
 			path:    filepath.Join("..", "..", "testdata", "no_service_config"),
 			want:    "",
 			wantErr: true,
 		},
 		{
-			name: "simulated load error",
-			contentLoader: func(file string) ([]byte, error) {
-				return nil, errors.New("simulate loading error for testing")
-			},
+			name:    "simulated load error",
 			path:    filepath.Join("..", "..", "testdata", "no_service_config"),
 			want:    "",
 			wantErr: true,
 		},
 		{
-			name: "invalid yaml",
-			contentLoader: func(file string) ([]byte, error) {
-				return os.ReadFile(file)
-			},
+			name:    "invalid yaml",
 			path:    filepath.Join("..", "..", "testdata", "invalid_yaml"),
 			want:    "",
 			wantErr: true,
 		},
 	} {
 		t.Run(test.name, func(t *testing.T) {
-			got, err := findServiceConfigIn(test.contentLoader, test.path)
+			got, err := findServiceConfigIn(test.path)
 			if test.wantErr {
 				if err == nil {
 					t.Errorf("findServiceConfigIn() should return error")
@@ -245,10 +229,7 @@ func TestPopulateServiceConfig(t *testing.T) {
 		},
 	} {
 		t.Run(test.name, func(t *testing.T) {
-			contentLoader := func(file string) ([]byte, error) {
-				return os.ReadFile(file)
-			}
-			err := populateServiceConfigIfEmpty(test.state, contentLoader, test.path)
+			err := populateServiceConfigIfEmpty(test.state, test.path)
 			if test.wantErr {
 				if err == nil {
 					t.Errorf("findServiceConfigIn() should return error")
@@ -310,9 +291,6 @@ func TestSaveLibrarianState(t *testing.T) {
 
 func TestReadConfigureResponseJSON(t *testing.T) {
 	t.Parallel()
-	contentLoader := func(data []byte, state *config.LibraryState) error {
-		return json.Unmarshal(data, state)
-	}
 	for _, test := range []struct {
 		name         string
 		jsonFilePath string
@@ -350,17 +328,12 @@ func TestReadConfigureResponseJSON(t *testing.T) {
 			name:      "invalid file name",
 			wantState: nil,
 		},
-		{
-			name:         "invalid content loader",
-			jsonFilePath: "../../testdata/invalid-contentLoader.json",
-			wantState:    nil,
-		},
 	} {
 		t.Run(test.name, func(t *testing.T) {
 			tempDir := t.TempDir()
 			if test.name == "invalid file name" {
 				filePath := filepath.Join(tempDir, "my\x00file.json")
-				_, err := readLibraryState(contentLoader, filePath)
+				_, err := readLibraryState(filePath)
 				if err == nil {
 					t.Error("readLibraryState() expected an error but got nil")
 				}
@@ -373,14 +346,11 @@ func TestReadConfigureResponseJSON(t *testing.T) {
 			}
 
 			if test.name == "invalid content loader" {
-				invalidContentLoader := func(data []byte, state *config.LibraryState) error {
-					return errors.New("simulated Unmarshal error")
-				}
 				dst := fmt.Sprintf("%s/copy.json", os.TempDir())
 				if err := copyFile(dst, test.jsonFilePath); err != nil {
 					t.Error(err)
 				}
-				_, err := readLibraryState(invalidContentLoader, dst)
+				_, err := readLibraryState(dst)
 				if err == nil {
 					t.Errorf("readLibraryState() expected an error but got nil")
 				}
@@ -398,7 +368,7 @@ func TestReadConfigureResponseJSON(t *testing.T) {
 				t.Error(err)
 			}
 
-			gotState, err := readLibraryState(contentLoader, dstFilePath)
+			gotState, err := readLibraryState(dstFilePath)
 
 			if test.name == "load content with an error message" {
 				if err == nil {