korbit-ai[bot] commented on code in PR #34762: URL: https://github.com/apache/superset/pull/34762#discussion_r2286146525
########## superset/migrations/versions/2025-08-13_19-09_da125679ebb8_fix_table_chart_conditional_formatting_.py: ########## @@ -0,0 +1,93 @@ +# 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. +"""fix_table_chart_conditional_formatting_add_flag + +Revision ID: da125679ebb8 +Revises: c233f5365c9e +Create Date: 2025-08-13 19:09:41.796801 + +""" + +# revision identifiers, used by Alembic. +revision = "da125679ebb8" +down_revision = "c233f5365c9e" + + +from alembic import op +from sqlalchemy import Column, Integer, String, Text +from sqlalchemy.ext.declarative import declarative_base + +from superset import db +from superset.utils import json + +Base = declarative_base() + + +class Slice(Base): + __tablename__ = "slices" + id = Column(Integer, primary_key=True) + viz_type = Column(String(250)) + params = Column(Text) + + +def upgrade(): + bind = op.get_bind() + session = db.Session(bind=bind) + + for slc in session.query(Slice).filter(Slice.viz_type == "table"): + params = json.loads(slc.params) + conditional_formatting = params.get("conditional_formatting", []) Review Comment: ### Unsafe JSON Deserialization <sub></sub> <details> <summary>Tell me more</summary> ###### What is the issue? Direct deserialization of untrusted JSON data from the database without validation of structure or content. ###### Why this matters Maliciously crafted JSON data could potentially contain unexpected structures or values that could lead to application errors or potential security issues during processing. ###### Suggested change ∙ *Feature Preview* ```python # Add schema validation before processing the JSON data from marshmallow import Schema, fields class FormattingSchema(Schema): colorScheme = fields.Str(required=True) # Add other expected fields try: params = json.loads(slc.params) conditional_formatting = params.get("conditional_formatting", []) # Validate each formatter schema = FormattingSchema() for formatter in conditional_formatting: schema.load(formatter) except (json.JSONDecodeError, ValidationError) as e: logger.error(f"Invalid formatting data for slice {slc.id}: {e}") continue ``` ###### Provide feedback to improve future suggestions [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/237e14f9-fee1-4522-885c-2b2646d97a59/upvote) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/237e14f9-fee1-4522-885c-2b2646d97a59?what_not_true=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/237e14f9-fee1-4522-885c-2b2646d97a59?what_out_of_scope=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/237e14f9-fee1-4522-885c-2b2646d97a59?what_not_in_standard=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/237e14f9-fee1-4522-885c-2b2646d97a59) </details> <sub> 💬 Looking for more details? Reply to this comment to chat with Korbit. </sub> <!--- korbi internal id:6892fbac-339c-4c8e-b61d-6eedc074eb0f --> [](6892fbac-339c-4c8e-b61d-6eedc074eb0f) ########## superset-frontend/src/explore/components/controls/ConditionalFormattingControl/FormattingPopoverContent.tsx: ########## @@ -251,6 +260,28 @@ export const FormattingPopoverContent = ({ (config?.colorScheme !== ColorSchemeEnum.Green && config?.colorScheme !== ColorSchemeEnum.Red), ); + + const [toAllRow, setToAllRow] = useState(() => Boolean(config?.toAllRow)); + const [toTextColor, setToTextColor] = useState(() => + Boolean(config?.toTextColor), + ); + + const showToAllRow = useMemo( + () => + conditionalFormattingFlag && conditionalFormattingFlag.toAllRowCheck + ? config?.toAllRow === undefined + : config?.toAllRow !== undefined, + [conditionalFormattingFlag, config], + ); Review Comment: ### Inverted Conditional Logic for Row Formatting <sub></sub> <details> <summary>Tell me more</summary> ###### What is the issue? The logic for showToAllRow is inverted. When toAllRowCheck is true, it hides the checkbox when toAllRow is defined, which is opposite to the intended behavior. ###### Why this matters This will prevent users from seeing the 'Apply to entire row' option when they should, making the feature unusable in certain conditions. ###### Suggested change ∙ *Feature Preview* Correct the logic by inverting the condition: ```typescript const showToAllRow = useMemo( () => conditionalFormattingFlag && conditionalFormattingFlag.toAllRowCheck ? config?.toAllRow !== undefined : config?.toAllRow === undefined, [conditionalFormattingFlag, config], ); ``` ###### Provide feedback to improve future suggestions [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/9534b984-63cf-4afe-8a5c-72a009f645cf/upvote) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/9534b984-63cf-4afe-8a5c-72a009f645cf?what_not_true=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/9534b984-63cf-4afe-8a5c-72a009f645cf?what_out_of_scope=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/9534b984-63cf-4afe-8a5c-72a009f645cf?what_not_in_standard=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/9534b984-63cf-4afe-8a5c-72a009f645cf) </details> <sub> 💬 Looking for more details? Reply to this comment to chat with Korbit. </sub> <!--- korbi internal id:db823faa-8388-4ae7-8f22-6131b690427d --> [](db823faa-8388-4ae7-8f22-6131b690427d) ########## superset/migrations/versions/2025-08-13_19-09_da125679ebb8_fix_table_chart_conditional_formatting_.py: ########## @@ -0,0 +1,93 @@ +# 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. +"""fix_table_chart_conditional_formatting_add_flag + +Revision ID: da125679ebb8 +Revises: c233f5365c9e +Create Date: 2025-08-13 19:09:41.796801 + +""" + +# revision identifiers, used by Alembic. +revision = "da125679ebb8" +down_revision = "c233f5365c9e" + + +from alembic import op +from sqlalchemy import Column, Integer, String, Text +from sqlalchemy.ext.declarative import declarative_base + +from superset import db +from superset.utils import json + +Base = declarative_base() + + +class Slice(Base): + __tablename__ = "slices" + id = Column(Integer, primary_key=True) + viz_type = Column(String(250)) + params = Column(Text) + + +def upgrade(): + bind = op.get_bind() + session = db.Session(bind=bind) + + for slc in session.query(Slice).filter(Slice.viz_type == "table"): + params = json.loads(slc.params) + conditional_formatting = params.get("conditional_formatting", []) + if conditional_formatting: + new_conditional_formatting = [] + for formatter in conditional_formatting: + color_scheme = formatter.get("colorScheme") + + if color_scheme not in ["Green", "Red"]: + new_conditional_formatting.append( + {**formatter, "toAllRow": False, "toTextColor": False} + ) + else: + new_conditional_formatting.append(formatter) + params["conditional_formatting"] = new_conditional_formatting + slc.params = json.dumps(params) + session.commit() Review Comment: ### Inefficient Transaction Management <sub></sub> <details> <summary>Tell me more</summary> ###### What is the issue? The code commits transactions inside a loop for each record update, causing unnecessary database round trips. ###### Why this matters Multiple individual commits create excessive database I/O and can significantly slow down the migration, especially with large datasets. ###### Suggested change ∙ *Feature Preview* Batch the commits by moving the session.commit() outside the loop: ```python for slc in session.query(Slice).filter(Slice.viz_type == "table"): # ... processing ... session.commit() # Single commit after all updates ``` ###### Provide feedback to improve future suggestions [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/420774a0-ee9e-4cf3-97e9-e73ba865a35b/upvote) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/420774a0-ee9e-4cf3-97e9-e73ba865a35b?what_not_true=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/420774a0-ee9e-4cf3-97e9-e73ba865a35b?what_out_of_scope=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/420774a0-ee9e-4cf3-97e9-e73ba865a35b?what_not_in_standard=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/420774a0-ee9e-4cf3-97e9-e73ba865a35b) </details> <sub> 💬 Looking for more details? Reply to this comment to chat with Korbit. </sub> <!--- korbi internal id:ec33f6f9-db98-4e9e-a3d9-32823c194f4e --> [](ec33f6f9-db98-4e9e-a3d9-32823c194f4e) -- 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]
