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
Show file tree
Hide file tree
Changes from all commits
Commits
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
11 changes: 10 additions & 1 deletion .eslintrc
Original file line number Diff line number Diff line change
@@ -1,5 +1,8 @@
{
"extends": "loopback",
"parserOptions": {
"ecmaVersion": 2017
},
"rules": {
"max-len": ["error", 110, 4, {
"ignoreComments": true,
Expand All @@ -9,6 +12,12 @@
// NOTE(bajtos) we should eventually remove this override
// and fix all of those 100+ violations
"one-var": "off",
"no-unused-expressions": "off"
"no-unused-expressions": "off",
// TODO(bajtos) move this to eslint-config-loopback
"space-before-function-paren": ["error", {
"anonymous": "never",
"named": "never",
"asyncArrow": "always"
}],
}
}
48 changes: 48 additions & 0 deletions lib/datasource.js
Original file line number Diff line number Diff line change
Expand Up @@ -2594,6 +2594,54 @@ DataSource.prototype.ping = function(cb) {
return cb.promise;
};

/**
* Execute an arbitrary command. The commands are connector specific,
* please refer to the documentation of your connector for more details.
*
* @param command String|Object The command to execute, e.g. an SQL query.
* @param [args] Array Parameters values to set in the command.
* @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.)

assert(typeof command === 'string' || typeof command === 'object',
'"command" must be a string or an object.');
assert(typeof args === 'object',
'"args" must be an object, an array or undefined.');
assert(typeof options === 'object',
'"options" must be an object or undefined.');

if (!this.connector) {
return Promise.reject(errorNotImplemented(
`DataSource "${this.name}" is missing a connector to execute the command.`
));
}

if (!this.connector.execute) {
return Promise.reject(new errorNotImplemented(
`The connector "${this.connector.name}" used by dataSource "${this.name}" ` +
'does not implement "execute()" API.'
));
}

return new Promise((resolve, reject) => {
this.connector.execute(command, args, options, onExecuted);
function onExecuted(err, result) {
if (err) return reject(err);
if (arguments.length > 2) {
result = Array.prototype.slice.call(arguments, 1);
}
resolve(result);
}
});

function errorNotImplemented(msg) {
const err = new Error(msg);
err.code = 'NOT_IMPLEMENTED';
return err;
}
};

/*! The hidden property call is too expensive so it is not used that much
*/
/**
Expand Down
96 changes: 96 additions & 0 deletions test/datasource.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -352,4 +352,100 @@ describe('DataSource', function() {
.should.not.containEql('TestModel');
});
});

describe('execute', () => {
let ds;
beforeEach(() => ds = new DataSource('ds', {connector: 'memory'}));

it('calls connnector to execute the command', async () => {
let called = 'not called';
ds.connector.execute = function(command, args, options, callback) {
called = {command, args, options};
callback(null, 'a-result');
};

const result = await ds.execute(
'command',
['arg1', 'arg2'],
{'a-flag': 'a-value'}
);

result.should.be.equal('a-result');
called.should.be.eql({
command: 'command',
args: ['arg1', 'arg2'],
options: {'a-flag': 'a-value'},
});
});

it('supports shorthand version (cmd)', async () => {
let called = 'not called';
ds.connector.execute = function(command, args, options, callback) {
called = {command, args, options};
callback(null, 'a-result');
};

const result = await ds.execute('command');
result.should.be.equal('a-result');
called.should.be.eql({
command: 'command',
args: [],
options: {},
});
});

it('supports shorthand version (cmd, args)', async () => {
let called = 'not called';
ds.connector.execute = function(command, args, options, callback) {
called = {command, args, options};
callback(null, 'a-result');
};

await ds.execute('command', ['arg1', 'arg2']);
called.should.be.eql({
command: 'command',
args: ['arg1', 'arg2'],
options: {},
});
});

it('converts multiple callbacks arguments into a promise resolved with an array', async () => {
ds.connector.execute = function(command, args, options, callback) {
callback(null, 'result1', 'result2');
};
const result = await ds.execute('command');
result.should.eql(['result1', 'result2']);
});

it('allows args as object', async () => {
let called = 'not called';
ds.connector.execute = function(command, args, options, callback) {
called = {command, args, options};
callback();
};

// See https://www.npmjs.com/package/loopback-connector-neo4j-graph
const command = 'MATCH (u:User {email: {email}}) RETURN u';
await ds.execute(command, {email: '[email protected]'});
called.should.be.eql({
command,
args: {email: '[email protected]'},
options: {},
});
});

it('throws NOT_IMPLEMENTED when no connector is provided', () => {
ds.connector = undefined;
return ds.execute('command').should.be.rejectedWith({
code: 'NOT_IMPLEMENTED',
});
});

it('throws NOT_IMPLEMENTED for connectors not implementing execute', () => {
ds.connector.execute = undefined;
return ds.execute('command').should.be.rejectedWith({
code: 'NOT_IMPLEMENTED',
});
});
});
});
7 changes: 7 additions & 0 deletions types/datasource.d.ts
Original file line number Diff line number Diff line change
Expand Up @@ -178,4 +178,11 @@ export declare class DataSource extends EventEmitter {
connect(callback?: Callback): PromiseOrVoid;
disconnect(callback?: Callback): PromiseOrVoid;
ping(callback?: Callback): PromiseOrVoid;

// Only promise variant, callback is intentionally not supported.
execute(
command: string | object,
args?: any[] | object,
options?: Options
): Promise<any>;
}