internal/router: add some more comments

internal/router/fieldinclude.go

@@ -25,6 +25,8 @@ type FieldHandlerFunc func(doc interface{}, id *charm.URL, path string, flags ur
 // The fields specify which fields are required by the given handler.
 // The fields passed to the query will be the union of all fields found
 // in all the handlers in the bulk request.
+//
+// See in ../v4/api.go for an example of its use.
 func FieldIncludeHandler(key interface{}, q FieldQueryFunc, fields []string, handle FieldHandlerFunc) BulkIncludeHandler {
 	return &fieldIncludeHandler{
 		key:    key,
@@ -61,15 +63,15 @@ func (h *fieldIncludeHandler) Handle(hs []BulkIncludeHandler, id *charm.URL, pat
 	if err != nil {
 		return nil, err
 	}
-	// Call all the handlers with the resulting query document
+	// Call all the handlers with the resulting query document.
 	results := make([]interface{}, len(hs))
 	for i, f := range funcs {
 		var err error
 		results[i], err = f(doc, id, paths[i], flags)
 		if err != nil {
 			// TODO correlate error with handler (perhaps return
 			// an error that identifies the slice position of the handler that
-			// failed
+			// failed).
 			return nil, err
 		}
 	}

internal/router/router.go

@@ -50,7 +50,7 @@ type BulkIncludeHandler interface {
 	// in hs after the prefix in Handlers.Meta has been stripped,
 	// and flags holds all the url query values.
 	//
-	// TODO(rog) document indexed errors
+	// TODO(rog) document indexed errors.
 	Handle(hs []BulkIncludeHandler, id *charm.URL, paths []string, flags url.Values) ([]interface{}, error)
 }
 
@@ -238,10 +238,14 @@ func (r *Router) serveBulkMeta(w http.ResponseWriter, req *http.Request) (interf
 			return nil, err
 		}
 		if err := r.resolveURL(url); err != nil {
+			// TODO(rog) if the error is because the id was
+			// not found, then just omit from result.
 			return nil, err
 		}
 		meta, err := r.serveMeta(url, req)
 		if err != nil {
+			// TODO(rog) if the error is because the id was
+			// not found, then just omit from result.
 			return nil, err
 		}
 		result[id] = meta
@@ -255,18 +259,30 @@ func (r *Router) GetMetadata(id *charm.URL, includes []string) (map[string]inter
 	groups := make(map[interface{}][]BulkIncludeHandler)
 	includesByGroup := make(map[interface{}][]string)
 	for _, include := range includes {
+		// Get the key that lets us choose the include handler.
 		includeKey, _ := handlerKey(include)
 		handler := r.handlers.Meta[includeKey]
 		if handler == nil {
 			return nil, fmt.Errorf("unrecognized metadata name %q", include)
 		}
+
+		// Get the key that lets us group this handler into the
+		// correct bulk group.
 		key := handler.Key()
 		groups[key] = append(groups[key], handler)
 		includesByGroup[key] = append(includesByGroup[key], include)
 	}
 	results := make(map[string]interface{})
 	for _, g := range groups {
+		// We know that we must have at least one element in the
+		// slice here. We could use any member of the slice to
+		// actually handle the request, so arbitrarily choose
+		// g[0]. Note that g[0].Key() is equal to g[i].Key() for
+		// every i in the slice.
 		groupIncludes := includesByGroup[g[0].Key()]
+
+		// Paths contains all the path elements after
+		// the handler key has been stripped off.
 		paths := make([]string, len(g))
 		for i, include := range groupIncludes {
 			_, paths[i] = handlerKey(include)
@@ -275,7 +291,7 @@ func (r *Router) GetMetadata(id *charm.URL, includes []string) (map[string]inter
 		if err != nil {
 			// TODO(rog) if it's a BulkError, attach
 			// the original include path to error (the BulkError
-			// should contain the index of the failed one)
+			// should contain the index of the failed one).
 			return nil, err
 		}
 		for i, result := range groupResults {

internal/router/router_test.go

@@ -468,6 +468,8 @@ func (s *RouterSuite) TestRouter(c *gc.C) {
 			resolve = test.resolveURL
 		}
 		router := New(&test.handlers, resolve)
+		// Note that fieldSelectHandler increments this each time
+		// a query is made.
 		queryCount = 0
 		storetesting.AssertJSONCall(c, router, "GET", test.urlStr, "", test.expectCode, test.expectBody)
 		c.Assert(queryCount, gc.Equals, test.expectQueryCount)

internal/router/singleinclude.go

@@ -12,20 +12,22 @@ var _ BulkIncludeHandler = SingleIncludeHandler(nil)
 // metadata retrieval function.
 type SingleIncludeHandler func(id *charm.URL, path string, flags url.Values) (interface{}, error)
 
-// Key implements BulkMetadataHander.Key
+// Key implements BulkMetadataHander.Key.
 func (h SingleIncludeHandler) Key() interface{} {
-	type singleMetaHandlerKey int
-	return singleMetaHandlerKey(0)
+	// Use a local type so that we are guaranteed that nothing
+	// other than SingleIncludeHandler can generate that key.
+	type singleMetaHandlerKey struct{}
+	return singleMetaHandlerKey(singleMetaHandlerKey{})
 }
 
-// Handle implements BulkMetadataHander.Handle
+// Handle implements BulkMetadataHander.Handle.
 func (h SingleIncludeHandler) Handle(hs []BulkIncludeHandler, id *charm.URL, paths []string, flags url.Values) ([]interface{}, error) {
 	results := make([]interface{}, len(hs))
 	for i, h := range hs {
 		h := h.(SingleIncludeHandler)
 		result, err := h(id, paths[i], flags)
 		if err != nil {
-			// TODO(rog) include index of failed handler
+			// TODO(rog) include index of failed handler.
 			return nil, err
 		}
 		results[i] = result