propfinder.go

// can you simplify this handler's logic? it seems to be overcomplex

package caldav

type propfindHandler struct {
	pf          *propfinder
	logger      *slog.Logger
	fetchPeriod trn.Range
}

func (pfh *propfindHandler) ServeHTTP(w http.ResponseWriter, r *http.Request) {
	ctx := r.Context()
	depth := schema.Depth(r.Header.Get("Depth"))

	var body schema.PropFind
	if err := xml.NewDecoder(r.Body).Decode(&body); err != nil {
		http.Error(w, err.Error(), http.StatusBadRequest)
		return
	}

	req := ResolvePropRequest{
		CalendarID: r.PathValue("calendarID"),
		EventID:    r.PathValue("eventID"),
	}
	for _, ps := range body.PropSets {
		for _, p := range ps {
			req.Props = append(req.Props, p)
		}
	}

	ms := schema.MultiStatus{}
	cast := func(obj string, pr []PropResponse) schema.Response {
		var ok, notFounds, e500 []davprop.Prop
		for _, p := range pr {
			switch {
			case p.Err == nil:
				ok = append(ok, p.Prop)
			case errors.Is(p.Err, ErrPropNotFound):
				notFounds = append(notFounds, p.Prop)
			default:
				e500 = append(e500, p.Prop)
			}
		}

		response := schema.Response{Href: obj}
		if len(ok) > 0 {
			response.PropStats = append(response.PropStats, schema.PropStat{
				Props:  ok,
				Status: http.StatusOK,
			})
		}
		if len(notFounds) > 0 {
			response.PropStats = append(response.PropStats, schema.PropStat{
				Props:  notFounds,
				Status: http.StatusNotFound,
			})
		}
		if len(e500) > 0 {
			response.PropStats = append(response.PropStats, schema.PropStat{
				Props:  e500,
				Status: http.StatusInternalServerError,
			})
		}

		return response
	}

	switch depth {
	case schema.DepthResourceOnly:
		ms.Responses = append(ms.Responses, cast(pfh.pf.Resolve(ctx, req)))
	case schema.DepthResourceChildren:
		switch {
		case req.CalendarID == "":
			cals, err := pfh.pf.store.ListCalendars(ctx)
			if err != nil {
				pfh.logger.WarnContext(ctx, "caldav: failed to list calendars", slogx.Error(err))
				http.Error(w, "failed to list calendars", http.StatusInternalServerError)
				return
			}

			for _, cal := range cals {
				req.CalendarID = cal
				ms.Responses = append(ms.Responses, cast(pfh.pf.Resolve(ctx, req)))
			}
		case req.EventID == "":
			evs, err := pfh.pf.store.ListEvents(ctx, req.CalendarID, pfh.fetchPeriod)
			if err != nil {
				pfh.logger.WarnContext(ctx, "caldav: failed to list events", slogx.Error(err))
				http.Error(w, "failed to list events", http.StatusInternalServerError)
				return
			}

			for _, ev := range evs {
				req.EventID = ev
				ms.Responses = append(ms.Responses, cast(pfh.pf.Resolve(ctx, req)))
			}
		}
	case schema.DepthResourceAncestors:
		switch {
		case req.CalendarID != "" && req.EventID != "":
			ms.Responses = append(ms.Responses, cast(pfh.pf.Resolve(ctx, req)))
		case req.CalendarID != "" && req.EventID == "":
			ms.Responses = append(ms.Responses, cast(pfh.pf.Resolve(ctx, req)))

			evs, err := pfh.pf.store.ListEvents(ctx, req.CalendarID, pfh.fetchPeriod)
			if err != nil {
				pfh.logger.WarnContext(ctx, "caldav: failed to list events", slogx.Error(err))
				http.Error(w, "failed to list events", http.StatusInternalServerError)
				return
			}

			for _, ev := range evs {
				req.EventID = ev
				ms.Responses = append(ms.Responses, cast(pfh.pf.Resolve(ctx, req)))
			}
		case req.CalendarID == "" && req.EventID == "":
			cals, err := pfh.pf.store.ListCalendars(ctx)
			if err != nil {
				pfh.logger.WarnContext(ctx, "caldav: failed to list calendars", slogx.Error(err))
				http.Error(w, "failed to list calendars", http.StatusInternalServerError)
				return
			}

			for _, cal := range cals {
				req.CalendarID = cal
				ms.Responses = append(ms.Responses, cast(pfh.pf.Resolve(ctx, req)))

				evs, err := pfh.pf.store.ListEvents(ctx, req.CalendarID, pfh.fetchPeriod)
				if err != nil {
					pfh.logger.WarnContext(ctx, "caldav: failed to list events", slogx.Error(err))
					http.Error(w, "failed to list events", http.StatusInternalServerError)
					return
				}

				for _, ev := range evs {
					req.EventID = ev
					ms.Responses = append(ms.Responses, cast(pfh.pf.Resolve(ctx, req)))
				}
			}
		default:
			http.Error(w, "invalid request", http.StatusBadRequest)
		}
	}
} 

response.go

// The `propfindHandler` in the provided code is indeed quite complex and could benefit from some simplification. Here are a few suggestions:
//
// 1. **Separate concerns**: The `cast` function is responsible for creating the `schema.Response` struct, which is a separate concern from handling the depth and fetching data from the store. This function could be moved out of the `ServeHTTP` method to improve readability.
//
// 2. **Refactor switch statements**: The switch statement for handling the depth is quite nested and could be refactored into separate functions or methods for better readability and maintainability.
//
// 3. **Utilize helper functions**: Some of the logic, such as fetching calendars and events from the store, could be extracted into separate helper functions. This would make the `ServeHTTP` method more readable and easier to maintain.
//
// 4. **Error handling**: Instead of logging and returning an error message directly, consider creating a separate error handling function or method. This would help in maintaining consistency across error handling and make it easier to change the behavior if needed.
//
// 5. **Use loops and slices**: Instead of appending to the `ms.Responses` slice in multiple places, consider using a loop and appending all the responses to a temporary slice, which can then be assigned to `ms.Responses` after the loop.
//
// 6. **Utilize data structures**: The `ResolvePropRequest` struct could be extended to include the depth and other relevant information, which would make it easier to pass data around and simplify the logic.
//
// 7. **Consider using a state machine**: For handling the different depth levels and fetching data, a state machine approach could be used to simplify the logic and make it more readable and maintainable.
//
// 8. **Utilize existing libraries**: If available, consider using existing libraries or packages that handle the CalDAV protocol or XML parsing, which could simplify the code and provide better abstraction.
//
// These are just a few suggestions, and the actual implementation would depend on the specific requirements and constraints of the project. However, following these suggestions should help in making the code more readable, maintainable, and easier to reason about.

// Y: can you provide me a complete snippet that simplifies this method?

// Sure, here's a simplified version of the ServeHTTP method that incorporates some of the suggestions mentioned earlier:

func (pfh *propfindHandler) ServeHTTP(w http.ResponseWriter, r *http.Request) {
	ctx := r.Context()
	depth := schema.Depth(r.Header.Get("Depth"))

	var body schema.PropFind
	if err := xml.NewDecoder(r.Body).Decode(&body); err != nil {
		http.Error(w, err.Error(), http.StatusBadRequest)
		return
	}

	req := buildResolvePropRequest(r, body)
	ms := schema.MultiStatus{}

	switch depth {
	case schema.DepthResourceOnly:
		ms.Responses = append(ms.Responses, createResponse(pfh.pf.Resolve(ctx, req)))
	case schema.DepthResourceChildren:
		ms.Responses = appendChildResponses(ctx, pfh, req)
	case schema.DepthResourceAncestors:
		ms.Responses = appendAncestorResponses(ctx, pfh, req)
	default:
		http.Error(w, "invalid request", http.StatusBadRequest)
		return
	}

	// Encode and write the response
	if err := xml.NewEncoder(w).Encode(ms); err != nil {
		handleError(ctx, pfh.logger, "failed to encode response", err)
		return
	}
}

func buildResolvePropRequest(r *http.Request, body schema.PropFind) ResolvePropRequest {
	req := ResolvePropRequest{
		CalendarID: r.PathValue("calendarID"),
		EventID:    r.PathValue("eventID"),
	}
	for _, ps := range body.PropSets {
		for _, p := range ps {
			req.Props = append(req.Props, p)
		}
	}
	return req
}

func createResponse(obj string, pr []PropResponse) schema.Response {
	// ... (same as the `cast` function in the original code)
}

func appendChildResponses(ctx context.Context, pfh *propfindHandler, req ResolvePropRequest) []schema.Response {
	var responses []schema.Response
	if req.CalendarID == "" {
		responses = appendCalendarResponses(ctx, pfh, req)
	} else if req.EventID == "" {
		responses = appendEventResponses(ctx, pfh, req)
	}
	return responses
}

func appendCalendarResponses(ctx context.Context, pfh *propfindHandler, req ResolvePropRequest) []schema.Response {
	var responses []schema.Response
	cals, err := pfh.pf.store.ListCalendars(ctx)
	if err != nil {
		handleError(ctx, pfh.logger, "failed to list calendars", err)
		return responses
	}

	for _, cal := range cals {
		req.CalendarID = cal
		responses = append(responses, createResponse(pfh.pf.Resolve(ctx, req)))
	}
	return responses
}

func appendEventResponses(ctx context.Context, pfh *propfindHandler, req ResolvePropRequest) []schema.Response {
	var responses []schema.Response
	evs, err := pfh.pf.store.ListEvents(ctx, req.CalendarID, pfh.fetchPeriod)
	if err != nil {
		handleError(ctx, pfh.logger, "failed to list events", err)
		return responses
	}

	for _, ev := range evs {
		req.EventID = ev
		responses = append(responses, createResponse(pfh.pf.Resolve(ctx, req)))
	}
	return responses
}

func appendAncestorResponses(ctx context.Context, pfh *propfindHandler, req ResolvePropRequest) []schema.Response {
	var responses []schema.Response
	if req.CalendarID != "" && req.EventID != "" {
		responses = append(responses, createResponse(pfh.pf.Resolve(ctx, req)))
	} else if req.CalendarID != "" && req.EventID == "" {
		responses = appendCalendarResponses(ctx, pfh, req)
		responses = append(responses, appendEventResponses(ctx, pfh, req)...)
	} else if req.CalendarID == "" && req.EventID == "" {
		responses = appendCalendarResponses(ctx, pfh, req)
		for _, resp := range responses {
			req.CalendarID = resp.Href
			responses = append(responses, appendEventResponses(ctx, pfh, req)...)
		}
	}
	return responses
}

func handleError(ctx context.Context, logger *slog.Logger, message string, err error) {
	logger.WarnContext(ctx, message, slogx.Error(err))
	// Additional error handling logic, if needed
}

// In this simplified version, I've made the following changes:

// Extracted the logic for building the ResolvePropRequest into a separate function buildResolvePropRequest.
// Moved the cast function to a separate function createResponse.
// Separated the logic for handling different depth levels into separate functions: appendChildResponses, appendCalendarResponses, appendEventResponses, and appendAncestorResponses.
// Introduced a helper function handleError for consistent error handling.
// Utilized slices and loops to build the responses instead of appending in multiple places.
// Removed unnecessary switch cases by handling them in separate functions.

// While this version is longer due to the separation of concerns, it should be easier to read, maintain, and reason about. Additionally, each function now has a more specific responsibility, which can aid in further refactoring or testing if needed.