tkdrob
concerns about url port autocomplete
Checklist
- [X] I'm running the newest version https://github.com/tkdrob/aiopyarr/releases/latest
- [X] I have filled out the issue template to the best of my ability.
- [X] This issue only contains 1 issue (if you have multiple issues, open one issue for each issue).
Describe the issue
I noticed in #36 and #38 that there is one setup not considered: reverse proxies that map a subdomain or subpath to a service at a defined port usually with https. so:
https://sonarr.local.xyz http://local.xyz/radarr
these shouldn't have port autocompleted or like the ssl should like use port 443 and non-ssl port 80
perhaps autocomplete should only happen for IP addresses as that would be mostly safe since IPs are rarely used with SSL and wouldn't be typical of reverse proxy usage.
however when you configure sonarr or radarr for SSL through settings (no reverse proxy) they use port 9898 so might be good to try that port when ip and https.
Logs
N/A
I'm having the same issues with ports now. I don't think it's a good idea to forcefully inject the ports, sometimes we host it on other ports or with proxies, and that makes it tedious to use this package as it's made right now.
This block especially makes me wonder;
if self._host.url:
regex = r"(https?:\/\/[a-z|\d|\.]*[^:])([\/^:\d]*)(.*)"
if (res := search(regex, self._host.url)) and not res.group(2):
self._host.url = f"{res.group(1).rstrip('/')}:{port}/{res.group(3)}"
self._host.url = self._host.url.rstrip("/")
If I have specified the entire base url in my configuration, why would you parse that and inject a port into it? I can understand the simplicity of default ports if you don't specify the entire config it will complete it with the sonarr default setup.
Are there good reasons for doing this, which edge case inspired the code block above?
If that block was removed, I could get around the port defaults just fine, since I would like to specify the entire url.
Another way you could allow us to configure away the port settings would be to make the port optional, which right now it is not really.
port: int = 8989,
Could be
port: int | None = 8989,
And then check later if port is None, and inject port if it is a number. That way I can explicitly declare it as None. I think this is a thing that should be done in addition to remove the parsing of a specified url, regardless of port settings.
TBH, this was a design choice with only Home Assistant in mind to make it one field configurable. I will have to look at possible breaking changes for others already relying on this package. The host configuration is created if it is not already specified. Can you try creating a host configuration with no url set and then pass that during client construction? That should skip the code that is causing the problem.
Not passing any host configuration still makes one and sets a port number in it.
When making requests, the following happens:
# in request_client.py
async def _async_request( # pylint:disable=too-many-arguments
...
) -> Any:
"""Send API request."""
url = self._host.api_url(command)
try:
request = await self._session.request(
url=url,
...
)
# in host_configuration.py
def api_url(self, command: str, initialize: bool = False) -> str:
"""Return the generated base URL based on host configuration."""
if initialize:
return f"{self.base_url}/initialize.js"
return f"{self.base_url}/api/{self.api_ver}/{command}"
@property
def base_url(self) -> str:
"""Return the base URL for the configured service."""
if self.url is not None:
return self.url
protocol = f"http{'s' if self.ssl else ''}"
host = f"{self.hostname or self.ipaddress}:{self.port}"
return f"{protocol}://{host}{self.base_api_path or ''}"
So a port is forced into the url configuration regardless. I can't remove it without modifying the library, as far as I can tell. All I have done so far is to just remove the block that modifies the url (from my first comment), and I can use it just fine.
If you just implement a check whether port is None when you check the regex, and skip if I explicitly set port to None, then it would be fine for me. It's a little janky, but workable.
Yes, not passing any host configuration still makes one and sets a port number in it. I was asking you to pass a host configuration without a url. The test suit I believe shows this working.
https://github.com/tkdrob/aiopyarr/blob/master/tests/test_host_configuration.py#L31
A url based reverse proxy setup not injecting a port will still need to be worked on but this should allow you to make this work from behind the reverse proxy.
Sorry, I read that a little too quickly. You can indeed, but I can't really find a way to avoid your lib forcing a port number into the base_url.
I need to hit this base url: https://hostname.com/tv
Specifying port 443 is no bueno either, but that could be me making some mistake as well. I just feel like it's wrong to assume that when I specify a complete url to the configuration, there's still a need to inject a port into it. Both this, and the fact that if I could just set port = None it should not assume that I want an integer inserted anywhere.
I can understand that there's a usecase for zero config, but if I do make one, why override what I specify in it?
For now, use the package like this. This should be good for reverse proxies. The question of why was already answered above in my first post.
host_config = PyArrHostConfiguration(
api_token=API_TOKEN,
ssl=True,
url="https://my.domain.com:443",
)
async with ClientSession():
client = RadarrClient(host_configuration=host_config)
await client.async_get_system_status()
I really hate that I have to specify the URL in home assistant like this https://sonarr.example.com:443. It is super unintuitive and has had me trying different URLs multiple times until figuring this out. I think it would much benefit the user experience if this behaviour could be changed.
