Refactor /peers endpoint to use swagger runner interface - Closes #925 #944

nazarhussain · 2017-11-07T18:26:17Z

Closes: #925
Parent: #899

MaciejBaj · 2017-11-09T13:33:03Z

api/controllers/peers.js

+	};
+
+	// Remove filters with null values
+	filters = _.pickBy(filters, function (v, k) {


k is unused

That's the signature of the method, while literately over key/value pair. But I only need value here.

I know, that's why the suggestion to remove it

MaciejBaj · 2017-11-09T13:35:57Z

api/controllers/peers.js

+		return !(v === undefined || v === null);
+	});
+
+	modules.peers.shared.getPeers(filters, function (err, data) {


What if err will be received here? Don't we want to return INTERNAL_SERVER_ERROR in this case?

Had introduced error handling structure in other PR, when will be merged, will updated these endpoints to use same pattern.

#949

MaciejBaj · 2017-11-09T13:37:27Z

api/fittings/lisk_params_validator.js

+				});
+			}
+		} else {
+			console.log('not a swagger operation, will not validate response');


How about using logger here?

Two reasons left here. Can't find any way to pass our application scope logger down to individual pipe definition. And secondly this condition is never going to meet, as all our endpoints will have swagger based operation defined in yml. So I didn't invest more time to invent a way to pass our variables to pipe definition.

We don't allow console logs in production, so this will never be seen anyways. ESLINT might get mad as well.

MaciejBaj · 2017-11-09T13:38:09Z

modules/peers.js

+	/**
+	 * Utility method to get peers
+	 *
+	 * @param {object} parameters - Object of all parameters


Capital letter please

MaciejBaj · 2017-11-09T13:39:37Z

schema/swagger.yml

+  wsPort:
+    name: wsPort
+    in: query
+    description: wsPort for the node or delegate


Web socket port ...

MaciejBaj · 2017-11-09T13:40:50Z

schema/swagger.yml

+  version:
+    name: version
+    in: query
+    description: Version of the lisk


Lisk version run by node

MaciejBaj · 2017-11-09T13:46:22Z

test/common/wsServerWorker.js

+var WAMPServer = require('wamp-socket-cluster/WAMPServer');
+var testConfig = require('../config.json');
+
+var necessaryRPCEndpoints = {


Variable duplicated with wsServer

Since later we will refactor and remove wsServer file completely and move to wsServerMaster. So didn't introduced a new dependency here.

Ok, please log the issue then.

MaciejBaj · 2017-11-09T13:49:02Z

test/common/wsServer.js

@@ -64,4 +64,4 @@ var wsServer = {
 	}
 };

-module.exports = wsServer;
+module.exports = wsServer;


Empty line missing

MaciejBaj · 2017-11-09T13:50:14Z

test/common/wsServerMaster.js

+'use strict';
+
+var ChildProcess = require('child_process');
+var path = require('path');


WS server running on separate process covers functionality of the same process wsServer - I think the logic should be reused and wsServer file removed

The dependent tests are complex and have redundant use of wsServer. Already spent more than hour with you to pass ws-functional tests with new process. I used wsServerMaster only where appropriate right now. Refactoring all existing usage, may be out of scope for this PR.

Agree. Another PR can do that refactoring.

MaciejBaj · 2017-11-09T13:55:01Z

test/functional/ws/transport.handshake.js

 					expect(code).equal(failureCodes.INVALID_HEADERS);
 					expect(description).contain('Missing required property');
-					done();
-				});
+				}}).start();


So in the first part of the test we are using new approach relying on WSClient with the handler (disconnect) passed into to verify if the WS connection succeeds or fails. In the with valid headers section, we are using existing approach using functions connect and then expectConnect.
Please choose one and apply in both sections.

Had already spent hour with you. Unless we resolve the socket-hangup issue its not feasible to move those complex test cases. Once we have connection id on client side, all tests will be refactored well. Using a good approach where possible for now, is a good approach. Please suggest.

MaciejBaj · 2017-11-09T13:57:56Z

test/functional/http/get/peers.js

-					node.expect(res.body).to.have.property('peers').that.is.an('array');
-					node.expect(res.body.peers.length).to.be.at.most(limit);
-					done();
+		it('using sort = "version:asc" should be ok', function () {


Change should be ok to should return results in ascending / descending order.

MaciejBaj · 2017-11-09T13:59:34Z

test/functional/http/get/peers.js

+		it('using sort = "version:desc" should be ok', function () {
+			return peersEndpoint.makeRequest({sort: 'version:desc'}, 200)
+				.then(function (res) {
+					_.orderBy(_.clone(res.body.peers), ['version'], ['desc']).should.be.eql(res.body.peers);


In the rest of files to test a proper sort order we are using construction with: _(res.body.peers).map('version').sort().value() with optional reverse().

Please apply it to the file.

Isabello · 2017-11-09T15:03:29Z

api/fittings/lisk_params_validator.js

+				});
+			}
+		} else {
+			console.log('not a swagger operation, will not validate response');


We don't allow console logs in production, so this will never be seen anyways. ESLINT might get mad as well.

Isabello · 2017-11-09T15:05:12Z

test/common/wsServerMaster.js

+'use strict';
+
+var ChildProcess = require('child_process');
+var path = require('path');


Agree. Another PR can do that refactoring.

Isabello · 2017-11-09T15:06:06Z

test/common/wsServerMaster.js

+}
+
+/**
+ * Start the socket server instance. It will start the server as well an instance of the client.


Start the socket server master instance. It will start the server and an instance of the client.

Isabello · 2017-11-09T15:06:52Z

test/functional/http/get/node.js

@@ -35,7 +35,7 @@ describe('GET /node', function () {
 		});

 		it('should return a result containing version = "0.0.0a"', function () {


this should say 0.0.1 if this is where we are going with the config file.

Isabello · 2017-11-09T15:07:53Z

test/functional/ws/transport.handshake.js

@@ -12,6 +12,7 @@ var scClient = require('socketcluster-client');
 var testConfig = require('../../config.json');
 var ws = require('../../common/wsCommunication');
 var wsServer = require('../../common/wsServer');
+var WSClient = require('../../common/wsClient');


casing here.

nazarhussain force-pushed the 925-swagger-peers branch from c8acb5a to a701508 Compare November 7, 2017 18:29

nazarhussain assigned karmacoma Nov 7, 2017

karmacoma added the pending review label Nov 7, 2017

karmacoma changed the title ~~Swagger baed /peers endpoint - Closes #925~~ Swagger based /peers endpoint - Closes #925 Nov 7, 2017

nazarhussain force-pushed the 925-swagger-peers branch from 3ee3094 to a944492 Compare November 8, 2017 16:11

karmacoma requested review from MaciejBaj and Isabello and removed request for MaciejBaj November 8, 2017 19:50

MaciejBaj suggested changes Nov 9, 2017

View reviewed changes

MaciejBaj reviewed Nov 9, 2017

View reviewed changes

Isabello suggested changes Nov 9, 2017

View reviewed changes

nazarhussain added 13 commits November 9, 2017 17:07

Update swagger schema for peers endpoint

d20e6c4

Remove peers api related code for old endpoint

fb3af8b

Add peers controller for swagger based peer endpoint

fb0c914

Add a customized params validator for swagger endpoints

a382a25

Update swagger spec api endpoint generation

d48e574

Add socket server creation constructor

33d2e0f

Update peer min version check to spawn multiple peers

7578926

Refactor peers endpoint functional test cases

990bfd2

Refactor some helper files to keep dependant test cases functional well

a0fdd14

Add jsdoc comments to helper objects

bb2c01f

Add extensive asserts for peers response

18e4e99

Fix a logical error for considering 0 as invalid value for param

af49c2a

Update peers related files to cover the review feedback

970d570

nazarhussain force-pushed the 925-swagger-peers branch from 6b0ae37 to 970d570 Compare November 9, 2017 16:08

Isabello approved these changes Nov 9, 2017

View reviewed changes

MaciejBaj approved these changes Nov 9, 2017

View reviewed changes

karmacoma approved these changes Nov 9, 2017

View reviewed changes

karmacoma merged commit 5cf2aa5 into 1.0.0 Nov 9, 2017

karmacoma deleted the 925-swagger-peers branch November 9, 2017 17:41

karmacoma added ready and removed pending review labels Nov 9, 2017

nazarhussain restored the 925-swagger-peers branch November 10, 2017 09:23

karmacoma deleted the 925-swagger-peers branch November 14, 2017 20:47

karmacoma changed the title ~~Swagger based /peers endpoint - Closes #925~~ Refactor /peers endpoint to use swagger runner interface - Closes #925 Nov 15, 2017

		@@ -35,7 +35,7 @@ describe('GET /node', function () {
		});

		it('should return a result containing version = "0.0.0a"', function () {

Refactor /peers endpoint to use swagger runner interface - Closes #925 #944

Refactor /peers endpoint to use swagger runner interface - Closes #925 #944

Conversation

nazarhussain commented Nov 7, 2017

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

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

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

Choose a reason for hiding this comment