search

labstack / gommon

Common packages for Go
MIT License
539 stars 101 forks source link

Escape strings with quotes #9

Closed asib closed 7 years ago

asib commented 8 years ago

The current implementation won't work if there are quotes in the string that's being logged.

strconv.Quote solves this issue by wrapping the supplied string in double quotes, escaping any existing quotes in the supplied string.

coveralls commented 8 years ago

Coverage Status

Coverage remained the same at 73.387% when pulling 15116a97d8f1eb482729dee8b85ee49caacdefca on asib:master into 431777a5117c8de4352a400dad1e2a55f484b189 on labstack:master.

vishr commented 8 years ago

@asib For performance reason, this should be handled outside the library. It adds another iteration to the string. Even standard log library doesn't have it.

asib commented 8 years ago

The standard library log package isn't trying to produce JSON output though.

As an example, logrus, if you set it to JSON formatting, will escape quotes for you (via a json.Marshal call).

If someone were not to realise that quotes aren't escaped in this library (which is the default for Echo), they would have a very difficult time if they were to attempt to parse their logs.

vishr commented 8 years ago

How are you sending json via Echo? Are you talking about logger middleware?

asib commented 8 years ago

E.g. c.Logger().Debugf("Content-Type: %q", "") where c is an echo.Context.

vishr commented 8 years ago

Can you use Debugj, it accepts map[string]interface{} and prints JSON? Perhaps we can change it to accept just interface{}.

asib commented 8 years ago

I think perhaps a better idea would be to have a function like SetQuote so that you can switch quoting on/off for all logs.

sam701 commented 7 years ago

@vishr I agree with @asib. The default behavior of c.Logger().Info(message) should be valid independent of the message's content. SetQuote might be an option but should be on by default.

vishr commented 7 years ago

@asib @sam701 thanks for your contribution 🎉