Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(sqllab): reverts #22695 #22861

Merged
merged 1 commit into from
Jan 26, 2023
Merged

fix(sqllab): reverts #22695 #22861

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
14 changes: 1 addition & 13 deletions superset-frontend/src/SqlLab/actions/sqlLab.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -128,22 +128,10 @@ export function getUpToDateQuery(rootState, queryEditor, key) {
sqlLab: { unsavedQueryEditor },
} = rootState;
const id = key ?? queryEditor.id;
const updatedQueryEditor = {
return {
...queryEditor,
...(id === unsavedQueryEditor.id && unsavedQueryEditor),
};

if (
'schema' in updatedQueryEditor &&
!updatedQueryEditor.schemaOptions?.find(
({ value }) => value === updatedQueryEditor.schema,
)
) {
// remove the deprecated schema option
delete updatedQueryEditor.schema;
}

return updatedQueryEditor;
}

export function resetState() {
Expand Down
48 changes: 0 additions & 48 deletions superset-frontend/src/SqlLab/actions/sqlLab.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -316,54 +316,6 @@ describe('async actions', () => {
});
});

describe('getUpToDateQuery', () => {
const id = 'randomidvalue2';
const unsavedQueryEditor = {
id,
sql: 'some query here',
schemaOptions: [{ value: 'testSchema' }, { value: 'schema2' }],
};

it('returns payload with the updated queryEditor', () => {
const queryEditor = {
id,
name: 'Dummy query editor',
schema: 'testSchema',
};
const state = {
sqlLab: {
tabHistory: [id],
queryEditors: [queryEditor],
unsavedQueryEditor,
},
};
expect(actions.getUpToDateQuery(state, queryEditor)).toEqual({
...queryEditor,
...unsavedQueryEditor,
});
});

it('ignores the deprecated schema option', () => {
const queryEditor = {
id,
name: 'Dummy query editor',
schema: 'oldSchema',
};
const state = {
sqlLab: {
tabHistory: [id],
queryEditors: [queryEditor],
unsavedQueryEditor,
},
};
expect(actions.getUpToDateQuery(state, queryEditor)).toEqual({
...queryEditor,
...unsavedQueryEditor,
schema: undefined,
});
});
});

describe('postStopQuery', () => {
const stopQueryEndpoint = 'glob:*/api/v1/query/stop';
fetchMock.post(stopQueryEndpoint, {});
Expand Down
4 changes: 1 addition & 3 deletions superset-frontend/src/SqlLab/components/ShareSqlLabQuery/ShareSqlLabQuery.test.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -43,7 +43,6 @@ const mockQueryEditor = {
autorun: false,
sql: 'SELECT * FROM ...',
remoteId: 999,
schemaOptions: [{ value: 'query_schema' }, { value: 'query_schema_updated' }],
};
const disabled = {
id: 'disabledEditorId',
Expand Down Expand Up @@ -83,7 +82,6 @@ const standardProviderWithUnsaved: React.FC = ({ children }) => (
...initialState,
sqlLab: {
...initialState.sqlLab,
queryEditors: [mockQueryEditor],
unsavedQueryEditor,
},
})}
Expand Down Expand Up @@ -125,7 +123,7 @@ describe('ShareSqlLabQuery', () => {
});
});
const button = screen.getByRole('button');
const { id, remoteId, schemaOptions, ...expected } = mockQueryEditor;
const { id, remoteId, ...expected } = mockQueryEditor;
const storeQuerySpy = jest.spyOn(utils, 'storeQuery');
userEvent.click(button);
expect(storeQuerySpy.mock.calls).toHaveLength(1);
Expand Down
97 changes: 13 additions & 84 deletions superset-frontend/src/SqlLab/components/SqlEditorLeftBar/SqlEditorLeftBar.test.jsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,22 +41,15 @@ const middlewares = [thunk];
const mockStore = configureStore(middlewares);
const store = mockStore(initialState);

beforeEach(() => {
fetchMock.get('glob:*/api/v1/database/**', { result: [] });
fetchMock.get('glob:*/api/v1/database/*/schemas/?*', { result: [] });
fetchMock.get('glob:*/superset/tables/**', {
options: [
{
label: 'ab_user',
value: 'ab_user',
},
],
tableLength: 1,
});
});

afterEach(() => {
fetchMock.restore();
fetchMock.get('glob:*/api/v1/database/*/schemas/?*', { result: [] });
fetchMock.get('glob:*/superset/tables/**', {
options: [
{
label: 'ab_user',
value: 'ab_user',
},
],
tableLength: 1,
});

const renderAndWait = (props, store) =>
Expand Down Expand Up @@ -117,9 +110,8 @@ test('should toggle the table when the header is clicked', async () => {
userEvent.click(header);

await waitFor(() => {
expect(store.getActions()[store.getActions().length - 1].type).toEqual(
'COLLAPSE_TABLE',
);
expect(store.getActions()).toHaveLength(4);
expect(store.getActions()[3].type).toEqual('COLLAPSE_TABLE');
});
});

Expand All @@ -137,77 +129,14 @@ test('When changing database the table list must be updated', async () => {
database_name: 'new_db',
backend: 'postgresql',
}}
queryEditorId={defaultQueryEditor.id}
queryEditor={{ ...mockedProps.queryEditor, schema: 'new_schema' }}
tables={[{ ...mockedProps.tables[0], dbId: 2, name: 'new_table' }]}
/>,
{
useRedux: true,
store: mockStore({
...initialState,
sqlLab: {
...initialState.sqlLab,
unsavedQueryEditor: {
id: defaultQueryEditor.id,
schema: 'new_schema',
},
},
}),
initialState,
},
);
expect(await screen.findByText(/new_db/i)).toBeInTheDocument();
expect(await screen.findByText(/new_table/i)).toBeInTheDocument();
});

test('ignore schema api when current schema is deprecated', async () => {
const invalidSchemaName = 'None';
await renderAndWait(
mockedProps,
mockStore({
...initialState,
sqlLab: {
...initialState.sqlLab,
unsavedQueryEditor: {
id: defaultQueryEditor.id,
schema: invalidSchemaName,
},
},
}),
);

expect(await screen.findByText(/Database/i)).toBeInTheDocument();
expect(screen.queryByText(/None/i)).not.toBeInTheDocument();
expect(fetchMock.calls()).not.toContainEqual(
expect.arrayContaining([
expect.stringContaining(
`/tables/${mockedProps.database.id}/${invalidSchemaName}/`,
),
]),
);
});

test('fetches schema api when current schema is among the list', async () => {
const validSchema = 'schema1';
await renderAndWait(
mockedProps,
mockStore({
...initialState,
sqlLab: {
...initialState.sqlLab,
unsavedQueryEditor: {
id: defaultQueryEditor.id,
schema: validSchema,
schemaOptions: [{ name: validSchema, value: validSchema }],
},
},
}),
);

expect(await screen.findByText(/schema1/i)).toBeInTheDocument();
expect(fetchMock.calls()).toContainEqual(
expect.arrayContaining([
expect.stringContaining(
`/tables/${mockedProps.database.id}/${validSchema}/`,
),
]),
);
});
1 change: 1 addition & 0 deletions superset-frontend/src/SqlLab/components/SqlEditorLeftBar/index.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -117,6 +117,7 @@ const SqlEditorLeftBar = ({
}: SqlEditorLeftBarProps) => {
const dispatch = useDispatch();
const queryEditor = useQueryEditor(queryEditorId, ['dbId', 'schema']);

const [emptyResultsWithSearch, setEmptyResultsWithSearch] = useState(false);
const [userSelectedDb, setUserSelected] = useState<DatabaseObject | null>(
null,
Expand Down
33 changes: 2 additions & 31 deletions superset-frontend/src/SqlLab/hooks/useQueryEditor/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,6 @@
* specific language governing permissions and limitations
* under the License.
*/
import { useMemo } from 'react';
import pick from 'lodash/pick';
import { shallowEqual, useSelector } from 'react-redux';
import { SqlLabRootState, QueryEditor } from 'src/SqlLab/types';
Expand All @@ -25,43 +24,15 @@ export default function useQueryEditor<T extends keyof QueryEditor>(
sqlEditorId: string,
attributes: ReadonlyArray<T>,
) {
const queryEditor = useSelector<
SqlLabRootState,
Pick<QueryEditor, T | 'id' | 'schema'>
>(
return useSelector<SqlLabRootState, Pick<QueryEditor, T | 'id'>>(
({ sqlLab: { unsavedQueryEditor, queryEditors } }) =>
pick(
{
...queryEditors.find(({ id }) => id === sqlEditorId),
...(sqlEditorId === unsavedQueryEditor.id && unsavedQueryEditor),
},
['id'].concat(attributes),
) as Pick<QueryEditor, T | 'id' | 'schema'>,
) as Pick<QueryEditor, T | 'id'>,
shallowEqual,
);
const { schema, schemaOptions } = useSelector<
SqlLabRootState,
Pick<QueryEditor, 'schema' | 'schemaOptions'>
>(
({ sqlLab: { unsavedQueryEditor, queryEditors } }) =>
pick(
{
...queryEditors.find(({ id }) => id === sqlEditorId),
...(sqlEditorId === unsavedQueryEditor.id && unsavedQueryEditor),
},
['schema', 'schemaOptions'],
) as Pick<QueryEditor, T | 'schema' | 'schemaOptions'>,
shallowEqual,
);

const schemaOptionsMap = useMemo(
() => new Set(schemaOptions?.map(({ value }) => value)),
[schemaOptions],
);

if ('schema' in queryEditor && schema && !schemaOptionsMap.has(schema)) {
delete queryEditor.schema;
}

return queryEditor as Pick<QueryEditor, T | 'id'>;
}
28 changes: 1 addition & 27 deletions superset-frontend/src/SqlLab/hooks/useQueryEditor/useQueryEditor.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -70,7 +70,7 @@ test('includes id implicitly', () => {
test('returns updated values from unsaved change', () => {
const expectedSql = 'SELECT updated_column\nFROM updated_table\nWHERE';
const { result } = renderHook(
() => useQueryEditor(defaultQueryEditor.id, ['id', 'sql', 'schema']),
() => useQueryEditor(defaultQueryEditor.id, ['id', 'sql']),
{
wrapper: createWrapper({
useRedux: true,
Expand All @@ -88,31 +88,5 @@ test('returns updated values from unsaved change', () => {
},
);
expect(result.current.id).toEqual(defaultQueryEditor.id);
expect(result.current.schema).toEqual(defaultQueryEditor.schema);
expect(result.current.sql).toEqual(expectedSql);
});

test('skips the deprecated schema option', () => {
const expectedSql = 'SELECT updated_column\nFROM updated_table\nWHERE';
const { result } = renderHook(
() => useQueryEditor(defaultQueryEditor.id, ['schema']),
{
wrapper: createWrapper({
useRedux: true,
store: mockStore({
...initialState,
sqlLab: {
...initialState.sqlLab,
unsavedQueryEditor: {
id: defaultQueryEditor.id,
sql: expectedSql,
schema: 'deprecated schema',
},
},
}),
}),
},
);
expect(result.current.schema).not.toEqual(defaultQueryEditor.schema);
expect(result.current.schema).toBeUndefined();
});
2 changes: 1 addition & 1 deletion superset-frontend/src/SqlLab/types.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,7 +35,7 @@ export interface QueryEditor {
id: string;
dbId?: number;
name: string;
schema?: string;
schema: string;
autorun: boolean;
sql: string;
remoteId: number | null;
Expand Down