chore(graph/education): reduce complexity and duplication

services/graph/pkg/identity/ldap_education_school.go

@@ -4,6 +4,7 @@ import (
 	"context"
 	"errors"
 	"fmt"
+	"slices"
 	"time"
 
 	"github.com/go-ldap/ldap/v3"
@@ -496,26 +497,25 @@ func (i *LDAP) AddUsersToEducationSchool(ctx context.Context, schoolNumberOrID s
 	}
 
 	for _, userEntry := range userEntries {
-		currentSchools := userEntry.GetEqualFoldAttributeValues(i.educationConfig.memberOfSchoolAttribute)
-		found := false
-		for _, currentSchool := range currentSchools {
-			if currentSchool == schoolID {
-				found = true
-				break
-			}
-		}
-		if !found {
-			mr := ldap.ModifyRequest{DN: userEntry.DN}
-			mr.Add(i.educationConfig.memberOfSchoolAttribute, []string{schoolID})
-			if err := i.conn.Modify(&mr); err != nil {
-				return err
-			}
+		if err := i.addEntryToSchool(userEntry, schoolID); err != nil {
+			return err
 		}
 	}
 
 	return nil
 }
 
+// addEntryToSchool adds the schoolID to the entry's memberOfSchool attribute if not already present.
+func (i *LDAP) addEntryToSchool(entry *ldap.Entry, schoolID string) error {
+	currentSchools := entry.GetEqualFoldAttributeValues(i.educationConfig.memberOfSchoolAttribute)
+	if slices.Contains(currentSchools, schoolID) {
+		return nil
+	}
+	mr := ldap.ModifyRequest{DN: entry.DN}
+	mr.Add(i.educationConfig.memberOfSchoolAttribute, []string{schoolID})
+	return i.conn.Modify(&mr)
+}
+
 // RemoveUserFromEducationSchool removes a single member (by ID) from a school
 func (i *LDAP) RemoveUserFromEducationSchool(ctx context.Context, schoolNumberOrID string, memberID string) error {
 	logger := i.logger.SubloggerWithRequestID(ctx)
@@ -644,20 +644,8 @@ func (i *LDAP) AddClassesToEducationSchool(ctx context.Context, schoolNumberOrID
 	}
 
 	for _, classEntry := range classEntries {
-		currentSchools := classEntry.GetEqualFoldAttributeValues(i.educationConfig.memberOfSchoolAttribute)
-		found := false
-		for _, currentSchool := range currentSchools {
-			if currentSchool == schoolID {
-				found = true
-				break
-			}
-		}
-		if !found {
-			mr := ldap.ModifyRequest{DN: classEntry.DN}
-			mr.Add(i.educationConfig.memberOfSchoolAttribute, []string{schoolID})
-			if err := i.conn.Modify(&mr); err != nil {
-				return err
-			}
+		if err := i.addEntryToSchool(classEntry, schoolID); err != nil {
+			return err
 		}
 	}
 

services/graph/pkg/identity/ldap_education_school_test.go

@@ -69,6 +69,8 @@ var (
 	filterSchoolSearchByIdNonexistant     = "(&(objectClass=openCloudEducationSchool)(|(openCloudUUID=xxxx-xxxx)(openCloudEducationSchoolNumber=xxxx-xxxx)))"
 	filterSchoolSearchByNumberExisting    = "(&(objectClass=openCloudEducationSchool)(|(openCloudUUID=0123)(openCloudEducationSchoolNumber=0123)))"
 	filterSchoolSearchByNumberNonexistant = "(&(objectClass=openCloudEducationSchool)(|(openCloudUUID=3210)(openCloudEducationSchoolNumber=3210)))"
+
+	schoolLDAPAttributeTypes = []string{"ou", "openCloudUUID", "openCloudEducationExternalId", "openCloudEducationSchoolNumber", "openCloudEducationSchoolTerminationTimestamp"}
 )
 
 func TestCreateEducationSchool(t *testing.T) {
@@ -128,7 +130,7 @@ func TestCreateEducationSchool(t *testing.T) {
 			Scope:      2,
 			SizeLimit:  1,
 			Filter:     "(&(objectClass=openCloudEducationSchool)(openCloudEducationSchoolNumber=0123))",
-			Attributes: []string{"ou", "openCloudUUID", "openCloudEducationExternalId", "openCloudEducationSchoolNumber", "openCloudEducationSchoolTerminationTimestamp"},
+			Attributes: schoolLDAPAttributeTypes,
 			Controls:   []ldap.Control(nil),
 		}
 		lm.On("Search", schoolNumberSearchRequest).
@@ -142,7 +144,7 @@ func TestCreateEducationSchool(t *testing.T) {
 			Scope:      2,
 			SizeLimit:  1,
 			Filter:     "(&(objectClass=openCloudEducationSchool)(openCloudEducationSchoolNumber=0666))",
-			Attributes: []string{"ou", "openCloudUUID", "openCloudEducationExternalId", "openCloudEducationSchoolNumber", "openCloudEducationSchoolTerminationTimestamp"},
+			Attributes: schoolLDAPAttributeTypes,
 			Controls:   []ldap.Control(nil),
 		}
 		lm.On("Search", existingSchoolNumberSearchRequest).
@@ -156,7 +158,7 @@ func TestCreateEducationSchool(t *testing.T) {
 			Scope:      2,
 			SizeLimit:  1,
 			Filter:     "(&(objectClass=openCloudEducationSchool)(openCloudEducationSchoolNumber=1111))",
-			Attributes: []string{"ou", "openCloudUUID", "openCloudEducationExternalId", "openCloudEducationSchoolNumber", "openCloudEducationSchoolTerminationTimestamp"},
+			Attributes: schoolLDAPAttributeTypes,
 			Controls:   []ldap.Control(nil),
 		}
 		lm.On("Search", schoolNumberSearchRequestError).
@@ -170,7 +172,7 @@ func TestCreateEducationSchool(t *testing.T) {
 			Scope:      0,
 			SizeLimit:  1,
 			Filter:     "(objectClass=openCloudEducationSchool)",
-			Attributes: []string{"ou", "openCloudUUID", "openCloudEducationExternalId", "openCloudEducationSchoolNumber", "openCloudEducationSchoolTerminationTimestamp"},
+			Attributes: schoolLDAPAttributeTypes,
 			Controls:   []ldap.Control(nil),
 		}
 		lm.On("Search", schoolLookupAfterCreate).
@@ -358,7 +360,7 @@ func TestDeleteEducationSchool(t *testing.T) {
 			Scope:      2,
 			SizeLimit:  1,
 			Filter:     tt.filter,
-			Attributes: []string{"ou", "openCloudUUID", "openCloudEducationExternalId", "openCloudEducationSchoolNumber", "openCloudEducationSchoolTerminationTimestamp"},
+			Attributes: schoolLDAPAttributeTypes,
 			Controls:   []ldap.Control(nil),
 		}
 		if tt.expectedItemNotFound {
@@ -427,7 +429,7 @@ func TestGetEducationSchool(t *testing.T) {
 			Scope:      2,
 			SizeLimit:  1,
 			Filter:     tt.filter,
-			Attributes: []string{"ou", "openCloudUUID", "openCloudEducationExternalId", "openCloudEducationSchoolNumber", "openCloudEducationSchoolTerminationTimestamp"},
+			Attributes: schoolLDAPAttributeTypes,
 			Controls:   []ldap.Control(nil),
 		}
 		if tt.expectedItemNotFound {
@@ -461,7 +463,7 @@ func TestGetEducationSchools(t *testing.T) {
 		Scope:      2,
 		SizeLimit:  0,
 		Filter:     "(objectClass=openCloudEducationSchool)",
-		Attributes: []string{"ou", "openCloudUUID", "openCloudEducationExternalId", "openCloudEducationSchoolNumber", "openCloudEducationSchoolTerminationTimestamp"},
+		Attributes: schoolLDAPAttributeTypes,
 		Controls:   []ldap.Control(nil),
 	}
 	lm.On("Search", sr1).Return(&ldap.SearchResult{Entries: []*ldap.Entry{schoolEntry, schoolEntry1}}, nil)
@@ -478,7 +480,7 @@ var schoolByIDSearch1 *ldap.SearchRequest = &ldap.SearchRequest{
 	Scope:      2,
 	SizeLimit:  1,
 	Filter:     filterSchoolSearchByIdExisting,
-	Attributes: []string{"ou", "openCloudUUID", "openCloudEducationExternalId", "openCloudEducationSchoolNumber", "openCloudEducationSchoolTerminationTimestamp"},
+	Attributes: schoolLDAPAttributeTypes,
 	Controls:   []ldap.Control(nil),
 }
 
@@ -487,7 +489,7 @@ var schoolByNumberSearch *ldap.SearchRequest = &ldap.SearchRequest{
 	Scope:      2,
 	SizeLimit:  1,
 	Filter:     filterSchoolSearchByNumberExisting,
-	Attributes: []string{"ou", "openCloudUUID", "openCloudEducationExternalId", "openCloudEducationSchoolNumber", "openCloudEducationSchoolTerminationTimestamp"},
+	Attributes: schoolLDAPAttributeTypes,
 	Controls:   []ldap.Control(nil),
 }
 

services/graph/pkg/service/v0/educationschools.go

@@ -1,6 +1,7 @@
 package svc
 
 import (
+	"context"
 	"errors"
 	"fmt"
 	"net/http"
@@ -30,36 +31,11 @@ func (g Graph) GetEducationSchools(w http.ResponseWriter, r *http.Request) {
 		return
 	}
 
-	var schools []*libregraph.EducationSchool
-	if odataReq.Query.Filter != nil {
-		attr, value, err := g.getEqualityFilter(r.Context(), odataReq)
-		if err != nil {
-			logger.Debug().Err(err).Str("filter", odataReq.Query.Filter.RawValue).Msg("failed to parse filter")
-			var errcode errorcode.Error
-			var godataerr *godata.GoDataError
-			switch {
-			case errors.As(err, &errcode):
-				errcode.Render(w, r)
-			case errors.As(err, &godataerr):
-				errorcode.GeneralException.Render(w, r, godataerr.ResponseCode, err.Error())
-			default:
-				errorcode.GeneralException.Render(w, r, http.StatusInternalServerError, err.Error())
-			}
-			return
-		}
-		schools, err = g.identityEducationBackend.FilterEducationSchoolsByAttribute(r.Context(), attr, value)
-		if err != nil {
-			logger.Debug().Err(err).Interface("query", r.URL.Query()).Msg("could not get schools from backend")
-			errorcode.RenderError(w, r, err)
-			return
-		}
-	} else {
-		schools, err = g.identityEducationBackend.GetEducationSchools(r.Context())
-		if err != nil {
-			logger.Debug().Err(err).Msg("could not get schools: backend error")
-			errorcode.RenderError(w, r, err)
-			return
-		}
+	schools, err := g.getEducationSchoolsFromBackend(r.Context(), odataReq)
+	if err != nil {
+		logger.Debug().Err(err).Interface("query", r.URL.Query()).Msg("could not get schools")
+		renderEqualityFilterError(w, r, err)
+		return
 	}
 
 	schools, err = sortEducationSchools(odataReq, schools)
@@ -641,3 +617,15 @@ func sortEducationSchools(req *godata.GoDataRequest, schools []*libregraph.Educa
 
 	return schools, nil
 }
+
+// getEducationSchoolsFromBackend fetches schools from the backend, applying an OData $filter if present.
+func (g Graph) getEducationSchoolsFromBackend(ctx context.Context, odataReq *godata.GoDataRequest) ([]*libregraph.EducationSchool, error) {
+	if odataReq.Query.Filter != nil {
+		attr, value, err := g.getEqualityFilter(ctx, odataReq)
+		if err != nil {
+			return nil, err
+		}
+		return g.identityEducationBackend.FilterEducationSchoolsByAttribute(ctx, attr, value)
+	}
+	return g.identityEducationBackend.GetEducationSchools(ctx)
+}

services/graph/pkg/service/v0/educationuser.go

@@ -32,38 +32,12 @@ func (g Graph) GetEducationUsers(w http.ResponseWriter, r *http.Request) {
 		return
 	}
 
-	var users []*libregraph.EducationUser
-	if odataReq.Query.Filter != nil {
-		attr, value, err := g.getEqualityFilter(r.Context(), odataReq)
-		if err != nil {
-			logger.Debug().Err(err).Str("filter", odataReq.Query.Filter.RawValue).Msg("failed to parse filter")
-			var errcode errorcode.Error
-			var godataerr *godata.GoDataError
-			switch {
-			case errors.As(err, &errcode):
-				errcode.Render(w, r)
-			case errors.As(err, &godataerr):
-				errorcode.GeneralException.Render(w, r, godataerr.ResponseCode, err.Error())
-			default:
-				errorcode.GeneralException.Render(w, r, http.StatusInternalServerError, err.Error())
-			}
-			return
-		}
-		users, err = g.identityEducationBackend.FilterEducationUsersByAttribute(r.Context(), attr, value)
-		if err != nil {
-			logger.Debug().Err(err).Interface("query", r.URL.Query()).Msg("could not get education users from backend")
-			errorcode.RenderError(w, r, err)
-			return
-		}
-	} else {
-		users, err = g.identityEducationBackend.GetEducationUsers(r.Context())
-		if err != nil {
-			logger.Debug().Err(err).Interface("query", r.URL.Query()).Msg("could not get education users from backend")
-			errorcode.RenderError(w, r, err)
-			return
-		}
+	users, err := g.getEducationUsersFromBackend(r.Context(), odataReq)
+	if err != nil {
+		logger.Debug().Err(err).Interface("query", r.URL.Query()).Msg("could not get education users")
+		renderEqualityFilterError(w, r, err)
+		return
 	}
-
 	users, err = sortEducationUsers(odataReq, users)
 	if err != nil {
 		logger.Debug().Interface("query", odataReq).Msg("error while sorting education users according to query")
@@ -413,6 +387,18 @@ func sortEducationUsers(req *godata.GoDataRequest, users []*libregraph.Education
 	return users, nil
 }
 
+// getEducationUsersFromBackend fetches users from the backend, applying an OData $filter if present.
+func (g Graph) getEducationUsersFromBackend(ctx context.Context, odataReq *godata.GoDataRequest) ([]*libregraph.EducationUser, error) {
+	if odataReq.Query.Filter != nil {
+		attr, value, err := g.getEqualityFilter(ctx, odataReq)
+		if err != nil {
+			return nil, err
+		}
+		return g.identityEducationBackend.FilterEducationUsersByAttribute(ctx, attr, value)
+	}
+	return g.identityEducationBackend.GetEducationUsers(ctx)
+}
+
 func (g Graph) getEqualityFilter(ctx context.Context, req *godata.GoDataRequest) (string, string, error) {
 	logger := g.logger.SubloggerWithRequestID(ctx)
 
@@ -439,3 +425,17 @@ func (g Graph) getEqualityFilter(ctx context.Context, req *godata.GoDataRequest)
 	value := strings.Trim(root.Children[1].Token.Value, "'")
 	return root.Children[0].Token.Value, value, nil
 }
+
+// renderEqualityFilterError writes the appropriate HTTP error response for errors returned by getEqualityFilter.
+func renderEqualityFilterError(w http.ResponseWriter, r *http.Request, err error) {
+	var errcode errorcode.Error
+	var godataerr *godata.GoDataError
+	switch {
+	case errors.As(err, &errcode):
+		errcode.Render(w, r)
+	case errors.As(err, &godataerr):
+		errorcode.GeneralException.Render(w, r, godataerr.ResponseCode, err.Error())
+	default:
+		errorcode.GeneralException.Render(w, r, http.StatusInternalServerError, err.Error())
+	}
+}