Merge pull request #346 from canonical/IAM-931

Improve validation error messages

internal/validation/registry_test.go

@@ -234,13 +234,9 @@ func TestNewValidationError(t *testing.T) {
 		t.Fatalf("response message does not match expected")
 	}
 
-	expectedData := map[string][]string{
-		"first_name": {
-			"value '' fails validation of type `required`",
-		},
-	}
+	expectedData := map[string][]string{"first_name": {"Missing required field 'first_name'"}}
 
 	if !reflect.DeepEqual(expectedData, response.Data) {
-		t.Fatalf("response data does not match expected validation errors")
+		t.Fatalf("Expected '%s', got '%s'", expectedData, response.Data)
 	}
 }

internal/validation/types.go

@@ -40,15 +40,22 @@ func buildErrorData(errors validator.ValidationErrors) map[string][]string {
 	for _, e := range errors {
 		field := e.Field()
 
+		var errMsg string
+		switch e.Tag() {
+		case "required":
+			errMsg = fmt.Sprintf("Missing required field '%s'", field)
+		case "oneof", "httpmethod":
+			errMsg = fmt.Sprintf("Field '%s' must be one of '%s'", field, e.Param())
+		default:
+			errMsg = e.Error()
+		}
+
 		failures, ok := failedValidations[field]
 		if !ok {
 			failedValidations[field] = make([]string, 0)
 		}
 
-		failures = append(
-			failures,
-			fmt.Sprintf("value '%s' fails validation of type `%s`", e.Value(), e.Tag()),
-		)
+		failures = append(failures, errMsg)
 		failedValidations[field] = failures
 	}
 

pkg/clients/handlers.go

@@ -202,7 +202,7 @@ func NewAPI(service ServiceInterface, logger logging.LoggerInterface) *API {
 	a.apiKey = "clients"
 
 	a.service = service
-	a.payloadValidator = NewClientsPayloadValidator(a.apiKey)
+	a.payloadValidator = NewClientsPayloadValidator(a.apiKey, logger)
 	a.logger = logger
 
 	return a

pkg/clients/validation.go

@@ -6,12 +6,14 @@ package clients
 import (
 	"context"
 	"encoding/json"
+	"fmt"
 	"net/http"
 	"strings"
 
 	"github.com/go-playground/validator/v10"
 	client "github.com/ory/hydra-client-go/v2"
 
+	"github.com/canonical/identity-platform-admin-ui/internal/logging"
 	"github.com/canonical/identity-platform-admin-ui/internal/validation"
 )
 
@@ -32,6 +34,8 @@ var (
 type PayloadValidator struct {
 	apiKey    string
 	validator *validator.Validate
+
+	logger logging.LoggerInterface
 }
 
 func (p *PayloadValidator) setupValidator() {
@@ -49,7 +53,9 @@ func (p *PayloadValidator) Validate(ctx context.Context, method, endpoint string
 	if p.isCreateClient(method, endpoint) || p.isUpdateClient(method, endpoint) {
 		clientRequest := new(client.OAuth2Client)
 		if err := json.Unmarshal(body, clientRequest); err != nil {
-			return ctx, nil, err
+			// TODO(nsklikas): evaluate usage of logger
+			p.logger.Error("Json parsing error: ", err)
+			return ctx, nil, fmt.Errorf("failed to parse JSON body")
 		}
 
 		err = p.validator.Struct(clientRequest)
@@ -76,9 +82,10 @@ func (p *PayloadValidator) isUpdateClient(method string, endpoint string) bool {
 	return method == http.MethodPut && strings.HasPrefix(endpoint, "/")
 }
 
-func NewClientsPayloadValidator(apiKey string) *PayloadValidator {
+func NewClientsPayloadValidator(apiKey string, logger logging.LoggerInterface) *PayloadValidator {
 	p := new(PayloadValidator)
 	p.apiKey = apiKey
+	p.logger = logger
 	p.validator = validation.NewValidator()
 
 	p.setupValidator()

pkg/groups/handlers.go

@@ -724,7 +724,7 @@ func NewAPI(service ServiceInterface, tracer tracing.TracingInterface, monitor m
 
 	a.apiKey = "groups"
 	a.service = service
-	a.payloadValidator = NewGroupsPayloadValidator(a.apiKey)
+	a.payloadValidator = NewGroupsPayloadValidator(a.apiKey, logger, tracer)
 	a.logger = logger
 	a.tracer = tracer
 	a.monitor = monitor

pkg/groups/validation.go

@@ -6,18 +6,24 @@ package groups
 import (
 	"context"
 	"encoding/json"
+	"fmt"
 	"net/http"
 	"strings"
 
 	"github.com/go-playground/validator/v10"
 	"github.com/go-playground/validator/v10/non-standard/validators"
 
+	"github.com/canonical/identity-platform-admin-ui/internal/logging"
+	"github.com/canonical/identity-platform-admin-ui/internal/tracing"
 	"github.com/canonical/identity-platform-admin-ui/internal/validation"
 )
 
 type PayloadValidator struct {
 	apiKey    string
 	validator *validator.Validate
+
+	logger logging.LoggerInterface
+	tracer tracing.TracingInterface
 }
 
 func (p *PayloadValidator) setupValidator() {
@@ -35,7 +41,8 @@ func (p *PayloadValidator) Validate(ctx context.Context, method, endpoint string
 	if p.isCreateGroup(method, endpoint) {
 		group := new(Group)
 		if err := json.Unmarshal(body, group); err != nil {
-			return ctx, nil, err
+			p.logger.Error("Json parsing error: ", err)
+			return ctx, nil, fmt.Errorf("failed to parse JSON body")
 		}
 
 		err = p.validator.Struct(group)
@@ -50,7 +57,8 @@ func (p *PayloadValidator) Validate(ctx context.Context, method, endpoint string
 	if p.isAssignRoles(method, endpoint) {
 		updateRoles := new(UpdateRolesRequest)
 		if err := json.Unmarshal(body, updateRoles); err != nil {
-			return ctx, nil, err
+			p.logger.Error("Json parsing error: ", err)
+			return ctx, nil, fmt.Errorf("failed to parse JSON body")
 		}
 
 		err = p.validator.Struct(updateRoles)
@@ -60,7 +68,8 @@ func (p *PayloadValidator) Validate(ctx context.Context, method, endpoint string
 	if p.isAssignPermissions(method, endpoint) {
 		updatePermissions := new(UpdatePermissionsRequest)
 		if err := json.Unmarshal(body, updatePermissions); err != nil {
-			return ctx, nil, err
+			p.logger.Error("Json parsing error: ", err)
+			return ctx, nil, fmt.Errorf("failed to parse JSON body")
 		}
 
 		err = p.validator.Struct(updatePermissions)
@@ -70,7 +79,8 @@ func (p *PayloadValidator) Validate(ctx context.Context, method, endpoint string
 	if p.isAssignIdentities(method, endpoint) {
 		updateIdentities := new(UpdateIdentitiesRequest)
 		if err := json.Unmarshal(body, updateIdentities); err != nil {
-			return ctx, nil, err
+			p.logger.Error("Json parsing error: ", err)
+			return ctx, nil, fmt.Errorf("failed to parse JSON body")
 		}
 
 		err = p.validator.Struct(updateIdentities)
@@ -108,12 +118,14 @@ func (p *PayloadValidator) isAssignIdentities(method, endpoint string) bool {
 	return method == http.MethodPatch && strings.HasSuffix(endpoint, "/identities")
 }
 
-func NewGroupsPayloadValidator(apiKey string) *PayloadValidator {
+func NewGroupsPayloadValidator(apiKey string, logger logging.LoggerInterface, tracer tracing.TracingInterface) *PayloadValidator {
 	p := new(PayloadValidator)
 	p.apiKey = apiKey
 	p.validator = validation.NewValidator()
 
 	p.setupValidator()
 
+	p.logger = logger
+	p.tracer = tracer
 	return p
 }

pkg/identities/handlers.go

@@ -269,7 +269,7 @@ func NewAPI(service ServiceInterface, logger logging.LoggerInterface) *API {
 	a.apiKey = "identities"
 	a.service = service
 
-	a.payloadValidator = NewIdentitiesPayloadValidator(a.apiKey)
+	a.payloadValidator = NewIdentitiesPayloadValidator(a.apiKey, logger)
 
 	a.logger = logger
 

pkg/identities/validation.go

@@ -6,11 +6,13 @@ package identities
 import (
 	"context"
 	"encoding/json"
+	"fmt"
 	"net/http"
 	"strings"
 
 	"github.com/go-playground/validator/v10"
 
+	"github.com/canonical/identity-platform-admin-ui/internal/logging"
 	"github.com/canonical/identity-platform-admin-ui/internal/validation"
 )
 
@@ -29,6 +31,8 @@ var (
 type PayloadValidator struct {
 	apiKey    string
 	validator *validator.Validate
+
+	logger logging.LoggerInterface
 }
 
 func (p *PayloadValidator) setupValidator() {
@@ -50,7 +54,8 @@ func (p *PayloadValidator) Validate(ctx context.Context, method, endpoint string
 	if p.isCreateIdentity(method, endpoint) {
 		createIdentity := new(CreateIdentityRequest)
 		if err := json.Unmarshal(body, createIdentity); err != nil {
-			return ctx, nil, err
+			p.logger.Error("Json parsing error: ", err)
+			return ctx, nil, fmt.Errorf("failed to parse JSON body")
 		}
 
 		err = p.validator.Struct(createIdentity)
@@ -59,7 +64,8 @@ func (p *PayloadValidator) Validate(ctx context.Context, method, endpoint string
 	} else if p.isUpdateIdentity(method, endpoint) {
 		updateIdentity := new(UpdateIdentityRequest)
 		if err := json.Unmarshal(body, updateIdentity); err != nil {
-			return ctx, nil, err
+			p.logger.Error("Json parsing error: ", err)
+			return ctx, nil, fmt.Errorf("failed to parse JSON body")
 		}
 
 		err = p.validator.Struct(updateIdentity)
@@ -86,9 +92,10 @@ func (p *PayloadValidator) isUpdateIdentity(method, endpoint string) bool {
 	return strings.HasPrefix(endpoint, "/") && method == http.MethodPut
 }
 
-func NewIdentitiesPayloadValidator(apiKey string) *PayloadValidator {
+func NewIdentitiesPayloadValidator(apiKey string, logger logging.LoggerInterface) *PayloadValidator {
 	p := new(PayloadValidator)
 	p.apiKey = apiKey
+	p.logger = logger
 	p.validator = validation.NewValidator()
 
 	p.setupValidator()

pkg/idp/handlers.go

@@ -256,7 +256,7 @@ func NewAPI(service ServiceInterface, logger logging.LoggerInterface) *API {
 	a := new(API)
 	a.apiKey = "idps"
 
-	a.payloadValidator = NewIdPPayloadValidator(a.apiKey)
+	a.payloadValidator = NewIdPPayloadValidator(a.apiKey, logger)
 	a.service = service
 	a.logger = logger
 

pkg/idp/validation.go

@@ -6,17 +6,21 @@ package idp
 import (
 	"context"
 	"encoding/json"
+	"fmt"
 	"net/http"
 	"strings"
 
 	"github.com/go-playground/validator/v10"
 
+	"github.com/canonical/identity-platform-admin-ui/internal/logging"
 	"github.com/canonical/identity-platform-admin-ui/internal/validation"
 )
 
 type PayloadValidator struct {
 	apiKey    string
 	validator *validator.Validate
+
+	logger logging.LoggerInterface
 }
 
 func (p *PayloadValidator) setupValidator() {
@@ -35,7 +39,8 @@ func (p *PayloadValidator) Validate(ctx context.Context, method, endpoint string
 	if p.isCreateIdP(method, endpoint) || p.isPartialUpdateIdP(method, endpoint) {
 		conf := new(Configuration)
 		if err := json.Unmarshal(body, conf); err != nil {
-			return ctx, nil, err
+			p.logger.Error("Json parsing error: ", err)
+			return ctx, nil, fmt.Errorf("failed to parse JSON body")
 		}
 
 		err = p.validator.Struct(conf)
@@ -61,9 +66,10 @@ func (p *PayloadValidator) isPartialUpdateIdP(method, endpoint string) bool {
 	return method == http.MethodPatch && strings.HasPrefix(endpoint, "/")
 }
 
-func NewIdPPayloadValidator(apiKey string) *PayloadValidator {
+func NewIdPPayloadValidator(apiKey string, logger logging.LoggerInterface) *PayloadValidator {
 	p := new(PayloadValidator)
 	p.apiKey = apiKey
+	p.logger = logger
 	p.validator = validation.NewValidator()
 
 	p.setupValidator()

pkg/roles/handlers.go

@@ -495,7 +495,7 @@ func NewAPI(service ServiceInterface, tracer tracing.TracingInterface, monitor m
 	a := new(API)
 
 	a.service = service
-	a.payloadValidator = NewRolesPayloadValidator(a.apiKey)
+	a.payloadValidator = NewRolesPayloadValidator(a.apiKey, tracer, monitor, logger)
 	a.logger = logger
 	a.tracer = tracer
 	a.monitor = monitor

pkg/roles/validation.go

@@ -6,18 +6,26 @@ package roles
 import (
 	"context"
 	"encoding/json"
+	"fmt"
 	"net/http"
 	"strings"
 
 	"github.com/go-playground/validator/v10"
 	"github.com/go-playground/validator/v10/non-standard/validators"
 
+	"github.com/canonical/identity-platform-admin-ui/internal/logging"
+	"github.com/canonical/identity-platform-admin-ui/internal/monitoring"
+	"github.com/canonical/identity-platform-admin-ui/internal/tracing"
 	"github.com/canonical/identity-platform-admin-ui/internal/validation"
 )
 
 type PayloadValidator struct {
 	apiKey    string
 	validator *validator.Validate
+
+	tracer  tracing.TracingInterface
+	monitor monitoring.MonitorInterface
+	logger  logging.LoggerInterface
 }
 
 func (p *PayloadValidator) setupValidator() {
@@ -35,7 +43,8 @@ func (p *PayloadValidator) Validate(ctx context.Context, method, endpoint string
 	if p.isCreateRole(method, endpoint) {
 		roleRequest := new(Role)
 		if err := json.Unmarshal(body, roleRequest); err != nil {
-			return ctx, nil, err
+			p.logger.Error("Json parsing error: ", err)
+			return ctx, nil, fmt.Errorf("failed to parse JSON body")
 		}
 
 		err = p.validator.Struct(roleRequest)
@@ -50,7 +59,8 @@ func (p *PayloadValidator) Validate(ctx context.Context, method, endpoint string
 	if p.isAssignPermissions(method, endpoint) {
 		updatePermissions := new(UpdatePermissionsRequest)
 		if err := json.Unmarshal(body, updatePermissions); err != nil {
-			return ctx, nil, err
+			p.logger.Error("Json parsing error: ", err)
+			return ctx, nil, fmt.Errorf("failed to parse JSON body")
 		}
 
 		err = p.validator.Struct(updatePermissions)
@@ -80,11 +90,15 @@ func (p *PayloadValidator) isAssignPermissions(method, endpoint string) bool {
 	return method == http.MethodPatch && strings.HasSuffix(endpoint, "/entitlements")
 }
 
-func NewRolesPayloadValidator(apiKey string) *PayloadValidator {
+func NewRolesPayloadValidator(apiKey string, tracer tracing.TracingInterface, monitor monitoring.MonitorInterface, logger logging.LoggerInterface) *PayloadValidator {
 	p := new(PayloadValidator)
 	p.apiKey = apiKey
 	p.validator = validation.NewValidator()
 
+	p.tracer = tracer
+	p.monitor = monitor
+	p.logger = logger
+
 	p.setupValidator()
 
 	return p

pkg/rules/handlers.go

@@ -305,7 +305,7 @@ func (a *API) handleRemove(w http.ResponseWriter, r *http.Request) {
 func NewAPI(service ServiceInterface, logger logging.LoggerInterface) *API {
 	a := new(API)
 	a.apiKey = "rules"
-	a.payloadValidator = NewRulesPayloadValidator(a.apiKey)
+	a.payloadValidator = NewRulesPayloadValidator(a.apiKey, logger)
 	a.service = service
 	a.logger = logger