Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -127,6 +127,28 @@ describe('SaveDatasetModal', () => {
expect(screen.getByRole('button', { name: /save/i })).toBeInTheDocument();
});

test('disables the save button when the dataset name is empty or whitespace-only', async () => {
renderModal();

const nameInput = screen.getByRole('textbox');
const saveBtn = screen.getByRole('button', { name: /save/i });

// Default name is present, so save starts enabled
expect(saveBtn).toBeEnabled();

// Clearing the name disables save
await userEvent.clear(nameInput);
await waitFor(() => expect(saveBtn).toBeDisabled());

// Whitespace-only name keeps save disabled
await userEvent.type(nameInput, ' ');
await waitFor(() => expect(saveBtn).toBeDisabled());

// A non-empty name re-enables save
await userEvent.type(nameInput, 'My dataset');
await waitFor(() => expect(saveBtn).toBeEnabled());
});

test('renders an overwrite button when "Overwrite existing" is selected', () => {
renderModal();

Expand Down Expand Up @@ -245,6 +267,22 @@ describe('SaveDatasetModal', () => {
});
});

test('trims surrounding whitespace from the dataset name on save', async () => {
renderModal();

const inputFieldText = screen.getByDisplayValue(/unimportant/i);
fireEvent.change(inputFieldText, { target: { value: ' my dataset ' } });

const saveConfirmationBtn = screen.getByRole('button', {
name: /save/i,
});
userEvent.click(saveConfirmationBtn);

expect(createDatasource).toHaveBeenCalledWith(
expect.objectContaining({ datasourceName: 'my dataset' }),
);
});

test('sends the catalog when creating the dataset', async () => {
renderModal({
datasource: { ...mockedProps.datasource, catalog: 'public' },
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -371,7 +371,7 @@ export const SaveDatasetModal = ({
catalog: datasource?.catalog ?? null,
schema: datasource?.schema ?? '',
templateParams,
datasourceName: datasetName,
datasourceName: datasetName.trim(),
}),
)
.then((data: { id: number }) => {
Expand Down Expand Up @@ -417,7 +417,7 @@ export const SaveDatasetModal = ({

const disableSaveAndExploreBtn =
(newOrOverwrite === DatasetRadioState.SaveNew &&
datasetName.length === 0) ||
datasetName.trim().length === 0) ||
(newOrOverwrite === DatasetRadioState.OverwriteDataset &&
isEmpty(selectedDatasetToOverwrite));

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -337,4 +337,32 @@ describe('SavedQuery', () => {
).not.toBeInTheDocument();
});
});

test('disables the save button when the query name is empty or whitespace-only', async () => {
render(<SaveQuery {...mockedProps} />, {
useRedux: true,
store: mockStore(mockState),
});

userEvent.click(screen.getByRole('button', { name: /save/i }));

const nameInput = screen.getAllByRole('textbox')[0] as HTMLInputElement;
const modalSaveBtn = () =>
screen.getAllByRole('button', { name: /save/i })[1];

// Default label is present, so the save button starts enabled
expect(modalSaveBtn()).toBeEnabled();

// Clearing the name disables the save button
userEvent.clear(nameInput);
await waitFor(() => expect(modalSaveBtn()).toBeDisabled());

// A whitespace-only name keeps the save button disabled
userEvent.type(nameInput, ' ');
await waitFor(() => expect(modalSaveBtn()).toBeDisabled());

// A non-empty name re-enables the save button
userEvent.type(nameInput, 'My query');
await waitFor(() => expect(modalSaveBtn()).toBeEnabled());
});
});
9 changes: 8 additions & 1 deletion superset-frontend/src/SqlLab/components/SaveQuery/index.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -112,6 +112,7 @@ const SaveQuery = ({
const [showSave, setShowSave] = useState<boolean>(false);
const [showSaveDatasetModal, setShowSaveDatasetModal] = useState(false);
const isSaved = !!query.remoteId;
const isLabelEmpty = label.trim().length === 0;
const canExploreDatabase = !!database?.allows_virtual_table_explore;
const shouldShowSaveButton =
database?.allows_virtual_table_explore !== undefined;
Expand Down Expand Up @@ -235,12 +236,18 @@ const SaveQuery = ({
<Button
buttonStyle={isSaved ? 'secondary' : 'primary'}
onClick={onSaveWrapper}
disabled={isLabelEmpty}
cta
>
{isSaved ? t('Save as new') : t('Save')}
</Button>
{isSaved && (
<Button buttonStyle="primary" onClick={onUpdateWrapper} cta>
<Button
buttonStyle="primary"
onClick={onUpdateWrapper}
disabled={isLabelEmpty}
cta
>
{t('Update')}
</Button>
)}
Expand Down
5 changes: 5 additions & 0 deletions superset/queries/saved_queries/api.py
Original file line number Diff line number Diff line change
Expand Up @@ -52,6 +52,7 @@
get_delete_ids_schema,
get_export_ids_schema,
openapi_spec_methods_override,
validate_label,
)
from superset.utils import json
from superset.utils.core import sanitize_cookie_token
Expand Down Expand Up @@ -193,8 +194,12 @@ class SavedQueryRestApi(BaseSupersetModelRestApi):
allowed_rel_fields = {"database", "changed_by", "created_by"}
allowed_distinct_fields = {"catalog", "schema"}

validators_columns = {"label": validate_label}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggestion: Add an explicit type annotation to this newly introduced class-level variable to comply with the requirement for annotating relevant variables. [custom_rule]

Severity Level: Minor ⚠️

Why it matters? 🤔

This is newly added Python code and the class-level variable is unannotated. The rule requires type hints for relevant variables that can be annotated, so this is a real violation.

Fix in Cursor Fix in VSCode Claude

(Use Cmd/Ctrl + Click for best experience)

Prompt for AI Agent 🤖
This is a comment left during a code review.

**Path:** superset/queries/saved_queries/api.py
**Line:** 197:197
**Comment:**
	*Custom Rule: Add an explicit type annotation to this newly introduced class-level variable to comply with the requirement for annotating relevant variables.

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.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix
👍 | 👎


def pre_add(self, item: SavedQuery) -> None:
item.user = g.user
if item.label:
item.label = item.label.strip()

def pre_update(self, item: SavedQuery) -> None:
self.pre_add(item)
Expand Down
10 changes: 9 additions & 1 deletion superset/queries/saved_queries/schemas.py
Original file line number Diff line number Diff line change
Expand Up @@ -14,9 +14,17 @@
# KIND, either express or implied. See the License for the
# specific language governing permissions and limitations
# under the License.
from marshmallow import fields, Schema
from flask_babel import lazy_gettext as _
from marshmallow import fields, Schema, ValidationError
from marshmallow.validate import Length


def validate_label(value: str | None) -> None:
"""Reject blank or whitespace-only saved query labels."""
if value is None or not value.strip():
raise ValidationError(_("Label must not be empty."))


openapi_spec_methods_override = {
"get": {"get": {"summary": "Get a saved query"}},
"get_list": {
Expand Down
63 changes: 63 additions & 0 deletions tests/integration_tests/queries/saved_queries/api_tests.py
Original file line number Diff line number Diff line change
Expand Up @@ -677,6 +677,69 @@ def test_create_saved_query(self):
db.session.delete(model)
db.session.commit()

def test_create_saved_query_blank_label(self) -> None:
"""
Saved Query API: Test create rejects blank/whitespace-only labels
"""
example_db = get_example_database()
self.login(ADMIN_USERNAME)
uri = "api/v1/saved_query/"

for label in ("", " ", "\t\n"):
post_data = {
"schema": "schema1",
"label": label,
"description": "some description",
"sql": "SELECT col1, col2 from table1",
"db_id": example_db.id,
}
Comment on lines +689 to +695

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggestion: Add an explicit type annotation for this request payload dictionary to satisfy the type-hint requirement for relevant local variables. [custom_rule]

Severity Level: Minor ⚠️

Why it matters? 🤔

The new test code introduces a local dictionary without any type annotation, and the rule explicitly requires type hints for relevant variables that can be annotated.

Fix in Cursor Fix in VSCode Claude

(Use Cmd/Ctrl + Click for best experience)

Prompt for AI Agent 🤖
This is a comment left during a code review.

**Path:** tests/integration_tests/queries/saved_queries/api_tests.py
**Line:** 689:695
**Comment:**
	*Custom Rule: Add an explicit type annotation for this request payload dictionary to satisfy the type-hint requirement for relevant local variables.

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.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix
👍 | 👎

rv = self.client.post(uri, json=post_data)
data = json.loads(rv.data.decode("utf-8"))
assert rv.status_code == 422
assert "label" in data["message"]

def test_create_saved_query_strips_label(self) -> None:
"""
Saved Query API: Test create trims surrounding whitespace from labels
"""
example_db = get_example_database()
self.login(ADMIN_USERNAME)
uri = "api/v1/saved_query/"

post_data = {
"schema": "schema1",
"label": " label1 ",
"description": "some description",
"sql": "SELECT col1, col2 from table1",
"db_id": example_db.id,
}
Comment on lines +709 to +715

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggestion: Add a concrete type annotation for this local dictionary used in the test input so the new code fully complies with required typing. [custom_rule]

Severity Level: Minor ⚠️

Why it matters? 🤔

This is another newly added local dictionary in Python code with no type hint, which matches the rule’s requirement to annotate relevant variables in modified code.

Fix in Cursor Fix in VSCode Claude

(Use Cmd/Ctrl + Click for best experience)

Prompt for AI Agent 🤖
This is a comment left during a code review.

**Path:** tests/integration_tests/queries/saved_queries/api_tests.py
**Line:** 709:715
**Comment:**
	*Custom Rule: Add a concrete type annotation for this local dictionary used in the test input so the new code fully complies with required typing.

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.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix
👍 | 👎

rv = self.client.post(uri, json=post_data)
data = json.loads(rv.data.decode("utf-8"))
assert rv.status_code == 201

model = db.session.query(SavedQuery).get(data.get("id"))
assert model.label == "label1"

# Rollback changes
db.session.delete(model)
db.session.commit()

@pytest.mark.usefixtures("create_saved_queries")
def test_update_saved_query_blank_label(self) -> None:
"""
Saved Query API: Test update rejects blank/whitespace-only labels
"""
saved_query = (
db.session.query(SavedQuery).filter(SavedQuery.label == "label1").all()[0]
)
Comment on lines +732 to +734

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggestion: Add an explicit type annotation for this ORM model variable to comply with the rule requiring type hints on relevant variables in new code. [custom_rule]

Severity Level: Minor ⚠️

Why it matters? 🤔

The new local ORM result variable is unannotated, and it is a relevant variable that can be type-hinted under the stated rule.

Fix in Cursor Fix in VSCode Claude

(Use Cmd/Ctrl + Click for best experience)

Prompt for AI Agent 🤖
This is a comment left during a code review.

**Path:** tests/integration_tests/queries/saved_queries/api_tests.py
**Line:** 732:734
**Comment:**
	*Custom Rule: Add an explicit type annotation for this ORM model variable to comply with the rule requiring type hints on relevant variables in new code.

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.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix
👍 | 👎

self.login(ADMIN_USERNAME)
uri = f"api/v1/saved_query/{saved_query.id}"

rv = self.client.put(uri, json={"label": " "})
data = json.loads(rv.data.decode("utf-8"))
assert rv.status_code == 422
assert "label" in data["message"]

@pytest.mark.usefixtures("create_saved_queries")
def test_update_saved_query(self):
"""
Expand Down
32 changes: 32 additions & 0 deletions tests/unit_tests/queries/saved_queries_schema_test.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,32 @@
# 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 pytest
from marshmallow import ValidationError

from superset.queries.saved_queries.schemas import validate_label


@pytest.mark.parametrize("value", ["", " ", "\t\n", None])
def test_validate_label_rejects_blank(value: str | None) -> None:
with pytest.raises(ValidationError):
validate_label(value)


@pytest.mark.parametrize("value", ["my query", " padded "])
def test_validate_label_accepts_non_blank(value: str) -> None:
# Should not raise for labels containing non-whitespace characters.
validate_label(value)
Loading