Re: [pmd-devel] Custom Apex rule questions

[Juan M. S. D. <jua...@gm...>]

ApexOptimizeSoqlRule:
>
> So, what I mean is that I'd like to check the ForEach to see if there is a
> Soql expression within its declaration, because that's a direct child node.
> Is that wrong?

Isn't this the "right" way to do it? Why do you want to look for it if it's
ok?

> Actually, I am not sure how to do this, since
> ASTVariableDeclarationStatements doesn't have a getCanonicalQuery()
> method, or anything like that.


I think you got this mixed up. The getCanonicalQuery() method is for the
SOQL's raw node, but you don't really care about it for this rule. You just
want to check there the variable to which the SOQL is assigned to is used.
ApexReadabilityRule:
>
> Triggers in Salesforce should not have any logic. The trigger should call
> a handler.


Given an example of what would be right / wrong would certainly help. I'd
not even be able to recognize a trigger from a class at this moment.

ApexSingleSoqlResultRule

> I can't seem to figure out how to check for a QueryException specifically
> as in the TryCatchFinallyBlockStatement node, there isn't a way to access
> what's inside the catch() argument, from what I can tell using PMD Designer.


I'd have to check, but you can probably get it from the raw node from Apex
Jorje (node.getNode())... Not sure though

On Fri, Jul 14, 2017 at 2:33 PM, Caleb Knox <cal...@en...>
wrote:

> ApexSingleSoqlResultRule
>
> I am checking ASTSoqlExpression nodes. If their parent nodes include a
>>> try/catch, no violation. Else, add violation.
>>>
>> 2 things here. I believe you already thought about this, but the try may
>> not be a direct parent but some other ancestor of the node, and you have to
>> actually check if the exception being caught is a QueryException
>>
> I can't seem to figure out how to check for a QueryException specifically
> as in the TryCatchFinallyBlockStatement node, there isn't a way to access
> what's inside the catch() argument, from what I can tell using PMD Designer.
>
>
> On Fri, Jul 14, 2017 at 10:46 AM, Caleb Knox <cal...@en...>
> wrote:
>
>>
>> ApexOptimizeSoqlRule:
>>>
>>> looking at ASTSoqlExpression nodes and checking their parents for a
>>>> List<> declaration
>>>
>>> This is actually good. Once you get that List<> declaration, you need
>>> to check if it is being used in a foreach and if so flag it.
>>>
>> Actually, I am not sure how to do this, since
>> ASTVariableDeclarationStatements doesn't have a getCanonicalQuery()
>> method, or anything like that.
>>
>>
>> On Fri, Jul 14, 2017 at 10:15 AM, Caleb Knox <cal...@en...>
>> wrote:
>>
>>> ApexOptimizeSoqlRule:
>>>
>>> I was planning on looking at ASTForEachStatement nodes, checking the
>>>> children for an ASTSoqlExpression node
>>>>
>>> I’m not sure I understand this… as far as I can tell, this is actually
>>>> the desired code, so there is no point ins searching for it (you only want
>>>> to add warnings for violations).
>>>>
>>> So, what I mean is that I'd like to check the ForEach to see if there is
>>> a Soql expression within its declaration, because that's a direct child
>>> node. Is that wrong?
>>>
>>>
>>> ApexReadabilityRule:
>>>
>>>
>>>>    - Non trigger framework code exists in triggers => add to violation
>>>>    count
>>>>
>>>> I’m not sure what this would be. I’m not an Apex developer. Given
>>>> proper samples I may assist you non the lest given I do know about it’s AST.
>>>>
>>> Triggers in Salesforce should not have any logic. The trigger should
>>> call a handler.
>>>
>>> Thanks.
>>>
>>>
>>> On Fri, Jul 14, 2017 at 9:14 AM, Caleb Knox <cal...@en...>
>>> wrote:
>>>
>>>> Thanks for the insight. I'll get back to you about the ApexOptimizeSoql
>>>> if I don't get it right soon.
>>>>
>>>> I can definitely add these rules to PMD, assuming my boss okays it.
>>>>
>>>> On Thu, Jul 13, 2017 at 6:48 PM, Juan Martín Sotuyo Dodero <
>>>> jua...@gm...> wrote:
>>>>
>>>>> Hi Caleb,
>>>>>
>>>>> I’m glad to see you are making progress. Are you planning to
>>>>> contribute these rules back to PMD? If so, it will be easier / we will be
>>>>> far more willing to help you out. Just submit a PR for each in such case
>>>>> and we will comment directly on your code.
>>>>>
>>>>> In the meantime, let me go in order:
>>>>> ApexOptimizeSoqlRule:
>>>>>
>>>>> I was planning on looking at ASTForEachStatement nodes, checking the
>>>>> children for an ASTSoqlExpression node
>>>>>
>>>>> I’m not sure I understand this… as far as I can tell, this is actually
>>>>> the desired code, so there is no point ins searching for it (you only want
>>>>> to add warnings for violations).
>>>>>
>>>>> looking at ASTSoqlExpression nodes and checking their parents for a
>>>>> List<> declaration
>>>>>
>>>>> This is actually good. Once you get that List<> declaration, you need
>>>>> to check if it is being used in a foreach and if so flag it.
>>>>> ApexSingleSoqlResultRule
>>>>>
>>>>> I am checking ASTSoqlExpression nodes. If their parent nodes include a
>>>>> try/catch, no violation. Else, add violation.
>>>>>
>>>>> 2 things here. I believe you already thought about this, but the try
>>>>> may not be a direct parent but some other ancestor of the node, and you
>>>>> have to actually check if the exception being caught is a
>>>>> QueryException
>>>>>
>>>>> I also check the ASTSoqlExpression for a LIMIT 1 since that’s how
>>>>> people avoid the QueryException.
>>>>>
>>>>> This is a must to avoid flagging all SOQL. Given ASTSoqlExpression
>>>>> node you can do node.getNode().getCanonicalQuery() to obtain the
>>>>> contents of the expression being used.
>>>>> ApexCommentRule
>>>>>
>>>>> can I just use a Java rule, like CommentRequiredRule?
>>>>>
>>>>> No you can’t. The Java rules use the Java Lexer and Parser, that only
>>>>> recognize Java code. You need Apex specific rules to do this.
>>>>>
>>>>> Comments are not part of the AST, so doing this will not be trivial.
>>>>> I’m not sure the Jorje parser provides access to comments at all, I’d have
>>>>> to check the APIs and do some tests…
>>>>> ApexReadabilityRule
>>>>>
>>>>>
>>>>>    - Lines of code in a method exceed 50 => add to violation count
>>>>>
>>>>> This is already implemented as apex-complexity/NcssMethodCount
>>>>> <https://pmd.github.io/pmd-5.8.1/pmd-apex/rules/apex/complexity.html#NcssMethodCount>.
>>>>> The default is 40 lines (without comments), but can be configured
>>>>>
>>>>>
>>>>>    - Nested if statements exceed more than 3 => add to violation count
>>>>>
>>>>> This is already implemented as apex-complexity/AvoidDeeplyNes
>>>>> tedIfStmts
>>>>> <https://pmd.github.io/pmd-5.8.1/pmd-apex/rules/apex/complexity.html#AvoidDeeplyNestedIfStmts>.
>>>>> The default limit is exactly 3, you would have to set it to 4 to allow up
>>>>> to 3.
>>>>>
>>>>>
>>>>>    - Non trigger framework code exists in triggers => add to
>>>>>    violation count
>>>>>
>>>>> I’m not sure what this would be. I’m not an Apex developer. Given
>>>>> proper samples I may assist you non the lest given I do know about it’s AST.
>>>>>
>>>>> Regards,
>>>>> ​
>>>>>
>>>>> On Thu, Jul 13, 2017 at 6:38 PM, Caleb Knox <cal...@en...>
>>>>> wrote:
>>>>>
>>>>>> Hi there folks,
>>>>>>
>>>>>> I am trying to implement a few more rules for the project I've
>>>>>> emailed about before, before we send it off to our customers. This is
>>>>>> working with Salesforce/Apex classes.
>>>>>>
>>>>>> Here they are:
>>>>>>
>>>>>>
>>>>>>
>>>>>> *ApexOptimizeSoqlRule*
>>>>>> Example:
>>>>>>
>>>>>> List<Account> acct = […];
>>>>>> for (Account a : acct ) {
>>>>>>
>>>>>> }
>>>>>>
>>>>>> // add violation
>>>>>>
>>>>>> -------------------------------------------
>>>>>>
>>>>>> // optimized version
>>>>>>
>>>>>> for (Account acct : [Select … ] ) {
>>>>>>
>>>>>> }
>>>>>>
>>>>>> // all good
>>>>>>
>>>>>> For this rule, I was planning on looking at ASTForEachStatement
>>>>>> nodes, checking the children for an ASTSoqlExpression node, as well as
>>>>>> looking at ASTSoqlExpression nodes and checking their parents for a List<>
>>>>>> declaration. However, what I have is not working, so I am wondering if my
>>>>>> basic way of thinking about this rule is wrong, and if I should do
>>>>>> something else.
>>>>>>
>>>>>>
>>>>>>
>>>>>> *ApexSingleSoqlResultRule*
>>>>>> Example:
>>>>>>
>>>>>> Account acct = [Select Id From Account Where Name = ‘abc’ ];
>>>>>>
>>>>>> // Get QueryException back if 0 or duplicate
>>>>>>
>>>>>> // People try to avoid by doing:
>>>>>>
>>>>>> List<Account> acct = [Select … LIMIT 1]; // bad practice to avoid
>>>>>> dupes etc --> add violation
>>>>>>
>>>>>> if (acct.size() > 0 )
>>>>>>
>>>>>> [...]
>>>>>>
>>>>>>
>>>>>> --------------------------------------------
>>>>>>
>>>>>> // Best practice is:
>>>>>>
>>>>>> try {
>>>>>> Account acct = […];
>>>>>> } catch (QueryException q){
>>>>>>
>>>>>> }
>>>>>>
>>>>>> For this rule, I am checking ASTSoqlExpression nodes. If their parent
>>>>>> nodes include a try/catch, no violation. Else, add violation.
>>>>>>
>>>>>> I also check the ASTSoqlExpression for a LIMIT 1 since that's how
>>>>>> people avoid the QueryException. But, I don't quite know how to do that.
>>>>>>
>>>>>> Maybe something like this?
>>>>>>
>>>>>> if(node.getNode().getQuery().toLowerCase().contains("list") &&
>>>>>>                 node.getNode().getQuery().toLowerCase().contains("limit
>>>>>> 1"))
>>>>>>
>>>>>>
>>>>>> *ApexCommentRule*
>>>>>>
>>>>>> The last two rules are related to readability. The first one looks at
>>>>>> the following:
>>>>>>
>>>>>>    - No class or method comments => add to violation count
>>>>>>
>>>>>>
>>>>>> For this, can I just use a Java rule, like CommentRequiredRule?
>>>>>>
>>>>>> *ApexReadabilityRule*
>>>>>>
>>>>>> The last one follows these criteria:
>>>>>>
>>>>>>    - Lines of code in a method exceed 50 => add to violation count
>>>>>>    - Nested if statements exceed more than 3  => add to violation
>>>>>>    count
>>>>>>    - Non trigger framework code  exists in triggers  => add to
>>>>>>    violation count
>>>>>>
>>>>>>
>>>>>> I'm not sure how to do this last one.
>>>>>>
>>>>>> If you have any insight or feedback, let me know!
>>>>>>
>>>>>> Thank you.
>>>>>>
>>>>>> --
>>>>>> Caleb Knox
>>>>>>
>>>>>> Endeveran
>>>>>>
>>>>>> ------------------------------------------------------------
>>>>>> ------------------
>>>>>> Check out the vibrant tech community on one of the world's most
>>>>>> engaging tech sites, Slashdot.org! http://sdm.link/slashdot
>>>>>> _______________________________________________
>>>>>> Pmd-devel mailing list
>>>>>> Pmd...@li...
>>>>>> https://lists.sourceforge.net/lists/listinfo/pmd-devel
>>>>>>
>>>>>>
>>>>>
>>>>
>>>>
>>>> --
>>>> Caleb Knox
>>>>
>>>> Endeveran
>>>>
>>>
>>>
>>>
>>> --
>>> Caleb Knox
>>>
>>> Endeveran
>>>
>>
>>
>>
>> --
>> Caleb Knox
>>
>> Endeveran
>>
>
>
>
> --
> Caleb Knox
>
> Endeveran
>