Allow binding of PL/SQL indexed tables

[doberkofler]

This pull request fixes the issue #98. It enhances the API to allow to bind procedural code using PL/SQL indexed tables.

Summary:

• added support for PL/SQL indexed tables
• arrays containing strings and numbers are supported (because of an OCI problem dates are not yet supported)
• only the object syntax of bindings is allowed
• IN, IN OUT and OUT binding are fully supported
• IN bindings only need a val property defined as an array to be identified as array binding
• IN OUT and OUT binding additionally need the maxSize and maxArraySize properties to be specified
• The new unit test PLSQLbinds.js shows all types of binding and also tests for all possible exceptions
• The documentation has not been adapted

PL/SQL:

...
TYPE stringsType IS TABLE OF VARCHAR2(30) INDEX BY BINARY_INTEGER;
TYPE numbersType IS TABLE OF NUMBER INDEX BY BINARY_INTEGER;
FUNCTION test(strings IN stringsType, numbers IN numbersType) RETURN VARCHAR2;
...

JavaScript:

var bindvars = {
  result: {type: oracledb.STRING, dir: oracledb.BIND_OUT, maxSize: 2000},
  strings:  {type: oracledb.STRING, dir: oracledb.BIND_IN, val: ['John', 'Doe']},
  numbers: {type: oracledb.NUMBER, dir: oracledb.BIND_IN, val: [0, 8, 11]}
};

connection.execute(
  "BEGIN :result := oracledb_testpack.test(:strings, :numbers); END;",
  bindvars,
  function(err, result) {
    should.not.exist(err);
    result.outBinds.result.should.be.exactly('JohnDoe0811');
  }
);

[pvenkatraman]

Where is NJSDebug::dump() is used in the code? Is it for debugging purpose?

[doberkofler]

@pvenkatraman Yes, I've use it extensively for debugging purposes but have removed its invocations.

[doberkofler]

@cjbj I have just merged in the 1.5 changes to make the pull request again conflict free.

[doberkofler]

@cjbj Is there anything else you need me do in order to have my pull request merged?

[cjbj]

@doberkofler thanks for updating it. I'm just back from vacation and will catch up with @pvenkatraman about the feature. It's on our list to review for 1.6.

[cjbj]

An update on this: @pvenkatraman is working on the merge. He is
looking at adding support for handling NULLs with IN binds; tweaking
the memory allocator calls for OUT binds; and adding some checking for
unsupported types. @octokyle also found some issues with range checks
too. @doberkofler - it's probably easiest for us to tidy these things
up as we find them while doing the merge.

[doberkofler]

@cjbj the existing code base is not the easiest to understand and I spend quite some time to get to know the code and try to fit the (currently undocumented) coding patterns and style. I must admit, that I would have preferred to have a chance to better understand where I might have made mistakes and/or to discuss them. Having changes modified and merged by others without a proper discussion does not really help a new project member and is actually also not very motivating.

[cjbj]

@doberkofler we can certainly discuss the issues we see before making changes. You can then decide whether to let us do the work or whether you want to spend more of your own time. I don't think anything is big. Your PR is great. @pvenkatraman is the best person to make comments; I'll let him jump in.

Feel free to shoot us any questions on the code base at anytime.

[pvenkatraman]

@doberkofler - The allocation AllocateBindArray() in Connection::GetOutBindParamsArray () function does not look right. For OUT binds, the allocation happens after the SQL execution in Connection::Async_AfterExecute (). As your code drop has working test cases, I am trying to understand and place this allocation correctly. Memory leak issues are difficult to investigate in node environment and would like to get this cleared off first.
NULL values in IN-Bind array - not allowed. This we need to allow as we do support NULL values for all other types.
You are using one error message with long-substituion string(s). Would like to replace this multiple error strings appropriately.
The enhancement is done in dpiStmt::bind () - bind-by-name function only, the support has to be added to bind-by-number also.
Thanks,

[doberkofler]

@pvenkatraman Thank you for reviewing my pull request. I would just have a few quick questions before I dig into the code:

• The allocation AllocateBindArray() in Connection::GetOutBindParamsArray () function does not look right.
  Did you actually discover a memory leak or is this based on your code review?
• NULL values in IN-Bind array - not allowed.
  It is clearly intended to allow NULL values but I seem to have "lost" my unit tests. Did it fail when you tested it?
• The enhancement is done in dpiStmt::bind () - bind-by-name function only
  Could you elaborate, on what you mean with "bind-by-number" when binding a PL/SQL array?

[pvenkatraman]

@doberkofler
My comments:
The allocation AllocateBindArray() in Connection::GetOutBindParamsArray () function does not look right.
Did you actually discover a memory leak or is this based on your code review?

• No, we are still testing your drop as it is and looking for such issues. For OUT binds, the allocation happens in Connection::cbDynBufferAllocate (), which includes DML returning also. Ideally I would like to move all allocation in one place. I need to walk through your code in debugger as I believe allocation for Array-Bind will happen in 2 places.

NULL values in IN bind array

• A test case would have helped. There are 3 loops in GetInBindParamsArray () and you are validating the IN value in the first loop, I want to make sure it supports NULL/Undefined value(s).
  If you have a test case, please pass it to me.
• src/dpi/include/dpiStmt.h - contains 2 bind() function. The first parameter for one is a string (used for Bind-by-name). Another has number (used for Bind-by-Position). Any support we need to add for both bind operations.
  Hope this helps.
  Thanks

[doberkofler]

@pvenkatraman

array allocation

When looking at switch(dir) in the method Connection::GetBindUnit you see that depending on IN, OUT or INOUT different functions are dispatched. It is my understanding that for IN and INOUT I had to allocate the array of bind variables needed to provide OCI with the values and additionally (but not either/or) for INOUT and OUT the method Connection::cbDynBufferAllocate () takes care of allocating the buffers where the values returned from OCI will be stored. I modeled my allocation mechanism based on how the scalar values are bind. I actually also found the mechanism quite complicated and would have preferred to combine it but I gave up in doing so after a while because if gets pretty complicated as Connection::cbDynBufferAllocate () is also used for other purposes.

[doberkofler]

@pvenkatraman

NULL values in IN bind array

I double checked my code and the processing of NULL values appears to be correct.
To make sure that I did not miss anything a now also added an unit test that explicitly uses 'null' values and it also worked as expected. Could you please elaborate on the problems you have found or do you have a concrete example of the problem with NULL values ?

[doberkofler]

@pvenkatraman
@cjbj

Bind-by-Position

I now double checked my implementation and came to the conclusion, that positional binding of arrays is actually possible and relatively easy to implement but make the API really hard to understand.
The only way to do this is to make an educated guess by either always assuming an array of strings or look at the first element and hope that the array types are consistent. Especially when the first element is NULL this leads to a lot of exceptions and rather strange error messages that seem quite odd to me.
OI strongly believe that when binding PL/SQL tables, we should only support binding by name and demand that the bind type has been explicitly given. This is how it currently implemented and I actually believe, we should not change it.

[cjbj]

@doberkofler

1. We're OK with deferring bind-by-pos until later. We don't want to delay getting what you have working.
   The add-on just needs clear errors when the user attempts bind-by-pos.
2. With bind-by-pos you could require the 'type' be specified, similar to this scalar snippet:

        connection.execute(
          "INSERT INTO test2 VALUES (:id, :nm)",
          [
            {val: 2, type:oracledb.NUMBER},
            {val: 'Alison', type:oracledb.STRING}
          ], . . .

[doberkofler]

@cjbj

The add-on just needs clear errors when the user attempts bind-by-pos.

I absolutely agree on this, if positional binding is not supported.

With bind-by-pos you could require the 'type' be specified, similar to this scalar snippet

I seem to have misunderstood the positional binding by looking at the documentation.
I was referring to some code like this:

connection.execute("INSERT INTO test2 VALUES (:ids)",
   [
      [1, 2, 3]
   ], . . .

that I would not want to support.

An example like this:

connection.execute("INSERT INTO test2 VALUES (:ids)",
   [
      {val: [1, 2, 3], type:oracledb.NUMBER}
   ], . . .

should be supported and I will try to add this in the next couple of days.

[doberkofler]

@pvenkatraman
@cjbj

You are using one error message with long-substituion string(s)

I get your point but I'm really not sure if using separate error messages for the rather large number of parameter validation errors would not only pollute the list of error messages. I generally agree that having separate error messages with individual numbers is a good idea but I guess I'm not convinces that the individual parameter validation errors are really different errors and not only different extra information on one and the same error.

[cjbj]

@doberkofler keep us in the loop on progress. I don't want to break our current cadence of monthly releases and we'll need some time to complete testing. I'd prefer to get all the other small issues sorted and add bind-by-pos next month.

One general problem with substitution in messages is it can make messages harder to translate (if we ever translate them).

[pvenkatraman]

Please move this allocation to Connection::cbDynBufferAllocate (). For IN (IN & INOUT) binds from the value, we allocate for C-type and fill in the value(s). For OUT bind this is deferred to Connection::PrepareAndBind(). Only after OCIStmtPrepare2() we can determine whether the given SQL statement contains RETURNING INTO clause (DML returning). In this case, the OUT bind will return an array of values.

It is difficult to consolidate all the allocations to one place, but we can always try in another transaction.

As your drop is allocating for array, moving this allocation to Connection::cbDynBufferAllocate would be appropriate.

[pvenkatraman]

Please try the test case:
set serveroutput on;

CREATE OR REPLACE PACKAGE oracledb_testpack
IS
TYPE stringsType IS TABLE OF VARCHAR2(30) INDEX BY BINARY_INTEGER;
TYPE numbersType IS TABLE OF NUMBER INDEX BY BINARY_INTEGER;
FUNCTION test(strings IN stringsType, numbers IN numbersType) RETURN
VARCHAR2;
END;
/

CREATE OR REPLACE PACKAGE BODY oracledb_testpack
IS
FUNCTION test(strings IN stringsType, numbers IN numbersType) RETURN
VARCHAR2
IS
s VARCHAR2(2000) := '';
BEGIN
FOR i IN 1 .. strings.COUNT LOOP
s := s || strings(i);
END LOOP;
FOR i IN 1 .. numbers.COUNT LOOP
s := s || numbers(i);
END LOOP;
RETURN s;
END;
END;
/

set serveroutput on;

CREATE OR REPLACE PACKAGE oracledb_testpack
IS
TYPE stringsType IS TABLE OF VARCHAR2(30) INDEX BY BINARY_INTEGER;
TYPE numbersType IS TABLE OF NUMBER INDEX BY BINARY_INTEGER;
FUNCTION test(strings IN stringsType, numbers IN numbersType) RETURN
VARCHAR2;
END;
/

CREATE OR REPLACE PACKAGE BODY oracledb_testpack
IS
FUNCTION test(strings IN stringsType, numbers IN numbersType) RETURN
VARCHAR2
IS
s VARCHAR2(2000) := '';
BEGIN
FOR i IN 1 .. strings.COUNT LOOP
s := s || strings(i);
END LOOP;
FOR i IN 1 .. numbers.COUNT LOOP
s := s || numbers(i);
END LOOP;
RETURN s;
END;
END;
/

this reports NJS-034 - does not support NULL bind values.

[pvenkatraman]

To reply to your comment on one error message/multiple error messate: We do use same error message in multiple places and if it suits it is good. My comment particularly is about the substitution strings, which are bigger. We try to keep all code, strings etc within 80 characters as per our coding standards. My suggestion is to phrase multiple smaller error messages.

[pvenkatraman]

@doberkofler I am working on suggestions. Let me know your ideas too.

[doberkofler]

@cjbj

I want to make sure that I understand the positional binding correctly. Does the following unit test properly describe what you expect to be supported?

    it('43.1.2 binding PL/SQL indexed table IN by position', function(done) {
      async.series([
        function(callback) {
          var proc = "CREATE OR REPLACE PACKAGE\n" +
                      "oracledb_testpack\n" +
                      "IS\n" +
                      "  TYPE stringsType IS TABLE OF VARCHAR2(30) INDEX BY BINARY_INTEGER;\n" +
                      "  TYPE numbersType IS TABLE OF NUMBER INDEX BY BINARY_INTEGER;\n" +
                      "  PROCEDURE test(strings IN stringsType, numbers IN numbersType);\n" +
                      "END;";
          connection.should.be.ok;
          connection.execute(
            proc,
            function(err) {
              should.not.exist(err);
              callback();
            }
          );
        },
        function(callback) {
          var proc = "CREATE OR REPLACE PACKAGE BODY\n" +
                     "oracledb_testpack\n" +
                     "IS\n" +
                     "  PROCEDURE test(strings IN stringsType, numbers IN numbersType)\n" +
                     "  IS\n" +
                     "    s VARCHAR2(2000) := '';\n" +
                     "  BEGIN\n" +
                     "    FOR i IN 1 .. strings.COUNT LOOP\n" +
                     "      s := s || NVL(strings(i), 'NULL');\n" +
                     "    END LOOP;\n" +
                     "    FOR i IN 1 .. numbers.COUNT LOOP\n" +
                     "       s := s || NVL(numbers(i), 0);\n" +
                     "    END LOOP;\n" +
                     "    IF (s <> 'NULLJohnDoe0811') THEN\n" +
                     "      RAISE VALUE_ERROR;\n" +
                     "    END IF;\n" +
                     "  END;\n" +
                     "END;";
          connection.should.be.ok;
          connection.execute(
            proc,
            function(err) {
              should.not.exist(err);
              callback();
            }
          );
        },
        function(callback) {
          var bindvars = [
            {type: oracledb.STRING, dir: oracledb.BIND_IN, val: ['John', 'Doe']},
            {type: oracledb.NUMBER, dir: oracledb.BIND_IN, val: [8, 11]}
          ];
          connection.execute(
            "BEGIN :result := oracledb_testpack.test(:1, :2); END;",
            bindvars,
            function(err, result) {
              should.not.exist(err);
              // console.log(result);
              callback();
            }
          );
        },
        function(callback) {
          connection.execute(
            "DROP PACKAGE oracledb_testpack",
            function(err) {
              should.not.exist(err);
              callback();
            }
          );
        }
      ], done);
    });

[doberkofler]

@pvenkatraman

My comment particularly is about the substitution strings, which are bigger.

I understand but have just been trying to make the error messages as helpful as possible.
I will try to shorten them.

We try to keep all code, strings etc within 80 characters as per our coding standards.

I understand and do not want to get into the coding style discussion but may I ask 3 short questions:

• are the coding standards documented somewhere?
• can the coding standards be enforced by some tooling?
• and finally: why would the line length be restricted to 80 characters (this is quite an extreme limitation and makes the code so much harder to read)?

My suggestion is to phrase multiple smaller error messages.

I understand and will try to shorten them. Can the error messages have multiple lines if the 80 character line length seems to be so important?

[cjbj]

@doberkofler your sample looks fine to me:

          var bindvars = [
            {type: oracledb.STRING, dir: oracledb.BIND_IN, val: ['John', 'Doe']},
            {type: oracledb.NUMBER, dir: oracledb.BIND_IN, val: [8, 11]}
          ];

Are you treating mismatched array lengths as an error?

Don't worry about the 80 char limit; I think I convinced @pvenkatraman not to worry too. It came about from an ancient C compiler implementation on a particular platform that truncated longer lines. I agree that it looks too short for modern programming. The node-oracledb coding standard is not strict. Follow the general style and everything will be fine.

[doberkofler]

@pvenkatraman
@cjbj

Bind-by-Position

I just pushed the commit a211c8b that implements the positional binding and adds the needed unit test.

[cjbj]

@doberkofler Thanks! We'll pick it up and get ready for a release. Did you see my email?

[cjbj]

Merged to 1.6. Thanks @doberkofler !