FWD » Core Development » Base Language

In #8308, it was discovered that errors related to the database are handled differently when a CATCH block is present in the current block. These changes reached trunk in trunk/rev.15255 and 7156b/rev.15076. #8308-212 - #8308-237 are the notes that refer to this issue. In #8308-222 and #8308-225, two lists of places where recordOrShowError() is using isError = false.

TBD: There are cases that were not tested, check the two lists provided and investigate them (it is important to look at the persist package), write tests for each one and fix them using recordOrShowDatabaseError() if necessary. (reference tests: #8308-212, #8308-219, #8308-220, #8308-235).

Created 8643a.

Committed 8643a/rev.15307 which contains the 15272 and 15273 revisions from 8308e. (see #8308-397)

Dănuț Filimon wrote:

Committed 8643a/rev.15307 which contains the 15272 and 15273 revisions from 8308e. (see #8308-397)

Those will need to be discarded completely .

Constantin Asofiei wrote:

Dănuț Filimon wrote:

Committed 8643a/rev.15307 which contains the 15272 and 15273 revisions from 8308e. (see #8308-397)

Those will need to be discarded completely .

I reverted the changes from revision 8643a/rev.15307 in 8643a/rev.15308.

I've rebased 7643a from trunk rev 15308.

The changes are in rev 15311. With this, the tests in #8308-397, #8308-306 and #8308-319 are working.

Danut: please run the tests for #8308 and post any details at that task (not in this task).

Constantin, patching ErrorManager for 7156b is a bit hard to understand as there are no ServerErrorDataAccessor and SuppressedErrorDataAccessor in 7156b, only LocalDataAccess, ServerDataAccess and ErrorDataAccess.

From what I can understand, ErrorDataAccess is actually ServerErrorDataAccessor and SuppressedErrorDataAccessor is actually ServerDataAccess but it uses false as a return value by default while in 7156b it uses ErrorManager. I've managed to patch the other two files, but this one is hard to understand as there is no javadoc that explains these classes in detail.

I've posted my results in #8308-403 where I compared the previously made changes from 8308e with the new changes from 8643a. From the note:

Previously there were 26 tests with 18 fixed by 8308e, 3 displaying a known error, 5 that had different errors. Now there are 17 fixed with 7 that have the index issue and 2 that still need to be fixed.

8643a changes fail this test where the error is shown into a modal instead of being caught by the catch block:

DEFINE TEMP-TABLE ttsrc13 BEFORE-TABLE bittsrc13 FIELD srcid AS INT FIELD srcname AS CHAR.
DEFINE TEMP-TABLE src13 FIELD srcid AS INTEGER FIELD srcname AS CHARACTER INDEX srcid IS UNIQUE srcid.
DEFINE DATASET dssrc13 FOR ttsrc13.

DEFINE VARIABLE httsrc AS HANDLE NO-UNDO.
DEFINE VARIABLE hsrc AS HANDLE NO-UNDO.

TEMP-TABLE ttsrc13:TRACKING-CHANGES = TRUE.
CREATE ttsrc13. ASSIGN ttsrc13.srcid = 1 ttsrc13.srcName = 's1'.
CREATE ttsrc13. ASSIGN ttsrc13.srcid = 1 ttsrc13.srcName = 's2'.
TEMP-TABLE ttsrc13:TRACKING-CHANGES = FALSE.

httsrc = BUFFER ttsrc13:HANDLE.
hsrc = BUFFER src13:HANDLE.

DEFINE VARIABLE srcsrc13 AS HANDLE NO-UNDO.
CREATE DATA-SOURCE srcsrc13.
srcsrc13:ADD-SOURCE-BUFFER(hsrc, "srcname,srcid").
httsrc:ATTACH-DATA-SOURCE(srcsrc13, "srcid,srcid,srcname,srcname").

DO TRANSACTION ON ERROR UNDO, THROW:
    FOR EACH bittsrc13 ON ERROR UNDO, THROW:
        BUFFER bittsrc13:SAVE-ROW-CHANGES().
    END.
    CATCH ex AS Progress.Lang.Error :
        MESSAGE 132 ex:GetMessageNum(1).
    END CATCH.
END.

It's caused by the revalidation of the record in BlockManager.processForBody():

         if (reason == null || isNormal(wa, reason, on))
         {
            executeStateManaged(wa, wa.tm::processValidate);
         }

I've mentioned this in an email but never in an issue so I'll be writing it here. There was a problem in #8308 with the introduction of the error 11910 when there was a validation exception in BufferImpl.saveRowChangesImpl2() and this was causing a "double validation" in TxWrapper$WorkArea.validate(). The fix is actually simple, if a record is in an INVALID state, it should not be revalidated as it already failed it once.

=== modified file 'src/com/goldencode/p2j/persist/RecordBuffer.java'
--- old/src/com/goldencode/p2j/persist/RecordBuffer.java    2024-06-26 06:05:25 +0000
+++ new/src/com/goldencode/p2j/persist/RecordBuffer.java    2024-06-26 09:44:02 +0000
@@ -6233,7 +6233,8 @@
       if (!isActive()                        ||
           currentRecord == null              ||
           !inChangeScope() && !isTransient() ||
-          currentRecord.checkState(DmoState.DELETING))
+          currentRecord.checkState(DmoState.DELETING) ||
+          currentRecord.checkState(DmoState.INVALID))
       {
          // nothing to do
          return;

The change mentioned in #8643-15 is not ok, the method called is RecordBuffer.validate() which calls flush(true) and the true value enables revalidating the record. It will require another fix.

Danut, trunk/14998 was especially created to handle rollbacks, as there were unit tests (on another customer application) that were not properly rollbacking on failure. You can cherry-pick these. But, you will also need trunk/15244 that is fixing a regression of 14998.

Alexandru Lungu wrote:

Danut, trunk/14998 was especially created to handle rollbacks, as there were unit tests (on another customer application) that were not properly rollbacking on failure. You can cherry-pick these. But, you will also need trunk/15244 that is fixing a regression of 14998.

Thanks! I'll take those changes and test them.

I've tried using BlockManager and TransactionManager from trunk (not just revs 14998/15244, so other changes, too), and this caused lots of regressions in the customer's unit tests. If you decide to include just the changes from 14998 and 15244, you need do a full run of both unittests and fwdtests.

Danut, please pend effort until I can get some logs from 6667g to cross-check.

Constantin Asofiei wrote:

I've tried using BlockManager and TransactionManager from trunk (not just revs 14998/15244, so other changes, too), and this caused lots of regressions in the customer's unit tests. If you decide to include just the changes from 14998 and 15244, you need do a full run of both unittests and fwdtests.

I only plan to test 14998 and 15244 with 8643a changes, can you take a look at #8643-12 and let me know if this patch is ok for 7156b?

Alexandru Lungu wrote:

Danut, please pend effort until I can get some logs from 6667g to cross-check.

This issue is related to error handling, is there something from 6667g that should be taken into account here?

6667g is based on trunk rev/15271, so it includes both revisions.

Dănuț Filimon wrote:

... can you take a look at #8643-12 and let me know if this patch is ok for 7156b?

The patch is OK (considering forceExplicit was added after).

Alexandru Lungu wrote:

6667g is based on trunk rev/15271, so it includes both revisions.

I understand now, I'll mark the tests solved by this issue as "fixed" so that I don't investigate them by mistake while continuing my work on the customer module.

Found an interesting scenario when testing #8947 to expand another usage of recordOrShowDatabaseError(), the following test which tests the scenario when a property that does not exist is accessed in BufferImpl.dereference() fails an assertion in FWD when the block uses ON ERROR UNDO, THROW because in 4GL the ERROR-STATUS:ERROR is TRUE, while using LEAVE makes it FALSE See exampleSee example

USING OpenEdge.Core.Assert.

ROUTINE-LEVEL ON ERROR UNDO, THROW.

CLASS TestDereferenceNullProperty:
    DEFINE PRIVATE TEMP-TABLE tt2 NO-UNDO FIELD f2 AS CHARACTER.

    @Setup.
    METHOD PUBLIC VOID Setup():
        FOR EACH tt2. DELETE tt2. END.
    END METHOD.

    @Test.
    METHOD PUBLIC VOID test-buffer-dereference4():
        CREATE tt2. tt2.f2 = "A". RELEASE tt2.
        DEFINE VARIABLE pth AS HANDLE.
        CREATE BUFFER pth FOR TABLE BUFFER tt2:HANDLE.

        OPEN QUERY q FOR EACH tt2.
        GET FIRST q.
        DO TRANSACTION ON ERROR UNDO, LEAVE:
            pth::f3 NO-ERROR.
            Assert:isFalse(ERROR-STATUS:ERROR). // 4GL FALSE, FWD FALSE
            Assert:Equals(1, ERROR-STATUS:NUM-MESSAGES).
            Assert:Equals(7351, ERROR-STATUS:GET-NUMBER(1)).
        END.
        FINALLY:
            CLOSE QUERY q.
            DELETE OBJECT pth.
        END FINALLY.
    END METHOD.

    @Test.
    METHOD PUBLIC VOID test-buffer-dereference6():
        CREATE tt2. tt2.f2 = "A". RELEASE tt2.
        DEFINE VARIABLE pth AS HANDLE.
        CREATE BUFFER pth FOR TABLE BUFFER tt2:HANDLE.

        OPEN QUERY q FOR EACH tt2.
        GET FIRST q.
        DO TRANSACTION ON ERROR UNDO, THROW:
            pth::f3 NO-ERROR.
            Assert:isTrue(ERROR-STATUS:ERROR). // 4GL TRUE, FWD FALSE
            Assert:Equals(1, ERROR-STATUS:NUM-MESSAGES).
            Assert:Equals(7351, ERROR-STATUS:GET-NUMBER(1)).
        END.
        FINALLY:
            CLOSE QUERY q.
            DELETE OBJECT pth.
        END FINALLY.
    END METHOD.
END CLASS.

USING OpenEdge.Core.Assert.

ROUTINE-LEVEL ON ERROR UNDO, THROW.

CLASS tests.Test9032FQPrevCustom:
    DEFINE TEMP-TABLE tt63 FIELD f1 AS INTEGER.

    @Test.
    METHOD PUBLIC VOID test1():
        CREATE tt63. tt63.f1 = 1. RELEASE tt63.
        FIND FIRST tt63.
        DELETE tt63.

        FIND PREV tt63 NO-ERROR.
        Assert:isTrue(ERROR-STATUS:ERROR). // 4GL yes FWD no
        Assert:Equals(0, ERROR-STATUS:GET-NUMBER(1)). // no error number/message
    END METHOD.
END CLASS.

Interesting scenario where a FIND PREV fails silently, there's no error number/message but the ERROR-STATUS:ERROR flag is set. Even without using NO-ERROR, there is no error displayed.

Tested unittests with the changes from 8643a. It indeed solves a lot of tests in service and some other modules. But it regresses hard base and masterdatamanagement module.

I looked a bit into regressed tests, and managed to create a small test case.

block-level on error undo, throw.

do transaction:
   repeat1:
   repeat:
       repeat2:
       repeat on error undo, leave:
          repeat3:
          repeat on error undo, throw:
             undo, throw new Progress.Lang.AppError("METHOD NOT IMPLEMENTED").
          end.
       end.

       message "abc1".
       leave repeat1.
   end. 

   CATCH e AS Progress.Lang.AppError :
           message "Error".
   END CATCH.
end. 

The output for 4gl and trunk is abc1.
The results for 8643a is Error.

Looking at the changes it seems that it happens because of the change in BlokcManager.processLegacyError noThrowOnError = !lex.isExplicit() && (imple.action != Action.THROW);.

Looking a bit more closely at the example from #8643-30, and the implementation of BlockManager.processLegacyError.

I throw a explicit LegacyError and have a catch block in a nested block. if I have on some block with on error undo phrase along the way back to the catch block, on error phrase with action different then throw takes precedence over catch block.

So I am inclined to say that !lex.isExplicit() check in noThrowOnError = !lex.isExplicit() && (imple.action != Action.THROW); should be removed.
Also the default error phrase of do block from saveRowChangesImpl2 should be changed to OnPhrase[] onPhrase0 = new OnPhrase[]{new OnPhrase(Condition.ERROR, Action.THROW, "save-row-changes-label")};.

Constantin, please review.

The changes make sense, we've seen weird behavior for db-related errors in other places.

But a question: does this test still work in FWD with 8643a?

DEFINE TEMP-TABLE ttsrc13 BEFORE-TABLE bittsrc13 FIELD srcid AS INT FIELD srcname AS CHAR.
DEFINE TEMP-TABLE src13 FIELD srcid AS INTEGER FIELD srcname AS CHARACTER INDEX srcid IS UNIQUE srcid.
DEFINE DATASET dssrc13 FOR ttsrc13.

DEFINE VARIABLE httsrc AS HANDLE NO-UNDO.
DEFINE VARIABLE hsrc AS HANDLE NO-UNDO.

TEMP-TABLE ttsrc13:TRACKING-CHANGES = TRUE.
CREATE ttsrc13. ASSIGN ttsrc13.srcid = 1 ttsrc13.srcName = 's1'.
CREATE ttsrc13. ASSIGN ttsrc13.srcid = 1 ttsrc13.srcName = 's2'.
TEMP-TABLE ttsrc13:TRACKING-CHANGES = FALSE.

httsrc = BUFFER ttsrc13:HANDLE.
hsrc = BUFFER src13:HANDLE.

DEFINE VARIABLE srcsrc13 AS HANDLE NO-UNDO.
CREATE DATA-SOURCE srcsrc13.
srcsrc13:ADD-SOURCE-BUFFER(hsrc, "srcname,srcid").
httsrc:ATTACH-DATA-SOURCE(srcsrc13, "srcid,srcid,srcname,srcname").

DO TRANSACTION ON ERROR UNDO, LEAVE:
    FOR EACH bittsrc13 ON ERROR UNDO, THROW:
        BUFFER bittsrc13:SAVE-ROW-CHANGES().
    END.
END.

I mean, when there is no structured exceptions involved.

Actully I think that the problematic line in this code from #8643-38.

mustManageError = !manageLegacyError && server.mustManageLegacyError(true);

For the testcase from #8643-38, it is sets mustManageError on true when the error is thrown. Which in turn will cause the call TransactionManager.triggerErrorInCaller(num, text, true, prefix); method where ignoreError flag is set.

From the Javadocs I see that this flag is used to ignore the error until it reached the top-level block

/**
 * Defines if an error condition should be ignored until reaching the 
 * caller of a top-level block.
 */

This is the reason why in Constantin's example DO TRANSACTION ON ERROR UNDO, LEAVE: statement is completely ignored. Which shouldn't happen in 4gl.

I run the example from #9298-38 using command line, and got the same result as if running from OE.

Got the exception ** src13 already exists with 1, and then Hello.

Eduard, so there are no changes required for the branch?

Constantin Asofiei wrote:

Eduard, so there are no changes required for the branch?

No, there should be a change. I committed 8643a, rev. 15530. I commented the line where mustManageError is modified.

Eduard, I retested #9353 with 8643a rev. 15530 and the exception is still handled right. ✅

I run unitest for a large application.

Also run unittest of a large GUI app, and harness of a big rest app.

Constantin Asofiei wrote:

Andrei: did you run both fwdtests and unittests?

No, I have only run the exact test that customer provided for #9353. However, I can run them asap, if it is needed.

Andrei Plugaru wrote:

Constantin Asofiei wrote:

Andrei: did you run both fwdtests and unittests?

No, I have only run the exact test that customer provided for #9353. However, I can run them asap, if it is needed.

Yes, please do, we need to do a full round of tests with that customer's app.

I have run the fwdtests and, unfortunately, it seems there are 5 regressions. I would want, however, to make another run tomorrow to be sure they are not false negatives.

Andrei Plugaru wrote:

I have run the fwdtests and, unfortunately, it seems there are 5 regressions. I would want, however, to make another run tomorrow to be sure they are not false negatives.

Please cross-reference with #9359 (and its related tasks) to make sure you are not chasing existing problems unrelated to the #8643 implementation.

Andrei Plugaru wrote:

I have run the fwdtests and, unfortunately, it seems there are 5 regressions. I would want, however, to make another run tomorrow to be sure they are not false negatives.

I re-run fwd tests myself on the same machine Andrei was using, and got the same number of tests failing with 8643a and without 8643a.

But the baseline result for fwd tests that I obtained it is a bit worse that the result Andrei shared with me.

Eduard Soltan wrote:

But the baseline result for fwd tests that I obtained it is a bit worse that the result Andrei shared with me.

Did you restore the database before running the tests?

ETF testing passed

The testing I have done yesterday was probably flawed.

The problem was that I didn't reimport the DB before running the FWD tests for the baseline. As I was also using an older conversion, the old DB probably allowed more tests to pass. Currently, I have reimported the newest DB, rerun the tests and got 30 failing tests as a baseline. Will update as soon as I will have the results for the tests with changes.

Constatin wrote:

Did you restore the database before running the tests?

yes, a couple of times.

Andrei Plugaru wrote:

The testing I have done yesterday was probably flawed.

The problem was that I didn't reimport the DB before running the FWD tests for the baseline. As I was also using an older conversion, the old DB probably allowed more tests to pass. Currently, I have reimported the newest DB, rerun the tests and got 30 failing tests as a baseline. Will update as soon as I will have the results for the tests with changes.

I got the same results, with 30 tests failing.

Andrei Plugaru wrote:

The testing I have done yesterday was probably flawed.

The problem was that I didn't reimport the DB before running the FWD tests for the baseline. As I was also using an older conversion, the old DB probably allowed more tests to pass. Currently, I have reimported the newest DB, rerun the tests and got 30 failing tests as a baseline. Will update as soon as I will have the results for the tests with changes.

I have rerun the tests with the changes, however I still have inconsistent results. The number of failing tests varies from 31 to 27.

I have run again one by one the fwd tests that failed only with 8643a and they passed.

Run CHUI regression tests, and they are passing.

Greg, can I merge 8643a?

Branch 8643a was merged into trunk 15582 and archived.