Repository: samuelcolvin/pydantic
Pull request #2447 (2021-03-02 00:15:09)

added `KafkaDsn` to `networks.py` and add default ports for `HttpUrl` 
❤️ pydantic

## Change Summary

- Added `KafkaDsn` class e.g. already existing `RedisDsn`  :fire:
- Added `default_parts` member and `apply_default_parts` method to the `AnyUrl` class

## Related issue number

## Checklist

* [x] Unit tests for the changes exist
* [x] Tests pass on CI and coverage remains at 100%
* [x] Documentation reflects the changes where applicable
* [x] `changes/<pull request or issue id>-<github username>.md` file added describing change
  (see [changes/README.md](https://github.com/samuelcolvin/pydantic/blob/master/changes/README.md) for details)


codecov[bot]: # [Codecov](https://codecov.io/gh/samuelcolvin/pydantic/pull/2447?src=pr&el=h1) Report
> Merging [#2447](https://codecov.io/gh/samuelcolvin/pydantic/pull/2447?src=pr&el=desc) (2d5f732) into [master](https://codecov.io/gh/samuelcolvin/pydantic/commit/d6194570665cfa7989068035b66dfbbd872c63e1?el=desc) (d619457) will **decrease** coverage by `1.82%`.
> The diff coverage is `100.00%`.


```diff
@@             Coverage Diff             @@
##            master    #2447      +/-   ##
===========================================
- Coverage   100.00%   98.17%   -1.83%     
===========================================
  Files           25       23       -2     
  Lines         5084     4554     -530     
  Branches      1042      923     -119     
===========================================
- Hits          5084     4471     -613     
- Misses           0       64      +64     
- Partials         0       19      +19     
```


| [Impacted Files](https://codecov.io/gh/samuelcolvin/pydantic/pull/2447?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [pydantic/networks.py](https://codecov.io/gh/samuelcolvin/pydantic/pull/2447/diff?src=pr&el=tree#diff-cHlkYW50aWMvbmV0d29ya3MucHk=) | `98.58% <100.00%> (-1.42%)` | :arrow_down: |
| [pydantic/typing.py](https://codecov.io/gh/samuelcolvin/pydantic/pull/2447/diff?src=pr&el=tree#diff-cHlkYW50aWMvdHlwaW5nLnB5) | `74.21% <0.00%> (-25.79%)` | :arrow_down: |
| [pydantic/annotated\_types.py](https://codecov.io/gh/samuelcolvin/pydantic/pull/2447/diff?src=pr&el=tree#diff-cHlkYW50aWMvYW5ub3RhdGVkX3R5cGVzLnB5) | `83.33% <0.00%> (-16.67%)` | :arrow_down: |
| [pydantic/version.py](https://codecov.io/gh/samuelcolvin/pydantic/pull/2447/diff?src=pr&el=tree#diff-cHlkYW50aWMvdmVyc2lvbi5weQ==) | `88.23% <0.00%> (-11.77%)` | :arrow_down: |
| [pydantic/decorator.py](https://codecov.io/gh/samuelcolvin/pydantic/pull/2447/diff?src=pr&el=tree#diff-cHlkYW50aWMvZGVjb3JhdG9yLnB5) | `88.43% <0.00%> (-11.57%)` | :arrow_down: |
| [pydantic/env\_settings.py](https://codecov.io/gh/samuelcolvin/pydantic/pull/2447/diff?src=pr&el=tree#diff-cHlkYW50aWMvZW52X3NldHRpbmdzLnB5) | `94.44% <0.00%> (-5.56%)` | :arrow_down: |
| [pydantic/utils.py](https://codecov.io/gh/samuelcolvin/pydantic/pull/2447/diff?src=pr&el=tree#diff-cHlkYW50aWMvdXRpbHMucHk=) | `98.29% <0.00%> (-1.71%)` | :arrow_down: |
| [pydantic/types.py](https://codecov.io/gh/samuelcolvin/pydantic/pull/2447/diff?src=pr&el=tree#diff-cHlkYW50aWMvdHlwZXMucHk=) | `99.60% <0.00%> (-0.40%)` | :arrow_down: |
| [pydantic/\_hypothesis\_plugin.py](https://codecov.io/gh/samuelcolvin/pydantic/pull/2447/diff?src=pr&el=tree#diff-cHlkYW50aWMvX2h5cG90aGVzaXNfcGx1Z2luLnB5) | `100.00% <0.00%> (ø)` | |
| ... and [1 more](https://codecov.io/gh/samuelcolvin/pydantic/pull/2447/diff?src=pr&el=tree-more) | |

------

[Continue to review full report at Codecov](https://codecov.io/gh/samuelcolvin/pydantic/pull/2447?src=pr&el=continue).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/samuelcolvin/pydantic/pull/2447?src=pr&el=footer). Last update [d619457...69dc6a0](https://codecov.io/gh/samuelcolvin/pydantic/pull/2447?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).

davidolrik: I think this will also allow a fix for my issue #2450, where I want default ports on http urls.

Will a stringification include the defaults? or will the original string be preserved?

MihanixA: > I think this will also allow a fix for my issue #2450, where I want default ports on http urls.
> 
> Will a stringification include the defaults? or will the original string be preserved?

An example of its behaviour is shown in the test for `KafkaDsn` model. Default host and port are included to the str version. 

https://github.com/samuelcolvin/pydantic/pull/2447/files#diff-f953eed7c7855cc439287af4864f147a1ed9c36995d18f335b57b96e3d7653b3R385
MihanixA: @davidolrik I could commit this solution and link your issue to this ticket. What do you think? 
MihanixA: I've also added default `443` or `80` ports to `HttpUrl` model which are accessible but not displayed in the stringified version to this PR. 
@davidolrik @samuelcolvin 

MihanixA: I've also added default 443 or 80 ports to HttpUrl model which are accessible but not displayed in the stringified version to this PR.
@davidolrik @samuelcolvin
davidolrik: Just what I was thinking! 😁

Maybe also add test for http urls with ports included which is also one of the defaults?

```python
m = Model(v='https://www.example.com:443')
assert m.v.port == '443'
assert m.v == 'https://www.example.com:443'
```

davidolrik: I was just playing around with your branch, and I found a bug:

```python
m = Model(v='https://www.example.com:1234')
assert m.v.port == '1234'
assert m.v == 'https://www.example.com:1234'
```

Here the port is also set to 443:

```
>       assert m.v.port == '1234'
E       AssertionError: assert '443' == '1234'
E         - 1234
E         + 443
```

MihanixA: I didn't notice any valid test in `test_networks.py` with port except 443 and 80 and I decided for a moment that behaviour with ports like 1234 is unsupported. 
I had already been working on fix by the time I read your message though 
But thank you any way! :) 
MihanixA: @davidolrik added tests as you requested 
MihanixA: #2450 
MihanixA: @samuelcolvin ready for review 
MihanixA: @PrettyWood I'd appreciate review and/or some feedback 🛩️ 😃 
MihanixA: I couldn't make this work with default port in `HttpUrl` for now. I really tried though
So I guess I am going to commit fixes for the PR issues and try to solve `HttpUrl` problem in a separate PR..
MihanixA: I added some `# noqa` (pls tell if you want them removed) 
And also made `HttpUrl` inherit `AnyHttpUrl` instead of `AnyUrl` 
MihanixA: I noticed that CI test "test FastAPI" failed. I run through an error message and I not sure if it's caused by my changes..
MihanixA: @PrettyWood 🙃 🎉 
PrettyWood: Thanks! Don't worry about the CI it's due to the new sqlalchemy 1.4 version that is not pinned nor supported by `encoded/databases`. I opened https://github.com/encode/databases/pull/299
MihanixA: Thank you for feedback!

> For the default port don't we want to add
> 
> ```python
>     @staticmethod
>     def get_default_parts(parts: 'Parts') -> 'PartialParts':
>         return {
>             'port': '80' if parts['scheme'] == 'http' else '443',
>         }
> ```

Maybe use only `'PartialParts'` without `'Parts'`? By the time `get_default_parts` is called, `parts` argument doesn't comply with described `TypedDict`. Besides this move would allow to avoid code duplication. 
MihanixA: @PrettyWood 👻 🥳 
MihanixA: > I'd like to continue to work on that issue in a separate PR when the situation about default parts is finalized)

Nevermind :) 
PrettyWood: Can you rebase please ?
MihanixA: @PrettyWood done as you requested.

All tests passed according to CI. 
Though when I run tests locally, I get this:
```
========================================================================================= short test summary info ==========================================================================================
FAILED tests/test_generics.py::test_replace_types - IndexError: tuple index out of range
FAILED tests/test_schema.py::test_unenforced_constraints_schema[kwargs9-type_9] - IndexError: tuple index out of range

Results (57.85s):
    2045 passed
       2 failed
         - tests/test_generics.py:787 test_replace_types
         - tests/test_schema.py:1442 test_unenforced_constraints_schema[kwargs9-type_9]
       6 skipped
make: *** [test] Error 1
```

Also codecov is less then 100% and I am not sure why :c 
PrettyWood: You have all the commits from master that unrelated. Please rebase on the upstream version and push force to have a clean change diff. And we'll see after for the coverage
MihanixA: Looks like I broke everything in this branch(
I am going to create close this branch and create a new one with clear change history
OK? @PrettyWood 
PrettyWood: Lol no you didn't ;) it's just your rebase that was done on your local version of master and not the upstream one. I can rewrite your history if you need help
MihanixA: > Lol no you didn't ;) it's just your rebase that was done on your local version of master and not the upstream one. I can rewrite your history if you need help

I'd appreciate that!
MihanixA: All tests passed. That is great! 
The only thing that remains is to push merge button 👻 
MihanixA: @PrettyWood 
PrettyWood: Samuel merges the PRs. And a 1.8.2 may be released before 1.9.
MihanixA: > Samuel merges the PRs. And a 1.8.2 may be released before 1.9.

@samuelcolvin ok, thanks! Did you forget to click 'Approve' button or there is something I should fix? 
PrettyWood: Thank you!

Patch:
diff --git a/changes/2447-MihanixA.md b/changes/2447-MihanixA.md
new file mode 100644
index 000000000..ea83cd133
--- /dev/null
+++ b/changes/2447-MihanixA.md
@@ -0,0 +1,2 @@
+ - add `KafkaDsn` type
+ - `HttpUrl` now has default port 80 for http and 443 for https
diff --git a/pydantic/__init__.py b/pydantic/__init__.py
index 7680ec3be..363701401 100644
--- a/pydantic/__init__.py
+++ b/pydantic/__init__.py
@@ -57,6 +57,7 @@ __all__ = [
     'IPvAnyNetwork',
     'PostgresDsn',
     'RedisDsn',
+    'KafkaDsn',
     'validate_email',
     # parse
     'Protocol',
diff --git a/pydantic/networks.py b/pydantic/networks.py
index c9fccaeec..48e53a371 100644
--- a/pydantic/networks.py
+++ b/pydantic/networks.py
@@ -31,12 +31,27 @@ from .validators import constr_length_validator, str_validator
 
 if TYPE_CHECKING:
     import email_validator
+    from typing_extensions import TypedDict
 
     from .config import BaseConfig
     from .fields import ModelField
     from .typing import AnyCallable
 
     CallableGenerator = Generator[AnyCallable, None, None]
+
+    class Parts(TypedDict, total=False):
+        scheme: str
+        user: Optional[str]
+        password: Optional[str]
+        ipv4: Optional[str]
+        ipv6: Optional[str]
+        domain: Optional[str]
+        port: Optional[str]
+        path: Optional[str]
+        query: Optional[str]
+        fragment: Optional[str]
+
+
 else:
     email_validator = None
 
@@ -54,6 +69,7 @@ __all__ = [
     'IPvAnyNetwork',
     'PostgresDsn',
     'RedisDsn',
+    'KafkaDsn',
     'validate_email',
 ]
 
@@ -109,6 +125,7 @@ class AnyUrl(str):
     allowed_schemes: Optional[Set[str]] = None
     tld_required: bool = False
     user_required: bool = False
+    hidden_parts: Set[str] = set()
 
     __slots__ = ('scheme', 'user', 'password', 'host', 'tld', 'host_type', 'port', 'path', 'query', 'fragment')
 
@@ -155,7 +172,7 @@ class AnyUrl(str):
         path: Optional[str] = None,
         query: Optional[str] = None,
         fragment: Optional[str] = None,
-        **kwargs: str,
+        **_kwargs: str,
     ) -> str:
         url = scheme + '://'
         if user:
@@ -165,7 +182,7 @@ class AnyUrl(str):
         if user or password:
             url += '@'
         url += host
-        if port:
+        if port and 'port' not in cls.hidden_parts:
             url += ':' + port
         if path:
             url += path
@@ -196,7 +213,9 @@ class AnyUrl(str):
         # the regex should always match, if it doesn't please report with details of the URL tried
         assert m, 'URL regex failed unexpectedly'
 
-        parts = m.groupdict()
+        original_parts = cast('Parts', m.groupdict())
+        cls.hide_parts(original_parts)
+        parts = cls.apply_default_parts(original_parts)
         parts = cls.validate_parts(parts)
 
         host, tld, host_type, rebuild = cls.validate_host(parts)
@@ -219,7 +238,7 @@ class AnyUrl(str):
         )
 
     @classmethod
-    def validate_parts(cls, parts: Dict[str, str]) -> Dict[str, str]:
+    def validate_parts(cls, parts: 'Parts') -> 'Parts':
         """
         A method used to validate parts of an URL.
         Could be overridden to set default values for parts if missing
@@ -242,10 +261,10 @@ class AnyUrl(str):
         return parts
 
     @classmethod
-    def validate_host(cls, parts: Dict[str, str]) -> Tuple[str, Optional[str], str, bool]:
+    def validate_host(cls, parts: 'Parts') -> Tuple[str, Optional[str], str, bool]:
         host, tld, host_type, rebuild = None, None, None, False
         for f in ('domain', 'ipv4', 'ipv6'):
-            host = parts[f]
+            host = parts[f]  # type: ignore[misc]
             if host:
                 host_type = f
                 break
@@ -281,6 +300,21 @@ class AnyUrl(str):
 
         return host, tld, host_type, rebuild  # type: ignore
 
+    @staticmethod
+    def get_default_parts(parts: 'Parts') -> 'Parts':
+        return {}
+
+    @classmethod
+    def hide_parts(cls, original_parts: 'Parts') -> None:
+        cls.hidden_parts = set()
+
+    @classmethod
+    def apply_default_parts(cls, parts: 'Parts') -> 'Parts':
+        for key, value in cls.get_default_parts(parts).items():
+            if not parts[key]:  # type: ignore[misc]
+                parts[key] = value  # type: ignore[misc]
+        return parts
+
     def __repr__(self) -> str:
         extra = ', '.join(f'{n}={getattr(self, n)!r}' for n in self.__slots__ if getattr(self, n) is not None)
         return f'{self.__class__.__name__}({super().__repr__()}, {extra})'
@@ -290,12 +324,21 @@ class AnyHttpUrl(AnyUrl):
     allowed_schemes = {'http', 'https'}
 
 
-class HttpUrl(AnyUrl):
-    allowed_schemes = {'http', 'https'}
+class HttpUrl(AnyHttpUrl):
     tld_required = True
     # https://stackoverflow.com/questions/417142/what-is-the-maximum-length-of-a-url-in-different-browsers
     max_length = 2083
 
+    @staticmethod
+    def get_default_parts(parts: 'Parts') -> 'Parts':
+        return {'port': '80' if parts['scheme'] == 'http' else '443'}
+
+    @classmethod
+    def hide_parts(cls, original_parts: 'Parts') -> None:
+        super().hide_parts(original_parts)
+        if 'port' in original_parts:
+            cls.hidden_parts.add('port')
+
 
 class PostgresDsn(AnyUrl):
     allowed_schemes = {
@@ -314,17 +357,24 @@ class PostgresDsn(AnyUrl):
 class RedisDsn(AnyUrl):
     allowed_schemes = {'redis', 'rediss'}
 
-    @classmethod
-    def validate_parts(cls, parts: Dict[str, str]) -> Dict[str, str]:
-        defaults = {
+    @staticmethod
+    def get_default_parts(parts: 'Parts') -> 'Parts':
+        return {
             'domain': 'localhost' if not (parts['ipv4'] or parts['ipv6']) else '',
             'port': '6379',
             'path': '/0',
         }
-        for key, value in defaults.items():
-            if not parts[key]:
-                parts[key] = value
-        return super().validate_parts(parts)
+
+
+class KafkaDsn(AnyUrl):
+    allowed_schemes = {'kafka'}
+
+    @staticmethod
+    def get_default_parts(parts: 'Parts') -> 'Parts':
+        return {
+            'domain': 'localhost',
+            'port': '9092',
+        }
 
 
 def stricturl(


Test patch:
diff --git a/tests/test_networks.py b/tests/test_networks.py
index bb10ae9b8..f4bcd42ac 100644
--- a/tests/test_networks.py
+++ b/tests/test_networks.py
@@ -1,6 +1,17 @@
 import pytest
 
-from pydantic import AnyUrl, BaseModel, EmailStr, HttpUrl, NameEmail, PostgresDsn, RedisDsn, ValidationError, stricturl
+from pydantic import (
+    AnyUrl,
+    BaseModel,
+    EmailStr,
+    HttpUrl,
+    KafkaDsn,
+    NameEmail,
+    PostgresDsn,
+    RedisDsn,
+    ValidationError,
+    stricturl,
+)
 from pydantic.networks import validate_email
 
 try:
@@ -332,6 +343,45 @@ def test_parses_tld(input, output):
     assert Model(v=input).v.tld == output
 
 
+def test_get_default_parts():
+    class MyConnectionString(AnyUrl):
+        @staticmethod
+        def get_default_parts(parts):
+            # get default parts allows to generate custom conn strings to services
+            return {
+                'user': 'admin',
+                'password': '123',
+            }
+
+    class C(BaseModel):
+        connection: MyConnectionString
+
+    c = C(connection='protocol://service:8080')
+    assert c.connection == 'protocol://admin:123@service:8080'
+    assert c.connection.user == 'admin'
+    assert c.connection.password == '123'
+
+
+@pytest.mark.parametrize(
+    'url,port',
+    [
+        ('https://www.example.com', '443'),
+        ('https://www.example.com:443', '443'),
+        ('https://www.example.com:8089', '8089'),
+        ('http://www.example.com', '80'),
+        ('http://www.example.com:80', '80'),
+        ('http://www.example.com:8080', '8080'),
+    ],
+)
+def test_http_urls_default_port(url, port):
+    class Model(BaseModel):
+        v: HttpUrl
+
+    m = Model(v=url)
+    assert m.v.port == port
+    assert m.v == url
+
+
 def test_postgres_dsns():
     class Model(BaseModel):
         a: PostgresDsn
@@ -388,6 +438,28 @@ def test_redis_dsns():
     assert m.a.path == '/0'
 
 
+def test_kafka_dsns():
+    class Model(BaseModel):
+        a: KafkaDsn
+
+    m = Model(a='kafka://')
+    assert m.a.scheme == 'kafka'
+    assert m.a.host == 'localhost'
+    assert m.a.port == '9092'
+    assert m.a == 'kafka://localhost:9092'
+
+    m = Model(a='kafka://kafka1')
+    assert m.a == 'kafka://kafka1:9092'
+
+    with pytest.raises(ValidationError) as exc_info:
+        Model(a='http://example.org')
+    assert exc_info.value.errors()[0]['type'] == 'value_error.url.scheme'
+
+    m = Model(a='kafka://kafka3:9093')
+    assert m.a.user is None
+    assert m.a.password is None
+
+
 def test_custom_schemes():
     class Model(BaseModel):
         v: stricturl(strip_whitespace=False, allowed_schemes={'ws', 'wss'})  # noqa: F821