lib: implement API versioning #231

lundibundi · 2017-06-22T09:26:01Z

allow to specify API version during handshake
make createAppsIndex handle multiple API versions
add API version parameter to Application
add API version parameter to SimpleConnectPolicy
add tests for interface versioning

Fixes: #31.

nechaido

LGTM

nechaido · 2017-06-23T10:08:25Z

lib/applications.js

+      index.set(application.name, appVersions);
+    }
+    if (!application.version) {
+      // no version means default version


I'd change default to latest, but it is not necessary.

nechaido · 2017-06-23T10:45:47Z

It should be mentioned that this PR Fixes: #31.

belochub

The implementation differs from what we discussed it should be, using semver isn't possible at all.

belochub · 2017-06-23T11:34:37Z

lib/applications.js

@@ -9,11 +9,13 @@ module.exports = apps;
 // your needs.
 //   name - application name
 //   api - application API


Although it is not related to this PR "application API" is a tautology, and this comment should provide more detailed information about the way API must be provided, instead of this one as it gives no relevant information.

belochub · 2017-06-23T11:35:11Z

lib/applications.js

@@ -9,11 +9,13 @@ module.exports = apps;
 // your needs.
 //   name - application name
 //   api - application API
+//   version - api version of 'name' application


I'd put "application version" or "application API version" here, this way it'll be easier to read.

belochub · 2017-06-23T12:02:40Z

lib/applications.js

+    }
+    if (!application.version) {
+      // no version means default version
+      if (appVersions.get('latest')) {


You should definitely use has method here because you don't use the value.

Oh, thanks.

belochub · 2017-06-23T12:04:22Z

lib/applications.js

+        appVersions.set('latest', application);
+      }
+    } else {
+      appVersions.set(application.version.toString(), application);


Can version be a non-string value?
I thought that we agreed on using the semver, which meant using strings as version values.

belochub · 2017-06-23T12:06:18Z

lib/applications.js

+
+      // save latest version to fill missing latest versions later
+      const latestApp = latestApps.get(application.name);
+      if (!latestApp || application.version > latestApp.version) {


Same here, it is incorrect to compare semver versions like that.

belochub · 2017-06-23T12:07:40Z

lib/connection.js

+    let target;
+    if (typeof app === 'string') {
+      target = app;
+    } else if (!app.version) {


You should also add a possibility to provide app as a string containing both name and version like that: appName@1.0.0.

lundibundi · 2017-06-27T18:24:34Z

@belochub as discussed I've added support for semver in application versions. Also I've updated tests to check that functionality. Btw I've changed that comment message (api parameter description) even though this PR doesn't directly modify API but it is related to the API, so I think it's okay to put those changes here.
@metarhia/jstp-core PTAL.

belochub · 2017-06-27T22:23:06Z

lib/server.js

@@ -24,6 +26,15 @@ const initServer = function(
    applications = apps.createAppsIndex(applications);
  }

+  // versions cached for efficient search when provided version is a range
+  this._cachedVersions = {};


In my opinion, it is better to use Object.create(null) or even Map here.

Oh, I again forgot about map, thanks.

belochub · 2017-06-28T22:34:50Z

lib/server.js

+  _getApplication(name, version) {
+    const appVersions = this.applications.get(name);
+    if (!appVersions) return null;
+    if (version) {


You can invert the condition here to avoid a lot of indentation below.

belochub

LGTM

nechaido

LGTM

aqrln · 2017-07-07T18:37:23Z

lib/connection.js

-      )
-    };
+    const packet = {};
+    if (Array.isArray(target)) {


I don't like the idea of making this function polymorphic. It is a hot function, and passing arguments of different types will mess up with V8's ICs.

@aqrln I guess I'll split this into 2 functions with one accepting an array and the other value, what do you think?

Yeah, that would work if we needed it at all. See my other comment.

Why not always expect an array here though?

@aqrln, to avoid creation of an array with one element in some cases, it should probably be better for performance.

Also I'd add that in most cases it is a single argument, as handshake is the only place with array and it's performed only once per connection.

@aqrln also what about this one?

@lundibundi based on the conversation above, I though there was consensus about it. I have explicitly approved splitting this into two functions above, and neither me nor anyone else has objected to your last comment, so feel free to pick either approach.

aqrln · 2017-07-07T18:44:05Z

test/node/api-versions.js

+  }
+};
+
+const interfacesVLatest = {


Wait... do I get it right that latest is a version by itself and not just a pointer to the latest semver version (2.0.0 in this case)? That sounds wrong to me. latest and the like are tags, they must not be independent versions.

No, It was made to check specific behavior (when app has no version it's latest version). If every app has a version then the biggest version will be the latest, see https://github.com/metarhia/jstp/pull/231/files/66c24103eef081bdb56c42696a400182502634f5#diff-13dd866a30aedfb1fab7c3d9da9446ceR162.

belochub · 2017-07-11T08:08:33Z

@aqrln, ping.

aqrln · 2017-07-11T08:13:15Z

lib/connection.js

@@ -112,15 +113,26 @@ class Connection extends EventEmitter {
  }

  // Send a handshake packet over the connection
-  //   appName - name of an application to connect to
+  //   app - string or object, application to connect to as 'name' or
+  //         'name@version' or { name, version }


Do we really need all the three of these? No strong opinion on it, but I'd rather make name@version the only format.

Same here, I don't really care. @metarhia/jstp-core somebody wants to leave the third option here?

Yeah, why not?

No more comments means 1 for, 0 against, hence we left it as is.

aqrln · 2017-07-11T08:15:14Z

lib/connection.js

+    if (typeof app === 'string') {
+      const [name, version] = app.split('@');
+      if (semver.valid(version)) target = [name, version];
+      else target = name;


I'd throw an error here instead of silently discarding an invalid version.

@aqrln Yeah, that will be better.

@lundibundi this is still unaddressed.

@aqrln as you can see in the comments below I'm waiting to get your approval to make all changes at once as I don't want to perform rebase multiple times for naught.

aqrln · 2017-07-11T08:18:04Z

lib/connection.js

+    } else if (!app.version) {
+      target = app.name;
+    } else {
+      target = [app.name, app.version];


Can you please stick to either a string (as in { handshake: [0, 'app@latest'] }) or an array (as in { handshake: [0, 'app', 'latest'] }) in both cases? As the parsed messages are plain objects, the current approach may suffer from the constantly changing object shapes, which will lead to either polymorphic IC in _processHandshake() in the best-case scenario or to deoptimizing it in the worst-case one.

@aqrln, as far as I understand, first option you provided isn't possible, because it is always an array, check out _createPacket function. Sometimes version can just be omitted in the array, meaning that latest version should be used, and packet will look like this:
{ handshake: [0, 'app'] }.

Ah, I see. Right.

aqrln · 2017-07-11T08:29:55Z

lib/server.js

+  applications.forEach((appVersions, appName) => {
+    const versions = Array.from(appVersions)
+      .filter(app => app[0] !== 'latest')
+      .map(version => [new semver.SemVer(version[0]), version[1]])


Can you please use destructuring in the left-hand side of the lambda to give some descriptive names for these values (i.e., ([something, somethingElse]) => [new semver.SemVer(something), somethingElse])?

Aside from that, why do app and version identifiers refer to the same objects in this and previous lines?

@aqrln, there's comment on line 111 in this file.

It doesn't help much. The file is read top to bottom, and these variable names are somewhat misleading. Both app and version at lines 33 and 34 are actually arrays of [version, app], AFAIU.

Yes, but here it is obvious, because of line 32: this is an array created from the map with versions as keys, and applications as values, there is no need to make a destruction assignment here.
However, I do agree that arguments in lambdas should be renamed.

@belochub @aqrln Also I see no particular reason to specifically emphasize arguments with destructuring here and it should be sufficient to rename .filter argument to version to be consistent, bit I don't really care so it's up to you.

This is still unaddressed.

@aqrln but there is no approval of yours to my proposed changes?

@lundibundi I would still prefer ([version, app]) => ..., but if you and @belochub are against it, then at least making the names consistent works for me too.

aqrln · 2017-07-11T08:36:43Z

lib/server.js

+        if (range.test(version[0])) return version[1];
+      }
+    } catch (error) {
+      // TODO (lundibundi): after loggers are implemented write using them


Should it return a new error code instead of ERR_APP_NOT_FOUND in this case? 🤔

@aqrln technically it's still missing application 😃, but it may indeed be clearer to actually add another error.

@aqrln there is no response here as well.

@lundibundi there haven't been any changes requested as well ;)

I'm fine with ERR_APP_NOT_FOUND.

belochub · 2017-07-14T17:36:08Z

@lundibundi, can you address the comments and rebase this on master, please?

lundibundi · 2017-07-14T20:51:59Z

@belochub I've already answered the comments and haven't seen any response since then, that's why I'm waiting. Also I didn't rebase it as I want to address the comments too at the same time.

belochub · 2017-07-25T11:11:12Z

lib/connection.js

@@ -364,7 +417,9 @@ class Connection extends EventEmitter {
    }

    const applicationName = packet.handshake[1];
-    const application = this.server.applications[applicationName];
+    const applicationVersion = packet.handshake[2];
+    const application =


I am not sure about the style of this, didn't we change styling for such cases?

belochub · 2017-07-25T11:15:55Z

lib/net.js

-    connectAndInspect(appName, client, interfaces, ...options),
+  connect: (app, client, ...options) =>
+    connect(app, client, ...options),
+  connectAndInspect: (app, client, interfaces, ...options) =>


You should also rename this argument in lib/cli/command-processor.js.

aqrln · 2017-07-26T15:52:50Z

lib/applications.js

-  constructor(name, api) {
-    this.name = name;
+  constructor(name, api, version) {
+    [this.name, this.version] = name.split('@');


Should it split from the right instead of from the left?

I mean, @metarhia/application@1.0 is a valid package name, so I'd expect it to be parsed as an application name correctly too.

@aqrln, in such cases you can provide an object with fields name and version set to corresponding values.

@belochub I don't find it a valid justification. Why provide two ways of doing the same thing with one of them being subtly broken? Either the npm-ish format must work according to users' expectations, or the { name, version } schema must be the only available contract.

@aqrln and in the above example will the user have application name as @metarhia/application? As we don't support scopes (they mean nothing in our implementation) is there any reason to support that? But anyway it's easy to implement so it's okay with me.

@aqrln, by the way, are you sure that @metarhia/application@1.0 is a valid package name?
Trying to create the package with such name using npm leads to the error:
Sorry, name can only contain URL-friendly characters.

Did you try to make @1.0 a part of the package name or...?

aqrln · 2017-07-26T15:55:43Z

lib/connection.js

+  handshake(app, login, password, callback) {
+    let name, version;
+    if (typeof app === 'string') {
+      [name, version] = app.split('@');


nechaido

LGTM.

nechaido · 2017-07-29T13:21:20Z

lib/applications.js

 //
 class Application {
-  constructor(name, api) {
-    this.name = name;
+  constructor(name, api, version) {


I'd prefer to swap parameters api and version but it's up to you.

Even though I agree that it will look better first of all it will not be compatible and second in case version is specified in the name you will have to pass null to the version parameter, because api will come after it.

nechaido · 2017-07-29T13:38:47Z

test/node/api-versions.js

+
+    const port = server.address().port;
+
+    // latest


I'd prefer this to be a subtest.

nechaido · 2017-07-29T13:38:56Z

test/node/api-versions.js

+      });
+    });
+
+    // v1


nechaido · 2017-07-29T13:39:05Z

test/node/api-versions.js

+      });
+    });
+
+    // v2


nechaido · 2017-07-29T13:39:24Z

test/node/api-versions.js

+
+    const port = server.address().port;
+
+    // ^1.0.0


nechaido · 2017-07-29T13:39:34Z

test/node/api-versions.js

+      });
+    });
+
+    // > v1.0.0


* allow to specify API version during handshake * make createAppsIndex handle multiple API versions * add API version parameter to Application * add tests for interface versioning

* Allow to specify API version during handshake. * Make createAppsIndex handle multiple API versions. * Add API version parameter to Application. * Add tests for interface versioning. Fixes: #31 PR-URL: #231 Reviewed-By: Dmytro Nechai <nechaido@gmail.com> Reviewed-By: Alexey Orlenko <eaglexrlnk@gmail.com> Reviewed-By: Mykola Bilochub <nbelochub@gmail.com>

belochub · 2017-07-31T12:25:48Z

Landed in 9116125.

* Allow to specify API version during handshake. * Make createAppsIndex handle multiple API versions. * Add API version parameter to Application. * Add tests for interface versioning. Fixes: #31 PR-URL: #231 Reviewed-By: Dmytro Nechai <nechaido@gmail.com> Reviewed-By: Alexey Orlenko <eaglexrlnk@gmail.com> Reviewed-By: Mykola Bilochub <nbelochub@gmail.com>

This is a new and shiny first major release for `metarhia-jstp`. Changes include API refactoring and improvements, implementations of CLI, sessions, and application versions, native addon build optimizations, lots of bug fixes, test coverage increase, and other, less notable changes. This release also denotes the bump of the protocol version to v1.0. The only difference from the previous version of the protocol is that "old" heartbeat messages (`{}`) are now deprecated and `ping`/`pong` messages must be used for this purpose instead. Notable changes: * **src,build:** improve the native module subsystem *(Alexey Orlenko)* [#36](#36) **\[semver-minor\]** * **build:** compile in ISO C++11 mode *(Alexey Orlenko)* [#37](#37) **\[semver-minor\]** * **build:** improve error handling *(Alexey Orlenko)* [#40](#40) **\[semver-minor\]** * **lib:** refactor record-serialization.js *(Alexey Orlenko)* [#41](#41) * **parser:** fix a possible memory leak *(Alexey Orlenko)* [#44](#44) **\[semver-minor\]** * **protocol:** change the format of handshake packets *(Alexey Orlenko)* [#54](#54) **\[semver-major\]** * **parser:** make parser single-pass *(Mykola Bilochub)* [#61](#61) * **parser:** remove special case for '\0' literal *(Mykola Bilochub)* [#68](#68) **\[semver-major\]** * **parser:** fix bug causing node to crash *(Mykola Bilochub)* [#75](#75) * **client:** drop redundant callback argument *(Alexey Orlenko)* [#104](#104) **\[semver-major\]** * **client:** handle errors in connectAndInspect *(Alexey Orlenko)* [#105](#105) **\[semver-major\]** * **socket,ws:** use socket.destroy() properly *(Alexey Orlenko)* [#84](#84) **\[semver-major\]** * **cli:** add basic implementation *(Mykola Bilochub)* [#107](#107) **\[semver-minor\]** * **connection:** fix error handling in optional cbs *(Alexey Orlenko)* [#147](#147) **\[semver-major\]** * **test:** add JSON5 specs test suite *(Alexey Orlenko)* [#158](#158) * **lib:** change event signature *(Denys Otrishko)* [#187](#187) **\[semver-major\]** * **lib:** add address method to Server *(Denys Otrishko)* [#190](#190) **\[semver-minor\]** * **parser:** implement NaN and Infinity parsing *(Mykola Bilochub)* [#201](#201) * **parser:** improve string parsing performance *(Mykola Bilochub)* [#220](#220) * **lib:** optimize connection events *(Denys Otrishko)* [#222](#222) **\[semver-major\]** * **lib:** refactor server and client API *(Denys Otrishko)* [#209](#209) **\[semver-major\]** * **lib,src:** rename term packet usages to message *(Denys Otrishko)* [#270](#270) **\[semver-major\]** * **lib:** emit events about connection messages *(Denys Otrishko)* [#252](#252) **\[semver-minor\]** * **lib:** implement API versioning *(Denys Otrishko)* [#231](#231) **\[semver-minor\]** * **lib:** allow to set event handlers in application *(Denys Otrishko)* [#286](#286) **\[semver-minor\]** * **lib:** allow to broadcast events from server *(Denys Otrishko)* [#287](#287) **\[semver-minor\]** * **connection:** make callback method private *(Alexey Orlenko)* [#306](#306) **\[semver-major\]** * **lib:** implement sessions *(Mykola Bilochub)* [#289](#289) **\[semver-major\]** * **connection:** use ping-pong instead of heartbeat *(Dmytro Nechai)* [#303](#303) **\[semver-major\]**

* Allow to specify API version during handshake. * Make createAppsIndex handle multiple API versions. * Add API version parameter to Application. * Add tests for interface versioning. Fixes: metarhia/jstp#31 PR-URL: metarhia/jstp#231 Reviewed-By: Dmytro Nechai <nechaido@gmail.com> Reviewed-By: Alexey Orlenko <eaglexrlnk@gmail.com> Reviewed-By: Mykola Bilochub <nbelochub@gmail.com>

lundibundi added connection protocol server labels Jun 22, 2017

lundibundi added this to the 1.0.0 milestone Jun 22, 2017

lundibundi self-assigned this Jun 22, 2017

lundibundi requested review from belochub, aqrln and nechaido June 22, 2017 09:26

lundibundi force-pushed the master branch from a09dfe8 to 38fc11f Compare June 22, 2017 10:42

lundibundi force-pushed the add-api-versioning branch from cd089f0 to 5b3795d Compare June 22, 2017 10:49

nechaido approved these changes Jun 23, 2017

View reviewed changes

nechaido reviewed Jun 23, 2017

View reviewed changes

belochub requested changes Jun 23, 2017

View reviewed changes

lundibundi force-pushed the add-api-versioning branch from bc3473c to d99092e Compare June 27, 2017 18:20

belochub reviewed Jun 27, 2017

View reviewed changes

belochub reviewed Jun 28, 2017

View reviewed changes

belochub previously approved these changes Jun 29, 2017

View reviewed changes

nechaido approved these changes Jul 1, 2017

View reviewed changes

aqrln reviewed Jul 7, 2017

View reviewed changes

aqrln reviewed Jul 11, 2017

View reviewed changes

lundibundi force-pushed the add-api-versioning branch from f018bef to 56667aa Compare July 25, 2017 10:56

belochub reviewed Jul 25, 2017

View reviewed changes

aqrln reviewed Jul 26, 2017

View reviewed changes

lundibundi force-pushed the add-api-versioning branch 2 times, most recently from c29d0cc to c11816d Compare July 28, 2017 08:22

nechaido approved these changes Jul 29, 2017

View reviewed changes

lundibundi requested a review from aqrln July 29, 2017 22:00

aqrln approved these changes Jul 30, 2017

View reviewed changes

lundibundi added 7 commits July 30, 2017 12:14

lib: implement API versioning

6c7ef2f

* allow to specify API version during handshake * make createAppsIndex handle multiple API versions * add API version parameter to Application * add tests for interface versioning

fixup! try to fix mac build

4d09ebb

fixup! fixup! try to fix mac build

ea3969d

fixup! split appnames by the last occurrence of @

5bed6f2

fixup! change missed during rebase function name

bedd8b3

fixup! split big tests into subtests

ecef10e

fixup! update semver package

e2e21ac

lundibundi force-pushed the add-api-versioning branch from 40db3bb to e2e21ac Compare July 30, 2017 09:15

fixup! add missed error check

918538a

belochub approved these changes Jul 31, 2017

View reviewed changes

belochub closed this Jul 31, 2017

belochub deleted the add-api-versioning branch July 31, 2017 12:25

fixup! change returning function name in inspect

b1cf0a0

belochub mentioned this pull request Jan 22, 2018

v1.0.0 proposal #311

Merged

aqrln added the semver-minor label Jan 22, 2018

+                    });
+                  });
+                  // v1

+                    });
+                  });
+                  // v2

lib: implement API versioning #231

lib: implement API versioning #231

Conversation

lundibundi commented Jun 22, 2017 • edited Loading

nechaido left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nechaido commented Jun 23, 2017

belochub 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

lundibundi commented Jun 27, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

belochub left a comment

Choose a reason for hiding this comment

nechaido 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

aqrln Jul 21, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

belochub commented Jul 11, 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

aqrln Jul 11, 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

lundibundi Jul 21, 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

belochub commented Jul 14, 2017

lundibundi commented Jul 14, 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

aqrln Jul 26, 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

nechaido left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lundibundi commented Jun 22, 2017 •

edited

Loading

lundibundi commented Jun 27, 2017 •

edited

Loading

aqrln Jul 21, 2017 •

edited

Loading

aqrln Jul 11, 2017 •

edited

Loading

lundibundi Jul 21, 2017 •

edited

Loading

lundibundi commented Jul 14, 2017 •

edited

Loading

aqrln Jul 26, 2017 •

edited

Loading