Skip to content

Commit d9d4205

Browse files
authored
chore(api-gateway): add limit for sql-runner method (#6050)
1 parent d6702ab commit d9d4205

File tree

9 files changed

+169
-12
lines changed

9 files changed

+169
-12
lines changed

packages/cubejs-api-gateway/src/gateway.ts

Lines changed: 41 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -78,6 +78,7 @@ import { SQLServer } from './sql-server';
7878
import { makeSchema } from './graphql';
7979
import { ConfigItem, prepareAnnotation } from './helpers/prepareAnnotation';
8080
import transformData from './helpers/transformData';
81+
import { shouldAddLimit } from './helpers/shouldAddLimit';
8182
import {
8283
transformCube,
8384
transformMeasure,
@@ -1042,26 +1043,55 @@ class ApiGateway {
10421043
const requestStarted = new Date();
10431044
try {
10441045
if (!query) {
1045-
throw new UserError(
1046-
'A user\'s query must contain a body'
1047-
);
1046+
throw new UserError('A user\'s query must contain a body');
10481047
}
10491048

10501049
if (!Array.isArray(query) && !query.query) {
10511050
throw new UserError(
10521051
'A user\'s query must contain at least one query param.'
10531052
);
10541053
}
1055-
1054+
10561055
query = {
10571056
...query,
1058-
requestId: context.requestId
1057+
requestId: context.requestId,
10591058
};
10601059

1061-
if (query.resultFilter?.objectTypes && !Array.isArray(query.resultFilter.objectTypes)) {
1062-
throw new UserError(
1063-
'A query.resultFilter.objectTypes must be an array of strings'
1064-
);
1060+
const orchestratorApi = this.getAdapterApi(context);
1061+
1062+
if (shouldAddLimit(query.query)) {
1063+
if (
1064+
!query.limit ||
1065+
!Number.isInteger(query.limit) ||
1066+
query.limit < 0
1067+
) {
1068+
throw new UserError(
1069+
'A user\'s query must contain limit query param and it must be positive number'
1070+
);
1071+
}
1072+
1073+
if (query.limit > getEnv('dbQueryDefaultLimit')) {
1074+
throw new UserError('The query limit has been exceeded');
1075+
}
1076+
1077+
if (
1078+
query.resultFilter?.objectTypes &&
1079+
!Array.isArray(query.resultFilter.objectTypes)
1080+
) {
1081+
throw new UserError(
1082+
'A query.resultFilter.objectTypes must be an array of strings'
1083+
);
1084+
}
1085+
1086+
query.query = query.query.trim();
1087+
if (query.query.charAt(query.query.length - 1) === ';') {
1088+
query.query = query.query.slice(0, -1);
1089+
}
1090+
1091+
const driver = await orchestratorApi
1092+
.driverFactory(query.dataSource || 'default');
1093+
1094+
driver.wrapQueryWithLimit(query);
10651095
}
10661096

10671097
this.log(
@@ -1072,7 +1102,8 @@ class ApiGateway {
10721102
context
10731103
);
10741104

1075-
const result = await this.getAdapterApi(context).executeQuery(query);
1105+
const result = await orchestratorApi.executeQuery(query);
1106+
10761107
if (result.data.length) {
10771108
const objectLimit = Number(query.resultFilter?.objectLimit) || 100;
10781109
const stringLimit = Number(query.resultFilter?.stringLimit) || 100;
Lines changed: 17 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,17 @@
1+
import { UserError } from '../UserError';
2+
3+
// Determine SQL query type and add LIMIT clause if needed
4+
export const shouldAddLimit = (sql: string): Boolean => {
5+
// TODO: Enhance the way we determine query type
6+
const ddlRegex = /^(CREATE|ALTER|DROP|TRUNCATE)\b/i;
7+
const dmlRegex = /^(INSERT|UPDATE|DELETE)\b/i;
8+
const selectRegex = /^(SELECT|WITH)\b/i;
9+
10+
if (ddlRegex.test(sql) || dmlRegex.test(sql)) {
11+
return false;
12+
} else if (selectRegex.test(sql)) {
13+
return true;
14+
}
15+
16+
throw new UserError('Invalid SQL query');
17+
};
Lines changed: 55 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,55 @@
1+
import { shouldAddLimit } from '../../src/helpers/shouldAddLimit';
2+
3+
describe('shouldAddLimit', () => {
4+
test('returns false for CREATE query', () => {
5+
const sql = 'CREATE TABLE users (id INT, name VARCHAR(255))';
6+
expect(shouldAddLimit(sql)).toBe(false);
7+
});
8+
9+
test('returns false for ALTER query', () => {
10+
const sql = 'ALTER TABLE users ADD COLUMN email VARCHAR(255)';
11+
expect(shouldAddLimit(sql)).toBe(false);
12+
});
13+
14+
test('returns false for DROP query', () => {
15+
const sql = 'DROP TABLE users';
16+
expect(shouldAddLimit(sql)).toBe(false);
17+
});
18+
19+
test('returns false for TRUNCATE query', () => {
20+
const sql = 'TRUNCATE TABLE users';
21+
expect(shouldAddLimit(sql)).toBe(false);
22+
});
23+
24+
test('returns false for INSERT query', () => {
25+
const sql = 'INSERT INTO users (id, name) VALUES (1, "John")';
26+
expect(shouldAddLimit(sql)).toBe(false);
27+
});
28+
29+
test('returns false for UPDATE query', () => {
30+
const sql = 'UPDATE users SET name="Jane" WHERE id=1';
31+
expect(shouldAddLimit(sql)).toBe(false);
32+
});
33+
34+
test('returns false for DELETE query', () => {
35+
const sql = 'DELETE FROM users WHERE id=1';
36+
expect(shouldAddLimit(sql)).toBe(false);
37+
});
38+
39+
test('returns true for SELECT query', () => {
40+
const sql = 'SELECT * FROM users';
41+
expect(shouldAddLimit(sql)).toBe(true);
42+
});
43+
44+
test('returns true for WITH query', () => {
45+
const sql = 'WITH cte AS (SELECT id, name FROM users) SELECT * FROM cte';
46+
expect(shouldAddLimit(sql)).toBe(true);
47+
});
48+
49+
test('throw UserError for invalid SQL query', () => {
50+
const sql = 'INVALID QUERY';
51+
expect(() => {
52+
shouldAddLimit(sql);
53+
}).toThrow('Invalid SQL query');
54+
});
55+
});

packages/cubejs-api-gateway/test/index.test.ts

Lines changed: 21 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -556,7 +556,8 @@ describe('API Gateway', () => {
556556
method: 'post',
557557
successBody: {
558558
query: {
559-
query: 'SELECT * FROM sql-runner',
559+
query: 'SELECT * FROM sql-runner; ',
560+
limit: 10000,
560561
resultFilter: {
561562
objectLimit: 2,
562563
stringLimit: 2,
@@ -579,6 +580,24 @@ describe('API Gateway', () => {
579580
error: 'A user\'s query must contain at least one query param.'
580581
},
581582
body: { query: {} }
583+
}, {
584+
result: {
585+
status: 400,
586+
error: 'A user\'s query must contain limit query param and it must be positive number'
587+
},
588+
body: { query: { query: 'SELECT * FROM sql-runner' } }
589+
}, {
590+
result: {
591+
status: 400,
592+
error: 'The query limit has been exceeded'
593+
},
594+
body: { query: { query: 'SELECT * FROM sql-runner', limit: 60000 } }
595+
}, {
596+
result: {
597+
status: 400,
598+
error: 'A user\'s query must contain limit query param and it must be positive number'
599+
},
600+
body: { query: { query: 'SELECT * FROM sql-runner', limit: -1 } }
582601
}, {
583602
result: {
584603
status: 400,
@@ -587,6 +606,7 @@ describe('API Gateway', () => {
587606
body: {
588607
query: {
589608
query: 'SELECT * FROM sql-runner',
609+
limit: 10000,
590610
resultFilter: {
591611
objectTypes: 'text'
592612
},

packages/cubejs-api-gateway/test/mocks.ts

Lines changed: 13 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -71,6 +71,10 @@ export const compilerApi = jest.fn().mockImplementation(() => ({
7171
};
7272
},
7373

74+
async getDbType() {
75+
return 'postgres';
76+
},
77+
7478
async metaConfig() {
7579
return [
7680
{
@@ -190,7 +194,7 @@ export class AdapterApiMock {
190194
}
191195

192196
public async executeQuery(query) {
193-
if (query?.query === 'SELECT * FROM sql-runner') {
197+
if (query?.query.includes('SELECT * FROM sql-runner')) {
194198
return {
195199
data: [
196200
{ skip: 'skip' },
@@ -204,6 +208,14 @@ export class AdapterApiMock {
204208
};
205209
}
206210

211+
public driverFactory() {
212+
return {
213+
wrapQueryWithLimit(query: { query: string; limit: number }) {
214+
query.query = `SELECT * FROM (${query.query}) AS t LIMIT ${query.limit}`;
215+
},
216+
};
217+
}
218+
207219
public addDataSeenSource() {
208220
return undefined;
209221
}

packages/cubejs-base-driver/src/BaseDriver.ts

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -438,4 +438,8 @@ export abstract class BaseDriver implements DriverInterface {
438438
public nowTimestamp() {
439439
return Date.now();
440440
}
441+
442+
public wrapQueryWithLimit(query: { query: string, limit: number}) {
443+
query.query = `SELECT * FROM (${query.query}) AS t LIMIT ${query.limit}`;
444+
}
441445
}

packages/cubejs-base-driver/test/unit/BaseDriver.test.ts

Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -56,4 +56,14 @@ describe('BaseDriver', () => {
5656
{ name: 'string', type: 'string' }
5757
]);
5858
});
59+
60+
test('wrapQueryWithLimit wraps the query with a limit', () => {
61+
const driver = new BaseDriverImplementedMock({});
62+
const query = { query: 'SELECT * FROM users', limit: 10 };
63+
driver.wrapQueryWithLimit(query);
64+
expect(query).toEqual({
65+
query: 'SELECT * FROM (SELECT * FROM users) AS t LIMIT 10',
66+
limit: 10,
67+
});
68+
});
5969
});

packages/cubejs-mssql-driver/driver/MSSqlDriver.js

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -181,6 +181,10 @@ class MSSqlDriver extends BaseDriver {
181181
readOnly() {
182182
return !!this.config.readOnly;
183183
}
184+
185+
wrapQueryWithLimit(query) {
186+
query.query = `SELECT TOP ${query.limit} * FROM (${query.query}) AS t`;
187+
}
184188
}
185189

186190
module.exports = MSSqlDriver;

packages/cubejs-oracle-driver/driver/OracleDriver.js

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -139,6 +139,10 @@ class OracleDriver extends BaseDriver {
139139
readOnly() {
140140
return true;
141141
}
142+
143+
wrapQueryWithLimit(query) {
144+
query.query = `SELECT * FROM (${query.query}) AS t WHERE ROWNUM <= ${query.limit}`;
145+
}
142146
}
143147

144148
module.exports = OracleDriver;

0 commit comments

Comments
 (0)