ua-parser / uap-go

Go implementation of ua-parser
Other
357 stars 106 forks source link

Stop using MustCompile, and instead return an error if any regex fails to compile #67

Open veqryn opened 4 years ago

veqryn commented 4 years ago

The use of regex.MustCompile (see: https://github.com/ua-parser/uap-go/blob/daf92ba38329329419decc8e322bcd9480964294/uaparser/parser.go#L354-L360 ) means that if any regex fails, there is a panic.

We pull down the master regex url daily, so that our regexes are always up to date, and load it right into our servers. The panic cause all our servers to crash last night, until we pinned the master regex url to a working version.

NewFromBytes already has the option to return an error, so why not return the error there? https://github.com/ua-parser/uap-go/blob/daf92ba38329329419decc8e322bcd9480964294/uaparser/parser.go#L209-L219

sylvain101010 commented 4 years ago

I agree! @elsigh would you consider a merge request changing the panics to correct error handling (also in NewFromSaved)?

(it's a breaking change)

yehaotong commented 3 years ago

regex.MustCompile 的使用(见:

https://github.com/ua-parser/uap-go/blob/daf92ba38329329419decc8e322bcd9480964294/uaparser/parser.go#L354-L360

) 意味着如果任何正则表达式失败,就会出现恐慌。 我们每天下拉主正则表达式网址,以便我们的正则表达式始终是最新的,并将其直接加载到我们的服务器中。恐慌导致我们所有的服务器昨晚崩溃,直到我们将主正则表达式 url 固定到一个工作版本。

NewFromBytes 已经可以选择返回错误,那么为什么不在那里返回错误呢?

https://github.com/ua-parser/uap-go/blob/daf92ba38329329419decc8e322bcd9480964294/uaparser/parser.go#L209-L219

I want to ask you if this error will cause the program to hang up directly without any error log. My program also uses client: = parser.parse, and hang up directly without any error log during operation, which makes me wonder where there is a bug

elsigh commented 3 years ago

I'm happy to accept a breaking change, but one issue really is that I'm not using this library in prod atm so I'd rather find someone else who can manage/own such a change.. Interested @skerkour ?

sylvain101010 commented 3 years ago

Hello, Unfortunately I absolutely don't have the bandwidth available to own the change.

Kind regards