bito-code-review[bot] commented on code in PR #36933:
URL: https://github.com/apache/superset/pull/36933#discussion_r2674837633
##########
superset/charts/api.py:
##########
@@ -1183,3 +1195,252 @@ def import_(self) -> Response:
)
command.run()
return self.response(200, message="OK")
+
+ @expose("/<id_or_uuid>/embedded", methods=("GET",))
+ @protect()
+ @safe
+ @permission_name("read")
+ @statsd_metrics
+ @event_logger.log_this_with_context(
+ action=lambda self, *args, **kwargs:
f"{self.__class__.__name__}.get_embedded",
+ log_to_statsd=False,
+ )
+ def get_embedded(self, id_or_uuid: str) -> Response:
+ """Get the chart's embedded configuration.
+ ---
+ get:
+ summary: Get the chart's embedded configuration
+ parameters:
+ - in: path
+ schema:
+ type: string
+ name: id_or_uuid
+ description: The chart id or uuid
+ responses:
+ 200:
+ description: Result contains the embedded chart config
+ content:
+ application/json:
+ schema:
+ type: object
+ properties:
+ result:
+ $ref:
'#/components/schemas/EmbeddedChartResponseSchema'
+ 401:
+ $ref: '#/components/responses/401'
+ 404:
+ $ref: '#/components/responses/404'
+ 500:
+ $ref: '#/components/responses/500'
+ """
+ try:
+ chart = ChartDAO.get_by_id_or_uuid(id_or_uuid)
+ except ChartNotFoundError:
+ return self.response_404()
+
+ if not chart.embedded:
+ return self.response(404)
+ embedded: EmbeddedChart = chart.embedded[0]
+ result = self.embedded_response_schema.dump(embedded)
+ return self.response(200, result=result)
+
+ @expose("/<id_or_uuid>/embedded", methods=["POST", "PUT"])
+ @protect()
+ @safe
+ @statsd_metrics
+ @event_logger.log_this_with_context(
+ action=lambda self, *args, **kwargs:
f"{self.__class__.__name__}.set_embedded",
+ log_to_statsd=False,
+ )
+ def set_embedded(self, id_or_uuid: str) -> Response:
+ """Set a chart's embedded configuration.
+ ---
+ post:
+ summary: Set a chart's embedded configuration
+ parameters:
+ - in: path
+ schema:
+ type: string
+ name: id_or_uuid
+ description: The chart id or uuid
+ requestBody:
+ description: The embedded configuration to set
+ required: true
+ content:
+ application/json:
+ schema: EmbeddedChartConfigSchema
+ responses:
+ 200:
+ description: Successfully set the configuration
+ content:
+ application/json:
+ schema:
+ type: object
+ properties:
+ result:
+ $ref:
'#/components/schemas/EmbeddedChartResponseSchema'
+ 401:
+ $ref: '#/components/responses/401'
+ 404:
+ $ref: '#/components/responses/404'
+ 500:
+ $ref: '#/components/responses/500'
+ put:
+ description: >-
+ Sets a chart's embedded configuration.
+ parameters:
+ - in: path
+ schema:
+ type: string
+ name: id_or_uuid
+ description: The chart id or uuid
+ requestBody:
+ description: The embedded configuration to set
+ required: true
+ content:
+ application/json:
+ schema: EmbeddedChartConfigSchema
+ responses:
+ 200:
+ description: Successfully set the configuration
+ content:
+ application/json:
+ schema:
+ type: object
+ properties:
+ result:
+ $ref:
'#/components/schemas/EmbeddedChartResponseSchema'
+ 401:
+ $ref: '#/components/responses/401'
+ 404:
+ $ref: '#/components/responses/404'
+ 500:
+ $ref: '#/components/responses/500'
+ """
+ try:
+ chart = ChartDAO.get_by_id_or_uuid(id_or_uuid)
+ except ChartNotFoundError:
+ return self.response_404()
+
+ try:
+ body = self.embedded_config_schema.load(request.json)
+
+ embedded = EmbeddedChartDAO.upsert(
+ chart,
+ body["allowed_domains"],
+ )
+ db.session.commit() # pylint: disable=consider-using-transaction
+
+ result = self.embedded_response_schema.dump(embedded)
+ return self.response(200, result=result)
+ except ValidationError as error:
+ db.session.rollback() # pylint: disable=consider-using-transaction
+ return self.response_400(message=error.messages)
+
Review Comment:
<div>
<div id="suggestion">
<div id="issue"><b>Missing Permission Decorator</b></div>
<div id="fix">
The set_embedded endpoint lacks a @permission_name decorator, unlike
get_embedded ('read') and delete_embedded ('set_embedded'). This could allow
unauthorized config changes if permissions aren't enforced elsewhere.
</div>
</div>
<details>
<summary><b>Citations</b></summary>
<ul>
<li>
Rule Violated: <a
href="https://github.com/apache/superset/blob/66ad889/AGENTS.md#L85";>AGENTS.md:85</a>
</li>
</ul>
</details>
<small><i>Code Review Run #4834de</i></small>
</div>
---
Should Bito avoid suggestions like this for future reviews? (<a
href=https://alpha.bito.ai/home/ai-agents/review-rules>Manage Rules</a>)
- [ ] Yes, avoid them
##########
superset/config.py:
##########
@@ -591,8 +593,6 @@ class D3TimeFormat(TypedDict, total=False):
"CACHE_IMPERSONATION": False,
# Enable caching per user key for Superset cache (not database cache
impersonation)
"CACHE_QUERY_BY_USER": False,
Review Comment:
<div>
<div id="suggestion">
<div id="issue"><b>Breaking change in stable feature flag</b></div>
<div id="fix">
Removing the stable EMBEDDABLE_CHARTS feature flag from
DEFAULT_FEATURE_FLAGS disables chart embedding by default, which is a breaking
change for users relying on the default configuration. According to
FEATURE_FLAGS.md and official docs, this flag is still stable and not
deprecated. If this removal is intentional, consider adding a deprecation
notice first.
</div>
</div>
<small><i>Code Review Run #4834de</i></small>
</div>
---
Should Bito avoid suggestions like this for future reviews? (<a
href=https://alpha.bito.ai/home/ai-agents/review-rules>Manage Rules</a>)
- [ ] Yes, avoid them
##########
superset-frontend/src/dashboard/components/EmbeddedChartModal/index.tsx:
##########
@@ -0,0 +1,289 @@
+/**
+ * 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 { useCallback, useEffect, useState } from 'react';
+import { logging, makeApi, SupersetApiError, t } from '@superset-ui/core';
+import { styled, css, Alert } from '@apache-superset/core/ui';
+import {
+ Button,
+ FormItem,
+ InfoTooltip,
+ Input,
+ Modal,
+ Loading,
+ Form,
+ Space,
+} from '@superset-ui/core/components';
+import { useToasts } from 'src/components/MessageToasts/withToasts';
+import { Typography } from '@superset-ui/core/components/Typography';
+import { ModalTitleWithIcon } from 'src/components/ModalTitleWithIcon';
+
+type Props = {
+ chartId: number;
+ formData: Record<string, unknown>;
+ show: boolean;
+ onHide: () => void;
+};
+
+type EmbeddedChart = {
+ uuid: string;
+ allowed_domains: string[];
+ chart_id: number;
+ changed_on: string;
+};
+
+type EmbeddedApiPayload = { allowed_domains: string[] };
+
+const stringToList = (stringyList: string): string[] =>
+ stringyList.split(/(?:\s|,)+/).filter(x => x);
+
+const ButtonRow = styled.div`
+ display: flex;
+ flex-direction: row;
+ justify-content: flex-end;
+`;
+
+export const ChartEmbedControls = ({ chartId, onHide }: Props) => {
+ const { addInfoToast, addDangerToast } = useToasts();
+ const [ready, setReady] = useState(true);
+ const [loading, setLoading] = useState(false);
+ const [embedded, setEmbedded] = useState<EmbeddedChart | null>(null);
+ const [allowedDomains, setAllowedDomains] = useState<string>('');
+ const [showDeactivateConfirm, setShowDeactivateConfirm] = useState(false);
+
+ const endpoint = `/api/v1/chart/${chartId}/embedded`;
+ const isDirty =
+ !embedded ||
+ stringToList(allowedDomains).join() !== embedded.allowed_domains.join();
+
+ const enableEmbedded = useCallback(() => {
+ setLoading(true);
+ makeApi<EmbeddedApiPayload, { result: EmbeddedChart }>({
+ method: 'POST',
+ endpoint,
+ })({
+ allowed_domains: stringToList(allowedDomains),
+ })
+ .then(
+ ({ result }) => {
+ setEmbedded(result);
+ setAllowedDomains(result.allowed_domains.join(', '));
+ addInfoToast(t('Changes saved.'));
+ },
+ err => {
+ logging.error(err);
+ addDangerToast(
+ t('Sorry, something went wrong. The changes could not be saved.'),
+ );
+ },
+ )
+ .finally(() => {
+ setLoading(false);
+ });
+ }, [endpoint, allowedDomains, addInfoToast, addDangerToast]);
+
+ const disableEmbedded = useCallback(() => {
+ setShowDeactivateConfirm(true);
+ }, []);
+
+ const confirmDeactivate = useCallback(() => {
+ setLoading(true);
+ makeApi<object>({ method: 'DELETE', endpoint })({})
+ .then(
+ () => {
+ setEmbedded(null);
+ setAllowedDomains('');
+ setShowDeactivateConfirm(false);
+ addInfoToast(t('Embedding deactivated.'));
+ onHide();
+ },
+ err => {
+ logging.error(err);
+ addDangerToast(
+ t(
+ 'Sorry, something went wrong. Embedding could not be
deactivated.',
+ ),
+ );
+ },
+ )
+ .finally(() => {
+ setLoading(false);
+ });
+ }, [endpoint, addInfoToast, addDangerToast, onHide]);
+
+ useEffect(() => {
+ setReady(false);
+ makeApi<object, { result: EmbeddedChart }>({
+ method: 'GET',
+ endpoint,
+ })({})
+ .catch(err => {
+ if ((err as SupersetApiError).status === 404) {
+ return { result: null };
+ }
+ addDangerToast(t('Sorry, something went wrong. Please try again.'));
+ throw err;
+ })
+ .then(({ result }) => {
+ setReady(true);
+ setEmbedded(result);
+ setAllowedDomains(result ? result.allowed_domains.join(', ') : '');
+ });
Review Comment:
<div>
<div id="suggestion">
<div id="issue"><b>Error Handling Issue</b></div>
<div id="fix">
The useEffect fails to set ready to true on API errors, potentially leaving
the component stuck in loading state. Add a .finally block to ensure ready is
always set.
</div>
<details>
<summary>
<b>Code suggestion</b>
</summary>
<blockquote>Check the AI-generated fix before applying</blockquote>
<div id="code">
```
@@ -141,6 +141,7 @@
.then(({ result }) => {
setReady(true);
setEmbedded(result);
setAllowedDomains(result ? result.allowed_domains.join(', ') : '');
})
+ .finally(() => setReady(true));
```
</div>
</details>
</div>
<small><i>Code Review Run #4834de</i></small>
</div>
---
Should Bito avoid suggestions like this for future reviews? (<a
href=https://alpha.bito.ai/home/ai-agents/review-rules>Manage Rules</a>)
- [ ] Yes, avoid them
--
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]
