Kaynağa Gözat

chore: reduce cyclomatic complexity

jamesread 7 ay önce
ebeveyn
işleme
de7129e1d7

+ 49 - 23
service/internal/api/api.go

@@ -464,50 +464,76 @@ func (api *oliveTinAPI) GetLogs(ctx ctx.Context, req *connect.Request[apiv1.GetL
 	return connect.NewResponse(ret), nil
 }
 
-func (api *oliveTinAPI) GetActionLogs(ctx ctx.Context, req *connect.Request[apiv1.GetActionLogsRequest]) (*connect.Response[apiv1.GetActionLogsResponse], error) {
-	user := acl.UserFromContext(ctx, req, api.cfg)
+// isValidLogEntry checks if a log entry has all required fields populated.
+func isValidLogEntry(e *executor.InternalLogEntry) bool {
+	return e != nil && e.Binding != nil && e.Binding.Action != nil
+}
 
-	if err := api.checkDashboardAccess(user); err != nil {
-		return nil, err
-	}
+// isLogEntryAllowed checks if a log entry is allowed to be viewed by the user.
+func (api *oliveTinAPI) isLogEntryAllowed(e *executor.InternalLogEntry, user *acl.AuthenticatedUser) bool {
+	return acl.IsAllowedLogs(api.cfg, user, e.Binding.Action)
+}
 
-	ret := &apiv1.GetActionLogsResponse{}
-	filtered := api.filterLogsByACL(api.executor.GetLogsByActionId(req.Msg.ActionId), user)
-	page := paginate(int64(len(filtered)), api.cfg.LogHistoryPageSize, req.Msg.StartOffset)
-	if page.empty {
-		ret.CountRemaining = 0
-		ret.PageSize = page.size
-		ret.TotalCount = page.total
-		ret.StartOffset = page.start
-		return connect.NewResponse(ret), nil
+// buildEmptyPageResponse creates a response for an empty page.
+func buildEmptyPageResponse(page pageInfo) *apiv1.GetActionLogsResponse {
+	return &apiv1.GetActionLogsResponse{
+		CountRemaining: 0,
+		PageSize:       page.size,
+		TotalCount:     page.total,
+		StartOffset:    page.start,
 	}
-	// Newest-first slicing: compute reversed indices
+}
+
+// calculateReversedIndices computes the reversed indices for newest-first pagination.
+func calculateReversedIndices(page pageInfo, filteredLen int) (int64, int64) {
 	startIdx := page.total - page.end
 	endIdx := page.total - page.start
 	if startIdx < 0 {
 		startIdx = 0
 	}
-	if endIdx > int64(len(filtered)) {
-		endIdx = int64(len(filtered))
+	if endIdx > int64(filteredLen) {
+		endIdx = int64(filteredLen)
 	}
+	return startIdx, endIdx
+}
+
+// buildActionLogsResponse builds the response with paginated log entries.
+func (api *oliveTinAPI) buildActionLogsResponse(filtered []*executor.InternalLogEntry, page pageInfo, user *acl.AuthenticatedUser) *apiv1.GetActionLogsResponse {
+	startIdx, endIdx := calculateReversedIndices(page, len(filtered))
+	ret := &apiv1.GetActionLogsResponse{}
 	for _, le := range filtered[startIdx:endIdx] {
 		ret.Logs = append(ret.Logs, api.internalLogEntryToPb(le, user))
 	}
-	// Entries older than the returned newest page
 	ret.CountRemaining = page.start
 	ret.PageSize = page.size
 	ret.TotalCount = page.total
 	ret.StartOffset = page.start
-	return connect.NewResponse(ret), nil
+	return ret
+}
+
+func (api *oliveTinAPI) GetActionLogs(ctx ctx.Context, req *connect.Request[apiv1.GetActionLogsRequest]) (*connect.Response[apiv1.GetActionLogsResponse], error) {
+	user := acl.UserFromContext(ctx, req, api.cfg)
+
+	if err := api.checkDashboardAccess(user); err != nil {
+		return nil, err
+	}
+
+	filtered := api.filterLogsByACL(api.executor.GetLogsByActionId(req.Msg.ActionId), user)
+	page := paginate(int64(len(filtered)), api.cfg.LogHistoryPageSize, req.Msg.StartOffset)
+	if page.empty {
+		return connect.NewResponse(buildEmptyPageResponse(page)), nil
+	}
+
+	return connect.NewResponse(api.buildActionLogsResponse(filtered, page, user)), nil
 }
 
 func (api *oliveTinAPI) pbLogsFiltered(entries []*executor.InternalLogEntry, user *acl.AuthenticatedUser) []*apiv1.LogEntry {
 	out := make([]*apiv1.LogEntry, 0, len(entries))
 	for _, e := range entries {
-		if e == nil || e.Binding == nil || e.Binding.Action == nil {
+		if !isValidLogEntry(e) {
 			continue
 		}
-		if acl.IsAllowedLogs(api.cfg, user, e.Binding.Action) {
+		if api.isLogEntryAllowed(e, user) {
 			out = append(out, api.internalLogEntryToPb(e, user))
 		}
 	}
@@ -517,10 +543,10 @@ func (api *oliveTinAPI) pbLogsFiltered(entries []*executor.InternalLogEntry, use
 func (api *oliveTinAPI) filterLogsByACL(entries []*executor.InternalLogEntry, user *acl.AuthenticatedUser) []*executor.InternalLogEntry {
 	filtered := make([]*executor.InternalLogEntry, 0, len(entries))
 	for _, e := range entries {
-		if e == nil || e.Binding == nil || e.Binding.Action == nil {
+		if !isValidLogEntry(e) {
 			continue
 		}
-		if acl.IsAllowedLogs(api.cfg, user, e.Binding.Action) {
+		if api.isLogEntryAllowed(e, user) {
 			filtered = append(filtered, e)
 		}
 	}

+ 70 - 38
service/internal/config/config_reloader.go

@@ -72,16 +72,29 @@ func afterLoadFinalize(cfg *Config, configPath string) {
 	}
 }
 
+// buildIncludePath constructs the full path to the include directory.
+func buildIncludePath(k *koanf.Koanf, baseConfigPath string) string {
+	relativeIncludePath := k.String("include")
+	return filepath.Join(filepath.Dir(baseConfigPath), relativeIncludePath)
+}
+
+// loadAndMergeYamlFiles loads and merges all YAML files from the include directory.
+func loadAndMergeYamlFiles(k *koanf.Koanf, includePath string, yamlFiles []string) {
+	sort.Strings(yamlFiles)
+	for _, filename := range yamlFiles {
+		loadAndMergeIncludedFile(k, includePath, filename)
+	}
+	log.Infof("Finished loading %d included config file(s)", len(yamlFiles))
+}
+
 // loadIncludedConfigsFromDir loads configuration files from an include directory and merges them
 func loadIncludedConfigsFromDir(k *koanf.Koanf, baseConfigPath string) {
 	relativeIncludePath := k.String("include")
-
 	if relativeIncludePath == "" {
 		return
 	}
 
-	includePath := filepath.Join(filepath.Dir(baseConfigPath), relativeIncludePath)
-
+	includePath := buildIncludePath(k, baseConfigPath)
 	log.WithFields(log.Fields{
 		"includePath": includePath,
 	}).Infof("Loading included configs from dir")
@@ -91,42 +104,58 @@ func loadIncludedConfigsFromDir(k *koanf.Koanf, baseConfigPath string) {
 		return
 	}
 
-	sort.Strings(yamlFiles)
-	for _, filename := range yamlFiles {
-		loadAndMergeIncludedFile(k, includePath, filename)
-	}
-
-	log.Infof("Finished loading %d included config file(s)", len(yamlFiles))
+	loadAndMergeYamlFiles(k, includePath, yamlFiles)
 }
 
-func listYamlFiles(includePath string) ([]string, bool) {
+// validateIncludeDirectory checks if the given path exists and is a directory.
+func validateIncludeDirectory(includePath string) bool {
 	dirInfo, err := os.Stat(includePath)
 	if err != nil {
 		log.Warnf("Include directory not found: %s", includePath)
-		return nil, false
+		return false
 	}
 	if !dirInfo.IsDir() {
 		log.Warnf("Include path is not a directory: %s", includePath)
-		return nil, false
-	}
-	entries, err := os.ReadDir(includePath)
-	if err != nil {
-		log.Errorf("Error reading include directory: %v", err)
-		return nil, false
+		return false
 	}
+	return true
+}
+
+// isYamlFile checks if a filename has a YAML extension.
+func isYamlFile(name string) bool {
+	return strings.HasSuffix(name, ".yml") || strings.HasSuffix(name, ".yaml")
+}
+
+// filterYamlFilesFromEntries extracts YAML file names from directory entries.
+func filterYamlFilesFromEntries(entries []os.DirEntry) []string {
 	var yamlFiles []string
 	for _, entry := range entries {
 		if entry.IsDir() {
 			continue
 		}
-		name := entry.Name()
-		if strings.HasSuffix(name, ".yml") || strings.HasSuffix(name, ".yaml") {
-			yamlFiles = append(yamlFiles, name)
+		if isYamlFile(entry.Name()) {
+			yamlFiles = append(yamlFiles, entry.Name())
 		}
 	}
+	return yamlFiles
+}
+
+func listYamlFiles(includePath string) ([]string, bool) {
+	if !validateIncludeDirectory(includePath) {
+		return nil, false
+	}
+
+	entries, err := os.ReadDir(includePath)
+	if err != nil {
+		log.Errorf("Error reading include directory: %v", err)
+		return nil, false
+	}
+
+	yamlFiles := filterYamlFilesFromEntries(entries)
 	if len(yamlFiles) == 0 {
 		log.Infof("No YAML files found in include directory: %s", includePath)
 	}
+
 	return yamlFiles, true
 }
 
@@ -143,27 +172,30 @@ func loadAndMergeIncludedFile(k *koanf.Koanf, includePath, filename string) {
 	}).Info("Successfully loaded included config file")
 }
 
+// mergeActionsWhenBothExist merges actions when both src and dest have actions.
+func mergeActionsWhenBothExist(srcActions interface{}, destActions interface{}, dest map[string]interface{}) {
+	srcSlice, ok1 := srcActions.([]interface{})
+	destSlice, ok2 := destActions.([]interface{})
+	if ok1 && ok2 {
+		dest["actions"] = append(destSlice, srcSlice...)
+	} else {
+		dest["actions"] = srcActions
+	}
+}
+
+// mergeActionsFromSource merges actions from source into destination.
+func mergeActionsFromSource(srcActions interface{}, dest map[string]interface{}) {
+	if destActions, ok := dest["actions"]; ok {
+		mergeActionsWhenBothExist(srcActions, destActions, dest)
+	} else {
+		dest["actions"] = srcActions
+	}
+}
+
 func mergeFunc(src map[string]interface{}, dest map[string]interface{}) error {
-	// Handle actions merging - koanf provides []interface{} not []*Action
-	// Merge src (new) into dest (existing) by appending src's actions to dest's actions
 	if srcActions, ok := src["actions"]; ok {
-		if destActions, ok := dest["actions"]; ok {
-			// Both have actions - append src to dest
-			srcSlice, ok1 := srcActions.([]interface{})
-			destSlice, ok2 := destActions.([]interface{})
-			if ok1 && ok2 {
-				dest["actions"] = append(destSlice, srcSlice...)
-			} else {
-				// Fallback: if types don't match, just use src
-				dest["actions"] = srcActions
-			}
-		} else {
-			// dest doesn't have actions, so use src's actions
-			dest["actions"] = srcActions
-		}
+		mergeActionsFromSource(srcActions, dest)
 	}
-	// If src doesn't have actions, leave dest unchanged
-
 	return nil
 }
 

+ 31 - 50
service/internal/config/config_reloader_test.go

@@ -90,63 +90,44 @@ var envConfigTests = []struct {
 }
 
 func TestEnvInConfig(t *testing.T) {
-    for _, tt := range envConfigTests {
-        cfg := DefaultConfig()
-        setIfNotEmpty("INPUT", tt.input)
-        processed := processYamlWithEnv(tt.yaml)
-        k, err := loadKoanf(processed)
-        if err != nil {
-            t.Errorf("Error loading YAML: %v", err)
-            continue
-        }
-        if err := k.Unmarshal(".", cfg); err != nil {
-            t.Errorf("Error unmarshalling config: %v", err)
-            continue
-        }
-        manualAssigns(k, cfg)
-        field := tt.selector(cfg)
-        assert.Equal(t, tt.output, field, "Unmarshaled config field doesn't match expected value: env=\"%s\"", tt.input)
-        os.Unsetenv("INPUT")
-    }
+	for _, tt := range envConfigTests {
+		cfg := DefaultConfig()
+		setIfNotEmpty("INPUT", tt.input)
+		processed := processYamlWithEnv(tt.yaml)
+		k, err := loadKoanf(processed)
+		if err != nil {
+			t.Errorf("Error loading YAML: %v", err)
+			continue
+		}
+		if err := k.Unmarshal(".", cfg); err != nil {
+			t.Errorf("Error unmarshalling config: %v", err)
+			continue
+		}
+		field := tt.selector(cfg)
+		assert.Equal(t, tt.output, field, "Unmarshaled config field doesn't match expected value: env=\"%s\"", tt.input)
+		os.Unsetenv("INPUT")
+	}
 }
 
 func setIfNotEmpty(key, val string) {
-    if val != "" {
-        os.Setenv(key, val)
-    }
+	if val != "" {
+		os.Setenv(key, val)
+	}
 }
 
 func processYamlWithEnv(content string) string {
-    return envRegex.ReplaceAllStringFunc(content, func(match string) string {
-        submatches := envRegex.FindStringSubmatch(match)
-        key := submatches[1]
-        val, _ := os.LookupEnv(key)
-        return val
-    })
+	return envRegex.ReplaceAllStringFunc(content, func(match string) string {
+		submatches := envRegex.FindStringSubmatch(match)
+		key := submatches[1]
+		val, _ := os.LookupEnv(key)
+		return val
+	})
 }
 
 func loadKoanf(processed string) (*koanf.Koanf, error) {
-    k := koanf.New(".")
-    if err := k.Load(rawbytes.Provider([]byte(processed)), yaml.Parser()); err != nil {
-        return nil, err
-    }
-    return k, nil
-}
-
-func manualAssigns(k *koanf.Koanf, cfg *Config) {
-    if k.Exists("PageTitle") {
-        cfg.PageTitle = k.String("PageTitle")
-    }
-    if k.Exists("CheckForUpdates") {
-        cfg.CheckForUpdates = k.Bool("CheckForUpdates")
-    }
-    if k.Exists("LogHistoryPageSize") {
-        cfg.LogHistoryPageSize = k.Int64("LogHistoryPageSize")
-    }
-    if k.Exists("actions") {
-        var actions []*Action
-        if err := k.Unmarshal("actions", &actions); err == nil {
-            cfg.Actions = actions
-        }
-    }
+	k := koanf.New(".")
+	if err := k.Load(rawbytes.Provider([]byte(processed)), yaml.Parser()); err != nil {
+		return nil, err
+	}
+	return k, nil
 }

+ 16 - 7
service/internal/executor/arguments.go

@@ -42,13 +42,8 @@ func parseCommandForReplacements(shellCommand string, values map[string]string,
 	return shellCommand, nil
 }
 
-func parseActionExec(values map[string]string, action *config.Action, entity *entities.Entity) ([]string, error) {
-	if action == nil {
-		return nil, fmt.Errorf("action is nil")
-	}
-	if err := validateArguments(values, action); err != nil {
-		return nil, err
-	}
+// parseExecArray parses all exec arguments in the action.
+func parseExecArray(action *config.Action, values map[string]string, entity *entities.Entity) ([]string, error) {
 	parsed := make([]string, len(action.Exec))
 	for i, a := range action.Exec {
 		out, err := parseSingleExec(a, values, entity)
@@ -57,6 +52,20 @@ func parseActionExec(values map[string]string, action *config.Action, entity *en
 		}
 		parsed[i] = out
 	}
+	return parsed, nil
+}
+
+func parseActionExec(values map[string]string, action *config.Action, entity *entities.Entity) ([]string, error) {
+	if action == nil {
+		return nil, fmt.Errorf("action is nil")
+	}
+	if err := validateArguments(values, action); err != nil {
+		return nil, err
+	}
+	parsed, err := parseExecArray(action, values, entity)
+	if err != nil {
+		return nil, err
+	}
 	logParsedExec(action, parsed, values)
 	return parsed, nil
 }

+ 28 - 9
service/internal/executor/executor.go

@@ -224,24 +224,39 @@ func (e *Executor) GetLogTrackingIds(startOffset int64, pageCount int64) ([]*Int
 	return trackingIds, pagingResult
 }
 
-// GetLogTrackingIdsACL returns logs filtered by ACL visibility for the user and
-// paginated correctly based on the filtered set.
-func (e *Executor) GetLogTrackingIdsACL(cfg *config.Config, user *acl.AuthenticatedUser, startOffset int64, pageCount int64) ([]*InternalLogEntry, *PagingResult) {
-	// Build filtered list in reverse-chronological order (matching GetLogTrackingIds)
+// isValidLogEntryForACL checks if a log entry has all required fields for ACL checking.
+func isValidLogEntryForACL(entry *InternalLogEntry) bool {
+	return entry != nil && entry.Binding != nil && entry.Binding.Action != nil
+}
+
+// isLogEntryAllowedByACL checks if a log entry is allowed to be viewed by the user.
+func isLogEntryAllowedByACL(cfg *config.Config, user *acl.AuthenticatedUser, entry *InternalLogEntry) bool {
+	return acl.IsAllowedLogs(cfg, user, entry.Binding.Action)
+}
+
+// filterLogsByACL builds a filtered list of logs in reverse-chronological order
+// that are visible to the user based on ACL rules.
+func (e *Executor) filterLogsByACL(cfg *config.Config, user *acl.AuthenticatedUser) []*InternalLogEntry {
 	filtered := make([]*InternalLogEntry, 0)
 
 	e.logmutex.RLock()
 	for i := len(e.logsTrackingIdsByDate) - 1; i >= 0; i-- {
 		entry := e.logs[e.logsTrackingIdsByDate[i]]
-		if entry == nil || entry.Binding == nil || entry.Binding.Action == nil {
+		if !isValidLogEntryForACL(entry) {
 			continue
 		}
-		if acl.IsAllowedLogs(cfg, user, entry.Binding.Action) {
+		if isLogEntryAllowedByACL(cfg, user, entry) {
 			filtered = append(filtered, entry)
 		}
 	}
 	e.logmutex.RUnlock()
 
+	return filtered
+}
+
+// paginateFilteredLogs applies pagination to a filtered list of logs and returns
+// the paginated results along with pagination metadata.
+func paginateFilteredLogs(filtered []*InternalLogEntry, startOffset int64, pageCount int64) ([]*InternalLogEntry, *PagingResult) {
 	total := int64(len(filtered))
 	paging := &PagingResult{PageSize: pageCount, TotalCount: total, StartOffset: startOffset}
 
@@ -250,13 +265,10 @@ func (e *Executor) GetLogTrackingIdsACL(cfg *config.Config, user *acl.Authentica
 		return []*InternalLogEntry{}, paging
 	}
 
-	// Compute start/end indices using the same semantics as GetLogTrackingIds,
-	// but over the filtered slice
 	startIndex := getPagingStartIndex(startOffset, total)
 	pageCount = min(total, pageCount)
 	endIndex := max(0, (startIndex-pageCount)+1)
 
-	// Slice is inclusive of both ends in original logic, so iterate and collect
 	out := make([]*InternalLogEntry, 0, pageCount)
 	for i := endIndex; i <= startIndex && i < int64(len(filtered)); i++ {
 		out = append(out, filtered[i])
@@ -266,6 +278,13 @@ func (e *Executor) GetLogTrackingIdsACL(cfg *config.Config, user *acl.Authentica
 	return out, paging
 }
 
+// GetLogTrackingIdsACL returns logs filtered by ACL visibility for the user and
+// paginated correctly based on the filtered set.
+func (e *Executor) GetLogTrackingIdsACL(cfg *config.Config, user *acl.AuthenticatedUser, startOffset int64, pageCount int64) ([]*InternalLogEntry, *PagingResult) {
+	filtered := e.filterLogsByACL(cfg, user)
+	return paginateFilteredLogs(filtered, startOffset, pageCount)
+}
+
 func (e *Executor) GetLog(trackingID string) (*InternalLogEntry, bool) {
 	e.logmutex.RLock()