jetstreamapp / soql-parser-js

Javascript SOQL parser
https://jetstreamapp.github.io/soql-parser-js/
MIT License
77 stars 20 forks source link

isQueryValid doesn't properly validate SOQL syntax #152

Closed pgonzaleznetwork closed 3 years ago

pgonzaleznetwork commented 3 years ago

The isQueryValid function doesn't seem to actually validate that the query is valid per SOQL (as opposed to SQL) standards. I was able to easily reproduce this

let { isQueryValid } = require('soql-parser-js');

function test(connection,query){

    const validSoql = `SELECT Name INVALID FROM Account INVALID`;

    console.log(isQueryValid(validSoql));// << returns true even though the above would fail in sfdc
}
paustint commented 3 years ago

@pgonzaleznetwork - Thank you for reporting this bug! It does look like Name INVALID is reported as valid, even though the field alias is not supported in this case.

The object alias is valid, and I played around with it just to be sure - for example the following is valid:

SELECT INVALID.Name,
    FORMAT(CreatedDate) dte,
    FORMAT(CreatedDate),
    FORMAT(INVALID.CreatedDate) dte1
FROM Account INVALID

Also, as I was reviewing the docs for aliases, I noticed another case that throws an error but it is actually a valid query: SELECT count() FROM Contact c, c.Account a WHERE a.name = 'MyriadPubs'

I don't know that it is worth all the hassle to support this specific case.

Documenting here just for my future reference:

The following are SOQL keywords that can’t be used as alias names: AND, ASC, DESC, EXCLUDES, FIRST, FROM, GROUP, HAVING, IN, INCLUDES, LAST, LIKE, LIMIT, NOT, NULL, NULLS, OR, SELECT, WHERE, WITH

pgonzaleznetwork commented 3 years ago

Thanks!

paustint commented 3 years ago

@pgonzaleznetwork - After looking into this, it looks like the main issue is that all fields allow an alias when there is a GROUP BY clause present, which isn't known at the time that the fields are being parsed.

This change would require some gymnastics, and I am going to leave the current behavior as-is for now even though there is a bug.

pgonzaleznetwork commented 3 years ago

@paustint Ignore the above, I thought this reply was in relation to the other issue I reported a few days ago.