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

fix(headers): avoid double spaces in the headers #8

Merged
merged 2 commits into from
Mar 27, 2024

Conversation

yacir
Copy link

@yacir yacir commented Mar 27, 2024

Copy link
Member

@paolobarbolini paolobarbolini 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 fix.
Could you add a regression test?

@yacir
Copy link
Author

yacir commented Mar 27, 2024

Thanks for the quick answer. Was thinking to add them later to the letter crate here https://github.com/lettre/lettre/blob/master/src/message/header/mod.rs but you're right i ll add it here

@yacir
Copy link
Author

yacir commented Mar 27, 2024

@paolobarbolini test added. Let me know if it's okay for you
thanks

@codecov-commenter
Copy link

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 98.71%. Comparing base (7c3ca39) to head (d212225).

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files
@@            Coverage Diff             @@
##             main       #8      +/-   ##
==========================================
+ Coverage   98.69%   98.71%   +0.01%     
==========================================
  Files          10       10              
  Lines         537      545       +8     
==========================================
+ Hits          530      538       +8     
  Misses          7        7              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@paolobarbolini paolobarbolini merged commit dbb1358 into lettre:main Mar 27, 2024
7 checks passed
@paolobarbolini
Copy link
Member

Turns out this doesn't work as I'd have originally expected.

new_line_and_space is just a silly micro-optimization that looking back doesn't make sense and I'm thinking of removing. Anyway the way it should work is the same as:

diff --git a/src/headers/writer.rs b/src/headers/writer.rs
index a36e6ab..0e024c5 100644
--- a/src/headers/writer.rs
+++ b/src/headers/writer.rs
@@ -55,11 +55,8 @@ impl<'a> EmailWriter<'a> {
 
     /// Equivalent to calling `new_line()` and `space()` consecutively.
     pub(crate) fn new_line_and_space(&mut self) -> fmt::Result {
-        self.spaces = 0;
-        self.writer.write_str("\r\n ")?;
-        self.line_len = 1;
-        self.optional_breakpoint = false;
-        self.can_go_to_new_line_now = false;
+        self.new_line()?;
+        self.space();
 
         Ok(())
     }

Doing so would break your test, so I'd have to also do:

diff --git a/src/headers/writer.rs b/src/headers/writer.rs
index a36e6ab..a418231 100644
--- a/src/headers/writer.rs
+++ b/src/headers/writer.rs
@@ -45,6 +45,7 @@ impl<'a> EmailWriter<'a> {
 
     /// Go to a new line and reset the `line_len` to `0`.
     pub fn new_line(&mut self) -> fmt::Result {
+        self.spaces = 0;
         self.writer.write_str("\r\n")?;
         self.line_len = 0;
         self.optional_breakpoint = false;

Which breaks many other tests.

@paolobarbolini
Copy link
Member

While it produces an ugly output this should now be fixed by c128b46

@yacir
Copy link
Author

yacir commented Mar 27, 2024

it's much better now, i ll give a try with my dataset to check if it fixes all the previous failing cases

@yacir
Copy link
Author

yacir commented Mar 28, 2024

@paolobarbolini i tested with +3400 real use case.

https://github.com/lettre/email-encoding/releases/tag/v0.2.1 14104c3 or #8
 456 encoding errors 6 remaining encoding errors 

It's way better, but we still have some corner cases

@paolobarbolini
Copy link
Member

Nice. I'm thinking of making a new release and handle any other report separately

@yacir
Copy link
Author

yacir commented Mar 28, 2024

I also think it's the best compromise since it fixes ~99% of the errors

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

Successfully merging this pull request may close these issues.

Invalid encoding for subject
3 participants