Add java rule EC24 : Optimize Database SQL Queries (Clause LIMIT)

[dedece35]

implement issue green-code-initiative/ecoCode#239
old PR : green-code-initiative/ecoCode#221 (duplicated because of stale state and no come back from original developer, thus I copied and corrected the code with review feddbacks)

[utarwyn]

Maybe we can wait for the next version of the artifact instead of using a snapshot here?

[dedece35]

waiting for approvement of this implementation before releasing ecocode-rules-specifications

[utarwyn]

In the JavaScript plugin, I added the WHERE keyword in the list of limiting keywords so as not to be too strict either. We could consider that this query already performs a stricter selection than all the data in the table. I don't know what your opinion is on this? maybe it lacks measure too :p

[dedece35]

@utarwyn update done

[Djoums]

This rule is going to be very low quality, I don't think it should be merged at all.
Just writing Console.Log("select * from table") will trigger a warning, but writing ExecuteCommand("select *" + " from table") won't

[github-actions]

This PR has been automatically marked as stale because it has no activity for 30 days.
Please add a comment if you want to keep the issue open. Thank you for your contributions!

[dedece35]

This rule is going to be very low quality, I don't think it should be merged at all. Just writing Console.Log("select * from table") will trigger a warning, but writing ExecuteCommand("select *" + " from table") won't

HI @Djoums, I'm ok with you with this rule hasn't an optimal quality, but ... to be homogenous with JS language that already has implemented this rule (check "where" and "limit" clauses), I think we can integrate the implementation in Java language and maybe in python, and php language.
For me, the main argument for this actual implementation is that this is a first level check on simple queries.
If a developer wants to avoid this rule, he will always find a way for it.
Maybe could we improve the rule with your new complex use cases : splitting query into several strings.

What do you think ?

[Djoums]

Hi @dedece35 , it's a product choice really : is a low quality rule better than nothing ?

IMO not in this case, because it is so limited that I don't see a use case for it, on top of being very prone to false positives/true negatives.

This rule may be helpful when developers build quick models, because they tend to quickly hard code sql queries to save time. But that's precisely when they don't need rules like this enforced. When it comes to actual production code that requires quality (which is what we're targeting really) :

• String interpolations/concatenations/etc cannot be handled by an analyzer (it would have to actually run the code to resolve them)
• The analyzer cannot know if a query is intended to be ran (see the Console.Log example)
• Many (if not most) sql queries are built programmatically, because their content depends on runtime variables. The analyzer cannot do anything in this case, see previous remark about having to actually run the code. That's why this rule should be enforced at a lower level imo, in the data access layer for ex.
• ORMs are everywhere these days, for exemple Entity Framework or Dapper for .Net. The analyzer cannot help here, unless we provide a specific implementation for each one.

[dedece35]

Hi @Djoums and @utarwyn (@jhertout),
I agree that is a low quality rule with actual state. The only really strong use case is the usage of "@query" with all the query inside (because we can't give a variable in that string query). Maybe, can we only deal with this use case ?

If we aren't all ok with this rule integration, if we are all ok with NO integration, next we flag this rule in our rule array to study more the implementation. But, the consequence are (I have no problem with it) :

• deletion of the rule implementation in JS plugin
• deletion of test java files in ecocode-java-test-project repository
• deletion of rule specification in ecocode repository
• update RULES.md and ecocode-rules-specification with deletions

Do we deal with only one use case ? or do we delete this rule for study it later ?

@utarwyn, @jhertout, @jycr, @glalloue ?

[Djoums]

I think that any rule dealing with sql query content shouldn't be handled by a static analyzer (Sonar, Roslyn or any other), this is also the case for the "select *" rule. I'm fine with going for the simplest case, but it's probably not going to be very helpful, especially for production level code.

[utarwyn]

Hello! I agree that the implementation of this rule is very basic currently. However I find you a little bit though with what has been done. I think this rule is important and it would be a shame not to include it in our plugins. We should have in mind that we will not be able to cover 100% of use cases, but it will always be better than 0%. As long as we don’t get false positives.

that any rule dealing with sql query content shouldn't be handled by a static analyzer

I don't understand the idea behind this: how can we deal with SQL queries if this not through a static analyzer? Do not handle them at all? I think it’s possible to do basic static analysis like we have done here.

The analyzer cannot know if a query is intended to be ran

I think it's possible to check if a variable or string is passed in a @Transactional or a JDBC operation. This is already how Intellij IDEA works to apply syntax highlighting to SQL queries. We could imagine this same feature for other languages. Note that this would complicate the implementation.

Many (if not most) sql queries are built programmatically

I don't understand? You can perform prepared statements with variables but this rule reads the selection part of the query. So it shouldn’t change anything.

ORMs are everywhere these days

I agree with you that nowadays we often do not use basic SQL queries but an ORM instead. So it could be an improvement to support some famous ORM tools in our implementations. I already have some ideas to do it in JavaScript.

I leave you the final choice, but I think we should keep this rule and continue to iterate on it to improve our implementations.

[Djoums]

All I'm saying is that it will be closer to 5% rather than 100%. And given that it will be very simplistic, that it has the potential for many false positives/true negatives, and that the poor quality will possibly hurt our plugin image, I don't think it's worth it. But I'm not trying to block it, just giving my opinion.

I don't understand the idea behind this: how can we deal with SQL queries if this not through a static analyzer? Do not handle them at all? I think it’s possible to do basic static analysis like we have done here.

Static analyzers can analyze static queries, by definition, but many are dynamic (constructed in several steps, with conditionals, function calls, etc). If what the rule does is just scan all the code base for strings that contain "select *", that is really poor imo and prone to errors.

And for "LIMIT X" this is even worse, how do you identify strings that should/shouldn't have it appended ? If I write :

string myStr = "SELECT myField FROM myTable"; // Are you going to warn here ?
myStr += " LIMIT 30";

The only way I see to properly analyze any query is at runtime, in the DAL or the DB engine, because they are fully formed at that point.

[github-actions]

This PR has been automatically marked as stale because it has no activity for 30 days.
Please add a comment if you want to keep the issue open. Thank you for your contributions!