G-1050 should not apply to check constraints

[rmarz]

G-1050 should not apply to check constraints.

Check constraints only accept literals, sql-functions (not PL/SQL) and references to columns in same row. see section "Restrictions on Check Constraints"

create table t (
    c char(1) default on null 'Y' not null,
    constraint t_c_chk check (c in ('Y', 'N'))
);

results in

Warning (3,37): G-1050: Avoid using literals in your code

[PhilippSalvisberg]

Thanks for reporting this issue.

Warning (3,37): G-1050: Avoid using literals in your code

I guess this is an issue with the product (SQLcl codescan command) you are using and not the guideline text under https://trivadis.github.io/plsql-and-sql-coding-guidelines/main/4-language-usage/1-general/g-1050/.

I would handle this as an implementation detail.

[rmarz]

OK, will open an Issue with Oracle SQLcl as well.

Nevertheless there are Oracle object types, where literals are fine, e.g. views for performance reasons - see #206 and others, where you can't get around them, e.g. check constraints.

Imho the rule should list those exceptions.

[PhilippSalvisberg]

Imho the rule should list those exceptions.

I agree that the rule should cover these exceptions. There should be a way to handle that without listing each place where you cannot replace a string literal with a constant or function call.

[rmarz]

table and column comments are other object types that should be exceptions from that rule. They are literals by definition.

Why not turn the point of view and define, where it should apply?

This rule applies to plsql-code and dml scripts (but not to ddl).

[PhilippSalvisberg]

Why not turn the point of view and define, where it should apply?

Because it (usually) makes sense to use constants instead of literals whenever possible. It is a limitation of the DBMS which does not allow alternatives to literals in certain areas. And these limitations are version-specific and are sometimes lifted over time. The G-1050 title starts with the keyword Avoid which "emphasizes that the action should be prevented, but some exceptions may exist". So, it's a fuzzy guideline by definition.

In your example

create table t (
    c char(1) default on null 'Y' not null,
    constraint t_c_chk check (c in ('Y', 'N'))
);

I could argue that it is not necessary to use literals there, or at least it should be possible to reduce the number of literals. You could use a boolean type instead of char(1), or define a domain, or c could become a foreign key. These considerations are good. And it's worth to document the outcome as comments in the code.

In any case, the guideline is the basis for the check/linter implementation. It's a part of the specification and should be precise. So you have for sure a point. But does it need to be a complete specification? And is it really incomplete?

This rule applies to plsql-code and dml scripts (but not to ddl).

However, I don't agree with this addition. Why? Because a create package body statement is plsql-code and DDL. If I add something like that I would reduce the scope for PL/SQL to anonymous PL/SQL blocks and functions/procedures of plsql_declarations in the with clause. I know you have not meant that. Anyway, I would like the guideline to be also applicable for the create view statement and create materialized view statement and a create table as select statement for example.

I still think it's an implementation detail. If the linter looks at the check constraints (not all do) then you could

• exclude the files/directories with the pure create table statements from the analysis,
• run the analysis only on the directories containing code,
• add markers in the code to identify false positives,
• disable the check for G-1050 on the whole workspace.

IMO it's easier and good enough to keep the reason chapter of G-1050 as is for the time being. Paragraph 2 covers just one solution. Maybe this should be extended or better be removed. Paragraph 3 which was added recently is more a guideline configuration, a parameter so to speak. I would remove it as well from the reason chapter. And maybe we could make it a bit more generic to include all kinds of replacements for a literal (constant, function, ...).

So the new reason chapter could look like this:

Literals are often used more than once in your code. Having them defined centrally reduces typos in your code and improves maintainability.