korbit-ai[bot] commented on code in PR #36086: URL: https://github.com/apache/superset/pull/36086#discussion_r2522916728
########## superset/migrations/versions/2025-11-12_12-54_x2s8ocx6rto6_expand_username_field_to_128_chars.py: ########## @@ -0,0 +1,97 @@ +# 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. +"""Expand username field to 128 chars + +Revision ID: x2s8ocx6rto6 +Revises: c233f5365c9e +Create Date: 2025-11-12 12:54:00.000000 + +""" + +import logging + +import sqlalchemy as sa +from alembic import op + +from superset.migrations.shared.utils import get_table_column + +logger = logging.getLogger("alembic.env") + +# revision identifiers, used by Alembic. +revision = "x2s8ocx6rto6" +down_revision = "c233f5365c9e" + + +def upgrade(): + """Expand ab_user.username field from 64 to 128 characters.""" + # Check current column length to avoid errors if already upgraded + column_info = get_table_column("ab_user", "username") Review Comment: ### Duplicate Column Check Logic <sub></sub> <details> <summary>Tell me more</summary> ###### What is the issue? The code for checking column information and handling migration logic is duplicated between upgrade() and downgrade() functions. ###### Why this matters Code duplication increases maintenance burden and risk of inconsistencies when changes are needed. If the column checking logic needs to be modified, it would require changes in multiple places. ###### Suggested change ∙ *Feature Preview* Extract the common column checking logic into a helper function: ```python def get_username_column_length(): column_info = get_table_column("ab_user", "username") if column_info is None: return None current_type = column_info.get("type") return getattr(current_type, "length", None) ``` ###### Provide feedback to improve future suggestions [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/b8ef20e1-7234-4c02-8b36-e7f6fe0132e9/upvote) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/b8ef20e1-7234-4c02-8b36-e7f6fe0132e9?what_not_true=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/b8ef20e1-7234-4c02-8b36-e7f6fe0132e9?what_out_of_scope=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/b8ef20e1-7234-4c02-8b36-e7f6fe0132e9?what_not_in_standard=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/b8ef20e1-7234-4c02-8b36-e7f6fe0132e9) </details> <sub> 💬 Looking for more details? Reply to this comment to chat with Korbit. </sub> <!--- korbi internal id:6e42a80f-ebb5-45cb-a183-6f335ebc5dac --> [](6e42a80f-ebb5-45cb-a183-6f335ebc5dac) ########## superset/migrations/versions/2025-11-12_12-54_x2s8ocx6rto6_expand_username_field_to_128_chars.py: ########## @@ -0,0 +1,97 @@ +# 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. +"""Expand username field to 128 chars + +Revision ID: x2s8ocx6rto6 +Revises: c233f5365c9e +Create Date: 2025-11-12 12:54:00.000000 + +""" + +import logging + +import sqlalchemy as sa +from alembic import op + +from superset.migrations.shared.utils import get_table_column + +logger = logging.getLogger("alembic.env") + +# revision identifiers, used by Alembic. +revision = "x2s8ocx6rto6" +down_revision = "c233f5365c9e" + + +def upgrade(): + """Expand ab_user.username field from 64 to 128 characters.""" + # Check current column length to avoid errors if already upgraded + column_info = get_table_column("ab_user", "username") + + if column_info is None: + logger.warning("Column ab_user.username not found. Skipping migration.") + return + + # Get current length - check the type attribute + current_type = column_info.get("type") + current_length = getattr(current_type, "length", None) + + if current_length is not None and current_length == 128: + logger.info( + "Column ab_user.username already has length %s. Skipping migration.", + current_length, + ) + return + + with op.batch_alter_table("ab_user") as batch_op: + batch_op.alter_column( + "username", + existing_type=sa.String(current_length) + if current_length + else sa.String(64), + type_=sa.String(128), + existing_nullable=False, + ) + + +def downgrade(): + """Revert ab_user.username field from 128 to 64 characters.""" + # Check current column length + column_info = get_table_column("ab_user", "username") + + if column_info is None: + logger.warning("Column ab_user.username not found. Skipping downgrade.") + return + + current_type = column_info.get("type") + current_length = getattr(current_type, "length", None) + + if current_length is not None and current_length <= 64: Review Comment: ### Incorrect downgrade skip condition <sub></sub> <details> <summary>Tell me more</summary> ###### What is the issue? The downgrade function uses '<= 64' condition which will skip the downgrade when the column length is exactly 64, but it should only skip when the length is already less than 64. ###### Why this matters This prevents the downgrade from running when it should, making the migration non-reversible in cases where the column is exactly 64 characters. ###### Suggested change ∙ *Feature Preview* Change the condition to check only for lengths less than 64: ```python if current_length is not None and current_length < 64: ``` ###### Provide feedback to improve future suggestions [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/9369e32a-184c-4637-859d-3b4882a5d7f1/upvote) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/9369e32a-184c-4637-859d-3b4882a5d7f1?what_not_true=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/9369e32a-184c-4637-859d-3b4882a5d7f1?what_out_of_scope=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/9369e32a-184c-4637-859d-3b4882a5d7f1?what_not_in_standard=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/9369e32a-184c-4637-859d-3b4882a5d7f1) </details> <sub> 💬 Looking for more details? Reply to this comment to chat with Korbit. </sub> <!--- korbi internal id:cc62ae88-c743-4bc0-b824-766109a0da54 --> [](cc62ae88-c743-4bc0-b824-766109a0da54) ########## superset/migrations/versions/2025-11-12_12-54_x2s8ocx6rto6_expand_username_field_to_128_chars.py: ########## @@ -0,0 +1,97 @@ +# 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. +"""Expand username field to 128 chars + +Revision ID: x2s8ocx6rto6 +Revises: c233f5365c9e +Create Date: 2025-11-12 12:54:00.000000 + +""" + +import logging + +import sqlalchemy as sa +from alembic import op + +from superset.migrations.shared.utils import get_table_column + +logger = logging.getLogger("alembic.env") + +# revision identifiers, used by Alembic. +revision = "x2s8ocx6rto6" +down_revision = "c233f5365c9e" + + +def upgrade(): + """Expand ab_user.username field from 64 to 128 characters.""" + # Check current column length to avoid errors if already upgraded + column_info = get_table_column("ab_user", "username") + + if column_info is None: + logger.warning("Column ab_user.username not found. Skipping migration.") + return + + # Get current length - check the type attribute + current_type = column_info.get("type") + current_length = getattr(current_type, "length", None) + + if current_length is not None and current_length == 128: Review Comment: ### Incomplete upgrade skip condition <sub></sub> <details> <summary>Tell me more</summary> ###### What is the issue? The upgrade function uses '== 128' condition which will not skip the migration if the column length is greater than 128, potentially causing issues with larger existing column sizes. ###### Why this matters This could lead to unnecessary schema changes or errors when the column is already larger than the target size of 128 characters. ###### Suggested change ∙ *Feature Preview* Change the condition to check for lengths greater than or equal to 128: ```python if current_length is not None and current_length >= 128: ``` ###### Provide feedback to improve future suggestions [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/9a1ca954-e59b-4a4d-bd5b-a1878c4b9fef/upvote) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/9a1ca954-e59b-4a4d-bd5b-a1878c4b9fef?what_not_true=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/9a1ca954-e59b-4a4d-bd5b-a1878c4b9fef?what_out_of_scope=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/9a1ca954-e59b-4a4d-bd5b-a1878c4b9fef?what_not_in_standard=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/9a1ca954-e59b-4a4d-bd5b-a1878c4b9fef) </details> <sub> 💬 Looking for more details? Reply to this comment to chat with Korbit. </sub> <!--- korbi internal id:0f3b5487-fa71-4448-b112-da517712d953 --> [](0f3b5487-fa71-4448-b112-da517712d953) -- 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]
