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

Add contexts api #500

Closed
wants to merge 3 commits into from
Closed
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
2 changes: 2 additions & 0 deletions src/app.js
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@ const testsRouter = require('./tests/routes/testsRoute.js');
const processorsRouter = require('./processors/routes/processorsRoute.js');
const filesRouter = require('./files/routes/filesRoute.js');
const webhooksRouter = require('./webhooks/routes/webhooksRouter');
const contextsRouter = require('./contexts/routes/contextsRoute.js');
const swaggerValidator = require('express-ajv-swagger-validation');

const audit = require('express-requests-logger');
Expand Down Expand Up @@ -68,6 +69,7 @@ module.exports = async () => {
app.use('/v1/processors', processorsRouter);
app.use('/v1/files', filesRouter);
app.use('/v1/webhooks', webhooksRouter);
app.use('/v1/contexts', contextsRouter);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Please add this api spec to

  1. predator/docs/devguide/docs/swagger-docs.yaml
  2. predator/docs/openapi3.yaml


app.use('/', function (req, res, next) {
res.redirect('/ui');
Expand Down
45 changes: 45 additions & 0 deletions src/contexts/controllers/contextsController.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,45 @@
'use strict';
const contextManager = require('../models/contextManager');

module.exports = {
createContext,
getContexts,
deleteContext
};

async function getContexts(_, res, next) {
let contextData;
try {
contextData = await contextManager.getContexts();
} catch (err) {
return next(err);
}

return res.json(contextData);
}

async function createContext(req, res, next) {
let contextId;
try {
contextId = await contextManager.saveContext(req.body.name);
} catch (err) {
return next(err);
}

return res.status(201).json({ id: contextId, name: req.body.name });
}

async function deleteContext(req, res, next) {
let deleteResult;
try {
deleteResult = await contextManager.deleteContext(req.body.id);
} catch (err) {
return next(err);
}

if (!deleteResult) {
res.status(204).json({ message: 'Context not found' });
Copy link
Collaborator

Choose a reason for hiding this comment

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

Hi!, we usually return 404 when context not found, and 204 if it's found and deleted

}

return res.status(202).json({ message: 'Context successfully deleted' });
}
50 changes: 50 additions & 0 deletions src/contexts/models/contextManager.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,50 @@
'use strict';
const uuid = require('uuid');

const database = require('./database/sequelize/sequelizeConnector'),
{ ERROR_MESSAGES } = require('../../common/consts');

module.exports = {
getContexts,
saveContext,
deleteContext
};

async function getContexts() {
const contexts = await database.getContexts();
if (contexts) {
return contexts.map(({ dataValues }) => ({
id: dataValues.id,
name: dataValues.name
}));
} else {
Copy link
Collaborator

Choose a reason for hiding this comment

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

It's multiple get, so it's ok to return empty array instead 404

const error = new Error(ERROR_MESSAGES.NOT_FOUND);
error.statusCode = 404;
throw error;
}
}

async function saveContext(contextName) {
const id = uuid();
try {
await database.saveContext(id, contextName);
} catch (err) {
const error = new Error(err.message);
Copy link
Collaborator

Choose a reason for hiding this comment

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

we don't expect any 400 here, the open API validator needs to verify the correctness of the input data, so no need for try-catch

error.statusCode = 400;
throw error;
}
return id;
}

async function deleteContext(contextId) {
let deleteResult;
try {
deleteResult = await database.deleteContext(contextId);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Same as above (no need try-catch)

Copy link
Collaborator

Choose a reason for hiding this comment

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

if context not found, better to throw here a 404 exception (then no need to catch it specifically in the controller)

} catch (err) {
const error = new Error(err.message);
error.statusCode = 400;
throw error;
}

return deleteResult;
}
64 changes: 64 additions & 0 deletions src/contexts/models/database/sequelize/sequelizeConnector.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,64 @@

const Sequelize = require('sequelize');
module.exports = {
init,
saveContext,
getContexts,
deleteContext
};

let client;

async function init(sequlizeClient) {
client = sequlizeClient;
await initSchemas();
}

async function initSchemas() {
const context = client.define('context', {
id: {
type: Sequelize.DataTypes.UUID,
primaryKey: true
},
name: {
type: Sequelize.DataTypes.STRING,
allowNull: false,
unique: true
}
});
await context.sync();
}

async function saveContext(id, contextName) {
const contextClient = client.model('context');

const duplicate = await contextClient.findOne({ where: { name: contextName } });
if (duplicate) {
throw Error('Duplicate context');
Copy link
Collaborator

@enudler enudler Oct 18, 2020

Choose a reason for hiding this comment

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

need to throw here an error with 409 status code.
also need to verify that context_id doesn't exist.

Copy link
Member

@NivLipetz NivLipetz Oct 18, 2020

Choose a reason for hiding this comment

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

you defined name as unique: true in the sequelize data spec, so this if might be unnecessary and sequelize would throw an error in the contextClient.create() function if it were a duplicated name.

The error thrown should be there with status code 409

}

const params = {
id: id,
name: contextName
};

const result = contextClient.create(params);
return result;
}

async function deleteContext(id) {
const contextClient = client.model('context');

const result = contextClient.destroy({ where: { id: id } });
return result;
}

async function getContexts() {
const contextClient = client.model('context');
const options = {
attributes: { exclude: ['updated_at', 'created_at'] }
Copy link
Collaborator

Choose a reason for hiding this comment

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

created_at and updated_at not exists in this table
maybe it's a good idea to add create_at to this table

};

const dbResult = await contextClient.findAll(options);
return dbResult;
}
12 changes: 12 additions & 0 deletions src/contexts/routes/contextsRoute.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,12 @@
'use strict';

const express = require('express');
const router = express.Router();
const contextsController = require('../controllers/contextsController');
const swaggerValidator = require('express-ajv-swagger-validation');

router.post('/', swaggerValidator.validate, contextsController.createContext);
router.get('/', swaggerValidator.validate, contextsController.getContexts);
router.delete('/', swaggerValidator.validate, contextsController.deleteContext);

module.exports = router;
2 changes: 2 additions & 0 deletions src/database/sequlize-handler/sequlize.js
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ const testsSequlizeConnector = require('../../tests/models/database/sequelize/se
const configSequlizeConnector = require('../../configManager/models/database/sequelize/sequelizeConnector');
const processorsSequlizeConnector = require('../../processors/models/database/sequelize/sequelizeConnector');
const fileSequlizeConnector = require('../../files/models/database/sequelize/sequelizeConnector');
const contextSequlizeConnector = require('../../contexts/models/database/sequelize/sequelizeConnector');
const logger = require('../../../src/common/logger');
const databaseConfig = require('../../config/databaseConfig');
const webhooksSequlizeConnector = require('../../webhooks/models/database/sequelize/sequelizeConnector');
Expand All @@ -23,6 +24,7 @@ module.exports.init = async () => {
await configSequlizeConnector.init(sequlizeClient);
await processorsSequlizeConnector.init(sequlizeClient);
await fileSequlizeConnector.init(sequlizeClient);
await contextSequlizeConnector.init(sequlizeClient);
await runSequlizeMigrations();
await sequlizeClient.sync();
};
Expand Down
57 changes: 57 additions & 0 deletions tests/integration-tests/contexts/contexts-test.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,57 @@

Copy link
Collaborator

Choose a reason for hiding this comment

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

I think there is a need to add unit tests as well to pass coverage threshold

const { expect } = require('chai');

const contextRequestSender = require('./helpers/requestCreator');

describe('Contexts api', function () {
this.timeout(5000000);
before(async function () {
await contextRequestSender.init();
});

describe('Good requests', async function () {
describe('GET /v1/contexts', async function () {
before('clean contexts', async function() {
const contextResponse = await contextRequestSender.getContexts();
await Promise.all(contextResponse.body.map(({ id }) => contextRequestSender.deleteContext({ id: id })));
});
const numOfContextsToInsert = 5;
it(`return ${numOfContextsToInsert} contexts`, async function() {
const contextsToInsert = (new Array(numOfContextsToInsert))
.fill(0, 0, numOfContextsToInsert)
.map((_, idx) => ({ name: String(idx) }));
await Promise.all(contextsToInsert.map(context => contextRequestSender.createContext(context)));

const contextGetResponse = await contextRequestSender.getContexts();
expect(contextGetResponse.statusCode).to.equal(200);

const contexts = contextGetResponse.body;
expect(contexts).to.be.an('array').and.have.lengthOf(numOfContextsToInsert);
});
});
describe('POST /v1/contexts', function () {
it('Create context and response 201 status code', async function() {
const createContextResponse = await contextRequestSender.createContext({ name: 'foo' });
expect(createContextResponse.statusCode).to.equal(201);
expect(createContextResponse.body.name).to.equal('foo');
});
});
});

describe('Bad requests', function () {
describe('POST /v1/contexts', function () {
describe('name validation', function() {
before('clean contexts', async function() {
const contextResponse = await contextRequestSender.getContexts();
await Promise.all(contextResponse.body.map(({ id }) => contextRequestSender.deleteContext({ id: id })));
});
it('invalidates name duplicates', async function () {
const uniqueName = { name: 'bob' };
await contextRequestSender.createContext(uniqueName);
const duplicateNameResponse = await contextRequestSender.createContext(uniqueName);
expect(duplicateNameResponse.statusCode).to.equal(400);
});
});
});
});
});
52 changes: 52 additions & 0 deletions tests/integration-tests/contexts/helpers/requestCreator.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,52 @@

const request = require('supertest');
const expressApp = require('../../../../src/app');

let app;
const headers = { 'Content-Type': 'application/json' };
const resourceUri = '/v1/contexts';

module.exports = {
init,
createContext,
getContexts,
deleteContext
};

async function init() {
try {
app = await expressApp();
} catch (err){
console.log(err);
process.exit(1);
}
}

function createContext(body) {
return request(app)
.post(resourceUri)
.send(body)
.set(headers)
.expect(function(res){
return res;
});
}

function getContexts() {
return request(app)
.get(resourceUri)
.set(headers)
.expect(function (res) {
return res;
});
}

function deleteContext(body) {
return request(app)
.delete(resourceUri)
.send(body)
.set(headers)
.expect(function (res) {
return res;
});
}