sonar-openedge icon indicating copy to clipboard operation
sonar-openedge copied to clipboard

Multiple buffers in one query

Open movedoa opened this issue 5 years ago • 9 comments

In the ABL you can have multiple buffers in one query. This can lead to fun problems when you, for example, refactor to local buffers and forget one line. The following example never hits the message since the global ttFoo Buffer is filled with the last created record. A query should just use one buffer on the left side. Joins are of course a bit different since a join are multiple queries, but still, each query should just use one buffer on the left side.

DEFINE TEMP-TABLE ttFoo NO-UNDO
   FIELD cField1 AS CHARACTER
   FIELD cField2 AS CHARACTER
.

CREATE ttFoo.
ASSIGN
   ttFoo.cField1 = "1":U
   ttFoo.cField2 = "2":U
.

CREATE ttFoo.
ASSIGN
   ttFoo.cField1 = "3":U
   ttFoo.cField2 = "4":U
.

DEFINE BUFFER lbFoo FOR ttFoo.

FOR EACH lbFoo WHERE
  lbFoo.cField1 = "1":U AND
  ttFoo.cField2 = "2":U
NO-LOCK:
   /* Message is not shown */
   MESSAGE
      ttFoo.cField1
      VIEW-AS ALERT-BOX
   .
END.

movedoa avatar Oct 06 '20 10:10 movedoa

Some comments, with unintended risk of hijacking the issue.

FIND FIRST ttFoo.
FOR EACH lbFoo WHERE
  lbFoo.cField1 = "1":U AND
  ttFoo.cField2 = lbFoo.cField2

Is valid for the compiler, but I prefer

FIND FIRST ttFoo.
FOR EACH lbFoo WHERE
  lbFoo.cField1 = "1":U AND
  lbFoo.cField2 = ttFoo.cField2

Maybe a rule to check if the "left hand side" of the condition is on a field of the, or a, buffer of the query. That rule would fire for your example and my first sample.

I think it would be great to detect these easily overlooked typos.

The rule should not fire for

FIND FIRST ttFoo.
FOR EACH lbFoo WHERE
  can-do("a*", lbFoo.cField1) AND
  (Field2Value = ? or lbFoo.cField2 = Field2Value)
  • can-do uses a function to test a lbFoo field so it's OK
  • (Field2Value = ? or lbFoo.cField2 = Field2Value) tests if the query value is known before using it.

Both are examples of code that may be totally misguided as far as performance (index usage) is concerned but to me, they would not be a violation of the rule.

cverbiest avatar Oct 21 '20 07:10 cverbiest

I'm not sure i agree with your two examples. As you said, can-do should not be used in general for performance reasons. (It even has its own rule.) I also see no necessity for the second example, there are ways to to this in a clean way without the = ? check in the query, which will improve both performance and readability. I see no good reason to write queries like this.

movedoa avatar Oct 21 '20 10:10 movedoa

Regarding the first example of @cverbiest, isn't it similar to @movedoa first example ? Issue is raised when the left-part of a simple comparison doesn't reference the buffer while the right-part does. That would raise an issue in both examples. Second example of @cverbiest wouldn't raise an issue.

gquerret avatar Oct 21 '20 20:10 gquerret

Regarding the first example of @cverbiest, isn't it similar to @movedoa first example ? Issue is raised when the left-part of a simple comparison doesn't reference the buffer while the right-part does. That would raise an issue in both examples. Second example of @cverbiest wouldn't raise an issue.

Yes it is, and i'm happy with the rule when it only raises an issue for simple comparisons.

movedoa avatar Oct 23 '20 13:10 movedoa

First version added in the develop branch

gquerret avatar Jul 01 '22 14:07 gquerret

I know this is only a first version but just to inform you:

The rule currently does not work with CAN-FIND.

movedoa avatar Aug 16 '22 11:08 movedoa

@movedoa Some improvements have been done on the rule, and so far can-find is handled correctly in the unit tests. Do you have simple examples so that I can add them to the tests ?

gquerret avatar Sep 08 '22 11:09 gquerret

Will the improvements be in the hotfix? If yes i will look for false negatives after the update.

movedoa avatar Sep 08 '22 11:09 movedoa

The improvements will be included in next version (which will probably be released tomorrow)

gquerret avatar Sep 08 '22 11:09 gquerret