FWD » Core Development » Database

This issue is for adding runtime support for all the database methods added in issue #1947. For the most part, we only added stubs, now they need to work.

The following static methods should be removed from AbstractQuery:

queryClose
queryForward
queryReposition
queryRepositionByID

These were added so they could be called both from the AbstractQuery and QueryWrapper instance methods of the same names. Instead, the implementations of these methods should be directly in the instance methods in AbstractQuery. The QueryWrapper instance methods should be delegating calls to QueryWrapper's internal delegate's instance methods, rather than calling these static methods.

Does P2J have common module responsible for 4GL errors processing? (I found actually ErrorManager.recordOrThrowError(...) after posted the question)

Where can I find full 4GL errors list?

I have an suspicion that errors

"The first argument to QUERY GET methods must be NO-LOCK, SHARE-LOCK or EXCLUSIVE-LOCK (7314)"

and

"QUERY GET second argument must be NO-WAIT or 0. (7360)"

are actually concatenated (or substituted) from

1) method context ("QUERY GET methods", "QUERY GET")
2) actual (possible parametrerized) 4GL errors
"The first argument to %1 must be NO-LOCK, SHARE-LOCK or EXCLUSIVE-LOCK (7314)"
"%1 second argument must be NO-WAIT or 0. (7360)"

Vadim Nebogatov wrote:

Does P2J have common module responsible for 4GL errors processing? (I found actually ErrorManager.recordOrThrowError(...) after posted the question)

Yes, that's the one.

Where can I find full 4GL errors list?

We just replicate errors that occur when running test cases, either scenarios we come up with or scenarios we see in actual programs.

I have an suspicion that errors

"The first argument to QUERY GET methods must be NO-LOCK, SHARE-LOCK or EXCLUSIVE-LOCK (7314)"

and

"QUERY GET second argument must be NO-WAIT or 0. (7360)"

are actually concatenated (or substituted) from

1) method context ("QUERY GET methods", "QUERY GET")
2) actual (possible parametrerized) 4GL errors
"The first argument to %1 must be NO-LOCK, SHARE-LOCK or EXCLUSIVE-LOCK (7314)"
"%1 second argument must be NO-WAIT or 0. (7360)"

Sounds likely. We can implement however it makes the best sense for us.

Code review 20130307a:

• There is no implementation for QueryWrapper.getNext(LockType). The other getXXX(LockType) methods are implemented, so it looks like this was just an oversight.
• In AbstractQuery, you have changed the no-argument forms of get{Next|Previous|First|Last}() to use wrapXXXX(LockType) with LockType.NONE, which I believe is incorrect. I think the previous implementation was correct. By forcing LockType.NONE, you are overriding any locking which was defined when the query was created. I'm pretty sure that when the no-arg form of these methods is invoked, Progress honors the lock type specified when the query was created.
• In BufferImpl, none of the findXXXX(String whereClause, ...) methods will work. As I wrote in note 3 above, do not implement these yet. We can only implement the no-arg versions at this point. The where clause strings have to be converted from 4GL syntax to HQL, which requires an entire dynamic, conversion-at-runtime infrastructure that does not exist yet.
• This is actually not new code, but looking through the Buffer/BufferImpl.findXXXX methods again, I think the lock-related parameter types are not right. I think we need to redefine this API to accept the same combination of lock type parameters we decided to use for all the P2JQuery.getXXXX methods. Please refactor the API accordingly, and update rules/annotations/database_general.rules to handle these methods along with all the others and create a test case similar to get-with-locks.p, to confirm correct conversion.

Everything else looks good.

Code review 20130311a:

The changes all look good.

I noticed on this second review that even the no-arg versions of Buffer.findXXXX will not work as implemented, because of the missing conversion-at-runtime facility. This is because we need to perform index selection on the query (part of the annotations phase of conversion), in order to generate an "order by" clause to pass in as the 4th parameter to the FindQuery constructor. In the current implementation, we simply pass in null, which will not give the database enough information to return the correct (first or last) record. For now, please just add a TODO comment to these implementations, so we don't forget to come around later and fix this.

Also, Constantin committed an overlapping change since you posted this. I don't think his changes conflict with yours, but they affect some of the same files. Please merge up to the latest level of bzr and re-submit, then I will run your update through conversion regression testing.

P2JQuery,

• 040 CA 20130311 Added bufferHandle(integer).

It seems wrong number

Code review 20130311c:

The merge looks fine. The implementation of BufferImpl.bufferCompare doesn't look right. I think you would just wrap a call to RecordBuffer.compare in a try-catch to handle the possible exception that gets thrown, much like you have with the other methods you implemented recently.

Nevertheless, I'm going to put this through regression testing as is, since it is mostly the API and conversion changes we care about right now. So, don't provide another version of this update, just fix this in the ongoing runtime work you are doing, and include that in your next update.

Update vmn_upd20130311c.zip has passed conversion regression testing. Please commit it to bzr and distribute. Note the revision number under this history.

Please also commit the test case in 0311d to the testcases project.

vmn_upd20130311c.zip checked in as bzr revision 10278.

Tests
uast/find-with-locks.p
uast/get-with-locks.p
committed revision 991.

Eric, I corrected BufferImpl.bufferCompare according #12, but noticed that you replaced function body with ... UnsupportedOperationException.
May I return (corrected) implementation back?

Yes, I handed my changes off to Constantin a few days ago, based on an older version, to integrate with some other changes he was making. I put the UnsupportedOperationExceptions in there so we wouldn't forget to implement these; they are not meant to be permanent. Please go ahead and replace that with the correct implementation.

Current status of methods from ref 1947

Implemented:

GET-CURRENT (KW_GET_CUR)
FIND-LAST (KW_FIND_LST)
FIND-FIRST (KW_FIND_1ST)
REPOSITION-TO-ROWID (KW_REPOS_2I)
QUERY-CLOSE (KW_QRY_CLOS)
BUFFER-RELEASE (KW_BUF_REL)
FIND-UNIQUE (KW_FIND_UNI)
REPOSITION-FORWARD (KW_REPOS_F)
REPOSITION-TO-ROW (KW_REPOS_2R)

Partially implemented:

BUFFER-COPY (KW_BUF_COPY)
BUFFER-COMPARE (KW_BUF_COMP)

Not implemented:

BUFFER-FIELD (KW_BUF_FLD)
GET-BUFFER-HANDLE (KW_GET_BUFH)
FIND-BY-ROWID (KW_FIND_BR)

The FIND-FIRST/LAST/UNIQUE methods can receive a character for the first parameter. Currently, the Buffer.find* implementations receive only a String - Eric, do we duplicate the APIs or always wrap the first parameter ?

I suspect that much of the use we will see of this feature will involve expressions resolving to character values (since much of the point of a dynamic find is to dynamically put together the condition/filter). Let's always wrap the first parameter. If we come across a lot of cases where string literals are being passed in, we can revisit this approach and add more API variants later as a code improvement.

Code review 20130312a:

Update looks fine.

Good catch on the queryReposition method. One question: why not just implement AbstractQuery.queryReposition(NumberType n) as

return queryReposition(n.intValue());

instead of duplicating all the scaffolding code?

With your next update, please replace the predicate data type of String in all the Buffer.findXXXX method variants with character (see notes 20-21 above).

Constantin: you didn't already do this, right?

The current project never uses the NO-LOBS parameter of BUFFER-COPY and BUFFER-COMPARE, so the variants of BufferImpl.buffer{Copy|Compare} which take this parameter can continue to throw UnsupportedOperationException for now. All the other variants are in use and need to be implemented. This means we will need updates to the backing implementations of these methods in the RecordBuffer class.

Eric Faulhaber wrote:

One question: why not just implement AbstractQuery.queryReposition(NumberType n) as
[...]

instead of duplicating all the scaffolding code?

I found two different methods reposition(NumberType row) and reposition(int row) and thought maybe it was important for some reasons

Low priority: related to my last note, we are using the wrong data type for the NO-LOBS parameter in our bufferCopy/bufferCompare API. It should be logical, not character.

Vadim Nebogatov wrote:

Eric Faulhaber wrote:

One question: why not just implement AbstractQuery.queryReposition(NumberType n) as
[...]

instead of duplicating all the scaffolding code?

I found two different methods reposition(NumberType row) and reposition(int row) and thought maybe it was important for some reasons

OK, makes sense. You're right, I assumed the int version would be the worker method, but it could be that we have slightly different implementations under the covers. This is implemented in a lot of places, and I don't remember exactly. Safer to go with your approach.

Eric Faulhaber wrote:

With your next update, please replace the predicate data type of String in all the Buffer.findXXXX method variants with character (see notes 20-21 above).

Will be ast processing with rules also required with wrapping String to character?

Vadim Nebogatov wrote:

Will be ast processing with rules also required with wrapping String to character?

Constantin: have you already done this or should Vadim make the conversion change?

for note 22:

Constantin: you didn't already do this, right?

No, I have not changed the Buffer.findXXXX APIs

for note 28:

Will be ast processing with rules also required with wrapping String to character?

No, I have not done the conversion change.

• add a new parameter, idx, which will be the index of the method's parameter
• register the required methods here (i.e. KW_FIND_1ST, KW_FIND_LAST, etc) and to wrap only the first parameter, by adding a test like ... or (idx 0 and (ttype prog.kw_find_1st or ttype == prog.kw_find_last ... etc))

2. base_structure.xml - on line 369, change the evalLib("needs_wrapped_literals", oldtype) call to evalLib("needs_wrapped_literals", oldtype, this.indexPos)

PS: sorry I haven't answered this earlier, I forgot to add myself as a watcher when I posted the comment.

What methods are priority for now?

I am implementing now RecordBuffer.compare(Buffer buffer, character mode, character except, character pairs) similarly to existing RecordBuffer.compare(...).
One question:

               // Simple getter invocations.
               BaseDataType d1 = (BaseDataType) getter1.invoke(rec1);
               BaseDataType d2 = (BaseDataType) getter2.invoke(rec2);

               if (!d1.equals(d2)) 
...

Do we suppose that our getters do not return nulls?

Vadim Nebogatov wrote:

I am implementing now RecordBuffer.compare(Buffer buffer, character mode, character except, character pairs) similarly to existing RecordBuffer.compare(...).
One question:
[...]

Do we suppose that our getters do not return nulls?

Yes, that's right. All the getters return a copy of their internal data, which are never null, so you will never get a null value. If you look at any of the ...dmo.impl.*Impl.java classes, you will see what I mean.

What methods are priority for now?

Continue with this one, then move on to Buffer.bufferCopy. The backing method for that RecordBuffer.copy does not deal with the pairs parameter. After that, just tackle any of the methods you can from #1947, which do not require dynamic database support.

What about pairs in RecordBuffer.compare()? Should it be supported?

Yes, the pairs parameter is in use.

Vadim, have you done the changes at notes #29? If yes, please upload just the common-progress.rules file. If not, I can take the common-progress.rules change and upload it here, because there is another case where the first literal needs to be wrapped (the QUOTER function).

Constantin Asofiei wrote:

Vadim, have you done the changes at notes #29? If yes, please upload just the common-progress.rules file. If not, I can take the common-progress.rules change and upload it here, because there is another case where the first literal needs to be wrapped (the QUOTER function).

Constantin, yes, it was included to #30 update

Vadim: the 20130314a update looks good. Please go ahead and commit it to bzr.

Constantin: can you just pick up common-progress.rules from bzr and merge your quoter changes in?

BTW, as I reviewed the FIND-FIRST, etc. changes, it seems like we can probably do away with the idx == 0 check in common-progress and just wrap all the literals for these methods. The other parameters are just about the locks (all the long/long, long/NumberType, etc. combinations). I think the case where a developer uses literals that are not the well-known constants will be very rare (basically, a programming error), so it seems it would be reasonable to just offer the NumberType variants. Anyway, something for a later update...let's go with this code as it is for now.

Constantin: can you just pick up common-progress.rules from bzr and merge your quoter changes in?

OK, I'll wait for the bzr commit.

BTW, as I reviewed the FIND-FIRST, etc. changes, it seems like we can probably do away with the idx == 0 check in common-progress and just wrap all the literals for these methods. The other parameters are just about the locks (all the long/long, long/NumberType, etc. combinations). I think the case where a developer uses literals that are not the well-known constants will be very rare (basically, a programming error), so it seems it would be reasonable to just offer the NumberType variants. Anyway, something for a later update...let's go with this code as it is for now.

For QUOTER, it's OK to wrap just the first parameter, so the common-progress.rules are good.

update [vmn_upd20130314a.zip]

Refs #2080, checked in as bzr revision 10291.

Corrected runtime for BUFFER-COPY (KW_BUF_COPY) and BUFFER-COMPARE (KW_BUF_COMP)
Corrected both conversion and runtime for REPOSITION-TO-ROW (KW_REPOS_2R)
First parameter type changed from String to character for FIND-UNIQUE (KW_GET_UNI), FIND-FIRST (KW_FIND_1ST), FIND-LAST (KW_FIND_LST)
data type for the NO-LOBS parameter is changed from character to logical for bufferCompare (note 25)

It seems,

FIND-FIRST(...)

Simple example below

def var hBuffer1 as handle.

def temp-table tt
field f1 as int
field f2 as int
field f3 as int.

create Buffer hBuffer1 for table "tt".

create tt.
assign f1 = 1
f2 = 2
f3 = 3.

hBuffer1:FIND-FIRST().

[03/16/2013 00:33:38 MSK] (ErrorManager:SEVERE) {00000001:00000006:bogus} Invalid handle.  Not initialized or points to a deleted object. (3135)
[03/16/2013 00:33:39 MSK] (ErrorManager:SEVERE) {00000001:00000006:bogus} Cannot access the findFirst attribute because the widget does not exist. (3140)

Vadim, I don't think the runtime for the CREATE BUFFER statement is implemented. Try this instead:

def var hBuffer1 as handle.

def temp-table tt
field f1 as int
field f2 as int
field f3 as int.

def buffer b for tt.
hBuffer1 = buffer b:handle.

create tt.
assign tt.f1 = 1
tt.f2 = 2
tt.f3 = 3.

hBuffer1:FIND-FIRST().

Yes, it works, thank you

Constantin's advice has gotten you past the immediate handle problem, but please re-read note 9. The runtime implementation of find{First|Last} will not work reliably for any table containing more than one record, because we do not yet have the dynamic database support in place to select an index to use for sorting.

An essential piece of information which FindQuery needs to find the first, last, next, or previous record is how the records are sorted. This requires that conversion selects an index on the table to walk, which determines the sort clause we pass to the FindQuery constructor. In the absence of a sort clause, the backing database will return records in an indeterminate order. Your test case works because there is only one record in the table, but this is not a robust test. If you are going to implement the find{First|Last} methods at this time (which is OK), you should add a TODO noting that dynamic conversion is required to generate the proper sort clause. For that matter, you can begin implementing the other variants, with a similar TODO about the where clause.

Note that the above doesn't apply to findUnique, since there should only be one matching record.

I have noticed from testing in 4GL that error message

Datatypes in pairlist argument for ATTACH-DATA-SOURCE, BUFFER-COPY or BUFFER-COMPARE 
methods must match for fields f2 and f3. (12860)

actually happens

1) only for BUFFER-COPY (no one time was reproduced for BUFFER-COMPARE)
2) only if

pairs

I did not check ATTACH-DATA-SOURCE

Also I have noticed the difference in behavior of 4GL and converted code.

I am comparing a lot of tests in the form:

def var BOOL as logical no-undo.

...

DO TRANSACTION ON ERROR UNDO, LEAVE:
BOOL = hBuffer3:buffer-copy(hBuffer4, "f1,f2,f3", "f2,f2").
END.
message "Result 1.1: " string(BOOL).

...

This form is convinient for test because cathes runtime ERRORs and allows continue 4GL code execution after error inside DO TRANSACTION block and use many ERRORs in one 4GL app.

I noticed that, if error happens in DO TRANSACTION block, 4GL leaves block without change BOOL value, but converted code changes BOOL value, i.e. does not leave block.

One more related question: how to check returning value in 4GL in such case?

This indicates that the idiom of:

try
   do something
catch ErrorConditionException
   return false

is incorrect (and not just here, I'm sure, but in all the methods/attributes which return logical), which I should have known. We must let the ErrorConditionException propagate up the stack. However, I don't know what the return value should be, if it is not an indication of the success of the operation.

Yes, I also think so. Return value has no sense if 4gl propagates its "exceptions". I only thought that maybe this propagation mechanism depends on some 4GL settings (ON ERROR or ROUTINE-LEVEL ON ERROR UNDO, THROW).

Eric/Vadim: there is another way to supress the 4GL's errors, the NO-ERROR clause. When trying to understand when a function method returns false, it will be easier if you add the NO-ERROR clause, as in:

BOOL = hBuffer3:buffer-copy(hBuffer4, "f1,f2,f3", "f2,f2") NO-ERROR.
message "Result 1.1: " string(BOOL).

Constantin: yes, it is just switch in behavior I was looking.
Conversion code should also support this option, returning value if ON-ERROR is set (current behaviour) and propagating exception up the stack otherwise.

Block

DO TRANSACTION ON ERROR UNDO, LEAVE:
...
END.

Conversion code should also support this option, returning value if ON-ERROR is set (current behaviour) and propagating exception up the stack otherwise.

Correct, if you want to see what kind of condition is raised (ERROR, STOP, QUIT, etc) enclose the statement in a block which treats that condition.

Another observation. Not all 4GL errors throw “exceptions”, some of them look like not critical errors or warnings, error message is printed in such case, but result is returned.

Please see below small example with 2 such errors:

BUFFER-COMPARE must have records in both target and source buffers. (9039)

Datatypes in pairlist argument for ATTACH-DATA-SOURCE, BUFFER-COPY or BUFFER-COMPARE 
methods must match for fields f1 and f1. (12860)

Critical error, result is not returned

In order to suppress real warnings I have set SESSION:SUPPRESS-WARNINGS = YES.

SESSION:SUPPRESS-WARNINGS = YES.
def var bool as logical no-undo.
/* tt1 */
def temp-table tt1
field f1 as character
field f2 as character
field f3 as int
field f4 as int.
create tt1.
assign f1 = "aa" 
f2 = "bb" 
f3 = 3
f4 = 4.
def buffer b1 for tt1.
def var hBuffer1 as handle.
hBuffer1 = buffer b1:handle.
/* tt3 */
def temp-table tt3
field f1 as character
field f2 as character
field f3 as int.
create tt3.
assign tt3.f1 = "aa" 
tt3.f2 = "bb" 
tt3.f3 = 3.
def buffer b3 for tt3.
def var hBuffer3 as handle.
hBuffer3 = buffer b3:handle.
/* tt4 */
def temp-table tt4
field f1 as int
field f2 as character
field f3 as int.
create tt4.
assign tt4.f1 = 7
tt4.f2 = "bb" 
tt4.f3 = 3.
def buffer b4 for tt4.
def var hBuffer4 as handle.
hBuffer4 = buffer b4:handle.

DO TRANSACTION ON ERROR UNDO, LEAVE:
bool=hBuffer1:buffer-compare(hBuffer4).
message "Result 7.1: " string(bool).
END.

hBuffer3:FIND-FIRST().
hBuffer4:FIND-FIRST().
hBuffer1:FIND-FIRST().

DO TRANSACTION ON ERROR UNDO, LEAVE:
bool=hBuffer3:buffer-copy(hBuffer4, "f1,     f2,     f3", "f1,f1").
message "Result 7.2: " string(bool).SESSION:SUPPRESS-WARNINGS = YES.
def var bool as logical no-undo.
/* tt1 */
def temp-table tt1
field f1 as character
field f2 as character
field f3 as int
field f4 as int.
create tt1.
assign f1 = "aa" 
f2 = "bb" 
f3 = 3
f4 = 4.
def buffer b1 for tt1.
def var hBuffer1 as handle.
hBuffer1 = buffer b1:handle.
/* tt3 */
def temp-table tt3
field f1 as character
field f2 as character
field f3 as int.
create tt3.
assign tt3.f1 = "aa" 
tt3.f2 = "bb" 
tt3.f3 = 3.
def buffer b3 for tt3.
def var hBuffer3 as handle.
hBuffer3 = buffer b3:handle.
/* tt4 */
def temp-table tt4
field f1 as int
field f2 as character
field f3 as int.
create tt4.
assign tt4.f1 = 7
tt4.f2 = "bb" 
tt4.f3 = 3.
def buffer b4 for tt4.
def var hBuffer4 as handle.
hBuffer4 = buffer b4:handle.

DO TRANSACTION ON ERROR UNDO, LEAVE:
bool=hBuffer1:buffer-compare(hBuffer4).
message "Result 7.1: " string(bool).
END.

hBuffer3:FIND-FIRST().
hBuffer4:FIND-FIRST().
hBuffer1:FIND-FIRST().

DO TRANSACTION ON ERROR UNDO, LEAVE:
bool=hBuffer3:buffer-copy(hBuffer4, "f1,     f2,     f3", "f1,f1").
message "Result 7.2: " string(bool).
END.

END.

Another observation. Not all 4GL errors throw “exceptions”, some of them look like not critical errors or warnings, error message is printed in such case, but result is returned.

For such cases, please look at the ErrorManager's APIs, like recordOrShowEror, recordOrThrowError, displayError, etc. Note that sometimes errors are shown with or without a starting **. More, you can use ERROR-STATUS's ERROR and related attributes to check if an error was registered by the call. Depending on the ERROR flag, you will decide on which ErrorManager API to use for the error.

PS: about SESSION:SUPPRESS-WARNINGS = YES.. Note that in the 4GL editor, if you run a program which doesn't set the SESSION:SUPPRESS-WARNINGS attribute after a program which set it, it will inherit the value of the SESSION:SUPPRESS-WARNINGS from the previous run. Thus, when dealing with errors, is recommended to start the test program from the command line, using the pro -p <progname> command. If you need perm database support, use pro -p <progname> -db <location of your p2j_test.db file>

Constantin, I use ErrorManager.recordOrThrowError.
In my example above, 4GL meets errors 9039 and 12860 with different way in the same environment.
Is this information available in converted code (by error number or by any criterion)?

DO ON ERROR UNDO, LEAVE:
   /* line of code which shows warning/raises error here */
   MESSAGE "case x: no error raised".
END.
MESSAGE "had error: " ERROR-STATUS:ERROR. /* check if error was registered */

• if the line of code shows error/warning message but the "case x: no error raised" is not shown, then error is raised
• if the line of code shows warning message and the "case x: no error raised" is shown, error is not raised
• use ERROR-STATUS:ERROR and other ERROR-STATUS's attributes to check how the error/warning was registered.

If you use SESSION:SUPPRESS-WARNINGS = YES., you will lose sight of generated warning messages.

Constantin,
I have removed SESSION:SUPPRESS-WARNINGS = YES. and started from command line. But it did not affect result.
I also inserted lines like you recommended, so example code now is

def var bool as logical no-undo.

/* tt1 */
def temp-table tt1
field f1 as character
field f2 as character
field f3 as int
field f4 as int.
create tt1.
assign f1 = "aa" 
f2 = "bb" 
f3 = 3
f4 = 4.
def buffer b1 for tt1.
def var hBuffer1 as handle.
hBuffer1 = buffer b1:handle.
/* tt3 */
def temp-table tt3
field f1 as character
field f2 as character
field f3 as int.
create tt3.
assign tt3.f1 = "aa" 
tt3.f2 = "bb" 
tt3.f3 = 3.
def buffer b3 for tt3.
def var hBuffer3 as handle.
hBuffer3 = buffer b3:handle.
/* tt4 */
def temp-table tt4
field f1 as int
field f2 as character
field f3 as int.
create tt4.
assign tt4.f1 = 7
tt4.f2 = "bb" 
tt4.f3 = 3.
def buffer b4 for tt4.
def var hBuffer4 as handle.
hBuffer4 = buffer b4:handle.

DO TRANSACTION ON ERROR UNDO, LEAVE:
bool=hBuffer1:buffer-compare(hBuffer4).
message "case 1: no error raised. Result = " string(bool).
END.
MESSAGE "had error: " ERROR-STATUS:ERROR.
hBuffer3:FIND-FIRST().
hBuffer4:FIND-FIRST().
hBuffer1:FIND-FIRST().

DO TRANSACTION ON ERROR UNDO, LEAVE:
bool=hBuffer3:buffer-copy(hBuffer4, "f1,     f2,     f3", "f1,f1").
message "case 2: no error raised. Result = " string(bool).
END.
MESSAGE "had error: " ERROR-STATUS:ERROR.

and execution result in 4GL is

BUFFER-COMPARE must have records in both target and source buffers. (9039)
case 1: no error raised. Result =  no
had error:  no
Datatypes in pairlist argument for ATTACH-DATA-SOURCE, BUFFER-COPY or BUFFER-COMPARE methods must match for fields f1 and f1. (12860)
had error:  no

The question is the same: how P2J could distinct these errors? Please notice that first error does not throw 4GL “exception” and returns value “no”

1. the 9039 message does not come with a raised error. Try executing the BUFFER-COMPARE with a NO-ERROR clause and if you check the ERROR-STATUS:ERROR attribute after that, you will see that is still NO. So, that case doesn't raise an error condition. But, if you check the ERROR:GET-MESSAGE(1), you will see that the message is recorded. I don't think ErrorManager has an API which allows a pseudo-error message to be logged, but not registered in the ERROR attribute. I think you need to write one for this case or, if we don't need to worry about this now, use ErrorManager.displayError instead (Greg, please provide some feedback on this).
2. the 12860 message comes with a raised error, so this case is simple - use ErrorManager.recordOrThrowError

Greg, as a side note, for the cases like 9039 (found in the #1920 task) I used ErrorManager.displayError, at that time I did not check the GET-MESSAGE for an error message. Can't tell how much impact this will have on the server runtime, but at this time what I think is that our error reporting/raising mechanism is missing at least the case for 9039 message.

I want to resolve this properly as part of this work. This includes any cases of ErrorManager.displayError() that should be storing the message in the error status.

Vadim: please do the following:

1. Find all places in P2J that use ErrorManager.displayError().
2. Based on the locations, please try to create a testcases to recreate each error. As part of the testcase, check if the message exists in the ERROR-STATUS handle.
3. Report the results here.

There will be 3 possible approaches from there:

- If all the current usage of ErrorManager.displayError() does not cause the ERROR-STATUS messages to be updated (in the 4GL), then it is already implemented properly. Add a new method to display and store the message, and use it in your new buffer-compare code.

- If some of the current usage of ErrorManager.displayError() DOES cause the ERROR-STATUS messages to be updated (in the 4GL), then those locations will have to be switched over to use something different. Add a new method to display and store the message, and use it in your new buffer-compare code. Also change the current displayError() call sites that have been found to be incorrect.

- If all the current usage of ErrorManager.displayError() DOES cause the ERROR-STATUS messages to be updated (in the 4GL), then it is not implemented properly. Modify that method to display and store the message, and use it in your new buffer-compare code.

Duding testing I have found out that FETCH-SELECTED-ROW() conversion requires BrowseWidget.fetchSelectedRow() with int parameter. So, it woulld be better have as usually two methods with NumberType and long type parameters. Example for reproduce converted code compilation error:

DEFINE temp-table tt FIELD ii AS int.
DEFINE QUERY custq FOR tt.
DEFINE BROWSE custb QUERY custq  DISPLAY ii WITH 10 DOWN MULTIPLE.
DEFINE FRAME brs-frame custb.
custb:FETCH-SELECTED-ROW(5)

Regarding vmn_upd20130326a.zip: there are hundreds of unnecessary/unwanted format changes in RecordBuffer, which makes it very hard to see the actual changes. Please turn off whatever feature in your editor automatically reformats code and resubmit this update. Also, please follow the coding standard with respect to curly braces in StringHelper.parseTokens.

Eric, sorry for so dangerous RecordBuffer.java. I think it was wrong bazaar merge with latest bzr revision. I do not use any formatting features, especially in code I did not touch

I have found also that SESSION:TIMEZONE is not supported (but mentioned in code).
Example is

SESSION:TIMEZONE=7666 NO-ERROR.
MESSAGE "had error: " ERROR-STATUS:ERROR. 
MESSAGE "message 1: " ERROR-STATUS:GET-MESSAGE(1). 
MESSAGE "message 2: " ERROR-STATUS:GET-MESSAGE(2).

In regard to the use of an .odt document for reporting information about the errors, I prefer that you put the information directly into the redmine history. If needed, you can use separate history entries. As is noted in the Developer Handbook:

As a general rule, do not use office documents (word processing, spreadsheets, presentations) except in very special circumstances. One example of a valid use is to create a diagram for use in the documentation for a project. If you believe that an office document is needed for a task, please first obtain the approval and input of your team leader. They may convey other specific standards or requirements as needed.

I realize that you were needing a table format, but we really want to avoid using office docs.

class Date, line 1435

Example:

SESSION:TIMEZONE=7666 NO-ERROR. 
MESSAGE "had error: " ERROR-STATUS:ERROR. 
MESSAGE "message 1: " ERROR-STATUS:GET-MESSAGE(1). 
MESSAGE "message 2: " ERROR-STATUS:GET-MESSAGE(2).

Invalid value 7666 for SESSION:TIMEZONE attribute. (14800) 

**Attribute TIMEZONE for the PSEUDO-WIDGET has an invalid value of 7666. (4057)

had error: no
message 1: Invalid value 7666 for SESSION:TIMEZONE attribute. (14800)
Press space bar to continue.

line 1:9: unexpected token: TIMEZONE 
    at com.goldencode.p2j.uast.ProgressParser.attribute_or_method(ProgressParser.java:46758) 
    at com.goldencode.p2j.uast.ProgressParser.lvalue(ProgressParser.java:13350) 
    at com.goldencode.p2j.uast.ProgressParser.assignment(ProgressParser.java:5524) 
    at com.goldencode.p2j.uast.ProgressParser.single_block(ProgressParser.java:4303) 
    at com.goldencode.p2j.uast.ProgressParser.block(ProgressParser.java:4062) 
    at com.goldencode.p2j.uast.ProgressParser.external_proc(ProgressParser.java:3988) 
    at com.goldencode.p2j.uast.AstGenerator.parse(AstGenerator.java:1416) 
    at com.goldencode.p2j.uast.AstGenerator.processFile(AstGenerator.java:942) 
    at com.goldencode.p2j.uast.AstGenerator.processFile(AstGenerator.java:814) 
    at com.goldencode.p2j.uast.ScanDriver.scan(ScanDriver.java:203) 
    at com.goldencode.p2j.uast.ScanDriver.scan(ScanDriver.java:122) 
    at com.goldencode.p2j.convert.ConP2J versionDriver.runScanDriver(ConversionDriver.java:381) 
    at com.goldencode.p2j.convert.ConversionDriver.front(ConversionDriver.java:278) 
    at com.goldencode.p2j.convert.ConversionDriver.main(ConversionDriver.java:1717)

class ControlFlowOps, line 2991

Example:

DYNAMIC-FUNCTION('qwerty')  NO-ERROR. 
MESSAGE "had error: " ERROR-STATUS:ERROR. 
MESSAGE "message: " ERROR-STATUS:GET-MESSAGE(1). 

User-defined function 'qwerty' invoked dynamically but could not be found. (5639)

User-defined function 'qwerty' invoked dynamically but could not be found. (5639)

But: message is printed before “had error”, so ERROR-STATUS:ERROR.MESSAGE was printed as empty

class ControlFlowOps, line 3577

Example:

RUN "qwerty" NO-ERROR. 
MESSAGE "had error: " ERROR-STATUS:ERROR. 
MESSAGE "message: " ERROR-STATUS:GET-MESSAGE(1).

** "qwerty" was not found. (293) 

** "qwerty" was not found. (293) 

class NumberType, line 3189

Example:

DISPLAY 12334 FORMAT "zzz9" NO-ERROR. 
MESSAGE "had error: " ERROR-STATUS:ERROR. 
MESSAGE "message: " ERROR-STATUS:GET-MESSAGE(1).

** Value 12334 cannot be displayed using zzz9. (74)

** Value 12334 cannot be displayed using zzz9. (74) 

But: message is printed before “had error”, so ERROR-STATUS:ERROR.MESSAGE was printed as empty

Moved from #2080, notes 3,4

class BrowseWidget, line 751

Example:

def temp-table tt field ii as int.
DEFINE QUERY custq FOR tt.
DEFINE BROWSE custb QUERY custq  DISPLAY ii WITH 10 DOWN MULTIPLE.
DEFINE FRAME brs-frame custb.
custb:FETCH-SELECTED-ROW(5)  NO-ERROR.
MESSAGE "ERROR-STATUS:ERROR =" ERROR-STATUS:ERROR ", ERROR-STATUS:NUM-MESSAGES =" ERROR-STATUS:NUM-MESSAGES.
MESSAGE "message 1: " ERROR-STATUS:GET-MESSAGE(1).
MESSAGE "message 2: " ERROR-STATUS:GET-MESSAGE(2).

ERROR-STATUS:ERROR = no , ERROR-STATUS:NUM-MESSAGES = 1
message 1:  Browse widget must contain at least one record before FETCH-SELECTED-ROW 
method is used. (2108)
message 2: 

Runtime error:

java.lang.IndexOutOfBoundsException: Invalid selected row index requested: 4. Only 0 rows are present.
com.goldencode.p2j.ui.client.Browse.getSelectedRowIndex(Browse.java:1143)
com.goldencode.p2j.ui.BrowseConfig.getSelectedRowIndex(BrowseConfig.java:464)
com.goldencode.p2j.ui.chui.ThinClient.getSelectedRowIndex(ThinClient.java:5401)

class handle, line 254

Example:

def temp-table tt  field f1 as int.
def var hBuffer1 as handle.
def buffer b1 for tt.
hBuffer1 = buffer b1:handle.
hBuffer1:NUM-FIELDS=8  NO-ERROR.
MESSAGE "ERROR-STATUS:ERROR =" ERROR-STATUS:ERROR ", ERROR-STATUS:NUM-MESSAGES =" 
ERROR-STATUS:NUM-MESSAGES.
MESSAGE "message 1: " ERROR-STATUS:GET-MESSAGE(1).
MESSAGE "message 2: " ERROR-STATUS:GET-MESSAGE(2).

ERROR-STATUS:ERROR = no , ERROR-STATUS:NUM-MESSAGES = 1
message 1:  **NUM-FIELDS is not a setable attribute for BUFFER widget. (4052)
message 2:  

P2J messages:

**NUM-FIELDS is not a setable attribute for BUFFER widget. (4052)
ERROR-STATUS:ERROR = no , ERROR-STATUS:NUM-MESSAGES = 0
message 1: 
message 2:  

this note should be ignored according note 85

class HandleOps, line 67

Example:

DEFINE VAR hQuery AS HANDLE. 
hQuery:QUERY-CLOSE() NO-ERROR. 
MESSAGE "had error: " ERROR-STATUS:ERROR. 
MESSAGE "message: " ERROR-STATUS:GET-MESSAGE(1). 

Invalid handle.  Not initialized or points to a deleted object. (3135) 

Invalid handle.  Not initialized or points to a deleted object. (3135)

Moved to #2106, note 1

class <Not found in code>

Example:

def var q11 as handle. 
def temp-table tt field ii as int. 
create tt. 
assign ii=4. 
define query q11 for tt. 
open query q11 for each tt. 
def var hQuery as handle. 
hQuery = query q11:handle. 

hQuery:REPOSITION-TO-ROW(1) NO-ERROR. 
MESSAGE "had error: " ERROR-STATUS:ERROR. 
MESSAGE "message 1: " ERROR-STATUS:GET-MESSAGE(1).

Cannot reposition on query q11 which is not defined as SCROLLING. (3164) 

P2J Messages: no messages

Moved to #2106, note 2

class <Not found in code>

Example:

CONNECT vtest NO-ERROR. 
MESSAGE "had error: " ERROR-STATUS:ERROR. 
MESSAGE "message: " ERROR-STATUS:GET-MESSAGE(1). 

** Warning -- database vtest is already connected. (1012) 

P2J: not tested so far

class ControlFlowOps, line 3519

Example:

DEFINE VARIABLE myhand AS HANDLE NO-UNDO. 
FUNCTION qwerty RETURNS INTEGER (INPUT parm1 AS INTEGER) IN myhand. 
qwerty(4) NO-ERROR. 
MESSAGE "had error: " ERROR-STATUS:ERROR. 
MESSAGE "message: " ERROR-STATUS:GET-MESSAGE(1). 

Could not evaluate the expression describing the context of external function 'qwerty'. (2767) 

Could not evaluate the expression describing the context of external function 'qwerty'. (2767)

But: message is printed before “had error”, so ERROR-STATUS:ERROR.MESSAGE was printed as empty

class <Not found in code>

Example:

DEFINE VARIABLE myhand AS HANDLE NO-UNDO. 
FUNCTION qwerty RETURNS INTEGER (INPUT parm1 AS INTEGER) IN myhand. 
RUN /home/vadimn/doubler PERSISTENT SET myhand. 
qwerty(4) NO-ERROR. 
MESSAGE "had error: " ERROR-STATUS:ERROR. 
MESSAGE "message: " ERROR-STATUS:GET-MESSAGE(1). 

External user defined function 'qwerty' is not defined in the procedure context given. (2779) 

P2J: not tested so far

class <Not found in code>

Example:

def var q11 as handle. 
def temp-table tt field ii as int. 
create tt. 
assign ii=?. 
define query q11 for tt. 
open query q11 for each tt. 
message ii. 
q11:REPOSITION-FORWARD(ii) NO-ERROR. 
MESSAGE "had error: " ERROR-STATUS:ERROR. 
MESSAGE "message 1: " ERROR-STATUS:GET-MESSAGE(1). 
MESSAGE "message 2: " ERROR-STATUS:GET-MESSAGE(2). 

Invalid handle.  Not initialized or points to a deleted object. (3135) 

Cannot access the REPOSITION-FORWARDS attribute because the widget does not exist. (3140) 

P2J had error: yes
P2J Messages:

Invalid handle.  Not initialized or points to a deleted object. (3135) 

Cannot access the queryForward attribute because the widget does not exist. (3140) 

class <Not found in code>

Example:

def var q11 as handle. 

def temp-table tt field ii as int. 

create tt. 

assign ii=4. 

define query q11 for tt. 

open query q11 for each tt. 

def var hQuery as handle. 
hQuery:QUERY-CLOSE() NO-ERROR. 

MESSAGE "had error: " ERROR-STATUS:ERROR. 
MESSAGE "message 1: " ERROR-STATUS:GET-MESSAGE(1). 
MESSAGE "message 2: " ERROR-STATUS:GET-MESSAGE(2).

Invalid handle.  Not initialized or points to a deleted object. (3135) 

Cannot access the QUERY-CLOSE attribute because the widget does not exist. (3140) 

P2J had error: yes
P2J Messages:

Invalid handle.  Not initialized or points to a deleted object. (3135) 

Cannot access the queryClose attribute because the widget does not exist. (3140)

Code review 20130328a:

• Why have you commented out the re-throwing of the reason Throwable in BlockManager.processBody with no explanation? This seems to be a very significant change in the control flow of P2J's block processing which has the potential to cause many regressions.
• There is no reason to add "throws ConditionException" to Block.body. It is an unchecked exception, so it really doesn't change the contract of the API. If you intend to add this as way of documenting that implementations of this method may throw this exception, why not add a throws attribute to the JavaDoc of the method instead?
• The JavaDoc for the pairs parameter of the BufferImpl.bufferCopy methods discusses comparing instead of copying.
• RecordBuffer is much easier to review now, thank you. There were already implementations of copy and compare for the language statement versions of BUFFER-COPY/BUFFER-COMPARE. Although I realize the return values and options may differ somewhat, it seems there is a lot of common code that is repeated across implementations. Also, it seems you have figured out more of the Progress error conditions than I did for the original implementations. I suspect some or all of these are necessary in the language statement versions, too. Would it be possible to have shared, core implementations of copy and compare that could be leveraged by both types of feature, so that we can maintain these common features in one place?
• In RecordBuffer.{copy|compare}, you refer to NO-ERROR as a parameter that needs to be supported. Conversion already should be emitting the NO-ERROR option as a pair of bracketed calls to ErrorManager.silentErrorEnable and ErrorManager.silentErrorDisable (i.e., before and after the call to copy or compare). You would then retrieve the NO-ERROR status inside copy/compare, with a call to ErrorManager.isSilentError. If this is not happening, we have to revisit the conversion of these methods to see why not.
• RecordBuffer.getFieldNamePairs needs JavaDoc.
• In RecordBuffer.compare (both your new version and my original version), d1.equals(d2) may not be the best way to perform the comparison of each field. Please investigate whether the compareTo method in the BaseDataType hierarchy, especially in Text would be a better choice.
• Please re-read the JavaDoc for Text.setTemporaryCaseSensitive. I'm not sure it was meant to be used in the way you are using it. If necessary, please consult with the original author (Constantin, I think) to check if your use of it is appropriate.

Vadim Nebogatov wrote:

Duding testing I have found out that FETCH-SELECTED-ROW() conversion requires BrowseWidget.fetchSelectedRow() with int parameter. So, it woulld be better have as usually two methods with NumberType and long type parameters. Example for reproduce converted code compilation error:
[...]

OK, please make the change.

Please edit each case and add the value of the ERROR:NUM-MESSAGES attribute (you can add it in the same MESSAGE line where the ERROR attribute is interogated); this will allow us to check how many messages were logged.

class Date, line 1435 - note 70
- you have a typo in the 14800 error message, please edit the note and fix it

class ControlFlowOps, line 2991 - note 71
- the message is printed before the "had error" because currently ErrorManager.displayError does not honor silent-error mode.

class ControlFlowOps, line 3577 - note 72
- this is OK, as in this case a STOP condition needs to be raised. But, you can test this by enclosing the RUN in a DO ON STOP UNDO, LEAVE block (so the sTOP condition is caught) and after the block, check the ERROR-STATUS data:

do on stop undo,leave:
  run "aa".
end.

message error-status:error error-status:num-messages.

class NumberType, line 3189 - note 73
- same problem as note 71, silent-error mode is not honored + message is not logged

class BrowseWidget, line 751 - note 74
- when you encounter conversion errors in terms of "missing APIs", nothing is stoping you to modify the testcase so that the existing API is used. In this case, if you don't have a fix for the missing API, change the custb:FETCH-SELECTED-ROW(5) to a custb:FETCH-SELECTED-ROW(i) where i is a def var i as int init 5., reconvert, check P2J behaviour, and post the results back to note 74.

class handle, line 254 - note 75
- the differences in behavior are caused by the fact that AFAIK the CREATE BUFFER statement is not yet implemented. Please initialize the hBuffer1 handle using hBuffer1 = temp-table tt:handle. instead of the CREATE BUFFER and post back the results to note 75.

class HandleOps, line 67 - note 76
- the testcase has nothing to do with the code at line 67 in HandleOps. That code is for a DELETE WIDGET h. statement, and this statement does not accept the NO-ERROR clause. And if you check this:

def var h1 as handle.
def var h2 as handle.
delete widget h1 h2.
message "finished: " error-status:error error-status:num-messages.

Invalid handle.  Not initialized or points to a deleted object. (3135)
Invalid handle.  Not initialized or points to a deleted object. (3135)
finished:  no 0

class <Not found in code> - note 77, 78 - I guess the runtime for these is not available yet, right? If so, there was not reason to add these here now, as we need to know how existing runtime behaves.

class ControlFlowOps, line 3519 - note 79
- the problem is the same as for the note 71, silent-error mode is not honored + message is not logged by ErrorManager.displayError

class <Not found in code> - note 80
- this is a missed error in the #1920 task. I've added details there. No further work is needed for this.

class <Not found in code> - note 81
- not sure what you are trying to duplicate here. Your q11 variable is not initialized, so the errors are OK. More, the code for this is in handle.java:1834, and is for ErrorManager.recordOrThrowError, not displayError

class <Not found in code> - note 82
- same as note 81 - not sure what you are trying to duplicate here. Your hQuery variable is not initialized, so the errors are OK. More, the code for this is in handle.java:1834

Note that the scope of this task was to identify all EXISTING usage of ErrorManager.displayError and create tests (or identify existing testcases from the testcases/ project) for each case (please ask if is not obvious how to reach the code from 4GL). You are still missing some cases (handle.java:254, handle.java:1844, and others), but anyway, at this time we have enough data to conclude that we need a new API for such cases, as at least note 72 (for RUN missing-program.p case) and note 76 (DELETE WIDGET case ) need current ErrorManager.displayError. The new API will need to honor silent error mode and save the error data.

After adding the new API, please do not make changes in existing code unless you have a working testcase which you are sure it reaches that code.

Eric Faulhaber wrote:

Code review 20130328a:

• Why have you commented out the re-throwing of the reason Throwable in BlockManager.processBody with no explanation? This seems to be a very significant change in the control flow of P2J's block processing which has the potential to cause many regressions.

I attached this update not as final update but because it was interrupted with errors investigation and for discuss if I am on the right way. I did not comment code in Block and BlockManager because I also understood that it could affect whole application behavior very much. So, I only wanted to discuss how better make converted code behavior closer to 4GL (like we discussed in notes 47-49 )

• There is no reason to add "throws ConditionException" to Block.body. It is an unchecked exception, so it really doesn't change the contract of the API. If you intend to add this as way of documenting that implementations of this method may throw this exception, why not add a throws attribute to the JavaDoc of the method instead?

My fault. I always remembered but forgot namely in this place that it is runtime exception.

• The JavaDoc for the pairs parameter of the BufferImpl.bufferCopy methods discusses comparing instead of copying.

Will correct

• RecordBuffer is much easier to review now, thank you. There were already implementations of copy and compare for the language statement versions of BUFFER-COPY/BUFFER-COMPARE. Although I realize the return values and options may differ somewhat, it seems there is a lot of common code that is repeated across implementations. Also, it seems you have figured out more of the Progress error conditions than I did for the original implementations. I suspect some or all of these are necessary in the language statement versions, too. Would it be possible to have shared, core implementations of copy and compare that could be leveraged by both types of feature, so that we can maintain these common features in one place?

Parameters of methods and statements are very different, but core processing should be the same provided the 4GL also does it consistently. Testing is needed.

• RecordBuffer.getFieldNamePairs needs JavaDoc.

Will add

Constantin Asofiei wrote:

Please edit each case and add the value of the ERROR:NUM-MESSAGES attribute (you can add it in the same MESSAGE line where the ERROR attribute is interogated); this will allow us to check how many messages were logged.

class Date, line 1435 - note 70
- you have a typo in the 14800 error message, please edit the note and fix it

Should I fix it directly in CVS without sending special update and regression?
ERROR-STATUS:NUM-MESSAGES - ok, I will update tests

class ControlFlowOps, line 2991 - note 71
- the message is printed before the "had error" because currently ErrorManager.displayError does not honor silent-error mode.

Do you mean - does not support? Will it be supported or such difference in behavior is ok?
As I see setSilentError(true) is used only in buffer preparing time.

class ControlFlowOps, line 3577 - note 72
- this is OK, as in this case a STOP condition needs to be raised. But, you can test this by enclosing the RUN in a DO ON STOP UNDO, LEAVE block (so the sTOP condition is caught) and after the block, check the ERROR-STATUS data:
[...]
In this case, no 0 is shown.

class NumberType, line 3189 - note 73
- same problem as note 71, silent-error mode is not honored + message is not logged

class BrowseWidget, line 751 - note 74
- when you encounter conversion errors in terms of "missing APIs", nothing is stoping you to modify the testcase so that the existing API is used. In this case, if you don't have a fix for the missing API, change the custb:FETCH-SELECTED-ROW(5) to a custb:FETCH-SELECTED-ROW(i) where i is a def var i as int init 5., reconvert, check P2J behaviour, and post the results back to note 74.

I understand this. I made with some another way, simply inserted wrapper to interger into converted code. I have updated note 74 now with runtime exception.

class handle, line 254 - note 75
- the differences in behavior are caused by the fact that AFAIK the CREATE BUFFER statement is not yet implemented. Please initialize the hBuffer1 handle using hBuffer1 = temp-table tt:handle. instead of the CREATE BUFFER and post back the results to note 75.

note 75 is updated

class HandleOps, line 67 - note 76
- the testcase has nothing to do with the code at line 67 in HandleOps. That code is for a DELETE WIDGET h. statement, and this statement does not accept the NO-ERROR clause. And if you check this:
[...]
you will notice that the following messages are shown:
[...]
So, no errors are logged or raised, but the messages are displayed.

OK, note 76 is marked as <should be ignored>

class <Not found in code> - note 77, 78 - I guess the runtime for these is not available yet, right? If so, there was not reason to add these here now, as we need to know how existing runtime behaves.

Runtime is not available or wrong. I agree these notes are not relevant to ErrorManager.displayError. Maybewe should move them to same another issue. I did not test them specially, but provided here as messages to be supported and not found in code.

class ControlFlowOps, line 3519 - note 79
- the problem is the same as for the note 71, silent-error mode is not honored + message is not logged by ErrorManager.displayError

The same question as for note 71. What do youn mean “not honored”?

class <Not found in code> - note 80
- this is a missed error in the #1920 task. I've added details there. No further work is needed for this.

ok

class <Not found in code> - note 81
- not sure what you are trying to duplicate here. Your q11 variable is not initialized, so the errors are OK. More, the code for this is in handle.java:1834, and is for ErrorManager.recordOrThrowError, not displayError

There is difference in second message: queryForward instead of REPOSITION-FORWARDS

class <Not found in code> - note 82
- same as note 81 - not sure what you are trying to duplicate here. Your hQuery variable is not initialized, so the errors are OK. More, the code for this is in handle.java:1834

There is difference in second message: queryClose instead of QUERY-CLOSE

Note that the scope of this task was to identify all EXISTING usage of ErrorManager.displayError and create tests (or identify existing testcases from the testcases/ project) for each case (please ask if is not obvious how to reach the code from 4GL). You are still missing some cases (handle.java:254, handle.java:1844, and others), but anyway, at this time we have enough data to conclude that we need a new API for such cases, as at least note 72 (for RUN missing-program.p case) and note 76 (DELETE WIDGET case ) need current ErrorManager.displayError. The new API will need to honor silent error mode and save the error data.

My investigation were not actually completed. Should I continue with places you pointed? Or everything is already clear and I should return back to BUFFER-COPY/BUFFER-COMPARE?

After adding the new API, please do not make changes in existing code unless you have a working testcase which you are sure it reaches that code.

class Date, line 1435 - note 70

Should I fix it directly in CVS without sending special update and regression?

The typo is in your comment, not in P2J ("Invalid value 7666 for SESSION 4GL code: Date, 1435 :TIMEZONE attribute. (14800)")

class ControlFlowOps, line 2991 - note 71

Do you mean - does not support? Will it be supported or such difference in behavior is ok?

What I mean is that ErrorManager.displayError will always show the message, even if in silent error mode or not (i.e. NO-ERROR clause is present or not); what I mean by not honoring the NO-ERROR clause is the fact that ErrorManager.displayError does not check this silent error mode. The new API will have to check the ErrorManager.isSilentError and if set, will not display the message, will just log it. ErrorManager.displayError should not be changed.

class BrowseWidget, line 751 - note 74

I understand this. I made with some another way, simply inserted wrapper to interger into converted code. I have updated note 74 now with runtime exception.

Again, the testcase does not help us in determining the NO-ERROR behavior, as a runtime exception is thrown. What is missing from the testcase I think is the fact that the query returns no results (as the table is empty).

note 75 is updated

Please update the testcase at note 75 too (the CREATE BUFFER is still there...)

class <Not found in code> - note 77, 78

Runtime is not available or wrong. I agree these notes are not relevant to ErrorManager.displayError. Maybewe should move them to same another issue. I did not test them specially, but provided here as messages to be supported and not found in code.

OK, then create a "Missing runtime error messages in P2J" task in Base Language and document all of them there.

class <Not found in code> - note 81
- not sure what you are trying to duplicate here. Your q11 variable is not initialized, so the errors are OK. More, the code for this is in handle.java:1834, and is for ErrorManager.recordOrThrowError, not displayError

There is difference in second message: queryForward instead of REPOSITION-FORWARDS

OK, these are known problems (part of #1920). You will see that for errors like 3140 (where the accessed attribute/method is logged) P2J displays the converted API name and not the legacy attribute/method name.

My investigation were not actually completed. Should I continue with places you pointed? Or everything is already clear and I should return back to BUFFER-COPY/BUFFER-COMPARE?

1. add the new ErrorManager.displayOrLogError API now, so you can use it for error messages in BUFFER-COPY/BUFFER-COMPARE
2. once you have an update for BUFFER-COPY/BUFFER-COMPARE, go on and fix all the ErrorManager.displayError cases

Constantin Asofiei wrote:

Again, the testcase does not help us in determining the NO-ERROR behavior, as a runtime exception is thrown. What is missing from the testcase I think is the fact that the query returns no results (as the table is empty).

I was not able to reproduce it in converted code. I have created more complicated test for note 74 with records which really works with FETCH-SELECTED-ROW, but does not work with converted code because of EndConditionException in ThinClient. So, I have created 2 notes (3 and 4) in #2106 related with FETCH-SELECTED-ROW and runtime exceptions.

Vadim Nebogatov wrote:

Constantin Asofiei wrote:

Again, the testcase does not help us in determining the NO-ERROR behavior, as a runtime exception is thrown. What is missing from the testcase I think is the fact that the query returns no results (as the table is empty).

I took a second look at note 74 and in 4GL it doesn't raise an error condition but errors are logged. For this case P2J does behave wrong as it abends instead of logging the exception, so this case belongs to #2106 (and you already added it there).

Eric Faulhaber wrote:

• Please re-read the JavaDoc for Text.setTemporaryCaseSensitive. I'm not sure it was meant to be used in the way you are using it. If necessary, please consult with the original author (Constantin, I think) to check if your use of it is appropriate.

Constantin,
what do you think, if it is possible to use setTemporaryCaseSensitive() before field comparison in BUFFER-COMPARE( ) conversion implementation (like I do in attached to issue vmn_upd20130328a.zip), or better use setCaseSensitive() for change case sensitive settings and return them back immediately after comparison.
Could you also write some words about the reason why caseSens was made as part of Text instance. What will happen if some another thread will change caseSens value? Why code does not take care about this?

The Text.setTemporaryCaseSensitive (actually the entire class) is extracted from the character.java class. I don't think setTemporaryCaseSensitive is the best approach; this API is used for shared character variables, for which the case-sensitive state (if changed) will remain changed until the scope which declared the shared variable ends. For your case, when you get the value for a DMO property (via getter.invoke), you get a copy, not the actual reference which is saved at the DMO. Thus, is enough to use setCaseSensitive() for that Text instance, without having to worry to restore the previous state (as you work with a copy which will get discarded once its job is done).

About the threading issue: note that Text.caseSens is a field which is set per each character/longchar var instance. Currently, all communication is done sync (with some few CTRL-C exceptions), so no two threads should be able to change the Text.caseSens field for the same instance.

Constantin Asofiei wrote:

1. add the new ErrorManager.displayOrLogError API now, so you can use it for error messages in BUFFER-COPY/BUFFER-COMPARE
2. once you have an update for BUFFER-COPY/BUFFER-COMPARE, go on and fix all the ErrorManager.displayError cases

I still don't understand how ErrorManager will recognize what mode (silent or not) should be used. From examples I wrote in the beginning I noticed that 4GL itself "throws" error for some error types and silently logs message for others.

I still don't understand how ErrorManager will recognize what mode (silent or not) should be used. From examples I wrote in the beginning I noticed that 4GL itself "throws" error for some error types and silently logs message for others.

Silent error mode is enabled only when the NO-ERROR clause is present. Any statement that has the NO-ERROR clause will be converted like this:

ErrorManager.silentErrorEnable(); /* enable silent error mode */
// execute statement which had NO-ERROR clause. no ERROR condition will be raised by the runtime, but error messages can be logged
ErrorManager.silentErrorDisable(); /* disable silent error mode */

Back to your cases: when the NO-ERROR clause is not present (i.e. silent error mode is not in effect), there are cases when ERROR condition is raised and cases when ERROR condition is not raised. To disambiguate between them, when the ERROR condition is raised, then you must use an ErrorManager API such as recordOrThrowError which will record the messages and will raise the ERROR condition+display them only if NO-ERROR is not in effect; if NO-ERROR is in effect, the recordOrThrowError will just record the messages.

For cases when ERROR condition is not raised but messages are still shown, the new displayOrLogError API (which is similar to how recordOrShowError works, but will not use a modal dialog) must record the messages and will display them only if the NO-ERROR is not in effect. "NO-ERROR is not in effect" means that "silent mode is not enabled" which results in ErrorManager.isSiletError() call to check if it returns false. if NO-ERROR is in effect (i.e. ErrorManager.isSilentError() returns true), then this will just log the messages and will not display them.

You described how to implement runtime behavior similar to 4GL in each concrete case. But my question was actually not about this. I meant the following: do you already have or plan to implement some common mechanism allowing select required ErrorManager method (displayOrLogError, recordOrThrowError or another) ? Or in each concrete case we should make 4GL test and only after that try to reproduce similar behavior with invocation of corresponding ErrorManager method?

Vadim Nebogatov wrote:

I have attached update vmn_upd20130401a.zip/vmn_upd20130401b.zip
It contains FETCH-SELECTED-ROW() conversion, using NumberType and long type parameters instead of int.
Ref. notes 74, 84
Merged with revision 10334

Comment for BrowseWidget.fetchSelectedRow(NumberType n) implementation.
I thought about using
long selListIndex = n.longValue() - 1;
instead of
int selListIndex = n.intValue() - 1;
but it will affect LogicalTerminal.getSelectedRowIndex(...)

OK, I didn't look at this carefully enough when you initially suggested to refactor this into method separate variants that accept NumberType and long. There is no need for a long version. An int version will suffice. The parameter n is the index into the selected rows in the browse, not the index of the row itself. Even if it were the latter, a value > Integer.MAX_VALUE is unlikely. Consider that we would have to have > Integer.MAX_VALUE selected rows for a long to be needed. We would have resource and usability problems long before we reached that scenario.

So, please replace fetchSelectedRow(long) with fetchSelectedRow(int) and wrap the parameter in an integer rather than decimal. The remaining changes seem good, except there were no conversion changes as you noted above. I'm not sure any conversion changes are needed; I would think the parameters would convert naturally.

You described how to implement runtime behavior similar to 4GL in each concrete case. But my question was actually not about this. I meant the following: do you already have or plan to implement some common mechanism allowing select required ErrorManager method (displayOrLogError, recordOrThrowError or another) ?

No.

Or in each concrete case we should make 4GL test and only after that try to reproduce similar behavior with invocation of corresponding ErrorManager method?

Correct. For each case, you need to determine how it behaves in 4GL and use the ErrorManager API depending on this behavior (i.e. recordOrThrowError if ERROR condition is raised or displayOrLogError if ERROR condition is not raised but error messages are recorded). On a side note, I think displayOrRecordError is a better name for the new API instead of displayOrLogError, so please use displayOrRecordError.

Constantin Asofiei wrote:

For cases when ERROR condition is not raised but messages are still shown, the new displayOrLogError API (which is similar to how recordOrShowError works, but will not use a modal dialog) …

Do we already have methods creating modeless widgets like ClientExports.messageBox(...)?

To display a message, use the LogicalTerminal.message API from the business logic (note that each message gets wrapped depending on the window's width, so you should put each message on its own line). This is all you need.

Constantin: I think that Vadim is trying to create an alternate version of ErrorManager.recordOrShowError() that doesn't use a dialog.

Vadim: is that correct?

If that is correct, then Constantin's suggestion to use client.message()/LogicalTerminal.message() instead of messageBox() is the right approach.

But perhaps we should add a parameter to recordOrShowError() that is a flag (e.g. boolean dialog). That way the worker routine can remain the same and just use the non-dialog message() when !dialog.

Vadim: please prepare a separate update with the ErrorManager related changes. I know you have ongoing work on the buffer-compare/copy stuff. I want to get the other work done separately, especially since you are further along with it. We also have other team-members who are dependent upon your ErrorManager changes, so they need to go in quickly.

Will P2J support SAVE RESULT IN option in BUFFER-COMPARE?

Vadim Nebogatov wrote:

Will P2J support SAVE RESULT IN option in BUFFER-COMPARE?

Yes, we need to support this option; it is used by the current project. IIRC, we currently have partial/incompatible support for this feature: I believe it generates a list of the new DMO property names, not the original field names. We will have to produce a list of the original field names instead. Please confirm this.

Code Review for vmn_upd20130409a.zip:

1. Please remove the recordOrDisplayError() variants. It is too confusing to have methods that are differentiated by Show versus Display and the only different is modality. The reader of the method name can't really tell the purpose from the method name. I prefer to have the call sites just add the boolean parameter as false where needed to allow the reader to know that the usage is non-modal.

2. I thought you had found cases in P2J that were wrong today. If that is true, then there will be some modification to other files to use the new "worker" version that has the boolean flag. Please include any such changes.

Otherwise, the code looks good.

Eric Faulhaber wrote:

Vadim Nebogatov wrote:

Will P2J support SAVE RESULT IN option in BUFFER-COMPARE?

Yes, we need to support this option; it is used by the current project. IIRC, we currently have partial/incompatible support for this feature: I believe it generates a list of the new DMO property names, not the original field names. We will have to produce a list of the original field names instead. Please confirm this.

Compare statement is calculated correctly. Namely, diffs contains names of different fields. But this value is returned as last parameter in

RecordBuffer.compare(tt1, new String[]
{
                     "f2" 
                  }, true, tt2, tt1.getF2());

and not committed to database

Greg Shah wrote:

Code Review for vmn_upd20130409a.zip:

1. Please remove the recordOrDisplayError() variants. It is too confusing to have methods that are differentiated by Show versus Display and the only different is modality. The reader of the method name can't really tell the purpose from the method name. I prefer to have the call sites just add the boolean parameter as false where needed to allow the reader to know that the usage is non-modal.

I made it as it was adviced above (“the new displayOrLogError API (which is similar to how recordOrShowError works, but will not use a modal dialog”) with changing displayOrLogError name to recordOrDisplayError name. OK, will correct.

2. I thought you had found cases in P2J that were wrong today. If that is true, then there will be some modification to other files to use the new "worker" version that has the boolean flag. Please include any such changes.

Otherwise, the code looks good.

I found some differences, part of them are known problems (part of #1920), some of them are not directly related with this issue (new issue 2106 was created). Rest of them seems could be supported with new API which will support silent error mode and new method.

Vadim Nebogatov wrote:

Eric Faulhaber wrote:

Vadim Nebogatov wrote:

Will P2J support SAVE RESULT IN option in BUFFER-COMPARE?

Yes, we need to support this option; it is used by the current project. IIRC, we currently have partial/incompatible support for this feature: I believe it generates a list of the new DMO property names, not the original field names. We will have to produce a list of the original field names instead. Please confirm this.

Compare statement is calculated correctly. Namely, diffs contains names of different fields.

I still think the field names are handled incorrectly. It happens to work in this case, because the converted field names are the same as the original, but try using a temp-table field name of f-2 in your test case, and you will see the problem.

But this value is returned as last parameter in

 
RecordBuffer.compare(tt1, new String[]
{
   "f2" 
}, true, tt2, tt1.getF2());

and not committed to database

Yes, it seems this is converting wrong. We should have another variant of RecordBuffer.compare that accepts a FieldReference for this parameter. It should not convert to tt1.getF2(), but rather to new FieldReference(tt1, "f2"). Within that variant of compare, the result should be stored using FieldReference.set(BaseDataType). To get the field reference conversion to change, you will have to add rules to convert/database_references.rules.

I assume the appropriate tt1 record has to be referenced from a previous lookup, like FIND, FOR, etc., correct?

Eric Faulhaber wrote:

Yes, it seems this is converting wrong. We should have another variant of RecordBuffer.compare ...

Actually, I just checked the entire server project and we never use the field variant, only the character variable variant. So, I do not want you to work on this, unless you estimate it is a very small effort (i.e., < 2 hours) to implement. If you believe it will take you longer than this, please just add the new method signature to the RecordBuffer class and have it throw UnsupportedOperationException for now. Put a TODO comment that this method needs to be implemented.

Eric Faulhaber wrote:

I assume the appropriate tt1 record has to be referenced from a previous lookup, like FIND, FOR, etc., correct?

Yes

vmn_upd20130411a.zip is fine. You can check it in and distribute it. It should have no risk to the conversion and such low risk on the runtime testing that we will let that be deferred.

Eric Faulhaber wrote:

I still think the field names are handled incorrectly. It happens to work in this case, because the converted field names are the same as the original, but try using a temp-table field name of f-2 in your test case, and you will see the problem.

Yes, you are correct, I have reproduced this problem. Is mapping information is available now in runtime? I see it only in *.rpt file which is printed in conversion time from NameConverterWorker.

BTW, I have fixed one more problem with wrong field order in result (not included to last update I sent). Now fields order are taken from getDeclaredMethods() - since Java 6 it seems returns order of fields as they are declared.

Vadim Nebogatov wrote:

BTW, I have fixed one more problem with wrong field order in result (not included to last update I sent). Now fields order are taken from getDeclaredMethods() - since Java 6 it seems returns order of fields as they are declared.

We can't rely on these always being in the correct order. From the JavaDoc for Class.getDeclaredMethods(): "The elements in the array returned are not sorted and are not in any particular order."

Once we properly support metadata, we will get the order from the _Field table. Please put a TODO in the code at the appropriate place.

Code review 20130416a:

Along with your changes, you have introduced some formatting and javadoc issues in BufferImpl. For example, see strange indent in bufferCompare(handle) and javadoc in bufferCopy(Buffer, character, character, logical). Also, I previously had made some javadoc corrections in some methods, where the documentation referred to "compare" vs. "copy" incorrectly (apparent cut and paste errors). These corrections have been lost in your version. Please diff with the latest bzr revision to see what I mean.

In RecordBuffer.copy(DataModelObject, String[], boolean, DataModelObject, boolean), you have removed the following code:

// Get destination record.  Create one if necessary.
Persistable dstRec = dstBuf.getCurrentRecord();
if (dstRec == null)
{
   dstBuf.create();
   dstRec = dstBuf.getCurrentRecord();
}

I'm pretty sure (at least in the language statement form of BUFFER-COPY), that this can be called with no record in the destination buffer and it will work. Without this code, we will have an error during the copy in this case. Have you determined that the behavior is otherwise?

I think we have a fundamental problem with the copy and compare implementations in RecordBuffer, that may have been made more confusing with my request in note 83 to share implementation code to the degree possible between the language statement and method versions of the 4GL copy/compare features. In response, you correctly wrote:

Parameters of methods and statements are very different, but core processing should be the same provided the 4GL also does it consistently. Testing is needed.

A primary parameter difference that is not handled yet is that in the language statement versions of BUFFER-COPY/COMPARE, the fields listed in the EXCEPT or USING clauses would be converted to DMO property names. In the 4GL method versions, this information is passed to us as a character/string of legacy names; since this text can be built up at runtime, no conversion of these names will have taken place at code conversion time. This requires that we convert those names to DMO property names at runtime, before we call the worker methods that do the actual copying/comparing. This again relies on metadata infrastructure that is not yet in place, but this differentiated requirement should be reflected in the code at this time, at least as a TODO. Accordingly, your test case should use a table which contains legacy field names which are not the same as their converted names (e.g., anything with an embedded hyphen).

Likewise, the List<String> returned by RecordBuffer.compare(RecordBuffer, RecordBuffer, Map<String, String>, character, boolean) contains converted field names instead of legacy field names, which is incorrect, even though we have gotten away with it in the past. Although long ago I put the following where this method is invoked:

// TODO:  confirm formatting;  use legacy field names.

In RecordBuffer.setCompareMode(BaseDataType, character), why are you turning on case sensitive comparison when the mode is "binary"?

In RecordBuffer.createSimpleFieldsMap, you have the following:

if (operationType == OperationType.COPY)
{
   StringBuilder msg = new StringBuilder("Datatypes in pairlist");
   msg.append(" argument for ATTACH-DATA-SOURCE, BUFFER-COPY or ");
   msg.append("BUFFER-COMPARE methods must match for fields ");
   msg.append(srcField).append(" and ").append(dstField);
   ErrorManager.recordOrThrowError(12860,
           msg.toString(),
           false);
}

The message text suggests this error should apply for BUFFER-COMPARE, but the enclosing conditional only invokes the error for the COPY operation type. Is this correct? Also, please align the ErrorManager.recordOrThrowError parameters, or better yet put them all on one line, if they fit within the standard line length. Please also use line feeds before inline comments, so the blocks of code they describe stand out to the reader as separate sections.

The reason of some formatting errors is unpleasant Ubuntu's feature: paste on middle mouse click.
Sometimes it happens when I scroll file and text is pasted to unpredictable places. Maybe you know how to switch off this feature? I googled several times but did not find easy way.

Sorry, I don't know how to disable this, and what I found online suggested there is not a simple way to just turn off the paste behavior without disabling the middle button entirely.

Eric Faulhaber wrote:

In RecordBuffer.copy(DataModelObject, String[], boolean, DataModelObject, boolean), you have removed the following code:
[...]

I'm pretty sure (at least in the language statement form of BUFFER-COPY), that this can be called with no record in the destination buffer and it will work. Without this code, we will have an error during the copy in this case. Have you determined that the behavior is otherwise?

Maybe I am wrong, during testing I understood that statement works but does nothing in this case.
But code that I removed creates buffer...

Eric Faulhaber wrote:

In RecordBuffer.setCompareMode(BaseDataType, character), why are you turning on case sensitive comparison when the mode is "binary"?

I made it based on 4GL tests. It compares Text like case-sensitive if "binary" is set.

Vadim Nebogatov wrote:

Maybe I am wrong, during testing I understood that statement works but does nothing in this case.

Please confirm this. The Progress documentation (though often wrong) makes a clear point that a record will be created if there is none in the buffer, AND I'm pretty sure I confirmed this when I originally implemented this feature.

But code that I removed creates buffer...

No, it creates a default record in the current buffer. The older, no-arg instance method RecordBuffer.create() is different than the newly added, static RecordBuffer.create(...) methods. It is unfortunate that the name is the same. We probably should rename the former createRecord() to avoid confusion.

Eric Faulhaber wrote:

Vadim Nebogatov wrote:

Maybe I am wrong, during testing I understood that statement works but does nothing in this case.

Please confirm this. The Progress documentation (though often wrong) makes a clear point that a record will be created if there is none in the buffer, AND I'm pretty sure I confirmed this when I originally implemented this feature.

You are correct. From the documentation:

At run time, BUFFER-COPY:
–
Creates a target record if none already exists and executes any applicable
CREATE triggers

"Source element of a BUFFER-COMPARE statement has no record. (5369)",

if remove or comment BUFFER-COPY statement.

/* tt1 */
def temp-table tt1
field f1 as character
field f2 as character.

/* tt2 */
def temp-table tt2 like tt1.

/* create record on 2nd table*/
create tt2.
assign tt2.f1 = "2" 
tt2.f2 = "2".

BUFFER-COPY tt2 TO tt1.

def var result as character.
BUFFER-COMPARE tt1 TO tt2.
message "AFTER: " result.

But BUFFER-COPY method does not create record. There are no mentions about this in
documentation and example below also gives

"Source element of a BUFFER-COMPARE statement has no record. (5369)"

independently on presence of BUFFER-COPY:

/* tt1 */
def temp-table tt1
field f1 as character
field f2 as character.

def buffer b1 for tt1.
def var hBuffer1 as handle.
hBuffer1 = buffer b1:handle.

/* tt2 */
def temp-table tt2 like tt1.

def buffer b2 for tt2.
def var hBuffer2 as handle.
hBuffer2 = buffer b2:handle.

/* create record on 2nd table*/
create tt2.
assign tt2.f1 = "2" 
tt2.f2 = "2".

def var log as logical.
log = hBuffer1:BUFFER-COPY(hBuffer2).
message "log: " log.

def var result as character.
BUFFER-COMPARE tt1 TO tt2.
message "AFTER: " result.

Eric Faulhaber wrote:

In RecordBuffer.createSimpleFieldsMap, you have the following:
if (operationType == OperationType.COPY) {
StringBuilder msg = new StringBuilder("Datatypes in pairlist");
msg.append(" argument for ATTACH-DATA-SOURCE, BUFFER-COPY or ");
msg.append("BUFFER-COMPARE methods must match for fields ");
msg.append(srcField).append(" and ").append(dstField);
ErrorManager.recordOrThrowError(12860,
msg.toString(),
false);
}
The message text suggests this error should apply for BUFFER-COMPARE, but the enclosing conditional only invokes the error for the COPY operation type. Is this correct?

It should be corrected. I tested one more time and understood that this message concerns only pairs mentioned in "pairs-list" parameter (not for all mapped pairs). So, this error could happen only in both compare and copy methods (not statements).

Current status of BUFFER-COMPARE/BUFFER-COPY runtime support.

My estimation for changes related with fixing notes 129-131 - about 1 day. Everything else seems implemented.

Tests I would like to (re)execute:

1) correctness of field map creation depending on operation (BUFFER-COMPARE/BUFFER-COPY)
2) if field types in pair are different – show error for pairs from “pairs-list" parameter and skip for others
3) BUFFER-COPY for records with fields of different but compatible types
4) regression testing BUFFER-COMPARE/BUFFER-COPY statements.
5) testing errors when buffer field have different sizes

I think such testing could take about one week.

BTW, how to create test case with indexed properties (using sizers)?

Code review 20130423:

In RecordBuffer, you have the following comment in a few places, before a call to a worker copy or compare method:

//TODO ..., null, null, ...

What does this TODO mean?

Please add a TODO to the RecordBuffer.compare worker method at the location where field names are added to the diffs list, that we must populate the list with legacy field names instead of post-conversion DMO property names, once metadata support is available. Similarly, we need a TODO in RecordBuffer.getFieldsMap to the effect that we need to map the field names in the except and pairs lists to their post-conversion, DMO property name equivalents before we process them. These parameters represent lists of legacy field names, but we currently are treating them as post-conversion DMO property names.

Your test case still needs to define some temp-tables with field names that will convert to something different, so that you can prove the TODOs in the previous paragraph are implemented correctly, once metadata support is available. For example:

/* tt5 */
def temp-table tt5
field f-1 as character
field f-2 as character
field f-3 as int
field f-4 as int.

Using this temp-table in any copy/compare test should break the current implementation, since the field names will convert to f1, f2, etc.

You should also have tests between temp-tables with some matching AND some non-matching field definitions. According to the 4GL doc for both language statements, fields in the source but not in the target are excluded from the copy/compare operations. Please confirm this and make sure we match the behavior.

Eric Faulhaber wrote:

Code review 20130423:

In RecordBuffer, you have the following comment in a few places, before a call to a worker copy or compare method:
[...]

What does this TODO mean?

When I wrote this TODO, I was not sure if we need to check types of corresponding fields for statements. Now I see, that it is not required, it is checked at the compile time.

BUFFER-COMPARE statement runtime does not support CASE-SENSITIVE option. Should it be supported now? What about EXPLICIT, COMPARES, and WHEN options?

Vadim Nebogatov wrote:

BUFFER-COMPARE statement runtime does not support CASE-SENSITIVE option. Should it be supported now?

Yes, it is used in the current project. I looked at some instances and noticed that we are not converting the statement correctly (the CASE-SENSITIVE option is ignored), so conversion will have to be corrected, too. How long do you think it will take to implement this?

What about EXPLICIT, COMPARES, and WHEN options?

There are no uses of these options in the current project, but it would be nice to get these finished now, so we don't have to come back around later and do it. The answer depends on the effort. Can you estimate how long you would expect it to take to add this support? Note that currently, conversion does not support these options, either.

Eric Faulhaber wrote:

Vadim Nebogatov wrote:

BUFFER-COMPARE statement runtime does not support CASE-SENSITIVE option. Should it be supported now?

Yes, it is used in the current project. I looked at some instances and noticed that we are not converting the statement correctly (the CASE-SENSITIVE option is ignored), so conversion will have to be corrected, too. How long do you think it will take to implement this?

What about EXPLICIT, COMPARES, and WHEN options?

There are no uses of these options in the current project, but it would be nice to get these finished now, so we don't have to come back around later and do it. The answer depends on the effort. Can you estimate how long you would expect it to take to add this support? Note that currently, conversion does not support these options, either.

My estimations:

CASE-SENSITIVE - 1-2 days, maybe less.
EXPLICIT, COMPARES, WHEN, THEN and using compare-operators - 5-6 days

OK, thanks. Please go forward with the CASE-SENSITIVE support, but not with the EXPLICIT, COMPARES, WHEN, THEN.

Besides CASE-SENSITIVE support for the statement version of BUFFER-COMPARE, what is left to do in order for copy/compare support to be complete (please provide estimates with the summary)? I understand you cannot implement the legacy field name support until we have metadata working, but I want to know everything else. Please also provide estimates for the 3 methods listed in the previous note.

Rest of BUFFER-COMPARE/COPY support

• for the statement version

1. CASE-SENSITIVE - 1-2 days, maybe less.
2. EXPLICIT, COMPARES, WHEN, THEN and using compare-operators - 5-6 days
3. Legacy field name support (when metadata working) – 2 days

• for the method version. All options seems supported.

1. Testing and fixes possible needed for indexed properties (using sizers, including different sizers) – 2 days.
2. Legacy field name support (when metadata working) – 2 days

I only want these features for now:

• CASE-SENSITIVE - 1-2 days, maybe less.
• Testing and fixes possible needed for indexed properties (using sizers, including different sizers) – 2 days.

Then move onto the items in note 142.

We'll handle retrofitting the legacy field name support in a separate task.

Estimations:

bufferRelease - seems already implemented. Is it correct implementation?

bufferCreate - reusing worker method RecordBuffer.create() - 3-5 days, maybe faster

findByRowID – from 3-4 days up to 7-10 days depending on how fully FIND statement is supported (and reusing):

"The FIND-BY-ROWID method corresponds to a FIND statement of the form FIND buffer WHERE ROWID ( buffer ) = rowid ..., etc. That is, triggers are honored, and the default lock mode is SHARE-LOCK. One difference, however, is that the FIND-BY-ROWID method does not raise an error if it
cannot find the record."

Vadim Nebogatov wrote:

Estimations:

bufferRelease - seems already implemented. Is it correct implementation?

I think the functionality is all there, but you will need to test and probably adjust the error handling for the method version, like with BUFFER-COPY/COMPARE.

bufferCreate - reusing worker method RecordBuffer.create() - 3-5 days, maybe faster

I hope this will be faster. Again, the functionality should be all there, but the error handling is probably different.

findByRowID – from 3-4 days up to 7-10 days depending on how fully FIND statement is supported (and reusing):

"The FIND-BY-ROWID method corresponds to a FIND statement of the form FIND buffer WHERE ROWID ( buffer ) = rowid ..., etc. That is, triggers are honored, and the default lock mode is SHARE-LOCK. One difference, however, is that the FIND-BY-ROWID method does not raise an error if it cannot find the record."

Runtime support for the FIND statement is quite good. It is used very heavily in the current production installation. Hopefully, the implementation in BufferImpl is as easy as the following:

[determine correct LockType constant]
ErrorManager.enableSilentError();
new FindQuery(this, "myBuf.id = ?", null, null, new Object[] { id }, LockType.{NONE|SHARE|EXCLUSIVE}).unique();
ErrorManager.disableSilentError();
[figure out appropriate, logical return value and return it]

Again, you will need to work out the error handling differences between this and regular FIND statement use. The above code suppresses any errors, but you will need to figure out exactly what the return value from findByRowID represents.

I found out that we need convert mode to low case before check

if ("case-sensitive".equals(mode.getValue()) || "binary".equals(mode.getValue()))

because 4GL accepts "cAse-SeNsitive" and "BiNaRy"

I should have caught this earlier in my code review. Please use:

if (mode.compareTo("case-sensitive") == 0 || mode.compareTo("binary") == 0)
...

This will do a fully Progress-compatible comparison.

Eric Faulhaber wrote:

I should have caught this earlier in my code review. Please use:
[...]

This will do a fully Progress-compatible comparison.

You mentioned about setTemporaryCaseSensitive, using case-sensitive when "binary", about support CASE-SENSITIVE option, but I don't remember about "case-sensitive" mix case ...

Code review 20130426:

The code changes look good.

However, you still don't have any test cases that compare/copy between tables with non-matching legacy field names, which convert to matching DMO property names (see note 136 above). Such a test currently would work with our implementation but should not work with the 4GL. Once we have metadata working, our implementation will recognize the mismatches, and we should behave the same way as the 4GL.

Eric Faulhaber wrote:

You should also have tests between temp-tables with some matching AND some non-matching field definitions. According to the 4GL doc for both language statements, fields in the source but not in the target are excluded from the copy/compare operations. Please confirm this and make sure we match the behavior.

Tests in last update include testing BUFFER-COPY/COMPARE methods in different situations:

• found and not found fields
• fields surrounded with spaces
• fields of different types
• fields of different extent presence
• fields of different extent
• missed second field in pairs
• repeated fields in using/except
• empty/non-empty clauses
• using legacy field names

The vmn_upd20130429a.zip update has passed conversion regression testing. Runtime regression testing is not yet finished.

The vmn_upd20130429a.zip update (the one I changed and uploaded with note 155) has passed runtime regression testing. Please check in and distribute this version of the code. We will push your bug fix from 20130430a into a future revision.

Vadim Nebogatov wrote:

4GL statement copy and compare give compile warnings (not errors)

[...]
and execution is continued.

• BUFFER-COPY statement
• BUFFER-COMPARE statement with SAVE RESULT IN logical variable
• BUFFER-COMPARE statement with SAVE RESULT IN character variable
• BUFFER-COPY method
• BUFFER-COMPARE method

Yes, much of the trigger functionality is still unimplemented. However, I don't understand why you need triggers to test the error handling of the RELEASE statement and the BUFFER-RELEASE method.

The way to raise an error during the release of a record is to make it fail validation. You do this by creating or updating a record in such a way that it will violate a constraint. The simplest way is to add two records that would be considered the same under a unique index.

If you can make your tests work with the p2j_test database, that would simplify sharing them. Have a look at the p2j_test.book table. I'm pretty sure you will find a suitable constraint there.

Test for RELEASE statement with p2j_test database.

create book.
book.book-id = 1111.
book.cost = 9.
message "before release".
release book.
message "after release".

4GL:

before release
** Book already exists with Book ID 0. (132)

P2J:

before release
** org.hibernate.exception.ConstraintViolationException: Unique index or primary key violation: "IDX__BOOK_ISBN ON PUBLIC.BOOK(__ISBN)"; SQL statement:
insert into book (book_id, book_title, publisher, isbn, on_hand_qty, cost, pub_date, author_id, sold_qty, price, id) values (?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?) [23505-169] (0)

Messages correspond not to the same field, but it is not so important of course because it depends what rows are already inserted.

My conclusion is that P2J should catch hibernate ConstraintViolationExceptions and throw/show 4GL-like messages.

Stack trace in log:

[05/09/2013 01:57:54 MSK] (org.hibernate.engine.jdbc.spi.SqlExceptionHelper:WARN) SQL Error: 23505, SQLState: 23505
[05/09/2013 01:57:54 MSK] (org.hibernate.engine.jdbc.spi.SqlExceptionHelper:ERROR) Unique index or primary key violation: "IDX__BOOK_ISBN ON PUBLIC.BOOK(__ISBN)"; SQL statement:
insert into book (book_id, book_title, publisher, isbn, on_hand_qty, cost, pub_date, author_id, sold_qty, price, id) values (?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?) [23505-169]
[05/09/2013 01:57:54 MSK] (com.goldencode.p2j.persist.Persistence$Context:WARNING) [00000001:00000006:bogus-->local/p2j_test/primary] rolling back orphaned transaction
[05/09/2013 01:57:54 MSK] (org.hibernate.engine.jdbc.spi.SqlExceptionHelper:WARN) SQL Error: 23505, SQLState: 23505
[05/09/2013 01:57:54 MSK] (org.hibernate.engine.jdbc.spi.SqlExceptionHelper:ERROR) Unique index or primary key violation: "IDX__BOOK_ISBN ON PUBLIC.BOOK(__ISBN)"; SQL statement:
insert into book (book_id, book_title, publisher, isbn, on_hand_qty, cost, pub_date, author_id, sold_qty, price, id) values (?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?) [23505-169]
[05/09/2013 01:57:54 MSK] (ErrorManager:SEVERE) {00000001:00000006:bogus} stack trace follows
com.goldencode.p2j.persist.PersistenceException: org.hibernate.exception.ConstraintViolationException: Unique index or primary key violation: "IDX__BOOK_ISBN ON PUBLIC.BOOK(__ISBN)"; SQL statement:
insert into book (book_id, book_title, publisher, isbn, on_hand_qty, cost, pub_date, author_id, sold_qty, price, id) values (?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?) [23505-169]
        at com.goldencode.p2j.persist.Persistence$Context.maybeFlushSession(Persistence.java:4706)
        at com.goldencode.p2j.persist.Persistence$Context.rollback(Persistence.java:4117)
        at com.goldencode.p2j.persist.Persistence$Context.closeSession(Persistence.java:4612)
        at com.goldencode.p2j.persist.Persistence.detach(Persistence.java:2861)
        at com.goldencode.p2j.persist.BufferManager.decrementDMOUseCount(BufferManager.java:1433)
        at com.goldencode.p2j.persist.RecordBuffer.decrementDMOUseCount(RecordBuffer.java:5132)
        at com.goldencode.p2j.persist.RecordBuffer.setCurrentRecord(RecordBuffer.java:7519)
        at com.goldencode.p2j.persist.RecordBuffer.release(RecordBuffer.java:5778)
        at com.goldencode.p2j.persist.BufferImpl.release(BufferImpl.java:786)>

Something is wrong here. This functionality has long since been in P2J. You should not be seeing Hibernate-level exceptions about unique constraint violations in the log, because we validate new and changed records against all such constraints from the RecordBuffer class (actually delegated to DMOValidator) before flushing them to the database. Either something is wrong with your installation or something was broken recently, perhaps with the Hibernate version upgrade (though that passed runtime regression testing, so that would be surprising).

I have corrected update above: actual message is some shorter. First time I copied message from log, sorry.

HibernateException is thrown from session.flush(); in rollback. I think it should do nothing in my simple example, and possible problem is with new Hibernate.

When I run your test case once in Progress, I get:

before release
after release
Procedure complete. Press space bar to continue.

When I run it a second time in Progress, I get:

** Book already exists with Book ID 1111. (132)

Procedure complete. Press space bar to continue.

This is what I would expect; the record is created on the first run, but the second run violates the unique index on the book-id field, hence the error.

When I run your test case once in P2J, I get:

before release
after release
Procedure complete. Press space bar to continue.

...and it has created a book record in the database as follows:

p2j_test=# select * from book where book_id = 1111;
 id | book_id | book_title | publisher | isbn | on_hand_qty | cost | pub_date | author_id | sold_qty | price 
----+---------+------------+-----------+------+-------------+------+----------+-----------+----------+-------
  5 |    1111 |            |           |      |           0 | 9.00 |          |         0 |        0 |  0.00
(1 row)

When I run the program a second time, I get:

** book already exists with bookId 1111. (132)
** Unable to update book Field. (142)
Procedure complete. Press space bar to continue.

Note that the table and field names in the P2J error message represent the converted names rather than the legacy names. This is a known issue. I'm not sure why the second error message does not appear in Progress. It could be that something has changed in this regard between Progress version 9.1C and 10.2b. The behavior is based on what we encountered with the older version, but perhaps we need to investigate that.

But the point is that the error condition is recognized and reported in P2J, without the Hibernate-level message you are seeing. The unique constraint violation is recognized early, and the record therefore is not flushed to the database.

The stack trace in the server.log is:

[05/09/2013 17:17:46 EDT] (ErrorManager:SEVERE) {00000004:00000010:bogus} stack trace follows
com.goldencode.p2j.NumberedException: Unable to update book Field
        at com.goldencode.p2j.util.ErrorManager.recordOrThrowError(ErrorManager.java:734)
        at com.goldencode.p2j.persist.RecordBuffer$Handler.invoke(RecordBuffer.java:8696)
        at com.goldencode.p2j.persist.$__Proxy0.setBookId(Unknown Source)
        at com.goldencode.testcases.CreateReleaseBookRecord$1.body(CreateReleaseBookRecord.java:31)
        at com.goldencode.p2j.util.BlockManager.processBody(BlockManager.java:6917)
        at com.goldencode.p2j.util.BlockManager.topLevelBlock(BlockManager.java:6824)
        at com.goldencode.p2j.util.BlockManager.externalProcedure(BlockManager.java:210)
        at com.goldencode.testcases.CreateReleaseBookRecord.execute(CreateReleaseBookRecord.java:25)
        at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
        at sun.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:57)
        at sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
        at java.lang.reflect.Method.invoke(Method.java:601)
        at com.goldencode.p2j.util.ControlFlowOps$InternalEntryCaller.invoke(ControlFlowOps.java:3776)
        at com.goldencode.p2j.util.ControlFlowOps.invokeImpl(ControlFlowOps.java:3099)
        at com.goldencode.p2j.util.ControlFlowOps.invokeImpl(ControlFlowOps.java:2878)
        at com.goldencode.p2j.util.ControlFlowOps.invoke(ControlFlowOps.java:301)
        at com.goldencode.p2j.util.ControlFlowOps.invoke(ControlFlowOps.java:287)
        at com.goldencode.testcases.Ask$1.body(Ask.java:47)
        at com.goldencode.p2j.util.BlockManager.processBody(BlockManager.java:6917)
        at com.goldencode.p2j.util.BlockManager.topLevelBlock(BlockManager.java:6824)
        at com.goldencode.p2j.util.BlockManager.externalProcedure(BlockManager.java:210)
        at com.goldencode.p2j.util.BlockManager.externalProcedure(BlockManager.java:192)
        at com.goldencode.testcases.Ask.execute(Ask.java:27)
        at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
        at sun.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:57)
        at sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
        at java.lang.reflect.Method.invoke(Method.java:601)
        at com.goldencode.p2j.util.Utils.invoke(Utils.java:1153)
        at com.goldencode.p2j.main.StandardServer$MainInvoker.execute(StandardServer.java:1513)
        at com.goldencode.p2j.main.StandardServer.invoke(StandardServer.java:1019)
        at com.goldencode.p2j.main.StandardServer.standardEntry(StandardServer.java:320)
        at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
        at sun.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:57)
        at sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
        at java.lang.reflect.Method.invoke(Method.java:601)
        at com.goldencode.p2j.util.MethodInvoker.invoke(MethodInvoker.java:76)
        at com.goldencode.p2j.net.Dispatcher.processInbound(Dispatcher.java:675)
        at com.goldencode.p2j.net.Conversation.block(Conversation.java:316)
        at com.goldencode.p2j.net.Conversation.run(Conversation.java:158)
        at java.lang.Thread.run(Thread.java:722)
Caused by: com.goldencode.p2j.persist.ValidationException: book already exists with bookId 1111
        at com.goldencode.p2j.persist.DMOValidator.uniqueConstraintViolation(DMOValidator.java:1151)
        at com.goldencode.p2j.persist.DMOValidator.checkUnique(DMOValidator.java:962)
        at com.goldencode.p2j.persist.DMOValidator.checkUniqueAndShare(DMOValidator.java:709)
        at com.goldencode.p2j.persist.DMOValidator.validate(DMOValidator.java:503)
        at com.goldencode.p2j.persist.DMOValidator.validate(DMOValidator.java:450)
        at com.goldencode.p2j.persist.RecordBuffer$ValidationHelper.prevalidate(RecordBuffer.java:8130)
        at com.goldencode.p2j.persist.RecordBuffer$Handler.detectChange(RecordBuffer.java:8964)
        at com.goldencode.p2j.persist.RecordBuffer$Handler.invoke(RecordBuffer.java:8543)
        at com.goldencode.p2j.persist.$__Proxy0.setBookId(Unknown Source)
        at com.goldencode.testcases.CreateReleaseBookRecord$1.body(CreateReleaseBookRecord.java:31)
        at com.goldencode.p2j.util.BlockManager.processBody(BlockManager.java:6917)
        at com.goldencode.p2j.util.BlockManager.topLevelBlock(BlockManager.java:6824)
        at com.goldencode.p2j.util.BlockManager.externalProcedure(BlockManager.java:210)
        at com.goldencode.testcases.CreateReleaseBookRecord.execute(CreateReleaseBookRecord.java:25)
        at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
        at sun.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:57)
        at sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
        at java.lang.reflect.Method.invoke(Method.java:601)
        at com.goldencode.p2j.util.ControlFlowOps$InternalEntryCaller.invoke(ControlFlowOps.java:3776)
        at com.goldencode.p2j.util.ControlFlowOps.invokeImpl(ControlFlowOps.java:3099)
        at com.goldencode.p2j.util.ControlFlowOps.invokeImpl(ControlFlowOps.java:2878)
        at com.goldencode.p2j.util.ControlFlowOps.invoke(ControlFlowOps.java:301)
        at com.goldencode.p2j.util.ControlFlowOps.invoke(ControlFlowOps.java:287)
        at com.goldencode.testcases.Ask$1.body(Ask.java:47)
        at com.goldencode.p2j.util.BlockManager.processBody(BlockManager.java:6917)
        at com.goldencode.p2j.util.BlockManager.topLevelBlock(BlockManager.java:6824)
        at com.goldencode.p2j.util.BlockManager.externalProcedure(BlockManager.java:210)
        at com.goldencode.p2j.util.BlockManager.externalProcedure(BlockManager.java:192)
        at com.goldencode.testcases.Ask.execute(Ask.java:27)
        at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
        at sun.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:57)
        at sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
        at java.lang.reflect.Method.invoke(Method.java:601)
        at com.goldencode.p2j.util.Utils.invoke(Utils.java:1153)
        at com.goldencode.p2j.main.StandardServer$MainInvoker.execute(StandardServer.java:1513)
        at com.goldencode.p2j.main.StandardServer.invoke(StandardServer.java:1019)
        at com.goldencode.p2j.main.StandardServer.standardEntry(StandardServer.java:320)
        at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
        at sun.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:57)
        at sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
        at java.lang.reflect.Method.invoke(Method.java:601)
        at com.goldencode.p2j.util.MethodInvoker.invoke(MethodInvoker.java:76)
        at com.goldencode.p2j.net.Dispatcher.processInbound(Dispatcher.java:675)
        at com.goldencode.p2j.net.Conversation.block(Conversation.java:316)
        at com.goldencode.p2j.net.Conversation.run(Conversation.java:158)
        at java.lang.Thread.run(Thread.java:722)

I am running the latest code in bzr (rev. 10350). I ran this with a PostgreSQL backend. I have not yet tried with an H2 backend.

Problem seems with indexes recognition, possible with H2 only. Method checkUniqueAndShare() returns before actual validation because both dirtyUnique and dirtyNonUnique below are empty (dirty properties are not empty).

      Collection<String> dirtyUnique = vData.getDirtyUnique();
      Collection<String> dirtyNonUnique = vData.getDirtyNonUnique();

      if (dirtyUnique.isEmpty() &&
          (dirtyNonUnique.isEmpty() || !vData.isShareDirty()))
      {
         // Nothing to do.
         return;
      }

Problem is in different P2JDialect.needsComputedColumns() implementations, affects registerDatabase().

for PostgreSQL

   public boolean needsComputedColumns()
   {
      return false;
   }

for H2:

   public boolean needsComputedColumns()
   {
      return true;
   }

Actual problem is that in case of H2 code

 if (p2jDial.needsComputedColumns())
 {
        ...
        mapComputedCharacterColumns(schema, db, config);
 }

 hibernateConfigs.put(database, config);

that tries to use empty hibernateConfigs map which is empty and filled below this code.

I was trying to recreate your results, but I cannot even generate a usable H2 table schema for the p2j_test database at the moment. I think something fundamental has changed in Hibernate 4 with respect to schema generation for the H2 database.

The problems you are seeing might also suggest your older, p2j_test H2 database schema may be incompatible with the new H2Dialect in Hibernate 4. Are you able to re-generate/install your p2j_test database with the newer code? For me, it generates a DDL script, but when I try to create the H2 database with the RunScript tool, it fails with:

org.h2.jdbc.JdbcSQLException: Table "PERSON__5" not found; SQL statement:

    alter table person__5 
        drop constraint FK32DA98E05395E213 [42102-169]
    at org.h2.message.DbException.getJdbcSQLException(DbException.java:329)
    at org.h2.message.DbException.get(DbException.java:169)
    at org.h2.message.DbException.get(DbException.java:146)
    at org.h2.command.Parser.readTableOrView(Parser.java:4775)
    at org.h2.command.Parser.readTableOrView(Parser.java:4753)
    at org.h2.command.Parser.parseAlterTable(Parser.java:4826)
    at org.h2.command.Parser.parseAlter(Parser.java:4315)
    at org.h2.command.Parser.parsePrepared(Parser.java:306)
    at org.h2.command.Parser.parse(Parser.java:279)
    at org.h2.command.Parser.parse(Parser.java:251)
    at org.h2.command.Parser.prepareCommand(Parser.java:217)
    at org.h2.engine.Session.prepareLocal(Session.java:415)
    at org.h2.engine.Session.prepareCommand(Session.java:364)
    at org.h2.jdbc.JdbcConnection.prepareCommand(JdbcConnection.java:1114)
    at org.h2.jdbc.JdbcStatement.executeInternal(JdbcStatement.java:164)
    at org.h2.jdbc.JdbcStatement.execute(JdbcStatement.java:152)
    at org.h2.tools.RunScript.process(RunScript.java:253)
    at org.h2.tools.RunScript.process(RunScript.java:186)
    at org.h2.tools.RunScript.process(RunScript.java:317)
    at org.h2.tools.RunScript.runTool(RunScript.java:140)
    at org.h2.tools.RunScript.main(RunScript.java:68)
org.h2.jdbc.JdbcSQLException: Table "VEHICLE__10" not found; SQL statement:

    alter table vehicle__10 
        drop constraint FKC9CB32BFB4620D8 [42102-169]
    at org.h2.message.DbException.getJdbcSQLException(DbException.java:329)
    at org.h2.message.DbException.get(DbException.java:169)
    at org.h2.message.DbException.get(DbException.java:146)
    at org.h2.command.Parser.readTableOrView(Parser.java:4775)
    at org.h2.command.Parser.readTableOrView(Parser.java:4753)
    at org.h2.command.Parser.parseAlterTable(Parser.java:4826)
    at org.h2.command.Parser.parseAlter(Parser.java:4315)
    at org.h2.command.Parser.parsePrepared(Parser.java:306)
    at org.h2.command.Parser.parse(Parser.java:279)
    at org.h2.command.Parser.parse(Parser.java:251)
    at org.h2.command.Parser.prepareCommand(Parser.java:217)
    at org.h2.engine.Session.prepareLocal(Session.java:415)
    at org.h2.engine.Session.prepareCommand(Session.java:364)
    at org.h2.jdbc.JdbcConnection.prepareCommand(JdbcConnection.java:1114)
    at org.h2.jdbc.JdbcStatement.executeInternal(JdbcStatement.java:164)
    at org.h2.jdbc.JdbcStatement.execute(JdbcStatement.java:152)
    at org.h2.tools.RunScript.process(RunScript.java:253)
    at org.h2.tools.RunScript.process(RunScript.java:186)
    at org.h2.tools.RunScript.process(RunScript.java:317)
    at org.h2.tools.RunScript.runTool(RunScript.java:140)
    at org.h2.tools.RunScript.main(RunScript.java:68)

There are create table statements earlier in the script for (lowercase) person__5 and vehicle__10. This seems similar to the p2j_id_generator_sequence failure you reported via email recently.

Please ignore the noise about the broken H2 schema in my last post. I misread the errors; they are nothing new and are not harmful.

So, I was able to create the p2j_test database using H2 and import the data. However, when I run the test case (and in fact earlier, upon server startup), I see the same error generating a new primary key which you reported in your email:

Error preparing SQL statement:  SELECT NEXT VALUE FOR "p2j_id_generator_sequence";

The code which generates this SQL statement is in P2JH2Dialect.getSequenceNextValString, which is an override from H2Dialect which Ovidiu added recently. Here he explicitly adds double quotes around the sequence name, which is not done in the parent class. However, I have tried commenting out the overriding method and letting the parent generate the SQL, which is:

call next value for p2j_id_generator_sequence

However, it still fails to find the sequence. I don't want to force the sequence to uppercase to work around one database's problem (and possibly break another), nor do I really want to use double-quoted names. ANSI behavior is for database relations to be case insensitive, AFAIK, so I want to figure out the proper approach.

I realize this is probably not the appropriate issue in which to be discussing this, but you will be blocked in this task until we work out these H2 failures.

I've finally worked around the sequence issues with some temporary code changes and I'm now seeing the constraint violation exceptions you reported in note 163. I don't quite follow your explanation at the end of note 169, though. Can you provide more detail, please?

Additionally to note 169 concerning H2 database behavior.

P2J requests hibernateConfigs map

on DataBaseManager, line 2055

before it is filled

on DataBaseManager, line 2058.

As proof, please check the following stack trace

com.goldencode.p2j.persist.DatabaseManager.getHibernateConfiguration(DatabaseManager.java:476)
      at com.goldencode.p2j.persist.DatabaseManager.getClassMapping(DatabaseManager.java:1759)
      at com.goldencode.p2j.persist.DatabaseManager.getBackingTable(DatabaseManager.java:1717)
      at com.goldencode.p2j.persist.DatabaseManager.getBackingTableName(DatabaseManager.java:642)
      at com.goldencode.p2j.persist.DMOIndex$DMOMetadata.collectIndexData(DMOIndex.java:1693)
      at com.goldencode.p2j.persist.DMOIndex$DMOMetadata.databaseIndexes(DMOIndex.java:1454)
      at com.goldencode.p2j.persist.DMOIndex$DMOMetadata.collectIndexedCharColData(DMOIndex.java:1727)
      at com.goldencode.p2j.persist.DMOIndex$DMOMetadata.getIndexedCharCols(DMOIndex.java:1640)
      at com.goldencode.p2j.persist.DMOIndex.getIndexedCharacterColumns(DMOIndex.java:554)
      at com.goldencode.p2j.persist.DatabaseManager.mapComputedCharacterColumns(DatabaseManager.java:2203)
      at com.goldencode.p2j.persist.DatabaseManager.registerDatabase(DatabaseManager.java:2055)

After I moved two lines on top of if block,

                  hibernateConfigs.put(database, config);
                  configBySchema.put(schema, config);
                  if (p2jDial.needsComputedColumns())
                  {
                     // Map all indexed, computed columns which hold text
                     // data.  For dirty databases, we do this using the
                     // primary database, since the dirty database
                     // indexes will not have been created during this
                     // stage of initialization.
                     Database db = (database.isDirty()
                                    ? dirty2Primary.get(database)
                                    : database);
                     mapComputedCharacterColumns(schema, db, config);
                  }

H2 became work properly with messages

before release
** book already exists with isbn. (132)

Test uniqueness violation with BUFFER-RELEASE method (buffer-release-method-2.p)

def buffer b1 for book.
def var hBuffer1 as handle.
hBuffer1 = buffer b1:handle.

create book.
message "before 1111".
book.book-id = 1111.
message "after 1111".
book.cost = 9.
book.isbn ="s".

def var log as logical.

message "before release".
log = hBuffer1:buffer-release().
message "after release" log.

4GL:

before 1111
** Book already exists with Book ID 1111. (132)

P2J:

before 1111
** book already exists with bookId 1111. (132)
** Unable to update book Field. (142)

Strange example for buffer-create() method.

def buffer b1 for book.
def var hBuffer1 as handle.
hBuffer1 = buffer b1:handle.

def var log as logical.
do transaction:
message "before create".
log = hBuffer1:buffer-create().
message "after create" log.
end.
message "after create" log.

4GL execution result is:

before create
after create yes
** b1 already exists with Book ID 0. (132)
after create no

The question is: why and how variable value of log is changed?

Try

def var log as logical no-undo.

Eric Faulhaber wrote:

Try
def var log as logical no-undo.

Thanks, it works!
One more question: do we plan to support tenant-expression parameter for BUFFER-CREATE method?

Difference related with transaction absense is found in error handling of BUFFER-CREATE() method:

def buffer b1 for book.
def var hBuffer1 as handle.
hBuffer1 = buffer b1:handle.

def var log as logical no-undo.

message "before create".
log = hBuffer1:buffer-create().
message "after create" log.

4GL execution result is:

before create
Unable to CREATE/DELETE unless a TRANSACTION is running. (7345)
after create no

P2J:

before create
after create no
** b1 already exists with bookId 0. (132)

Vadim Nebogatov wrote:

One more question: do we plan to support tenant-expression parameter for BUFFER-CREATE method?

Not at this time. There is no such option under v10.2B, which is the target environment for the current project. Multi-tenancy was added with version 11, I believe.

Vadim Nebogatov wrote:

Difference related with transaction absense is found in error handling of BUFFER-CREATE() method:

You can use TransactionManager.isTransaction() to determine whether a transaction is active.

Yes, I just do with this way

Code review 20130510:

Update looks fine. However, since the change really isn't about the BUFFER-RELEASE method specifically, please change the header entry in DatabaseManager to:

** 051 VMN 20130510          Fixed registerDatabase, which must store Hibernate configs before
**                           computed columns are mapped.

Also, please correct Stansislav's date for the previous header entry 050. It should be 20130331, not 20120331.

After you make these changes, please continue your development on top of this update. We won't regression test it separately, but rather combined with your FIND-BY-ROWID changes, when they are ready.

I am comparing and fixing differences with 4GL and P2J error handling after I implemented method FIND-BY-ROWID as

   public logical findByRowID(rowid id, LockType lockType)
   {
      try
      {
         //ErrorManager.enableSilentError();
         new FindQuery(buffer(),
                       buffer().getDMOAlias() + ".id = " + longRowid,
                       null,
                       null,
                       new Object[] { id },
                       lockType).unique();
         //ErrorManager.disableSilentError();
      }
      catch (IllegalArgumentException exc)
      {
         success = false;
      }
      catch (ErrorConditionException exc)
      {
         success = false;
      }
      catch (QueryOffEndException exc)
      {
         success = false;
      }
      return new logical(success);
   }

Some examples:

1.

def var log as logical no-undo.

def temp-table tt1
field f1 as character
field f2 as character
field f3 as int
field f4 as int.

def temp-table tt2
field f1 as character
field f2 as character
field f3 as int
field f4 as int.

create tt2.

def buffer b1 for tt1.
def var hBuffer1 as handle.
hBuffer1 = buffer b1:handle.

log = hBuffer1:FIND-BY-ROWID(rowid(tt2)).
message "after find" log.

4GL

after find no

P2J

• b1 record not on file (138)
  after find no

2.

def var log as logical no-undo. 

def temp-table tt1 
field f1 as character 
field f2 as character 
field f3 as int 
field f4 as int. 

create tt1. 
assign f1 = "aa" 
f2 = "ccc" 
f3 = 3 
f4 = 4. 

def buffer b1 for tt1. 
def var hBuffer1 as handle. 
hBuffer1 = buffer b1:handle. 

log = hBuffer1:FIND-BY-ROWID(rowid(tt1)). 
message "after find" log. 

4GL

after find yes

P2J

• b1 record not on file (138)
  after find no

3.

def var log as logical no-undo. 

def temp-table tt1 
field f1 as character 
field f2 as character 
field f3 as int 
field f4 as int. 

def buffer b1 for tt1. 
def var hBuffer1 as handle. 
hBuffer1 = buffer b1:handle. 

log = hBuffer1:FIND-BY-ROWID(rowid(tt1)). 
message "after find" log. 

4GL

Invalid rowid for FIND-BY-ROWID method of BUFFER object b1. (7341)
after find no

P2J

• b1 record not on file (138)
  after find no

Now it is already corrected with check

      Long longRowid = id.getValue();
      if (longRowid == null)
      {
         String msg = "Invalid rowid for FIND-BY-ROWID method of BUFFER object " +
            buffer().getDMOAlias();
         ErrorManager.recordOrShowError(7341, msg, false);
         return new logical(false);
      }

I am continuing creating tests and fixing such places

I think you need to execute the FindQuery with silent error mode enabled, then return the result of ErrorManager.isError(). Please try something like:

StringBuilder sb = new StringBuilder(buffer().getDMOAlias());
sb.append(".id = ");
sb.append(longRowid);

ErrorManager.enableSilentError();
new FindQuery(buffer(),
              sb.toString(),
              null,
              null,
              new Object[] { id },
              lockType).unique();
ErrorManager.disableSilentError();

return ErrorManager.isError();

Vadim Nebogatov wrote:

Eric Faulhaber wrote:

I think you need to execute the FindQuery with silent error mode enabled, then return the result of ErrorManager.isError(). Please try something like:
[...]

Do you mean RecordBuffer.enableSilentError() or ErrorManager.silentErrorEnable()?

ErrorManager.silentErrorEnable().

I thought these methods are still not implemented properly and prepared update based on replace boolean errorOnNull to the object.

Where did that come from? Have you found cases where silent error mode is not implemented correctly?

All 3 examples in note 184 show the same messages in P2J as in 4GL.

OK, but consider there are other cases where an error can be raised, which probably are not handled properly yet. For example, if the record you are trying to find with a lock is already locked by another user, you could get a "** <table> record is locked (445)" error (you will only see this using a permanent table and multiple sessions). I think my suggested approach above would deal with this.

Please check attached vmn_upd20130522a. I have read your message when update was ready.

Please help me understand the need for the ErrorIfNull enum and the changes to RecordBuffer.errorNotOnFile. Specifically, what case drove the requirement to add the logic path which results in the call to ErrorManager.recordOrShowError? Under what circumstances are SHOW_ERROR_MODAL and SHOW_ERROR_MODELESS needed?

When you check a rowid for validity, please use rowid.isUnknown() instead of rowid.getValue().

Eric Faulhaber wrote:

Vadim Nebogatov wrote:

I thought these methods are still not implemented properly and prepared update based on replace boolean errorOnNull to the object.

Where did that come from? Have you found cases where silent error mode is not implemented correctly?

I thought this because of the notes 85-89, namely phrase "The new API will need to honor silent error mode and save the error data."

Eric Faulhaber wrote:

Please help me understand the need for the ErrorIfNull enum and the changes to RecordBuffer.errorNotOnFile. Specifically, what case drove the requirement to add the logic path which results in the call to ErrorManager.recordOrShowError? Under what circumstances are SHOW_ERROR_MODAL and SHOW_ERROR_MODELESS needed?

It was my initial idea to use these constants in order to show error instead of throw. I meant these constants would be useful, but, you are right, in process of development and testing it is turned out that they are not used now. I will roll back it.

Eric Faulhaber wrote:

I think you need to execute the FindQuery with silent error mode enabled, then return the result of ErrorManager.isError().

Possible new logical(!ErrorManager.isError().booleanValue()); ?

Vadim Nebogatov wrote:

Eric Faulhaber wrote:

Vadim Nebogatov wrote:

I thought these methods are still not implemented properly and prepared update based on replace boolean errorOnNull to the object.

Where did that come from? Have you found cases where silent error mode is not implemented correctly?

I thought this because of the notes 85-89, namely phrase "The new API will need to honor silent error mode and save the error data."

OK, yes, but fixing that should be handled as a separate issue. I still think we need to use the ErrorManager API, knowing that it will be fixed.

Vadim Nebogatov wrote:

Eric Faulhaber wrote:

I think you need to execute the FindQuery with silent error mode enabled, then return the result of ErrorManager.isError().

Possible new logical(!ErrorManager.isError().booleanValue()); ?

You are right, we need the opposite result. I would suggest new logical(!ErrorManager.isErrorFlag()).

Created example for test FIND-BY-ROWID with locked records, and found also NPE on RandomAccessQuery, line 673 - indexHelper is null for some reason, investigation is in progress.

Example is

lock-blk:
repeat transaction on error undo, retry:

  message "top of loop...".
  if retry then
  do:
    if avail person then
      message "buffer record not released upon retry".
  end.
  pause.

  def buffer b1 for person.
  def var hBuffer1 as handle.
  hBuffer1 = buffer b1:handle.

  def var log as logical no-undo.
create person.
  log = hBuffer1:FIND-BY-ROWID(rowid(person)).

/*
*  find first person exclusive-lock
*    no-wait no-error.
*/

  if not avail person then
  do:
    if locked person then
      message "LOCKED OUT".
    else
      message "NOT FOUND".
  end.
  else
    message "GOT IT!".

  message "About to apply ERROR".
  pause.

  apply "error".

end.

There is probably an implicit assumption that there will always be a valid sort (order by) clause passed into the FindQuery c'tor. This NPE is probably the result of a null sort phrase being passed.

In the case of a query which searches only on a row ID, I believe this would be the table's primary index. Unfortunately, being able to come up with the correct matching sort phrase for the table's primary index at runtime again presupposes we have runtime conversion support, which is still not ready.

Code review 20130606:

• prepare an update which backs out the index-related changes for findByRowID, but leaves all of your other changes, and includes all other pending changes for this issue;
• switch to #2043 (all history updates to the parent issue #1588, please).

Regression seems passed. There are 2-3 errors related with timeout and unexpected EOF. No 7345 errors ("Unable to CREATE/DELETE unless a TRANSACTION is running") in html files

I looked over the test results.

One of the failures, tc_job_002, is expected, as described in Constantin's regression testing notes.

Another failure, tc_so_stat_maint_003, failed this time but not last time. I can't tell what went wrong here. A secondary failure, tc_so_stat_maint_005 was a dependency of that test, so this failure is understandable.

Another failure, tc_job_matlcron_001 is a bit concerning. The failing screen displays the error message:

data_collection_time_attend in use by syman, syman. Wait or press CTRL-C to stop

The tc_job_matlcron_003 test also failed in both runs, but differently each time. It seems like in this run, the report simply did not complete in the timeout period.

Please run regression testing again and check the results against the above. If the same tests fail (other than tc_job_002), this suggests there is a real regression. If no tests fail, then we are good for check-in. If different tests fail, please do an analysis similar to the above and post it here, and we'll figure out where to go from there.

Regression 20130711_075408 completed with the following results.

Only 2 tests failed: tc_job_002 as expected and tc_job_clock_002.

tc_job_clock_002

Failure reason(s): failure in step 20: 'timeout before the specific screen buffer became available (Mismatched data at line 5, column 18. Expected '' (0x0000 at relative Y 5, relative X 18) and found '┌' (0x250C at relative Y 5, relative X 18).)'

This test passed in previous regression 20130705_195935.

So, I think everything is ok now.

OK, please check in and distribute this update.

Please summarize what methods are left to be implemented in order to complete this task. If any of them require metadata support, please describe the information needed and a proposed MetaSchema API to provide that information (similar to #1668, note 114).

I realize some still require other dynamic support which is not yet complete, such as dynamic buffer or query creation. I'm just trying to get an idea of the remaining dependencies of this task on others in process.

I suggest to include the following methods to MetaSchema API

   /**
    * Get an iterator on all indexes for the DMO specified by the given
    * schema and interface.  The resulting iterator returns
    * P2JIndex objects, each of which defines the columns
    * participating in an index.
    *
    * @param   schema
    *          Name of database schema.
    * @param   database
    *          Database containing the indices being queried.
    * @param   iface
    *          DMO interface.
    *
    * @return  Iterator on index definition objects.
    *
    */
   public Iterator<P2JIndex> getDatabaseIndexes(String schema,
                                                Database database,
                                                Class iface);

   /**
    * Get primary index for the DMO specified by the given
    * schema and interface.
    *
    * @param   schema
    *          Name of database schema.
    * @param   database
    *          Database containing the indices being queried.
    * @param   iface
    *          DMO interface.
    *
    * @return  Index definition object.
    *
    */
   public Iterator<P2JIndex> getPrimaryIndex(String schema,
                                             Database database,
                                             Class iface);

Some methods of DMOIndex and Persistence also could be rewritten using MetaSchema instead of using DatabaseMetaData.

About using DatabaseMetaData for PostgreSQL. Have you discussed using direct sql select to information_schema instead of using DatabaseMetaData? I know that for some column types DatabaseMetaData gives wrong results, unfortunately I cannot give an example right now.

I have corrected methods signatures in note 205.

CompoundQuery.bufferHandle(int bufferSequenceNumber): could be based on recordBuffers() method returning iterator, but it seems iterator order is not the same as required in spec:

"Sequence numbers for buffers of a query start at one, where one represents the top level and subsequent numbers represent lower levels of join, if any." 

Support of methods added in issue #1947:

BUFFER-FIELD (KW_BUF_FLD)

Implemented with MetaShemaImpl (stub with one method so far).

GET-BUFFER-HANDLE (KW_GET_BUFH)

Implemented. It is possible that order is not quite correct for bufferHandle(int bufferSequenceNumber) in CompoundQuery and PreselectQuery.

BUFFER-COPY (KW_BUF_COPY)
BUFFER-COMPARE (KW_BUF_COMP)
FIND-FIRST (KW_FIND_1ST)
FIND-UNIQUE (KW_FIND_UNI)
FIND-LAST (KW_FIND_LST)
FIND-BY-ROWID (KW_FIND_BR)
REPOSITION-TO-ROWID (KW_REPOS_2I)
REPOSITION-FORWARD (KW_REPOS_F)
QUERY-CLOSE (KW_QRY_CLOS)
REPOSITION-TO-ROW (KW_REPOS_2R)
BUFFER-RELEASE (KW_BUF_REL)
GET-CURRENT (KW_GET_CUR)

Earlier implemented.

DISABLE-LOAD-TRIGGERS (KW_DIS_L_TR)

Reassigned to #1949: