New rule: Avoid using a FOR LOOP for a query that should return not more than one row
PhilippSalvisberg opened this issue · 1 comments
This rule was suggested by @dmcghan via X (Twitter).
Here's an example of a bad SQL:
create or replace function emp_name(in_empno in integer) return varchar2 is
l_ename emp.ename%type;
begin
<<fetch_name>>
for r in (
select ename
from emp
where empno = in_empno
)
loop
l_ename := r.ename;
end loop fetch_name;
return l_ename;
end emp_name;
/
Why is this bad?
- It can hide a
too_many_rows
exception. In case thein_empno
is not unique in the underlying model (for whatever reason) the function would return the name of the last fetched row. The more complex the query and the where_clause the higher the risk. - It can hide a
no_data_found
exception. Reading the code it is not clear, if this is wanted. Even if you know that this is often the reason to use a for loop to avoid writing a no_data_found exception handler, we cannot be sure. - Intention is lost. The loop is misleading. It implies that a set of rows needs to be processed.
But how can we detect that via static code analysis?
I don't know yet. Static code analysis is based on the source only. No access to the data dictionary. However, there are some cases like in the one above where the fetched data is returned by the function where it should be possible. In the bad example above we know that a too_many_rows
case would lead to a wrong result because the last fetched value is returned. But when the data is passed to a procedure, this cannot be deduced.
In any case, we should not avoid defining a rule just because it is difficult to write an automated check using static code analysis.
And the good example?
Use a select-into
instead. Something like this.
create or replace function emp_name(in_empno in integer) return varchar2 is
l_ename emp.ename%type;
begin
select ename
into l_ename
from emp
where empno = in_empno;
return l_ename;
exception
when no_data_found then
return null;
when too_many_rows then
raise;
end emp_name;
/
And in this case, the guideline G-5060 must be followed. And this makes sense IMO as I stated on X (Twitter). Just because something is hard to test does not mean we should avoid it.
This is beautiful! 😍 Thanks, Philipp!