Merge pull request #42 from CiscoM31/compute-bug-fix

Bug fixes for $compute parsing

Author: migoff875

.github/workflows/golangci-lint.yml

@@ -14,7 +14,7 @@ jobs:
         uses: golangci/golangci-lint-action@v2
         with:
           # Optional: version of golangci-lint to use in form of v1.2 or v1.2.3 or `latest` to use the latest version
-          version: v1.49
+          version: v1.53.3
 
           # Optional: working directory, useful for monorepos
           # working-directory: somedir

compute_parser.go

@@ -2,6 +2,8 @@ package godata
 
 import (
 	"context"
+	"errors"
+	"fmt"
 	"regexp"
 	"strings"
 )
@@ -10,18 +12,33 @@ import (
 // See https://docs.oasis-open.org/odata/odata/v4.01/os/part2-url-conventions/odata-v4.01-os-part2-url-conventions.html#sec_SystemQueryOptioncompute
 const computeAsSeparator = " as "
 
-// Dynamic property names are restricted to case-insensitive a-z
-var computeFieldRegex = regexp.MustCompile("^[a-zA-Z]+$")
+// Dynamic property names are restricted to case-insensitive a-z and the path separator /.
+var computeFieldRegex = regexp.MustCompile("^[a-zA-Z/]+$")
 
 type ComputeItem struct {
 	Tree  *ParseNode // The compute expression parsed as a tree.
 	Field string     // The name of the computed dynamic property.
 }
 
+// GlobalAllTokenParser is a Tokenizer which matches all tokens and ignores none. It differs from the
+// GlobalExpressionTokenizer which ignores whitespace tokens.
+var GlobalAllTokenParser *Tokenizer
+
+func init() {
+	t := NewExpressionParser().tokenizer
+	t.TokenMatchers = append(t.IgnoreMatchers, t.TokenMatchers...)
+	t.IgnoreMatchers = nil
+	GlobalAllTokenParser = t
+}
+
 func ParseComputeString(ctx context.Context, compute string) (*GoDataComputeQuery, error) {
-	items := strings.Split(compute, ",")
+	items, err := SplitComputeItems(compute)
+	if err != nil {
+		return nil, err
+	}
 
 	result := make([]*ComputeItem, 0)
+	fields := map[string]struct{}{}
 
 	for _, v := range items {
 		v = strings.TrimSpace(v)
@@ -45,7 +62,7 @@ func ParseComputeString(ctx context.Context, compute string) (*GoDataComputeQuer
 			case *GoDataError:
 				return nil, &GoDataError{
 					ResponseCode: e.ResponseCode,
-					Message:      "Invalid $compute query option",
+					Message:      fmt.Sprintf("Invalid $compute query option, %s", e.Message),
 					Cause:        e,
 				}
 			default:
@@ -62,6 +79,16 @@ func ParseComputeString(ctx context.Context, compute string) (*GoDataComputeQuer
 					Message:      "Invalid $compute query option",
 				}
 			}
+
+			if _, ok := fields[field]; ok {
+				return nil, &GoDataError{
+					ResponseCode: 400,
+					Message:      "Invalid $compute query option",
+				}
+			}
+
+			fields[field] = struct{}{}
+
 			result = append(result, &ComputeItem{
 				Tree:  tree.Tree,
 				Field: field,
@@ -71,3 +98,52 @@ func ParseComputeString(ctx context.Context, compute string) (*GoDataComputeQuer
 
 	return &GoDataComputeQuery{result, compute}, nil
 }
+
+// SplitComputeItems splits the input string based on the comma delimiter. It does so with awareness as to
+// which commas delimit $compute items and which ones are an inline part of the item, such as a separator
+// for function arguments.
+//
+// For example the input "someFunc(one,two) as three, 1 add 2 as four" results in the
+// output ["someFunc(one,two) as three", "1 add 2 as four"]
+func SplitComputeItems(in string) ([]string, error) {
+
+	var ret []string
+
+	tokens, err := GlobalAllTokenParser.Tokenize(context.Background(), in)
+	if err != nil {
+		return nil, err
+	}
+
+	item := strings.Builder{}
+	parenGauge := 0
+
+	for _, v := range tokens {
+		switch v.Type {
+		case ExpressionTokenOpenParen:
+			parenGauge++
+		case ExpressionTokenCloseParen:
+			if parenGauge == 0 {
+				return nil, errors.New("unmatched parentheses")
+			}
+			parenGauge--
+		case ExpressionTokenComma:
+			if parenGauge == 0 {
+				ret = append(ret, item.String())
+				item.Reset()
+				continue
+			}
+		}
+
+		item.WriteString(v.Value)
+	}
+
+	if parenGauge != 0 {
+		return nil, errors.New("unmatched parentheses")
+	}
+
+	if item.Len() > 0 {
+		ret = append(ret, item.String())
+	}
+
+	return ret, nil
+}

compute_parser_test.go

@@ -0,0 +1,71 @@
+package godata
+
+import (
+	"context"
+	"strings"
+	"testing"
+)
+
+func TestParseCompute(t *testing.T) {
+	err := DefineCustomFunctions([]CustomFunctionInput{
+		{Name: "zeroArgFunc", NumParams: []int{0}},
+		{Name: "oneArgFunc", NumParams: []int{1}},
+		{Name: "twoArgFunc", NumParams: []int{2}},
+	})
+
+	if err != nil {
+		t.Error(err)
+		t.FailNow()
+	}
+
+	type testcase struct {
+		computeStrings []string
+		shouldPass     bool
+	}
+
+	testCases := []testcase{
+		{[]string{"oldField as newField"}, true},
+		{[]string{"1 as newField"}, true},
+		{[]string{"one add 2 as newField"}, true},
+		{[]string{"one add two as extra/newField"}, true},
+		{[]string{"zeroArgFunc() as newField"}, true},
+		{[]string{"oneArgFunc(one) as newField"}, true},
+		{[]string{"twoArgFunc(one, two) as newField"}, true},
+		{[]string{"twoArgFunc(one, two) as newField", "tolower(three) as  newFieldTwo"}, true},
+
+		// negative cases
+		{[]string{"one add two as newField2"}, false},
+		{[]string{"one add two newField2"}, false},
+		{[]string{""}, false},
+		{[]string{"as"}, false},
+		{[]string{"as newField"}, false},
+		{[]string{"zeroArgFunc() as "}, false},
+	}
+
+	for i, v := range testCases {
+
+		var result *GoDataComputeQuery
+		result, err = ParseComputeString(context.Background(), strings.Join(v.computeStrings, ","))
+		if v.shouldPass {
+			if err != nil {
+				t.Errorf("testcase %d: %v", i, err)
+				t.FailNow()
+			}
+			if result == nil {
+				t.Errorf("testcase %d: nil result", i)
+				t.FailNow()
+			}
+
+			if len(result.ComputeItems) != len(v.computeStrings) {
+				t.Errorf("testcase %d: wrong number of result items", i)
+				t.FailNow()
+			}
+		} else {
+			if err == nil {
+				t.Errorf("testcase %d: expected error", i)
+				t.FailNow()
+			}
+		}
+
+	}
+}

url_parser_test.go

@@ -539,6 +539,15 @@ func TestUnescapeStringTokens(t *testing.T) {
 				},
 			},
 		},
+		{
+			url:      "/Product?$compute=Price mul Quantity as Extra/TotalPrice",
+			errRegex: nil,
+			expectedCompute: []ComputeItem{
+				{
+					Field: "Extra/TotalPrice",
+				},
+			},
+		},
 		{
 			url: "/Product?$compute=Price mul Quantity as TotalPrice,A add B as C",
 			expectedCompute: []ComputeItem{