FWD » Testing

Another issue related to testcases directory configuration: #9398-17. Roger, please review this as well.

Vladimir Tsichevski wrote:

The directory.xml.template.diff contains fixes for directory.xml.template. I will commit them if nobody objects.

Thanks for finding this, and pointing out. I will make the update.

As for the client_start_dir issue with the spawner failing if this is not an absolute path, I can fix that a little more creatively by updated prepare_dir.sh to determine the absolute path to ../client with:

client_start_dir=$(getval "client_start_dir" $(cd ${srcpath}/../client; echo $(pwd)) $infile)

Remember that these are configuration files, intended to be modifiable for each user's particular setup. That being said, I do agree defaulting them to a relative path was wrong. Thanks for your patience in finding this.

I forgot to mention the JSON files are themselves templates, which causes problems:

vvt@gcddev$ ./prepare_dir.sh -f prepare_dir_postgres.json 
parse error: Invalid numeric literal at line 37, column 22
parse error: Invalid numeric literal at line 37, column 22
parse error: Invalid numeric literal at line 37, column 22
parse error: Invalid numeric literal at line 37, column 22
parse error: Invalid numeric literal at line 37, column 22
parse error: Invalid numeric literal at line 37, column 22
parse error: Invalid numeric literal at line 37, column 22
parse error: Invalid numeric literal at line 37, column 22
parse error: Invalid numeric literal at line 37, column 22
parse error: Invalid numeric literal at line 37, column 22
parse error: Invalid numeric literal at line 37, column 22
parse error: Invalid numeric literal at line 37, column 22
parse error: Invalid numeric literal at line 37, column 22
parse error: Invalid numeric literal at line 37, column 22
parse error: Invalid numeric literal at line 37, column 22
parse error: Invalid numeric literal at line 37, column 22
parse error: Invalid numeric literal at line 37, column 22
parse error: Invalid numeric literal at line 37, column 22
parse error: Invalid numeric literal at line 37, column 22
parse error: Invalid numeric literal at line 37, column 22
parse error: Invalid numeric literal at line 37, column 22
parse error: Invalid numeric literal at line 37, column 22
parse error: Invalid numeric literal at line 37, column 22
parse error: Invalid numeric literal at line 37, column 22
parse error: Invalid numeric literal at line 37, column 22
parse error: Invalid numeric literal at line 37, column 22
parse error: Invalid numeric literal at line 37, column 22
parse error: Invalid numeric literal at line 37, column 22
dirname: missing operand
Try 'dirname --help' for more information.
realpath: missing operand
Try 'realpath --help' for more information.
basename: missing operand
Try 'basename --help' for more information.
ERROR: Output file / exists, and '-o' not specified.
Usage: ./prepare_dir.sh [-o] [-f <json_cfg_file>]
Where:
    o = Overwrite the specified directory.xml, otherwise fail
    f = input from [json_cfg_file] (def="prepare_dir_postgres.json". To use the default, make sure this is the last parameter.)

Vladimir Tsichevski wrote:

I forgot to mention the JSON files are themselves templates, which causes problems:

Yes... I am addressing this in testcases_setup.sh, which will create a non-template prepare_dir.json if you are configuring for postgres.

Yes. Also, use bzr+ssh instead of sftp.

The fragments are combined with com.goldencode.p2j.directory.DirectoryCopy, so perhaps that tool is not preserving case? It is calling the below when copy is passed:

      // copy the branch between directories
      rc = DirectoryService.copySubtree(rs, args[1], rt, args[3], true);

Eugenie, do you know whether or not the case could be preserved?

Roger Borrello wrote:

The fragments are combined with com.goldencode.p2j.directory.DirectoryCopy, so perhaps that tool is not preserving case? It is calling the below when copy is passed:
[...]

Eugenie, do you know whether or not the case could be preserved?

I think we should assume the names are not case sensitive. So it is better to avoid using CamelCase formatting for complex names, instead of veryLongName use very-long-name or very_long_name.

The fragments are combined with com.goldencode.p2j.directory.DirectoryCopy, so perhaps that tool is not preserving case?

Unless it is documented to change the case, we should preserve it.

If I get the logic of prepare_dir.sh right, the directory_db.xml.template unconditionally replaces the corresponding fragment at /server/standard/database in directory.xml.template (which is explicitly for h2).

Is this correct?

If yes, then this replacement may be done with the directory.xml.template file in repository, so no call to com.goldencode.p2j.directory.DirectoryCopy (and overall dependence of the configuration scripts on FWD binaries) is required anymore, and the directory_db.xml.template file can be eliminated.
If no, then the script shall be fixed for h2-based configurations.

Vladimir Tsichevski wrote:

If I get the logic of prepare_dir.sh right, the directory_db.xml.template unconditionally replaces the corresponding fragment at /server/standard/database in directory.xml.template (which is explicitly for h2).

Is this correct?

No... The same process happens if you are configuring postgres or h2. The template is swept with values based upon the DB type and named for the DB, then each is merged into the directory. This way multiple DB configurations can be handled.

Roger Borrello wrote:

Vladimir Tsichevski wrote:

If I get the logic of prepare_dir.sh right, the directory_db.xml.template unconditionally replaces the corresponding fragment at /server/standard/database in directory.xml.template (which is explicitly for h2).

Is this correct?

No... The same process happens if you are configuring postgres or h2. The template is swept with values based upon the DB type and named for the DB, then each is merged into the directory. This way multiple DB configurations can be handled.

I mean not the values, but the template fragment itself. The existing DB node in directory.xml.template is always replaced by the contents of directory_db.xml.template. If this is OK for h2, then the original DB part of the original directory.xml.template is dead code, and can be replaced in the original directory.xml.template repository source.

Vladimir Tsichevski wrote:

I mean not the values, but the template fragment itself. The existing DB node in directory.xml.template is always replaced by the contents of directory_db.xml.template. If this is OK for h2, then the original DB part of the original directory.xml.template is dead code, and can be replaced in the original directory.xml.template repository source.

The template fragment is to be used in the directory an unknown number of times. It has to do with the number of databases that are to be configured. In many cases you could have "shards" of databases configured. For example, if your JSON has:

{
...
   "dbnames"   : "hotel,ledger,prices",
   "hotel"  :
   { 
      "dbtype"     : "postgres",
      "dbuser"     : "fwd_user",
      "dbuserpass" : "user",
      "dbadmin"    : "fwd_admin",
      "dbadminpass": "admin",
      "collation"  : "en_US@iso88591_fwd_basic",
      "hostname"   : "localhost",
      "port"       : 5432
   },
   "ledger" :
   {
      "dbtype"     : "postgres",
      "dbuser"     : "fwd_user",
      "dbuserpass" : "user",
      "dbadmin"    : "fwd_admin",
      "dbadminpass": "admin",
      "collation"  : "en_US@iso88591_fwd_basic",
      "hostname"   : "localhost",
      "port"       : 5432
   },
   "prices" :
   {
      "dbtype"     : "postgres",
      "dbuser"     : "fwd_user",
      "dbuserpass" : "user",
      "dbadmin"    : "fwd_admin",
      "dbadminpass": "admin",
      "collation"  : "en_US@iso88591_fwd_basic",
      "hostname"   : "localhost",
      "port"       : 5432
   }
}

You will need 3 of the directory_db.xml.template fragments to be included in the directory. Each with its own substituted values.

Edited #9525-21 for clarity.

Roger Borrello wrote:

Edited #9525-21 for clarity.

Now I see why the directory copy is necessary.

But anyway, the existing database node in directory.xml.template is always replaced by the prepared node for the first item in $dbnames.
So this part of template is never used (and must never be!) and has to be removed to avoid reader confusion and make the program less fragile and faster a bit.

Vladimir Tsichevski wrote:

But anyway, the existing database node in directory.xml.template is always replaced by the prepared node for the first item in $dbnames.
So this part of template is never used (and must never be!) and has to be removed to avoid reader confusion and make the program less fragile and faster a bit.

I see what you are finding. I am comparing the DB template in the testcases to the one in the Hotel GUI, and I do notice a difference I'd like to get some feedback on. There is a "_temp" database in the Hotel GUI:

  <node class="container" name="">
   <node class="container" name="server">
     <node class="container" name="standard">
       <node class="container" name="database">
          <node class="container" name="_temp">
            <node class="container" name="p2j">
              <node class="string" name="schema">
                <node-attribute name="value" value="_temp"/>
              </node>
              <node class="string" name="embedded-collation">
                <node-attribute name="value" value="{collation}"/>
              </node>
            </node>
          </node>

Also in Hotel GUI there are some settings for the DB being entered:

   <node class="container" name="server">
     <node class="container" name="standard">
       <node class="container" name="database">
          <node class="container" name="{dbname}">
            <node class="container" name="orm">
              <node class="container" name="c3p0">
                <node class="boolean" name="validate">
                  <node-attribute name="value" value="FALSE"/>
                </node>
              </node>
              <node class="container" name="cache">
                <node class="boolean" name="use_second_level_cache">
                  <node-attribute name="value" value="FALSE"/>
                </node>
                <node class="boolean" name="use_query_cache">
                  <node-attribute name="value" value="FALSE"/>
                </node>
              </node>

I could keep these 2 things different between Hotel GUI and the testcases, but if they should match, now is the time to correct one of them.

So the directory.xml.template will just have:

        <node class="container" name="database">
        </node>

Roger Borrello wrote:

Vladimir Tsichevski wrote:

But anyway, the existing database node in directory.xml.template is always replaced by the prepared node for the first item in $dbnames.
So this part of template is never used (and must never be!) and has to be removed to avoid reader confusion and make the program less fragile and faster a bit.

I see what you are finding. I am comparing the DB template in the testcases to the one in the Hotel GUI, and I do notice a difference I'd like to get some feedback on. There is a "_temp" database in the Hotel GUI:
[...]

Probably, this entry shall retain as-is. As long as the name of the database node does not clash with dbnames, the directory tool will not change this node.

Also in Hotel GUI there are some settings for the DB being entered:
[...]

I could keep these 2 things different between Hotel GUI and the testcases, but if they should match, now is the time to correct one of them.

Yes, the nodes shall be compared, merged and "There Can Be Only One".

So the directory.xml.template will just have:
[...]

In case of testcases, I suppose yes.

Thanks, Vladimir. Greg, there are some entries in Hotel GUI which I noted, and I'm not sure if they should be included in the DB setup. Is there another reviewer that can check on this? There is a _temp database defined, and the hotel database includes orm/c3po settings (cache/use_second_level_cache and cache/use_query_cache). Should these be generally included in the testcase DB (as well as any others that follow the Hotel GUI template)?

Roger Borrello wrote:

Greg Shah wrote:

The fragments are combined with com.goldencode.p2j.directory.DirectoryCopy, so perhaps that tool is not preserving case?

Unless it is documented to change the case, we should preserve it.

I looked through the Chapter 12 Directory wiki, and all it mentions is that links are case-insensitive. It doesn't specify details related to that in any of the sections.

On #8876, the template XML I was using has the case of the keys lowercased when the DirectoryCopy takes place. It seems this is deep within directory.IdUtils, and other classes related to directory. It seems like a risky area to make changes. However, it does pose a problem with some tools that may be searching for things within the directory, and not finding them because the case doesn't match.

The problem is likely due to using the normalized nodeId in add or copy operations.

Under which project would this bug be created?

This one.

Roger Borrello wrote:

Thanks, Vladimir. Greg, there are some entries in Hotel GUI which I noted, and I'm not sure if they should be included in the DB setup. Is there another reviewer that can check on this? There is a _temp database defined,

I don't know if this is still needed. IIRC, the only remaining use for it (as of some years ago) was to set the collation of the embedded, H2, temp-table database, using the embedded-collation config node. But this code changed significantly when we added support for some legacy codepage and collation features.

Ovidiu, any thoughts on this?

and the hotel database includes orm/c3po settings (cache/use_second_level_cache and cache/use_query_cache). Should these be generally included in the testcase DB (as well as any others that follow the Hotel GUI template)?

The whole cache section should be removed. use_second_level_cache and use_query_cache are Hibernate-era settings which are no longer in use. The c3p0 section should stay, but please remove the maxStatementsPerConnection setting. This refers to a prepared statement cache within the connection pooling software. The setting was speculative and could be beneficial, neutral, or detrimental, depending on the application. We shouldn't be setting it by default.

Eric Faulhaber wrote:

Thanks, Vladimir. Greg, there are some entries in Hotel GUI which I noted, and I'm not sure if they should be included in the DB setup. Is there another reviewer that can check on this? There is a _temp database defined,

I don't know if this is still needed. IIRC, the only remaining use for it (as of some years ago) was to set the collation of the embedded, H2, temp-table database, using the embedded-collation config node. But this code changed significantly when we added support for some legacy codepage and collation features.
Ovidiu, any thoughts on this?

Yes, the embedded-collation is still needed. It is the way to load collations from com.goldencode.p2j.spi package. Off the top of my head, it is probably the only active setting for _temp database.

Ovidiu Maxiniuc wrote:

Yes, the embedded-collation is still needed. It is the way to load collations from com.goldencode.p2j.spi package. Off the top of my head, it is probably the only active setting for _temp database.

I'm not clear on the default value for this. In H2Helper, we have this:

   /** Default embedded collation. Intentionally use of AT. It will be replaced with underscore. */
   private static final String DEFAULT_COLLATION = "en_US@iso88591_fwd_basic";

IIRC, the default used to be en_US_P2J.

If no override of DEFAULT_COLLATION is found in the directory, there is a SET COLLATION call to use this default (with the @ symbol replace by _). Does this properly resolve to a Java collation provider from fwdspi.jar?

Assuming yes, we only need the _temp container in the directory if we need to override the default embedded-collation, correct?

As more customers use UTF8 databases, is en_US@iso88591_fwd_basic still a sensible default?

• the DEFAULT_COLLATION = "en_US@iso88591_fwd_basic" (all variants en_US, en_GB, nl_NL) are mapped to com.goldencode.p2j.spi.ISO_8859_1Collator
• the en_US@P2J is also mapped to com.goldencode.p2j.spi.ISO_8859_1Collator
  so they are handled the same.

If I remember correctly, we kind of deprecated en_US@P2J in the favour of the more explicit en_US@iso88591_fwd_basic, but decided to keep the old name as an alias. The @ was replaced by _ because of some constraints with the locale naming. See #5491-44. We tried to unify the naming/aliases of the built-in collators with OS level locales.

If the embedded-collation is not specified, the "default Java collation" is used for H2 databases. So this entry in directory should be removed for setups which have UTF-8 native collations (no set collation ... will be executed at database initialization).

I looked back again to H2Helper.prepareDatabase. Indeed, the way it is written now, I do not think it is possible to get a null collation (because the DEFAULT_COLLATION is returned in case the directory key is not found).

Therefore, the native/utf collation is excluded. We need to either replace the DEFAULT_COLLATION with null or replace the if (collation != null) condition with one which would skip the collation configuration if the embedded-collation is utf8 or a similar name.

However, UTF-8 is just a character encoding schema, it does not impose any constraints on how the characters are sorted / collation.

• Regarding server/standard/database/_temp. Is that to be a database included by default in projects? It is in the Hotel ChUI and GUI project DB templates, but not in the testcases DB template. Should it be part of all directories?
• I am removing server/standard/database/{dbname}/orm/c3p0/maxStatementsPerConnection from the templates.
• I will continue to have server/standard/database/{dbname}/p2j/embedded-collation/{collation} be something swept via json configuration in the templates.

Roger,
The collation is an attribute of each database. The DBCOLLATION(db) function will return it. However, there is no way for passing the _temp database to it. _temp is a FWD specific alias. And the temp database notion is a bit different, as well. I assume its collation is the same as SESSION:CPCOLL (-cpinternal startup parameter). So I guess we want to use that value for _temp database.

I believe this task is ready for review.

Roger Borrello wrote:

I believe this task is ready for review.

Which changes are we expected to review?

Vladimir Tsichevski wrote:

Roger Borrello wrote:

I believe this task is ready for review.

Which changes are we expected to review?

Anything related to why you created the ticket. Everything in #9525-1

Roger Borrello wrote:

Vladimir Tsichevski wrote:

Roger Borrello wrote:

I believe this task is ready for review.

Which changes are we expected to review?

Anything related to why you created the ticket. Everything in #9525-1

I mean where can we find the changes? Which repositories, revisions?

Vladimir Tsichevski wrote:

I mean where can we find the changes? Which repositories, revisions?

The xfer.goldencode.com/opt/testcases/ is the repository I had committed my updates to:

rev 1711rev 1711

------------------------------------------------------------
revno: 1711
committer: Roger F Borrello <rfb@goldencode.com>
branch nick: testcases
timestamp: Wed 2025-03-05 12:20:22 -0500
message:
  Remove ignorelist and white list, replaced with zset files. Also removed an unneeded parameter from the directory DB template.
modified:
  build.xml
  deploy/server/directory_db.xml.template

rev 1696rev 1696

------------------------------------------------------------
revno: 1696
committer: Roger F Borrello <rfb@goldencode.com>
branch nick: testcases
timestamp: Mon 2025-01-27 19:17:48 -0500
message:
  Need to remove the DB section, as it is merged in with the DB template. Ref. #9525.
modified:
  deploy/server/directory.xml.template

rev 1687rev 1687

------------------------------------------------------------
revno: 1687
committer: Roger F Borrello <rfb@goldencode.com>
branch nick: testcases
timestamp: Wed 2025-01-15 14:00:11 -0500
message:
  Fixed some typos in the template, and also prevented relative paths from being in the 'client_start_dir' when using 'prepare_dir.sh'. Added capability to use a zset file in the build.xml for conversion. Update testcas
es_setup.sh to configure the JSON files.
modified:
  build.xml
  deploy/server/directory.xml.template
  deploy/server/prepare_dir.sh
  deploy/server/prepare_dir_h2.json
  deploy/server/prepare_dir_postgres.json
  testcases_setup.sh

Review xfertestcases: r1687, r1696, r1711.

Issues found in build.xml:

1. The convert target does not depend on check-zfile-set:

   <target name="convert" 
           depends="init, check-zfile-set" 
           description="Convert the 4GL source code to Java source code, using a pattern as input." 

2. Same for this:

   <target name="convert.front" 
           depends="init, check-zfile-set" 
           description="Execute the parse phase of the conversion process, using a pattern as input." 

3. And this:

   <target name="convert.middle" depends="init, check-zfile-set" 
           description="Execute the parse and schema conversion phases of the conversion process, using a pattern as input." 
           unless="no-list-mode">

Vladimir Tsichevski wrote:

Review xfertestcases: r1687, r1696, r1711.

You cut off the targets before the very important stuff... unless="no-list-mode">

   <target name="check-zfile-set">
      <available file="${zfile}" property="zfile-set"/>
   </target>

   <condition property="no-list-mode">
      <available file="${zfile}"/>
   </condition>

   <!-- convert 4GL code from ${app.4gl.src} into ${src}, using ${app.4gl.pattern} pattern -->
   <target name="convert" 
           depends="init, check-zfile-set" 
           description="Convert the 4GL source code to Java source code, using a pattern as input." 
           unless="no-list-mode">
...

   <target name="convert.front" 
           depends="init, check-zfile-set" 
           description="Execute the parse phase of the conversion process, using a pattern as input." 
           unless="no-list-mode">

Those targets are utilizing the ConversionDriver -S option, which takes the conversion list from the ${app.4gl.pattern} value. The .zset targets use the -Z option to ConversionDriver, and that requires the ${zset} file as input.

Roger Borrello wrote:

Vladimir Tsichevski wrote:

Review xfertestcases: r1687, r1696, r1711.

You cut off the targets before the very important stuff... unless="no-list-mode">
[...]
The check-zfile-set target will set zfile-set to true if there is a ${zfile} found. (This defaults to zfile_set.txt unless -Dzfile=... is passed in.) Likewise, no-list-mode is set if there is a ${zfile}. This satisfies the convert target (and convert.front and convert.middle) to only run if there isn't a ${zfile}.

Those targets are utilizing the ConversionDriver -S option, which takes the conversion list from the ${app.4gl.pattern} value. The .zset targets use the -Z option to ConversionDriver, and that requires the ${zset} file as input.

Understood. Based on this logic, conversion targets that do not rely on using zfile_set.txt are effectively unreachable. To make them active, you must first remove the zfile_set.txt file, which is risky since it’s part of the repository. I’d prefer a solution that allows running any conversion without deleting repository files.

Also, I would simplify things by removing all ‘old’ conversion targets entirely. I’d also rename the repository’s zfile_set.txt file to something like zfile_set.txt.template to prevent users from accidentally committing their customizations to the repository.

Vladimir, these build.xml files are written to handle conversion either via the fileset method (-Z) or the sources method (-S) of collecting files. Most projects will be one or the other, but the targets are in the build.xml for reference. I don't believe we know exactly how this project will be utilized in all use cases, so we need to leave them there.

I did follow your advice about moving zfile_set.txt. I named it zfile_set.txt.sample.

• There is a connections area set: server/default/runtime/default/database-connections="tstcasesdb"
• There is a _temp database: server/default/database/_temp/p2j
• Removed server/default/database/tstcasesdb/orm/c3p0/maxStatementsPerConnection and server/default/database/tstcasesdb/orm/c3p0/initialpoolsize

I think maxStatementsPerConnection and initialPoolSize from server/default/database/tstcasesdb/orm/c3p0 are still in use. Or there is a task for dropping them I am not aware? If the elimination is only in the testcases project, it's would be OK. But this project may be used as a template by the customers, so my opinion is that they should stay.

No other objections.

The testcases project will not be used as a template for customers. It is private.

Also: unless we need a specific value to be specified, the content should be removed.

Eric, is the server/standard/database/_temp necessary to have in the testcases and/or hotel directories?