From 27e7eecf16dc616d048820d02f93c29920b59060 Mon Sep 17 00:00:00 2001
From: Sahas Subramanian <sahas.subramanian@proton.me>
Date: Tue, 3 Sep 2024 10:56:56 +0200
Subject: [PATCH] Remove default port from grpc ChannelOptions

URIs should always specify a port.  If they don't, a `ValueError` will
be raised.

Signed-off-by: Sahas Subramanian <sahas.subramanian@proton.me>
---
 src/frequenz/client/base/channel.py | 10 ++++------
 tests/test_channel.py               | 27 +++++++++++----------------
 tests/test_client.py                |  2 +-
 3 files changed, 16 insertions(+), 23 deletions(-)

diff --git a/src/frequenz/client/base/channel.py b/src/frequenz/client/base/channel.py
index 5a930e9..ae39474 100644
--- a/src/frequenz/client/base/channel.py
+++ b/src/frequenz/client/base/channel.py
@@ -45,9 +45,6 @@ class SslOptions:
 class ChannelOptions:
     """Options for a gRPC channel."""
 
-    port: int = 9090
-    """The port number to connect to."""
-
     ssl: SslOptions = SslOptions()
     """SSL options for the channel."""
 
@@ -110,9 +107,10 @@ def parse_grpc_uri(
 
     options = _parse_query_params(uri, parsed_uri.query)
 
-    target = (
-        parsed_uri.netloc if parsed_uri.port else f"{parsed_uri.netloc}:{defaults.port}"
-    )
+    if not parsed_uri.port:
+        raise ValueError(f"The gRPC URI '{uri}' doesn't specify a port.")
+
+    target = parsed_uri.netloc
 
     ssl = defaults.ssl.enabled if options.ssl is None else options.ssl
     if ssl:
diff --git a/tests/test_channel.py b/tests/test_channel.py
index f5ebe3c..12b8867 100644
--- a/tests/test_channel.py
+++ b/tests/test_channel.py
@@ -24,6 +24,7 @@ class _ValidUrlTestCase:
     title: str
     uri: str
     expected_host: str
+    expected_port: int
     expected_options: ChannelOptions
     defaults: ChannelOptions = ChannelOptions()
 
@@ -33,10 +34,10 @@ class _ValidUrlTestCase:
     [
         _ValidUrlTestCase(
             title="default",
-            uri="grpc://localhost",
+            uri="grpc://localhost:9090",
             expected_host="localhost",
+            expected_port=9090,
             expected_options=ChannelOptions(
-                port=9090,
                 ssl=SslOptions(
                     enabled=True,
                     root_certificates=None,
@@ -47,11 +48,11 @@ class _ValidUrlTestCase:
         ),
         _ValidUrlTestCase(
             title="default no SSL defaults",
-            uri="grpc://localhost",
-            defaults=ChannelOptions(port=2355, ssl=SslOptions(enabled=False)),
+            uri="grpc://localhost:2355",
+            defaults=ChannelOptions(ssl=SslOptions(enabled=False)),
             expected_host="localhost",
+            expected_port=2355,
             expected_options=ChannelOptions(
-                port=2355,
                 ssl=SslOptions(
                     enabled=False,
                     root_certificates=None,
@@ -62,9 +63,8 @@ class _ValidUrlTestCase:
         ),
         _ValidUrlTestCase(
             title="default with SSL defaults",
-            uri="grpc://localhost",
+            uri="grpc://localhost:2355",
             defaults=ChannelOptions(
-                port=2355,
                 ssl=SslOptions(
                     enabled=True,
                     root_certificates=None,
@@ -73,8 +73,8 @@ class _ValidUrlTestCase:
                 ),
             ),
             expected_host="localhost",
+            expected_port=2355,
             expected_options=ChannelOptions(
-                port=2355,
                 ssl=SslOptions(
                     enabled=True,
                     root_certificates=None,
@@ -88,8 +88,8 @@ class _ValidUrlTestCase:
             uri="grpc://localhost:1234?ssl=1&ssl_root_certificates_path=/root_cert"
             "&ssl_private_key_path=/key&ssl_certificate_chain_path=/chain",
             expected_host="localhost",
+            expected_port=1234,
             expected_options=ChannelOptions(
-                port=1234,
                 ssl=SslOptions(
                     enabled=True,
                     root_certificates=pathlib.Path("/root_cert"),
@@ -103,7 +103,6 @@ class _ValidUrlTestCase:
             uri="grpc://localhost:1234?ssl=1&ssl_root_certificates_path=/root_cert"
             "&ssl_private_key_path=/key&ssl_certificate_chain_path=/chain",
             defaults=ChannelOptions(
-                port=4444,
                 ssl=SslOptions(
                     enabled=True,
                     root_certificates=pathlib.Path("/root_cert_def"),
@@ -112,8 +111,8 @@ class _ValidUrlTestCase:
                 ),
             ),
             expected_host="localhost",
+            expected_port=1234,
             expected_options=ChannelOptions(
-                port=1234,
                 ssl=SslOptions(
                     enabled=True,
                     root_certificates=pathlib.Path("/root_cert"),
@@ -138,11 +137,7 @@ def test_parse_uri_ok(  # pylint: disable=too-many-locals
     expected_credentials = mock.MagicMock(
         name="mock_credentials", spec=ssl_channel_credentials
     )
-    expected_port = (
-        expected_options.port
-        if f":{expected_options.port}" in uri or defaults.port is None
-        else defaults.port
-    )
+    expected_port = case.expected_port
     expected_ssl = (
         expected_options.ssl.enabled
         if "ssl=" in uri or defaults.ssl.enabled is None
diff --git a/tests/test_client.py b/tests/test_client.py
index c59e240..2cbdcbc 100644
--- a/tests/test_client.py
+++ b/tests/test_client.py
@@ -105,7 +105,7 @@ def test_base_api_client_init_with_channel_defaults(
     mocker: pytest_mock.MockFixture,
 ) -> None:
     """Test initializing the BaseApiClient with channel defaults."""
-    channel_defaults = ChannelOptions(port=1234, ssl=SslOptions(enabled=False))
+    channel_defaults = ChannelOptions(ssl=SslOptions(enabled=False))
     client, mocks = create_client_with_mocks(mocker, channel_defaults=channel_defaults)
     assert client.server_url == _DEFAULT_SERVER_URL
     mocks.parse_grpc_uri.assert_called_once_with(client.server_url, channel_defaults)