Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

why is panic used instead of returning errors? #92

Closed
vzalevsky opened this issue Oct 23, 2023 · 10 comments
Closed

why is panic used instead of returning errors? #92

vzalevsky opened this issue Oct 23, 2023 · 10 comments

Comments

@vzalevsky
Copy link

In build.go in the func (b *builder) processFunctionNode (root *function Node) (query, error) method in the cases: starts-with,ends-with,contains,matches,substring,sum, callbacks are recorded, which then cause panic when called. Why was this done as a panic and is refactoring planned to return errors instead of panic? In our code, we build xpath in advance, and then use it. It is very unpleasant when the service crashes due to errors in callbacks that are called inside xpath. At the moment we are temporarily using recover()

on the example of the "matches" case:

case "matches":
//matches(string , pattern)
if len(root.Args) != 2 {
return nil, errors.New("xpath: matches function must have two parameters")
}
var (
arg1, arg2 query
err error
)
if arg1, err = b.processNode(root.Args[0]); err != nil {
return nil, err
}
if arg2, err = b.processNode(root.Args[1]); err != nil {
return nil, err
}
qyOutput = &functionQuery{Input: b.firstInput, Func: matchesFunc(arg1, arg2)}

func matchesFunc(arg1, arg2 query) func(query, iterator) interface{} {
return func(q query, t iterator) interface{} {
var s string
switch typ := functionArgs(arg1).Evaluate(t).(type) {
case string:
s = typ
case query:
node := typ.Select(t)
if node == nil {
return ""
}
s = node.Value()
}
var pattern string
var ok bool
if pattern, ok = functionArgs(arg2).Evaluate(t).(string); !ok {
panic(errors.New("matches() function second argument type must be string"))
}
re, err := getRegexp(pattern)
if err != nil {
panic(fmt.Errorf("matches() function second argument is not a valid regexp pattern, err: %s", err.Error()))
}
return re.MatchString(s)
}
}

@zhengchun
Copy link
Contributor

Are you using an invalid XPath function or is your XPath syntax wrong? Should be checking XPath expression is correct before use it.

@zhengchun
Copy link
Contributor

You can use Compile() to check your input express whether is correct before use.

@vzalevsky
Copy link
Author

vzalevsky commented Oct 24, 2023

in xpath we use the string " //DataContent//p[position()>last()-3][matches(., '^[\u0621-\u064AA-Za-z\-]{2,20}/\w{2,4}')] " , it calmly passes the Compile() method. After we use xmlquery.QuerySelectorAll() it calls the saved callback matchesFunc(), which in the line re, err := get Regexp(pattern) returns err and causes panic. When calling Compile(), the regex located in xpath is not checked

@zhengchun
Copy link
Contributor

Hello, are you testing your regex expr whether correct by regexp.Compile()? If your expr is correct, you should testing your passed argument to regex expr whether causing panic.

@vzalevsky
Copy link
Author

Hello, we do not check the regular expression ourselves, otherwise we would have to parse xpath.expr ourselves, for this we use the xpath package. xpath.expr is fully processed in xpath, xpath itself gets a regular expression from it and then checks it only when calling a callback. Why is it that when calling Compile, only the correctness of the xpath path is checked, without checking the regular expression lying inside?

@zhengchun
Copy link
Contributor

Maybe I known what your what. You cannot promise XPath expression is correct every time, especially if include a regular expression. If it is wrong, it will cause the entire application to crash, right? You want to add some rules to avoid this accident, like add check for the regular expression in Compiler(), right?

@vzalevsky
Copy link
Author

Yes, that's what I meant)

@vzalevsky
Copy link
Author

It is possible, when calling Compile(), to check in the processFunctionNode() function for cases that may cause panic when calling back, such as: starts-with, ends-with, contains, matches, substring, sum

@zhengchun
Copy link
Contributor

I guess only one matches() function will occurred this error, it will throw panic error in XPath executing a regular query. starts-with(),ends-with() will return an error on the Compiler(), not in executing.

@vzalevsky
Copy link
Author

when can it appear in the release?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants