Addresses Review Comments

RMI-PACTA · Dec 18, 2023 · 8982c96 · 8982c96
1 parent c86abae
commit 8982c96
Show file tree

Hide file tree

Showing 5 changed files with 59 additions and 54 deletions.
diff --git a/cmd/server/pactasrv/populate.go b/cmd/server/pactasrv/populate.go
@@ -9,6 +9,59 @@ import (
 	"go.uber.org/zap"
 )
 
+func (s *Server) populatePortfoliosInPortfolioGroups(
+	ctx context.Context,
+	ts []*pacta.PortfolioGroup,
+) error {
+	getFn := func(pg *pacta.PortfolioGroup) ([]*pacta.Portfolio, error) {
+		result := []*pacta.Portfolio{}
+		for _, member := range pg.Members {
+			result = append(result, member.Portfolio)
+		}
+		return result, nil
+	}
+	lookupFn := func(ids []pacta.PortfolioID) (map[pacta.PortfolioID]*pacta.Portfolio, error) {
+		return s.DB.Portfolios(s.DB.NoTxn(ctx), ids)
+	}
+	getIDFn := func(p *pacta.Portfolio) pacta.PortfolioID {
+		return p.ID
+	}
+	if err := populateAll(ts, getFn, getIDFn, lookupFn); err != nil {
+		return oapierr.Internal("populating portfolios in portfolio groups failed", zap.Error(err))
+	}
+	return nil
+}
+
+func (s *Server) populatePortfolioGroupsInPortfolios(
+	ctx context.Context,
+	ts []*pacta.Portfolio,
+) error {
+	getFn := func(pg *pacta.Portfolio) ([]*pacta.PortfolioGroup, error) {
+		result := []*pacta.PortfolioGroup{}
+		for _, member := range pg.MemberOf {
+			result = append(result, member.PortfolioGroup)
+		}
+		return result, nil
+	}
+	lookupFn := func(ids []pacta.PortfolioGroupID) (map[pacta.PortfolioGroupID]*pacta.PortfolioGroup, error) {
+		return s.DB.PortfolioGroups(s.DB.NoTxn(ctx), ids)
+	}
+	getIDFn := func(p *pacta.PortfolioGroup) pacta.PortfolioGroupID {
+		return p.ID
+	}
+	if err := populateAll(ts, getFn, getIDFn, lookupFn); err != nil {
+		return oapierr.Internal("populating portfolio groups in portfolios failed", zap.Error(err))
+	}
+	return nil
+}
+
+// This helper function populates the given targets in the given sources,
+// to allow for generic population of nested data structures.
+// sources = entities that you want to populate sub-entity references in.
+// the sub-entities should be pointers to structs with an ID populated.
+// getTargetsFn = function that takes a source and returns zero or more sub-entities to populate.
+// getTargetIDFn = function that takes a sub-entity and returns its ID.
+// lookupTargetsFn = function that takes a list of sub-entity IDs and returns a map of ID -> sub-entity.
 func populateAll[Source any, TargetID ~string, Target any](
 	sources []*Source,
 	getTargetsFn func(*Source) ([]*Target, error),
@@ -54,51 +107,3 @@ func populateAll[Source any, TargetID ~string, Target any](
 	}
 	return nil
 }
-
-func populatePortfoliosInPortfolioGroups(
-	s *Server,
-	ctx context.Context,
-	ts []*pacta.PortfolioGroup,
-) error {
-	getFn := func(pg *pacta.PortfolioGroup) ([]*pacta.Portfolio, error) {
-		result := []*pacta.Portfolio{}
-		for _, member := range pg.Members {
-			result = append(result, member.Portfolio)
-		}
-		return result, nil
-	}
-	lookupFn := func(ids []pacta.PortfolioID) (map[pacta.PortfolioID]*pacta.Portfolio, error) {
-		return s.DB.Portfolios(s.DB.NoTxn(ctx), ids)
-	}
-	getIDFn := func(p *pacta.Portfolio) pacta.PortfolioID {
-		return p.ID
-	}
-	if err := populateAll(ts, getFn, getIDFn, lookupFn); err != nil {
-		return oapierr.Internal("populating portfolios in portfolio groups failed", zap.Error(err))
-	}
-	return nil
-}
-
-func populatePortfolioGroupsInPortfolios(
-	s *Server,
-	ctx context.Context,
-	ts []*pacta.Portfolio,
-) error {
-	getFn := func(pg *pacta.Portfolio) ([]*pacta.PortfolioGroup, error) {
-		result := []*pacta.PortfolioGroup{}
-		for _, member := range pg.MemberOf {
-			result = append(result, member.PortfolioGroup)
-		}
-		return result, nil
-	}
-	lookupFn := func(ids []pacta.PortfolioGroupID) (map[pacta.PortfolioGroupID]*pacta.PortfolioGroup, error) {
-		return s.DB.PortfolioGroups(s.DB.NoTxn(ctx), ids)
-	}
-	getIDFn := func(p *pacta.PortfolioGroup) pacta.PortfolioGroupID {
-		return p.ID
-	}
-	if err := populateAll(ts, getFn, getIDFn, lookupFn); err != nil {
-		return oapierr.Internal("populating portfolio groups in portfolios failed", zap.Error(err))
-	}
-	return nil
-}
diff --git a/cmd/server/pactasrv/portfolio.go b/cmd/server/pactasrv/portfolio.go
@@ -24,7 +24,7 @@ func (s *Server) ListPortfolios(ctx context.Context, request api.ListPortfoliosR
 	if err != nil {
 		return nil, oapierr.Internal("failed to query portfolios", zap.Error(err))
 	}
-	if err := populatePortfolioGroupsInPortfolios(s, ctx, ps); err != nil {
+	if err := s.populatePortfolioGroupsInPortfolios(ctx, ps); err != nil {
 		return nil, err
 	}
 	items, err := dereference(conv.PortfoliosToOAPI(ps))
@@ -59,7 +59,7 @@ func (s *Server) FindPortfolioById(ctx context.Context, request api.FindPortfoli
 	if err != nil {
 		return nil, err
 	}
-	if err := populatePortfolioGroupsInPortfolios(s, ctx, []*pacta.Portfolio{p}); err != nil {
+	if err := s.populatePortfolioGroupsInPortfolios(ctx, []*pacta.Portfolio{p}); err != nil {
 		return nil, err
 	}
 	converted, err := conv.PortfolioToOAPI(p)

diff --git a/cmd/server/pactasrv/portfolio_group.go b/cmd/server/pactasrv/portfolio_group.go
@@ -22,7 +22,7 @@ func (s *Server) FindPortfolioGroupById(ctx context.Context, request api.FindPor
 		}
 		return nil, oapierr.Internal("failed to load portfolio_group", zap.String("portfolio_group_id", request.Id), zap.Error(err))
 	}
-	if err := populatePortfoliosInPortfolioGroups(s, ctx, []*pacta.PortfolioGroup{pg}); err != nil {
+	if err := s.populatePortfoliosInPortfolioGroups(ctx, []*pacta.PortfolioGroup{pg}); err != nil {
 		return nil, err
 	}
 	resp, err := conv.PortfolioGroupToOAPI(pg)
@@ -44,7 +44,7 @@ func (s *Server) ListPortfolioGroups(ctx context.Context, request api.ListPortfo
 	if err != nil {
 		return nil, oapierr.Internal("failed to query portfolio groups", zap.Error(err))
 	}
-	if err := populatePortfoliosInPortfolioGroups(s, ctx, pgs); err != nil {
+	if err := s.populatePortfoliosInPortfolioGroups(ctx, pgs); err != nil {
 		return nil, err
 	}
 	items, err := dereference(conv.PortfolioGroupsToOAPI(pgs))

diff --git a/db/sqldb/portfolio.go b/db/sqldb/portfolio.go
@@ -72,7 +72,7 @@ func (d *DB) PortfoliosByOwner(tx db.Tx, ownerID pacta.OwnerID) ([]*pacta.Portfo
 	if err != nil {
 		return nil, fmt.Errorf("translating rows to portfolios: %w", err)
 	}
-	// Note the map interface here is ~required in the deserialization process to track multiple membersihps,
+	// Note the map interface here is ~required in the deserialization process to track multiple memberships,
 	// so we're not just converting to a map and back.
 	return valuesFromMap(pvs), nil
 }

diff --git a/db/sqldb/portfolio_group.go b/db/sqldb/portfolio_group.go
@@ -51,7 +51,7 @@ func (d *DB) PortfolioGroupsByOwner(tx db.Tx, ownerID pacta.OwnerID) ([]*pacta.P
 	if err != nil {
 		return nil, fmt.Errorf("querying portfolio_groups: %w", err)
 	}
-	// Note the map interface here is ~required in the deserialization process to track multiple membersihps,
+	// Note the map interface here is ~required in the deserialization process to track multiple memberships,
 	// so we're not just converting to a map and back.
 	asMap, err := rowsToPortfolioGroups(rows)
 	if err != nil {