[apex] New Rule: Avoid soql/sosl queries without a where clause or limit statement

[JAertgeerts]

Rule Set: Apex, perfomance

Description:
When working with very large amounts of data, unfiltered SOQL queries can quickly cause governor limit exceptions

Code Sample demonstrating the issue:

public class Something {
	public static void main( String as[] ) {  
		Account[] accs = [ select id from account ];  //Bad
	}
}

[JAertgeerts]

Right now it's pretty dumb where it won't catch subqueries:
select id, (select id from contact) from account where 1=1 limit 1 will not be a violation, which it should.

Should probably extract subqueries first and count them separately...
Regex to get the subqueries:

• Regular Expression: \((select.*?)\)
• as a Java string: "\\((select.*?)\\)"

Could be an improvement, the rule should be usable and catch most issues already in it's simple state. Subqueries don't happen that often.

[rsoesemann]

@jsotuyod I recommend to close "feature-request" issues that are not followed by a PR (or ongoing communication) after 2 month.

[jsotuyod]

I'm closing this as the PR proved the capabilities to parse the SOQL and enforce the rule are very limited. Maybe future versions of Jorje will make this easier, otherwise, we would need something such as #237 along with the PL/SQL grammar.

[dschach]

@jsotuyod With the new Apex parser, is this request more feasible?

[adangel]

@dschach not really. The new apex parser parses both SOQL and SOSL queries. But this parse tree is not yet translated into a syntax tree. That would need to be added to Summit AST. Summit AST right now just replaced Jorje and didn't add functionality. The SOQL/SOSL queries are provided as raw strings, so these are currently available in PMD. See e.g.

pmd/pmd-apex/src/main/java/net/sourceforge/pmd/lang/apex/ast/ASTSoqlExpression.java

Lines 33 to 43 in 648e2eb

	/**
	* Returns the query with the SOQL keywords normalized as uppercase.
	*/
	public String getCanonicalQuery() {
	return canoncialQuery;
	}
	private static String convertToCanonicalQuery(String rawQuery) {
	// note: this is a very crude way. At some point, the parsed query should be
	// provided by Summit AST. The Apex Parser already parses the SOQL/SOSL queries.
	String query = rawQuery;

In Summmit AST, see https://github.com/google/summit-ast/blob/b036f6a2544db86064c8fd7adca01075b60ba954/src/main/java/com/google/summit/translation/Translate.kt#L1434-L1442

So, in summary, these are the options:

• continue to use a crude way to "parse" the query string with regex etc. E.g. as suggested originally, this would already find the most common cases and could be just enough as an initial implementation. We could also count the "SELECT" tokens and the "LIMIT" tokens - each SELECT must have one LIMIT...
• reparse the SOSL/SOQL queries in the new rule. The you can deal with subqueries and match the limit clause to the correct select.
• Update the AST that summit-ast produces to provide a subtree for the SOSL/SOQL queries. We probably would still have a SoqlExpression or SoslExpression, but additional child nodes like "selectList", "fromNameList", "limitClause", ... (see https://github.com/apex-dev-tools/apex-parser/blob/35b8febe61ca796781f4a03750106a64478a6543/antlr/ApexParser.g4#L554-L571). Once this is done, this rule could be written as a simple XPath based rule (e.g. //SoqlExpression[not(LimitClause) or .//Subquery[not(LimitClause)]]).