Re: [PR] chore: module scope should not require the app context [superset]
github-advanced-security[bot] commented on code in PR #28378:
URL: https://github.com/apache/superset/pull/28378#discussion_r1614840632
##
superset/views/base.py:
##
@@ -153,9 +151,7 @@
payload = payload or {"error": f"{msg}"}
return Response(
-json_utils.dumps(
-payload, default=json_utils.json_iso_dttm_ser, ignore_nan=True
-),
+json.dumps(payload, default=json_utils.json_iso_dttm_ser,
ignore_nan=True),
Review Comment:
## Information exposure through an exception
[Stack trace information](1) flows to this location and may be exposed to an
external user.
[Stack trace information](2) flows to this location and may be exposed to an
external user.
[Stack trace information](3) flows to this location and may be exposed to an
external user.
[Stack trace information](4) flows to this location and may be exposed to an
external user.
[Stack trace information](5) flows to this location and may be exposed to an
external user.
[Stack trace information](6) flows to this location and may be exposed to an
external user.
[Stack trace information](7) flows to this location and may be exposed to an
external user.
[Stack trace information](8) flows to this location and may be exposed to an
external user.
[Stack trace information](9) flows to this location and may be exposed to an
external user.
[Stack trace information](10) flows to this location and may be exposed to
an external user.
[Stack trace information](11) flows to this location and may be exposed to
an external user.
[Stack trace information](12) flows to this location and may be exposed to
an external user.
[Show more
details](https://github.com/apache/superset/security/code-scanning/1225)
--
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
Re: [I] Deployment behind nginx proxy manager [superset]
lianstemp commented on issue #23740: URL: https://github.com/apache/superset/issues/23740#issuecomment-2131336233 > Issue resolved in NPM sorry Hi, May I know how you fixed this problem, as this is my problem now -- 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
Re: [PR] fix: Handling of column types for Presto, Trino, et al. [superset]
admsev commented on PR #28653: URL: https://github.com/apache/superset/pull/28653#issuecomment-2131318572 @gooroodev review -- 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
Re: [PR] fix: Handling of column types for Presto, Trino, et al. [superset]
gooroodev commented on PR #28653:
URL: https://github.com/apache/superset/pull/28653#issuecomment-2131311669
### 1. Summary of Changes
The pull request addresses the handling of column types for Presto and Trino
engines in the Superset project. The key changes include:
- **Presto Engine:**
- Handling of column types in the `get_children` and
`where_latest_partition` methods.
- Added logic to convert string column types to SQLAlchemy types
dynamically.
- **Trino Engine:**
- Added a similar handling of column types in the `where_latest_partition`
method.
- Added unit tests for the `where_latest_partition` method.
- **Typing Changes:**
- Introduced a new type alias `SQLType` in `superset_typing.py` to
represent SQLAlchemy column types.
- Updated `ResultSetColumnType` to use `SQLType`.
### 2. Issues, Bugs, or Typos
Issue 1:
- **File:** `superset/db_engine_specs/presto.py`
- **Code:**
```python
match = pattern.match(cast(str, column["type"]))
```
**Problem:** The use of `cast(str, column["type"])` is unnecessary if
`column["type"]` is already a string.
**Improvement:**
```python
match = pattern.match(column["type"])
```
Issue 2:
- **File:** `superset/db_engine_specs/presto.py`
- **Code:**
```python
if isinstance(col_type, str):
if col_type_class := getattr(types, col_type, None):
col_type = col_type_class()
else:
col_type = None
```
**Problem:** The assignment of `None` to `col_type` if `col_type_class` is
not found can be simplified.
**Improvement:**
```python
if isinstance(col_type, str):
col_type_class = getattr(types, col_type, None)
col_type = col_type_class() if col_type_class else None
```
### 3. General Review of Code Quality and Style
- **Code Quality:** The code changes are well-structured and address the
problem of handling different column types effectively. The use of type hinting
and type checking improves code readability and maintainability.
- **Code Style:** The code adheres to PEP 8 standards, making it clean and
easy to read. The use of modern Python features like type hinting and `:=`
operator (walrus operator) is commendable.
- **Testing:** The addition of unit tests for the `where_latest_partition`
method in both Presto and Trino engines ensures that the changes are tested and
validated.
### Additional Suggestions
- **Documentation:** Consider adding docstrings to the modified methods to
explain the purpose and functionality of the changes.
- **Error Handling:** The use of broad exception handling (`raise
Exception`) could be improved by raising more specific exceptions to provide
better context in case of errors.
Overall, the pull request is well-implemented and addresses the problem
effectively. The suggested improvements are minor and aim to further enhance
the code quality.
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
Re: [PR] fix: Handling of column types for Presto, Trino, et al. [superset]
gooroodev commented on PR #28653:
URL: https://github.com/apache/superset/pull/28653#issuecomment-2131311297
### Summary of Changes
1. **Presto Engine Specification**:
- Minor formatting adjustments.
- Updated the `get_children` function to cast the column type to a string.
- Modified the `where_latest_partition` function to handle column types
that are strings by converting them to SQLAlchemy types if possible.
2. **Superset Typing**:
- Introduced a new type alias `SQLType` for Union of `TypeEngine` and
`type[TypeEngine]`.
- Updated `ResultSetColumnType` to use the new `SQLType` alias.
3. **Unit Tests for Presto**:
- Adjusted the `test_where_latest_partition` to use string
representations of column types instead of SQLAlchemy types.
4. **Unit Tests for Trino**:
- Added a new test `test_where_latest_partition` for Trino engine
specification.
- Used `MagicMock` instead of `Mock` for database mocking.
- Included the Trino dialect for SQL compilation in the test.
### Issues, Bugs, and Typos
1. **File: `superset/db_engine_specs/presto.py`**
- **Issue**: Redundant import of `annotations` from `__future__`.
- **Improvement**: Remove the import statement if it's not being used.
2. **File: `superset/superset_typing.py`**
- **Issue**: The `SQLType` alias is defined but not used in
`ResultSetColumnType`.
- **Improvement**: Ensure `SQLType` is utilized correctly in the
`ResultSetColumnType` definition.
3. **File: `tests/unit_tests/db_engine_specs/test_presto.py`**
- **Issue**: The test function `test_where_latest_partition` has
inconsistent parameter typing.
- **Improvement**: Update the parameter typing to match the expected
types.
4. **File: `tests/unit_tests/db_engine_specs/test_trino.py`**
- **Issue**: The `test_where_latest_partition` function name conflicts
with the one in `test_presto.py`.
- **Improvement**: Rename the test function to avoid conflicts and
improve clarity.
### Proposed Code Improvements
1. **Remove Redundant Import in Presto Engine Spec**
```python
# Remove the following line if not used:
from __future__ import annotations
```
2. **Ensure Correct Typing in Superset Typing**
```python
# Ensure SQLType is used correctly
class ResultSetColumnType(TypedDict):
name: str # legacy naming convention keeping this for backwards
compatibility
column_name: str
type: Optional[Union[SQLType, str]]
is_dttm: Optional[bool]
type_generic: NotRequired[Optional["GenericDataType"]]
```
3. **Update Typing in Presto Test**
```python
@pytest.mark.parametrize(
["column_type", "column_value", "expected_value"],
[
("DATE", "2023-05-01", "DATE '2023-05-01'"),
("TIMESTAMP", "2023-05-01", "TIMESTAMP '2023-05-01'"),
("VARCHAR", "2023-05-01", "'2023-05-01'"),
("INT", 1234, "1234"),
],
)
def test_where_latest_partition(
mock_latest_partition: Mock,
column_type: str,
column_value: Any,
expected_value: str,
) -> None:
from superset.db_engine_specs.presto import PrestoEngineSpec
mock_latest_partition.return_value = (["partition_key"],
[column_value])
assert (
str(
PrestoEngineSpec.where_latest_partition( # type: ignore
database=mock.MagicMock(),
table=Table("table"),
query=sql.select(text("* FROM table")),
columns=[
{
"column_name": "partition_key",
"name": "partition_key",
"type": column_type,
"is_dttm": False,
}
],
).compile(
dialect=PrestoDialect(),
compile_kwargs={"literal_binds": True},
)
)
== f"""SELECT * FROM table \nWHERE "partition_key" =
{expected_value}"""
)
```
4. **Rename Trino Test Function**
```python
@patch("superset.db_engine_specs.trino.TrinoEngineSpec.latest_partition")
@pytest.mark.parametrize(
["column_type", "column_value", "expected_value"],
[
(types.DATE(), "2023-05-01", "DATE '2023-05-01'"),
(types.TIMESTAMP(), "2023-05-01", "TIMESTAMP '2023-05-01'"),
(types.VARCHAR(), "2023-05-01", "'2023-05-01'"),
(types.INT(), 1234, "1234"),
],
)
def test_trino_where_latest_partition(
mock_latest_partition: Mock,
column_type: SQLType,
Re: [PR] fix: Handling of column types for Presto, Trino, et al. [superset]
admsev commented on PR #28653: URL: https://github.com/apache/superset/pull/28653#issuecomment-2131311259 @gooroodev review -- 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
Re: [PR] fix: Chart cache-warmup task fails on Superset 4.0 [superset]
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,
Re: [PR] fix: Handling of column types for Presto, Trino, et al. [superset]
admsev commented on PR #28653: URL: https://github.com/apache/superset/pull/28653#issuecomment-2131310754 @gooroodev review please -- 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
Re: [PR] fix: Chart cache-warmup task fails on Superset 4.0 [superset]
admsev commented on PR #28706: URL: https://github.com/apache/superset/pull/28706#issuecomment-2131310493 @gooroodev please review -- 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
Re: [PR] chore: remove ipython from development dependencies [superset]
mistercrunch merged PR #28703: URL: https://github.com/apache/superset/pull/28703 -- 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
