Skip to content

Commit 978af54

Browse files
committed
Fix potential concurrency bug in results view
This was pointed out by CodeQL: when calling `setState` and using `this.props`, it may not be up-to-date because `setState` may run concurrently. Therefore, we should use the `setState` callback variant to ensure we get the latest props. This refactors the code a bit to ensure we're not using `this.props` anywhere, including in the `getResultSets` function which is called in the `setState` callback.
1 parent 84928fa commit 978af54

File tree

1 file changed

+69
-50
lines changed

1 file changed

+69
-50
lines changed

extensions/ql-vscode/src/view/results/result-tables.tsx

Lines changed: 69 additions & 50 deletions
Original file line numberDiff line numberDiff line change
@@ -78,6 +78,52 @@ function renderResultCountString(resultSet: ResultSet): JSX.Element {
7878
);
7979
}
8080

81+
function getInterpretedTableName(interpretation: Interpretation): string {
82+
return interpretation?.data.t === "GraphInterpretationData"
83+
? GRAPH_TABLE_NAME
84+
: ALERTS_TABLE_NAME;
85+
}
86+
87+
function getResultSetNames(
88+
interpretation: Interpretation | undefined,
89+
parsedResultSets: ParsedResultSets,
90+
): string[] {
91+
return interpretation
92+
? parsedResultSets.resultSetNames.concat([
93+
getInterpretedTableName(interpretation),
94+
])
95+
: parsedResultSets.resultSetNames;
96+
}
97+
98+
function getResultSets(
99+
rawResultSets: readonly ResultSet[],
100+
interpretation: Interpretation | undefined,
101+
): ResultSet[] {
102+
const resultSets: ResultSet[] =
103+
// eslint-disable-next-line @typescript-eslint/ban-ts-comment
104+
// @ts-ignore 2783
105+
rawResultSets.map((rs) => ({ t: "RawResultSet", ...rs }));
106+
107+
if (interpretation !== undefined) {
108+
const tableName = getInterpretedTableName(interpretation);
109+
resultSets.push({
110+
t: "InterpretedResultSet",
111+
// FIXME: The values of version, columns, tupleCount are
112+
// unused stubs because a InterpretedResultSet schema isn't used the
113+
// same way as a RawResultSet. Probably should pull `name` field
114+
// out.
115+
schema: {
116+
name: tableName,
117+
rows: 1,
118+
columns: [],
119+
},
120+
name: tableName,
121+
interpretation,
122+
});
123+
}
124+
return resultSets;
125+
}
126+
81127
/**
82128
* Displays multiple `ResultTable` tables, where the table to be displayed is selected by a
83129
* dropdown.
@@ -86,51 +132,13 @@ export class ResultTables extends React.Component<
86132
ResultTablesProps,
87133
ResultTablesState
88134
> {
89-
private getResultSets(): ResultSet[] {
90-
const resultSets: ResultSet[] =
91-
// eslint-disable-next-line @typescript-eslint/ban-ts-comment
92-
// @ts-ignore 2783
93-
this.props.rawResultSets.map((rs) => ({ t: "RawResultSet", ...rs }));
94-
95-
if (this.props.interpretation !== undefined) {
96-
const tableName = this.getInterpretedTableName();
97-
resultSets.push({
98-
t: "InterpretedResultSet",
99-
// FIXME: The values of version, columns, tupleCount are
100-
// unused stubs because a InterpretedResultSet schema isn't used the
101-
// same way as a RawResultSet. Probably should pull `name` field
102-
// out.
103-
schema: {
104-
name: tableName,
105-
rows: 1,
106-
columns: [],
107-
},
108-
name: tableName,
109-
interpretation: this.props.interpretation,
110-
});
111-
}
112-
return resultSets;
113-
}
114-
115-
private getInterpretedTableName(): string {
116-
return this.props.interpretation?.data.t === "GraphInterpretationData"
117-
? GRAPH_TABLE_NAME
118-
: ALERTS_TABLE_NAME;
119-
}
120-
121-
private getResultSetNames(): string[] {
122-
return this.props.interpretation
123-
? this.props.parsedResultSets.resultSetNames.concat([
124-
this.getInterpretedTableName(),
125-
])
126-
: this.props.parsedResultSets.resultSetNames;
127-
}
128-
129135
constructor(props: ResultTablesProps) {
130136
super(props);
131137
const selectedTable =
132138
props.parsedResultSets.selectedTable ||
133-
getDefaultResultSet(this.getResultSets());
139+
getDefaultResultSet(
140+
getResultSets(props.rawResultSets, props.interpretation),
141+
);
134142
const selectedPage = `${props.parsedResultSets.pageNumber + 1}`;
135143
this.state = {
136144
selectedTable,
@@ -148,18 +156,23 @@ export class ResultTables extends React.Component<
148156
this.props.parsedResultSets.resultSetNames.some(
149157
(v) => this.state.selectedTable === v,
150158
) ||
151-
this.getResultSets().some(
159+
getResultSets(this.props.rawResultSets, this.props.interpretation).some(
152160
(v) => this.state.selectedTable === v.schema.name,
153161
);
154162

155163
// If the selected result set does not exist, select the default result set.
156164
if (!resultSetExists) {
157-
const selectedTable =
158-
this.props.parsedResultSets.selectedTable ||
159-
getDefaultResultSet(this.getResultSets());
160-
this.setState({
161-
selectedTable,
162-
selectedPage: `${this.props.parsedResultSets.pageNumber + 1}`,
165+
this.setState((state, props) => {
166+
const selectedTable =
167+
props.parsedResultSets.selectedTable ||
168+
getDefaultResultSet(
169+
getResultSets(props.rawResultSets, props.interpretation),
170+
);
171+
172+
return {
173+
selectedTable,
174+
selectedPage: `${props.parsedResultSets.pageNumber + 1}`,
175+
};
163176
});
164177
}
165178
}
@@ -328,8 +341,14 @@ export class ResultTables extends React.Component<
328341

329342
render(): React.ReactNode {
330343
const { selectedTable } = this.state;
331-
const resultSets = this.getResultSets();
332-
const resultSetNames = this.getResultSetNames();
344+
const resultSets = getResultSets(
345+
this.props.rawResultSets,
346+
this.props.interpretation,
347+
);
348+
const resultSetNames = getResultSetNames(
349+
this.props.interpretation,
350+
this.props.parsedResultSets,
351+
);
333352

334353
const resultSet = resultSets.find(
335354
(resultSet) => resultSet.schema.name === selectedTable,

0 commit comments

Comments
 (0)