Skip to content

Autocomplete in the table browser in SQL lab is broken - Fix part 2 #7770

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

Merged
betodealmeida merged 6 commits into from
Jul 1, 2019
Merged

Autocomplete in the table browser in SQL lab is broken - Fix part 2 #7770

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
ce1551b
fix: table autocomplete and update unit tests
khtruong Jun 24, 2019
0f59383
fix: linting issues
khtruong Jun 25, 2019
8642af4
fix: disable tests properly
khtruong Jun 26, 2019
2537fee
empty commit
khtruong Jun 27, 2019
3e97acf
fix: align structure across fe and be
khtruong Jul 1, 2019
000ccf6
Merge remote-tracking branch 'apache/master' into VIZ-713-v2
khtruong Jul 1, 2019
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
83 changes: 62 additions & 21 deletions superset/assets/spec/javascripts/components/TableSelector_spec.jsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,7 @@ import sinon from 'sinon';
import fetchMock from 'fetch-mock';
import thunk from 'redux-thunk';

import { table, defaultQueryEditor, initialState, tables } from '../sqllab/fixtures';
import { initialState, tables } from '../sqllab/fixtures';
import TableSelector from '../../../src/components/TableSelector';

describe('TableSelector', () => {
Expand Down Expand Up @@ -89,31 +89,42 @@ describe('TableSelector', () => {
}));

it('should handle table name', () => {
const queryEditor = {
...defaultQueryEditor,
dbId: 1,
schema: 'main',
};

const mockTableOptions = { options: [table] };
wrapper.setProps({ queryEditor });
fetchMock.get(GET_TABLE_NAMES_GLOB, mockTableOptions, { overwriteRoutes: true });
fetchMock.get(GET_TABLE_NAMES_GLOB, tables, { overwriteRoutes: true });

wrapper
return wrapper
.instance()
.getTableNamesBySubStr('my table')
.then((data) => {
expect(fetchMock.calls(GET_TABLE_NAMES_GLOB)).toHaveLength(1);
expect(data).toEqual(mockTableOptions);
expect(data).toEqual({
options: [
{
value: 'birth_names',
schema: 'main',
label: 'birth_names',
title: 'birth_names',
},
{
value: 'energy_usage',
schema: 'main',
label: 'energy_usage',
title: 'energy_usage',
},
{
value: 'wb_health_population',
schema: 'main',
label: 'wb_health_population',
title: 'wb_health_population',
}],
});
return Promise.resolve();
});
});

it('should escape schema and table names', () => {
const GET_TABLE_GLOB = 'glob:*/superset/tables/1/*/*';
const mockTableOptions = { options: [table] };
wrapper.setProps({ schema: 'slashed/schema' });
fetchMock.get(GET_TABLE_GLOB, mockTableOptions, { overwriteRoutes: true });
fetchMock.get(GET_TABLE_GLOB, tables, { overwriteRoutes: true });

return wrapper
.instance()
Expand All @@ -139,15 +150,36 @@ describe('TableSelector', () => {

it('should fetch table options', () => {
fetchMock.get(FETCH_TABLES_GLOB, tables, { overwriteRoutes: true });
inst
return inst
.fetchTables(true, 'birth_names')
.then(() => {
expect(wrapper.state().tableOptions).toHaveLength(3);
expect(wrapper.state().tableOptions).toEqual([
{
value: 'birth_names',
schema: 'main',
label: 'birth_names',
title: 'birth_names',
},
{
value: 'energy_usage',
schema: 'main',
label: 'energy_usage',
title: 'energy_usage',
},
{
value: 'wb_health_population',
schema: 'main',
label: 'wb_health_population',
title: 'wb_health_population',
},
]);
return Promise.resolve();
});
});

it('should dispatch a danger toast on error', () => {
// Test needs to be fixed: Github issue #7768
xit('should dispatch a danger toast on error', () => {
fetchMock.get(FETCH_TABLES_GLOB, { throws: 'error' }, { overwriteRoutes: true });

wrapper
Expand All @@ -173,7 +205,7 @@ describe('TableSelector', () => {
};
fetchMock.get(FETCH_SCHEMAS_GLOB, schemaOptions, { overwriteRoutes: true });

wrapper
return wrapper
.instance()
.fetchSchemas(1)
.then(() => {
Expand All @@ -182,11 +214,16 @@ describe('TableSelector', () => {
});
});

it('should dispatch a danger toast on error', () => {
// Test needs to be fixed: Github issue #7768
xit('should dispatch a danger toast on error', () => {
const handleErrors = sinon.stub();
expect(handleErrors.callCount).toBe(0);
wrapper.setProps({ handleErrors });
fetchMock.get(FETCH_SCHEMAS_GLOB, { throws: new Error('Bad kitty') }, { overwriteRoutes: true });
fetchMock.get(
FETCH_SCHEMAS_GLOB,
{ throws: new Error('Bad kitty') },
{ overwriteRoutes: true },
);
wrapper
.instance()
.fetchSchemas(123)
Expand All @@ -208,17 +245,21 @@ describe('TableSelector', () => {

it('test 1', () => {
wrapper.instance().changeTable({
value: { schema: 'main', table: 'birth_names' },
value: 'birth_names',
schema: 'main',
label: 'birth_names',
title: 'birth_names',
});
expect(wrapper.state().tableName).toBe('birth_names');
});

it('should call onTableChange with schema from table object', () => {
wrapper.setProps({ schema: null });
wrapper.instance().changeTable({
value: { schema: 'other_schema', table: 'my_table' },
value: 'my_table',
schema: 'other_schema',
label: 'other_schema.my_table',
title: 'other_schema.my_table',
});
expect(mockedProps.onTableChange.getCall(0).args[0]).toBe('my_table');
expect(mockedProps.onTableChange.getCall(0).args[1]).toBe('other_schema');
Expand Down
12 changes: 9 additions & 3 deletions superset/assets/spec/javascripts/sqllab/fixtures.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -343,16 +343,22 @@ export const databases = {
export const tables = {
options: [
{
value: { schema: 'main', table: 'birth_names' },
value: 'birth_names',
schema: 'main',
label: 'birth_names',
title: 'birth_names',
},
{
value: { schema: 'main', table: 'energy_usage' },
value: 'energy_usage',
schema: 'main',
label: 'energy_usage',
title: 'energy_usage',
},
{
value: { schema: 'main', table: 'wb_health_population' },
value: 'wb_health_population',
schema: 'main',
label: 'wb_health_population',
title: 'wb_health_population',
},
],
};
Expand Down
40 changes: 21 additions & 19 deletions superset/assets/src/components/TableSelector.jsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -99,15 +99,22 @@ export default class TableSelector extends React.PureComponent {
});
}
getTableNamesBySubStr(input) {
const { tableName } = this.state;
if (!this.props.dbId || !input) {
const options = this.addOptionIfMissing([], tableName);
const options = [];
return Promise.resolve({ options });
}
return SupersetClient.get({
endpoint: encodeURI(`/superset/tables/${this.props.dbId}/` +
`${encodeURIComponent(this.props.schema)}/${encodeURIComponent(input)}`),
}).then(({ json }) => ({ options: this.addOptionIfMissing(json.options, tableName) }));
}).then(({ json }) => {
const options = json.options.map(o => ({
value: o.value,
schema: o.schema,
label: o.label,
title: o.title,
}));
return ({ options });
});
}
dbMutator(data) {
this.props.getDbList(data.result);
Expand All @@ -130,15 +137,16 @@ export default class TableSelector extends React.PureComponent {
`${encodeURIComponent(schema)}/${encodeURIComponent(substr)}/${forceRefresh}/`);
return SupersetClient.get({ endpoint })
.then(({ json }) => {
const filterOptions = createFilterOptions({ options: json.options });
const options = json.options.map(o => ({
value: o.value,
schema: o.schema,
label: o.label,
title: o.title,
}));
this.setState(() => ({
filterOptions,
filterOptions: createFilterOptions({ options }),
tableLoading: false,
tableOptions: json.options.map(o => ({
value: o.value,
label: o.label,
title: o.label,
})),
tableOptions: options,
}));
this.props.onTablesLoad(json.options);
})
Expand Down Expand Up @@ -176,8 +184,8 @@ export default class TableSelector extends React.PureComponent {
this.setState({ tableName: '' });
return;
}
const schemaName = tableOpt.value.schema;
const tableName = tableOpt.value.table;
const schemaName = tableOpt.schema;
const tableName = tableOpt.value;
if (this.props.tableNameSticky) {
this.setState({ tableName }, this.onChange);
}
Expand All @@ -191,12 +199,6 @@ export default class TableSelector extends React.PureComponent {
this.onChange();
});
}
addOptionIfMissing(options, value) {
Copy link
Copy Markdown
Member

@betodealmeida betodealmeida Jun 25, 2019

Choose a reason for hiding this comment

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

@khtruong we don't need this anymore?

Copy link
Copy Markdown
Contributor Author

@khtruong khtruong Jun 25, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

Oh I'm sorry. I totally forgot to mention in my description. So this entire component is buggy and doesn't work as expected. What this function does is that if the option is missing, then we add it to the list with just the table name. I believe the original intent was to display something while we were fetching the tables. This led to a few bugs:

  1. We may be adding a table that no longer exists to the list, which doesn't make sense. So when the user selects this table, it just breaks silently because the table doesn't exist.
  2. Even if the table exists, we did not populate this option with the schema and so when we make a request to get the table, again it breaks silently because we get the table with just its name and no schema information.

I felt that if we really wanted to display something while we fetch the tables, we should do it properly with tests. So I just removed the buggy code for now and thought we can revisit it later if that is a feature we really want.

The component is still buggy but it's less buggy. I'll add this explanation to the description. Let me know your thoughts.

Copy link
Copy Markdown
Member

@betodealmeida betodealmeida Jun 26, 2019

Choose a reason for hiding this comment

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

Thanks, make sense!

if (options.filter(o => o.value === this.state.tableName).length === 0 && value) {
return [...options, { value, label: value }];
}
return options;
}
renderDatabaseOption(db) {
return (
<span>
Expand Down Expand Up @@ -269,7 +271,7 @@ export default class TableSelector extends React.PureComponent {
tableSelectPlaceholder = t('Select table ');
tableSelectDisabled = true;
}
const options = this.addOptionIfMissing(this.state.tableOptions, this.state.tableName);
const options = this.state.tableOptions;
const select = this.props.schema ? (
<Select
name="select-table"
Expand Down
14 changes: 9 additions & 5 deletions superset/views/core.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1802,18 +1802,22 @@ def get_datasource_label(ds_name: utils.DatasourceName) -> str:
max_tables = max_items * len(tables) // total_items
max_views = max_items * len(views) // total_items

def get_datasource_value(ds_name: utils.DatasourceName) -> Dict[str, str]:
return {"schema": ds_name.schema, "table": ds_name.table}

table_options = [
{"value": get_datasource_value(tn), "label": get_datasource_label(tn)}
{
"value": tn.table,
"schema": tn.schema,
"label": get_datasource_label(tn),
"title": get_datasource_label(tn),
}
for tn in tables[:max_tables]
]
table_options.extend(
[
{
"value": get_datasource_value(vn),
"value": vn.table,
"schema": vn.schema,
"label": f"[view] {get_datasource_label(vn)}",
"title": f"[view] {get_datasource_label(vn)}",
}
for vn in views[:max_views]
]
Expand Down