caldav + carddav: Optimized handleMultiGet with GetCalendarObjects and GetAddressObjects #194
Conversation
Added GetCalendarObjects function to interface. Using it during handleMultiGet.
Added GetAddressObjects function to interface. Using it during handleMultiGet.
caldav/server.go
Outdated
| @@ -36,6 +36,8 @@ type Backend interface { | |||
| GetCalendar(ctx context.Context, path string) (*Calendar, error) | |||
|
|
|||
| GetCalendarObject(ctx context.Context, path string, req *CalendarCompRequest) (*CalendarObject, error) | |||
There was a problem hiding this comment.
Can we remove GetCalendarObject, to avoid having two functions which do a very similar thing? GetCalendarObject is equivalent to GetCalendarObjects with a single path.
There was a problem hiding this comment.
Both Single and Multiple Objects creation are required:
caldav Server options makes call to GetCalendarObject (which is single-get), GetCalendarObjects could either call GetCalendarObject or handle the bulk processing at Backend in it way.
There was a problem hiding this comment.
The GET handler can invoke GetCalendarObjects with a single path in the list.
There was a problem hiding this comment.
PR updated: Replaced GetAddressObject & GetCalendarObject with GetAddressObjects & GetCalendarObjects and its demand in Backend interface too.
| } | ||
|
|
||
| // multi-fetch and lookup collection | ||
| lookups := make(map[string]*CalendarObject) |
There was a problem hiding this comment.
Do we really need to build a lookup map? Can't we just iterate over the results?
There was a problem hiding this comment.
This is minor optimization (considering the objects could be more than 1000)
There was a problem hiding this comment.
I'm suggesting to replace the multiget.Hrefs loop below when building responses with a loop over objects.
There was a problem hiding this comment.
@emersion - The lookups ensures we can match each href to its object efficiently and preserve request order without nested loops. It also gracefully handles missing paths.
This way, backend GetCalendarObjects or GetAddressObjects need to worry about the order of returned objects or missing those that are not found.
caldav/server.go
Outdated
| var err error | ||
| var found bool | ||
| if co, found = lookups[href.Path]; !found { | ||
| err = fmt.Errorf("Couldn't find calendar object at: %s", href.Path) |
There was a problem hiding this comment.
This will not send a 404. We should use internal.HTTPErrorf() with http.StatusNotFound.
There was a problem hiding this comment.
Here error is packed into the responses return value.
There was a problem hiding this comment.
The error doesn't have the HTTPError type, so will be encoded as a 500.
There was a problem hiding this comment.
@emersion - Updated error handling as suggested.
Reduces code duplication in implementation.
Reduces code duplication in implementation
|
@emersion - If there are more changes to review let me know. |
2 participants
Delegating multi-get requests to Backend (GetCalendarObjects and GetAddressObjects)
Backend implementation can optimize the way it fetches details based on paths.