feat: add build method back to multihosturl

[ollz272]

Change Summary

Linked to pydantic/pydantic#6355 in pydantic. Essentially, we used to have a build() method on the AnyUrl class:

@classmethod
    def build(
        cls,
        *,
        scheme: str,
        user: Optional[str] = None,
        password: Optional[str] = None,
        host: str,
        port: Optional[str] = None,
        path: Optional[str] = None,
        query: Optional[str] = None,
        fragment: Optional[str] = None,
        **_kwargs: str,
    ) -> str:
        parts = Parts(
            scheme=scheme,
            user=user,
            password=password,
            host=host,
            port=port,
            path=path,
            query=query,
            fragment=fragment,
            **_kwargs,  # type: ignore[misc]
        )

        url = scheme + '://'
        if user:
            url += user
        if password:
            url += ':' + password
        if user or password:
            url += '@'
        url += host
        if port and ('port' not in cls.hidden_parts or cls.get_default_parts(parts).get('port') != port):
            url += ':' + port
        if path:
            url += path
        if query:
            url += '?' + query
        if fragment:
            url += '#' + fragment
        return url

However this seems to have been lost in the transition to v2. This PR attempts to re implement it. I'm new to rust so this may not be done right, however it seems to be working as expected for me. Any help pushing this forward would be appreciated.

Related issue number

pydantic/pydantic#6355

Checklist

• Unit tests for the changes exist
• Documentation reflects the changes where applicable
• Pydantic tests pass with this pydantic-core (except for expected changes)
• My PR is ready to review, please add a comment including the phrase "please review" to assign reviewers

Selected Reviewer: @samuelcolvin

[ollz272]

please review

[codecov]

Codecov Report

Merging #730 (a202960) into main (1a17d42) will increase coverage by 0.01%.
The diff coverage is 95.45%.

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #730      +/-   ##
==========================================
+ Coverage   93.65%   93.67%   +0.01%     
==========================================
  Files          99       99              
  Lines       13970    14080     +110     
  Branches       25       25              
==========================================
+ Hits        13084    13189     +105     
- Misses        880      885       +5     
  Partials        6        6              

Impacted Files	Coverage Δ
src/url.rs	98.30% <95.45%> (-1.29%)	⬇️

---

Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 1a17d42...a202960. Read the comment docs.

[codspeed-hq]

CodSpeed Performance Report

Merging #730 will not alter performances

_{Comparing ollz272:add-build-method-to-multihosturl (a202960) with main (1a17d42)}

Summary

✅ 126 untouched benchmarks

[samuelcolvin]

I'm impressived if this is your first foray into Rust, congrats.

Overall good start, but some things to add and alter.

[samuelcolvin]

this method be on both PyUrl (with this signature), and PyMultiHostUrl - I guess allowing multiple hosts.

[ollz272]

Thanks @samuelcolvin - not 100% sure what it is thats needed here, are you saying:

• Implement the method on PyUrl too
• Have PyMultiHostUrl call its ref_url.build for its build function? or implement differently logic for PyMultiHostUrl?

[ollz272]

@samuelcolvin I think I've addressed your comments now

[davidhewitt]

Design question: do we want these methods to return Url / MultiHostUrl, rather than str?

[davidhewitt]

I think we want to accept multiple host/user/password/port combinations in MutiHostUrl.build. Potentially easiest to accept these as MultiHostHost arguments (see __init__.py).

On the Rust side that would potentially be hosts: Vec<&PyDict> where you could then extract each host in turn.

[ollz272]

Interestingly again the existing behaviour was that Url/MultiHostUrl had the same .build() behaviour in V1, so thats what I've done here. Would be happy to change if that's what we think needs to be done! 🙂

[ollz272]

Design question: do we want these methods to return Url / MultiHostUrl, rather than str?

Maybe! For this i reimplemented existing behaviour which returned str, but it may make sense to change this.

[davidhewitt]

👍 let's get feedback from @samuelcolvin whether to stick to the V1 design or to make breaking changes.

[davidhewitt]

FWIW I think rebasing should clear out the large claimed change in benchmarks.

[ollz272]

Thanks for the help there @davidhewitt! I think i've got an implementation with those changes done

[Kludex]

please review

[davidhewitt]

Thank you for the many refinements, overall looks great!

If you're interested, I've put a rust style suggestion about using format! in the Display implementation of MultiHostUrlHost. Additionally, a question about whether it makes sense to reuse that implementation instead of the match statements you put around the place.

And finally, I've noticed that I defined MultiHostUrlHost wrong and it probably should use an integer for the port 🙈 . This does raise the question if we should update the other port arguments to be integers too?

[ollz272]

Thanks @davidhewitt ! Made the changes you suggested 😄 left a question as well

[ollz272]

please review

[davidhewitt]

Great! Just two final comments, and once we've decided on those, let's merge 🚀

[ollz272]

@davidhewitt last comments addressed, should be GTG 🚀

[davidhewitt]

This is looking good to me, many thanks! I'll merge as soon as CI is green 👍

Changes are addressed

[Kludex]

Yey! 🎉

[tshipenchko]

Looks like MultiHostUrl.build() returns MultiHostUrl, not str. So type-hint is wrong.

>>> from pydantic_core import MultiHostUrl
>>> type(MultiHostUrl.build(scheme="http", host="example.com"))
<class 'pydantic_core._pydantic_core.MultiHostUrl'>

pydantic_core version: 2.3.0 from pypi

>>> url = MultiHostUrl.build(scheme="http", host="example.com")
>>> isinstance(url, str)
False

Isinstance also fails. It breaks SQLAlchemy for example

  File "/home/user/.cache/pypoetry/virtualenvs/-5tQAlcAR-py3.10/lib/python3.10/site-packages/sqlalchemy/engine/url.py", line 842, in make_url
    raise exc.ArgumentError(
sqlalchemy.exc.ArgumentError: Expected string or URL object, got MultiHostUrl('postgresql+asyncpg://postgres@localhost:5432//postgres')

[Kludex]

• Yes. Fixed in fix type annotations for Url & MultiHostUrl build #781.

[tshipenchko]

@Kludex shouldn't it return str instance like in v1? Or it is planned to return MultiHostUrl in v2

[Kludex]

@Kludex shouldn't it return str instance like in v1? Or it is planned to return MultiHostUrl in v2

Eh... You are literally in the PR that has the discussion about the design decision.