Addressing go vet / golint

api_test.go

@@ -1,7 +1,9 @@
 package vain
 
 import (
+	"bytes"
 	"fmt"
+	"io"
 	"net/http"
 	"net/http/httptest"
 	"strings"
@@ -20,8 +22,8 @@ func TestAdd(t *testing.T) {
 		t.Errorf("couldn't GET: %v", err)
 	}
 	resp.Body.Close()
-	if len(s.storage.p) != 0 {
+	if len(ms.p) != 0 {
-		t.Errorf("started with something in it; got %d, want %d", len(s.storage.p), 0)
+		t.Errorf("started with something in it; got %d, want %d", len(ms.p), 0)
 	}
 
 	bad := ts.URL
@@ -30,8 +32,8 @@ func TestAdd(t *testing.T) {
 		t.Errorf("couldn't POST: %v", err)
 	}
 	resp.Body.Close()
-	if len(s.storage.p) != 0 {
+	if len(ms.p) != 0 {
-		t.Errorf("started with something in it; got %d, want %d", len(s.storage.p), 0)
+		t.Errorf("started with something in it; got %d, want %d", len(ms.p), 0)
 	}
 
 	good := fmt.Sprintf("%s/foo", ts.URL)
@@ -40,16 +42,16 @@ func TestAdd(t *testing.T) {
 		t.Errorf("couldn't POST: %v", err)
 	}
 
-	if len(s.storage.p) != 1 {
+	if len(ms.p) != 1 {
-		t.Errorf("storage should have something in it; got %d, want %d", len(s.storage.p), 1)
+		t.Errorf("storage should have something in it; got %d, want %d", len(ms.p), 1)
 	}
 
-	p, ok := s.storage.p[good]
+	p, ok := ms.p[good]
 	if !ok {
 		t.Fatalf("did not find package for %s; should have posted a valid package", good)
 	}
-	if p.Path != good {
+	if p.path != good {
-		t.Errorf("package name did not go through as expected; got %q, want %q", p.Path, good)
+		t.Errorf("package name did not go through as expected; got %q, want %q", p.path, good)
 	}
 	if want := "https://s.mcquay.me/sm/vain"; p.Repo != want {
 		t.Errorf("repo did not go through as expected; got %q, want %q", p.Repo, want)
@@ -57,6 +59,22 @@ func TestAdd(t *testing.T) {
 	if want := Git; p.Vcs != want {
 		t.Errorf("Vcs did not go through as expected; got %q, want %q", p.Vcs, want)
 	}
+
+	resp, err = http.Get(ts.URL)
+	if err != nil {
+		t.Errorf("couldn't GET: %v", err)
+	}
+	defer resp.Body.Close()
+	if want := http.StatusOK; resp.StatusCode != want {
+		t.Errorf("Should have succeeded to fetch /; got %s, want %s", resp.Status, http.StatusText(want))
+	}
+	buf := &bytes.Buffer{}
+	if _, err := io.Copy(buf, resp.Body); err != nil {
+		t.Errorf("couldn't read content from server: %v", err)
+	}
+	if got, want := strings.Count(buf.String(), "meta"), 1; got != want {
+		t.Errorf("did not find all the tags I need; got %d, want %d", got, want)
+	}
 }
 
 func TestInvalidPath(t *testing.T) {
@@ -71,11 +89,11 @@ func TestInvalidPath(t *testing.T) {
 	if err != nil {
 		t.Errorf("couldn't POST: %v", err)
 	}
-	if len(s.storage.p) != 0 {
+	if len(ms.p) != 0 {
-		t.Errorf("should have failed to insert; got %d, want %d", len(s.storage.p), 0)
+		t.Errorf("should have failed to insert; got %d, want %d", len(ms.p), 0)
 	}
-	if resp.StatusCode != http.StatusBadRequest {
+	if want := http.StatusBadRequest; resp.StatusCode != want {
-		t.Errorf("should have failed to post at bad route; got %s, want %s", resp.Status, http.StatusText(http.StatusBadRequest))
+		t.Errorf("should have failed to post at bad route; got %s, want %s", resp.Status, http.StatusText(want))
 	}
 }
 
@@ -131,3 +149,86 @@ func TestCannotAddExistingSubPath(t *testing.T) {
 		t.Errorf("initial post should have worked; got %s, want %s", resp.Status, http.StatusText(want))
 	}
 }
+
+func TestMissingRepo(t *testing.T) {
+	ms := NewMemStore("")
+	s := &Server{
+		storage: ms,
+	}
+	ts := httptest.NewServer(s)
+	s.hostname = ts.URL
+	url := fmt.Sprintf("%s/foo", ts.URL)
+	resp, err := http.Post(url, "application/json", strings.NewReader(`{}`))
+	if err != nil {
+		t.Errorf("couldn't POST: %v", err)
+	}
+	if len(ms.p) != 0 {
+		t.Errorf("should have failed to insert; got %d, want %d", len(ms.p), 0)
+	}
+	if want := http.StatusBadRequest; resp.StatusCode != want {
+		t.Errorf("should have failed to post at bad route; got %s, want %s", resp.Status, http.StatusText(want))
+	}
+}
+
+func TestBadJson(t *testing.T) {
+	ms := NewMemStore("")
+	s := &Server{
+		storage: ms,
+	}
+	ts := httptest.NewServer(s)
+	s.hostname = ts.URL
+	url := fmt.Sprintf("%s/foo", ts.URL)
+	resp, err := http.Post(url, "application/json", strings.NewReader(`{`))
+	if err != nil {
+		t.Errorf("couldn't POST: %v", err)
+	}
+	if len(ms.p) != 0 {
+		t.Errorf("should have failed to insert; got %d, want %d", len(ms.p), 0)
+	}
+	if want := http.StatusBadRequest; resp.StatusCode != want {
+		t.Errorf("should have failed to post at bad route; got %s, want %s", resp.Status, http.StatusText(want))
+	}
+}
+
+func TestUnsupportedMethod(t *testing.T) {
+	ms := NewMemStore("")
+	s := &Server{
+		storage: ms,
+	}
+	ts := httptest.NewServer(s)
+	s.hostname = ts.URL
+	url := fmt.Sprintf("%s/foo", ts.URL)
+	client := &http.Client{}
+	req, err := http.NewRequest("PUT", url, nil)
+	resp, err := client.Do(req)
+	if err != nil {
+		t.Errorf("couldn't POST: %v", err)
+	}
+	if len(ms.p) != 0 {
+		t.Errorf("should have failed to insert; got %d, want %d", len(ms.p), 0)
+	}
+	if want := http.StatusMethodNotAllowed; resp.StatusCode != want {
+		t.Errorf("should have failed to post at bad route; got %s, want %s", resp.Status, http.StatusText(want))
+	}
+}
+
+func TestNewServer(t *testing.T) {
+	ms := NewMemStore("")
+	sm := http.NewServeMux()
+	s := NewServer(sm, ms, "foo")
+	ts := httptest.NewServer(s)
+	s.hostname = ts.URL
+	url := fmt.Sprintf("%s/foo", ts.URL)
+	client := &http.Client{}
+	req, err := http.NewRequest("PUT", url, nil)
+	resp, err := client.Do(req)
+	if err != nil {
+		t.Errorf("couldn't POST: %v", err)
+	}
+	if len(ms.p) != 0 {
+		t.Errorf("should have failed to insert; got %d, want %d", len(ms.p), 0)
+	}
+	if want := http.StatusMethodNotAllowed; resp.StatusCode != want {
+		t.Errorf("should have failed to post at bad route; got %s, want %s", resp.Status, http.StatusText(want))
+	}
+}

server.go

@@ -7,9 +7,20 @@ import (
 	"strings"
 )
 
+// NewServer populates a server, adds the routes, and returns it for use.
+func NewServer(sm *http.ServeMux, store Storage, hostname string) *Server {
+	s := &Server{
+		storage:  store,
+		hostname: hostname,
+	}
+	sm.Handle("/", s)
+	return s
+}
+
+// Server serves up the http.
 type Server struct {
 	hostname string
-	storage  *MemStore
+	storage  Storage
 }
 
 func (s *Server) ServeHTTP(w http.ResponseWriter, req *http.Request) {
@@ -27,11 +38,15 @@ func (s *Server) ServeHTTP(w http.ResponseWriter, req *http.Request) {
 		}
 		p := Package{}
 		if err := json.NewDecoder(req.Body).Decode(&p); err != nil {
-			http.Error(w, fmt.Sprintf("unable to parse json from body: %v", err), http.StatusInternalServerError)
+			http.Error(w, fmt.Sprintf("unable to parse json from body: %v", err), http.StatusBadRequest)
 			return
 		}
-		p.Path = fmt.Sprintf("%s/%s", s.hostname, strings.Trim(req.URL.Path, "/"))
+		if p.Repo == "" {
-		if !Valid(p.Path, s.storage.All()) {
+			http.Error(w, fmt.Sprintf("invalid repository %q", req.URL.Path), http.StatusBadRequest)
+			return
+		}
+		p.path = fmt.Sprintf("%s/%s", s.hostname, strings.Trim(req.URL.Path, "/"))
+		if !Valid(p.path, s.storage.All()) {
 			http.Error(w, fmt.Sprintf("invalid path; prefix already taken %q", req.URL.Path), http.StatusConflict)
 			return
 		}
@@ -39,23 +54,7 @@ func (s *Server) ServeHTTP(w http.ResponseWriter, req *http.Request) {
 			http.Error(w, fmt.Sprintf("unable to add package: %v", err), http.StatusInternalServerError)
 			return
 		}
-		if err := s.storage.Save(); err != nil {
-			http.Error(w, fmt.Sprintf("unable to store db: %v", err), http.StatusInternalServerError)
-			if err := s.storage.Remove(p.Path); err != nil {
-				fmt.Fprintf(w, "to add insult to injury, could not delete package: %v\n", err)
-			}
-			return
-		}
 	default:
 		http.Error(w, fmt.Sprintf("unsupported method %q; accepted: POST, GET", req.Method), http.StatusMethodNotAllowed)
 	}
 }
-
-func NewServer(sm *http.ServeMux, ms *MemStore, hostname string) *Server {
-	s := &Server{
-		storage:  ms,
-		hostname: hostname,
-	}
-	sm.Handle("/", s)
-	return s
-}

storage.go

@@ -2,21 +2,32 @@ package vain
 
 import (
 	"encoding/json"
+	"errors"
+	"fmt"
 	"os"
 	"strings"
 	"sync"
 )
 
+// Valid checks that p will not confuse the go tool if added to packages.
 func Valid(p string, packages []Package) bool {
 	for _, pkg := range packages {
-		if strings.HasPrefix(pkg.Path, p) {
+		if strings.HasPrefix(pkg.path, p) {
 			return false
 		}
 	}
 	return true
 }
 
-type MemStore struct {
+// Storage is a shim to allow for alternate storage types.
+type Storage interface {
+	Add(p Package) error
+	Remove(path string) error
+	All() []Package
+}
+
+// SimpleStore implements a simple json on-disk storage.
+type SimpleStore struct {
 	l sync.RWMutex
 	p map[string]Package
 
@@ -24,28 +35,51 @@ type MemStore struct {
 	path string
 }
 
-func NewMemStore(path string) *MemStore {
+// NewMemStore returns a ready-to-use SimpleStore storing json at path.
-	return &MemStore{
+func NewMemStore(path string) *SimpleStore {
+	return &SimpleStore{
 		path: path,
 		p:    make(map[string]Package),
 	}
 }
 
-func (ms *MemStore) Add(p Package) error {
+// Add adds p to the SimpleStore.
+func (ms *SimpleStore) Add(p Package) error {
 	ms.l.Lock()
-	ms.p[p.Path] = p
+	ms.p[p.path] = p
 	ms.l.Unlock()
+	m := ""
+	if err := ms.Save(); err != nil {
+		m = fmt.Sprintf("unable to store db: %v", err)
+		if err := ms.Remove(p.path); err != nil {
+			m = fmt.Sprintf("%s\nto add insult to injury, could not delete package: %v\n", m, err)
+		}
+		return errors.New(m)
+	}
 	return nil
 }
 
-func (ms *MemStore) Remove(path string) error {
+// Remove removes p from the SimpleStore.
+func (ms *SimpleStore) Remove(path string) error {
 	ms.l.Lock()
 	delete(ms.p, path)
 	ms.l.Unlock()
 	return nil
 }
 
-func (ms *MemStore) Save() error {
+// All returns all current packages.
+func (ms *SimpleStore) All() []Package {
+	r := []Package{}
+	ms.l.RLock()
+	for _, p := range ms.p {
+		r = append(r, p)
+	}
+	ms.l.RUnlock()
+	return r
+}
+
+// Save writes the db to disk.
+func (ms *SimpleStore) Save() error {
 	// running in-memory only
 	if ms.path == "" {
 		return nil
@@ -60,7 +94,8 @@ func (ms *MemStore) Save() error {
 	return json.NewEncoder(f).Encode(ms.p)
 }
 
-func (ms *MemStore) Load() error {
+// Load reads the db from disk and populates ms.
+func (ms *SimpleStore) Load() error {
 	// running in-memory only
 	if ms.path == "" {
 		return nil
@@ -74,13 +109,3 @@ func (ms *MemStore) Load() error {
 	defer f.Close()
 	return json.NewDecoder(f).Decode(&ms.p)
 }
-
-func (ms *MemStore) All() []Package {
-	r := []Package{}
-	ms.l.RLock()
-	for _, p := range ms.p {
-		r = append(r, p)
-	}
-	ms.l.RUnlock()
-	return r
-}

vain.go

@@ -1,3 +1,6 @@
+// Package vain implements a vanity service for use by the the go tool.
+//
+// The executable, cmd/ysvd, is located in the respective subdirectory.
 package vain
 
 import "fmt"
@@ -5,7 +8,10 @@ import "fmt"
 type vcs int
 
 const (
+	// Git is the default Vcs.
 	Git vcs = iota
+
+	// Hg is mercurial
 	Hg
 )
 
@@ -23,16 +29,25 @@ var labelToVcs = map[string]vcs{
 // String returns the name of the vcs ("git", "mercurial", ...).
 func (v vcs) String() string { return vcss[v] }
 
+// Package stores the three pieces of information needed to create the meta
+// tag. Two of these (Vcs and Repo) are stored explicitly, and the third is
+// determined implicitly by the path POSTed to. For more information refer to
+// the documentation for the go tool:
+//
+// https://golang.org/cmd/go/#hdr-Remote_import_paths
 type Package struct {
-	Vcs  vcs    `json:"vcs"`
+	//Vcs (version control system) supported: "git", "mercurial"
-	Path string `json:"path"`
+	Vcs vcs `json:"vcs"`
+	// Repo: the remote repository url
 	Repo string `json:"repo"`
+
+	path string
 }
 
 func (p Package) String() string {
 	return fmt.Sprintf(
 		"<meta name=\"go-import\" content=\"%s %s %s\">",
-		p.Path,
+		p.path,
 		p.Vcs,
 		p.Repo,
 	)

vain_test.go

@@ -7,7 +7,7 @@ import (
 
 func TestString(t *testing.T) {
 	p := Package{
-		Path: "mcquay.me/bps",
+		path: "mcquay.me/bps",
 		Repo: "https://s.mcquay.me/sm/bps",
 	}
 	got := fmt.Sprintf("%s", p)
@@ -49,37 +49,37 @@ func TestValid(t *testing.T) {
 		},
 		{
 			pkgs: []Package{
-				{Path: ""},
+				{path: ""},
 			},
 			in:   "bobo",
 			want: true,
 		},
 		{
 			pkgs: []Package{
-				{Path: "bobo"},
+				{path: "bobo"},
 			},
 			in:   "bobo",
 			want: false,
 		},
 		{
 			pkgs: []Package{
-				{Path: "a/b/c"},
+				{path: "a/b/c"},
 			},
 			in:   "a/b/c",
 			want: false,
 		},
 		{
 			pkgs: []Package{
-				{Path: "foo/bar"},
+				{path: "foo/bar"},
-				{Path: "foo/baz"},
+				{path: "foo/baz"},
 			},
 			in:   "foo",
 			want: false,
 		},
 		{
 			pkgs: []Package{
-				{Path: "bilbo"},
+				{path: "bilbo"},
-				{Path: "frodo"},
+				{path: "frodo"},
 			},
 			in:   "foo/bar/baz",
 			want: true,