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.

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

T360 sql editor bug #28

Open
wants to merge 11 commits into
base: master
Choose a base branch
from
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@ const ChartViewer = ({ dashboard, charts, meteorMethodCall, routeTo, setChartVie
prevArrow: <NavButton />,
nextArrow: <NavButton right />,
};

return (
<div
className="chart-viewer anchor-for-click"
Expand Down
1 change: 1 addition & 0 deletions client/modules/publishing/components/chart.jsx
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@ const Chart = props => {
}
const { viewObject: { data, chartType, dataTimeStamp, pivot }, queryObject } = chart;
const needRedraw = !dataTimeStamp || Date.now() - dataTimeStamp < 1000;

if (chartType) {
return (
<ChartCanvas
Expand Down
52 changes: 38 additions & 14 deletions client/modules/workplace/actions/preview.js
Original file line number Diff line number Diff line change
Expand Up @@ -38,31 +38,29 @@ export default {
const requestId = _.uniqueId();
const database = LocalState.get('CURRENT_DATABASE');
LocalState.set('QUASAR_REQUEST_ID', requestId);

Meteor.call('quasar.getData', database, query, requestId,
(err, { data = [], answerId } = {}) => {
if (err) cb({ error: true });
if (err) {
cb({ error: true });
}
else if (LocalState.get('QUASAR_REQUEST_ID') === answerId || isSingleRequest) {
const processedData = dataProcessing.process(data, fields);
cb({ data: processedData });
}
}
);
},
determineDefaultFields({ Notificator }, fields, chartType, pivot) {
determineDefaultFields({ Notificator, LocalState }, fields, chartType, pivot) {
let result;
if (pivot && pivot.model) {
const measuresId = pivot.model.values;
let dimensionsCounter = 0;
fields.forEach(f => {
if (!~measuresId.indexOf(f.id)) {
dimensionsCounter++;
if (dimensionsCounter < 3) f.constructorType = 'dimensions';
} else {
f.constructorType = 'measures';
}
});
result = fields;
} else {
if (pivot.model.constructors) {
result = determineFieldsBySavedConstructors(fields, pivot.model.constructors);
} else {
result = determineFieldsByModel(fields);
}
} else
{
const getNeededfields = () => {
const fieldsArray = [];
_.each(chartTypeRules[chartType], (val, key) => {
Expand Down Expand Up @@ -102,7 +100,33 @@ export default {
});
if (neededFields.filter(f => f.id).length) result = fields;
}

return result;

function determineFieldsBySavedConstructors(fieldsArr, constructors) {
Copy link
Contributor

Choose a reason for hiding this comment

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

please, don't mutate function parameter fieldsArr http://eslint.org/docs/rules/no-param-reassign
It is better to create a clone and make manipulations on it

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Done

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I should use map to return cloned array

fieldsArr.forEach(field => {
if (constructors.dimensions.includes(field.id)) {
field.constructorType = 'dimensions';
} else if (constructors.measures.includes(field.id)) {
field.constructorType = 'measures';
}
});
return fieldsArr;
}

function determineFieldsByModel(fieldsArr) {
Copy link
Contributor

Choose a reason for hiding this comment

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

same with mutations here

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Updated

const measuresId = pivot.model.values;
let dimensionsCounter = 0;
fieldsArr.forEach(f => {
if (!~measuresId.indexOf(f.id)) {
dimensionsCounter++;
if (dimensionsCounter < 3) f.constructorType = 'dimensions';
} else {
f.constructorType = 'measures';
}
});
return fieldsArr;
}
},

getNewChartModelFields(context, { chartType, fields }, { fieldId, isChecked, label }) {
Expand Down
25 changes: 25 additions & 0 deletions client/modules/workplace/actions/workplace_state.js
Original file line number Diff line number Diff line change
Expand Up @@ -6,9 +6,12 @@ export default {
setSQLQuery({ LocalState }, query) {
const fields = LocalState.get('COLLECTION_FIELDS');
const oldQueryObj = LocalState.get('SQL_QUERY_OBJECT');
setFieldsConstructorsType(oldQueryObj.fields, LocalState);

const queryObj = SQLParser.parseToQueryObject(query, fields);
queryObj.from = oldQueryObj.from;
queryObj.on = oldQueryObj.on;

LocalState.set('SQL_QUERY_OBJECT', queryObj);
},

Expand Down Expand Up @@ -109,3 +112,25 @@ function resetData(LocalState) {
LocalState.set('VIEW_OBJECT', viewObj);
}
}

function setFieldsConstructorsType(fieldsList, LocalState) {
Copy link
Contributor

Choose a reason for hiding this comment

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

lets use LocalState only inside setSQLQuery function. This setFieldsConstructorsType function should only return some result that we can use for setting to LocalState

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Done

let isConstructorsFinded = false;
Copy link
Contributor

Choose a reason for hiding this comment

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

isConstructorsFound

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Done

const constructorTypes = {
measures: [],
dimensions: [],
};

fieldsList.forEach((item, index) => {
const constructorType = item.constructorType;
if (constructorType) {
constructorTypes[constructorType].push(index);
isConstructorsFinded = true;
}
});

if (isConstructorsFinded) {
const viewObj = LocalState.get('VIEW_OBJECT');
viewObj.pivot.model.constructors = constructorTypes;
LocalState.set('VIEW_OBJECT', viewObj);
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@ class DataVisualisation extends React.Component {
updateSQLQueryObj } = this.props;
const { chartType } = viewObject;
const tableHeight = `${window.innerHeight - 74}px`;

return (
<div className="data-visualisation">
{tableType === 'simple' &&
Expand Down
1 change: 1 addition & 0 deletions client/modules/workplace/components/preview/preview.jsx
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,7 @@ class Preview extends React.Component {
const { savedQuery, queryObject, viewObject, tableType } = this.props;
const { isQueryChanged, isLive } = this.state;
const isFields = !_.isEmpty(queryObject.fields);

return (
<div
className="preview-wrap"
Expand Down
1 change: 1 addition & 0 deletions client/modules/workplace/containers/workplace.js
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@ export const composer = ({ context, setSQLQueryObj }, onData) => {
const { chartType, query: savedQuery } = viewObject;
parseCollectionFieldNames(queryObject.collectionFields);
setSQLQueryObj(queryObject, tableType);

LocalState.set('VIEW_OBJECT', viewObject);
onData(null, { isSavedChart: true, tableType, chartType, savedQuery });
}
Expand Down
86 changes: 78 additions & 8 deletions lib/sql_parser.js
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@ export default {
};
objectSQL.groupBy = getGroupBy(query, objectSQL.select);
objectSQL.orderBy = getOrderBy(query, objectSQL.select);

return objectSQL;
},
parseToQueryObject(rawQuery, fields) {
Expand All @@ -42,7 +43,7 @@ export default {
};

function getSelect(query) {
let select = query.match(/^\s*select\s+([\w([\])"'`.,/\s]+)from\s/i);
let select = query.match(/^\s*select\s+([\w([\])"'`*:&;.,/\s]+)from\s/i);
if (!select) return { error: 'Wrong syntax: "SELECT <fields> FROM <collections>" expected' };
select = select[1];
const asNames = select.match(/\s+as\s+\w+/ig);
Expand Down Expand Up @@ -74,21 +75,25 @@ const parseQueryPartial = (partial, paramName) =>
partial.slice(0, paramName.index).match(/(\w+)\/(\w+)/i);

function getFrom(query) {
const from = query.match(/\sfrom\s+([\w([\])"'`.,/=<>\s]+$)/i)[1].replace(cutOff, '');
const matchRes = query.match(/\sfrom\s+([\w([\])"'`.,/=<>\s]+$)/im);
if (!matchRes) return null;
const from = matchRes[1].replace(cutOff, '');
const name = getParamName(from);
const path = parseQueryPartial(from, name);
return { db: path[1], collection: path[2], name: name[1] };
}

function getJoin(query) {
const from = query.match(/\sjoin\s+([\w([\])"'`.,/=<>\s]+$)/i)[1].replace(cutOff, '');
const matchRes = query.match(/\sjoin\s+([\w([\])"'`.,/=<>\s]+$)/im);
if (!matchRes) return null;
const from = matchRes[1].replace(cutOff, '');
const name = getParamName(from);
const path = parseQueryPartial(from, name);
return path[2];
}

function getGroupBy(query, select) {
let groupBy = query.match(/\sgroup by\s+([\w([\])"'`.,/=<>\s]+$)/i);
let groupBy = query.match(/^\s*group by\s+([\w([\])"'`*:&;.,/\s]+)/im);
if (groupBy) {
groupBy = groupBy[1].replace(cutOff, '')
.replace(/\s/g, '');
Expand Down Expand Up @@ -119,19 +124,57 @@ function getOrderBy(query, select) {
}

function getHaving(query) {
let having = query.match(/\shaving\s+([\w([\])"'`.,/=<>\s]+$)/i);

let having = query.match(/^\s*having\s+([\w([\])"'`*:&;.,-<>%@=\/\s]+)limit\s/im);
if (having) {
having = having[1].replace(cutOff, '');
return parseFiltering(having);
having = removeBraces(having);
return parseHaveingConditions(having);
}
return null;


function removeBraces(queryPart) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

can also be moved outside from function.

let result = '';

if (isCountExist(queryPart)) {
const countWordINdex = getWordIndex(queryPart, 'count(');
const afterCountIndex = getWordIndex(queryPart, ')', countWordINdex);
result += removebracesWithRegex(queryPart.substr(0, countWordINdex));
result += queryPart.substring(countWordINdex, afterCountIndex + 1);
result += removeBraces(queryPart.substring(afterCountIndex + 1));
} else {
result += removebracesWithRegex(queryPart);
}
return result;
}

function isCountExist(queryToCheck) {
return queryToCheck.match(/COUNT[(]/gi);
}

function removebracesWithRegex(line) {
return line.replace(/[()]/g, '');
}

function getWordIndex(line, word, fromPosition = 0) {
let result = line.indexOf(word, fromPosition);
if (result === -1) {
result = line.indexOf(word.toUpperCase(), fromPosition);
}
return result;
}
}

function getWhere(query) {
let where = query.match(/\swhere\s+([\w([\])"'`.,/=<>\s]+$)/i);
let where = query.match(/\swhere\s+([\w([\])"'`.,/=<>\s]+$)/im);
Copy link
Collaborator

Choose a reason for hiding this comment

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

replace let

if (where) {
where = where[1].replace(cutOff, '');
return parseFiltering(where);
const parsed = parseFiltering(where);
parsed.forEach( (item) => {
item.exp2 = item.exp2.replace(/^["]/i, '').replace(/["]$/i, '');
});
return parsed;
}
return null;
}
Expand All @@ -148,6 +191,33 @@ function getOffset(query) {
return null;
}

function parseHaveingConditions(str) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Typo in Haveing

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Fixed

const filters = [];
const andOr = str.match(/\s+(and|or)\s+/gi);

if (andOr) {
let rest = str.slice();
Copy link
Collaborator

Choose a reason for hiding this comment

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

remove let

andOr.forEach(item => {
const part = rest.slice(0, rest.indexOf(item));
filters.push(part);
rest = rest.slice(rest.indexOf(item) + item.length);
});
filters.push(rest);
} else {
filters.push(str);
}
return filters.map((filter, i) => {
Copy link
Collaborator

Choose a reason for hiding this comment

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

split to smaller functions

const obj = { operator: filter.match(/(=|<>|<|>|\slike\s)/i)[1] };
const regex = new RegExp(`${obj.operator}([\\w([\\])"'\`*:&;%@.,-/\\s]+)`, 'i');
obj.exp2 = filter.match(regex)[1];
obj.exp1 = filter.replace(obj.operator, '').replace(obj.exp2, '').replace(/\s/g, '');
obj.operator = obj.operator.replace(/\s/g, '');
obj.exp2 = obj.exp2.replace(/^\s+|\s+$/g, '');
if (andOr && i < andOr.length) obj.join = andOr[i].replace(/\s/g, '');
return obj;
});
}

function parseFiltering(str) {
const filters = [];
const andOr = str.match(/\s+(and|or)\s+/gi);
Expand Down