From ea734440de8344256ad0514ecaabb122e7c75bd7 Mon Sep 17 00:00:00 2001 From: Ralf Haferkamp Date: Wed, 27 Sep 2023 12:29:55 +0200 Subject: [PATCH] graph/education: slightly improve error handling and logging - Use var for common errors - Add the addition error message to the Error() output of errorcode.Error - in PatchEducationSchool() use errorcode.RenderError() to turn the errorcode in to the right HTTP Status (instead of return 500 always) --- .../pkg/identity/ldap_education_class_test.go | 10 ++--- .../pkg/identity/ldap_education_school.go | 17 ++++---- .../identity/ldap_education_school_test.go | 8 ++-- .../pkg/identity/ldap_education_user_test.go | 4 +- .../graph/pkg/identity/ldap_group_test.go | 40 +++++-------------- services/graph/pkg/identity/ldap_test.go | 24 +++-------- .../graph/pkg/service/v0/educationschools.go | 2 +- .../pkg/service/v0/errorcode/errorcode.go | 6 ++- 8 files changed, 42 insertions(+), 69 deletions(-) diff --git a/services/graph/pkg/identity/ldap_education_class_test.go b/services/graph/pkg/identity/ldap_education_class_test.go index b8f481b142d..b6f34cc8a3a 100644 --- a/services/graph/pkg/identity/ldap_education_class_test.go +++ b/services/graph/pkg/identity/ldap_education_class_test.go @@ -72,9 +72,7 @@ func TestGetEducationClasses(t *testing.T) { lm.On("Search", mock.Anything).Return(nil, ldap.NewError(ldap.LDAPResultOperationsError, errors.New("mock"))) b, _ := getMockedBackend(lm, lconfig, &logger) _, err := b.GetEducationClasses(context.Background()) - if err == nil || err.Error() != "itemNotFound" { - t.Errorf("Expected 'itemNotFound' got '%s'", err.Error()) - } + assert.ErrorContains(t, err, "itemNotFound:") lm = &mocks.Client{} lm.On("Search", mock.Anything).Return(&ldap.SearchResult{}, nil) @@ -156,7 +154,7 @@ func TestGetEducationClass(t *testing.T) { if tt.expectedItemNotFound { assert.NotNil(t, err) - assert.Equal(t, "itemNotFound", err.Error()) + assert.Equal(t, "itemNotFound: not found", err.Error()) } else { assert.Nil(t, err) assert.Equal(t, "Math", class.GetDisplayName()) @@ -228,7 +226,7 @@ func TestDeleteEducationClass(t *testing.T) { if tt.expectedItemNotFound { lm.AssertNumberOfCalls(t, "Del", 0) assert.NotNil(t, err) - assert.Equal(t, "itemNotFound", err.Error()) + assert.Equal(t, "itemNotFound: not found", err.Error()) } else { assert.Nil(t, err) } @@ -301,7 +299,7 @@ func TestGetEducationClassMembers(t *testing.T) { if tt.expectedItemNotFound { lm.AssertNumberOfCalls(t, "Search", 1) assert.NotNil(t, err) - assert.Equal(t, "itemNotFound", err.Error()) + assert.Equal(t, "itemNotFound: not found", err.Error()) } else { lm.AssertNumberOfCalls(t, "Search", 2) assert.Nil(t, err) diff --git a/services/graph/pkg/identity/ldap_education_school.go b/services/graph/pkg/identity/ldap_education_school.go index 36bf162f953..bbdf3e01840 100644 --- a/services/graph/pkg/identity/ldap_education_school.go +++ b/services/graph/pkg/identity/ldap_education_school.go @@ -49,7 +49,11 @@ const ( const ldapDateFormat = "20060102150405Z0700" -var errNotSet = errors.New("Attribute not set") +var ( + errNotSet = errors.New("Attribute not set") + errSchoolNameExists = errorcode.New(errorcode.NameAlreadyExists, "A school with that name is already present") + errSchoolNumberExists = errorcode.New(errorcode.NameAlreadyExists, "A school with that number is already present") +) func defaultEducationConfig() educationConfig { return educationConfig{ @@ -120,9 +124,8 @@ func (i *LDAP) CreateEducationSchool(ctx context.Context, school libregraph.Educ _, err := i.getSchoolByNumber(school.GetSchoolNumber()) switch err { case nil: - msg := "A school with that number is already present" - logger.Warn().Str("schoolNumber", school.GetSchoolNumber()).Msg(msg) - return nil, errorcode.New(errorcode.NameAlreadyExists, msg) + logger.Debug().Err(errSchoolNumberExists).Str("schoolNumber", school.GetSchoolNumber()).Msg("duplicate school number") + return nil, errSchoolNumberExists case ErrNotFound: break default: @@ -153,7 +156,7 @@ func (i *LDAP) CreateEducationSchool(ctx context.Context, school libregraph.Educ logger.Debug().Err(err).Msg("error adding school") if errors.As(err, &lerr) { if lerr.ResultCode == ldap.LDAPResultEntryAlreadyExists { - err = errorcode.New(errorcode.NameAlreadyExists, "A school with that name is already present") + err = errSchoolNameExists } } return nil, err @@ -228,7 +231,7 @@ func (i *LDAP) updateDisplayName(ctx context.Context, dn string, providedDisplay logger.Debug().Err(err).Msg("error updating school name") if errors.As(err, &lerr) { if lerr.ResultCode == ldap.LDAPResultEntryAlreadyExists { - err = errorcode.New(errorcode.NameAlreadyExists, "A school with that name is already present") + err = errSchoolNameExists } } return err @@ -247,7 +250,7 @@ func (i *LDAP) updateSchoolProperties(ctx context.Context, dn string, currentSch if *updatedSchoolNumber != "" && currentSchool.GetSchoolNumber() != *updatedSchoolNumber { _, err := i.getSchoolByNumber(*updatedSchoolNumber) if err == nil { - return errorcode.New(errorcode.NameAlreadyExists, "A school with that number is already present") + return errSchoolNumberExists } mr.Replace(i.educationConfig.schoolAttributeMap.schoolNumber, []string{*updatedSchoolNumber}) } diff --git a/services/graph/pkg/identity/ldap_education_school_test.go b/services/graph/pkg/identity/ldap_education_school_test.go index 8ce007ca9b1..8c67b93eea2 100644 --- a/services/graph/pkg/identity/ldap_education_school_test.go +++ b/services/graph/pkg/identity/ldap_education_school_test.go @@ -377,7 +377,7 @@ func TestDeleteEducationSchool(t *testing.T) { if tt.expectedItemNotFound { lm.AssertNumberOfCalls(t, "Del", 0) assert.NotNil(t, err) - assert.Equal(t, "itemNotFound", err.Error()) + assert.Equal(t, "itemNotFound: not found", err.Error()) } else { assert.Nil(t, err) } @@ -441,7 +441,7 @@ func TestGetEducationSchool(t *testing.T) { if tt.expectedItemNotFound { assert.NotNil(t, err) - assert.Equal(t, "itemNotFound", err.Error()) + assert.Equal(t, "itemNotFound: not found", err.Error()) } else { assert.Nil(t, err) assert.Equal(t, "Test School", school.GetDisplayName()) @@ -594,7 +594,7 @@ func TestRemoveMemberFromEducationSchool(t *testing.T) { err = b.RemoveUserFromEducationSchool(context.Background(), "abcd-defg", "does-not-exist") lm.AssertNumberOfCalls(t, "Search", 2) assert.NotNil(t, err) - assert.Equal(t, "itemNotFound", err.Error()) + assert.Equal(t, "itemNotFound: not found", err.Error()) err = b.RemoveUserFromEducationSchool(context.Background(), "abcd-defg", "abcd-defg") lm.AssertNumberOfCalls(t, "Search", 4) lm.AssertNumberOfCalls(t, "Modify", 1) @@ -705,7 +705,7 @@ func TestRemoveClassFromEducationSchool(t *testing.T) { err = b.RemoveClassFromEducationSchool(context.Background(), "abcd-defg", "does-not-exist") lm.AssertNumberOfCalls(t, "Search", 2) assert.NotNil(t, err) - assert.Equal(t, "itemNotFound", err.Error()) + assert.Equal(t, "itemNotFound: not found", err.Error()) err = b.RemoveClassFromEducationSchool(context.Background(), "abcd-defg", "abcd-defg") lm.AssertNumberOfCalls(t, "Search", 4) lm.AssertNumberOfCalls(t, "Modify", 1) diff --git a/services/graph/pkg/identity/ldap_education_user_test.go b/services/graph/pkg/identity/ldap_education_user_test.go index 8b0e9fff53f..763f27b4f37 100644 --- a/services/graph/pkg/identity/ldap_education_user_test.go +++ b/services/graph/pkg/identity/ldap_education_user_test.go @@ -139,7 +139,7 @@ func TestDeleteEducationUser(t *testing.T) { lm.AssertNumberOfCalls(t, "Search", 2) lm.AssertNumberOfCalls(t, "Del", 1) assert.NotNil(t, err) - assert.Equal(t, "itemNotFound", err.Error()) + assert.Equal(t, "itemNotFound: not found", err.Error()) } func TestGetEducationUser(t *testing.T) { @@ -157,7 +157,7 @@ func TestGetEducationUser(t *testing.T) { _, err = b.GetEducationUser(context.Background(), "xxxx-xxxx") lm.AssertNumberOfCalls(t, "Search", 2) assert.NotNil(t, err) - assert.Equal(t, "itemNotFound", err.Error()) + assert.Equal(t, "itemNotFound: not found", err.Error()) } func TestGetEducationUsers(t *testing.T) { diff --git a/services/graph/pkg/identity/ldap_group_test.go b/services/graph/pkg/identity/ldap_group_test.go index e3613967b87..79431d4760f 100644 --- a/services/graph/pkg/identity/ldap_group_test.go +++ b/services/graph/pkg/identity/ldap_group_test.go @@ -59,34 +59,22 @@ func TestGetGroup(t *testing.T) { b, _ := getMockedBackend(lm, lconfig, &logger) _, err := b.GetGroup(context.Background(), "group", nil) - if err == nil || err.Error() != "itemNotFound" { - t.Errorf("Expected 'itemNotFound' got '%s'", err.Error()) - } + assert.ErrorContains(t, err, "itemNotFound:") _, err = b.GetGroup(context.Background(), "group", queryParamExpand) - if err == nil || err.Error() != "itemNotFound" { - t.Errorf("Expected 'itemNotFound' got '%s'", err.Error()) - } + assert.ErrorContains(t, err, "itemNotFound:") _, err = b.GetGroup(context.Background(), "group", queryParamSelect) - if err == nil || err.Error() != "itemNotFound" { - t.Errorf("Expected 'itemNotFound' got '%s'", err.Error()) - } + assert.ErrorContains(t, err, "itemNotFound:") // Mock an empty Search Result lm = &mocks.Client{} lm.On("Search", mock.Anything).Return(&ldap.SearchResult{}, nil) b, _ = getMockedBackend(lm, lconfig, &logger) _, err = b.GetGroup(context.Background(), "group", nil) - if err == nil || err.Error() != "itemNotFound" { - t.Errorf("Expected 'itemNotFound' got '%s'", err.Error()) - } + assert.ErrorContains(t, err, "itemNotFound:") _, err = b.GetGroup(context.Background(), "group", queryParamExpand) - if err == nil || err.Error() != "itemNotFound" { - t.Errorf("Expected 'itemNotFound' got '%s'", err.Error()) - } + assert.ErrorContains(t, err, "itemNotFound:") _, err = b.GetGroup(context.Background(), "group", queryParamSelect) - if err == nil || err.Error() != "itemNotFound" { - t.Errorf("Expected 'itemNotFound' got '%s'", err.Error()) - } + assert.ErrorContains(t, err, "itemNotFound:") // Mock an invalid Search Result lm = &mocks.Client{} @@ -95,17 +83,11 @@ func TestGetGroup(t *testing.T) { }, nil) b, _ = getMockedBackend(lm, lconfig, &logger) g, err := b.GetGroup(context.Background(), "group", nil) - if err == nil || err.Error() != "itemNotFound" { - t.Errorf("Expected 'itemNotFound' got '%s'", err.Error()) - } + assert.ErrorContains(t, err, "itemNotFound:") g, err = b.GetGroup(context.Background(), "group", queryParamExpand) - if err == nil || err.Error() != "itemNotFound" { - t.Errorf("Expected 'itemNotFound' got '%s'", err.Error()) - } + assert.ErrorContains(t, err, "itemNotFound:") g, err = b.GetGroup(context.Background(), "group", queryParamSelect) - if err == nil || err.Error() != "itemNotFound" { - t.Errorf("Expected 'itemNotFound' got '%s'", err.Error()) - } + assert.ErrorContains(t, err, "itemNotFound:") // Mock a valid Search Result lm = &mocks.Client{} @@ -240,9 +222,7 @@ func TestGetGroups(t *testing.T) { lm.On("Search", mock.Anything).Return(nil, ldap.NewError(ldap.LDAPResultOperationsError, errors.New("mock"))) b, _ := getMockedBackend(lm, lconfig, &logger) _, err := b.GetGroups(context.Background(), url.Values{}) - if err == nil || err.Error() != "itemNotFound" { - t.Errorf("Expected 'itemNotFound' got '%s'", err.Error()) - } + assert.ErrorContains(t, err, "itemNotFound:") lm = &mocks.Client{} lm.On("Search", mock.Anything).Return(&ldap.SearchResult{}, nil) diff --git a/services/graph/pkg/identity/ldap_test.go b/services/graph/pkg/identity/ldap_test.go index fcd5a551ae9..908862f8197 100644 --- a/services/graph/pkg/identity/ldap_test.go +++ b/services/graph/pkg/identity/ldap_test.go @@ -205,14 +205,10 @@ func TestGetUser(t *testing.T) { } _, err = b.GetUser(context.Background(), "fred", odataReqDefault) - if err == nil || err.Error() != "itemNotFound" { - t.Errorf("Expected 'itemNotFound' got '%s'", err.Error()) - } + assert.ErrorContains(t, err, "itemNotFound:") _, err = b.GetUser(context.Background(), "fred", odataReqExpand) - if err == nil || err.Error() != "itemNotFound" { - t.Errorf("Expected 'itemNotFound' got '%s'", err.Error()) - } + assert.ErrorContains(t, err, "itemNotFound:") // Mock an empty Search Result lm = &mocks.Client{} @@ -221,14 +217,10 @@ func TestGetUser(t *testing.T) { &ldap.SearchResult{}, nil) b, _ = getMockedBackend(lm, lconfig, &logger) _, err = b.GetUser(context.Background(), "fred", odataReqDefault) - if err == nil || err.Error() != "itemNotFound" { - t.Errorf("Expected 'itemNotFound' got '%s'", err.Error()) - } + assert.ErrorContains(t, err, "itemNotFound:") _, err = b.GetUser(context.Background(), "fred", odataReqExpand) - if err == nil || err.Error() != "itemNotFound" { - t.Errorf("Expected 'itemNotFound' got '%s'", err.Error()) - } + assert.ErrorContains(t, err, "itemNotFound:") // Mock a valid Search Result lm = &mocks.Client{} @@ -265,9 +257,7 @@ func TestGetUser(t *testing.T) { b, _ = getMockedBackend(lm, lconfig, &logger) _, err = b.GetUser(context.Background(), "invalid", nil) - if err == nil || err.Error() != "itemNotFound" { - t.Errorf("Expected 'itemNotFound' got '%s'", err.Error()) - } + assert.ErrorContains(t, err, "itemNotFound:") } func TestGetUsers(t *testing.T) { @@ -283,9 +273,7 @@ func TestGetUsers(t *testing.T) { b, _ := getMockedBackend(lm, lconfig, &logger) _, err = b.GetUsers(context.Background(), odataReqDefault) - if err == nil || err.Error() != "generalException" { - t.Errorf("Expected 'generalException' got '%s'", err.Error()) - } + assert.ErrorContains(t, err, "generalException:") lm = &mocks.Client{} lm.On("Search", mock.Anything).Return(&ldap.SearchResult{}, nil) diff --git a/services/graph/pkg/service/v0/educationschools.go b/services/graph/pkg/service/v0/educationschools.go index eb7b10d7e7b..8a176ef2057 100644 --- a/services/graph/pkg/service/v0/educationschools.go +++ b/services/graph/pkg/service/v0/educationschools.go @@ -148,7 +148,7 @@ func (g Graph) PatchEducationSchool(w http.ResponseWriter, r *http.Request) { if school, err = g.identityEducationBackend.UpdateEducationSchool(r.Context(), schoolID, *school); err != nil { logger.Debug().Err(err).Interface("school", school).Msg("could not update school: backend error") - errorcode.GeneralException.Render(w, r, http.StatusInternalServerError, err.Error()) + errorcode.RenderError(w, r, err) return } diff --git a/services/graph/pkg/service/v0/errorcode/errorcode.go b/services/graph/pkg/service/v0/errorcode/errorcode.go index 6ac3b0a44a3..f5e5b49d0bd 100644 --- a/services/graph/pkg/service/v0/errorcode/errorcode.go +++ b/services/graph/pkg/service/v0/errorcode/errorcode.go @@ -130,7 +130,11 @@ func (e ErrorCode) String() string { } func (e Error) Error() string { - return errorCodes[e.errorCode] + errString := errorCodes[e.errorCode] + if e.msg != "" { + errString += ": " + e.msg + } + return errString } // RenderError render the Graph Error based on a code or default one