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

replace ToASCII with mime base64 #197

Merged
merged 3 commits into from Jul 20, 2021
Merged

Conversation

Alexfilus
Copy link
Contributor

it fixes non ascii attach names such as cyrillic

@jhillyerd
Copy link
Owner

I fixed the static check failure in #198 -- that should go away if you rebase.

We'll need to update the golden files, please run the unit tests, inspect the output and run go test -update if it looks correct to you.

@jhillyerd jhillyerd self-requested a review June 26, 2021 19:57
@Alexfilus Alexfilus force-pushed the master branch 2 times, most recently from 81e266c to d9fb44a Compare June 27, 2021 16:45
it fixes non ascii attach names such as cyrillic
@Alexfilus
Copy link
Contributor Author

I hope how it's right order of commits

@jhillyerd
Copy link
Owner

Looks like it picked up my PR, but no changes to the golden files:

--- FAIL: TestEncodePartQuotedHeaders (0.00s)
    encode_test.go:83: Test output did not match testdata/encode/part-quoted-headers.golden
        To update golden file, run: go test -update
    encode_test.go:83: diff -want +got:
        |-Content-Disposition: attachment; filename="arvizturo \"x\"
        |- tukorfurogep.zip"; modification-date="01 Feb 03 04:05 GMT"
        |+Content-Disposition: attachment;
        |+ filename="=?utf-8?b?w6FydsOtenTFsXLFkSAieCIgdMO8a8O2cmbDunLDs2fDqXAuemlw?=";
        |+ modification-date="01 Feb 03 04:05 GMT"
        | Content-Id: <mycontentid>
        | Content-Transfer-Encoding: base64
        | Content-Type: application/zip; boundary=enmime-abcdefg0123456789;
        |- charset=binary; name="arvizturo \"x\" tukorfurogep.zip";
        |+ charset=binary;
        |+ name="=?utf-8?b?w6FydsOtenTFsXLFkSAieCIgdMO8a8O2cmbDunLDs2fDqXAuemlw?=";
        |  param1=myparameter1; param2=myparameter2
        | 
        | WklQWklQWklQ
        | 
        
--- FAIL: TestEncodePartQuotedPrintableHeaders (0.00s)
    encode_test.go:104: Test output did not match testdata/encode/part-quoted-printable-headers.golden
        To update golden file, run: go test -update
    encode_test.go:104: diff -want +got:
        |-Content-Disposition: attachment; filename="arvizturo \"x\"
        |- tukorfurogep.zip"; modification-date="01 Feb 03 04:05 GMT"
        |+Content-Disposition: attachment;
        |+ filename="=?utf-8?b?w6FydsOtenTFsXLFkSAieCIgdMO8a8O2cmbDunLDs2fDqXAuemlw?=";
        |+ modification-date="01 Feb 03 04:05 GMT"
        | Content-Id: <mycontentid>
        | Content-Transfer-Encoding: base64
        | Content-Type: application/zip; boundary=enmime-abcdefg0123456789;
        |- charset=binary; name="arvizturo \"x\" tukorfurogep.zip";
        |+ charset=binary;
        |+ name="=?utf-8?b?w6FydsOtenTFsXLFkSAieCIgdMO8a8O2cmbDunLDs2fDqXAuemlw?=";
        |  param1=myparameter1; param2=myparameter2
        | X-Qp-Header: =?utf-8?q?Just_enough_to_need_qp_=E2=98=86?=
        | 
        | WklQWklQWklQ
        | 

@Alexfilus
Copy link
Contributor Author

I see, those tests checks file names got from ToAscii function witch replace some symbols to similar, but it is not keep original file names.
mime.BEncoding.Encode allows to keep original file names, it is especially important in case of cyrillic file names, witch was looked like _________.pdf

Maybe I should replace golden files?

@jhillyerd
Copy link
Owner

Yes, we should update the golden files if the new output looks correct.

Something that I didn't realize when initially reviewing this PR, is that BEncode will always to base64. That makes it difficult to read an ASCII filename when looking at the raw text. I don't think we should do this.

Instead, please use selectTransferEncoding in encode.go to pick the encoder. That will allow us to pass through pure ASCII unchanged, use q-encoding for filenames with a few special characters, or b-encoding for ones with a majority.

@jhillyerd
Copy link
Owner

Do you plan to keep working on this? Otherwise I'll take a crack at it.

@Alexfilus
Copy link
Contributor Author

I'm sorry for delay. I made changes in golden files and add switch case for file name encodings.

Copy link
Owner

@jhillyerd jhillyerd left a comment

Choose a reason for hiding this comment

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

Looks good, thank you!

@jhillyerd jhillyerd merged commit 0e47cdb into jhillyerd:master Jul 20, 2021
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.

None yet

2 participants