RTDB error handling revamp (#345)

hiranya911 · web-flow · commit 3c1a8e333771 · 2020-03-10T10:26:45.000-07:00
diff --git a/db/db.go b/db/db.go
@@ -68,17 +68,8 @@ func NewClient(ctx context.Context, c *internal.DatabaseConfig) (*Client, error)
 		return nil, err
 	}
 
-	ep := func(b []byte) string {
-		var p struct {
-			Error string `json:"error"`
-		}
-		if err := json.Unmarshal(b, &p); err != nil {
-			return ""
-		}
-		return p.Error
-	}
-	hc.ErrParser = ep
-
+	hc.CreateErrFn = handleRTDBError
+	hc.SuccessFn = internal.HasSuccessStatus
 	return &Client{
 		hc:           hc,
 		url:          fmt.Sprintf("https://%s", p.Host),
@@ -102,24 +93,18 @@ func (c *Client) NewRef(path string) *Ref {
 	}
 }
 
-func (c *Client) send(
-	ctx context.Context,
-	method, path string,
-	body internal.HTTPEntity,
-	opts ...internal.HTTPOption) (*internal.Response, error) {
-
-	if strings.ContainsAny(path, invalidChars) {
-		return nil, fmt.Errorf("invalid path with illegal characters: %q", path)
+func (c *Client) sendAndUnmarshal(
+	ctx context.Context, req *internal.Request, v interface{}) (*internal.Response, error) {
+	if strings.ContainsAny(req.URL, invalidChars) {
+		return nil, fmt.Errorf("invalid path with illegal characters: %q", req.URL)
 	}
+
+	req.URL = fmt.Sprintf("%s%s.json", c.url, req.URL)
 	if c.authOverride != "" {
-		opts = append(opts, internal.WithQueryParam(authVarOverride, c.authOverride))
+		req.Opts = append(req.Opts, internal.WithQueryParam(authVarOverride, c.authOverride))
 	}
-	return c.hc.Do(ctx, &internal.Request{
-		Method: method,
-		URL:    fmt.Sprintf("%s%s.json", c.url, path),
-		Body:   body,
-		Opts:   opts,
-	})
+
+	return c.hc.DoAndUnmarshal(ctx, req, v)
 }
 
 func parsePath(path string) []string {
@@ -131,3 +116,16 @@ func parsePath(path string) []string {
 	}
 	return segs
 }
+
+func handleRTDBError(resp *internal.Response) error {
+	err := internal.NewFirebaseError(resp)
+	var p struct {
+		Error string `json:"error"`
+	}
+	json.Unmarshal(resp.Body, &p)
+	if p.Error != "" {
+		err.String = fmt.Sprintf("http error status: %d; reason: %s", resp.Status, p.Error)
+	}
+
+	return err
+}
diff --git a/db/query.go b/db/query.go
@@ -109,11 +109,14 @@ func (q *Query) Get(ctx context.Context, v interface{}) error {
 	if err := initQueryParams(q, qp); err != nil {
 		return err
 	}
-	resp, err := q.client.send(ctx, "GET", q.path, nil, internal.WithQueryParams(qp))
-	if err != nil {
-		return err
+
+	req := &internal.Request{
+		Method: http.MethodGet,
+		URL:    q.path,
+		Opts:   []internal.HTTPOption{internal.WithQueryParams(qp)},
 	}
-	return resp.Unmarshal(http.StatusOK, v)
+	_, err := q.client.sendAndUnmarshal(ctx, req, v)
+	return err
 }
 
 // GetOrdered executes the Query and returns the results as an ordered slice.
diff --git a/db/query_test.go b/db/query_test.go
@@ -16,8 +16,11 @@ package db
 import (
 	"context"
 	"fmt"
+	"net/http"
 	"reflect"
 	"testing"
+
+	"firebase.google.com/go/v4/errorutils"
 )
 
 var sortableKeysResp = map[string]interface{}{
@@ -768,10 +771,15 @@ func TestQueryHttpError(t *testing.T) {
 
 	want := "http error status: 500; reason: test error"
 	result, err := testref.OrderByChild("child").GetOrdered(context.Background())
-	if err == nil || err.Error() != want {
-		t.Errorf("GetOrdered() = %v; want = %v", err, want)
+	if result != nil || err == nil || err.Error() != want {
+		t.Fatalf("GetOrdered() = (%v, %v); want = (nil, %v)", result, err, want)
 	}
-	if result != nil {
-		t.Errorf("GetOrdered() = %v; want = nil", result)
+	if !errorutils.IsInternal(err) {
+		t.Errorf("IsInternal(err) = false; want = true")
+	}
+
+	resp := errorutils.HTTPResponse(err)
+	if resp == nil || resp.StatusCode != http.StatusInternalServerError {
+		t.Errorf("HTTPResponse(err) = %v; want = {StatusCode: %d}", resp, http.StatusInternalServerError)
 	}
 }
diff --git a/db/ref.go b/db/ref.go
@@ -75,33 +75,41 @@ func (r *Ref) Child(path string) *Ref {
 // therefore v has the same requirements as the json package. Specifically, it must be a pointer,
 // and must not be nil.
 func (r *Ref) Get(ctx context.Context, v interface{}) error {
-	resp, err := r.send(ctx, "GET")
-	if err != nil {
-		return err
+	req := &internal.Request{
+		Method: http.MethodGet,
 	}
-	return resp.Unmarshal(http.StatusOK, v)
+	_, err := r.sendAndUnmarshal(ctx, req, v)
+	return err
 }
 
 // GetWithETag retrieves the value at the current database location, along with its ETag.
 func (r *Ref) GetWithETag(ctx context.Context, v interface{}) (string, error) {
-	resp, err := r.send(ctx, "GET", internal.WithHeader("X-Firebase-ETag", "true"))
+	req := &internal.Request{
+		Method: http.MethodGet,
+		Opts: []internal.HTTPOption{
+			internal.WithHeader("X-Firebase-ETag", "true"),
+		},
+	}
+	resp, err := r.sendAndUnmarshal(ctx, req, v)
 	if err != nil {
 		return "", err
-	} else if err := resp.Unmarshal(http.StatusOK, v); err != nil {
-		return "", err
 	}
+
 	return resp.Header.Get("Etag"), nil
 }
 
 // GetShallow performs a shallow read on the current database location.
 //
 // Shallow reads do not retrieve the child nodes of the current reference.
 func (r *Ref) GetShallow(ctx context.Context, v interface{}) error {
-	resp, err := r.send(ctx, "GET", internal.WithQueryParam("shallow", "true"))
-	if err != nil {
-		return err
+	req := &internal.Request{
+		Method: http.MethodGet,
+		Opts: []internal.HTTPOption{
+			internal.WithQueryParam("shallow", "true"),
+		},
 	}
-	return resp.Unmarshal(http.StatusOK, v)
+	_, err := r.sendAndUnmarshal(ctx, req, v)
+	return err
 }
 
 // GetIfChanged retrieves the value and ETag of the current database location only if the specified
@@ -112,16 +120,26 @@ func (r *Ref) GetShallow(ctx context.Context, v interface{}) error {
 // If the etag matches, returns false along with the same ETag passed into the function. No data
 // will be stored in v in this case.
 func (r *Ref) GetIfChanged(ctx context.Context, etag string, v interface{}) (bool, string, error) {
-	resp, err := r.send(ctx, "GET", internal.WithHeader("If-None-Match", etag))
+	req := &internal.Request{
+		Method: http.MethodGet,
+		Opts: []internal.HTTPOption{
+			internal.WithHeader("If-None-Match", etag),
+		},
+		SuccessFn: successOrNotModified,
+	}
+	resp, err := r.sendAndUnmarshal(ctx, req, nil)
 	if err != nil {
 		return false, "", err
 	}
+
 	if resp.Status == http.StatusNotModified {
 		return false, etag, nil
 	}
-	if err := resp.Unmarshal(http.StatusOK, v); err != nil {
+
+	if err := json.Unmarshal(resp.Body, v); err != nil {
 		return false, "", err
 	}
+
 	return true, resp.Header.Get("ETag"), nil
 }
 
@@ -131,28 +149,39 @@ func (r *Ref) GetIfChanged(ctx context.Context, etag string, v interface{}) (boo
 // v has the same requirements as the json package. Values like functions and channels cannot be
 // saved into Realtime Database.
 func (r *Ref) Set(ctx context.Context, v interface{}) error {
-	resp, err := r.sendWithBody(ctx, "PUT", v, internal.WithQueryParam("print", "silent"))
-	if err != nil {
-		return err
+	req := &internal.Request{
+		Method: http.MethodPut,
+		Body:   internal.NewJSONEntity(v),
+		Opts: []internal.HTTPOption{
+			internal.WithQueryParam("print", "silent"),
+		},
 	}
-	return resp.CheckStatus(http.StatusNoContent)
+	_, err := r.sendAndUnmarshal(ctx, req, nil)
+	return err
 }
 
 // SetIfUnchanged conditionally sets the data at this location to the given value.
 //
 // Sets the data at this location to v only if the specified ETag matches. Returns true if the
 // value is written. Returns false if no changes are made to the database.
 func (r *Ref) SetIfUnchanged(ctx context.Context, etag string, v interface{}) (bool, error) {
-	resp, err := r.sendWithBody(ctx, "PUT", v, internal.WithHeader("If-Match", etag))
+	req := &internal.Request{
+		Method: http.MethodPut,
+		Body:   internal.NewJSONEntity(v),
+		Opts: []internal.HTTPOption{
+			internal.WithHeader("If-Match", etag),
+		},
+		SuccessFn: successOrPreconditionFailed,
+	}
+	resp, err := r.sendAndUnmarshal(ctx, req, nil)
 	if err != nil {
 		return false, err
 	}
+
 	if resp.Status == http.StatusPreconditionFailed {
 		return false, nil
 	}
-	if err := resp.CheckStatus(http.StatusOK); err != nil {
-		return false, err
-	}
+
 	return true, nil
 }
 
@@ -164,16 +193,18 @@ func (r *Ref) Push(ctx context.Context, v interface{}) (*Ref, error) {
 	if v == nil {
 		v = ""
 	}
-	resp, err := r.sendWithBody(ctx, "POST", v)
-	if err != nil {
-		return nil, err
+
+	req := &internal.Request{
+		Method: http.MethodPost,
+		Body:   internal.NewJSONEntity(v),
 	}
 	var d struct {
 		Name string `json:"name"`
 	}
-	if err := resp.Unmarshal(http.StatusOK, &d); err != nil {
+	if _, err := r.sendAndUnmarshal(ctx, req, &d); err != nil {
 		return nil, err
 	}
+
 	return r.Child(d.Name), nil
 }
 
@@ -182,11 +213,16 @@ func (r *Ref) Update(ctx context.Context, v map[string]interface{}) error {
 	if len(v) == 0 {
 		return fmt.Errorf("value argument must be a non-empty map")
 	}
-	resp, err := r.sendWithBody(ctx, "PATCH", v, internal.WithQueryParam("print", "silent"))
-	if err != nil {
-		return err
+
+	req := &internal.Request{
+		Method: http.MethodPatch,
+		Body:   internal.NewJSONEntity(v),
+		Opts: []internal.HTTPOption{
+			internal.WithQueryParam("print", "silent"),
+		},
 	}
-	return resp.CheckStatus(http.StatusNoContent)
+	_, err := r.sendAndUnmarshal(ctx, req, nil)
+	return err
 }
 
 // UpdateFn represents a function type that can be passed into Transaction().
@@ -207,55 +243,65 @@ type UpdateFn func(TransactionNode) (interface{}, error)
 // The update function may also force an early abort by returning an error instead of returning a
 // value.
 func (r *Ref) Transaction(ctx context.Context, fn UpdateFn) error {
-	resp, err := r.send(ctx, "GET", internal.WithHeader("X-Firebase-ETag", "true"))
+	req := &internal.Request{
+		Method: http.MethodGet,
+		Opts: []internal.HTTPOption{
+			internal.WithHeader("X-Firebase-ETag", "true"),
+		},
+	}
+	resp, err := r.sendAndUnmarshal(ctx, req, nil)
 	if err != nil {
 		return err
-	} else if err := resp.CheckStatus(http.StatusOK); err != nil {
-		return err
 	}
-	etag := resp.Header.Get("Etag")
 
+	etag := resp.Header.Get("Etag")
 	for i := 0; i < txnRetries; i++ {
 		new, err := fn(&transactionNodeImpl{resp.Body})
 		if err != nil {
 			return err
 		}
-		resp, err = r.sendWithBody(ctx, "PUT", new, internal.WithHeader("If-Match", etag))
+
+		req := &internal.Request{
+			Method: http.MethodPut,
+			Body:   internal.NewJSONEntity(new),
+			Opts: []internal.HTTPOption{
+				internal.WithHeader("If-Match", etag),
+			},
+			SuccessFn: successOrPreconditionFailed,
+		}
+		resp, err = r.sendAndUnmarshal(ctx, req, nil)
 		if err != nil {
 			return err
 		}
+
 		if resp.Status == http.StatusOK {
 			return nil
-		} else if err := resp.CheckStatus(http.StatusPreconditionFailed); err != nil {
-			return err
 		}
+
 		etag = resp.Header.Get("ETag")
 	}
 	return fmt.Errorf("transaction aborted after failed retries")
 }
 
 // Delete removes this node from the database.
 func (r *Ref) Delete(ctx context.Context) error {
-	resp, err := r.send(ctx, "DELETE")
-	if err != nil {
-		return err
+	req := &internal.Request{
+		Method: http.MethodDelete,
 	}
-	return resp.CheckStatus(http.StatusOK)
+	_, err := r.sendAndUnmarshal(ctx, req, nil)
+	return err
 }
 
-func (r *Ref) send(
-	ctx context.Context,
-	method string,
-	opts ...internal.HTTPOption) (*internal.Response, error) {
-
-	return r.client.send(ctx, method, r.Path, nil, opts...)
+func (r *Ref) sendAndUnmarshal(
+	ctx context.Context, req *internal.Request, v interface{}) (*internal.Response, error) {
+	req.URL = r.Path
+	return r.client.sendAndUnmarshal(ctx, req, v)
 }
 
-func (r *Ref) sendWithBody(
-	ctx context.Context,
-	method string,
-	body interface{},
-	opts ...internal.HTTPOption) (*internal.Response, error) {
+func successOrNotModified(resp *internal.Response) bool {
+	return internal.HasSuccessStatus(resp) || resp.Status == http.StatusNotModified
+}
 
-	return r.client.send(ctx, method, r.Path, internal.NewJSONEntity(body), opts...)
+func successOrPreconditionFailed(resp *internal.Response) bool {
+	return internal.HasSuccessStatus(resp) || resp.Status == http.StatusPreconditionFailed
 }
diff --git a/db/ref_test.go b/db/ref_test.go
diff --git a/integration/db/db_test.go b/integration/db/db_test.go

Original file line number	Diff line number	Diff line change
`@@ -109,11 +109,14 @@ func (q *Query) Get(ctx context.Context, v interface{}) error {`
`109`	`109`	`if err := initQueryParams(q, qp); err != nil {`
`110`	`110`	`return err`
`111`	`111`	`}`
`112`		`- resp, err := q.client.send(ctx, "GET", q.path, nil, internal.WithQueryParams(qp))`
`113`		`- if err != nil {`
`114`		`- return err`
	`112`	`+`
	`113`	`+ req := &internal.Request{`
	`114`	`+ Method: http.MethodGet,`
	`115`	`+ URL: q.path,`
	`116`	`+ Opts: []internal.HTTPOption{internal.WithQueryParams(qp)},`
`115`	`117`	`}`
`116`		`- return resp.Unmarshal(http.StatusOK, v)`
	`118`	`+ _, err := q.client.sendAndUnmarshal(ctx, req, v)`
	`119`	`+ return err`
`117`	`120`	`}`
`118`	`121`
`119`	`122`	`// GetOrdered executes the Query and returns the results as an ordered slice.`