Add authenticated IPFS RPC connections to IPFSStore #35
Conversation
WalkthroughThis pull request enhances the Changes
Possibly related PRs
Suggested reviewers
Poem
✨ Finishing Touches
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #35 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 4 4
Lines 447 461 +14
=========================================
+ Hits 447 461 +14 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
py_hamt/store.py(3 hunks)
🧰 Additional context used
🪛 GitHub Actions: Triggered on push from TheGreatAlgo to branch/tag add-authentication
py_hamt/store.py
[error] 124-126: Coverage failure: total of 99 is less than fail-under=100. Missing coverage on lines 124 and 126.
🔇 Additional comments (4)
py_hamt/store.py (4)
79-84: Authentication parameters well-structured and documentedThe addition of multiple authentication options (basic auth, bearer token, API key) with clear type hints and defaults is a good approach. This provides flexibility for users to choose the authentication method that best suits their IPFS API setup.
108-116: Well-documented authentication attributesThe authentication-related attributes are properly initialized and documented with helpful docstrings that explain their purpose.
144-155: Authentication integration looks goodThe updates to the
savemethod correctly integrate the new authentication capabilities by using the headers from_get_request_headers()and the basic authentication when available. This implementation properly separates the URL building from the request execution.
124-126:❓ Verification inconclusive
Missing test coverage for authentication logic
The CI pipeline shows that lines 124 and 126 lack test coverage, bringing the total coverage to 99% (below the required 100%). These lines handle the bearer token and API key authentication logic.
You should add test cases that cover both authentication methods:
- Testing with a bearer token
- Testing with an API key
🏁 Script executed:
#!/bin/bash # Search for existing tests for IPFSStore to see what needs to be extended rg -A 5 "test.*IPFSStore" --type=pythonLength of output: 72
Authentication Test Coverage Missing – Please Add Necessary Tests
The CI pipeline indicates that the authentication logic in
py_hamt/store.py(lines 124–126) isn’t exercised by any tests. In your current setup, the initial search for tests using:rg -A 5 "test.*IPFSStore" --type=pythonproduced an error (“unrecognized file type: python”). This suggests that either the grep parameters need adjustment or — more critically — that there are no test cases covering this functionality.
Action Items:
- Add test cases for both branches of the authentication logic:
- Bearer Token Authentication: Ensure that when a bearer token is provided, the header
"Authorization": "Bearer <token>"is correctly set.- API Key Authentication: Verify that when an API key is provided, the header defined by
self.api_key_headeris properly assigned.- Revise the test search command if needed (for example, using a glob pattern such as below) to validate the existence of tests in your repository:
rg -g "*.py" -A5 "IPFSStore"Please confirm that tests for both methods are added and ensure they are executed in the CI pipeline to boost coverage to 100%.
🧰 Tools
🪛 GitHub Actions: Triggered on push from TheGreatAlgo to branch/tag add-authentication
[error] 124-126: Coverage failure: total of 99 is less than fail-under=100. Missing coverage on lines 124 and 126.
There was a problem hiding this comment.
PR looks good so far, small and functional, the best qualities for a PR.
The tests are currently failing since this does not include code coverage and so does not rigorously test the functionality provided. A test file should be created that creates a temporary authenticated gateway and then creates an IPFSStore that can talk to that authenticated gateway and then tests some ordinary HAMT key setting and retrieval functionality.
py_hamt/store.py
Outdated
| def _get_request_headers(self) -> dict[str, str]: | ||
| """Helper method to construct authentication headers""" | ||
| headers = {} | ||
|
|
||
| # Apply authentication based on provided credentials | ||
| if self.bearer_token: | ||
| headers["Authorization"] = f"Bearer {self.bearer_token}" | ||
| elif self.api_key: | ||
| headers[self.api_key_header] = self.api_key | ||
| return headers |
There was a problem hiding this comment.
This method is not used elsewhere, nor does it abstract away any complex functionality, for code style I would prefer it including right next to the headers variable initialization in line 147
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
tests/test_zarr_ipfs.py (3)
142-180: Consider reducing code duplication with test_encryptionThis new test function duplicates most of the logic from
test_encryptionwith the only difference being the authentication parameters. Consider refactoring to reduce duplication, perhaps by creating a helper function that takes authentication parameters.-def test_authenticated_gateway(random_zarr_dataset: tuple[str, xr.Dataset]): - zarr_path, expected_ds = random_zarr_dataset - test_ds = xr.open_zarr(zarr_path) - - with pytest.raises(ValueError, match="Encryption key is not 32 bytes"): - create_zarr_encryption_transformers(bytes(), bytes()) - - encryption_key = bytes(32) - # Encrypt only precipitation, not temperature - encrypt, decrypt = create_zarr_encryption_transformers( - encryption_key, header="sample-header".encode(), exclude_vars=["temp"] - ) - hamt = HAMT( - store=IPFSStore( - api_key="test", - bearer_token="Test", - basic_auth=("test", "test") - ), transformer_encode=encrypt, transformer_decode=decrypt - ) - test_ds.to_zarr(store=hamt, mode="w") - - hamt.make_read_only() - loaded_ds = xr.open_zarr(store=hamt) - xr.testing.assert_identical(loaded_ds, expected_ds) - - # Now trying to load without a decryptor, xarray should be able to read the metadata and still perform operations on the unencrypted variable - print("Attempting to read and print metadata of partially encrypted zarr") - ds = xr.open_zarr( - store=HAMT(store=IPFSStore( - api_key="test", - bearer_token="Test", - basic_auth=("test", "test") - ), root_node_id=hamt.root_node_id, read_only=True) - ) - print(ds) - assert ds.temp.sum() == expected_ds.temp.sum() - # We should be unable to read precipitation values which are still encrypted - with pytest.raises(Exception): - ds.precip.sum() +def _test_encryption_with_store(random_zarr_dataset: tuple[str, xr.Dataset], store_params: dict = None): + """Helper function to test encryption with different store configurations""" + zarr_path, expected_ds = random_zarr_dataset + test_ds = xr.open_zarr(zarr_path) + + with pytest.raises(ValueError, match="Encryption key is not 32 bytes"): + create_zarr_encryption_transformers(bytes(), bytes()) + + encryption_key = bytes(32) + # Encrypt only precipitation, not temperature + encrypt, decrypt = create_zarr_encryption_transformers( + encryption_key, header="sample-header".encode(), exclude_vars=["temp"] + ) + + store_params = store_params or {} + store = IPFSStore(**store_params) + + hamt = HAMT( + store=store, transformer_encode=encrypt, transformer_decode=decrypt + ) + test_ds.to_zarr(store=hamt, mode="w") + + hamt.make_read_only() + loaded_ds = xr.open_zarr(store=hamt) + xr.testing.assert_identical(loaded_ds, expected_ds) + + # Now trying to load without a decryptor, xarray should be able to read the metadata and still perform operations on the unencrypted variable + print("Attempting to read and print metadata of partially encrypted zarr") + read_store = IPFSStore(**store_params) + ds = xr.open_zarr( + store=HAMT(store=read_store, root_node_id=hamt.root_node_id, read_only=True) + ) + print(ds) + assert ds.temp.sum() == expected_ds.temp.sum() + # We should be unable to read precipitation values which are still encrypted + with pytest.raises(Exception): + ds.precip.sum() + +def test_encryption(random_zarr_dataset: tuple[str, xr.Dataset]): + """Test encryption without authentication""" + _test_encryption_with_store(random_zarr_dataset) + +def test_authenticated_gateway(random_zarr_dataset: tuple[str, xr.Dataset]): + """Test encryption with authentication""" + auth_params = { + "api_key": "test", + "bearer_token": "Test", + "basic_auth": ("test", "test") + } + _test_encryption_with_store(random_zarr_dataset, auth_params)🧰 Tools
🪛 Ruff (0.8.2)
179-179:
pytest.raises(Exception)should be considered evil(B017)
155-159: Test authentication methods separatelyCurrently, you're testing all authentication methods (API key, bearer token, and basic auth) simultaneously. This doesn't verify that each method works independently. Consider adding separate tests for each authentication method to ensure comprehensive coverage.
def test_api_key_authentication(random_zarr_dataset): auth_params = {"api_key": "test"} _test_encryption_with_store(random_zarr_dataset, auth_params) def test_bearer_token_authentication(random_zarr_dataset): auth_params = {"bearer_token": "Test"} _test_encryption_with_store(random_zarr_dataset, auth_params) def test_basic_auth_authentication(random_zarr_dataset): auth_params = {"basic_auth": ("test", "test")} _test_encryption_with_store(random_zarr_dataset, auth_params)
179-180: Use a more specific exception typeUsing
pytest.raises(Exception)is considered too broad as it will catch any exception, not just the specific one you're expecting. This could mask bugs where the wrong type of exception is raised.- with pytest.raises(Exception): - ds.precip.sum() + # Replace 'Exception' with the specific exception type that's expected, e.g.: + with pytest.raises((ValueError, RuntimeError)): + ds.precip.sum()🧰 Tools
🪛 Ruff (0.8.2)
179-179:
pytest.raises(Exception)should be considered evil(B017)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
py_hamt/store.py(4 hunks)tests/test_zarr_ipfs.py(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- py_hamt/store.py
🧰 Additional context used
🪛 Ruff (0.8.2)
tests/test_zarr_ipfs.py
179-179: pytest.raises(Exception) should be considered evil
(B017)
🔇 Additional comments (1)
tests/test_zarr_ipfs.py (1)
155-174:❓ Verification inconclusive
Address uncovered lines in store.py
According to the PR objectives, there are 2 uncovered lines in
py_hamt/store.py. Consider adding tests that specifically target these uncovered code paths to improve the coverage back to 100%.
🏁 Script executed:
#!/bin/bash # Script to identify the uncovered lines in store.py # and analyze what authentication scenarios might be missing # Check if we can locate the coverage report fd -t f -e xml "coverage" || echo "No coverage report found" # Look at the store.py file to understand the authentication logic cat py_hamt/store.py | grep -A 30 "def _get_request_headers" # Check for potential conditional branches in authentication logic ast-grep --pattern 'if $condition { $$$body }' py_hamt/store.pyLength of output: 154
Attention: Uncovered authentication branches in store.py – Manual verification needed
The current investigation did not produce output showing the specific uncovered lines inpy_hamt/store.py(notably within the_get_request_headersfunction). Could you please manually verify the following?
- Extract and review the entire
_get_request_headersfunction to pinpoint the two uncovered conditional branches in the authentication logic.- Confirm whether both the token and basic authentication scenarios have corresponding tests. If not, please add tests to cover these paths and bring the coverage to 100%.
For your inspection, please run these additional shell scripts:
Once you’ve confirmed the uncovered paths and updated the tests accordingly, we can revisit this review item.
🏁 Script executed:
#!/bin/bash # Extract the _get_request_headers function from py_hamt/store.py for manual review sed -n '/def _get_request_headers/,/^def /p' py_hamt/store.py # Additionally, output the conditional branches within that function (if/else statements) sed -n '/def _get_request_headers/,/^def /p' py_hamt/store.py | grep -E 'if |else'Length of output: 148
Action Required: Manual Verification of Uncovered Code Paths in
store.py
The initial automated checks did not reveal the contents of the_get_request_headersfunction or its conditional branches. Please manually verify the following inpy_hamt/store.py:
- Confirm that the
_get_request_headersfunction exists and check its implementation for conditional branches (e.g., branches handling token authentication vs. basic authentication).- Identify the two uncovered lines mentioned in the report and ensure that tests cover both scenarios.
- If either authentication scenario is missing corresponding tests, add tests to cover these branches and restore full (100%) coverage.
To aid with this verification, please execute the following additional shell script to try to extract the function and its conditionals:
#!/bin/bash # Verify the presence and content of the _get_request_headers function in py_hamt/store.py # Locate the function definition rg "def\s+_get_request_headers" py_hamt/store.py # Extract the function content (from its definition until an empty line or next function definition) sed -n '/def _get_request_headers/,/^\s*$/p' py_hamt/store.py # Optionally, highlight conditional statements within that function to inspect the authentication branches sed -n '/def _get_request_headers/,/^\s*$/p' py_hamt/store.py | grep -E 'if |else'After reviewing and ensuring that both authentication scenarios are properly tested, update the tests to cover any uncovered paths as needed.
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (2)
tests/test_zarr_ipfs.py (2)
142-179: New test for authenticated gateway has code duplicationThis new test function largely duplicates the logic from
test_encryption, making the same assertions and following the same structure, but with added authentication parameters. Consider refactoring to reduce duplication, possibly using pytest's parameterization.@pytest.mark.parametrize("auth_params", [ {}, # No authentication { "api_key": "test", "bearer_token": "Test", "basic_auth": ("test", "test") } # With authentication ]) def test_encryption_with_options(random_zarr_dataset: tuple[str, xr.Dataset], auth_params): zarr_path, expected_ds = random_zarr_dataset test_ds = xr.open_zarr(zarr_path) with pytest.raises(ValueError, match="Encryption key is not 32 bytes"): create_zarr_encryption_transformers(bytes(), bytes()) encryption_key = bytes(32) # Encrypt only precipitation, not temperature encrypt, decrypt = create_zarr_encryption_transformers( encryption_key, header="sample-header".encode(), exclude_vars=["temp"] ) hamt = HAMT( store=IPFSStore(**auth_params), transformer_encode=encrypt, transformer_decode=decrypt ) test_ds.to_zarr(store=hamt, mode="w") hamt.make_read_only() loaded_ds = xr.open_zarr(store=hamt) xr.testing.assert_identical(loaded_ds, expected_ds) # Now trying to load without a decryptor, xarray should be able to read the metadata and still perform operations on the unencrypted variable print("Attempting to read and print metadata of partially encrypted zarr") ds = xr.open_zarr( store=HAMT(store=IPFSStore(**auth_params), root_node_id=hamt.root_node_id, read_only=True) ) print(ds) assert ds.temp.sum() == expected_ds.temp.sum() # We should be unable to read precipitation values which are still encrypted with pytest.raises(Exception): ds.precip.sum()🧰 Tools
🪛 Ruff (0.8.2)
178-178:
pytest.raises(Exception)should be considered evil(B017)
155-159: Consider testing each authentication method separatelyThe test is using all authentication methods (API key, bearer token, basic auth) simultaneously, which doesn't validate that each method works independently. Consider testing each authentication method separately to ensure proper implementation.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
tests/test_zarr_ipfs.py(1 hunks)
🧰 Additional context used
🪛 Ruff (0.8.2)
tests/test_zarr_ipfs.py
178-178: pytest.raises(Exception) should be considered evil
(B017)
🔇 Additional comments (1)
tests/test_zarr_ipfs.py (1)
142-179: Missing verification for authentication functionalityThe test doesn't verify that authentication is actually working. Consider adding assertions to validate that requests are made with proper headers or test with invalid credentials to ensure authentication failures are handled correctly.
Would you like to add a test case with invalid credentials to verify that authentication failure is handled properly?
🧰 Tools
🪛 Ruff (0.8.2)
178-178:
pytest.raises(Exception)should be considered evil(B017)
tests/test_zarr_ipfs.py
Outdated
| store=HAMT(store=IPFSStore( | ||
| api_key="test", | ||
| basic_auth=("test", "test") | ||
| ), root_node_id=hamt.root_node_id, read_only=True) |
There was a problem hiding this comment.
Authentication parameters inconsistency
The authentication parameters used for reading (lines 170-173) differ from those used for writing (lines 155-159). The read operation omits the bearer_token. This inconsistency may lead to test failures if all authentication parameters are required.
ds = xr.open_zarr(
store=HAMT(store=IPFSStore(
api_key="test",
+ bearer_token="Test",
basic_auth=("test", "test")
), root_node_id=hamt.root_node_id, read_only=True)
)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| store=HAMT(store=IPFSStore( | |
| api_key="test", | |
| basic_auth=("test", "test") | |
| ), root_node_id=hamt.root_node_id, read_only=True) | |
| ds = xr.open_zarr( | |
| store=HAMT(store=IPFSStore( | |
| api_key="test", | |
| bearer_token="Test", | |
| basic_auth=("test", "test") | |
| ), root_node_id=hamt.root_node_id, read_only=True) | |
| ) |
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
tests/test_zarr_ipfs.py (1)
154-160: 🛠️ Refactor suggestionTest authentication methods separately
To ensure proper test coverage of the authentication functionality, test each authentication method separately rather than combining them. This helps isolate which authentication method is being used and confirms each works independently.
hamt = HAMT( store=IPFSStore( - api_key="test", - bearer_token="Test", - basic_auth=("test", "test") + bearer_token="Test" # Test only bearer token first ), transformer_encode=encrypt, transformer_decode=decrypt )
🧹 Nitpick comments (3)
tests/test_zarr_ipfs.py (3)
181-183: Authentication parameters inconsistencyWhen reading the data back, no authentication parameters are provided to IPFSStore, but authentication parameters were used when writing the data. For comprehensive testing, consider using the same authentication parameters for both write and read operations.
ds = xr.open_zarr( - store=HAMT(store=IPFSStore(), root_node_id=hamt.root_node_id, read_only=True) + store=HAMT(store=IPFSStore(api_key="test"), root_node_id=hamt.root_node_id, read_only=True) )
142-188: Reduce code duplication with test_encryptionThe
test_authenticated_gatewayfunction duplicates much of the code fromtest_encryption. Consider refactoring to extract common testing logic into helper functions to improve maintainability.def _test_encryption_with_store(random_zarr_dataset, store_factory): """Helper function to test encryption with a given store factory function""" zarr_path, expected_ds = random_zarr_dataset test_ds = xr.open_zarr(zarr_path) encryption_key = bytes(32) encrypt, decrypt = create_zarr_encryption_transformers( encryption_key, header="sample-header".encode(), exclude_vars=["temp"] ) hamt = HAMT(store=store_factory(), transformer_encode=encrypt, transformer_decode=decrypt) # Test write and read operations test_ds.to_zarr(store=hamt, mode="w") hamt.make_read_only() loaded_ds = xr.open_zarr(store=hamt) xr.testing.assert_identical(loaded_ds, expected_ds) # Test without decryptor ds = xr.open_zarr( store=HAMT(store=store_factory(no_auth=True), root_node_id=hamt.root_node_id, read_only=True) ) assert ds.temp.sum() == expected_ds.temp.sum() with pytest.raises(ValueError): # Specify expected exception ds.precip.sum()🧰 Tools
🪛 Ruff (0.8.2)
187-187:
pytest.raises(Exception)should be considered evil(B017)
142-188:❓ Verification inconclusive
Add targeted tests for authentication mechanisms
The new
test_authenticated_gatewayfunction is a good addition for testing authenticated operations, but it doesn't explicitly verify the core authentication functionality. The PR shows a coverage gap inpy_hamt/store.py(99.78% coverage with 1 line uncovered).Consider adding more targeted test assertions to verify that the authentication parameters are being correctly used to generate request headers. This could involve mocking the request layer or adding specific tests for each authentication method.
🏁 Script executed:
#!/bin/bash # Check for uncovered lines in store.py related to authentication fd -e py "store\.py$" | xargs grep -n -A 5 -B 5 "_get_request_headers"Length of output: 70
Action: Request Manual Verification for Authentication Header Tests
The current automated check did not locate any instances of
_get_request_headersinpy_hamt/store.py. This raises uncertainty about the targeted authentication logic mentioned in the review comment. Before finalizing the test improvements, please manually verify the following:
- Confirm whether a function (such as
_get_request_headersor an equivalent mechanism) exists inpy_hamt/store.pythat handles the generation of authentication request headers.- If such a function exists, add tests that explicitly validate the construction of authentication headers (e.g., by mocking the request layer to check for the correct inclusion of API key, bearer token, and basic auth).
- If the authentication header creation is implemented differently, adjust test assertions accordingly to ensure comprehensive coverage.
🧰 Tools
🪛 Ruff (0.8.2)
187-187:
pytest.raises(Exception)should be considered evil(B017)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
tests/test_zarr_ipfs.py(1 hunks)
🧰 Additional context used
🪛 Ruff (0.8.2)
tests/test_zarr_ipfs.py
187-187: pytest.raises(Exception) should be considered evil
(B017)
🪛 GitHub Actions: Triggered on push from TheGreatAlgo to branch/tag add-authentication
tests/test_zarr_ipfs.py
[error] 1-1: Ruff formatting check failed. The file would be reformatted.
🔇 Additional comments (1)
tests/test_zarr_ipfs.py (1)
187-188: Specify the expected exception typeUsing
pytest.raises(Exception)is too broad and may catch unexpected exceptions. Specify the exact exception type that should be raised when attempting to access encrypted data.- with pytest.raises(Exception): + with pytest.raises((ValueError, KeyError, RuntimeError)): # Replace with actual exception types ds.precip.sum()🧰 Tools
🪛 Ruff (0.8.2)
187-187:
pytest.raises(Exception)should be considered evil(B017)
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
tests/test_zarr_ipfs.py (1)
178-188: 🛠️ Refactor suggestionSpecify the expected exception type instead of using
pytest.raises(Exception)Using
pytest.raises(Exception)is too broad and may catch unexpected exceptions. This is also flagged by the static analysis tool.Specify the exact exception type that should be raised when attempting to access encrypted data:
-with pytest.raises(Exception): +with pytest.raises((ValueError, KeyError, RuntimeError)): # Replace with actual exception types ds.precip.sum()🧰 Tools
🪛 Ruff (0.8.2)
187-187:
pytest.raises(Exception)should be considered evil(B017)
🧹 Nitpick comments (5)
.github/workflows/run-checks.yaml (1)
28-42: Authentication setup looks good but consider adding a small comment about restartsThe GitHub workflow now correctly configures multiple authentication methods for IPFS, which is essential for testing the new authentication features. The implementation sets up Bearer token, API key, and Basic authentication with test credentials.
Consider adding a comment to clarify whether the IPFS daemon needs to be restarted after these configuration changes, as this might be useful information for developers running these commands locally:
# Basic auth with test:test echo "test:test" > ~/.ipfs/credentials BASIC_AUTH_BASE64=$(base64 ~/.ipfs/credentials | tr -d '\n') ipfs config --json Gateway.HTTPHeaders.Basic-Auth "{\"Authorization\": \"Basic $BASIC_AUTH_BASE64\"}" +# Note: These changes take effect immediately without needing to restart the IPFS daemontests/test_zarr_ipfs.py (1)
142-177: Authentication test setup looks good but inconsistent auth methods in testsThe new test for authenticated gateways effectively tests the authentication functionality. However, the test only verifies a subset of authentication combinations.
Consider testing each authentication method independently to ensure they all work correctly on their own:
# Test with just api key hamt = HAMT( store=IPFSStore(api_key="test"), transformer_encode=encrypt, transformer_decode=decrypt, ) test_ds.to_zarr(store=hamt, mode="w") hamt.make_read_only() loaded_ds = xr.open_zarr(store=hamt) xr.testing.assert_identical(loaded_ds, expected_ds) + +# Test with just bearer token +hamt = HAMT( + store=IPFSStore(bearer_token="Test"), + transformer_encode=encrypt, + transformer_decode=decrypt, +) +test_ds.to_zarr(store=hamt, mode="w") + +hamt.make_read_only() +loaded_ds = xr.open_zarr(store=hamt) +xr.testing.assert_identical(loaded_ds, expected_ds) + +# Test with just basic auth +hamt = HAMT( + store=IPFSStore(basic_auth=("test", "test")), + transformer_encode=encrypt, + transformer_decode=decrypt, +) +test_ds.to_zarr(store=hamt, mode="w") + +hamt.make_read_only() +loaded_ds = xr.open_zarr(store=hamt) +xr.testing.assert_identical(loaded_ds, expected_ds)py_hamt/store.py (3)
139-149: Authentication implementation looks solid but consider using the load method tooThe authentication implementation in the
savemethod looks solid. However, theloadmethod doesn't use the authentication credentials when retrieving data from the gateway.Consider applying the same authentication logic to the
loadmethod to ensure consistent authentication behavior:def load(self, id: CID) -> bytes: # type: ignore """ This retrieves the raw bytes by calling the provided HTTP gateway. ```python from py_hamt import IPFSStore from multiformats import CID ipfs_store = IPFSStore() # This is just an example CID cid = CID.decode("bafybeiaysi4s6lnjev27ln5icwm6tueaw2vdykrtjkwiphwekaywqhcjze") data = ipfs_store.load(cid) print(data) ``` """ + # Apply authentication based on provided credentials + headers = {} + if self.bearer_token: + headers["Authorization"] = f"Bearer {self.bearer_token}" + elif self.api_key: + headers[self.api_key_header] = self.api_key + + # Prepare request parameters + auth = self.basic_auth if self.basic_auth else None + response = requests.get( - f"{self.gateway_uri_stem}/ipfs/{str(id)}", timeout=self.timeout_seconds + f"{self.gateway_uri_stem}/ipfs/{str(id)}", + headers=headers, + auth=auth, + timeout=self.timeout_seconds ) response.raise_for_status() if self.debug: self.total_received += len(response.content) # type: ignore return response.content
139-145: Consider extracting authentication header logic into a helper methodThe authentication header logic is only applied in the
savemethod. If it's used in other methods (like theloadmethod as suggested), it would be better to extract it into a helper method.Extract the authentication header logic into a helper method:
+def _get_request_headers(self) -> dict[str, str]: + """Helper method to construct authentication headers""" + headers = {} + if self.bearer_token: + headers["Authorization"] = f"Bearer {self.bearer_token}" + elif self.api_key: + headers[self.api_key_header] = self.api_key + return headers def save(self, data: bytes, cid_codec: str) -> CID: # ... pin_string: str = "true" if self.pin_on_add else "false" - # Apply authentication based on provided credentials - headers = {} - if self.bearer_token: - headers["Authorization"] = f"Bearer {self.bearer_token}" - elif self.api_key: - headers[self.api_key_header] = self.api_key + # Get authentication headers + headers = self._get_request_headers() # Prepare request parameters url = f"{self.rpc_uri_stem}/api/v0/add?hash={self.hasher}&pin={pin_string}" auth = self.basic_auth if self.basic_auth else None
141-144: Consider handling the case when both bearer_token and api_key are providedThe current implementation uses the bearer token only if it's present, and falls back to the API key. It doesn't handle the case when both are provided, which might be confusing.
Add a comment to clarify the precedence when both authentication methods are provided:
# Apply authentication based on provided credentials headers = {} if self.bearer_token: + # Bearer token takes precedence over API key if both are provided headers["Authorization"] = f"Bearer {self.bearer_token}" elif self.api_key: headers[self.api_key_header] = self.api_key
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
.github/workflows/run-checks.yaml(1 hunks)py_hamt/store.py(4 hunks)tests/test_zarr_ipfs.py(1 hunks)
🧰 Additional context used
🪛 Ruff (0.8.2)
tests/test_zarr_ipfs.py
187-187: pytest.raises(Exception) should be considered evil
(B017)
🔇 Additional comments (3)
tests/test_zarr_ipfs.py (1)
181-183:❓ Verification inconclusive
Authentication parameters inconsistency when reading
The code instantiates
IPFSStorefor reading without any authentication parameters, which differs from the write operation.If the IPFS gateway requires authentication for reading as well, this might lead to test failures. Consider modifying the code to include authentication parameters when reading:
ds = xr.open_zarr( - store=HAMT(store=IPFSStore(), root_node_id=hamt.root_node_id, read_only=True) + store=HAMT(store=IPFSStore(api_key="test"), root_node_id=hamt.root_node_id, read_only=True) )Let's verify if the Gateway is configured to require authentication for reads:
🏁 Script executed:
#!/bin/bash # Check if the Gateway is configured to require authentication for reads ipfs config show | grep -A 10 Gateway.HTTPHeadersLength of output: 146
Attention: Manual Verification Needed for IPFS Authentication on Read
The current test instantiates
IPFSStorefor reading without authentication parameters, which is inconsistent with the write operation. Our attempt to verify the gateway configuration usingipfs config showfailed due to the command not being found. This prevents automated confirmation of whether the IPFS gateway requires authentication for reads.
- Action Required: Please manually verify if the IPFS gateway requires authentication when reading. If authentication is indeed required, update the instantiation accordingly. For example:
ds = xr.open_zarr( - store=HAMT(store=IPFSStore(), root_node_id=hamt.root_node_id, read_only=True) + store=HAMT(store=IPFSStore(api_key="test"), root_node_id=hamt.root_node_id, read_only=True) )py_hamt/store.py (2)
65-74: Authentication docstring is thorough and well explainedThe updated class docstring clearly explains the authentication features that have been added. This is a good improvement to the documentation.
84-122: Well-structured authentication parameters with clear documentationThe authentication parameters are well-structured and have clear documentation explaining their purpose. Good job on providing a customizable API key header option.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
tests/test_zarr_ipfs.py (1)
197-198:⚠️ Potential issueSpecify the expected exception type
Using
pytest.raises(Exception)is too broad and may catch unexpected exceptions. Specify the exact exception type that should be raised when attempting to save with an invalid API key.- with pytest.raises(Exception): + with pytest.raises(requests.exceptions.HTTPError, match="401 Client Error: Unauthorized"): test_ds.to_zarr(store=hamt, mode="w")This change will help ensure that the test is specifically catching HTTP 401 errors from the Nginx auth configuration, which is what we expect when using an invalid API key.
🧰 Tools
🪛 Ruff (0.8.2)
197-197:
pytest.raises(Exception)should be considered evil(B017)
🧹 Nitpick comments (2)
.github/workflows/run-checks.yaml (2)
29-63: Great addition of Nginx reverse proxy with auth!The configuration correctly sets up a reverse proxy on port 5002 with API key authentication using the
X-API-Keyheader. This aligns well with the authentication features being added to theIPFSStoreclass.One minor issue to fix:
- sudo nginx -t + sudo nginx -tRemove the trailing whitespace at line 63.
🧰 Tools
🪛 YAMLlint (1.35.1)
[error] 63-63: trailing spaces
(trailing-spaces)
64-74: Ensure testing of all authentication methodsThe Nginx setup is configured to test API key authentication, but the PR mentions supporting basic authentication and bearer token as well. Consider extending this configuration to test all authentication methods being added to the
IPFSStoreclass.You could add additional location blocks in the Nginx configuration to test different auth methods, like:
+ location /api/v0/basic/ { + # Basic Auth + auth_basic "Restricted Area"; + auth_basic_user_file /etc/nginx/.htpasswd; + + # Proxy to IPFS RPC API + proxy_pass http://127.0.0.1:5001/api/v0/; + proxy_set_header Host $host; + proxy_set_header X-Real-IP $remote_addr; + } + + location /api/v0/bearer/ { + # Bearer token auth + if ($http_authorization != "Bearer test") { + return 401 "Unauthorized: Invalid bearer token"; + } + + # Proxy to IPFS RPC API + proxy_pass http://127.0.0.1:5001/api/v0/; + proxy_set_header Host $host; + proxy_set_header X-Real-IP $remote_addr; + }And include a command to create the basic auth credentials:
sudo sh -c "echo -n 'test:' > /etc/nginx/.htpasswd" sudo sh -c "openssl passwd -apr1 test >> /etc/nginx/.htpasswd"
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
.github/workflows/run-checks.yaml(1 hunks)tests/test_zarr_ipfs.py(1 hunks)
🧰 Additional context used
🪛 YAMLlint (1.35.1)
.github/workflows/run-checks.yaml
[error] 63-63: trailing spaces
(trailing-spaces)
🪛 Ruff (0.8.2)
tests/test_zarr_ipfs.py
197-197: pytest.raises(Exception) should be considered evil
(B017)
208-208: pytest.raises(Exception) should be considered evil
(B017)
🪛 GitHub Actions: Triggered on push from TheGreatAlgo to branch/tag add-authentication
tests/test_zarr_ipfs.py
[error] 189-189: requests.exceptions.HTTPError: 401 Client Error: Unauthorized for url: http://127.0.0.1:5002/api/v0/add?hash=blake3&pin=false
🔇 Additional comments (2)
tests/test_zarr_ipfs.py (2)
142-170: Good test coverage for API key authenticationThis test case effectively validates the
IPFSStorewith API key authentication against the Nginx reverse proxy. The encryption setup and verification are also well-structured.
201-209:✅ Verification successful
Inconsistent authentication parameters
The authentication parameters used for reading (lines 203) differ from those used for writing (lines 190-193). The read operation omits the authentication parameters. This works because you're reading from the local store, but it creates confusing test logic.
Additionally, the exception type for line 208 should be specified more precisely.
# Now trying to load without a decryptor, xarray should be able to read the metadata and still perform operations on the unencrypted variable print("Attempting to read and print metadata of partially encrypted zarr") ds = xr.open_zarr( - store=HAMT(store=IPFSStore(), root_node_id=hamt.root_node_id, read_only=True) + store=HAMT( + store=IPFSStore(rpc_uri_stem="http://127.0.0.1:5002", api_key="test"), + root_node_id=hamt.root_node_id, + read_only=True + ) ) print(ds) assert ds.temp.sum() == expected_ds.temp.sum() # We should be unable to read precipitation values which are still encrypted - with pytest.raises(Exception): + with pytest.raises((ValueError, RuntimeError), match="Unable to decode array"): ds.precip.sum()
🌐 Web query:
What exceptions does xarray raise when trying to access encrypted or corrupt data arrays?💡 Result:
When working with encrypted or corrupt data arrays in Xarray, several exceptions can be raised depending on the underlying file format and the specific issue encountered:
Common Exceptions
OSError/IOError
Occurs when Xarray cannot access the file due to encryption, incorrect permissions, or file corruption. This is raised by low-level I/O libraries like netCDF4 or h5py.
Example:OSError: Unable to open file (truncated file: e.g., corrupt data)ValueError
Triggered during operations like concatenation with inconsistent coordinates or invalid chunks.
- Example from invalid chunk configuration:
([Source][1])ValueError: Expected all values to be non-negative. Got (-1, -1) instead.NetCDF/HDF5 Backend Errors
Errors from libraries likenetCDF4orh5pypropagate through Xarray:
RuntimeErrorfor invalid NetCDF4/HDF5 file structures.HDF5ExtErrorfor corrupted HDF5 files.KeyError
Raised when accessing variables/dimensions that don’t exist in a corrupt dataset.NotImplementedError
Occurs for unsupported operations, such as indexing withmethod="nearest"on slices.
Handling Encrypted Data
Xarray does not natively support encrypted data. Attempting to open encrypted files without decryption will result in OSError or format-specific parsing errors (e.g.,
NetCDF: Unknown file format).
Corrupt Data Scenarios
NetCDF/HDF5 Files
During Read:
ValueError: Failed to decode time variable 't' (CF conventions violation)Caused by invalid metadata or corrupted time encoding.
During Write:
HDF5ExtError: Unable to create dataset (corrupted chunk)Common when writing to partially corrupted Zarr/NetCDF stores ([Example][6]).
Zarr Stores
- Invalid Chunks:
ValueError: Chunk size must be a positive integer- Consolidated Metadata Issues:
KeyError: 'Variable not found in consolidated metadata'
Debugging Tips
- Use
xr.open_dataset(..., engine='h5netcdf')withdecode_cf=Falseto bypass metadata validation.- Check for missing/corrupt chunks using
xr.Dataset.chunks.- For Zarr, verify metadata with
zarr.open_group(..., mode='r').For example, corrupted coordinates might raise:
ValueError: Could not find dimension coordinates to concatenate datasets([Source][4]).
Always handle exceptions with try/except blocks when working with unreliable data sources.
Citations:
- 1: https://docs.xarray.dev/en/stable/user-guide/io.html
- 2: https://docs.xarray.dev/en/stable/generated/xarray.open_dataset.html
- 3: https://xarray.pydata.org/en/v0.9.5/dask.html
- 4: https://forum.earthdata.nasa.gov/viewtopic.php?t=4889
- 5: https://xarray.pydata.org/en/v0.13.0/generated/xarray.open_dataset.html
- 6: HDF error when trying to write Dataset read with rasterio to NetCDF pydata/xarray#2535
- 7: https://discourse.pangeo.io/t/xarray-unable-to-allocate-memory-how-to-size-up-problem/3233
- 8: https://docs.xarray.dev/en/stable/generated/xarray.open_zarr.html
- 9: https://docs.xarray.dev/en/latest/getting-started-guide/faq.html
- 10: https://docs.xarray.dev/en/latest/user-guide/indexing.html
- 11: https://docs.xarray.dev/en/stable/whats-new.html
- 12: https://xarray.pydata.org/en/v0.10.2/dask.html
- 13: https://docs.xarray.dev/en/latest/user-guide/data-structures.html
- 14: Feature Request: Hierarchical storage and processing in xarray pydata/xarray#4118
- 15: https://docs.webmethods.io/on-premises/entirex/en/10.8.0/pdf/mac.pdf
- 16: https://discourse.pangeo.io/t/struggling-with-large-dataset-loading-reading-using-xarray/866
- 17: ValueError when trying to encode time variable in a NetCDF file with CF convensions pydata/xarray#3739
- 18: Lack of resilience towards missing
_ARRAY_DIMENSIONSxarray's special zarr attribute #280 pydata/xarray#8749- 19: https://docs.xarray.dev/en/stable/api.html
- 20: https://docs.coiled.io/blog/xarray-at-scale.html
Ensure consistent authentication and precise exception checking in test
- Update the
HAMTinitialization intests/test_zarr_ipfs.py(line 203) to include the same authentication parameters used during writing (as set in lines 190–193). This avoids confusion by making the test logic consistent.- Replace the broad
pytest.raises(Exception)(line 208) with a more precise check that catchesValueErrorandRuntimeErrorand verifies the error message contains"Unable to decode array". This aligns with Xarray’s behavior when encountering encrypted (or corrupt) data.🧰 Tools
🪛 Ruff (0.8.2)
208-208:
pytest.raises(Exception)should be considered evil(B017)
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
tests/test_zarr_ipfs.py (2)
200-210: 🛠️ Refactor suggestionSpecify the expected exception type.
Using
pytest.raises(Exception)is too broad and may catch unexpected exceptions. Specify the exact exception type that should be raised when authentication fails.- with pytest.raises(Exception): + with pytest.raises((requests.exceptions.HTTPError)): hamt = HAMT( store=IPFSStore( rpc_uri_stem = "http://127.0.0.1:5002", api_key="badKey", ), transformer_encode=encrypt, transformer_decode=decrypt, )Remember to add
import requestsat the top of the file if not already present.🧰 Tools
🪛 Ruff (0.8.2)
201-201:
pytest.raises(Exception)should be considered evil(B017)
211-220: 🛠️ Refactor suggestionSpecify the expected exception type for encrypted data access.
Using
pytest.raises(Exception)is too broad when testing encryption functionality. Specify the exact exception type that should be raised when attempting to access encrypted data.- with pytest.raises(Exception): + with pytest.raises((ValueError, RuntimeError)): # Replace with actual exception type ds.precip.sum()🧰 Tools
🪛 Ruff (0.8.2)
219-219:
pytest.raises(Exception)should be considered evil(B017)
🧹 Nitpick comments (1)
.github/workflows/run-checks.yaml (1)
80-80: Fix trailing whitespace.There is a trailing whitespace at the end of line 80 that should be removed.
- sudo nginx -t + sudo nginx -t🧰 Tools
🪛 YAMLlint (1.35.1)
[error] 80-80: trailing spaces
(trailing-spaces)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
.github/workflows/run-checks.yaml(1 hunks)tests/test_zarr_ipfs.py(1 hunks)
🧰 Additional context used
🪛 YAMLlint (1.35.1)
.github/workflows/run-checks.yaml
[error] 80-80: trailing spaces
(trailing-spaces)
🪛 Ruff (0.8.2)
tests/test_zarr_ipfs.py
201-201: pytest.raises(Exception) should be considered evil
(B017)
219-219: pytest.raises(Exception) should be considered evil
(B017)
🪛 GitHub Actions: Triggered on push from TheGreatAlgo to branch/tag add-authentication
tests/test_zarr_ipfs.py
[error] 1-1: Ruff formatting check failed. 1 file would be reformatted.
🔇 Additional comments (4)
.github/workflows/run-checks.yaml (2)
28-80: Authentication configuration is well-structured and comprehensive.The Nginx configuration properly implements multiple authentication methods (API key, Bearer token, Basic Auth) for the IPFS API proxy. This aligns well with the authentication functionality being added to the
IPFSStoreclass.🧰 Tools
🪛 YAMLlint (1.35.1)
[error] 80-80: trailing spaces
(trailing-spaces)
81-91: Startup sequence is correctly ordered.The sequence of starting Nginx, restarting IPFS daemon, and adding a sleep interval ensures that both services are properly initialized before running tests.
tests/test_zarr_ipfs.py (2)
142-170: Test implementation for API key authentication is robust.The test correctly verifies that the API key authentication works by successfully writing and reading data through the authenticated gateway.
171-198: Authentication method tests are comprehensive.The tests for bearer token and basic authentication are well-implemented, ensuring all authentication methods supported by the
IPFSStoreclass are verified.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
tests/test_zarr_ipfs.py (1)
172-172: 🛠️ Refactor suggestionSpecify the expected exception type in pytest.raises()
Using
pytest.raises(Exception)is too broad and may catch unexpected exceptions. Specify the exact exception types that you expect to be raised.- with pytest.raises(Exception): + with pytest.raises((requests.exceptions.HTTPError, RuntimeError)): # Update with actual exception typesApply this change to all four instances in the test (lines 172, 199, 225, and 243).
Also applies to: 199-199, 225-225, 243-243
🧰 Tools
🪛 Ruff (0.8.2)
172-172:
pytest.raises(Exception)should be considered evil(B017)
🧹 Nitpick comments (1)
tests/test_zarr_ipfs.py (1)
174-180: Consider testing API key with custom headerThe test covers API key authentication with the default header, but doesn't test with a custom
api_key_headerparameter. Consider adding a test case for this scenario to ensure complete coverage.+ # Test with API key and custom header + hamt = HAMT( + store=IPFSStore( + rpc_uri_stem="http://127.0.0.1:5002", + api_key="test", + api_key_header="X-Custom-API-Key", + ), + transformer_encode=encrypt, + transformer_decode=decrypt, + ) + test_ds.to_zarr(store=hamt, mode="w") + + hamt.make_read_only() + loaded_ds = xr.open_zarr(store=hamt) + xr.testing.assert_identical(loaded_ds, expected_ds)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
tests/test_zarr_ipfs.py(1 hunks)
🧰 Additional context used
🪛 Ruff (0.8.2)
tests/test_zarr_ipfs.py
172-172: pytest.raises(Exception) should be considered evil
(B017)
199-199: pytest.raises(Exception) should be considered evil
(B017)
225-225: pytest.raises(Exception) should be considered evil
(B017)
243-243: pytest.raises(Exception) should be considered evil
(B017)
🪛 GitHub Actions: Triggered on push from TheGreatAlgo to branch/tag add-authentication
tests/test_zarr_ipfs.py
[error] 1-1: Ruff formatting check failed. 1 file would be reformatted.
🔇 Additional comments (3)
tests/test_zarr_ipfs.py (3)
142-245: LGTM! Good job implementing the authentication test function.The test covers all three authentication methods: API key, bearer token, and basic auth, including tests for invalid credentials. This provides comprehensive coverage for the authentication functionality.
🧰 Tools
🪛 Ruff (0.8.2)
172-172:
pytest.raises(Exception)should be considered evil(B017)
199-199:
pytest.raises(Exception)should be considered evil(B017)
225-225:
pytest.raises(Exception)should be considered evil(B017)
243-243:
pytest.raises(Exception)should be considered evil(B017)
159-160: Fix spacing around equals signs in keyword argumentsThere are inconsistent spaces around the equals signs in keyword arguments. Remove the spaces to match the project's style guidelines.
- rpc_uri_stem = "http://127.0.0.1:5002", + rpc_uri_stem="http://127.0.0.1:5002",This formatting issue is causing the pipeline failure. Please run
ruff formatto automatically fix all formatting issues.Also applies to: 175-176, 187-187, 203-203, 213-213, 229-229
237-239:❓ Verification inconclusive
Consider adding authentication parameters when reading
The test reads from the IPFS store without providing authentication parameters, which differs from the write operation. Consider adding the same authentication parameters to verify they're required for both operations.
🏁 Script executed:
#!/bin/bash # Check if authentication is required for reading operations grep -n "_get_request_headers" $(fd "store.py$") | head -10Length of output: 118
Review Authentication Consistency Verification
The current test in
tests/test_zarr_ipfs.py(lines 237–239) instantiates the IPFS store for reading without supplying authentication parameters. Our initial grep for_get_request_headersin relevant store files (e.g.,py_hamt/store.py) did not yield clear evidence regarding the use of authentication in the read path. Please manually verify the following:
- Confirm whether authentication parameters (or corresponding headers) are indeed required for the read operation, similar to the write path.
- If authentication is enforced for writing via mechanisms implemented in functions like
_get_request_headers, ensure the read operation reflects this requirement for consistency.- Update the test or the store instantiation accordingly if the authentication should be applied.
abidsikder
changed the title
abidsikder
force-pushed
the
add-authentication
branch
from
7fe666b to
7fbc901
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
py_hamt/store.py (1)
139-155: Authentication integration in the save method looks good, but should consider extracting header logicThe authentication logic is correctly implemented, but consider extracting the header construction into a private method for better maintainability and reusability.
class IPFSStore(Store): + def _get_request_headers(self) -> dict[str, str]: + """Helper method to construct authentication headers""" + headers = {} + if self.bearer_token: + headers["Authorization"] = f"Bearer {self.bearer_token}" + elif self.api_key: + headers[self.api_key_header] = self.api_key + return headers def save(self, data: bytes, cid_codec: str) -> CID: # ... pin_string: str = "true" if self.pin_on_add else "false" - # Apply authentication based on provided credentials - headers = {} - if self.bearer_token: - headers["Authorization"] = f"Bearer {self.bearer_token}" - elif self.api_key: - headers[self.api_key_header] = self.api_key + # Get authentication headers + headers = self._get_request_headers() # Prepare request parameters url = f"{self.rpc_uri_stem}/api/v0/add?hash={self.hasher}&pin={pin_string}"Note: I see from past review comments that there was a suggestion both for and against extracting this logic. It's ultimately a matter of code style preference.
.github/workflows/run-checks.yaml (2)
28-80: Good Nginx setup for authentication testingThe Nginx configuration properly sets up a reverse proxy with authentication checks that match the authentication methods implemented in the
IPFSStoreclass.There's a trailing whitespace on line 80 that should be removed:
- sudo nginx -t - + sudo nginx -t +🧰 Tools
🪛 YAMLlint (1.35.1)
[error] 80-80: trailing spaces
(trailing-spaces)
35-72: Security considerations for authentication credentialsUsing hardcoded credentials (
test) is acceptable for testing, but consider using environment variables or GitHub secrets for a more secure pattern, especially if this might be reused in other environments.- if ($http_x_api_key = "test") { + if ($http_x_api_key = "$ENV{TEST_API_KEY}") {And similar changes for the other authentication methods.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
.github/workflows/run-checks.yaml(1 hunks)py_hamt/store.py(4 hunks)tests/test_zarr_ipfs.py(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/test_zarr_ipfs.py
🧰 Additional context used
🪛 YAMLlint (1.35.1)
.github/workflows/run-checks.yaml
[error] 80-80: trailing spaces
(trailing-spaces)
🔇 Additional comments (6)
py_hamt/store.py (5)
69-74: Documentation enhancement looks good!The expanded class docstring clearly describes the new authentication methods supported by the IPFSStore class.
84-89: Authentication parameters implementation looks appropriateThe constructor has been properly extended with well-typed authentication parameters that match the documentation.
113-122: Well-documented instance variablesEach authentication-related instance variable has a clear docstring explaining its purpose.
147-147: Good variable naming for the URLMoving the URL to a separate variable with a descriptive name improves readability.
182-205:❓ Verification inconclusive
Consider adding authentication for load method
The
loadmethod uses the HTTP Gateway but doesn't include any authentication parameters. If the gateway also requires authentication, consider updating this method to include authentication headers.
🏁 Script executed:
#!/bin/bash # Check if the gateway URL might need authentication by testing access patterns in tests rg -A 3 "gateway_uri_stem" tests/Length of output: 33
Below is a revised review comment. Since the initial search for test references to authentication (using “gateway_uri_stem”) returned no output, and no evidence of authentication headers is seen elsewhere in the codebase, it remains unclear whether the HTTP gateway requires authentication. In light of this, please manually verify if the gateway requires authentication; if it does, consider enhancing the
loadmethod to pass authentication headers (or other credentials) to the GET request.
- Action Point: Confirm with the HTTP gateway documentation or other parts of the project whether authentication is needed.
- If required: Update the method to include configurable authentication (e.g., by accepting an optional authentication parameter and passing corresponding headers to
requests.get).Attention: Manual verification is advised for the authentication requirement, as current test searches (e.g. for “gateway_uri_stem” and “Authorization”) do not yield evidence of such use.
.github/workflows/run-checks.yaml (1)
81-92: Service restart logic looks goodThe steps to restart the services and wait for them to initialize are appropriate and necessary for the tests to run correctly.
Summary by CodeRabbit
New Features
Tests