Endpoints for Apis collection #2545

marla-singer · 2017-05-10T09:56:25Z

Closes #2441
Endpoints for Apis collection

Known issues

Swagger file doesn't have /login and /logout path
After using POST managerIds doesn't contain ID

Add query parameters as limit, skip, lifecycle, q Update logic for "organization" query parameter Make CRUD methods with authentication

marla-singer · 2017-05-10T12:49:59Z

apis/server/api.js


 // Collection imports
 import Apis from '/apis/collection';
+import Organizations from '/organizations/collection';


@mauriciovieira Ok

mauriciovieira

Request to order imports and a comment on the rest api path.

mauriciovieira · 2017-05-10T12:51:28Z

apis/server/api.js


 // Collection imports
 import Apis from '/apis/collection';
+import Organizations from '/organizations/collection';


Sort the imports. Organizations would go after ApiV1

mauriciovieira · 2017-05-10T12:52:30Z

core/server/api.js

@@ -10,7 +10,7 @@ import { Meteor } from 'meteor/meteor';
 import { Restivus } from 'meteor/nimble:restivus';

 const ApiV1 = new Restivus({
-  apiPath: 'api',
+  apiPath: 'apinf-rest',


I wonder if this path should be a Branding configuration. This way we whitelabel also the rest-api.

@kyyberi will we need to have a configurable API path? GET /apinf-rest/v1/users/:id for APInf installation, and GET /my-org-rest/v1/users/:id for a my-org customer, for instance.

@marla-singer I talked to @kyyberi and I think it's fine, and this path can be hardcoded... Anyways, we can change later :-)

Yes, just remove the 'apinf' part of the path.

I wonder if this path should be a Branding configuration.

Also, as a general guideline less is more. When possible, we should find simple, generic solutions, without adding more options to the user interface. This impacts both maintainability and usability.

Describe tags, parameters, securityDefinition, definitions for Apis collection endpoints swagger Set the related swagger meta-data for paths

brylie · 2017-05-12T07:33:14Z

apis/server/api.js

        responses: {
          200: {
-            description: 'List of APIs.',
+            description: 'Returns list of APIs',


Should this say 'Returns list of public APIs'?

brylie · 2017-05-12T07:36:44Z

apis/server/api.js

+        tags: [
+          ApiV1.swagger.tags.api,
+        ],
+        description: 'Returns result is one or zero (no match found) APIs',


This description is confusing. The code comment is easier to understand. Could you explain a bit more here?

brylie · 2017-05-12T07:37:30Z

apis/server/api.js

+        tags: [
+          ApiV1.swagger.tags.api,
+        ],
+        description: 'Adds an API to catalog. On success methods return added object with details.',


Consider describing as:

Adds an API to catalog. On success, returns newly added API object.

brylie · 2017-05-12T07:38:14Z

apis/server/api.js

+        ],
+        responses: {
+          200: {
+            description: 'API was successful added',


Change to:

API successfully added

brylie · 2017-05-12T07:45:15Z

apis/server/api.js

+    put: {
+      authRequired: true,
+      // manager role is required. If a user already has an API then the user has manager role
+      roleRequired: ['manager', 'admin'],


What is the reason to include 'manager' role here?

Look comment above

If a user already has an API then the user has manager role

Right, but what do we achieve from using the manager role here?

brylie · 2017-05-12T07:48:11Z

apis/server/api.js

        parameters: [
          ApiV1.swagger.params.apiId,
        ],
        responses: {
          200: {
-            description: 'One API.',
+            description: 'API was successful removed.',


Successful -> successfully

brylie · 2017-05-12T07:48:39Z

core/server/api.js

@@ -10,7 +10,7 @@ import { Meteor } from 'meteor/meteor';
 import { Restivus } from 'meteor/nimble:restivus';

 const ApiV1 = new Restivus({
-  apiPath: 'api',
+  apiPath: 'apinf-rest',


Yes, just remove the 'apinf' part of the path.

brylie · 2017-05-12T07:49:40Z

core/server/api.js

  },
  params: {
    apiId: {
      name: 'id',
      in: 'path',
-      description: 'Api ID',
+      description: 'Pass ID of the API',


Change to

ID of API

No verb ('pass') is necessary.

brylie · 2017-05-12T07:50:14Z

core/server/api.js

+    optionalSearch: {
+      name: 'q',
+      in: 'query',
+      description: 'Pass an optional search string for looking up inventory.',


Change to:

optional search string for finding APIs.

brylie · 2017-05-12T07:52:31Z

core/server/api.js

    routeOptions: {
      authRequired: true,
    },
    endpoints: {
+      // GET /apinf-rest/v1/users/:id


Remember to update all of these comments if changing base path (removing apinf from base path).

brylie · 2017-05-12T09:34:28Z

apis/server/api.js

-        // Make sure API exists
-        if (api) {
+        // Make sure API exists & user can manage
+        if (api && api.managerIds.indexOf(userId) > 0) {


Can we use the api.currentUserCanManage() helper here?

@brylie Not, we can not

Error: Meteor.userId can only be invoked in method calls. Use this.userId in publish functions. at AccountsServer.userId (packages/accounts-base/accounts_server.js:82:13) at Object.Meteor.userId (packages/accounts-base/accounts_common.js:252:19) at Document.Apis.helpers.currentUserCanManage (apis/collection/helpers.js:33:27) at Object.ApiV1.addCollection.endpoints.put.action (apis/server/api.js:193:24) at Route.share.Route.Route._callEndpoint (packages/nimble_restivus/lib/route.coffee:149:25) at packages/nimble_restivus/lib/route.coffee:59:33 at packages/simple_json-routes.js:98:9

brylie · 2017-05-12T09:36:23Z

apis/server/api.js

+        ],
+        responses: {
+          200: {
+            description: 'Returns API entity',


Maybe we don't need to say 'entity' here.

brylie · 2017-05-12T09:38:14Z

apis/server/api.js

+        },
+        security: [
+          {
+            userSecurityToken: [],


What do these do?

Do authorization. Read docs about creation Swagger specification

Ah, I thought they were related to Restivus. Also, it is helpful to provide links to related documentation.

@brylie http://swagger.io/specification/#securityRequirementObject

brylie · 2017-05-12T09:39:08Z

apis/server/api.js

+        const api = Apis.findOne(apiId);
+
+        // Make sure API exists & user can manage
+        if (api && api.managerIds.indexOf(userId) > 0) {


Same as above. Lets re-use the api.currentUserCanManage() helper, since it does this same thing in a re-usable and self-describing manner.

Improve comments in Swagger file Add condition for permissions

marla-singer · 2017-05-12T10:39:37Z

@brylie Ready

marla-singer added 3 commits May 5, 2017 11:07

ApiV1: comments, clean up

3f9937b

Eslint: add exception in rule

6438c9e

Add REST API for /apis/ endpoint

41bc550

Add query parameters as limit, skip, lifecycle, q Update logic for "organization" query parameter Make CRUD methods with authentication

marla-singer added the in progress label May 10, 2017

marla-singer commented May 10, 2017

View reviewed changes

mauriciovieira reviewed May 10, 2017

View reviewed changes

Update apinf:restivus-swagger package

c90b19d

brylie assigned jawidahmadi and matleppa May 11, 2017

Define Swagger block for Apis collection endpoints

96a928c

Describe tags, parameters, securityDefinition, definitions for Apis collection endpoints swagger Set the related swagger meta-data for paths

marla-singer force-pushed the feature/rest-apis branch from a246214 to 96a928c Compare May 11, 2017 14:23

marla-singer added this to the Sprint 42 milestone May 11, 2017

marla-singer added the Ready for review label May 11, 2017

brylie suggested changes May 12, 2017

View reviewed changes

apis/server/api.js Outdated

],

responses: {

200: {

description: 'Returns API entity',

Copy link

Contributor

brylie May 12, 2017

Choose a reason for hiding this comment

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

Maybe we don't need to say 'entity' here.

brylie reviewed May 12, 2017

View reviewed changes

brylie suggested changes May 12, 2017

View reviewed changes

brylie removed the Ready for review label May 12, 2017

Update logic for PUT and DELETE request (Apis collection)

3e0cf7c

Improve comments in Swagger file Add condition for permissions

marla-singer force-pushed the feature/rest-apis branch from 5682951 to 3e0cf7c Compare May 12, 2017 10:39

marla-singer added the Ready for review label May 12, 2017

brylie merged commit ef0a138 into develop May 12, 2017

brylie deleted the feature/rest-apis branch May 12, 2017 13:31

brylie removed in progress Ready for review labels May 12, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Endpoints for Apis collection #2545

Endpoints for Apis collection #2545

marla-singer commented May 10, 2017 •

edited

Loading

marla-singer May 10, 2017

mauriciovieira left a comment

mauriciovieira May 10, 2017

mauriciovieira May 10, 2017

mauriciovieira May 11, 2017

brylie May 12, 2017

brylie May 12, 2017

brylie May 12, 2017

brylie May 12, 2017

brylie May 12, 2017

brylie May 12, 2017

brylie May 12, 2017

marla-singer May 12, 2017

brylie May 12, 2017

brylie May 12, 2017

brylie May 12, 2017

brylie May 12, 2017

brylie May 12, 2017

brylie May 12, 2017

brylie May 12, 2017

marla-singer May 12, 2017

brylie May 12, 2017

brylie May 12, 2017

marla-singer May 12, 2017

brylie May 12, 2017

marla-singer May 12, 2017

brylie May 12, 2017

marla-singer commented May 12, 2017

Endpoints for Apis collection #2545

Endpoints for Apis collection #2545

Conversation

marla-singer commented May 10, 2017 • edited Loading

Known issues

Choose a reason for hiding this comment

mauriciovieira 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

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

marla-singer commented May 12, 2017

marla-singer commented May 10, 2017 •

edited

Loading