tediousjs / node-mssql

Microsoft SQL Server client for Node.js
https://tediousjs.github.io/node-mssql
MIT License
2.23k stars 467 forks source link

Allow for `ps.execute()` to not throw a linting error when no parameters are provided and there are no inputs #1457

Closed AlexChadwickP closed 1 year ago

AlexChadwickP commented 1 year ago

Not a bug or issue, but an enhancement.

ps.execute() always requires an object, even when there aren't any inputs.

Suggested behaviour:

When I have a prepared statement (ps), I would like that when I don't give it any inputs, (for example in a simple SELECT * FROM x) to not have to give it an empty object (currently have to call it like this: ps.execute({});.

Having the empty object feels redundant.

Additionally, if this were to be implemented, I'd expect an error to be thrown in the case that inputs were provided but execute was called without any parameters.

Examples

// I assume getPreparedStatement returns a prepared statement
const ps = await getPreparedStatement():

await ps.prepare('SELECT * FROM Example');

// No error or lint warning is thrown
const result = await ps.execute();
const ps = await getPreparedStatement();

ps.input('id', mssql.BigInt);

await ps.prepare('SELECT * FROM Example WHERE ID = @id');

// Throws a similar error to the one you get when you don't provide all the inputs?
const result = await ps.execute();

If maintainers are happy to include this enhancement, I'm willing to give it a shot if someone can guide me a bit through the repository as I haven't contributed code to an open source project before.

Cheers.

dhensby commented 1 year ago

Why would you be using a PS with no parameters?

I don't mind the redundant parameter because it shows intent (the developer has recognised they are intentionally not passing any parameters), however I wouldn't block a PR making it optional.

AlexChadwickP commented 1 year ago

Admittedly it's a bit silly to use a parameterless PS, but in my case I just do it for consistency accross the codebase, all of my data repositories that fetch data from SQL Server just happen to use a prepared statement because everything else (which does have parameters) does, it makes it easier to become familiar with what's going on that little bit quicker.

dhensby commented 1 year ago

Then I think you should always pass an object for said consistency ;)

Using PS for these queries is going to have a performance impact because it's an extra DB request to prepare and then execute the query. It'll also hold onto the connections for longer, meaning your pool is more likely to be depleted.

It's "easier" to use the automatic parameterisation:

// I assume getPool returns a pool
const pool = await getPool():

const id = 1;

// uses tagged template to generate a parameterised query
const result = await pool.query`SELECT * FROM Example WHERE ID = ${id}`;

That will create a parameterised query auto-magically for you (NB: the missing brackets are important)

AlexChadwickP commented 1 year ago

Excellent point Daniel! I hadn't considered this. With that taken into account I think it may be best to close this now then, thanks for explaining that!