Skip to content

feat: add Email Length validation#430

Merged
thinkingserious merged 7 commits intosendgrid:mainfrom
itsksaurabh:main
Apr 21, 2021
Merged

feat: add Email Length validation#430
thinkingserious merged 7 commits intosendgrid:mainfrom
itsksaurabh:main

Conversation

@itsksaurabh
Copy link
Copy Markdown
Contributor

@itsksaurabh itsksaurabh commented Apr 16, 2021

Fixes #429

Add Email Length validation according to best practices and RFCs

Checklist

  • I acknowledge that all my contributions will be made under the project's license
  • I have made a material change to the repo (functionality, testing, spelling, grammar)
  • I have read the Contribution Guidelines and my PR follows them
  • I have titled the PR appropriately
  • I have updated my branch with the main branch
  • I have added tests that prove my fix is effective or that my feature works
  • I have added the necessary documentation about the functionality in the appropriate .md file
  • I have added inline documentation to the code I modified

If you have questions, please file a support ticket, or create a GitHub Issue in this repository.

@thinkingserious thinkingserious added the status: code review request requesting a community code review or review from Twilio label Apr 16, 2021
thinkingserious
thinkingserious reviewed Apr 16, 2021
View reviewed changes
Copy link
Copy Markdown
Contributor

@thinkingserious thinkingserious 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, we greatly appreciate it!

What prompted this PR, did you run into this limitation in practice?

Comment thread helpers/mail/mail_v3_test.go
}
}

func TestParseInvalidEmailLength(t *testing.T) {
Copy link
Copy Markdown
Contributor

@thinkingserious thinkingserious Apr 16, 2021

Choose a reason for hiding this comment

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

Please provide the happy path test, thanks!

Copy link
Copy Markdown
Contributor Author

@itsksaurabh itsksaurabh Apr 18, 2021

Choose a reason for hiding this comment

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

sure.

Copy link
Copy Markdown
Contributor Author

@itsksaurabh itsksaurabh Apr 18, 2021

Choose a reason for hiding this comment

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

@thinkingserious I have added new tests for the happy path :)

Copy link
Copy Markdown
Contributor

@thinkingserious thinkingserious Apr 21, 2021

Choose a reason for hiding this comment

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

Thank you!

@thinkingserious thinkingserious changed the title Add Email Length validation feat: add Email Length validation Apr 16, 2021
@thinkingserious thinkingserious added status: waiting for feedback waiting for feedback from the submitter type: community enhancement feature request not on Twilio's roadmap and removed status: code review request requesting a community code review or review from Twilio labels Apr 16, 2021
@itsksaurabh
Copy link
Copy Markdown
Contributor Author

itsksaurabh commented Apr 18, 2021 •
edited
Loading

Thank you for the PR, we greatly appreciate it!

What prompted this PR, did you run into this limitation in practice?

Thank you so much @thinkingserious for looking at this PR!

Yes, We have this limitation in our product as it's under best practices. We were using SMTP and I am happy to say that we recently integrated Sendgrid along with it as it has great features! But we noticed that this SendGrid client doesn’t have these checks.

The technical limit for an email address is 64 characters before the @ sign and 255 after, allowing for a total length of 320 characters (including the @ sign).

Reference: https://tools.ietf.org/html/rfc3696#section-3

In addition to restrictions on syntax, there is a length limit on
   email addresses.  That limit is a maximum of 64 characters (octets)
   in the "local part" (before the "@") and a maximum of 255 characters
   (octets) in the domain part (after the "@") for a total length of 320
   characters.  Systems that handle email should be prepared to process
   addresses which are that long, even though they are rarely
   encountered

Also, According to Sendgrid's best practices described in this blog https://sendgrid.com/blog/email-marketing-length-best-practices", it also states the same thing.

We have added these checks already in our codebase as its standard and have been followed by the industry. Also stated in the RFCs.

Please let me know if you have any doubts. Thank you so much! :)

thinkingserious
thinkingserious reviewed Apr 19, 2021
View reviewed changes
Copy link
Copy Markdown
Contributor

@thinkingserious thinkingserious left a comment

Choose a reason for hiding this comment

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

This LGTM, just have one more piece of feedback regarding the error message. Thank you!

Comment thread helpers/mail/mail_v3.go Outdated
local, domain := parts[0], parts[1]

if len(domain) > maxEmailDomainLength || len(local) > maxEmailLocalLength {
return nil, errors.New("invalid email length")
Copy link
Copy Markdown
Contributor

@thinkingserious thinkingserious Apr 19, 2021

Choose a reason for hiding this comment

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

I think we should specify a bit more detail about the violation. Something like: "Invalid email length, you have used X out of Y characters )

Copy link
Copy Markdown
Contributor Author

@itsksaurabh itsksaurabh Apr 20, 2021

Choose a reason for hiding this comment

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

I agree. Error message should have more information.

Copy link
Copy Markdown
Contributor Author

@itsksaurabh itsksaurabh Apr 20, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

@thinkingserious I have improved the error messages. Please take a look :)

thinkingserious
thinkingserious approved these changes Apr 21, 2021
View reviewed changes
Copy link
Copy Markdown
Contributor

@thinkingserious thinkingserious left a comment

Choose a reason for hiding this comment

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

🎆

Comment thread helpers/mail/mail_v3_test.go
}
}

func TestParseInvalidEmailLength(t *testing.T) {
Copy link
Copy Markdown
Contributor

@thinkingserious thinkingserious Apr 21, 2021

Choose a reason for hiding this comment

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

Thank you!

@thinkingserious thinkingserious merged commit 9b780fd into sendgrid:main Apr 21, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@thinkingserious thinkingserious thinkingserious approved these changes

Assignees

No one assigned

Labels

status: waiting for feedback waiting for feedback from the submitter type: community enhancement feature request not on Twilio's roadmap

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Add Email length validation

3 participants

@itsksaurabh @thinkingserious @twilio-dx