feat: add ws client adapter #319
Conversation
Souvikns
requested review from
fmvilas,
jonaslagoni and
asyncapi-bot-eve
as code owners
Pull Request Test Coverage Report for Build 3592104156Warning: This coverage report may be inaccurate.This pull request's base commit is no longer the HEAD commit of its target branch. This means it includes changes from outside the original pull request, including, potentially, unrelated coverage changes.
Details
💛 - Coveralls |
|
@fmvilas need your review again |
|
@fmvilas as you started the review process, I am assuming you wish to finish it 🙂 |
|
One weird issue I am running into while sending headers from the client side, is the headers are converted to lowercase. {
'sec-websocket-version': '13',
'sec-websocket-key': 'nL+pNzTS+0ryHcEU/4ccPQ==',
connection: 'Upgrade',
upgrade: 'websocket',
authorization: 'bearer arb-tokenValue',
'sec-websocket-extensions': 'permessage-deflate; client_max_window_bits',
host: 'localhost:3000'
}Whereas I am sending the bindings:
ws:
bindingVersion: 0.1.0
headerValues:
Authorization: 'bearer $TOKEN'Found this https://stackoverflow.com/questions/50477233/issue-with-netty-websocket-headers-name-in-lowercase |
- Supports ws client to pass in header and query values from asyncapi file. - injects env to string - makes header validation case insensitive
| websocket: | ||
| url: ws://localhost:3000 | ||
| protocol: ws | ||
| x-kind: local |
There was a problem hiding this comment.
Let's move this property out of the server definition because that's how it will be done in v3.0.0. For now, we can just use our own extension like this:
| x-kind: local | |
| x-localServers: | |
| - websocket |
This will change the logic in the registerAdapters.ts file.
|
Kudos, SonarCloud Quality Gate passed!
|
| websocket: | ||
| url: ws://localhost:3000 | ||
| protocol: ws | ||
| x-kind: local |
There was a problem hiding this comment.
Let's move this property out of the server definition because that's how it will be done in v3.0.0. For now, we can just use our own extension like this:
| x-kind: local | |
| x-localServers: | |
| - websocket |
This will change the logic in the registerAdapters.ts file.
| @@ -18,8 +18,9 @@ export type WebsocketAdapterConfig = { | |||
| port?: number, | |||
| }, | |||
| client?: { | |||
| query?: any | |||
| authentication?: { | |||
There was a problem hiding this comment.
| authentication?: { | |
| headers?: any |
Instead of adding authentication should we just allow header and query config, with headers the user is able to set any type of authentication he desires?
There was a problem hiding this comment.
But the user can already do that once you merge this PR of yours, right? We only saw the need for authentication because it usually contains secrets.
There was a problem hiding this comment.
Yeah, just a suggestion, currently we have an authentication config and we are creating a header Authentication: bearer TOKEN, and it works but maybe we just allow headers instead of authentication, and then we have a broader control, but we only have the need for authentication. We can always change it in the future if there is a need.
There was a problem hiding this comment.
We'd have broader control at Glee but we'd be breaking the purpose of the AsyncAPI specification. If the headers values are in the AsyncAPI document, other tools will be able to see them. If they're in the Glee config, only Glee will be able to see them. You see the problem?
For instance, in #261 we want to use Generator to generate docs automatically. If the info is not in the AsyncAPI file, it will not end up in the documentation. And the same will happen for all the tools that the user might want to apply to their AsyncAPI file.
|
Kudos, SonarCloud Quality Gate passed!
|
|
@fmvilas sentiment analysis is failing, can you rerun the tests, I can't merge right now. |
|
@Souvikns done. I hope your sentiments are fine now 😜 |
|
/rtm |
|
🎉 This PR is included in version 0.12.0 🎉 The release is available on: Your semantic-release bot 📦🚀 |
Description
This PR is trying to implement a
websocketclient adapter for glee.So the main idea is, if I have a WebSocket server, my glee application should be able to -
Related issue(s)
Fixes #259