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

[apex] Feature/unused variable bind false positive with dynamic SOQL #4110

Merged
merged 2 commits into from
Jan 26, 2023

Conversation

tprouvot
Copy link
Contributor

@tprouvot tprouvot commented Sep 2, 2022

Describe the PR

This PR fix the false positive issue on UnusedLocalVariable rule, when variable is used as bind variable in string soql query.

Related issues

Ready?

  • Added unit tests for fixed bug/feature
  • Passing all unit tests
  • Complete build ./mvnw clean verify passes (checked automatically by github actions)
  • Added (in-code) documentation (if needed)

@tprouvot
Copy link
Contributor Author

tprouvot commented Sep 2, 2022

Hi @adangel ! Would you have some tips on how to solve this issue ? I tried to take a look on ASTBindExpressions but not sure to fully understand how it works.
Thanks !

@adangel
Copy link
Member

adangel commented Sep 6, 2022

Not sure, if it's that easy. Looking at the sample code with the Rule Designer shows, that the parsed AST doesn't use ASTBindExpression. The dynamic SOQL are plain strings (ASTLiteralExpression).

I'm not familiar with Apex... how can you make dynamic SOQL to be executed? Is Database.getQueryLocator the only way?

It seems, you need to start at the end, where some string is passed to a Apex function (a MethodCallExpression), that expects a SOQL - then you know, that the passed string must be a SOQL. And from there you need to go back and find the assignment/literals that make up the SOQL-string. Then you'd need to parse the SOQL yourself to get the var names out of it.

@adangel adangel changed the title Feature/unused variable bind false positive [apex] Feature/unused variable bind false positive with dynamic SOQL Sep 6, 2022
@adangel adangel marked this pull request as draft September 6, 2022 16:48
@tprouvot
Copy link
Contributor Author

tprouvot commented Sep 8, 2022

Not sure, if it's that easy. Looking at the sample code with the Rule Designer shows, that the parsed AST doesn't use ASTBindExpression. The dynamic SOQL are plain strings (ASTLiteralExpression).

I'm not familiar with Apex... how can you make dynamic SOQL to be executed? Is Database.getQueryLocator the only way?

It seems, you need to start at the end, where some string is passed to a Apex function (a MethodCallExpression), that expects a SOQL - then you know, that the passed string must be a SOQL. And from there you need to go back and find the assignment/literals that make up the SOQL-string. Then you'd need to parse the SOQL yourself to get the var names out of it.

Thanks for those guidelines, yes you're right dynamic SOQL are used with Database.getQueryLocator method.
I'll try to implement this and get back to you.

Thanks for your help !

@adangel
Copy link
Member

adangel commented Oct 27, 2022

Hi @tprouvot , how is it going with this? Do you think we could try schedule it for the next release (in a month)? Please let us know if you need any help.
Cheers

@adangel adangel added this to the 6.52.0 milestone Oct 27, 2022
@tprouvot
Copy link
Contributor Author

Hi @tprouvot , how is it going with this? Do you think we could try schedule it for the next release (in a month)? Please let us know if you need any help. Cheers

Hi @adangel,
I could not work on it since I have troubles to run the designer Rule (JAVAFX missing), I'll try to find some time to work on it but I'm not sure to be on time for next release.
Do you want me to close the PR and re-open it when it's done ?

@adangel
Copy link
Member

adangel commented Oct 28, 2022

We can leave this PR in draft until it is ready - just wanted to hear back, whether you are still working on it.

Regarding Designer and JavaFX - did you see #4153 (reply in thread) ? Maybe this helps...

@adangel
Copy link
Member

adangel commented Nov 18, 2022

Hi @tprouvot , any news on this? Can we help?

@adangel adangel modified the milestones: 6.52.0, 6.53.0 Nov 22, 2022
@adangel adangel modified the milestones: 6.53.0, 6.54.0 Dec 31, 2022
@adangel adangel force-pushed the feature/unusedVariableBindFalsePositive branch from 815f665 to a03a55b Compare January 23, 2023 13:49
@adangel adangel marked this pull request as ready for review January 23, 2023 13:49
@pmd-test
Copy link

1 Message
📖 Compared to master:
This changeset changes 0 violations,
introduces 0 new violations, 0 new errors and 0 new configuration errors,
removes 0 violations, 15 errors and 7 configuration errors.
Full report

Generated by 🚫 Danger

@adangel adangel merged commit a03a55b into pmd:master Jan 26, 2023
adangel added a commit to adangel/pmd that referenced this pull request Jan 26, 2023
adangel added a commit to adangel/pmd that referenced this pull request Jan 26, 2023
…alsePositive

[apex] Feature/unused variable bind false positive with dynamic SOQL pmd#4110
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[apex] UnusedLocalVariable false positive in dynamic SOQL
3 participants