Validation module #109

mukkachaitanya · 2018-12-17T18:02:08Z

Requirements

Need for proper error handling for invalid commands
Need for offloading the validation from the inputs.

Description of the Change

Adds a validation module which offloads the command validation and provides proper error messages. The changes also makes the applications closer to the MVC architecure. The cli/input module are simple view with no buisness logic.
The changes also identify and decrease coupling between modules by the use of mesage passing.

Benefits

The invalid commands exit now with proper error messages. Brings all the validation to a single module.
This module brings in ease of intergating new views.
Message passing decouples various modules allowing for better modularity.
The change also decreases the tests of cli/input which is a according to the SRP.

Applicable Issues

Addresses #100

codecov · 2018-12-17T18:04:09Z

Codecov Report

Merging #109 into dev will increase coverage by 0.02%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##              dev     #109      +/-   ##
==========================================
+ Coverage   99.24%   99.27%   +0.02%     
==========================================
  Files          22       24       +2     
  Lines         662      688      +26     
==========================================
+ Hits          657      683      +26     
  Misses          5        5

Flag	Coverage Δ
#integration	`94.91% <93.61%> (-0.11%)`	⬇️
#unit	`99.27% <100%> (+0.02%)`	⬆️

Impacted Files	Coverage Δ
lib/cli/input/prefs.js	`100% <100%> (ø)`	⬆️
lib/cli/input/eval.js	`100% <100%> (ø)`	⬆️
lib/controller/eval.js	`100% <100%> (ø)`	⬆️
lib/cli/output/eval.js	`100% <100%> (ø)`	⬆️
lib/controller/validate/eval.js	`100% <100%> (ø)`
lib/cli/input/prefsprompt.js	`100% <100%> (ø)`	⬆️
lib/controller/prefs.js	`100% <100%> (ø)`	⬆️
lib/model/eval.js	`91.37% <100%> (+0.15%)`	⬆️
lib/controller/validate/prefs.js	`100% <100%> (ø)`
... and 1 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 3969cab...a0ccfc5. Read the comment docs.

prasadtalasila

@mukkachaitanya please see the suggested changes.

prasadtalasila · 2018-12-18T04:42:00Z

lib/controller/validation.js

@@ -0,0 +1,27 @@
+const prefsOutput = require('../cli/output/prefs');


We agreed on doing the validation at the controller. So, the location of this module is appropriate. However, we are going to need validation for all the commands, not just preferences. Hence, we may need to think about a better structure (perhaps validation/prefs.js to include validators for all the controllers).

Another possibility is to put small validators in the respective controllers themselves and only push reasonably big / complex validators to a separate module.

prasadtalasila · 2018-12-18T04:43:20Z

lib/controller/prefs.js

@@ -2,14 +2,20 @@ const prefsInput = require('../cli/input/prefs');
 const prefsOutput = require('../cli/output/prefs');
 const prefsModel = require('../model/prefs');


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

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.

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.

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.

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.

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.

prasadtalasila · 2018-12-18T05:07:28Z

lib/controller/validation.js

@@ -0,0 +1,27 @@
+const prefsOutput = require('../cli/output/prefs');
+
+const supportedServers = ['ms', 'gitlab'];


These constants are better derived from config.

prasadtalasila · 2018-12-18T05:09:22Z

lib/controller/validation.js

+const supportedServers = ['ms', 'gitlab'];
+const supportedCommand = ['show', 'changelang', 'changeserver', 'logger'];
+
+const prefs = (args, options) => {


We can ponder a bit on appropriate guidelines for the use of arrow functions in this project. If you can read a bit, and create a wiki page while also quoting good quality references, we can stick to the created guidelines everywhere in the project.

Ok sir. I'll read up about it.

mukkachaitanya · 2018-12-22T19:44:18Z

Some considerations:

The length validations in cli/input/init.js are left inside input to provide proper user feedback while input.
eval module didn't implement message passing between input and model. That has been implemented. (and also here)

Also Sir, how should we take care of the root user and the id parameter, should it be taken care at validation? Currently, it's been taken care like this.
I haven't put the test cases yet, wanted approval of the structure and idea.

prasadtalasila · 2018-12-23T20:26:56Z

@mukkachaitanya The code looks good. Please proceed with the same structure for tests. The validation of username to rootshould be moved to controller/validator/

mukkachaitanya · 2018-12-26T17:43:45Z

Sir, the tests have been updated. I have updated the PR description also to include benefits and changes.

prasadtalasila

@mukkachaitanya Apart from two minor changes, the PR looks good. Thanks.

prasadtalasila · 2018-12-27T04:33:33Z

test/integration/prefs.js

@@ -287,7 +287,7 @@ async function testChangeMSPrompt() {
  process.argv = ['/usr/local/nodejs/bin/node',
    '/usr/local/nodejs/bin/autolabjs', 'prefs', 'changeserver', '--type', 'ms'];
  const mockInquirer = sandbox.mock(inquirer);
-  const testPort = 5687;
+  const testPort = '5687';


Is there any specific reason for this change?

This description here explains the change https://github.com/chriso/validator.js#strings-only

This library validates and sanitizes strings only.
If you're not sure if your input is a string, coerce it using input + ''. Passing anything other than a string is an error.

Honestly, this is because of the #108. I think I have an updated version of validator.js in my node_modules.

prasadtalasila · 2018-12-27T04:35:03Z

test/unit/lib/cli/input/prefs.js

-  it('should send the appropriate message when invalid host is given for gitlab server', testGitlabInvalidHost);
-  it('should send the appropriate message when invalid port is given', testInvalidPort);
-  it('should send the appropriate message when invalid server is given', testInvalidServer);
+  it('should send the right event when differnt server is changed', testDefaultServer);


spell check: different

Adds validation module

3028ee5

prasadtalasila requested changes Dec 18, 2018

View reviewed changes

mukkachaitanya added 2 commits December 20, 2018 00:43

Add validate module to encapsulate validations

b90abc3

Decouple the modules and refactor validation

e2b6b3f

Adds tests for validation module

1961af9

prasadtalasila requested changes Dec 27, 2018

View reviewed changes

Minor spelling correction

a0ccfc5

prasadtalasila merged commit d6de73c into AutolabJS:dev Dec 27, 2018

snyk-bot mentioned this pull request Sep 14, 2021

[Snyk] Security upgrade caporal from 0.9.0 to 1.0.0 #139

Open

prasadtalasila mentioned this pull request May 17, 2022

[Snyk] Security upgrade caporal from 0.9.0 to 1.0.0 #142

Open

prasadtalasila mentioned this pull request Nov 28, 2023

[Snyk] Security upgrade caporal from 0.9.0 to 1.0.0 #149

Open

prasadtalasila mentioned this pull request Apr 17, 2024

[Snyk] Fix for 1 vulnerabilities #151

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Validation module #109

Validation module #109

mukkachaitanya commented Dec 17, 2018 •

edited

Loading

codecov bot commented Dec 17, 2018 •

edited

Loading

prasadtalasila left a comment

prasadtalasila Dec 18, 2018

prasadtalasila Dec 18, 2018

prasadtalasila Dec 18, 2018

mukkachaitanya Dec 18, 2018

mukkachaitanya Dec 18, 2018

prasadtalasila Dec 18, 2018

prasadtalasila Dec 18, 2018

prasadtalasila Dec 18, 2018

mukkachaitanya Dec 18, 2018

prasadtalasila Dec 18, 2018

mukkachaitanya Dec 18, 2018

mukkachaitanya commented Dec 22, 2018

prasadtalasila commented Dec 23, 2018

mukkachaitanya commented Dec 26, 2018 •

edited

Loading

prasadtalasila left a comment

prasadtalasila Dec 27, 2018

mukkachaitanya Dec 27, 2018

prasadtalasila Dec 27, 2018

		@@ -0,0 +1,27 @@
		const prefsOutput = require('../cli/output/prefs');

		@@ -2,14 +2,20 @@ const prefsInput = require('../cli/input/prefs');
		const prefsOutput = require('../cli/output/prefs');
		const prefsModel = require('../model/prefs');

		@@ -0,0 +1,27 @@
		const prefsOutput = require('../cli/output/prefs');

		const supportedServers = ['ms', 'gitlab'];

Validation module #109

Validation module #109

Conversation

mukkachaitanya commented Dec 17, 2018 • edited Loading

Requirements

Description of the Change

Benefits

Applicable Issues

codecov bot commented Dec 17, 2018 • edited Loading

Codecov Report

prasadtalasila left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mukkachaitanya commented Dec 22, 2018

prasadtalasila commented Dec 23, 2018

mukkachaitanya commented Dec 26, 2018 • edited Loading

prasadtalasila left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mukkachaitanya commented Dec 17, 2018 •

edited

Loading

codecov bot commented Dec 17, 2018 •

edited

Loading

mukkachaitanya commented Dec 26, 2018 •

edited

Loading