feat: Improve account loading errors (#1669)

* refactor: Improve account loading error messages

* fix: Fix typos in error messages

* refactor: Improve readability and error messages

* refactor: Refactor persistence to account transform

* refactor: Move code

pkg/persistence/account/loader/load.go

@@ -18,13 +18,15 @@ package loader
 
 import (
 	"fmt"
+
+	"github.com/spf13/afero"
+	"gopkg.in/yaml.v2"
+
 	"github.com/dynatrace/dynatrace-configuration-as-code/v2/internal/files"
 	"github.com/dynatrace/dynatrace-configuration-as-code/v2/internal/log"
 	"github.com/dynatrace/dynatrace-configuration-as-code/v2/internal/log/field"
 	"github.com/dynatrace/dynatrace-configuration-as-code/v2/pkg/account"
 	persistence "github.com/dynatrace/dynatrace-configuration-as-code/v2/pkg/persistence/account/internal/types"
-	"github.com/spf13/afero"
-	"gopkg.in/yaml.v2"
 )
 
 // Load loads account management resources from YAML configuration files
@@ -34,16 +36,16 @@ import (
 //  2. validates the loaded data for correct syntax
 //  3. returns the data in the in-memory account.Resources representation
 func Load(fs afero.Fs, rootPath string) (*account.Resources, error) {
-	persisted, err := load(fs, rootPath)
+	persisted, err := findAndLoadResources(fs, rootPath)
 	if err != nil {
-		return nil, fmt.Errorf("failed to load account managment resources from %s: %w", rootPath, err)
+		return nil, fmt.Errorf("failed to load account management resources from %q: %w", rootPath, err)
 	}
 
 	if err := validateReferences(persisted); err != nil {
-		return nil, fmt.Errorf("account managment resources from %s are invalid: %w", rootPath, err)
+		return nil, fmt.Errorf("account management resources from %q are invalid: %w", rootPath, err)
 	}
 
-	return transform(persisted), nil
+	return transformToAccountResources(persisted), nil
 }
 
 // HasAnyAccountKeyDefined checks whether the map has any AM key defined.
@@ -56,8 +58,8 @@ func HasAnyAccountKeyDefined(m map[string]any) bool {
 	return m[persistence.KeyUsers] != nil || m[persistence.KeyGroups] != nil || m[persistence.KeyPolicies] != nil
 }
 
-func load(fs afero.Fs, rootPath string) (*persistence.Resources, error) {
-	resources := &persistence.Resources{
+func findAndLoadResources(fs afero.Fs, rootPath string) (*persistence.Resources, error) {
+	resources := persistence.Resources{
 		Policies: make(map[string]persistence.Policy),
 		Groups:   make(map[string]persistence.Group),
 		Users:    make(map[string]persistence.User),
@@ -71,155 +73,85 @@ func load(fs afero.Fs, rootPath string) (*persistence.Resources, error) {
 	for _, yamlFilePath := range yamlFilePaths {
 		log.WithFields(field.F("file", yamlFilePaths)).Debug("Loading file %q", yamlFilePath)
 
-		bytes, err := afero.ReadFile(fs, yamlFilePath)
+		file, err := loadFile(fs, yamlFilePath)
 		if err != nil {
-			return nil, err
-		}
-
-		var content map[string]any
-		if err := yaml.Unmarshal(bytes, &content); err != nil {
-			return nil, err
-		}
-
-		if _, f := content["configs"]; f {
-			if HasAnyAccountKeyDefined(content) {
-				return nil, fmt.Errorf("failed to parse file %q: %w", yamlFilePath, ErrMixingConfigs)
-			}
-
-			log.WithFields(field.F("file", yamlFilePath)).Warn("File %q appears to be an config file, skipping loading", yamlFilePath)
-			continue
+			return nil, fmt.Errorf("failed to load file %q: %w", yamlFilePath, err)
 		}
 
-		if _, f := content["delete"]; f {
-			if HasAnyAccountKeyDefined(content) {
-				return nil, fmt.Errorf("failed to parse file %q: %w", yamlFilePath, ErrMixingDelete)
-			}
-
-			log.WithFields(field.F("file", yamlFilePath)).Debug("File %q appears to be an delete file, skipping loading", yamlFilePath)
-			continue
+		err = validateFile(*file)
+		if err != nil {
+			return nil, fmt.Errorf("invalid file %q: %w", yamlFilePath, err)
 		}
 
-		var res persistence.File
-		err = yaml.Unmarshal(bytes, &res)
+		err = addResourcesFromFile(resources, *file)
 		if err != nil {
-			return nil, err
+			return nil, fmt.Errorf("failed to add resources from file %q: %w", yamlFilePath, err)
 		}
+	}
+	return &resources, nil
+}
 
-		for _, p := range res.Policies {
-			if err := validatePolicy(p); err != nil {
-				return nil, fmt.Errorf("error in file %q: %w", yamlFilePath, err)
-			}
-			if _, exists := resources.Policies[p.ID]; exists {
-				return nil, fmt.Errorf("found duplicate policy with id %q", p.ID)
-			}
-			resources.Policies[p.ID] = p
-		}
+func loadFile(fs afero.Fs, yamlFilePath string) (*persistence.File, error) {
+	log.WithFields(field.F("file", yamlFilePath)).Debug("Loading file %q", yamlFilePath)
 
-		for _, g := range res.Groups {
-			if err := validateGroup(g); err != nil {
-				return nil, fmt.Errorf("error in file %q: %w", yamlFilePath, err)
-			}
-			if _, exists := resources.Groups[g.ID]; exists {
-				return nil, fmt.Errorf("found duplicate group with id %q", g.ID)
-			}
-			resources.Groups[g.ID] = g
-		}
+	bytes, err := afero.ReadFile(fs, yamlFilePath)
+	if err != nil {
+		return nil, err
+	}
 
-		for _, u := range res.Users {
-			if err := validateUser(u); err != nil {
-				return nil, fmt.Errorf("error in file %q: %w", yamlFilePath, err)
-			}
-			if _, exists := resources.Users[u.Email.Value()]; exists {
-				return nil, fmt.Errorf("found duplicate user with email %q", u.Email)
-			}
-			resources.Users[u.Email.Value()] = u
-		}
+	var content map[string]any
+	if err := yaml.Unmarshal(bytes, &content); err != nil {
+		return nil, err
 	}
-	return resources, nil
-}
 
-func transform(resources *persistence.Resources) *account.Resources {
-	transformLevel := func(level persistence.PolicyLevel) any {
-		switch level.Type {
-		case persistence.PolicyLevelAccount:
-			return account.PolicyLevelAccount{Type: level.Type}
-		case persistence.PolicyLevelEnvironment:
-			return account.PolicyLevelEnvironment{Type: level.Type, Environment: level.Environment}
-		default:
-			panic("unable to convert persistence model")
+	if _, f := content["configs"]; f {
+		if HasAnyAccountKeyDefined(content) {
+			return nil, ErrMixingConfigs
 		}
+
+		log.WithFields(field.F("file", yamlFilePath)).Warn("File %q appears to be an config file, skipping loading", yamlFilePath)
+		return &persistence.File{}, nil
 	}
 
-	transformRefs := func(in []persistence.Reference) []account.Ref {
-		var res []account.Ref
-		for _, el := range in {
-			switch el.Type {
-			case persistence.ReferenceType:
-				res = append(res, account.Reference{Id: el.Id})
-			case "":
-				res = append(res, account.StrReference(el.Value))
-			default:
-				panic("unable to convert persistence model")
-			}
+	if _, f := content["delete"]; f {
+		if HasAnyAccountKeyDefined(content) {
+			return nil, ErrMixingDelete
 		}
-		return res
+
+		log.WithFields(field.F("file", yamlFilePath)).Debug("File %q appears to be an delete file, skipping loading", yamlFilePath)
+		return &persistence.File{}, nil
 	}
 
-	inMemResources := account.Resources{
-		Policies: make(map[account.PolicyId]account.Policy),
-		Groups:   make(map[account.GroupId]account.Group),
-		Users:    make(map[account.UserId]account.User),
+	var file persistence.File
+	err = yaml.Unmarshal(bytes, &file)
+	if err != nil {
+		return nil, err
 	}
-	for id, v := range resources.Policies {
-		inMemResources.Policies[id] = account.Policy{
-			ID:             v.ID,
-			Name:           v.Name,
-			Level:          transformLevel(v.Level),
-			Description:    v.Description,
-			Policy:         v.Policy,
-			OriginObjectID: v.OriginObjectID,
+
+	return &file, err
+}
+
+func addResourcesFromFile(res persistence.Resources, file persistence.File) error {
+	for _, p := range file.Policies {
+		if _, exists := res.Policies[p.ID]; exists {
+			return fmt.Errorf("found duplicate policy with id %q", p.ID)
 		}
+		res.Policies[p.ID] = p
 	}
-	for id, v := range resources.Groups {
-		var acc *account.Account
-		if v.Account != nil {
-			acc = &account.Account{
-				Permissions: v.Account.Permissions,
-				Policies:    transformRefs(v.Account.Policies),
-			}
-		}
-		env := make([]account.Environment, len(v.Environment))
-		for i, e := range v.Environment {
-			env[i] = account.Environment{
-				Name:        e.Name,
-				Permissions: e.Permissions,
-				Policies:    transformRefs(e.Policies),
-			}
-		}
-		mz := make([]account.ManagementZone, len(v.ManagementZone))
-		for i, m := range v.ManagementZone {
-			mz[i] = account.ManagementZone{
-				Environment:    m.Environment,
-				ManagementZone: m.ManagementZone,
-				Permissions:    m.Permissions,
-			}
-		}
-		inMemResources.Groups[id] = account.Group{
-			ID:                       v.ID,
-			Name:                     v.Name,
-			Description:              v.Description,
-			FederatedAttributeValues: v.FederatedAttributeValues,
-			Account:                  acc,
-			Environment:              env,
-			ManagementZone:           mz,
-			OriginObjectID:           v.OriginObjectID,
+
+	for _, g := range file.Groups {
+		if _, exists := res.Groups[g.ID]; exists {
+			return fmt.Errorf("found duplicate group with id %q", g.ID)
 		}
+		res.Groups[g.ID] = g
 	}
-	for id, v := range resources.Users {
-		inMemResources.Users[id] = account.User{
-			Email:  v.Email,
-			Groups: transformRefs(v.Groups),
+
+	for _, u := range file.Users {
+		if _, exists := res.Users[u.Email.Value()]; exists {
+			return fmt.Errorf("found duplicate user with email %q", u.Email)
 		}
+		res.Users[u.Email.Value()] = u
 	}
-	return &inMemResources
+
+	return nil
 }

pkg/persistence/account/loader/transform.go

@@ -0,0 +1,134 @@
+/*
+ * @license
+ * Copyright 2025 Dynatrace LLC
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package loader
+
+import (
+	"github.com/dynatrace/dynatrace-configuration-as-code/v2/pkg/account"
+	persistence "github.com/dynatrace/dynatrace-configuration-as-code/v2/pkg/persistence/account/internal/types"
+)
+
+func transformToAccountResources(resources *persistence.Resources) *account.Resources {
+	return &account.Resources{
+		Policies: transformPolicies(resources.Policies),
+		Groups:   transformGroups(resources.Groups),
+		Users:    transformUsers(resources.Users),
+	}
+}
+
+func transformPolicies(pPolicies map[string]persistence.Policy) map[account.PolicyId]account.Policy {
+	policies := make(map[account.PolicyId]account.Policy, len(pPolicies))
+	for id, v := range pPolicies {
+		policies[id] = account.Policy{
+			ID:             v.ID,
+			Name:           v.Name,
+			Level:          transformLevel(v.Level),
+			Description:    v.Description,
+			Policy:         v.Policy,
+			OriginObjectID: v.OriginObjectID,
+		}
+	}
+	return policies
+}
+
+func transformLevel(pLevel persistence.PolicyLevel) any {
+	switch pLevel.Type {
+	case persistence.PolicyLevelAccount:
+		return account.PolicyLevelAccount{Type: pLevel.Type}
+	case persistence.PolicyLevelEnvironment:
+		return account.PolicyLevelEnvironment{Type: pLevel.Type, Environment: pLevel.Environment}
+	default:
+		panic("unable to convert persistence model")
+	}
+}
+
+func transformGroups(pGroups map[string]persistence.Group) map[account.GroupId]account.Group {
+	groups := make(map[account.GroupId]account.Group, len(pGroups))
+	for id, v := range pGroups {
+		groups[id] = account.Group{
+			ID:                       v.ID,
+			Name:                     v.Name,
+			Description:              v.Description,
+			FederatedAttributeValues: v.FederatedAttributeValues,
+			Account:                  transformAccount(v.Account),
+			Environment:              transformEnvironments(v.Environment),
+			ManagementZone:           transformManagementZones(v.ManagementZone),
+			OriginObjectID:           v.OriginObjectID,
+		}
+	}
+	return groups
+}
+
+func transformAccount(pAccount *persistence.Account) *account.Account {
+	if pAccount == nil {
+		return nil
+	}
+
+	return &account.Account{
+		Permissions: pAccount.Permissions,
+		Policies:    transformReferences(pAccount.Policies),
+	}
+}
+
+func transformEnvironments(pEnvironments []persistence.Environment) []account.Environment {
+	env := make([]account.Environment, len(pEnvironments))
+	for i, e := range pEnvironments {
+		env[i] = account.Environment{
+			Name:        e.Name,
+			Permissions: e.Permissions,
+			Policies:    transformReferences(e.Policies),
+		}
+	}
+	return env
+}
+
+func transformManagementZones(pManagementZones []persistence.ManagementZone) []account.ManagementZone {
+	managementZones := make([]account.ManagementZone, len(pManagementZones))
+	for i, m := range pManagementZones {
+		managementZones[i] = account.ManagementZone{
+			Environment:    m.Environment,
+			ManagementZone: m.ManagementZone,
+			Permissions:    m.Permissions,
+		}
+	}
+	return managementZones
+}
+
+func transformUsers(pUsers map[string]persistence.User) map[account.UserId]account.User {
+	users := make(map[account.UserId]account.User, len(pUsers))
+	for id, v := range pUsers {
+		users[id] = account.User{
+			Email:  v.Email,
+			Groups: transformReferences(v.Groups),
+		}
+	}
+	return users
+}
+
+func transformReferences(pReferences []persistence.Reference) []account.Ref {
+	res := make([]account.Ref, len(pReferences))
+	for i, el := range pReferences {
+		switch el.Type {
+		case persistence.ReferenceType:
+			res[i] = account.Reference{Id: el.Id}
+		case "":
+			res[i] = account.StrReference(el.Value)
+		default:
+			panic("unable to convert persistence model")
+		}
+	}
+	return res
+}