Comments on the API

[jhthorsen]

First: I'm sorry for making an issue, which is simply a series of questions/comments. Please let me know if I should have contacted you in a different format.

The reason I'm asking is that before I saw this implementation, I was working on my own, which I consider much simpler. https://github.com/jhthorsen/swagger2/compare/websocket

So to the questions/comments:

1. I don't understand the handshake part. Which benefit does it add? Why is it required? The way I'm implementing it, is purely by structuring the REST HTTP request in a JSON format sent over the websocket. Something like this:

{
  "id": "whatever", # the response will contain the same ID
  "op": "getSomething", # the operationId added to the swagger spec
  "headers": { "If-None-Match": "123" }, # HTTP headers
  "query": { "limit": 20 }, # URL query string
  "path": { "id": 42 }, # URL path parts
  "body": {}, # HTTP body
}

1. I agree on Allow Client to Define ID #9. Actually, I think the client should create an ID and not the server. In my API, the client have to create an ID for each of the requests and not just the session. The reason for this is that the session is already logically identified on the server side. On the other hand, each request need a unique ID, so the client will know which request the server sends the respond back to. After all, there is no guaranty that the answers come in the same order as the requests was sent.

I would really like to get some feedback.

Thanks in advance.

[jfarcand]

The handshake is required because the protocol support fallback and to make sure the proper protocol version is used.

[elakito]

@jhthorsen
I also thought about when the handshaking part becomes useful. One is for the protocol check at the beginning as @jfarcand mentions. The other one is for providing some context (multiple contexts over a single socket). This latter one is not done now from the implementation point of view. But having a separate ID in addition to the per-message ID in the protocol makes it possible. So, I don't think it is bad to have this step which happens only once.

Regarding your second point (#9), if you are talking about the per-message ID, you can set it in the request or get it out of the request object if you don't set it.

@jfarcand
There is one issue that seems to require a change to the current protocol. I was thinking of writing to the ML or creating another issue ticket, but I think I can describe it here.

When you have a service that returns a large response, the underlining framework will typically trigger in a series of the write operations, which, in turn, will generate a series of swaggersocket responses that have no indication of which one is the last one. I think we should add a flag in the response to indicate the last message. Otherwise, we would need to buffer the data until the close is invoked and then to flush the entire data at once as a single swaggersocket over the WebSocket.

So, it's one thing to add this flag to the protocol, the other thing is how to implement it. When the underlining framework is guaranteed to invoke the close operation at the end, we can buffer the data up to the default buffer size, return only the overflown data during the write operation and return the final message by flushing the remaining buffer at the close operation.

In addition to this requirement, we also might want to provide a way to return no body responses. Currently, when the service returns no body, that means there is no write operation invoked and there is no response at all. In conjunction with the above approach of buffering and flushing at the close operation, we can also provide a way to return a body-less swaggersocket responses.

How do you think? I think we should provide this option so that various usages can be supported.

regards, aki

[jfarcand]

@elakito OK I agree we could add a flag saying this is the last message, but note that with Atmosphere if the TrackMessageSizeInterceptor is installed this issue won't happens, but true the bytes will be buffered client side.

Agree for the body-less response!

[elakito]

@jfarcand
Umm. I don't think TrackMessgeSizeInterceptor will help for this case. I thought TMSI would only help for those cases where the response data written in a single write operation is split into two messages by the underling protocol and returned to the client in two chunks. In this case, the client can assembly the entire message. In contrast, if there are two write operations resulting from a single request, this will results in two separate responses each having the size prefix. And there is no way to know that the second message is part of the first message, no?

[elakito]

I created #27 and #28 to track the above two issues.
so further comments specific to those issues can be added to the individual tickets.
thanks

[jfarcand]

@elakito Ok completely swamped those by the work this week, will take a look ASAP.

[elakito]

@jfarcand no problem.
thanks for following up on this topic.

[jfarcand]

@elakito IIRC, the protocol is able to handle that case using the response=>uuid. True TMSI won't make any difference, but the client side javascript should know how to re-associate the second response, no?

[elakito]

@jfarcand,
using the uuid, the client can associate the series belonging to a single
logical response, but the client still doesn't know when the EOF of that
series is reached unless the original data has a hierarchical structure
like xml or json that can implicitly indicate the end of its structure or
in other words, its ground-level.

In #28, I went ahead and added the code that adds a boolean primitive
property in the response. But I am not sure if I should use a String or
Boolean property instead to make this property only included when it is set
to true?

regards, aki

2015-03-09 13:39 GMT+01:00 Jeanfrancois Arcand notifications@github.com:

@elakito https://github.com/elakito IIRC, the protocol is able to
handle that case using the response=>uuid. True TMSI won't make any
difference, but the client side javascript should know how to re-associate
the second response, no?

—
Reply to this email directly or view it on GitHub
#18 (comment)
.

[jfarcand]

@elakito For consistency I would vote for a boolean which is part of the protocol and always included.

[elakito]

@jfarcand Okay. Then, we can keep it that way. I think further
wishes/customization that @jhthorsen and others have brought can be handled
when we work #5, which can be handled after 2.0.0?

One thing that I wanted to add before 2.0.0 is an option to filter/include
the headers added in the response message. Currently, we only include the
content-type header that the client may need to interpret the response. I
think we should have a way to tell the protocol handler which headers are
needed by the client. Alternatively, we could include all the headers in
the response but some headers have no use in the non-standard http
responses and they just lead to wasted bytes.

2015-03-09 14:47 GMT+01:00 Jeanfrancois Arcand notifications@github.com:

@elakito https://github.com/elakito For consistency I would vote for a
boolean which is part of the protocol and always included.

—
Reply to this email directly or view it on GitHub
#18 (comment)
.

[jfarcand]

👍 for the headers.

Yes I think we need to release 2.0.0 sooner than later, so I propose we work on it in 2.0.1. As for the release of 2.0.0, how confortable are you with it? I propose we open another issue and try to sketch a roadmap..maybe @fehguy will be interested to help as well :-)

[elakito]

@jfarcarnd For 2.0.0, I think we can close it up this week. For the
headers, I'll make some proposals (not sure if we should use the include
and exclude regex or explicit include or exclude lists to identify the
headers, etc). The other minor things that have to be before 2.0.0 are to
upgrade the atmosphere.js et al to 2.2.8. and updating the 2011 Wordnick
copyright. @fehguy, what should be at the copyright line? 2015 Wordnik or
2015 Reverb Technology, Inc. as in other swagger projects?

2015-03-09 16:26 GMT+01:00 Jeanfrancois Arcand notifications@github.com:

[image: 👍] for the headers.

Yes I think we need to release 2.0.0 sooner than later, so I propose we
work on it in 2.0.1. As for the release of 2.0.0, how confortable are you
with it? I propose we open another issue and try to sketch a roadmap..maybe
@fehguy https://github.com/fehguy will be interested to help as well :-)

—
Reply to this email directly or view it on GitHub
#18 (comment)
.

[fehguy]

@elakito should be 2015 Reverb :) Thanks for checking in, we're sweeping through things right now and updating any missing copyright notices.

[jhthorsen]

Closing this, since it seems like a stale issue.