Skip to content

Commit fb8d903

Browse files
committed
Fix #40 - Added rules G-9010, G-9020 and G-9030 about conversion functions
1 parent 6c74c2f commit fb8d903

File tree

7 files changed

+120
-2
lines changed

7 files changed

+120
-2
lines changed
Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1 @@
1+
title: Function Usage
Lines changed: 38 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,38 @@
1+
# G-9010: Always use a format model in string to date/time conversion functions.
2+
3+
!!! warning "Major"
4+
Changeability, Maintainability, Reliability, Security, Testability
5+
6+
## Reason
7+
8+
Converting from strings to `date` or `timestamp` datatypes (using `to_date`, `to_timestamp`, `to_timestamp_tz` or `cast` to any of those datatypes) in practice always expects a fixed format (unlike converting *to* strings that can be fixed as well as allow the session to decide). Therefore it is a bad idea to allow this conversion to rely on the session NLS settings (`nls_date_format`, `nls_timestamp_format` and `nls_timestamp_tz_format`) as this makes the code vulnerable to changes in session and/or server configuration. It is even possible to utilize session `nls_date_format` for SQL injection if you use dynamic SQL.
9+
10+
Using an explicit format model for string to `date` or `timestamp` conversion avoids this inappropriate dependability on configurable NLS parameters.
11+
12+
## Example (bad)
13+
14+
``` sql
15+
create package body employee_api is
16+
procedure set_dob (in_employee_id in employees.employee_id%type, in_dob_str in varchar2)
17+
begin
18+
update employee
19+
set date_of_birth = to_date(in_dob_str)
20+
where employee_id = in_employee_id;
21+
end set_dob;
22+
end employee_api;
23+
/
24+
```
25+
26+
## Example (good)
27+
28+
``` sql
29+
create package body employee_api is
30+
procedure set_dob (in_employee_id in employees.employee_id%type, in_dob_str in varchar2)
31+
begin
32+
update employee
33+
set date_of_birth = to_date(in_dob_str, 'YYYY-MM-DD')
34+
where employee_id = in_employee_id;
35+
end set_dob;
36+
end employee_api;
37+
/
38+
```
Lines changed: 38 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,38 @@
1+
# G-9020: Try to use a format model and NLS_NUMERIC_CHARACTERS in string to number conversion functions.
2+
3+
!!! warning "Major"
4+
Changeability, Maintainability, Reliability, Security, Testability
5+
6+
## Reason
7+
8+
Converting from strings to numeric datatypes (using `to_number`, `to_binary_double`, `to_binary_float` or `cast` to any of those datatypes) rely on session NLS settings for `nls_numeric_characters`. Typically the input string is expected to have a given decimal- and group-separator, so it is best practice to specify `nls_numeric_characters` in the function call. However, this requires also setting a format model, which is a good idea but can require a very large format model with many 9's if you do not know the maximum length of the string.
9+
10+
To avoid an inappropriate dependability on configurable NLS parameters, try to use both format model and `nls_numeric_characters` in the conversion function call. The exceptions can be if the input is known to always be integer with no decimal- or group-separator, or if you do not know the maximum number of digits **and** have control over the session `nls_numeric_characters` parameter.
11+
12+
## Example (bad)
13+
14+
``` sql
15+
create package body employee_api is
16+
procedure set_salary (in_employee_id in employees.employee_id%type, in_salary in varchar2)
17+
begin
18+
update employee
19+
set salary = to_number(in_salary)
20+
where employee_id = in_employee_id;
21+
end set_dob;
22+
end employee_api;
23+
/
24+
```
25+
26+
## Example (good)
27+
28+
``` sql
29+
create package body employee_api is
30+
procedure set_salary (in_employee_id in employees.employee_id%type, in_salary in varchar2)
31+
begin
32+
update employee
33+
set salary = to_number(in_salary, '99999999999999999999.99999', q'[nls_numeric_characters='.,']')
34+
where employee_id = in_employee_id;
35+
end set_dob;
36+
end employee_api;
37+
/
38+
```
Lines changed: 36 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,36 @@
1+
# G-9030: Try to define a default value on conversion errors.
2+
3+
!!! tip "Minor"
4+
Maintainability, Reliability, Testability
5+
6+
## Reason
7+
8+
When converting from strings to other datatypes using a conversion function that supports the `default ... on conversion error` clause, it is a good idea to use this clause to avoid getting an error raised on bad input. The exception can be when you explicitly *want* an error to be raised to catch and process it in a later exception handler.
9+
10+
## Example (bad)
11+
12+
``` sql
13+
create package body employee_api is
14+
procedure set_dob (in_employee_id in employees.employee_id%type, in_dob_str in varchar2)
15+
begin
16+
update employee
17+
set date_of_birth = to_date(in_dob_str, 'YYYY-MM-DD')
18+
where employee_id = in_employee_id;
19+
end set_dob;
20+
end employee_api;
21+
/
22+
```
23+
24+
## Example (good)
25+
26+
``` sql
27+
create package body employee_api is
28+
procedure set_dob (in_employee_id in employees.employee_id%type, in_dob_str in varchar2)
29+
begin
30+
update employee
31+
set date_of_birth = to_date(in_dob_str default null on conversion error, 'YYYY-MM-DD')
32+
where employee_id = in_employee_id;
33+
end set_dob;
34+
end employee_api;
35+
/
36+
```

docs/9-appendix/appendix.md

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -123,4 +123,6 @@ n/a | 8120 | Never check existence of a row to decide whether to create it or no
123123
n/a | 8310 | Always validate input parameter size by assigning the parameter to a size limited variable in the declaration section of program unit. | Minor | | | ✘ | | ✘ | ✘ | | ✘
124124
n/a | 8410 | Always use application locks to ensure a program unit is only running once at a given time. | Minor | | ✘ | | | ✘ | | |
125125
n/a | 8510 | Always use dbms_application_info to track program process transiently. | Minor | | ✘ | | | ✘ | | |
126-
n/a | 9010 | Try to label your sub blocks. | Minor | | | ✘ | | | | |
126+
n/a | 9010 | Always use a format model in string to date/time conversion functions. | Major | ✘ | | ✘ | | ✘ | | ✘ | ✘
127+
n/a | 9020 | Try to use a format model and NLS_NUMERIC_CHARACTERS in string to number conversion functions. | Major | ✘ | | ✘ | | ✘ | | ✘ | ✘
128+
n/a | 9030 | Try to define a default value on conversion errors. | Minor | | | ✘ | | ✘ | | | ✘

docs/stylesheets/extra.css

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -224,7 +224,8 @@
224224
[id="access-objects-of-foreign-application-schemas"],
225225
[id="validating-input-parameter-size"],
226226
[id="ensure-single-execution-at-a-time-of-a-program-unit"],
227-
[id="use-dbms_application_info-package-to-follow-progress-of-a-process"] {
227+
[id="use-dbms_application_info-package-to-follow-progress-of-a-process"],
228+
[id="function-usage"] {
228229
page-break-before: always;
229230
}
230231

tools/run-in-container/genpdf.sh

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -164,6 +164,8 @@ write_text "### Ensure single execution at a time of a program unit"
164164
write_guidelines "4-language-usage/8-patterns/4-ensure-single-execution-at-a-time-of-a-program-unit" "####"
165165
write_text "### Use dbms_application_info package to follow progress of a process"
166166
write_guidelines "4-language-usage/8-patterns/5-use-dbms-application-info-package-to-follow-progress-of-a-process" "####"
167+
write_text "## Function Usage"
168+
write_guidelines "4-language-usage/9-function-usage" "###"
167169
write_file "5-complexity-analysis/complexity-analysis.md"
168170
write_file "6-code-reviews/code-reviews.md"
169171
write_file "7-tool-support/tool-support.md"

0 commit comments

Comments
 (0)