Skip to content

feat: ✨ Add option to unescape strings inside log string #48

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.

Sign up for GitHub

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

Jump to bottom
Merged
mohammed90 merged 3 commits into from
Dec 23, 2024
Merged

feat: ✨ Add option to unescape strings inside log string #48

mohammed90 merged 3 commits into from
Dec 23, 2024

Conversation

@adrienyhuel
Copy link
Contributor

@adrienyhuel adrienyhuel commented Dec 22, 2024

Unescaping string fields can be useful to have a readable console log

mohammed90
mohammed90 requested changes Dec 22, 2024
View reviewed changes
Copy link
Member

@mohammed90 mohammed90 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for the PR! It's a good change, simplifying log processing. Just a minor change to be more idiomatic, and it'll be good. I love the tests 🙂

formatencoder.go
Comment on lines 159 to 164
if !unescapeStrings {
return value, true
} else {
str, _ := jsonparser.ParseString(value)
return str, true
}
Copy link
Member

@mohammed90 mohammed90 Dec 22, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This can be shortened like this:

Suggested change
if !unescapeStrings {
return value, true
} else {
str, _ := jsonparser.ParseString(value)
return str, true
}
if !unescapeStrings {
return value, true
}
return jsonparser.ParseString(value), true

Copy link
Contributor Author

@adrienyhuel adrienyhuel Dec 22, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Change done
Though we can't return jsonparser.ParseString() result directly, as it is a tuple (str, err)

Copy link
Member

@mohammed90 mohammed90 Dec 22, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My bad 🤦🏻‍♂️ Thank you for catching it

@adrienyhuel
Copy link
Contributor Author

adrienyhuel commented Dec 22, 2024

@mohammed90
I was testing on my local caddy server, and I noticed that if we put special chars (like \t) in the template, it appears as a string in the log, and not like a tab.
Do you perhaps know where it comes from ?

@mohammed90
Copy link
Member

mohammed90 commented Dec 22, 2024

I'm sorry, forgot to mention it the first time. Can you lose stretcher/testify?

@adrienyhuel
Copy link
Contributor Author

adrienyhuel commented Dec 22, 2024

I'm sorry, forgot to mention it the first time. Can you lose stretcher/testify?

Lose stretcher/testify ?

@adrienyhuel
Copy link
Contributor Author

adrienyhuel commented Dec 22, 2024 •
edited
Loading

@mohammed90 I was testing on my local caddy server, and I noticed that if we put special chars (like \t) in the template, it appears as a string in the log, and not like a tab. Do you perhaps know where it comes from ?

@mohammed90
I got it !
When caddy read the Caddyfile, it escape strings
We can see when we run "caddy adapt" -> "\t" in Caddyfile becomes "\t" in JSON Config
I found a solution, unescaping template in the UnmarshalCaddyfile method.
Do you think I could/should include this change in this PR ?

@mohammed90
Copy link
Member

mohammed90 commented Dec 23, 2024

I'm sorry, forgot to mention it the first time. Can you lose stretcher/testify?

Lose stretcher/testify ?

I mean could you remove the dependency on the testify package?

@mohammed90 I was testing on my local caddy server, and I noticed that if we put special chars (like \t) in the template, it appears as a string in the log, and not like a tab. Do you perhaps know where it comes from ?

@mohammed90 I got it ! When caddy read the Caddyfile, it escape strings We can see when we run "caddy adapt" -> "\t" in Caddyfile becomes "\t" in JSON Config I found a solution, unescaping template in the UnmarshalCaddyfile method. Do you think I could/should include this change in this PR ?

Thanks for the find! I see this comes from the Caddyfile lexer. The lexer area isn't my strongest. @mholt, @francislavoie, I see the same unavoidable escape when doing respond "Hello\tworld" which is adapted to "Hello\\tworld" in the JSON. Is this expected? This effectively means escape sequences cannot be included in Caddyfile to be produced unescaped.

mohammed90
mohammed90 reviewed Dec 23, 2024
View reviewed changes
@francislavoie
Copy link
Member

francislavoie commented Dec 23, 2024

Just use a literal tab character in the Caddyfile 🤷 it wasn't designed to support escape sequences.

@adrienyhuel
Copy link
Contributor Author

adrienyhuel commented Dec 23, 2024

@mohammed90
I removed testify dependency

mohammed90
mohammed90 approved these changes Dec 23, 2024
View reviewed changes
Copy link
Member

@mohammed90 mohammed90 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! Thank you for the contribution 🎉

@mohammed90 mohammed90 merged commit 47f376e into caddyserver:master Dec 23, 2024
6 checks passed
@adrienyhuel adrienyhuel deleted the feat-add-unescape branch December 23, 2024 11:58
lemniskett pushed a commit to lemniskett/transform-encoder that referenced this pull request Mar 13, 2025
@adrienyhuel @lemniskett
feat: ✨ Add option to unescape strings inside log string (caddyserver#48
400198e 
)

* feat: ✨ Add option to unescape strings inside log string

* fix: 🎨 Minor changes

* fix: arrow_down: Remove testify dependency
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@mohammed90 mohammed90 mohammed90 approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@adrienyhuel @mohammed90 @francislavoie