fix(dbrp): improve DBRP API validation errors

CHANGELOG.md

@@ -34,6 +34,7 @@ Replacement `tsi1` indexes will be automatically generated on startup for shards
 1. [20313](https://github.com/influxdata/influxdb/pull/20313): Fix logging initialization for storage engine.
 1. [20442](https://github.com/influxdata/influxdb/pull/20442): Don't return 500 codes for partial write failures.
 1. [20440](https://github.com/influxdata/influxdb/pull/20440): Add confirmation step w/ file sizes before copying data files in `influxd upgrade`.
+1. [20409](https://github.com/influxdata/influxdb/pull/20409): Improve messages in DBRP API validation errors.
 
 ## v2.0.3 [2020-12-14]
 

dbrp/error.go

@@ -1,16 +1,12 @@
 package dbrp
 
 import (
+	"fmt"
+
 	"github.com/influxdata/influxdb/v2"
 )
 
 var (
-	// ErrInvalidDBRPID is used when the ID of the DBRP cannot be encoded.
-	ErrInvalidDBRPID = &influxdb.Error{
-		Code: influxdb.EInvalid,
-		Msg:  "DBRP ID is invalid",
-	}
-
 	// ErrDBRPNotFound is used when the specified DBRP cannot be found.
 	ErrDBRPNotFound = &influxdb.Error{
 		Code: influxdb.ENotFound,
@@ -29,8 +25,51 @@ var (
 		Code: influxdb.EInternal,
 		Msg:  "unable to generate valid id",
 	}
+
+	ErrNoOrgProvided = &influxdb.Error{
+		Code: influxdb.EInvalid,
+		Msg:  "either 'org' or 'orgID' must be provided",
+	}
 )
 
+// ErrOrgNotFound returns a more informative error about a 404 on org name.
+func ErrOrgNotFound(org string) error {
+	return &influxdb.Error{
+		Code: influxdb.ENotFound,
+		Msg:  fmt.Sprintf("invalid org %q", org),
+		Err:  influxdb.ErrOrgNotFound,
+	}
+}
+
+// ErrInvalidOrgID returns a more informative error about a failure
+// to decode an organization ID.
+func ErrInvalidOrgID(id string, err error) error {
+	return &influxdb.Error{
+		Code: influxdb.EInvalid,
+		Msg:  fmt.Sprintf("invalid org ID %q", id),
+		Err:  err,
+	}
+}
+
+// ErrInvalidBucketID returns a more informative error about a failure
+// to decode a bucket ID.
+func ErrInvalidBucketID(id string, err error) error {
+	return &influxdb.Error{
+		Code: influxdb.EInvalid,
+		Msg:  fmt.Sprintf("invalid bucket ID %q", id),
+		Err:  err,
+	}
+}
+
+// ErrInvalidDBRPID is used when the ID of the DBRP cannot be encoded.
+func ErrInvalidDBRPID(id string, err error) error {
+	return &influxdb.Error{
+		Code: influxdb.EInvalid,
+		Msg:  fmt.Sprintf("invalid DBRP ID %q", id),
+		Err:  err,
+	}
+}
+
 // ErrInvalidDBRP is used when a service was provided an invalid DBRP.
 func ErrInvalidDBRP(err error) *influxdb.Error {
 	return &influxdb.Error{

dbrp/http_client_dbrp.go

@@ -92,8 +92,8 @@ func (c *Client) Create(ctx context.Context, dbrp *influxdb.DBRPMappingV2) error
 			Database:        dbrp.Database,
 			RetentionPolicy: dbrp.RetentionPolicy,
 			Default:         dbrp.Default,
-			OrganizationID:  dbrp.OrganizationID,
-			BucketID:        dbrp.BucketID,
+			OrganizationID:  dbrp.OrganizationID.String(),
+			BucketID:        dbrp.BucketID.String(),
 		}, c.Prefix).
 		DecodeJSON(&newDBRP).
 		Do(ctx); err != nil {

dbrp/http_server_dbrp.go

@@ -56,12 +56,21 @@ func NewHTTPHandler(log *zap.Logger, dbrpSvc influxdb.DBRPMappingServiceV2, orgS
 }
 
 type createDBRPRequest struct {
-	Database        string      `json:"database"`
-	RetentionPolicy string      `json:"retention_policy"`
-	Default         bool        `json:"default"`
-	Org             string      `json:"org"`
-	OrganizationID  influxdb.ID `json:"orgID"`
-	BucketID        influxdb.ID `json:"bucketID"`
+	Database        string `json:"database"`
+	RetentionPolicy string `json:"retention_policy"`
+	Default         bool   `json:"default"`
+	Org             string `json:"org"`
+	// N.B. These are purposefully typed as string instead of
+	// influxdb.ID so we can provide more specific error messages.
+	// If they have the ID type, our JSON decoder will just return
+	// a generic "invalid ID" error without stating which ID is
+	// the problem.
+	//
+	// Ideally we'd fix the decoder so we could get more useful
+	// errors everywhere, but I'm worried about the impact of a
+	// system-wide change to our "invalid ID" error format.
+	OrganizationID string `json:"orgID"`
+	BucketID       string `json:"bucketID"`
 }
 
 func (h *Handler) handlePostDBRP(w http.ResponseWriter, r *http.Request) {
@@ -76,27 +85,38 @@ func (h *Handler) handlePostDBRP(w http.ResponseWriter, r *http.Request) {
 		return
 	}
 
-	if !req.OrganizationID.Valid() {
+	var orgID influxdb.ID
+	var bucketID influxdb.ID
+
+	if req.OrganizationID == "" {
 		if req.Org == "" {
-			h.api.Err(w, r, influxdb.ErrInvalidID)
+			h.api.Err(w, r, ErrNoOrgProvided)
 			return
 		}
 		org, err := h.orgSvc.FindOrganization(r.Context(), influxdb.OrganizationFilter{
 			Name: &req.Org,
 		})
 		if err != nil {
-			h.api.Err(w, r, influxdb.ErrOrgNotFound)
+			h.api.Err(w, r, ErrOrgNotFound(req.Org))
 			return
 		}
-		req.OrganizationID = org.ID
+		orgID = org.ID
+	} else if err := orgID.DecodeFromString(req.OrganizationID); err != nil {
+		h.api.Err(w, r, ErrInvalidOrgID(req.OrganizationID, err))
+		return
+	}
+
+	if err := bucketID.DecodeFromString(req.BucketID); err != nil {
+		h.api.Err(w, r, ErrInvalidBucketID(req.BucketID, err))
+		return
 	}
 
 	dbrp := &influxdb.DBRPMappingV2{
 		Database:        req.Database,
 		RetentionPolicy: req.RetentionPolicy,
 		Default:         req.Default,
-		OrganizationID:  req.OrganizationID,
-		BucketID:        req.BucketID,
+		OrganizationID:  orgID,
+		BucketID:        bucketID,
 	}
 	if err := h.dbrpSvc.Create(ctx, dbrp); err != nil {
 		h.api.Err(w, r, err)
@@ -294,12 +314,12 @@ func (h *Handler) getFilterFromHTTPRequest(r *http.Request) (f influxdb.DBRPMapp
 	return f, nil
 }
 
-func getIDFromHTTPRequest(r *http.Request, key string) (*influxdb.ID, error) {
+func getIDFromHTTPRequest(r *http.Request, key string, onErr func(string, error) error) (*influxdb.ID, error) {
 	var id influxdb.ID
 	raw := r.URL.Query().Get(key)
 	if raw != "" {
 		if err := id.DecodeFromString(raw); err != nil {
-			return nil, influxdb.ErrInvalidID
+			return nil, onErr(raw, err)
 		}
 	} else {
 		return nil, nil
@@ -310,30 +330,30 @@ func getIDFromHTTPRequest(r *http.Request, key string) (*influxdb.ID, error) {
 // mustGetOrgIDFromHTTPRequest returns the org ID parameter from the request, falling
 // back to looking up the org ID by org name if the ID parameter is not present.
 func (h *Handler) mustGetOrgIDFromHTTPRequest(r *http.Request) (*influxdb.ID, error) {
-	orgID, err := getIDFromHTTPRequest(r, "orgID")
+	orgID, err := getIDFromHTTPRequest(r, "orgID", ErrInvalidOrgID)
 	if err != nil {
 		return nil, err
 	}
 	if orgID == nil {
 		name := r.URL.Query().Get("org")
 		if name == "" {
-			return nil, influxdb.ErrOrgNotFound
+			return nil, ErrNoOrgProvided
 		}
 		org, err := h.orgSvc.FindOrganization(r.Context(), influxdb.OrganizationFilter{
 			Name: &name,
 		})
 		if err != nil {
-			return nil, influxdb.ErrOrgNotFound
+			return nil, ErrOrgNotFound(name)
 		}
 		orgID = &org.ID
 	}
 	return orgID, nil
 }
 
 func getDBRPIDFromHTTPRequest(r *http.Request) (*influxdb.ID, error) {
-	return getIDFromHTTPRequest(r, "id")
+	return getIDFromHTTPRequest(r, "id", ErrInvalidDBRPID)
 }
 
 func getBucketIDFromHTTPRequest(r *http.Request) (*influxdb.ID, error) {
-	return getIDFromHTTPRequest(r, "bucketID")
+	return getIDFromHTTPRequest(r, "bucketID", ErrInvalidBucketID)
 }