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

Fixed #18119 -- Added a DomainNameValidator validator. #18037

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

nmenezes0
Copy link
Contributor

@nmenezes0 nmenezes0 commented Mar 31, 2024

Trac ticket number

ticket-18119

Branch description

Provide a concise overview of the issue or rationale behind the proposed changes.
This ticket adds a DomainNameValidator to validate International domain names. This validator takes a flag is_idna (defaults to True) - to determine whether or not we should use non-ASCII international domain names.

This builds on the existing PR with some changes: not using IP addresses (don't think they are part of a domain name), removing changes to docs as out of date (not sure what updates are needed here now?), changes to the tests and how they were written, reusing the regex in URLValidator.

The ticket in Trac suggests that the regex could be reused elsewhere in the codebase. I have reused it in the URLValidator, but I couldn't see how to use it in django/core/mail/message.py or django/utils/html.py.

Checklist

  • This PR targets the main branch.
  • The commit message is written in past tense, mentions the ticket number, and ends with a period.
  • I have checked the "Has patch" ticket flag in the Trac system.
  • I have added or updated relevant tests.
  • I have added or updated relevant docs, including release notes if applicable.
  • For UI changes, I have attached screenshots in both light and dark modes. - N/A

I wasn't sure what changes I should make to the documentation.

@sarahboyce
Copy link
Contributor

Thank you for this Nina ⭐

removing changes to docs as out of date (not sure what updates are needed here now?)

All new features need some docs and a release note, so this is roughly what I am expecting:

  • Add DomainNameValidator to docs/ref/validators.txt with .. versionadded:: 5.1
  • Add a reference to this change in the 5.1 release notes

@nmenezes0 nmenezes0 force-pushed the feature/18119-domain-name-validation branch from fc3bd96 to f77f5f3 Compare April 20, 2024 21:45
@nmenezes0
Copy link
Contributor Author

Thanks Sarah - docs now updated.

docs/ref/validators.txt Outdated Show resolved Hide resolved
docs/ref/validators.txt Outdated Show resolved Hide resolved
docs/ref/validators.txt Outdated Show resolved Hide resolved
docs/releases/5.1.txt Show resolved Hide resolved
@nmenezes0
Copy link
Contributor Author

Thanks @felixxm - docs now updated following comments.

Copy link
Contributor

@sarahboyce sarahboyce 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 @nmenezes0 ⭐ I have a few more comments.
Please also squash this to a single commit 👍

docs/ref/validators.txt Outdated Show resolved Hide resolved
docs/ref/validators.txt Outdated Show resolved Hide resolved
django/core/validators.py Outdated Show resolved Hide resolved
django/core/validators.py Outdated Show resolved Hide resolved
django/core/validators.py Outdated Show resolved Hide resolved
docs/ref/validators.txt Outdated Show resolved Hide resolved
@nmenezes0 nmenezes0 force-pushed the feature/18119-domain-name-validation branch 2 times, most recently from 151845f to dd9816e Compare May 11, 2024 18:04
@nmenezes0
Copy link
Contributor Author

Thanks @sarahboyce - I've made those changes and squashed to a single commit.

@nmenezes0
Copy link
Contributor Author

I'm not sure why some of the checks are failing. The error is "commit changed" - potentially due to me pushing the squashed commit?

Copy link
Contributor

@sarahboyce sarahboyce 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 updates @nmenezes0, we're really close ⭐

I'm not sure why some of the checks are failing. The error is "commit changed" - potentially due to me pushing the squashed commit?

Don't worry about this, sounds unrelated to your changes

The ticket in Trac suggests that the regex could be reused elsewhere in the codebase. I have reused it in the URLValidator, but I couldn't see how to use it in django/core/mail/message.py or django/utils/html.py.

Read through the original comment made by @claudep 12 years ago.

  • It's still valid that we could chose to use this validation in other areas of the codebase.
  • I don't think refactoring to include this is in scope of this ticket, however we should make sure it's defined in a good place.
  • I'm not sure which __init__ is being referred to in the comment and I don't get circular imports if I was to import DomainNameValidator to the mentioned files.

@claudep can you clarify where you think DomainNameValidator should be defined or whether it is correct to be in django/core/validators.py?
I think it is ok to be defined here but I might be missing something 👍

docs/ref/validators.txt Outdated Show resolved Hide resolved
docs/ref/validators.txt Outdated Show resolved Hide resolved
docs/ref/validators.txt Show resolved Hide resolved
@nmenezes0
Copy link
Contributor Author

Thanks @sarahboyce - have commited those docs changes - I assume I should squash into one commit again (will do that later).

Co-authored-by: Mariusz Felisiak <[email protected]>
Co-authored-by: Sarah Boyce <[email protected]>
Co-authored-by: Nina Menezes <[email protected]>
@nmenezes0 nmenezes0 force-pushed the feature/18119-domain-name-validation branch from 4a05b3f to b7d5494 Compare May 14, 2024 20:14
@claudep
Copy link
Member

claudep commented May 15, 2024

@claudep can you clarify where you think DomainNameValidator should be defined or whether it is correct to be in django/core/validators.py? I think it is ok to be defined here but I might be missing something 👍

I think the suggested location is fine.

name. Values longer than 255 characters are always considered invalid.

In addition to the optional arguments of its parent :class:`RegexValidator`
class, ``DomainNameValidator`` accepts an extra optional attribute:
Copy link
Member

Choose a reason for hiding this comment

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

schemes is not an accepted parameter. regex shouldn't accepted either, as it is rewritten in all cases in __init__.
BTW, you still have an extra new line just below.


This determines whether or not to accept internationalized domain name,
that is, domain names that contain non-ASCII characters. Default value
is true.
Copy link
Member

Choose a reason for hiding this comment

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

true => True (enclosed in double backticks)

(validate_domain_name, "python..org", ValidationError),
(validate_domain_name, "python-.org", ValidationError),
(validate_domain_name, "too-long-name." * 20 + "com", ValidationError),
(validate_domain_name, "stupid-name试", ValidationError),
Copy link
Member

Choose a reason for hiding this comment

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

I think we should also add lines with IP (v4/v6) addresses to demonstrate by tests that they are excluded by validation. And that should also be clarified in docs.

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