Skip to content

feat: The ability to use an existing odbc connection #282

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 4 commits into
base: master
Choose a base branch
from
Open

feat: The ability to use an existing odbc connection #282

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
8c24c9a
added support for using an existing node-odbc connection
ehenson May 8, 2020
eac8aab
Updated the odbcOptions jsDoc
ehenson May 8, 2020
4e5a2f0
Updated the odbcOptions jsDoc
ehenson May 8, 2020
700b8e7
changed the function name from processXml to processResults
ehenson May 9, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions lib/Connection.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,7 @@ const availableTransports = {
* @property {string} [ipc=*NA] - The key name/security route to XMLSERVICE job. Default is ``*NA``.
* @property {string} [ctl=*here] - The control options for XMLSERVICE jobs. Default is ``*here``.
* @property {string} [xslib=QXMLSERV] - The XMLSERVICE library. Default is ``QXMLSERV``.
* @property {object} [odbcConnection] - An existing odbc connection.
*/

/**
Expand All @@ -48,6 +49,7 @@ const availableTransports = {
* @property {string} [ipc=*NA] - The key name/security route to XMLSERVICE job. Default is ``*NA``.
* @property {string} [ctl=*here] - The control options for XMLSERVICE jobs. Default is ``*here``.
* @property {string} [xslib=QXMLSERV] - The XMLSERVICE library. Default is ``QXMLSERV``.
* @property {object} odbcConnection - An optional odbc connection instead of a creating a new one.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Line 52 documents the idb transport options therefore we should not add odbcConnection property here.

Suggested change
* @property {object} odbcConnection - An optional odbc connection instead of a creating a new one.

*/

/**
Expand Down
57 changes: 37 additions & 20 deletions lib/transports/odbcTransport.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@ function odbcCall(config, xmlInput, done) {
const odbc = require('odbc');

const {
odbcConnection = null,
host = 'localhost',
username = null,
password = null,
Expand Down Expand Up @@ -51,36 +52,52 @@ function odbcCall(config, xmlInput, done) {
console.log(`SQL to run is ${sql}`);
}

odbc.connect(connectionString, (connectError, connection) => {
if (connectError) {
done(connectError, null);
function processResults(results) {
if (!results) {
done('Empty result set was returned', null);
return;
}

let xmlOutput = '';

results.forEach((chunk) => {
xmlOutput += chunk.OUT151;
});
done(null, xmlOutput);
}

function query(connection) {
connection.query(sql, [ipc, ctl, xmlInput], (queryError, results) => {
if (queryError) {
done(queryError, null);
return;
}
connection.close((closeError) => {
if (closeError) {
done(closeError, null);
return;
}

if (!results) {
done('Empty result set was returned', null);
return;
}
if (!odbcConnection) {
connection.close((closeError) => {
if (closeError) {
done(closeError, null);
return;
}

let xmlOutput = '';

results.forEach((chunk) => {
xmlOutput += chunk.OUT151;
processResults(results);
});
done(null, xmlOutput);
});
} else {
processResults(results);
}
});
});
}
Comment on lines +55 to +88
Copy link
Member

@abmusse abmusse Jan 19, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ehenson

I think this would be great to get this merged in. I would like to request some changes before merging this one in though.

Instead of nesting the processResults and query functions maybe this should be split into one method because whether or not an existing connection is passed we would need to query xml service and process the results. The only difference would be if we should close the connection or not. If an existing connection was not passed close it otherwise don't call close.

Checkout this branch with how I think this could be done

Let me know If you have time to add the requested changes so we can fix this up otherwise I can pull these changes from this branch into a new PR and work on getting it merged.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@abmusse
As much as I would like to assist, I cannot get to this any time soon. Sorry!
You are correct about "if we should close the connection or not."
Will the consuming application need to pass the shouldCloseConnection parameter instead of the code checking for a null/undefined connection object?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We could make this a configurable transport option. That way the user could allow closing the connection object if desired. Would this be desirable for your application?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm flexible either way as long as a programmer is fully aware that it will close the connection on their behalf unless the parameter is set appropriately. In my case, I use the same connection to query results of a program call in the QTEMP library, and if it's closed I'll lose the reference to the job.

Copy link
Member

@abmusse abmusse Jan 19, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As written now, the consuming application would not need to pass shouldCloseConnection / is unable to at all. If a odbcConnection option is set/passed by the user shouldCloseConnection is set to false therefore not closing the passed connection. If a connection was not passed then we create one implicitly and run the query, process results, close out that connection.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like it! Thanks!


if (!odbcConnection) {
odbc.connect(connectionString, (connectError, connection) => {
if (connectError) {
done(connectError, null);
return;
}
query(connection);
});
} else {
query(odbcConnection);
}
}

exports.odbcCall = odbcCall;