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)

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)

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

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)

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

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)

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)

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
 	}
 

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