rusackas commented on code in PR #38807: URL: https://github.com/apache/superset/pull/38807#discussion_r3321153802
########## superset-frontend/packages/superset-ui-core/src/components/RlsBadge/index.tsx: ########## @@ -0,0 +1,94 @@ +/** + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ +import { t } from '@apache-superset/core/translation'; +import { useTheme } from '@apache-superset/core/theme'; Review Comment: Import `css` here so the template-literal helper is available for the style fixes below. ```suggestion import { useTheme, css } from '@apache-superset/core/theme'; ``` ########## superset/explore/schemas.py: ########## @@ -100,6 +100,10 @@ class DatasetSchema(Schema): verbose_map = fields.Dict( metadata={"description": "Mapping from raw name to verbose name."} ) + rls_filters = fields.List( + fields.Dict(), + metadata={"description": "Row-level security filters applied to this dataset."}, + ) Review Comment: `fields.Dict()` has no key/value typing. `fields.Raw()` is the right field for an arbitrary JSON object (or better yet, a typed `fields.Nested()` schema if you want OpenAPI to reflect the shape). Adding `load_default=[]` makes it explicit that the field defaults to an empty list. ```suggestion rls_filters = fields.List( fields.Raw(), load_default=[], metadata={"description": "Row-level security filters applied to this dataset."}, ) ``` ########## superset-frontend/packages/superset-ui-core/src/components/RlsBadge/index.tsx: ########## @@ -0,0 +1,94 @@ +/** + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ +import { t } from '@apache-superset/core/translation'; +import { useTheme } from '@apache-superset/core/theme'; +import { Icons } from '@superset-ui/core/components/Icons'; +import { Tooltip } from '../Tooltip'; +import type { IconType } from '@superset-ui/core/components/Icons/types'; + +export interface RlsFilterSummary { + id: number; + name: string; + filter_type?: string; + group_key?: string | null; + inherited?: boolean; + clause?: string; + roles?: Array<{ id: number; name: string }>; +} + +export interface RlsBadgeProps { + rlsFilters: RlsFilterSummary[]; + size?: IconType['iconSize']; +} + +export function RlsBadge({ rlsFilters, size = 'l' }: RlsBadgeProps) { + const theme = useTheme(); + + if (!rlsFilters?.length) { + return null; + } + + const hasInherited = rlsFilters.some(f => f.inherited); + + const tooltipContent = ( + <div> + <strong> + {t( + 'Row-Level Security: %d filter(s) may restrict data based on your role.', + rlsFilters.length, + )} + </strong> + <ul style={{ paddingLeft: 16, margin: '4px 0 0' }}> + {rlsFilters.map(filter => ( + <li key={filter.id}> + <div> + {filter.name} + {filter.filter_type ? ` (${filter.filter_type})` : ''} + {filter.group_key ? ` [${filter.group_key}]` : ''} + {filter.inherited ? ` — ${t('from underlying table')}` : ''} + </div> + {filter.roles && filter.roles.length > 0 && ( + <div> + {t('Roles')}: {filter.roles.map(role => role.name).join(', ')} + </div> + )} + {filter.clause && ( + <div> + {t('Clause')}: {filter.clause} + </div> + )} + </li> + ))} + </ul> + {hasInherited && ( + <div style={{ marginTop: 4, fontStyle: 'italic' }}> Review Comment: Same pattern — `css` prop with theme tokens instead of inline `style`. ```suggestion <div css={css` margin-top: ${theme.sizeUnit}px; font-style: italic; `} > ``` ########## superset/datasets/api.py: ########## @@ -307,6 +307,43 @@ class DatasetRestApi(BaseSupersetModelRestApi): list_outer_default_load = True show_outer_default_load = True + def get_list_headless(self, **kwargs: Any) -> Response: + response = super().get_list_headless(**kwargs) + if response.status_code == 200: + try: + payload = response.json Review Comment: **Double-serialization note:** `response.json` deserializes the response body back from bytes (produced by `super().get_list_headless()`), and then `self.response(200, **payload)` on line 339 re-serializes it. For large dataset lists this is noticeable overhead. A follow-up improvement worth considering: override `_get_list_response_object` (or the equivalent FAB hook) instead of `get_list_headless`, so the RLS data can be injected *before* the response is built rather than after. In the meantime, the current approach is functional — just worth a TODO. ########## superset-frontend/packages/superset-ui-core/src/components/RlsBadge/index.tsx: ########## @@ -0,0 +1,94 @@ +/** + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ +import { t } from '@apache-superset/core/translation'; +import { useTheme } from '@apache-superset/core/theme'; +import { Icons } from '@superset-ui/core/components/Icons'; +import { Tooltip } from '../Tooltip'; +import type { IconType } from '@superset-ui/core/components/Icons/types'; + +export interface RlsFilterSummary { + id: number; + name: string; + filter_type?: string; + group_key?: string | null; + inherited?: boolean; + clause?: string; + roles?: Array<{ id: number; name: string }>; +} + +export interface RlsBadgeProps { + rlsFilters: RlsFilterSummary[]; + size?: IconType['iconSize']; +} + +export function RlsBadge({ rlsFilters, size = 'l' }: RlsBadgeProps) { + const theme = useTheme(); + + if (!rlsFilters?.length) { + return null; + } + + const hasInherited = rlsFilters.some(f => f.inherited); + + const tooltipContent = ( + <div> + <strong> + {t( + 'Row-Level Security: %d filter(s) may restrict data based on your role.', + rlsFilters.length, + )} + </strong> + <ul style={{ paddingLeft: 16, margin: '4px 0 0' }}> Review Comment: Avoid inline `style` — use `css` prop with theme tokens per the CLAUDE.md style guide. ```suggestion <ul css={css` padding-left: ${theme.sizeUnit * 4}px; margin: ${theme.sizeUnit}px 0 0; `} > ``` -- 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]
