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.

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

Conversation

ehenson
Copy link

@ehenson ehenson commented May 8, 2020

By setting the odbcConnection parameter of the transportOptions it will use the existing connection instead of creating and managing its own connection. This is useful to be able to run SQL queries in the same job. An example would be if a program created files in the QTEMP library and you need to retrieve the records.

@ehenson ehenson changed the title The ability to use an existing odbc connection feat: The ability to use an existing odbc connection May 8, 2020
@abmusse abmusse requested review from abmusse and kadler May 11, 2020 17:26
Copy link
Member

@abmusse abmusse left a comment

Choose a reason for hiding this comment

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

@ehenson

This looks like it will work. I think the next step is to add some functional tests ensuring it works. Can you show how you create the existing odbc connection that gets passed to the transport?

I see that this your first time contributing, to accept your patches we ask that you please sign-off your commits. This can be done with the commands suggsted by the DCO bot.

Thanks for your contribution!

@ehenson
Copy link
Author

ehenson commented May 11, 2020

In the initialization I am creating the pool:

this.odbcPool = await odbc.pool('DSN=<dsn name>');

I obtain a connection:

this._odbcConnection = await this.odbcPool.connect();

finally, I pass it to the itoolkit Connection:

    const connection = new Connection({
      transport: 'odbc',
      transportOptions: {
        odbcConnection: this._odbcConnection,
      },
    });

@ehenson
Copy link
Author

ehenson commented May 11, 2020

My apologies for the lack of signOffs. I have realized that I have forgotten about the contributors document and I have now configured my tools for signOff and gpg.

@github-actions
Copy link

👋 Hi! This pull request has been marked stale due to inactivity. If no further activity occurs, it will automatically be closed.

@github-actions github-actions bot added the stale label Jun 11, 2020
@abmusse abmusse added the keep-open Exempts stale action from auto closing the issue/pr. label Jun 11, 2020
@abmusse abmusse self-requested a review June 30, 2020 16:43
Copy link
Member

@abmusse abmusse left a comment

Choose a reason for hiding this comment

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

@ehenson

Sorry for the late reply, Looks the commits 5cd87d9 34998ba 6de2075 12b6263 were accidently signed off on. These commits should not be apart of this PR. You can clean things up by running.

$ git rebase -i HEAD~8

Then prefixing the commits with d as shown below:

image

Lastly force push up the changes to your branch

ehenson added 4 commits July 1, 2020 08:03
Signed-off-by: Eric Henson <ehenson238@gmail.com>
Signed-off-by: Eric Henson <ehenson238@gmail.com>
Signed-off-by: Eric Henson <ehenson238@gmail.com>
Signed-off-by: Eric Henson <ehenson238@gmail.com>
@github-actions github-actions bot closed this Oct 6, 2020
@kadler kadler removed the stale label Oct 6, 2020
@kadler kadler reopened this Oct 6, 2020
@kadler kadler linked an issue Oct 21, 2020 that may be closed by this pull request
Comment on lines +55 to +88
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);
}
});
});
}
Copy link
Member

@abmusse abmusse Jan 19, 2021

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

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!

@@ -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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
keep-open Exempts stale action from auto closing the issue/pr.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Is the SQL feature going to be removed from the toolkit?
4 participants