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.

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

Validation module #109

Merged
merged 5 commits into from
Dec 27, 2018
Merged

Validation module #109

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
3028ee5
Adds validation module
mukkachaitanya Dec 17, 2018
b90abc3
Add validate module to encapsulate validations
mukkachaitanya Dec 19, 2018
e2b6b3f
Decouple the modules and refactor validation
mukkachaitanya Dec 22, 2018
1961af9
Adds tests for validation module
mukkachaitanya Dec 26, 2018
a0ccfc5
Minor spelling correction
mukkachaitanya Dec 27, 2018
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
21 changes: 10 additions & 11 deletions lib/cli/input/eval.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -49,30 +49,29 @@ const getSubmitInfo = () => {
return inquirer.prompt(submitInfo);
};

const getIdNo = (options) => {
const getIdNo = () => {
logger.moduleLog('debug', 'Eval Input', 'Fetching id to submit.');
const { username } = preferenceManager.getPreference({ name: 'gitLabPrefs' });
if (username === 'root') {
logger.moduleLog('debug', 'Eval Input', `Root user detected, passing up the ID provided : ${options.i}.`);
return options.i || '';
}
return username;
};

const isValidOptions = options => options.l && options.lang
&& supportedLanguages.indexOf(options.lang) !== -1;

const getInput = async (args, options) => {
let evalOptions = {};
if (!isValidOptions(options)) {
if (!options.l || !options.lang) {
evalOptions = await getSubmitInfo();
} else {
evalOptions.lab = options.l;
evalOptions.lang = options.lang;
evalOptions.i = options.i;
evalOptions.commitHash = options.hash || '';
}
evalOptions.idNo = getIdNo(options);
return evalOptions;
evalOptions.idNo = getIdNo();
return {
name: 'evaluate',
details: {
...evalOptions,
},
};
};

module.exports = {
Expand Down
76 changes: 12 additions & 64 deletions lib/cli/input/prefs.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,50 +1,21 @@
const inquirer = require('inquirer');
const path = require('path');

const defaultPrefPath = path.join(__dirname, '../../../default-prefs.json');
const defaultPrefs = JSON.parse(require('fs').readFileSync(defaultPrefPath, 'utf8'));

const { supportedLanguages } = defaultPrefs;
const { logger } = require('../../utils/logger');
const prefPrompts = require('./prefsprompt');

const changeLangFlag = (lang) => {
if (supportedLanguages.indexOf(lang) === -1) {
logger.moduleLog('debug', 'Prefs Input', `Invalid language for changelang command ${lang}`);
return {
name: 'invalid_lang',
details: {
supportedLanguages,
},
};
const changeLang = async (options) => {
if (!options.lang) {
return prefPrompts.changeLangPrompt();
}
return {
name: 'lang_changed',
details: {
lang,
lang: options.lang,
},
};
};

const changeLang = async (options) => {
if (options.lang) {
return changeLangFlag(options.lang);
}
return prefPrompts.changeLangPrompt();
};

const changeMainServerFlag = (host, port) => {
if (!prefPrompts.hostValidator(host, true)) {
logger.moduleLog('debug', 'Prefs Input', `Invalid mainserver host ${host}`);
return {
name: 'invalid_host',
};
}
if (!prefPrompts.portValidator(port, true)) {
logger.moduleLog('debug', 'Prefs Input', `Invalid mainserver port ${host}`);
return {
name: 'invalid_port',
};
const changeMainServer = (host, port) => {
if (!host || !port) {
return prefPrompts.changeMainServerPrompt();
}
return {
name: 'server_changed',
Expand All @@ -56,12 +27,9 @@ const changeMainServerFlag = (host, port) => {
};
};

const changeGitlabFlag = (host) => {
if (!prefPrompts.hostValidator(host, true)) {
logger.moduleLog('debug', 'Prefs Input', `Invalid Gitlab host ${host}`);
return {
name: 'invalid_host',
};
const changeGitlab = (host) => {
if (!host) {
return prefPrompts.changeGitlabPrompt();
}
return {
name: 'server_changed',
Expand All @@ -72,20 +40,6 @@ const changeGitlabFlag = (host) => {
};
};

const changeMainServer = (host, port) => {
if (!host || !port) {
return prefPrompts.changeMainServerPrompt();
}
return changeMainServerFlag(host, port);
};

const changeGitlab = (host) => {
if (!host) {
return prefPrompts.changeGitlabPrompt();
}
return changeGitlabFlag(host);
};

const changeServer = async (options) => {
const { host, port } = options;
let { type } = options;
Expand All @@ -99,9 +53,9 @@ const changeServer = async (options) => {
return changeGitlab(host);
}
return {
name: 'invalid_server',
name: 'server_changed',
details: {
supportedServers: ['ms', 'gitlab'],
type, host, port,
},
};
};
Expand All @@ -113,12 +67,6 @@ const changeLogger = async (options) => {
return prefPrompts.changeLoggerPrompt();
}

if (blacklist && !prefPrompts.keywordValidator(blacklist, true)) {
return {
name: 'invalid_blacklist_keyword',
};
}

return {
name: 'logger_pref_changed',
details: {
Expand Down
33 changes: 3 additions & 30 deletions lib/cli/input/prefsprompt.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,9 +2,7 @@ const inquirer = require('inquirer');
const path = require('path');
const fs = require('fs');

const validator = require('validator');
const PromptGenerator = require('../../utils/PromptGenerator');
const preferenceManager = require('../../utils/preference-manager');

const defaultPrefPath = path.join(__dirname, '../../../default-prefs.json');
const defaultPrefs = JSON.parse(fs.readFileSync(defaultPrefPath, 'utf8'));
Expand All @@ -27,28 +25,6 @@ const changeLangPrompt = async () => {
};
};

const hostValidator = (host, flags) => {
if (validator.isFQDN(host) || validator.isIP(host)) {
return true;
}
return flags === true ? false : 'Enter a valid hostname or IP';
};

const portValidator = (port, flags) => {
if (validator.isPort(port)) {
return true;
}
return flags === true ? false : 'Enter a valid port';
};

const keywordValidator = (keyword, flags) => {
const { blacklist } = preferenceManager.getPreference({ name: 'cliPrefs' }).logger;
if (!blacklist.includes(keyword)) {
return true;
}
return flags === true ? false : 'Keyword already exists. Enter any other keyword';
};

const getPromptGenerator = (name, type, message, validate) => {
const promptGenerator = new PromptGenerator();
promptGenerator.addProperty('name', name);
Expand All @@ -65,12 +41,12 @@ const getTypePromptGenerator = (name, type, message, validate, choices) => {
};

const getHostPrompt = () => {
const hostPromptGenerator = getPromptGenerator('host', 'input', 'Enter the host:', hostValidator);
const hostPromptGenerator = getPromptGenerator('host', 'input', 'Enter the host:');
return hostPromptGenerator.getPrompt();
};

const getPortPrompt = () => {
const portPromptGenerator = getPromptGenerator('port', 'input', 'Enter the port:', portValidator);
const portPromptGenerator = getPromptGenerator('port', 'input', 'Enter the port:');
return portPromptGenerator.getPrompt();
};

Expand Down Expand Up @@ -133,7 +109,7 @@ const changeMaxSize = async () => {
};

const changeBlacklist = async () => {
const blacklistPromptGenrator = getPromptGenerator('keyword', 'input', 'Enter the keyword to be added to logger blacklist:', keywordValidator);
const blacklistPromptGenrator = getPromptGenerator('keyword', 'input', 'Enter the keyword to be added to logger blacklist:');
const blacklistPrefs = await inquirer.prompt(blacklistPromptGenrator.getPrompt());

return {
Expand All @@ -159,9 +135,6 @@ const changeLoggerPrompt = async () => {

module.exports = {
changeLangPrompt,
hostValidator,
portValidator,
keywordValidator,
changeMainServerPrompt,
changeGitlabPrompt,
getServerTypePrompt,
Expand Down
4 changes: 4 additions & 0 deletions lib/cli/output/eval.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -75,6 +75,10 @@ const sendOutput = (event) => {
stopSpinner();
onScores(event.details);
break;
case 'invalid_lang':
stopSpinner();
console.log(chalk.red(`\nPlease provide the a valid language. The supported languages are ${event.details.supportedLanguages}`));
break;
default:
console.log(chalk.red('\nInvalid Event'));
}
Expand Down
6 changes: 3 additions & 3 deletions lib/controller/eval.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@ const evalInput = require('../cli/input/eval');
const evalOutput = require('../cli/output/eval');
const evalModel = require('../model/eval');
const commandValidator = require('../utils/command-validator');

const evalValidate = require('./validate/eval');

const onEvalResult = (evalResult) => {
evalOutput.sendOutput(evalResult);
Expand All @@ -14,13 +14,13 @@ const onEval = async (args, options, logger) => {
return;
}
const evalOptions = await evalInput.getInput(args, options);

const validatedOptions = evalValidate.validate(evalOptions);
logger.moduleLog('debug', 'Eval', 'Evaluate request for');
logger.moduleLog('debug', 'Eval', evalOptions);
evalOutput.sendOutput({
name: 'eval_started',
});
evalModel.evaluate(evalOptions, onEvalResult);
evalModel.evaluate(validatedOptions, onEvalResult);
};

const addTo = (program) => {
Expand Down
5 changes: 5 additions & 0 deletions lib/controller/prefs.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,10 +2,15 @@ const prefsInput = require('../cli/input/prefs');
const prefsOutput = require('../cli/output/prefs');
const prefsModel = require('../model/prefs');
Copy link
Member

Choose a reason for hiding this comment

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

Is prefs the only command to suffer from the validation problem? What about other commands. Please check again.

Copy link
Member

Choose a reason for hiding this comment

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

Top of my head, I can forsee the following invalid inputs.

  • Invalid hostname and or port
  • Invalid login credentials - deduced without even contacting the gitlab. For example, usernames such as 123 are obviously not valid. We can look at the validity of usernames for GitLab and repeat the same logic here.

We need validations for the above scenarios too.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Sir since we have a dual nature of input, these validations can only be done after the input it taken from the stdin when necessary. This happens in the input/prefs module.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Is prefs the only command to suffer from the validation problem? What about other commands. Please check again.

For now only prefs has the nested command model which is not taken care by caporal. No other commands have this issue right now. I made the validation module to be extensible, so can be extended if need arises in any of the other modules.

Copy link
Member

Choose a reason for hiding this comment

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

We discussed about the validation functions feature provided by caporal. Is it better to leave the validation functions inside the controller modules or to have controllers/validators/<controller-name.js>?
Putting the non-trivial validators in controllers/validators/<controller-name.js> may improve the code design.

Copy link
Member

Choose a reason for hiding this comment

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

BTW, the nested commands are all lined up in the TODO feature list. So, the validators.js is not just one single file; we might need a lot of them.


const prefsValidate = require('./validate/prefs');

const onPrefs = async (args, options, logger) => {
logger.moduleLog('info', 'Prefs', 'Prefs command invoked for');
logger.moduleLog('info', 'Prefs', args.preference);
let changePrefEvent = await prefsInput.getInput(args, options);

changePrefEvent = prefsValidate.validate(changePrefEvent);

logger.moduleLog('debug', 'Prefs', 'Event for the prefs command called');
logger.moduleLog('debug', 'Prefs', changePrefEvent);
changePrefEvent = await prefsModel.storePrefs(changePrefEvent);
Expand Down
42 changes: 42 additions & 0 deletions lib/controller/validate/eval.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,42 @@
const fs = require('fs');
const path = require('path');

const defaultPrefPath = path.join(__dirname, '../../../default-prefs.json');
const defaultPrefs = JSON.parse(fs.readFileSync(defaultPrefPath, 'utf8'));

const { supportedLanguages } = defaultPrefs;

const validate = (evalEvent) => {
if (supportedLanguages.indexOf(evalEvent.details.lang) === -1) {
return {
name: 'invalid_lang',
details: {
supportedLanguages,
},
};
}
if (evalEvent.details.idNo === 'root') {
return {
name: 'evaluate',
details: {
idNo: evalEvent.details.i,
lab: evalEvent.details.lab,
lang: evalEvent.details.lang,
commitHash: evalEvent.details.commitHash,
},
};
}
return {
name: 'evaluate',
details: {
idNo: evalEvent.details.idNo,
lab: evalEvent.details.lab,
lang: evalEvent.details.lang,
commitHash: evalEvent.details.commitHash,
},
};
};

module.exports = {
validate,
};
Loading