Skip to content
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

feat: dataSource.execute(cmd, args, opts) #1671

Merged
merged 2 commits into from
Dec 7, 2018
Merged

feat: dataSource.execute(cmd, args, opts) #1671

merged 2 commits into from
Dec 7, 2018

Conversation

bajtos
Copy link
Member

@bajtos bajtos commented Dec 6, 2018

Implement a new helper API for calling connector's "execute" method
in a promise-friendly way.

Example usage:

await MyModel.dataSource.execute(
  'SELECT * FROM Users WHERE email = ?',
  ['[email protected]'],
  {/* options */});

While it's possible to call MyModel.dataSource.execute directly, this new API adds promise support to existing callback-based connectors. When we upgrade connector API to use promises under the hood, consumers of this new API won't be affected by that change, as opposed to consumers of connector.execute that will have to upgrade their code.

/cc @koalabi

Related issues

Checklist

  • New tests added or existing tests modified to cover all changes
  • Code conforms with the style
    guide

@bajtos
Copy link
Member Author

bajtos commented Dec 6, 2018

A related question on StackOverflow: How to execute arbitrary SQL query

lib/datasource.js Outdated Show resolved Hide resolved
lib/datasource.js Outdated Show resolved Hide resolved
@bajtos
Copy link
Member Author

bajtos commented Dec 6, 2018

Added a new commit to remove callback style and keep Promise style only.

Copy link
Contributor

@jannyHou jannyHou left a comment

Choose a reason for hiding this comment

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

@bajtos Mostly LGTM 👏 , I left a concern about the command's type.

For the types in the callback flavour, I am ok to remove them.

test/datasource.test.js Outdated Show resolved Hide resolved
lib/datasource.js Outdated Show resolved Hide resolved
* @param [options] Object Additional options, e.g. the transaction to use.
* @returns Promise A promise of the result
*/
DataSource.prototype.execute = function(command, args = [], options = {}) {
Copy link
Contributor

Choose a reason for hiding this comment

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

@bajtos I found the command type here a bit confusing: the function types defined at the bottom of this PR only takes string type as command. Any particular reason to allow an object?

Copy link
Member Author

@bajtos bajtos Dec 7, 2018

Choose a reason for hiding this comment

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

the function types defined at the bottom of this PR only takes string type as command.

I forgot to update typedefs, they were describing a callback variant too. I'll fix that.

Copy link
Member Author

Choose a reason for hiding this comment

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

Any particular reason to allow an object?

Some community connectors are using object value for the comman and I want to preserve support for those connectors.

See e.g. https://www.npmjs.com/package/loopback-connector-neo4j-graph

var cypher = {
    query: "MATCH (u:User {email: {email}}) RETURN u",
    params: {
        email: '[email protected]',
    }
};
// ...
ModelClass.dataSource.connector.execute(
    cypher,
    [],
    options,
    callback
);

I think the connector authors misunderstood how command & args are meant to be used together. I think the following would be a better usage:

ModelClass.dataSource.connector.execute(
  `MATCH (u:User {email: {email}}) RETURN u`,
   {email: '[email protected]'},
   options,
   callback);

Based on this analysis, I am going to change the type of args from array to object. (Note that arrays are objects too.)

@bajtos
Copy link
Member Author

bajtos commented Dec 7, 2018

I have addressed the review comments and made the following extra changes:

  • Enabled ES2017 (async functions) in eslint config
  • Rewrote new tests using async/await. This is ok because juggler v4 requires Node.js 8+

@jannyHou LGTY now?

Implement a new helper API for calling connector's "execute" method
in a promise-friendly way.
Copy link
Contributor

@jannyHou jannyHou left a comment

Choose a reason for hiding this comment

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

:shipit: LGTM

@bajtos bajtos merged commit 0cf0fa9 into master Dec 7, 2018
@bajtos bajtos deleted the feat/execute branch December 7, 2018 15:21
@bajtos bajtos changed the title feat: dataSource.execute(cmd, args, opts, cb) feat: dataSource.execute(cmd, args, opts) Dec 7, 2018
@dougal83
Copy link

Is this supported for mongodb connector?

@jannyHou
Copy link
Contributor

@dougal83 I believe so, mongodb has its execute implementation.

@dougal83
Copy link

@jannyHou Do you know if there are any usage examples?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants