Skip to content

Encoding detection fix - #788

Open
blagoySimandov wants to merge 3 commits into
gocolly:masterfrom
blagoySimandov:encodingDetectionFix
Open

Encoding detection fix#788
blagoySimandov wants to merge 3 commits into
gocolly:masterfrom
blagoySimandov:encodingDetectionFix

Conversation

@blagoySimandov

Copy link
Copy Markdown

Connected to issue #777 "HTML encoding is not autodetected properly". I removed the current gocolly encoding detection, which through tests showed to be unreliable when detecting Cyrillic encodings, and in place of it used the built-in function DetermineEncoding from the charset package.

Comment thread response.go
Comment on lines +89 to +90
_, nameOfEncoding, _ := charset.DetermineEncoding(r.Body, contentType) //name of charset/encoding
contentType = "text/plain; charset=" + nameOfEncoding

@WGH- WGH- Oct 16, 2023

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

charset.DetermineEncoding returns the detected encoding, so I think this entire function can be greatly simplified and largely replaced with rougly this:

enc, _, certain := charset.DetermineEncoding(r.Body, contentType)
if !certain && !detectCharset {
	return nil
}
var err error
r.Body, err = ioutil.ReadAll(enc.NewDecoder().Reader(bytes.NewReader(r.Body)))
return err

Comment thread response.go

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

This needs a unit test.

@WGH- WGH- left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Thanks! This needs a couple of changes, though (see above).

And please clean up your commit history (or I will do it myself before the merge)

@StantonMatt StantonMatt left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

I checked this on a current master merge. The branch still applies cleanly, and the existing suite passes there:

go test ./... -count=1
git diff --check origin/master

gofmt -l response.go still reports response.go, so the submitted diff needs formatting.

I also added a local-only regression for the behavior this PR is trying to fix. With DetectCharset, current master misdecodes HTML bodies that declare their encoding through a meta tag but have only Content-Type: text/html in the response header:

shift-jis meta:    expected the UTF-8 Japanese body text, but got mojibake bytes rendered as `\x22\xfa-\x7b\x8c\xea`
windows-1251 meta: expected the UTF-8 Cyrillic body text, but got mojibake bytes rendered as `\xcf\xf0\xe8\xe2\xe5\xf2`

The same test passes on this PR merged into current master, because charset.DetermineEncoding sees the HTML meta charset before decoding.

So the behavior change still looks useful to me. I think the remaining merge blockers are the ones already called out in review: format response.go, simplify around the charset.DetermineEncoding result, and commit a unit test for this no-header-charset/meta-charset case.

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants