codeant-ai-for-open-source[bot] commented on code in PR #38376:
URL: https://github.com/apache/superset/pull/38376#discussion_r2881412736
##########
superset-frontend/src/pages/DatabaseList/index.tsx:
##########
@@ -108,20 +122,106 @@ function DatabaseList({
addSuccessToast,
user,
}: DatabaseListProps) {
+ const theme = useTheme();
+ const showSemanticLayers = isFeatureEnabled(FeatureFlag.SemanticLayers);
+
+ // Standard database list view resource (used when SL flag is OFF)
const {
state: {
- loading,
- resourceCount: databaseCount,
- resourceCollection: databases,
+ loading: dbLoading,
+ resourceCount: dbCount,
+ resourceCollection: dbCollection,
},
hasPerm,
- fetchData,
- refreshData,
+ fetchData: dbFetchData,
+ refreshData: dbRefreshData,
} = useListViewResource<DatabaseObject>(
'database',
t('database'),
addDangerToast,
);
+
+ // Combined endpoint state (used when SL flag is ON)
+ const [combinedItems, setCombinedItems] = useState<ConnectionItem[]>([user]);
Review Comment:
**Suggestion:** The combined connections list is initialized with the `user`
object instead of an empty array, so before the first fetch the table will
render a bogus row whose shape doesn't match `ConnectionItem` (missing fields
like `database_name`, `backend`, etc.), which can lead to incorrect UI state or
runtime errors when columns try to access these properties; initialize it as an
empty array instead. [logic error]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ⚠️ Databases list briefly shows bogus row from logged-in user.
- ⚠️ Clicking actions on bogus row triggers failing database API call.
```
</details>
```suggestion
const [combinedItems, setCombinedItems] = useState<ConnectionItem[]>([]);
```
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Enable the `SEMANTIC_LAYERS` feature flag in the backend configuration so
that
`isFeatureEnabled(FeatureFlag.SemanticLayers)` returns true (see semantic
layers wiring
under `superset/superset/config.py` and usage in
`superset-frontend/src/pages/DatabaseList/index.tsx`).
2. Log in to Superset as a user who has `can_write` on databases so that the
Databases
page is accessible and actions are visible.
3. Navigate to the Databases list UI, which renders the `DatabaseList`
component defined
in `superset-frontend/src/pages/DatabaseList/index.tsx`. When
`showSemanticLayers` is
true, it passes `databases = combinedItems` into `ListView<DatabaseObject>`
(see
`ListView` usage near the bottom of this file).
4. On first render before any network response, `combinedItems` is
initialized to `[user]`
(the `user` prop has shape `{ userId, firstName, lastName }` at
`DatabaseList`'s props).
`ListView` (implemented in
`superset-frontend/src/components/ListView/ListView.tsx`, lines
~268–323) immediately uses `data` to build table rows, so a single row with
`original ===
user` is rendered even though it does not have `DatabaseObject` fields like
`database_name`, `backend`, `allow_run_async`, or `id`.
5. Observe that this bogus row is displayed in the Databases table (mostly
blank columns).
If the user quickly clicks the Actions column on this row before
`/api/v1/semantic_layer/connections/` returns, the handlers in the Actions
column (defined
in `DatabaseList`'s `columns` useMemo) treat `original` as a
`DatabaseObject` and call
helpers like `openDatabaseDeleteModal(original)`, which triggers a request to
`/api/v1/database/${original.id}/related_objects/` with `original.id ===
undefined`,
producing an invalid API call and resulting toast error.
```
</details>
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:** superset-frontend/src/pages/DatabaseList/index.tsx
**Line:** 145:145
**Comment:**
*Logic Error: The combined connections list is initialized with the
`user` object instead of an empty array, so before the first fetch the table
will render a bogus row whose shape doesn't match `ConnectionItem` (missing
fields like `database_name`, `backend`, etc.), which can lead to incorrect UI
state or runtime errors when columns try to access these properties; initialize
it as an empty array instead.
Validate the correctness of the flagged issue. If correct, How can I resolve
this? If you propose a fix, implement it and please make it concise.
```
</details>
<a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F38376&comment_hash=db5364a258ddf06b37456e04def959e5a041e064215ba74c582ffb27c5ab1ddc&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F38376&comment_hash=db5364a258ddf06b37456e04def959e5a041e064215ba74c582ffb27c5ab1ddc&reaction=dislike'>👎</a>
##########
superset/semantic_layers/api.py:
##########
@@ -436,6 +523,164 @@ def delete(self, uuid: str) -> FlaskResponse:
)
return self.response_422(message=str(ex))
+ @expose("/connections/", methods=("GET",))
+ @protect()
+ @safe
+ @statsd_metrics
+ @rison(get_list_schema)
+ @event_logger.log_this_with_context(
+ action=lambda self, *args, **kwargs: f"{self.__class__.__name__}"
+ ".connections",
+ log_to_statsd=False,
+ )
+ def connections(self, **kwargs: Any) -> FlaskResponse:
+ """List databases and semantic layers combined.
+ ---
+ get:
+ summary: List databases and semantic layers combined
+ parameters:
+ - in: query
+ name: q
+ content:
+ application/json:
+ schema:
+ $ref: '#/components/schemas/get_list_schema'
+ responses:
+ 200:
+ description: Combined list of databases and semantic layers
+ 401:
+ $ref: '#/components/responses/401'
+ 500:
+ $ref: '#/components/responses/500'
+ """
+ args = kwargs.get("rison", {})
+ page = args.get("page", 0)
+ page_size = args.get("page_size", 25)
+ order_column = args.get("order_column", "changed_on")
+ order_direction = args.get("order_direction", "desc")
+ filters = args.get("filters", [])
+
+ source_type = "all"
+ name_filter = None
+ for f in filters:
+ if f.get("col") == "source_type":
+ source_type = f.get("value", "all")
+ elif f.get("col") == "database_name" and f.get("opr") == "ct":
+ name_filter = f.get("value")
+
+ if not is_feature_enabled("SEMANTIC_LAYERS"):
+ source_type = "database"
+
+ reverse = order_direction == "desc"
+
+ def _sort_key_changed_on(
+ item: tuple[str, Database | SemanticLayer],
+ ) -> Any:
+ return item[1].changed_on or ""
Review Comment:
**Suggestion:** The `_sort_key_changed_on` helper currently returns either a
`datetime` (when `changed_on` is set) or the empty string `""` (when it is
`None`), so when there is at least one object with `changed_on=None` the
in-place sort over `all_items` will attempt to compare `datetime` and `str`
keys and raise a `TypeError` at runtime; returning a single numeric type (e.g.
a timestamp or 0.0) for all cases avoids this mixed-type comparison issue.
[type error]
<details>
<summary><b>Severity Level:</b> Critical 🚨</summary>
```mdx
- ❌ GET /api/v1/semantic_layer/connections/ can crash sorting.
- ⚠️ Semantic connections UI breaks when some rows lack changed_on.
```
</details>
```suggestion
def _sort_key_changed_on(
item: tuple[str, Database | SemanticLayer],
) -> float:
changed_on = item[1].changed_on
return changed_on.timestamp() if changed_on else 0.0
```
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Start a Superset instance with semantic layers enabled, as in tests using
`SEMANTIC_LAYERS_APP`
(`superset/tests/unit_tests/semantic_layers/api_test.py:37-41`),
which configure `FEATURE_FLAGS: {"SEMANTIC_LAYERS": True}`.
2. Ensure there are at least two connection-like objects: (a) a `Database`
row with a
non-null `changed_on` timestamp and (b) a `SemanticLayer` or `Database` row
with
`changed_on` set to NULL in the database. The `changed_on` column is
nullable by design in
`AuditMixinNullable` (`superset/models/helpers.py:563-566`), and both
`Database`
(`superset/models/core.py:144-151` with `AuditMixinNullable` mixin) and
`SemanticLayer`
(`superset/semantic_layers/models.py:110-118` with `AuditMixinNullable`
mixin) inherit
this behavior, so rows with `changed_on` = NULL are valid.
3. Call the combined connections endpoint, which is registered by
Flask-AppBuilder as
`/api/v1/semantic_layer/connections/` for `SemanticLayerRestApi`
(`superset/semantic_layers/api.py:242-247` and `@expose("/connections/",
methods=("GET",))` at `superset/semantic_layers/api.py:526-536`), e.g. via
HTTP GET:
`GET
/api/v1/semantic_layer/connections/?q={"order_column":"changed_on_delta_humanized","order_direction":"desc"}`.
4. Inside `SemanticLayerRestApi.connections`
(`superset/semantic_layers/api.py:556-634`),
the code builds `all_items` combining `("database", Database)` and
`("semantic_layer",
SemanticLayer)` tuples and then sorts them in-place with:
- sort key selection at `superset/semantic_layers/api.py:592-596`, mapping
`"changed_on_delta_humanized"` to `_sort_key_changed_on`.
- `_sort_key_changed_on` definition at
`superset/semantic_layers/api.py:576-579`, which
currently returns `item[1].changed_on` (a `datetime`) or the empty string
`""` when
`changed_on` is `None`.
When both non-null and null `changed_on` values exist, Python's list sort
evaluates
keys that are a mix of `datetime` and `str`, and comparison between these
types raises
`TypeError: '<' not supported between instances of 'datetime.datetime'
and 'str'`,
causing the `/api/v1/semantic_layer/connections/` request to fail with
HTTP 500.
```
</details>
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:** superset/semantic_layers/api.py
**Line:** 576:579
**Comment:**
*Type Error: The `_sort_key_changed_on` helper currently returns either
a `datetime` (when `changed_on` is set) or the empty string `""` (when it is
`None`), so when there is at least one object with `changed_on=None` the
in-place sort over `all_items` will attempt to compare `datetime` and `str`
keys and raise a `TypeError` at runtime; returning a single numeric type (e.g.
a timestamp or 0.0) for all cases avoids this mixed-type comparison issue.
Validate the correctness of the flagged issue. If correct, How can I resolve
this? If you propose a fix, implement it and please make it concise.
```
</details>
<a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F38376&comment_hash=d7702e32b8aafb7140cbdc1ec6c8bf9809a99acc0f88edafef35d90b3a3c3d79&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F38376&comment_hash=d7702e32b8aafb7140cbdc1ec6c8bf9809a99acc0f88edafef35d90b3a3c3d79&reaction=dislike'>👎</a>
--
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]
