mirror/opencloud
1
0
Fork 0
mirror of https://github.com/opencloud-eu/opencloud.git synced 2026-01-06 20:29:54 -06:00
Code Issues Packages Projects Releases Wiki Activity

graph/education: slightly improve error handling and logging

Browse Source 
- 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)
This commit is contained in:
Ralf Haferkamp
2023-09-27 12:29:55 +02:00
committed by Ralf Haferkamp
parent 4f59de9c52
commit a34d467285
8 changed files with 42 additions and 69 deletions
Download Patch File Download Diff File Expand all files Collapse all files

10
services/graph/pkg/identity/ldap_education_class_test.go
View File

@@ -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)

17
services/graph/pkg/identity/ldap_education_school.go
View File

@@ -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})
}

8
services/graph/pkg/identity/ldap_education_school_test.go
View File

@@ -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)

4
services/graph/pkg/identity/ldap_education_user_test.go
View File

@@ -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) {

40
services/graph/pkg/identity/ldap_group_test.go
View File

@@ -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)

24
services/graph/pkg/identity/ldap_test.go
View File

@@ -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)

2
services/graph/pkg/service/v0/educationschools.go
View File

@@ -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
}

6
services/graph/pkg/service/v0/errorcode/errorcode.go
View File

@@ -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