Skip to content

Change /schemas/tasks API implementation according to new task schema design #619

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

Merged
yyscamper merged 1 commit into from
Apr 25, 2017
Merged

Change /schemas/tasks API implementation according to new task schema design #619

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
35 changes: 18 additions & 17 deletions lib/api/2.0/schemas2.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,9 @@ var controller = injector.get('Http.Services.Swagger').controller;
var schemaApiService = injector.get('Http.Api.Services.Schema');
var nameSpace = '/api/2.0/schemas/';
var Errors = injector.get('Errors');
var taskOptionValidator = injector.get('TaskOption.Validator');
var workflowApiService = injector.get('Http.Services.Api.Workflows');
var Task = injector.get('Task.Task');
var _ = injector.get('_');

// GET /api/2.0/schemas
var schemasGet = controller(function() {
Expand All @@ -26,26 +28,25 @@ var schemasIdGet = controller(function(req) {

// GET /api/2.0/schemas/tasks
var taskSchemasGet = controller(function () {
return taskOptionValidator.getAllSchemaNames({ includeNameSpace: false });
return workflowApiService.getTaskDefinitions()
.then(function(tasks) {
Copy link
Contributor

@brianparry brianparry Apr 4, 2017

Choose a reason for hiding this comment

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

@yyscamper FYI - Bluebird promises have a chaiable map method: http://bluebirdjs.com/docs/api/promise.map.html

Copy link
Contributor Author

@yyscamper yyscamper Apr 5, 2017

Choose a reason for hiding this comment

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

The promise that returned from getTaskDefinition is not bluebird style promise, it is waterline's promise, it doesn't have a map function, so that's the reason I ended up using the array's map.

Now in RackHD code there is various Promise (at least there are Bluebird promise, node.js native promise, waterline promise), we'd better to combine them to a single one in future.

Copy link
Contributor

@brianparry brianparry Apr 5, 2017

Choose a reason for hiding this comment

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

@yyscamper I like to wrap promise returning function calls in Promise.try for this reason. This normalizes promises to Bluebird style. It also will handle any synchronous errors that might be thrown by a promise returning function.

Copy link
Contributor Author

@yyscamper yyscamper Apr 7, 2017

Choose a reason for hiding this comment

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

I like Promise.try as well, it changes a lot of things (synchronous code or erros, non-bluebird promises) into bluebird promise, though a little cumbersome sometimes.

return tasks.map(function(task) {
return task.injectableName;
});
});
});

// GET /api/2.0/schemas/tasks/:identifier
Copy link
Member

@pengz1 pengz1 Mar 30, 2017

Choose a reason for hiding this comment

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

:name should be more accurate than :identifier.

var taskSchemasIdGet = controller(function (req) {
var name = req.swagger.params.identifier.value;
var resolveRef = req.swagger.params.resolveRef.value;
var schema;
if (resolveRef) {
schema = taskOptionValidator.getSchemaResolved(name);
}
else {
schema = taskOptionValidator.getSchema(name);
}

if (schema) {
return schema;
}

throw new Errors.NotFoundError(name + ' Not Found');
var name = req.swagger.params.identifier.value;
return workflowApiService.getWorkflowsTasksByName(name)
.then(function(taskDefinitions) {
if (_.isEmpty(taskDefinitions)) {
throw new Errors.NotFoundError(
'Unable to find the task defintion with injectablenName ' + name);
}
return Task.getFullSchema(taskDefinitions[0]);
});
});

module.exports = {
Expand Down
128 changes: 49 additions & 79 deletions spec/lib/api/2.0/schemas-spec.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,36 +5,33 @@

describe('Http.Api.Schemas', function () {
var schemaService;
var stubGetNamespace;
var stubGetSchema;
var taskOptionValidator;
var workflowApiService;
var Task;
var sandbox;

before('start HTTP server', function () {
this.timeout(10000);
sandbox = sinon.sandbox.create();

return helper.startServer([]).then(function () {
schemaService = helper.injector.get('Http.Api.Services.Schema');
stubGetNamespace = sinon.stub(schemaService, "getNamespace");
stubGetSchema = sinon.stub(schemaService, "getSchema");
taskOptionValidator = helper.injector.get('TaskOption.Validator');
workflowApiService = helper.injector.get('Http.Services.Api.Workflows');
Task = helper.injector.get('Task.Task');
});
});

afterEach("reset stubs", function() {
stubGetNamespace.reset();
stubGetSchema.reset();
sandbox.restore();
});

after('stop HTTP server', function () {
schemaService.getSchema.restore();
schemaService.getNamespace.restore();
return helper.stopServer();
});

describe("GET /schemas", function() {

it("should a list of all catalogs", function() {

stubGetNamespace.returns([
it("should a list of all schemas", function() {
sandbox.stub(schemaService, "getNamespace").returns([
"schema1",
"schema2"
]);
Expand All @@ -45,13 +42,12 @@ describe('Http.Api.Schemas', function () {
.expect(function (res) {
expect(res.body).to.be.an("Array").with.length(2);
expect(res.body[0]).to.equal("schema1");
expect(stubGetNamespace).to.be.called.once;
expect(schemaService.getNamespace).to.be.called.once;
});
});

it("should return an empty array if no schemas exist", function() {

stubGetNamespace.returns([]);
sandbox.stub(schemaService, "getNamespace").returns([]);

return helper.request().get('/api/2.0/schemas')
.expect('Content-Type', /^application\/json/)
Expand All @@ -65,8 +61,7 @@ describe('Http.Api.Schemas', function () {
describe("GET /schemas/:identifier", function() {

it("should return an individual schema", function() {

stubGetSchema.returns({
sandbox.stub(schemaService, "getSchema").returns({
title: 'schema',
description: 'a schema',
type: 'object',
Expand All @@ -79,13 +74,12 @@ describe('Http.Api.Schemas', function () {
.expect(function (res) {
expect(res.body).to.have.property("title", "schema");
expect(res.body).to.be.an("Object").with.property('description', "a schema");
expect(stubGetSchema).to.be.called.once;
expect(schemaService.getSchema).to.be.called.once;
});
});

it("should return a 404 if no schema can be found", function() {

stubGetSchema.returns(undefined);
sandbox.stub(schemaService, "getSchema").returns(undefined);

return helper.request().get('/api/2.0/schemas/junk')
.expect('Content-Type', /^application\/json/)
Expand All @@ -94,40 +88,36 @@ describe('Http.Api.Schemas', function () {
});

describe("GET /schemas/tasks", function() {
before(function () {
sinon.stub(taskOptionValidator, 'getAllSchemaNames');
});

beforeEach(function () {
taskOptionValidator.getAllSchemaNames.reset();
});

after(function () {
taskOptionValidator.getAllSchemaNames.restore();
});

it("should return a list of all task schemas", function() {

taskOptionValidator.getAllSchemaNames.returns([
"tasks/schema1",
"tasks/schema2"
it("should return a list of all task schemas' name", function() {
sandbox.stub(workflowApiService, 'getTaskDefinitions').resolves([
{
injectableName: 'Task.foo',
friendlyName: 'foo',
options: {
a: 1
}
},
{
injectableName: 'Task.bar',
friendlyName: 'bar',
options: {
b: 2
}
}
]);

return helper.request().get('/api/2.0/schemas/tasks')
.expect('Content-Type', /^application\/json/)
.expect(200)
.expect(function (res) {
expect(res.body).to.be.an("Array").with.length(2);
expect(res.body[0]).to.equal("tasks/schema1");
expect(res.body[1]).to.equal("tasks/schema2");
expect(taskOptionValidator.getAllSchemaNames).to.be.called.once;
expect(res.body[0]).to.equal("Task.foo");
expect(res.body[1]).to.equal("Task.bar");
});
});

it("should return an empty array if no schemas exist", function() {

taskOptionValidator.getAllSchemaNames.returns([]);

sandbox.stub(workflowApiService, 'getTaskDefinitions').resolves([]);
return helper.request().get('/api/2.0/schemas/tasks')
.expect('Content-Type', /^application\/json/)
.expect(200)
Expand All @@ -138,6 +128,14 @@ describe('Http.Api.Schemas', function () {
});

describe("GET /schemas/tasks/:identifier", function() {
var task = {
injectableName: 'Task.foo',
friendlyName: 'foo',
options: {
a: 1
}
};

var testSchema = {
title: 'schema',
description: 'a test schema',
Expand All @@ -147,51 +145,23 @@ describe('Http.Api.Schemas', function () {
}
};

before(function () {
sinon.stub(taskOptionValidator, 'getSchema');
sinon.stub(taskOptionValidator, 'getSchemaResolved');
});

beforeEach(function () {
taskOptionValidator.getSchema.reset();
taskOptionValidator.getSchemaResolved.reset();
});

after(function () {
taskOptionValidator.getSchema.restore();
taskOptionValidator.getSchemaResolved.restore();
});

it("should return a task schema", function() {
taskOptionValidator.getSchema.returns(testSchema);

return helper.request().get('/api/2.0/schemas/tasks/testSchema')
.expect('Content-Type', /^application\/json/)
.expect(200)
.expect(function (res) {
expect(taskOptionValidator.getSchema).to.be.called.once;
expect(taskOptionValidator.getSchemaResolved).to.not.be.called;
expect(res.body).to.deep.equals(testSchema);
});
});
sandbox.stub(workflowApiService, 'getWorkflowsTasksByName')
.withArgs('Task.foo').resolves([task]);
sandbox.stub(Task, 'getFullSchema')
.withArgs(task).returns(testSchema);

it("should return a task schema with reference resolved", function() {
taskOptionValidator.getSchemaResolved.returns(testSchema);

return helper.request()
.get('/api/2.0/schemas/tasks/testSchema?resolveRef=true')
return helper.request().get('/api/2.0/schemas/tasks/Task.foo')
.expect('Content-Type', /^application\/json/)
.expect(200)
.expect(function (res) {
expect(taskOptionValidator.getSchemaResolved).to.be.called.once;
expect(taskOptionValidator.getSchema).to.not.be.called;
expect(res.body).to.deep.equals(testSchema);
});
});

it("should return a 404 if no schema can be found", function() {
taskOptionValidator.getSchema.returns();

sandbox.stub(workflowApiService, 'getWorkflowsTasksByName')
.withArgs('junk').resolves([]);
return helper.request().get('/api/2.0/schemas/tasks/junk')
.expect('Content-Type', /^application\/json/)
.expect(404);
Expand Down
11 changes: 1 addition & 10 deletions static/monorail-2.0.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3001,20 +3001,11 @@ paths:
description: Get the specified task schema.
operationId: taskSchemasIdGet
parameters:
- description: The filename of the task schema
- description: The name of task schema
in: path
name: identifier
required: true
type: string
- description: >
The reference resolve flag.
false - a simple schema with any internal reference only in the response.
true - all the internal references will be resolved and fully defined in
the response.
in: query
name: resolveRef
required: false
type: boolean
responses:
200:
description: Successfully retrieved the task schema
Expand Down