server: add a way to move connections to a new app version #310

nechaido · 2017-12-06T20:39:39Z

No description provided.

lundibundi · 2017-12-11T09:21:56Z

lib/applications.js

+    const versions = new Map();
+    index.set(name, versions);
+    if (instances.length === 1) {
+      versions.set('1.0.0', instances[0]);


You should check if app has version and set 1.0.0 only if it doesn't. You discard app version here.

lundibundi · 2017-12-11T09:29:23Z

lib/connection.js

    );

    if (!application) {
      this._handshakeError(errors.ERR_APP_NOT_FOUND);
      return;
    }

+    if (!this.requestedVersion) {
+      this.requestedVersion = semver.major(application.version).toString();


In case when there is only one app without version and no version specified upon handshake call this still seems to call semver.major on undefined, or have I missed something?

lundibundi · 2017-12-11T19:59:54Z

lib/server.js

@@ -92,6 +92,19 @@ class Server {
    } = prepareApplications(applications));
  }

+  // Update applivation version for each connection.
+  updateConnections() {


Maybe updateConnectionsApi? Just a nit though.

I don't like both of them, maybe someone has better ideas @metarhia/jstp-core ?

lundibundi · 2017-12-11T20:03:04Z

lib/server.js

+  // Update applivation version for each connection.
+  updateConnections() {
+    for (const connection of this.clients.values()) {
+      if (semver.valid(connection.requestedVersion)) {


It's better to add a comment that this function will not update clients who specified direct versions and not version-range upon request in order to make this clearer. Or just comment here like 'ignore clients who requested specific API versions'.

lundibundi · 2017-12-11T20:07:17Z

test/node/api-versions.js

+    });
+  });
+});
+


I think we should also add test that'll test 'must connect to app without version('latest') when client didn't specify version'

@lundibundi there is no app without version now, and we already have a test that checks if a user connects to the latest app, if no version was specified.

lundibundi · 2017-12-11T20:11:48Z

test/node/update-api.js

@@ -55,3 +90,79 @@ test.test('must update API', (test) => {
    }
  );
 });
+
+test.test(
+  'must not update API on existing connection if no supported version is found',


We should clarify test description here and in the tests after this one (probably adding 'version' should be enough), it'll be hard to understand the problem if one of those fails.

@lundibundi could you please clarify proposed change?

@nechaido We can for example for this test add version to the description as this ''must not update API on existing connection if no supported version is found (connect to @1)" and so with other tests in order to clearly discern them should an error occur.

belochub · 2017-12-12T18:51:49Z

Rebase this on master and rename it to correspond to our general naming style, please.

belochub · 2017-12-13T18:12:01Z

lib/applications.js

@@ -17,13 +17,14 @@ const errors = require('./errors');
 //                   of which contains functions by event names. Each method
 //                   has the following signature
 //                   (connection, <0 or more event arguments>)
-//   version - application version of 'name' application (optional)
+//   version - application version of 'name' application (optional) if version
+//             is not provided neither here nor in `name` '1.0.0' will be used


There is a double negative and a missing comma in this sentence which should be fixed and I think it'll be better to start the new sentence from 'if' so that the end result will look like this:

// version - application version of 'name' application (optional). // If a version is provided neither here nor in `name`, // '1.0.0' will be used

or

// version - application version of 'name' application (optional). // If a version is not provided either here or in `name`, // '1.0.0' will be used

belochub · 2017-12-13T18:29:58Z

lib/applications.js

-    if (!this.version) this.version = version;
-    if (this.version && !semver.valid(this.version)) {
+    if (!this.version) this.version = version || '1.0.0';
+    if (!semver.valid(this.version)) {


There is no need to check the validity of the version after you set its value to '1.0.0', I propose to move the assignment to this.version after if:

const providedVersion = this.version || version; if (providedVersion && !semver.valid(providedVersion)) { throw new TypeError('Invalid semver version'); } this.version = providedVersion || '1.0.0';

belochub · 2017-12-13T18:32:53Z

test/node/update-api.js

+
+test.test(
+  'must not update API on existing connection if no supported version is ' +
+  'found (connection to @1.0.1)',


It says '(connection to @1.0.1)' but you are actually connecting to '1.0.0' on line 158, is this maybe a typo?

belochub · 2017-12-13T18:49:36Z

lib/applications.js

-      }
+    if (versions.get(application.version)) {
+      throw new Error(
+        'Multiple instances of application with the same version'


I think it'll be better to provide more information in this error, such as application name and version value that is being repeated.

belochub · 2017-12-13T18:51:01Z

lib/applications.js

-      if (!latestApp || semver.gt(application.version, latestApp.version)) {
-        latestApps.set(application.name, application);
-      }
+    if (versions.get(application.version)) {


It is better to use has instead of get in this case.

belochub · 2017-12-13T19:16:50Z

lib/server.js

@@ -92,6 +92,22 @@ class Server {
    } = prepareApplications(applications));
  }

+  // Update application version for each connection.
+  // It will update to a newest version in the requested range. If client
+  // requested version in (1.1) range he will not be updated to (1.2.x). If no


I think it'll be better to specify the first range as (1.1.x) instead of just (1.1) for consistency with the second one.
You should also add a comma after 'range': 'If client requested version in (1.1.x) range, he will...'.

belochub · 2017-12-13T19:25:02Z

lib/server.js

+  // Update application version for each connection.
+  // It will update to a newest version in the requested range. If client
+  // requested version in (1.1) range he will not be updated to (1.2.x). If no
+  // suitable version is found client will use it's previous app version.


Missing a comma and an article here: '...is found, the client will...'.
Also, 'it's' shouldn't have an apostrophe, it is a possessive here.

belochub · 2017-12-13T19:33:14Z

lib/server.js

+      const newApp = this._getApplication(
+        connection.application.name, connection.requestedVersion
+      );
+      connection.application = newApp || connection.application;


In my opinion, it is better to use if here for readability, because in the case when !newApp is true it comes down to the assignment of the variable to itself, which isn't more clear than just doing

if (newApp) { connection.application = newApp; }

belochub · 2017-12-13T19:41:03Z

test/node/api-versions.js

@@ -58,9 +59,15 @@ test.test('must allow to specify version in application name', (test) => {
  test.end();
 });

+test.test('must set version to (1.0.0) if none was provided', (test) => {


Why does this test have the same description as the one on line 221 in this file, do they test the same thing?

I forgot to delete a test on line 221. It is deleted now.

PR-URL: #310 Reviewed-By: Mykola Bilochub <nbelochub@gmail.com> Reviewed-By: Denys Otrishko <shishugi@gmail.com>

nechaido · 2017-12-19T16:11:08Z

Landed in f94b50e and 23b8b18.

PR-URL: #310 Reviewed-By: Mykola Bilochub <nbelochub@gmail.com> Reviewed-By: Denys Otrishko <shishugi@gmail.com>

nechaido added blocked server labels Dec 6, 2017

nechaido requested a review from lundibundi December 7, 2017 10:39

nechaido added the work-in-progress label Dec 7, 2017

nechaido force-pushed the update-connection-app branch from 8b0257a to 1f67196 Compare December 9, 2017 16:03

nechaido removed blocked work-in-progress labels Dec 9, 2017

nechaido requested a review from belochub December 9, 2017 16:05

lundibundi reviewed Dec 11, 2017

View reviewed changes

nechaido changed the title ~~Update connection app~~ server: add a way to move connections to a new app version Dec 13, 2017

nechaido requested a review from lundibundi December 13, 2017 17:25

lundibundi approved these changes Dec 13, 2017

View reviewed changes

belochub requested changes Dec 13, 2017

View reviewed changes

nechaido requested a review from belochub December 13, 2017 20:37

nechaido added 2 commits December 14, 2017 17:56

application: replace no version with 1.0.0

7bbaf81

server: reconnect connections to a newer api

99eb3d4

nechaido force-pushed the update-connection-app branch from b214a3a to 99eb3d4 Compare December 14, 2017 15:57

belochub approved these changes Dec 14, 2017

View reviewed changes

nechaido added a commit that referenced this pull request Dec 19, 2017

application: make 1.0.0 the default version

f94b50e

PR-URL: #310 Reviewed-By: Mykola Bilochub <nbelochub@gmail.com> Reviewed-By: Denys Otrishko <shishugi@gmail.com>

nechaido added a commit that referenced this pull request Dec 19, 2017

server: add a way for updating API on connections

23b8b18

PR-URL: #310 Reviewed-By: Mykola Bilochub <nbelochub@gmail.com> Reviewed-By: Denys Otrishko <shishugi@gmail.com>

nechaido closed this Dec 19, 2017

nechaido deleted the update-connection-app branch December 19, 2017 16:11

belochub pushed a commit that referenced this pull request Jan 22, 2018

application: make 1.0.0 the default version

6fea9a0

PR-URL: #310 Reviewed-By: Mykola Bilochub <nbelochub@gmail.com> Reviewed-By: Denys Otrishko <shishugi@gmail.com>

belochub pushed a commit that referenced this pull request Jan 22, 2018

server: add a way for updating API on connections

690d9ac

PR-URL: #310 Reviewed-By: Mykola Bilochub <nbelochub@gmail.com> Reviewed-By: Denys Otrishko <shishugi@gmail.com>

belochub pushed a commit that referenced this pull request Jan 22, 2018

application: make 1.0.0 the default version

fbc6e04

PR-URL: #310 Reviewed-By: Mykola Bilochub <nbelochub@gmail.com> Reviewed-By: Denys Otrishko <shishugi@gmail.com>

belochub pushed a commit that referenced this pull request Jan 22, 2018

server: add a way for updating API on connections

a04de1f

PR-URL: #310 Reviewed-By: Mykola Bilochub <nbelochub@gmail.com> Reviewed-By: Denys Otrishko <shishugi@gmail.com>

belochub mentioned this pull request Jan 22, 2018

v1.0.0 proposal #311

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

server: add a way to move connections to a new app version #310

server: add a way to move connections to a new app version #310

nechaido commented Dec 6, 2017 •

edited

Loading

lundibundi Dec 11, 2017

lundibundi Dec 11, 2017

nechaido Dec 11, 2017

lundibundi Dec 11, 2017

nechaido Dec 12, 2017

lundibundi Dec 11, 2017

lundibundi Dec 11, 2017

nechaido Dec 12, 2017

lundibundi Dec 11, 2017

nechaido Dec 13, 2017

lundibundi Dec 13, 2017 •

edited

Loading

belochub commented Dec 12, 2017

belochub Dec 13, 2017

belochub Dec 13, 2017

belochub Dec 13, 2017

belochub Dec 13, 2017

belochub Dec 13, 2017

belochub Dec 13, 2017

belochub Dec 13, 2017

belochub Dec 13, 2017

belochub Dec 13, 2017

nechaido Dec 13, 2017

nechaido commented Dec 19, 2017

server: add a way to move connections to a new app version #310

server: add a way to move connections to a new app version #310

Conversation

nechaido commented Dec 6, 2017 • edited Loading

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

lundibundi Dec 13, 2017 • edited Loading

Choose a reason for hiding this comment

belochub commented Dec 12, 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

nechaido commented Dec 19, 2017

nechaido commented Dec 6, 2017 •

edited

Loading

lundibundi Dec 13, 2017 •

edited

Loading