Merge lp:~pedronis/ubuntu-push/api-handling-dry into lp:ubuntu-push/automatic

[Samuele Pedroni]

refactor api handling to avoid repetition and have just one ServeHTTP

refactor api handling to avoid repetition and have just one ServeHTTP

[lp:~pedronis/ubuntu-push/api-handling-dry]

199. By Samuele Pedroni on 2014-06-25

Merged better-ok-matching into api-handling-dry.

[Guillermo Gonzalez (verterok)]

+1

review: Approve

=== modified file 'server/api/handlers.go'
--- server/api/handlers.go	2014-06-02 15:32:07 +0000
+++ server/api/handlers.go	2014-06-25 11:08:44 +0000
@@ -277,75 +277,88 @@
 	return sto, nil
 }
 
-func (ctx *context) prepare(w http.ResponseWriter, request *http.Request, reqObj interface{}) (store.PendingStore, *APIError) {
+// JSONPostHandler is able to handle POST requests with a JSON body
+// delegating for the actual details.
+type JSONPostHandler struct {
+	*context
+	parsingBodyObj func() interface{}
+	doHandle       func(ctx *context, sto store.PendingStore, parsedBodyObj interface{}) (map[string]interface{}, *APIError)
+}
+
+func (h *JSONPostHandler) prepare(w http.ResponseWriter, request *http.Request) (interface{}, store.PendingStore, *APIError) {
 	body, apiErr := ReadBody(request, MaxRequestBodyBytes)
 	if apiErr != nil {
-		return nil, apiErr
+		return nil, nil, apiErr
 	}
 
-	err := json.Unmarshal(body, reqObj)
+	parsedBodyObj := h.parsingBodyObj()
+	err := json.Unmarshal(body, parsedBodyObj)
 	if err != nil {
-		return nil, ErrMalformedJSONObject
-	}
-
-	return ctx.getStore(w, request)
-}
-
-type BroadcastHandler struct {
-	*context
-}
-
-func (h *BroadcastHandler) doBroadcast(sto store.PendingStore, bcast *Broadcast) *APIError {
+		return nil, nil, ErrMalformedJSONObject
+	}
+
+	sto, apiErr := h.getStore(w, request)
+	if apiErr != nil {
+		return nil, nil, apiErr
+	}
+	return parsedBodyObj, sto, nil
+}
+
+func (h *JSONPostHandler) ServeHTTP(writer http.ResponseWriter, request *http.Request) {
+	var apiErr *APIError
+	defer func() {
+		if apiErr != nil {
+			RespondError(writer, apiErr)
+		}
+	}()
+
+	parsedBodyObj, sto, apiErr := h.prepare(writer, request)
+	if apiErr != nil {
+		return
+	}
+	defer sto.Close()
+
+	res, apiErr := h.doHandle(h.context, sto, parsedBodyObj)
+	if apiErr != nil {
+		return
+	}
+
+	writer.Header().Set("Content-Type", "application/json")
+	if res == nil {
+		fmt.Fprintf(writer, `{"ok":true}`)
+	} else {
+		res["ok"] = true
+		resp, err := json.Marshal(res)
+		if err != nil {
+			panic(fmt.Errorf("couldn't marshal our own response: %v", err))
+		}
+		writer.Write(resp)
+	}
+}
+
+func doBroadcast(ctx *context, sto store.PendingStore, parsedBodyObj interface{}) (map[string]interface{}, *APIError) {
+	bcast := parsedBodyObj.(*Broadcast)
 	expire, apiErr := checkBroadcast(bcast)
 	if apiErr != nil {
-		return apiErr
+		return nil, apiErr
 	}
 	chanId, err := sto.GetInternalChannelId(bcast.Channel)
 	if err != nil {
 		switch err {
 		case store.ErrUnknownChannel:
-			return ErrUnknownChannel
+			return nil, ErrUnknownChannel
 		default:
-			return ErrUnknown
+			return nil, ErrUnknown
 		}
 	}
 	err = sto.AppendToChannel(chanId, bcast.Data, expire)
 	if err != nil {
-		h.logger.Errorf("could not store notification: %v", err)
-		return ErrCouldNotStoreNotification
-	}
-
-	h.broker.Broadcast(chanId)
-	return nil
-}
-
-func (h *BroadcastHandler) ServeHTTP(writer http.ResponseWriter, request *http.Request) {
-	var apiErr *APIError
-	defer func() {
-		if apiErr != nil {
-			RespondError(writer, apiErr)
-		}
-	}()
-
-	broadcast := &Broadcast{}
-
-	sto, apiErr := h.prepare(writer, request, broadcast)
-	if apiErr != nil {
-		return
-	}
-	defer sto.Close()
-
-	apiErr = h.doBroadcast(sto, broadcast)
-	if apiErr != nil {
-		return
-	}
-
-	writer.Header().Set("Content-Type", "application/json")
-	fmt.Fprintf(writer, `{"ok":true}`)
-}
-
-type UnicastHandler struct {
-	*context
+		ctx.logger.Errorf("could not store notification: %v", err)
+		return nil, ErrCouldNotStoreNotification
+	}
+
+	ctx.broker.Broadcast(chanId)
+	return nil, nil
 }
 
 func checkUnicast(ucast *Unicast) (time.Time, *APIError) {
@@ -363,62 +376,34 @@
 	return base64.StdEncoding.EncodeToString(uuid.NewUUID())
 }
 
-func (h *UnicastHandler) doUnicast(sto store.PendingStore, ucast *Unicast) *APIError {
+func doUnicast(ctx *context, sto store.PendingStore, parsedBodyObj interface{}) (map[string]interface{}, *APIError) {
+	ucast := parsedBodyObj.(*Unicast)
 	expire, apiErr := checkUnicast(ucast)
 	if apiErr != nil {
-		return apiErr
+		return nil, apiErr
 	}
 	chanId, err := sto.GetInternalChannelIdFromToken(ucast.Token, ucast.AppId, ucast.UserId, ucast.DeviceId)
 	if err != nil {
 		switch err {
 		case store.ErrUnknownToken:
-			return ErrUnknownToken
+			return nil, ErrUnknownToken
 		case store.ErrUnauthorized:
-			return ErrUnauthorized
+			return nil, ErrUnauthorized
 		default:
-			h.logger.Errorf("could not resolve token: %v", err)
-			return ErrCouldNotResolveToken
+			ctx.logger.Errorf("could not resolve token: %v", err)
+			return nil, ErrCouldNotResolveToken
 		}
 	}
 
 	msgId := generateMsgId()
 	err = sto.AppendToUnicastChannel(chanId, ucast.AppId, ucast.Data, msgId, expire)
 	if err != nil {
-		h.logger.Errorf("could not store notification: %v", err)
-		return ErrCouldNotStoreNotification
-	}
-
-	h.broker.Unicast(chanId)
-	return nil
-}
-
-func (h *UnicastHandler) ServeHTTP(writer http.ResponseWriter, request *http.Request) {
-	var apiErr *APIError
-	defer func() {
-		if apiErr != nil {
-			RespondError(writer, apiErr)
-		}
-	}()
-
-	unicast := &Unicast{}
-
-	sto, apiErr := h.prepare(writer, request, unicast)
-	if apiErr != nil {
-		return
-	}
-	defer sto.Close()
-
-	apiErr = h.doUnicast(sto, unicast)
-	if apiErr != nil {
-		return
-	}
-
-	writer.Header().Set("Content-Type", "application/json")
-	fmt.Fprintf(writer, `{"ok":true}`)
-}
-
-type RegisterHandler struct {
-	*context
+		ctx.logger.Errorf("could not store notification: %v", err)
+		return nil, ErrCouldNotStoreNotification
+	}
+
+	ctx.broker.Unicast(chanId)
+	return nil, nil
 }
 
 func checkRegister(reg *Registration) *APIError {
@@ -428,49 +413,18 @@
 	return nil
 }
 
-func (h *RegisterHandler) doRegister(sto store.PendingStore, reg *Registration) (string, *APIError) {
+func doRegister(ctx *context, sto store.PendingStore, parsedBodyObj interface{}) (map[string]interface{}, *APIError) {
+	reg := parsedBodyObj.(*Registration)
 	apiErr := checkRegister(reg)
 	if apiErr != nil {
-		return "", apiErr
+		return nil, apiErr
 	}
 	token, err := sto.Register(reg.DeviceId, reg.AppId)
 	if err != nil {
-		h.logger.Errorf("could not make a token: %v", err)
-		return "", ErrCouldNotMakeToken
-	}
-	return token, nil
-}
-
-func (h *RegisterHandler) ServeHTTP(writer http.ResponseWriter, request *http.Request) {
-	var apiErr *APIError
-	defer func() {
-		if apiErr != nil {
-			RespondError(writer, apiErr)
-		}
-	}()
-
-	reg := &Registration{}
-
-	sto, apiErr := h.prepare(writer, request, reg)
-	if apiErr != nil {
-		return
-	}
-	defer sto.Close()
-
-	token, apiErr := h.doRegister(sto, reg)
-	if apiErr != nil {
-		return
-	}
-
-	writer.Header().Set("Content-Type", "application/json")
-	res, err := json.Marshal(map[string]interface{}{
-		"ok":    true,
-		"token": token,
-	})
-	if err != nil {
-		panic(fmt.Errorf("couldn't marshal our own response: %v", err))
-	}
-	writer.Write(res)
+		ctx.logger.Errorf("could not make a token: %v", err)
+		return nil, ErrCouldNotMakeToken
+	}
+	return map[string]interface{}{"token": token}, nil
 }
 
 // MakeHandlersMux makes a handler that dispatches for the various API endpoints.
@@ -481,8 +435,20 @@
 		logger:          logger,
 	}
 	mux := http.NewServeMux()
-	mux.Handle("/broadcast", &BroadcastHandler{context: ctx})
-	mux.Handle("/notify", &UnicastHandler{context: ctx})
-	mux.Handle("/register", &RegisterHandler{context: ctx})
+	mux.Handle("/broadcast", &JSONPostHandler{
+		context:        ctx,
+		parsingBodyObj: func() interface{} { return &Broadcast{} },
+		doHandle:       doBroadcast,
+	})
+	mux.Handle("/notify", &JSONPostHandler{
+		context:        ctx,
+		parsingBodyObj: func() interface{} { return &Unicast{} },
+		doHandle:       doUnicast,
+	})
+	mux.Handle("/register", &JSONPostHandler{
+		context:        ctx,
+		parsingBodyObj: func() interface{} { return &Registration{} },
+		doHandle:       doRegister,
+	})
 	return mux
 }

=== modified file 'server/api/handlers_test.go'
--- server/api/handlers_test.go	2014-06-25 11:00:15 +0000
+++ server/api/handlers_test.go	2014-06-25 11:08:44 +0000
@@ -162,14 +162,15 @@
 func (s *handlersSuite) TestDoBroadcast(c *C) {
 	sto := store.NewInMemoryPendingStore()
 	bsend := &checkBrokerSending{store: sto}
-	bh := &BroadcastHandler{&context{nil, bsend, nil}}
+	ctx := &context{nil, bsend, nil}
 	payload := json.RawMessage(`{"a": 1}`)
-	apiErr := bh.doBroadcast(sto, &Broadcast{
+	res, apiErr := doBroadcast(ctx, sto, &Broadcast{
 		Channel:  "system",
 		ExpireOn: future,
 		Data:     payload,
 	})
-	c.Check(apiErr, IsNil)
+	c.Assert(apiErr, IsNil)
+	c.Assert(res, IsNil)
 	c.Check(bsend.err, IsNil)
 	c.Check(bsend.chanId, Equals, store.SystemInternalChannelId)
 	c.Check(bsend.top, Equals, int64(1))
@@ -178,8 +179,7 @@
 
 func (s *handlersSuite) TestDoBroadcastUnknownChannel(c *C) {
 	sto := store.NewInMemoryPendingStore()
-	bh := &BroadcastHandler{}
-	apiErr := bh.doBroadcast(sto, &Broadcast{
+	_, apiErr := doBroadcast(nil, sto, &Broadcast{
 		Channel:  "unknown",
 		ExpireOn: future,
 		Data:     json.RawMessage(`{"a": 1}`),
@@ -224,8 +224,7 @@
 			return errors.New("other")
 		},
 	}
-	bh := &BroadcastHandler{}
-	apiErr := bh.doBroadcast(sto, &Broadcast{
+	_, apiErr := doBroadcast(nil, sto, &Broadcast{
 		Channel:  "system",
 		ExpireOn: future,
 		Data:     json.RawMessage(`{"a": 1}`),
@@ -244,8 +243,7 @@
 		},
 	}
 	ctx := &context{logger: s.testlog}
-	bh := &BroadcastHandler{ctx}
-	apiErr := bh.doBroadcast(sto, &Broadcast{
+	_, apiErr := doBroadcast(ctx, sto, &Broadcast{
 		Channel:  "system",
 		ExpireOn: future,
 		Data:     json.RawMessage(`{"a": 1}`),
@@ -316,16 +314,17 @@
 	}
 	sto := store.NewInMemoryPendingStore()
 	bsend := &checkBrokerSending{store: sto}
-	bh := &UnicastHandler{&context{nil, bsend, nil}}
+	ctx := &context{nil, bsend, nil}
 	payload := json.RawMessage(`{"a": 1}`)
-	apiErr := bh.doUnicast(sto, &Unicast{
+	res, apiErr := doUnicast(ctx, sto, &Unicast{
 		UserId:   "user1",
 		DeviceId: "DEV1",
 		AppId:    "app1",
 		ExpireOn: future,
 		Data:     payload,
 	})
-	c.Check(apiErr, IsNil)
+	c.Assert(apiErr, IsNil)
+	c.Check(res, IsNil)
 	c.Check(bsend.err, IsNil)
 	c.Check(bsend.chanId, Equals, store.UnicastInternalChannelId("user1", "DEV1"))
 	c.Check(bsend.top, Equals, int64(0))
@@ -340,8 +339,7 @@
 
 func (s *handlersSuite) TestDoUnicastMissingIdField(c *C) {
 	sto := store.NewInMemoryPendingStore()
-	bh := &UnicastHandler{}
-	apiErr := bh.doUnicast(sto, &Unicast{
+	_, apiErr := doUnicast(nil, sto, &Unicast{
 		ExpireOn: future,
 		Data:     json.RawMessage(`{"a": 1}`),
 	})
@@ -359,8 +357,7 @@
 		},
 	}
 	ctx := &context{logger: s.testlog}
-	bh := &UnicastHandler{ctx}
-	apiErr := bh.doUnicast(sto, &Unicast{
+	_, apiErr := doUnicast(ctx, sto, &Unicast{
 		UserId:   "user1",
 		DeviceId: "DEV1",
 		AppId:    "app1",
@@ -383,24 +380,23 @@
 		},
 	}
 	ctx := &context{logger: s.testlog}
-	bh := &UnicastHandler{ctx}
 	u := &Unicast{
 		Token:    "tok",
 		AppId:    "app1",
 		ExpireOn: future,
 		Data:     json.RawMessage(`{"a": 1}`),
 	}
-	apiErr := bh.doUnicast(sto, u)
+	_, apiErr := doUnicast(ctx, sto, u)
 	c.Check(apiErr, Equals, ErrCouldNotResolveToken)
 	c.Check(s.testlog.Captured(), Equals, "ERROR could not resolve token: fail\n")
 	s.testlog.ResetCapture()
 
 	fail = store.ErrUnknownToken
-	apiErr = bh.doUnicast(sto, u)
+	_, apiErr = doUnicast(ctx, sto, u)
 	c.Check(apiErr, Equals, ErrUnknownToken)
 	c.Check(s.testlog.Captured(), Equals, "")
 	fail = store.ErrUnauthorized
-	apiErr = bh.doUnicast(sto, u)
+	_, apiErr = doUnicast(ctx, sto, u)
 	c.Check(apiErr, Equals, ErrUnauthorized)
 	c.Check(s.testlog.Captured(), Equals, "")
 }
@@ -533,7 +529,11 @@
 		return store.NewInMemoryPendingStore(), nil
 	}
 	ctx := &context{stoForReq, nil, nil}
-	testServer := httptest.NewServer(&BroadcastHandler{ctx})
+	testServer := httptest.NewServer(&JSONPostHandler{
+		context:        ctx,
+		parsingBodyObj: func() interface{} { return &Broadcast{} },
+		doHandle:       doBroadcast,
+	})
 	defer testServer.Close()
 
 	packedMessage := []byte(`{"channel": "system"}`)
@@ -554,7 +554,10 @@
 		return store.NewInMemoryPendingStore(), nil
 	}
 	ctx := &context{stoForReq, nil, nil}
-	testServer := httptest.NewServer(&BroadcastHandler{ctx})
+	testServer := httptest.NewServer(&JSONPostHandler{
+		context:        ctx,
+		parsingBodyObj: func() interface{} { return &Broadcast{} },
+	})
 	defer testServer.Close()
 
 	packedMessage := []byte("{some bogus-message: ")
@@ -571,7 +574,7 @@
 }
 
 func (s *handlersSuite) TestCannotBroadcastTooBigMessages(c *C) {
-	testServer := httptest.NewServer(&BroadcastHandler{})
+	testServer := httptest.NewServer(&JSONPostHandler{})
 	defer testServer.Close()
 
 	bigString := strings.Repeat("a", MaxRequestBodyBytes)
@@ -589,7 +592,7 @@
 }
 
 func (s *handlersSuite) TestCannotBroadcastWithoutContentLength(c *C) {
-	testServer := httptest.NewServer(&BroadcastHandler{})
+	testServer := httptest.NewServer(&JSONPostHandler{})
 	defer testServer.Close()
 
 	dataString := `{"foo":"bar"}`
@@ -607,7 +610,7 @@
 }
 
 func (s *handlersSuite) TestCannotBroadcastEmptyMessages(c *C) {
-	testServer := httptest.NewServer(&BroadcastHandler{})
+	testServer := httptest.NewServer(&JSONPostHandler{})
 	defer testServer.Close()
 
 	packedMessage := make([]byte, 0)
@@ -624,7 +627,7 @@
 }
 
 func (s *handlersSuite) TestCannotBroadcastNonJSONMessages(c *C) {
-	testServer := httptest.NewServer(&BroadcastHandler{})
+	testServer := httptest.NewServer(&JSONPostHandler{})
 	defer testServer.Close()
 
 	dataString := `{"foo":"bar"}`
@@ -642,7 +645,7 @@
 }
 
 func (s *handlersSuite) TestCannotBroadcastNonPostMessages(c *C) {
-	testServer := httptest.NewServer(&BroadcastHandler{})
+	testServer := httptest.NewServer(&JSONPostHandler{})
 	defer testServer.Close()
 
 	dataString := `{"foo":"bar"}`
@@ -703,40 +706,6 @@
 	c.Check(notifications, HasLen, 1)
 }
 
-func (s *handlersSuite) TestCannotUnicastTooBigMessages(c *C) {
-	testServer := httptest.NewServer(&UnicastHandler{})
-	defer testServer.Close()
-
-	bigString := strings.Repeat("a", MaxRequestBodyBytes)
-	dataString := fmt.Sprintf(`"%v"`, bigString)
-
-	request := newPostRequest("/", &Unicast{
-		ExpireOn: future,
-		Data:     json.RawMessage([]byte(dataString)),
-	}, testServer)
-
-	response, err := s.client.Do(request)
-	c.Assert(err, IsNil)
-	checkError(c, response, ErrRequestBodyTooLarge)
-}
-
-func (s *handlersSuite) TestCannotUnicastWithMissingFields(c *C) {
-	stoForReq := func(http.ResponseWriter, *http.Request) (store.PendingStore, error) {
-		return store.NewInMemoryPendingStore(), nil
-	}
-	ctx := &context{stoForReq, nil, nil}
-	testServer := httptest.NewServer(&UnicastHandler{ctx})
-	defer testServer.Close()
-
-	request := newPostRequest("/", &Unicast{
-		Data: json.RawMessage(`{"foo":"bar"}`),
-	}, testServer)
-
-	response, err := s.client.Do(request)
-	c.Assert(err, IsNil)
-	checkError(c, response, ErrMissingIdField)
-}
-
 func (s *handlersSuite) TestCheckRegister(c *C) {
 	registration := func() *Registration {
 		return &Registration{
@@ -761,10 +730,9 @@
 
 func (s *handlersSuite) TestDoRegisterMissingIdField(c *C) {
 	sto := store.NewInMemoryPendingStore()
-	rh := &RegisterHandler{}
-	token, apiErr := rh.doRegister(sto, &Registration{})
+	token, apiErr := doRegister(nil, sto, &Registration{})
 	c.Check(apiErr, Equals, ErrMissingIdField)
-	c.Check(token, Equals, "")
+	c.Check(token, IsNil)
 }
 
 func (s *handlersSuite) TestDoRegisterCouldNotMakeToken(c *C) {
@@ -778,8 +746,7 @@
 		},
 	}
 	ctx := &context{logger: s.testlog}
-	rh := &RegisterHandler{ctx}
-	_, apiErr := rh.doRegister(sto, &Registration{
+	_, apiErr := doRegister(ctx, sto, &Registration{
 		DeviceId: "DEV1",
 		AppId:    "app1",
 	})
@@ -842,36 +809,3 @@
 	c.Check(top, Equals, int64(0))
 	c.Check(notifications, HasLen, 1)
 }
-
-func (s *handlersSuite) TestCannotRegisterWithMissingFields(c *C) {
-	stoForReq := func(http.ResponseWriter, *http.Request) (store.PendingStore, error) {
-		return store.NewInMemoryPendingStore(), nil
-	}
-	ctx := &context{stoForReq, nil, nil}
-	testServer := httptest.NewServer(&RegisterHandler{ctx})
-	defer testServer.Close()
-
-	request := newPostRequest("/", &Registration{
-		DeviceId: "DEV1",
-	}, testServer)
-
-	response, err := s.client.Do(request)
-	c.Assert(err, IsNil)
-	checkError(c, response, ErrMissingIdField)
-}
-
-func (s *handlersSuite) TestCannotRegisterWithNonPOST(c *C) {
-	stoForReq := func(http.ResponseWriter, *http.Request) (store.PendingStore, error) {
-		return store.NewInMemoryPendingStore(), nil
-	}
-	ctx := &context{stoForReq, nil, nil}
-	testServer := httptest.NewServer(&RegisterHandler{ctx})
-	defer testServer.Close()
-
-	request, err := http.NewRequest("GET", testServer.URL, nil)
-	c.Assert(err, IsNil)
-
-	response, err := s.client.Do(request)
-	c.Assert(err, IsNil)
-	checkError(c, response, ErrWrongRequestMethod)
-}