Add Swagger document for the Message service by pvallone · Pull Request #2 · ni/systemlink-OpenAPI-documents

pvallone · 2018-08-23T22:48:17Z

[X ] This contribution adheres to CONTRIBUTING.md.

What does this Pull Request accomplish?

This pull request adds a Swagger document for the Message service. We'll treat this as the master copy of the Message service's Swagger document, for use in both SystemLink Server and SystemLink cloud. This document is a cleaned up version of the document generated from SystemLink Cloud's implementation of the Message HTTP API.

Why should this Pull Request be merged?

This pull requests completes a user story targeted to 18.2.

What testing has been done?

I've done basic testing of all of the routes against SystemLink Server using this document and Swagger UI.

spanglerco · 2018-08-23T23:13:16Z

+          schema:
+            $ref: '#/definitions/SessionToken'
+          description: Success
+        201:


Was this meant to be 401?

spanglerco · 2018-08-23T23:19:59Z

+          $ref: '#/responses/Unauthorized'
+        default:
+          $ref: '#/responses/Error'
+      parameters:


From a style standpoint, I would think it would be more logical to put the request information before the response information, which here would mean moving parameters above responses. Any reason in particular to put the responses above?

That's the order they were in in Cloud's document, but I agree it makes much more sense for the request information to be first.

spanglerco · 2018-08-23T23:24:32Z

+          name: timeoutMilliseconds
+          description: The time to keep the connection open while waiting for a message
+          type: integer
+          required: false


Do we know if the default timeout is consistent between server and cloud? If it is, we could document it if we wanted to. We may also want to mention in the description that there's a maximum (again, we can check if there's a consistent max or not).

From testing, they both seem to have a max timeout of ten seconds. Server seems to have a default timeout of one second, but I'm not sure what Cloud's is. I'll document these values once we know for sure.

@tschmittni: do you know what SystemLink Cloud uses as the default value for timeoutMilliseconds?

From SystemLink Cloud's node documentation:

Both the SystemLink Server and the SystemLink Cloud server timeout after 10 seconds.

spanglerco · 2018-08-23T23:26:02Z

+        - Message Queue
+      summary: Get the current queue size
+      description: Gets the current queue size (in bytes). This is helpful for determining the amount of memory that unread messages are consuming.
+      operationId: GetCurrentQueueSIze


SIze -> Size

spanglerco · 2018-08-23T23:31:07Z

+              type: string
+        426:
+          description: Upgrade Required
+        default:


A websocket route will never return a JSON error response. If the web service rejects the connection for any reason, the client will receive a 403 (though I don't think we have any specific failure reasons, so probably don't need to document it explicitly?).

spanglerco · 2018-08-23T23:33:50Z

+          description: A value used in the WebSocket opening handshake. Provides part of the information used by the server
+             to prove that it received a valid WebSocket opening handshake.
+          type: string
+          required: true


All of these headers are required for a successful websocket upgrade. I would say remove the default or replace them with x-example.

If we want to be pedantic about it (and I'm not sure we need to), the 101 response must also contain the Upgrade and Connection headers.

Replaced default with x-example.

I'll leave the 101 headers as is, since we're not sure if it's worth defining them.

spanglerco · 2018-08-23T23:37:05Z

+    description: The topic name
+  SessionToken:
+    type: object
+    title: SessionToken


For consistency, there should be a space in SessionToken.

…o output

jjaco2

Edits for clarity and style plus a few notes about how much (if at all) we care about matching corresponding nodes in NXG.

jjaco2 · 2018-08-27T18:51:40Z

+      tags:
+        - Sessions
+      summary: Create a session
+      description: Creates a unique messaging session that can send and receive messages.


Creates a message session with the server to send and receive messages.

jjaco2 · 2018-08-28T14:00:43Z

+    description: Not authorized
+    headers:
+      WWW_Authenticate:
+        description: Information for how to authenticate (only sent by SystemLink Server).


(only sent by SystemLink Server)

(sent only from SystemLink Server)

jjaco2 · 2018-08-28T14:03:14Z

+  Token:
+    in: path
+    name: token
+    description: The unique session ID


jjaco2 · 2018-08-28T14:06:47Z

+      tags:
+        - Topics
+      summary: Subscribe to a topic
+      description: Subscribes to a topic to receive all messages broadcast with that topic name.


I like your description better, but I'm curious what you think of the node description we ended up with for NXG 3.0:

Registers an endpoint to a specific topic, or a name channel.

I think the endpoint they're referring to is the node that gets dropped, so that seems reasonable. I think both are okay in their respective contexts.

jjaco2 · 2018-08-28T14:36:39Z

+    type: object
+    properties:
+      name:
+        description: The string error code


Here and throughout these: drop "The" and "A"

(really minor. can CAR this for later if you want)

Removed leading "The" and "A" from the model descriptions

jjaco2 · 2018-08-28T14:37:40Z

+        description: The identifier of the resource associated with the error
+        type: string
+      message:
+        description: The filled in error message


filled in

complete?

jjaco2 · 2018-08-28T14:40:34Z

+    description: The maximum queue size (in bytes)
+  WebSocketPublish:
+    type: object
+    title: WebSocket Publish Action


Could "Action" be left off these and still make sense?

I'd rather leave it on, as "action" has a very specific meaning in WebSockets. Swagger doesn't support documenting WebSocket APIs, so we have to be as descriptive as possible in our WebSocket action models, and I want to make sure folks can figure out which of our models correspond to our WebSocket actions.

jjaco2 · 2018-08-28T14:43:33Z

+      parameters:
+        - in: query
+          name: timeoutMilliseconds
+          description: The time to keep the connection open while waiting for a message


Amount of time, in milliseconds, to keep the messaging connection open.

BKnight760 · 2018-08-28T15:53:41Z

+  /v1/sessions:
+    post:
+      tags:
+        - Sessions


All tags should be lowercase.

Here, the strings are used to group the routes in Swagger UI, and I don't think we want lowercase for that.

The example "pet store" Swagger file that appears when you first open the editor also uses lowercase tags, so we assume that's the convention.

BKnight760 · 2018-08-28T15:59:40Z

+      parameters:
+        - in: query
+          name: timeoutMilliseconds
+          description: The time to keep the connection open while waiting for a message


May be good to note that this value may be overridden by a maximum value that the webserver administrator allows.

Added a note. Server allows for a ten second maximum by default, and we're waiting on Thomas to tell us what Cloud's maximum is.

BKnight760 · 2018-08-28T16:02:22Z

+      description: Flushes the queue of received messages, discarding all messages without reading them.
+      operationId: DeleteMessages
+      responses:
+        200:


This probably should have been a 204. It would require a change on the webservice itself to fix it.

I filed a CAR: http://nippm.natinst.com/itg/web/knta/crt/RequestDetail.jsp?REQUEST_ID=711012

The same would apply to subscribe, unsubscribe, and publish.

Updated the CAR

…er documentation # Justification #1573106 #1576958 # Implementation In a previous attempt, I tried to fix these bugs by listing the supported values as an in-lined enum. However, that broke some codegen-ed data types and also the public facing python API. Since that would very likely break client code, I resorted to just updating the documentation to specifically list the supported orderby fields. # Testing Since this PR is just plain-text changes now, it shouldn't break a build anywhere. I verified that the changes look correct by copying the yaml file contents over to a swagger doc viewer. You can view it yourself by pasting the contents of the modified file [here](https://editor.swagger.io/). # Checklist - [X] I tested changes to product code in product - [X] I considered updates to the wiki - [ ] If this PR affects user-visible strings, I added Jenna Jaco as a reviewer and attached screenshots of the strings in the product

Add initial version with Paul's changes

0ceeaee

pvallone self-assigned this Aug 23, 2018

pvallone requested review from jjaco2, spanglerco and tschmittni August 23, 2018 22:48

spanglerco requested changes Aug 23, 2018

View reviewed changes

Addressing Paul's feedback

6ebc8d7

spanglerco previously approved these changes Aug 24, 2018

View reviewed changes

Rename to nimessage, for consistency with what our codegen is going t…

baa5ab4

…o output

jjaco2 reviewed Aug 28, 2018

View reviewed changes

BKnight760 requested changes Aug 28, 2018

View reviewed changes

pvallone added 4 commits August 28, 2018 11:39

Addressing Jenna's comments

77a4909

Add a note that timeoutMilliseconds can be overriden by the server

b9c7408

Add examples for the models

878580b

camelCase tags

bb6533d

pvallone dismissed spanglerco’s stale review via bb6533d August 28, 2018 19:02

spanglerco previously approved these changes Aug 28, 2018

View reviewed changes

Iterate on the description for opening a websocket session

1774abd

pvallone dismissed spanglerco’s stale review via 1774abd August 29, 2018 19:52

jjaco2 approved these changes Aug 29, 2018

View reviewed changes

BKnight760 approved these changes Aug 30, 2018

View reviewed changes

pvallone merged commit a902a28 into master Sep 5, 2018

pvallone deleted the users/pvallone/add-message-swagger-doc branch September 20, 2018 15:44

Conversation

pvallone commented Aug 23, 2018

What does this Pull Request accomplish?

Why should this Pull Request be merged?

What testing has been done?

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jjaco2 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels