gooroodev commented on PR #28706:
URL: https://github.com/apache/superset/pull/28706#issuecomment-2131310765
### Summary of Changes
1. **New Functionality**:
- Added a new function `fetch_csrf_token` to fetch a CSRF token for API
requests.
- Integrated the `fetch_csrf_token` function into the `fetch_url` task to
ensure that CSRF tokens are included in the headers for API requests.
2. **Modifications**:
- Updated the `fetch_url` method to use `ChartRestApi.warm_up_cache`
instead of `Superset.warm_up_cache`.
- Modified the integration test `test_fetch_url` to include tests for the
new `fetch_csrf_token` function.
3. **Tests**:
- Added a new test `test_fetch_csrf_token` to verify the functionality of
fetching CSRF tokens.
- Updated the `test_fetch_url` to mock and validate the behavior of
`fetch_csrf_token`.
### Issues, Bugs, or Typos
1. **Error Handling in `fetch_csrf_token`**:
- The function `fetch_csrf_token` logs an error but does not raise an
exception or handle the error case effectively. This can lead to silent
failures.
2. **Variable Naming**:
- The variable `session_cookie` should be checked for `None` before being
added to the headers to avoid potential issues if the cookie is not set.
3. **Test Improvements**:
- The test `test_fetch_csrf_token` should include a case where the
response code is not 200 to ensure error handling is tested.
### Proposed Code Improvements
#### Improvement 1: Error Handling in `fetch_csrf_token`
**Current Code**:
```python
logger.error("Error fetching CSRF token, status code: %s", response.code)
return {}
```
**Improved Code**:
```python
logger.error("Error fetching CSRF token, status code: %s", response.code)
raise Exception(f"Failed to fetch CSRF token, status code: {response.code}")
```
#### Improvement 2: Check for `None` in `session_cookie`
**Current Code**:
```python
session_cookie = response.headers.get("Set-Cookie")
if response.code == 200:
data = json.loads(body)
return {"Cookie": session_cookie, "X-CSRF-Token": data["result"]}
```
**Improved Code**:
```python
session_cookie = response.headers.get("Set-Cookie")
if response.code == 200 and session_cookie:
data = json.loads(body)
return {"Cookie": session_cookie, "X-CSRF-Token": data["result"]}
else:
logger.error("Error fetching CSRF token or session cookie missing,
status code: %s", response.code)
raise Exception(f"Failed to fetch CSRF token or session cookie missing,
status code: {response.code}")
```
#### Improvement 3: Test for Non-200 Response Code
**Current Test**:
```python
def test_fetch_csrf_token(mock_urlopen, mock_request_cls, base_url,
app_context):
# ... existing test code ...
```
**Improved Test**:
```python
def test_fetch_csrf_token(mock_urlopen, mock_request_cls, base_url,
app_context):
from superset.tasks.cache import fetch_csrf_token
mock_request = mock.MagicMock()
mock_request_cls.return_value = mock_request
mock_response = mock.MagicMock()
mock_urlopen.return_value.__enter__.return_value = mock_response
# Test for successful response
mock_response.code = 200
mock_response.read.return_value = b'{"result": "csrf_token"}'
mock_response.headers.get.return_value = "new_session_cookie"
app.config["WEBDRIVER_BASEURL"] = base_url
headers = {"Cookie": "original_session_cookie"}
result_headers = fetch_csrf_token(headers)
mock_request_cls.assert_called_with(
"http://base-url/api/v1/security/csrf_token/";,
headers=headers,
method="GET",
)
assert result_headers["X-CSRF-Token"] == "csrf_token"
assert result_headers["Cookie"] == "new_session_cookie"
mock_urlopen.assert_called_once_with(mock_request, timeout=mock.ANY)
# Test for unsuccessful response
mock_response.code = 500
with pytest.raises(Exception):
fetch_csrf_token(headers)
```
### General Review of Code Quality and Style
- **Code Quality**: The code is generally well-structured and follows good
practices. The addition of the `fetch_csrf_token` function abstracts the CSRF
token fetching logic, making the `fetch_url` function cleaner and more
maintainable.
- **Code Style**: The code follows PEP 8 guidelines, and the use of type
hints improves readability and maintainability.
- **Logging**: The use of logging is appropriate for debugging and tracking
the flow of the program, but error handling could be more robust.
- **Testing**: The tests cover the new functionality well, but additional
edge cases and error conditions should be tested to ensure robustness.
Yours, [Gooroo.dev](https://gooroo.dev). To receive reviews automatically,
[install Github App](https://github.com/apps/gooroo-dev)
--
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: notifications-unsubscr...@superset.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscr...@superset.apache.org
For additional commands, e-mail: notifications-h...@superset.apache.org
