Skip to content

Commit

Permalink
linting validator variables
Browse files Browse the repository at this point in the history
per package validations
  • Loading branch information
ortz committed Jan 18, 2022
1 parent 48a31a8 commit e68ddb9
Show file tree
Hide file tree
Showing 8 changed files with 229 additions and 205 deletions.
5 changes: 2 additions & 3 deletions pkg/api/controller.go
Original file line number Diff line number Diff line change
Expand Up @@ -34,7 +34,6 @@ import (
"github.com/treeverse/lakefs/pkg/permissions"
"github.com/treeverse/lakefs/pkg/stats"
"github.com/treeverse/lakefs/pkg/upload"
"github.com/treeverse/lakefs/pkg/validator"
"github.com/treeverse/lakefs/pkg/version"
)

Expand Down Expand Up @@ -120,7 +119,7 @@ func (c *Controller) DeleteObjects(w http.ResponseWriter, r *http.Request, body
StatusCode: http.StatusForbidden,
Message: err.Error(),
})
case errors.Is(err, validator.ErrPathRequiredValue):
case errors.Is(err, catalog.ErrPathRequiredValue):
// issue #1706 - https://github.com/treeverse/lakeFS/issues/1706
// Spark trying to delete the path "main/", which we map to branch "main" with an empty path.
// Spark expects it to succeed (not deleting anything is a success), instead of returning an error.
Expand Down Expand Up @@ -1621,7 +1620,7 @@ func handleAPIError(w http.ResponseWriter, err error) bool {
errors.Is(err, permissions.ErrInvalidAction),
errors.Is(err, model.ErrValidationError),
errors.Is(err, graveler.ErrInvalidRef),
errors.Is(err, validator.ErrInvalidValue):
errors.Is(err, graveler.ErrInvalidValue):
writeError(w, http.StatusBadRequest, err)

case errors.Is(err, graveler.ErrNotUnique):
Expand Down
130 changes: 65 additions & 65 deletions pkg/catalog/catalog.go

Large diffs are not rendered by default.

5 changes: 5 additions & 0 deletions pkg/catalog/errors.go
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,11 @@ import (
)

var (
ErrInvalid = errors.New("validation error")
ErrInvalidType = fmt.Errorf("invalid type: %w", ErrInvalid)
ErrRequiredValue = fmt.Errorf("required value: %w", ErrInvalid)
ErrPathRequiredValue = fmt.Errorf("missing path: %w", ErrRequiredValue)
ErrInvalidValue = errors.New("invalid value")
ErrNotFound = db.ErrNotFound
ErrInvalidMetadataSrcFormat = errors.New("invalid metadata src format")
ErrExpired = errors.New("expired from storage")
Expand Down
29 changes: 29 additions & 0 deletions pkg/catalog/validate.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,29 @@
package catalog

import (
"fmt"

"github.com/treeverse/lakefs/pkg/validator"
)

const (
MaxPathLength = 1024
)

func ValidatePath(v interface{}) error {
s, ok := v.(Path)
if !ok {
panic(ErrInvalidType)
}

l := len(s)
if l == 0 {
return ErrPathRequiredValue
}
if l > MaxPathLength {
return fmt.Errorf("%w: %d is above maximum length (%d)", ErrInvalidValue, l, MaxPathLength)
}
return nil
}

var ValidatePathOptional = validator.MakeValidateOptional(ValidatePath)
6 changes: 6 additions & 0 deletions pkg/graveler/errors.go
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,12 @@ var (
ErrNoMergeBase = errors.New("no merge base")
ErrInvalidRef = fmt.Errorf("ref: %w", ErrInvalidValue)
ErrInvalidCommitID = fmt.Errorf("commit id: %w", ErrInvalidValue)
ErrInvalidBranchID = fmt.Errorf("branch id: %w", ErrInvalidValue)
ErrInvalidTagID = fmt.Errorf("tag id: %w", ErrInvalidValue)
ErrInvalid = errors.New("validation error")
ErrInvalidType = fmt.Errorf("invalid type: %w", ErrInvalid)
ErrInvalidRepositoryID = fmt.Errorf("repository id: %w", ErrInvalidValue)
ErrRequiredValue = fmt.Errorf("required value: %w", ErrInvalid)
ErrCommitNotFound = fmt.Errorf("commit %w", ErrNotFound)
ErrCreateBranchNoCommit = fmt.Errorf("can't create a branch without commit")
ErrRepositoryNotFound = fmt.Errorf("repository %w", ErrNotFound)
Expand Down
118 changes: 118 additions & 0 deletions pkg/graveler/validate.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,118 @@
package graveler

import (
"strings"

"github.com/treeverse/lakefs/pkg/ident"
"github.com/treeverse/lakefs/pkg/validator"
)

func ValidateStorageNamespace(v interface{}) error {
s, ok := v.(StorageNamespace)
if !ok {
panic(ErrInvalidType)
}

if len(s) == 0 {
return ErrRequiredValue
}
return nil
}

func ValidateRef(v interface{}) error {
s, ok := v.(Ref)
if !ok {
panic(ErrInvalidType)
}
if len(s) == 0 {
return ErrRequiredValue
}
if !validator.ReValidRef.MatchString(s.String()) {
return ErrInvalidRef
}
return nil
}

func ValidateBranchID(v interface{}) error {
s, ok := v.(BranchID)
if !ok {
panic(ErrInvalidType)
}
if len(s) == 0 {
return ErrRequiredValue
}
if !validator.ReValidBranchID.MatchString(s.String()) {
return ErrInvalidBranchID
}
return nil
}

func ValidateTagID(v interface{}) error {
s, ok := v.(TagID)
if !ok {
panic(ErrInvalidType)
}

// https://git-scm.com/docs/git-check-ref-format
if len(s) == 0 {
return ErrRequiredValue
}

tagID := s.String()
if tagID == "@" {
return ErrInvalidTagID
}
if strings.HasSuffix(tagID, ".") || strings.HasSuffix(tagID, ".lock") || strings.HasSuffix(tagID, "/") {
return ErrInvalidTagID
}
if strings.Contains(tagID, "..") || strings.Contains(tagID, "//") || strings.Contains(tagID, "@{") {
return ErrInvalidTagID
}
// Unlike git, we do allow '~'. That supports migration from our previous ref format where commits started with a tilde.
if strings.ContainsAny(tagID, "^:?*[\\") {
return ErrInvalidTagID
}
for _, r := range tagID {
if isControlCodeOrSpace(r) {
return ErrInvalidTagID
}
}
return nil
}

func isControlCodeOrSpace(r rune) bool {
const space = 0x20
return r <= space
}

func ValidateCommitID(v interface{}) error {
s, ok := v.(CommitID)
if !ok {
panic(ErrInvalidType)
}

if len(s) == 0 {
return ErrRequiredValue
}
if !ident.IsContentAddress(s.String()) {
return ErrInvalidCommitID
}
return nil
}

func ValidateRepositoryID(v interface{}) error {
s, ok := v.(RepositoryID)
if !ok {
panic(ErrInvalidType)
}

if len(s) == 0 {
return ErrRequiredValue
}
if !validator.ReValidRepositoryID.MatchString(s.String()) {
return ErrInvalidRepositoryID
}
return nil
}

var ValidateTagIDOptional = validator.MakeValidateOptional(ValidateTagID)
10 changes: 4 additions & 6 deletions pkg/validator/validate_test.go → pkg/graveler/validate_test.go
Original file line number Diff line number Diff line change
@@ -1,16 +1,14 @@
package validator
package graveler

import (
"errors"
"testing"

"github.com/treeverse/lakefs/pkg/graveler"
)

func TestValidateTagID(t *testing.T) {
tests := []struct {
name string
tag graveler.TagID
tag TagID
wantErr error
}{
{name: "empty", tag: "", wantErr: ErrRequiredValue},
Expand All @@ -35,7 +33,7 @@ func TestValidateTagID(t *testing.T) {
t.Run(tt.name, func(t *testing.T) {
err := ValidateTagID(tt.tag)
if !errors.Is(err, tt.wantErr) {
t.Errorf("ValidateTagID() error = %v, wantErr %v", err, tt.wantErr)
t.Errorf("ValidateTagID() error = %v, wantErr %v (%v)", err, tt.wantErr, tt.name)
}
})
}
Expand All @@ -53,7 +51,7 @@ func TestValidateTagID_Type(t *testing.T) {
func TestValidateBranchID(t *testing.T) {
tests := []struct {
name string
branchID graveler.BranchID
branchID BranchID
wantErr error
}{
{name: "alpha dash", branchID: "valid-branch", wantErr: nil},
Expand Down
131 changes: 0 additions & 131 deletions pkg/validator/validate.go
Original file line number Diff line number Diff line change
Expand Up @@ -3,15 +3,7 @@ package validator
import (
"errors"
"fmt"
"reflect"
"regexp"
"strings"

"github.com/treeverse/lakefs/pkg/ident"
)

const (
MaxPathLength = 1024
)

var (
Expand Down Expand Up @@ -64,110 +56,6 @@ func MakeValidateOptional(fn ValidateFunc) ValidateFunc {
}
}

func ValidateStorageNamespace(v interface{}) error {
storageNamespace := reflect.ValueOf(v).String()
if reflect.TypeOf(v).String() != "graveler.StorageNamespace" {
panic(ErrInvalidType)
}
if len(storageNamespace) == 0 {
return ErrRequiredValue
}
return nil
}

func ValidateRef(v interface{}) error {
ref := reflect.ValueOf(v).String()
if reflect.TypeOf(v).String() != "graveler.Ref" {
panic(ErrInvalidType)
}
if len(ref) == 0 {
return ErrRequiredValue
}
if !ReValidRef.MatchString(ref) {
return ErrInvalidValue
}
return nil
}

func ValidateBranchID(v interface{}) error {
branchId := reflect.ValueOf(v).String()
if reflect.TypeOf(v).String() != "graveler.BranchID" {
panic(ErrInvalidType)
}

if len(branchId) == 0 {
return ErrRequiredValue
}
if !ReValidBranchID.MatchString(branchId) {
return ErrInvalidValue
}
return nil
}

func ValidateTagID(v interface{}) error {
tagId := reflect.ValueOf(v).String()
if reflect.TypeOf(v).String() != "graveler.TagID" {
panic(ErrInvalidType)
}

// https://git-scm.com/docs/git-check-ref-format
if len(tagId) == 0 {
return ErrRequiredValue
}
if tagId == "@" {
return ErrInvalidValue
}
if strings.HasSuffix(tagId, ".") || strings.HasSuffix(tagId, ".lock") || strings.HasSuffix(tagId, "/") {
return ErrInvalidValue
}
if strings.Contains(tagId, "..") || strings.Contains(tagId, "//") || strings.Contains(tagId, "@{") {
return ErrInvalidValue
}
// Unlike git, we do allow '~'. That supports migration from our previous ref format where commits started with a tilde.
if strings.ContainsAny(tagId, "^:?*[\\") {
return ErrInvalidValue
}
for _, r := range tagId {
if isControlCodeOrSpace(r) {
return ErrInvalidValue
}
}
return nil
}

func isControlCodeOrSpace(r rune) bool {
const space = 0x20
return r <= space
}

func ValidateCommitID(v interface{}) error {
commitId := reflect.ValueOf(v).String()
if reflect.TypeOf(v).String() != "graveler.CommitID" {
panic(ErrInvalidType)
}
if len(commitId) == 0 {
return ErrRequiredValue
}
if !ident.IsContentAddress(commitId) {
return ErrInvalidValue
}
return nil
}

func ValidateRepositoryID(v interface{}) error {
repositoryId := reflect.ValueOf(v).String()
if reflect.TypeOf(v).String() != "graveler.RepositoryID" {
panic(ErrInvalidType)
}
if len(repositoryId) == 0 {
return ErrRequiredValue
}
if !ReValidRepositoryID.MatchString(repositoryId) {
return ErrInvalidValue
}
return nil
}

func ValidateRequiredString(v interface{}) error {
s, ok := v.(string)
if !ok {
Expand All @@ -189,22 +77,3 @@ func ValidateNonNegativeInt(v interface{}) error {
}
return nil
}

func ValidatePath(v interface{}) error {
path := reflect.ValueOf(v).String()
if reflect.TypeOf(v).String() != "catalog.Path" {
panic(ErrInvalidType)
}

l := len(path)
if l == 0 {
return ErrPathRequiredValue
}
if l > MaxPathLength {
return fmt.Errorf("%w: %d is above maximum length (%d)", ErrInvalidValue, l, MaxPathLength)
}
return nil
}

var ValidateTagIDOptional = MakeValidateOptional(ValidateTagID)
var ValidatePathOptional = MakeValidateOptional(ValidatePath)

0 comments on commit e68ddb9

Please sign in to comment.