Fix pagination links for Orders pagination wrt the use of overriding url_for

[philvarner]

What I'm changing

Currently, the pagination links for Orders and Order Statuses don't have next link relation hrefs with the value returned from url_for, so they're always relative to the server's URL (default) rather than what the implementation has overridden url_for to do.

How I did it

• the pagination link function calls the url_for function now, so the url is correct

Checklist

• Tests pass: uv run pytest
• Checks pass: uv run pre-commit run --all-files
• CHANGELOG is updated (if necessary)

[jkeifer]

Maybe self.url_for shouldn't be returning a string? If so then I think maybe you do this:

href = str(
    self.url_for(
        request,
        f"{self.name}:{ROOT}"
     ).include_query_params(
        next=pagination_token,
        limit=limit,
     ),
)

The need to stringify the the URL is not because the Link class actually wants a string. The problem is that the model uses the pydantic type AnyUrl where FastAPI is built on starlette, the latter of which has its own URL class that is not directly compatible with AnyUrl. For that matter, str isn't exactly either, but note the Link model is defined like so:

class Link(BaseModel):
    href: AnyUrl
    rel: str
    type: str | None = None
    title: str | None = None
    method: str | None = None
    headers: dict[str, str | list[str]] | None = None
    body: Any = None

    model_config = ConfigDict(extra="allow")

    # redefining init is a hack to get str type to validate for `href`,
    # as str is ultimately coerced into an AnyUrl automatically anyway
    def __init__(self, href: AnyUrl | str, **kwargs: Any) -> None:
        super().__init__(href=href, **kwargs)

That __init__ allows us to use str as the serialized URL format to convert between starlette's URL type and AnyUrl, because AnyUrl will convert str instances to itself (assuming the str is a valid URL). What we could do here to obviate the need for every call site to convert starlette URLs to str might be something like this:

    def __init__(self, href: Any, **kwargs: Any) -> None:
        if not isinstance(href, (AnyUrl, str)):
            href = str(href)
        super().__init__(href=href, **kwargs)

With this new transformation happening in the __init__, it feels like a before validator on the href field would be a better approach. Except I don't think before validators will change the __init__ signature so type checking would not allow anything but AnyUrl without an override. But I also think the kwargs as written is not ideal because that is also breaking type checking for the other fields??? Whatever I guess, that's an existing issue no one is complaining about, we can ignore it for now unless we want to better type the __init__ override.

To summarize: I don't love doing replace when instances not overriding url_for will run the replace without any change to the generated href. So I'm looking to a solution that can avoid that extra operation without having to re-insert the str() call everywhere it has been removed.

[philvarner]

yeah, i thought about that -- i think i was trying to make the minimal change that could work, and now force users to change how they're overriding url_for -- but I think i'm the only consumer of this so far, so I should just do it this way -- thanks for the encouragement.

[philvarner]

@jkeifer reworked, please re-review, thanks!

[jkeifer]

Doesn't this make starlette a new dependency of stapi-pydantic? It seems like we should avoid adding a dependency just to get a type. Normally I'd use a protocol for this, but everything in python supports __str__ so it doesn't really seem like something we can check for (hence my previous suggestion, not that I'm attached to the other code, just that was my justification for not using the URL type).

[philvarner]

ah good point -- I didn't catch that because the dev dependencies are messed up. there's a root dependency on fastapi[standard] for some reason. I'm going to look into fixing that first.

[philvarner]

refactored this and got it fail as it should. See MR #126

[philvarner]

Merged the refactor MR, so all the tests are run with only the dependencies defined in their pyproject.toml. After that, the changes in this MR failed because fastapi isn't a dependency of stapi-pydantic. Refactored that to not use that class in the type definition, with code similar to what you suggested earlier.