Project

General

Profile

Bug #10277

Adaptive Query REPOSITION statement/method error handling improvement

Added by Teodor Gorghe about 1 year ago. Updated 12 months ago.

Status:
Test
Priority:
Normal
Assignee:
Teodor Gorghe
Target version:
-
Start date:
Due date:
% Done:

100%

billable:
No
vendor_id:
GCD
case_num:
version_reported:
version_resolved:
reviewer:
Alexandru Lungu, Constantin Asofiei
production:
No
env_name:
topics:

Related issues

Related to Database - Bug #7737: QueryWrapper should either show or throw errors depending on the query type Test

History

#1 Updated by Teodor Gorghe about 1 year ago

We have this tests which fails on FWD (trunk rev 16025):

BLOCK-LEVEL ON ERROR UNDO, THROW.

USING OpenEdge.Core.Assert FROM PROPATH.
USING support.test.AssertExt FROM PROPATH.
USING OpenEdge.Core.Assertion.AssertError FROM PROPATH.

CLASS tests.TestQueryRepositionById:
    DEFINE QUERY q1 FOR adaptivept1 SCROLLING.

    @Test.
    METHOD PUBLIC VOID TestRowUnopenedQuery():
        REPOSITION q1 TO ROW 1 NO-ERROR.
        Assert:Equals(1, ERROR-STATUS:NUM-MESSAGES).
        AssertExt:Error(3163, "Cannot reposition on query q1, which is not opened.").
    END METHOD.

    @Test.
    METHOD PUBLIC VOID TestRowIdInvalidRowIdQuery():
        DEFINE VARIABLE rowObject AS ROWID NO-UNDO.
        rowObject = TO-ROWID ("0x01").
        REPOSITION q1 TO ROWID rowObject NO-ERROR.
        Assert:Equals(1, ERROR-STATUS:NUM-MESSAGES).
        AssertExt:Error(3165, "Could not evaluate reposition amount for query q1.").
    END METHOD.

    @Test.
    METHOD PUBLIC VOID TestRowIdUnopenedPrepareAdaptiveQuery():
        DEFINE VARIABLE rowObject AS ROWID NO-UNDO.
        DEFINE VARIABLE dQuery AS HANDLE NO-UNDO.

        dQuery = QUERY q1:HANDLE.
        dQuery:QUERY-PREPARE ("FOR EACH adaptivept1").
        rowObject = TO-ROWID ("0x01").
        dQuery:REPOSITION-TO-ROWID (rowObject) NO-ERROR.
        Assert:Equals(1, ERROR-STATUS:NUM-MESSAGES).
        AssertExt:Error(7315, "Cannot REPOSITION QUERY q1 until it is opened.").
    END METHOD.

    @Test.
    METHOD PUBLIC VOID TestRowIdUnopenedAdaptiveQuery():
        DEFINE VARIABLE rowObject AS ROWID NO-UNDO.
        DEFINE VARIABLE dQuery AS HANDLE NO-UNDO.

        dQuery = QUERY q1:HANDLE.
        rowObject = TO-ROWID ("0x01").
        dQuery:REPOSITION-TO-ROWID (rowObject) NO-ERROR.
        Assert:Equals(1, ERROR-STATUS:NUM-MESSAGES).
        AssertExt:Error(7315, "Cannot REPOSITION QUERY q1 until it is opened.").
    END METHOD.

    @Test.
    METHOD PUBLIC VOID TestRowIdUnopenedDefaultDelegateQuery():
        DEFINE VARIABLE dQuery AS HANDLE NO-UNDO.
        DEFINE VARIABLE rowObject AS ROWID NO-UNDO.
        rowObject = TO-ROWID ("0x01").

        CREATE QUERY dQuery.
        dQuery:REPOSITION-TO-ROWID (rowObject) NO-ERROR.
        Assert:Equals(1, ERROR-STATUS:NUM-MESSAGES).
        AssertExt:Error(7315, "Cannot REPOSITION QUERY  until it is opened.").
    END METHOD.
END CLASS.

FWD test logs:
The most important issue that needs to be fixed is the tests that fails because of NullPointerException (TestRowIdUnopenedPrepareAdaptiveQuery and TestRowIdUnopenedAdaptiveQuery).

#3 Updated by Teodor Gorghe about 1 year ago

  • Status changed from New to WIP

Created task branch 10277a.

#4 Updated by Teodor Gorghe about 1 year ago

  • % Done changed from 0 to 30
  • Assignee set to Teodor Gorghe

Committed revision 16032 on task branch 10277a:
- fixed canReposition when the query is not opened.
- changed error code and message to match with the OE error message.

Please note that there are still issues on static queries, but this should fix some issues on customer application.

#5 Updated by Teodor Gorghe about 1 year ago

  • % Done changed from 30 to 50
  • Status changed from WIP to Review
  • reviewer Constantin Asofiei added

Committed revision 16033 on task branch 10277a:
- fix error throw on handle queries.

Constantin, please review this. The changes until now should fix the NullPointerException issue and the incorrect error message and type on dynamic queries. This does not fix the testRowUnopenedQuery and testRowIdInvalidRowIdQuery tests, as I don't see this issue on a customer application.

#6 Updated by Constantin Asofiei about 1 year ago

OK, please make sure the tests are committed to xfer.

#7 Updated by Teodor Gorghe about 1 year ago

Committed revision 1787 on testcases:
- added tests/query/TestQueryRepositionErrorHandling.cls

#8 Updated by Constantin Asofiei about 1 year ago

Please find the original task for the canReposition line you've changed from 'show' to 'throw' and the error number. We need to make sure this is not somehow related to BROWSE'd query.

#9 Updated by Teodor Gorghe 12 months ago

I have found the #7737, which changes recordOrThrowError into recordOrShowError on QueryWrapper.canReposition. I am running the cases right now and I will come with an answer later on.

#10 Updated by Dănuț Filimon 12 months ago

Teodor Gorghe wrote:

I have found the #7737, which changes recordOrThrowError into recordOrShowError on QueryWrapper.canReposition. I am running the cases right now and I will come with an answer later on.

Just want to confirm that you should make sure to run the forward only test sets from xfer testcases/1655.

#11 Updated by Teodor Gorghe 12 months ago

I did some research for 4GL behavior.
The recordOrShowError solution seems OK, but the problem is on BLOCK-LEVEL ON ERROR UNDO, THROW. statement, which makes dynamic queries to be also thrown (and stop execution afterwards).

On FWD this does not happen and this is why I have initially changed that back into recordOrShowError. I need to investigate this more, maybe there is a wrong parameter on recordOrShowError call.

#12 Updated by Alexandru Lungu 12 months ago

  • Related to Bug #7737: QueryWrapper should either show or throw errors depending on the query type added

#13 Updated by Teodor Gorghe 12 months ago

The only cause of this issue what I have found is that there is a CATCH block queryReposition(NumberType) which cancels every exception thrown by recordOrShowError, when the BLOCK-LEVEL ON ERROR UNDO, THROW. is used.

When BLOCK-LEVEL ON ERROR UNDO, THROW. is not used, then recordOrShowError does not throw any error, which makes this method to return TRUE instead of FALSE.

I thinking for a workaround on this, to make reposition to throw an error different than ErrorConditionException and then to catch errors that are not ErrorConditionException.

#14 Updated by Teodor Gorghe 12 months ago

Committed revision 16034 on task branch 10277a:
- Replaced recordOrThrowError with recordOrShowError. Please check queryReposition(NumberType), I have copied the implementation to throw the exception or return FALSE when using recordOrShowError.

#15 Updated by Constantin Asofiei 12 months ago

Teodor, I can't tell how safe the change is:

   public logical queryReposition(NumberType n)
   {
      if (!canReposition())
      {
         return logical.FALSE;
      }

      getDelegate().reposition(n);
      maybeFireCallback();

      return logical.TRUE;
   }

Previously any ERROR condition from the reposition(n) call (including the canReposition() check?) was ignored. Now any ERROR condition is thrown.

#16 Updated by Teodor Gorghe 12 months ago

  • Status changed from Review to WIP

canReposition looks good for me because recordOrShowError does not throw error on block execution when the BLOCK-LEVEL ON ERROR UNDO, THROW. flag is not set.
I need to double check for more error, to see how behaves on 4GL, dynamic and static queries.
How to actually set the ERROR-STATUS:ERROR flag on FWD? The only way I know which works is recordOrThrowError, but this throws exception on all cases.

#17 Updated by Teodor Gorghe 12 months ago

Committed revision 16035 on task branch 10277a:
- dynamic queries does not throw or show any error message when the ROW is unknown.

Static queries throws error when the ROW parameter is unknown.
I don't see any error throw of an ErrorConditionException on any query reposition delegate implementations.

#18 Updated by Teodor Gorghe 12 months ago

  • Status changed from WIP to Review

#19 Updated by Constantin Asofiei 12 months ago

  • reviewer Alexandru Lungu added

Alexandru, can you take a look, too, please?

#20 Updated by Alexandru Lungu 12 months ago

Alexandru, can you take a look, too, please?

Hmm, I was inclining to share the concern of #10277-15. But honestly I think that the catch implementation was a poor design choice of handling the !canReposition case without making the reposition method actually return boolean. Other ERROR conditions should have been honored, but I don't have example of other cases where REPOSITION is throwing an error.

  • please check that repositioning outside a query works properly. Usually we use QueryOffEndException to signal that and should be caught internally in FWD. Removing that catch may "leak" the QOEE. I am not sure though.

I am also concerned that there are other queryReposition... methods that do the same (see queryRepositionByID). Can we get this fixed completely if time permits? The pattern should be the same.

#21 Updated by Teodor Gorghe 12 months ago

Alexandru Lungu wrote:

Alexandru, can you take a look, too, please?

Hmm, I was inclining to share the concern of #10277-15. But honestly I think that the catch implementation was a poor design choice of handling the !canReposition case without making the reposition method actually return boolean. Other ERROR conditions should have been honored, but I don't have example of other cases where REPOSITION is throwing an error.

  • please check that repositioning outside a query works properly. Usually we use QueryOffEndException to signal that and should be caught internally in FWD. Removing that catch may "leak" the QOEE. I am not sure though.

I am also concerned that there are other queryReposition... methods that do the same (see queryRepositionByID). Can we get this fixed completely if time permits? The pattern should be the same.

QueryOffEndException directly inherits ConditionException, these exceptions does not enter into the catch that I have removed (does not inherit ErrorConditionException).
I was also thinking about other reposition types. I don't know how the conversion and runtime behaves if we change the static REPOSITION methods to return boolean, but another solution to this is to make private methods in the same class, which shares common implementation. queryReposition implementation was simple, but there is queryRepositionByID(rowid[]) which has a little bit more logic into it.

#22 Updated by Alexandru Lungu 12 months ago

queryReposition implementation was simple, but there is queryRepositionByID(rowid[]) which has a little bit more logic into it.

  • queryRepositionByID(rowid[] joinIDs) shouldn't be changed
  • queryRepositionByID(rowid id1, rowid...joinIDs) can be changed, but still call repositionByID(id1, joinIDs); without wrapping it in a try-catch. You call duplicate the canReposition conditional as you please.

#23 Updated by Teodor Gorghe 12 months ago

Committed revision 16036 on task branch 10277a:
- Added canReposition check for queryRepositionByID. Removed catch block.

#24 Updated by Alexandru Lungu 12 months ago

  • Status changed from Review to Internal Test

I am OK with the changes.

#25 Updated by Constantin Asofiei 12 months ago

  • Status changed from Internal Test to Merge Pending

Please merge now and also update the %Done

#26 Updated by Teodor Gorghe 12 months ago

  • Status changed from Merge Pending to Test
  • % Done changed from 50 to 100

Branch 10277a was merged to trunk rev 16082 and archived.

Also available in: Atom PDF