Copilot commented on code in PR #2055:
URL: https://github.com/apache/iceberg-python/pull/2055#discussion_r2231654180
##########
pyiceberg/catalog/rest/__init__.py:
##########
@@ -247,7 +249,23 @@ def _create_session(self) -> Session:
elif ssl_client_cert := ssl_client.get(CERT):
session.cert = ssl_client_cert
- session.auth =
AuthManagerAdapter(self._create_legacy_oauth2_auth_manager(session))
+ if auth_config := self.properties.get(AUTH):
+ auth_type = auth_config.get("type")
+ if auth_type is None:
+ raise ValueError("auth.type must be defined")
+ auth_type_config = auth_config.get(auth_type, {})
+ auth_impl = auth_config.get("impl")
+
+ if auth_type is CUSTOM and not auth_impl:
+ raise ValueError("auth.impl must be specified when using
custom auth.type")
+
+ if auth_type is not CUSTOM and auth_impl:
Review Comment:
The comparison `auth_type is CUSTOM` uses identity comparison with a string
literal. This should use equality comparison (`==`) instead since `auth_type`
comes from dictionary input and may not be the same string object.
```suggestion
if auth_type == CUSTOM and not auth_impl:
raise ValueError("auth.impl must be specified when using
custom auth.type")
if auth_type != CUSTOM and auth_impl:
```
##########
tests/catalog/test_rest.py:
##########
@@ -1519,6 +1520,132 @@ def test_request_session_with_ssl_client_cert() -> None:
assert "Could not find the TLS certificate file, invalid path:
path_to_client_cert" in str(e.value)
+def test_rest_catalog_with_basic_auth_type(rest_mock: Mocker) -> None:
+ # Given
+ rest_mock.get(
+ f"{TEST_URI}v1/config",
+ json={"defaults": {}, "overrides": {}},
+ status_code=200,
+ )
+ # Given
+ catalog_properties = {
+ "uri": TEST_URI,
+ "auth": {
+ "type": "basic",
+ "basic": {
+ "username": "one",
+ "password": "two",
+ },
+ },
+ }
+ catalog = RestCatalog("rest", **catalog_properties) # type: ignore
+ assert catalog.uri == TEST_URI
+
+ encoded_user_pass = base64.b64encode(b"one:two").decode()
+ expected_auth_header = f"Basic {encoded_user_pass}"
+ assert rest_mock.last_request.headers["Authorization"] ==
expected_auth_header
+
+
+def test_rest_catalog_with_custom_auth_type() -> None:
+ # Given
+ catalog_properties = {
+ "uri": TEST_URI,
+ "auth": {
+ "type": "custom",
+ "impl": "dummy.nonexistent.package",
+ "custom": {
+ "property1": "one",
+ "property2": "two",
+ },
+ },
+ }
+ with pytest.raises(ValueError) as e:
+ # Missing namespace
+ RestCatalog("rest", **catalog_properties) # type: ignore
+ assert "Could not load AuthManager class for 'dummy.nonexistent.package'"
in str(e.value)
+
+
+def test_rest_catalog_with_custom_basic_auth_type(rest_mock: Mocker) -> None:
+ # Given
+ catalog_properties = {
+ "uri": TEST_URI,
+ "auth": {
+ "type": "custom",
+ "impl": "pyiceberg.catalog.rest.auth.BasicAuthManager",
+ "custom": {
+ "username": "one",
+ "password": "two",
+ },
+ },
+ }
+ rest_mock.get(
+ f"{TEST_URI}v1/config",
+ json={"defaults": {}, "overrides": {}},
+ status_code=200,
+ )
+ catalog = RestCatalog("rest", **catalog_properties) # type: ignore
+ assert catalog.uri == TEST_URI
+
+ encoded_user_pass = base64.b64encode(b"one:two").decode()
+ expected_auth_header = f"Basic {encoded_user_pass}"
+ assert rest_mock.last_request.headers["Authorization"] ==
expected_auth_header
+
+
+def test_rest_catalog_with_custom_auth_type_no_impl() -> None:
+ # Given
+ catalog_properties = {
+ "uri": TEST_URI,
+ "auth": {
+ "type": "custom",
+ "custom": {
+ "property1": "one",
+ "property2": "two",
+ },
+ },
+ }
+ with pytest.raises(ValueError) as e:
+ # Missing namespace
+ RestCatalog("rest", **catalog_properties) # type: ignore
+ assert "auth.impl must be specified when using custom auth.type" in
str(e.value)
+
+
+def test_rest_catalog_with_non_custom_auth_type_impl() -> None:
+ # Given
+ catalog_properties = {
+ "uri": TEST_URI,
+ "auth": {
+ "type": "basic",
+ "impl": "basic.package",
+ "basic": {
+ "username": "one",
+ "password": "two",
+ },
+ },
+ }
+ with pytest.raises(ValueError) as e:
+ # Missing namespace
+ RestCatalog("rest", **catalog_properties) # type: ignore
+ assert "auth.impl can only be specified when using custom auth.type" in
str(e.value)
+
+
+def test_rest_catalog_with_unsupported_auth_type() -> None:
+ # Given
+ catalog_properties = {
+ "uri": TEST_URI,
+ "auth": {
+ "type": "unsupported",
+ "unsupported": {
+ "property1": "one",
+ "property2": "two",
+ },
+ },
+ }
+ with pytest.raises(ValueError) as e:
+ # Missing namespace
Review Comment:
The comment 'Missing namespace' is misleading. The test is actually checking
for an unsupported auth type, not a missing namespace.
```suggestion
# Unsupported authentication type
```
##########
tests/catalog/test_rest.py:
##########
@@ -1519,6 +1520,132 @@ def test_request_session_with_ssl_client_cert() -> None:
assert "Could not find the TLS certificate file, invalid path:
path_to_client_cert" in str(e.value)
+def test_rest_catalog_with_basic_auth_type(rest_mock: Mocker) -> None:
+ # Given
+ rest_mock.get(
+ f"{TEST_URI}v1/config",
+ json={"defaults": {}, "overrides": {}},
+ status_code=200,
+ )
+ # Given
+ catalog_properties = {
+ "uri": TEST_URI,
+ "auth": {
+ "type": "basic",
+ "basic": {
+ "username": "one",
+ "password": "two",
+ },
+ },
+ }
+ catalog = RestCatalog("rest", **catalog_properties) # type: ignore
+ assert catalog.uri == TEST_URI
+
+ encoded_user_pass = base64.b64encode(b"one:two").decode()
+ expected_auth_header = f"Basic {encoded_user_pass}"
+ assert rest_mock.last_request.headers["Authorization"] ==
expected_auth_header
+
+
+def test_rest_catalog_with_custom_auth_type() -> None:
+ # Given
+ catalog_properties = {
+ "uri": TEST_URI,
+ "auth": {
+ "type": "custom",
+ "impl": "dummy.nonexistent.package",
+ "custom": {
+ "property1": "one",
+ "property2": "two",
+ },
+ },
+ }
+ with pytest.raises(ValueError) as e:
+ # Missing namespace
+ RestCatalog("rest", **catalog_properties) # type: ignore
+ assert "Could not load AuthManager class for 'dummy.nonexistent.package'"
in str(e.value)
+
+
+def test_rest_catalog_with_custom_basic_auth_type(rest_mock: Mocker) -> None:
+ # Given
+ catalog_properties = {
+ "uri": TEST_URI,
+ "auth": {
+ "type": "custom",
+ "impl": "pyiceberg.catalog.rest.auth.BasicAuthManager",
+ "custom": {
+ "username": "one",
+ "password": "two",
+ },
+ },
+ }
+ rest_mock.get(
+ f"{TEST_URI}v1/config",
+ json={"defaults": {}, "overrides": {}},
+ status_code=200,
+ )
+ catalog = RestCatalog("rest", **catalog_properties) # type: ignore
+ assert catalog.uri == TEST_URI
+
+ encoded_user_pass = base64.b64encode(b"one:two").decode()
+ expected_auth_header = f"Basic {encoded_user_pass}"
+ assert rest_mock.last_request.headers["Authorization"] ==
expected_auth_header
+
+
+def test_rest_catalog_with_custom_auth_type_no_impl() -> None:
+ # Given
+ catalog_properties = {
+ "uri": TEST_URI,
+ "auth": {
+ "type": "custom",
+ "custom": {
+ "property1": "one",
+ "property2": "two",
+ },
+ },
+ }
+ with pytest.raises(ValueError) as e:
+ # Missing namespace
+ RestCatalog("rest", **catalog_properties) # type: ignore
+ assert "auth.impl must be specified when using custom auth.type" in
str(e.value)
+
+
+def test_rest_catalog_with_non_custom_auth_type_impl() -> None:
+ # Given
+ catalog_properties = {
+ "uri": TEST_URI,
+ "auth": {
+ "type": "basic",
+ "impl": "basic.package",
+ "basic": {
+ "username": "one",
+ "password": "two",
+ },
+ },
+ }
+ with pytest.raises(ValueError) as e:
+ # Missing namespace
Review Comment:
The comment 'Missing namespace' is misleading. The test is actually checking
for an invalid impl property usage, not a missing namespace.
```suggestion
# Invalid impl property usage in non-custom auth.type
```
##########
pyiceberg/catalog/rest/__init__.py:
##########
@@ -247,7 +249,23 @@ def _create_session(self) -> Session:
elif ssl_client_cert := ssl_client.get(CERT):
session.cert = ssl_client_cert
- session.auth =
AuthManagerAdapter(self._create_legacy_oauth2_auth_manager(session))
+ if auth_config := self.properties.get(AUTH):
+ auth_type = auth_config.get("type")
+ if auth_type is None:
+ raise ValueError("auth.type must be defined")
+ auth_type_config = auth_config.get(auth_type, {})
+ auth_impl = auth_config.get("impl")
+
+ if auth_type is CUSTOM and not auth_impl:
+ raise ValueError("auth.impl must be specified when using
custom auth.type")
+
+ if auth_type is not CUSTOM and auth_impl:
Review Comment:
The comparison `auth_type is not CUSTOM` uses identity comparison with a
string literal. This should use inequality comparison (`!=`) instead since
`auth_type` comes from dictionary input and may not be the same string object.
```suggestion
if auth_type != CUSTOM and auth_impl:
```
##########
tests/catalog/test_rest.py:
##########
@@ -1519,6 +1520,132 @@ def test_request_session_with_ssl_client_cert() -> None:
assert "Could not find the TLS certificate file, invalid path:
path_to_client_cert" in str(e.value)
+def test_rest_catalog_with_basic_auth_type(rest_mock: Mocker) -> None:
+ # Given
+ rest_mock.get(
+ f"{TEST_URI}v1/config",
+ json={"defaults": {}, "overrides": {}},
+ status_code=200,
+ )
+ # Given
+ catalog_properties = {
+ "uri": TEST_URI,
+ "auth": {
+ "type": "basic",
+ "basic": {
+ "username": "one",
+ "password": "two",
+ },
+ },
+ }
+ catalog = RestCatalog("rest", **catalog_properties) # type: ignore
+ assert catalog.uri == TEST_URI
+
+ encoded_user_pass = base64.b64encode(b"one:two").decode()
+ expected_auth_header = f"Basic {encoded_user_pass}"
+ assert rest_mock.last_request.headers["Authorization"] ==
expected_auth_header
+
+
+def test_rest_catalog_with_custom_auth_type() -> None:
+ # Given
+ catalog_properties = {
+ "uri": TEST_URI,
+ "auth": {
+ "type": "custom",
+ "impl": "dummy.nonexistent.package",
+ "custom": {
+ "property1": "one",
+ "property2": "two",
+ },
+ },
+ }
+ with pytest.raises(ValueError) as e:
+ # Missing namespace
+ RestCatalog("rest", **catalog_properties) # type: ignore
+ assert "Could not load AuthManager class for 'dummy.nonexistent.package'"
in str(e.value)
+
+
+def test_rest_catalog_with_custom_basic_auth_type(rest_mock: Mocker) -> None:
+ # Given
+ catalog_properties = {
+ "uri": TEST_URI,
+ "auth": {
+ "type": "custom",
+ "impl": "pyiceberg.catalog.rest.auth.BasicAuthManager",
+ "custom": {
+ "username": "one",
+ "password": "two",
+ },
+ },
+ }
+ rest_mock.get(
+ f"{TEST_URI}v1/config",
+ json={"defaults": {}, "overrides": {}},
+ status_code=200,
+ )
+ catalog = RestCatalog("rest", **catalog_properties) # type: ignore
+ assert catalog.uri == TEST_URI
+
+ encoded_user_pass = base64.b64encode(b"one:two").decode()
+ expected_auth_header = f"Basic {encoded_user_pass}"
+ assert rest_mock.last_request.headers["Authorization"] ==
expected_auth_header
+
+
+def test_rest_catalog_with_custom_auth_type_no_impl() -> None:
+ # Given
+ catalog_properties = {
+ "uri": TEST_URI,
+ "auth": {
+ "type": "custom",
+ "custom": {
+ "property1": "one",
+ "property2": "two",
+ },
+ },
+ }
+ with pytest.raises(ValueError) as e:
+ # Missing namespace
Review Comment:
The comment 'Missing namespace' is misleading. The test is actually checking
for a missing impl property when using custom auth type, not a missing
namespace.
```suggestion
# Missing 'impl' property in custom auth configuration
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
