Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat: replace server url with server host and pathname #888
feat: replace server url with server host and pathname #888
Changes from 2 commits
93d2206
95ef6e4
3ef1cc0
a71d125
3c29f6b
a8c9534
44bef64
2a115ab
0d2c235
100d504
e1b829d
cd06f18
d4ed532
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
path
? where is this coming from?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We were having a "URL" and now we have a host. A host does not have the "path" part so we have to add it separately.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The other option is to remove the
protocol
field and go with a URL as we do now. Just not sure how this would play with protocols likekafka-secure
(e.g.,kafka-secure://hostname:port/path
) andsecure-mqtt
(e.g.,secure-mqtt://hostname:port/path
), which are not "real" protocols. In such a case, we should probably get rid of the "secure" variants (that would includehttps
andwss
) and allow the user to specify it in the Security Requirement Object.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm wondering if we are actually fixing something, or making things a bit more complex. You add now
path
example that is a typical REST example for me. All the other examples I know (for example for Kafka) include a version in the topic name. In case of websocket it is completely fine to add path to channel name, but also if someone wants, can add it tourl
. The only use case forpath
that makes sense is HTTP, but then, if it is protocol specify, shouldn't it end up in the binding?so basically my question is, do we really need to change
url
tohost
andpath
or, as majority of people in #274 wrote that we basically need to make description and examples better?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There are two things here:
url
is "whatever.example.com". That's not a URL and that should fail because it doesn't have the protocol part of the URL (you know,protocol://
).protocol
to define the protocol we want to use but it's already in the URL (!) ☝️ So you can end up withurl: kafka://broker.mycompany.com
andprotocol: https
in the same server definition, which is incongruent.So we either remove the need to put a protocol in the
url
field (which makes it a host instead) or we remove theprotocol
field.I think you're mixing concepts here. A path is not about REST or not even HTTP. A path is a part of a URL. URLs are composed by
protocol://
,username:password
(optional),hostname
,port
(optional),/path/to/resource
(optional),?query=parameters
(optional), and#fragment
(optional). These have nothing to do with REST or HTTP. I can have an AMQP broker atamqp://broker.mycompany.com/public
and another one atamqp://broker.mycompany.com/partners
. Depending on the path (public
orpartners
), I'll reach one broker or another. This is actually a common pattern, BTW.So what I'm doing here is making
path
optional (defaulting to/
as it's assumed by the URL RFC). In the majority of cases, this path will not be needed but in some cases, it will. And yeah, especially in the case of HTTP and WS it's going to be needed but it's also frequent in AMQP and MQTT brokers.So yeah, I took the approach to keep the
protocol
field and convert theurl
field to ahost
(hostname + port) one. Another approach is to removeprotocol
altogether as it's already in the URL field. A matter of preference, I guess.We definitely have to improve the examples but this incongruency goes beyond showing examples or improving description.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well one thing is "concepts" and one thing is "just a simple association"
When I see
My brains says "REST". Then my brain also reminds me of AsyncAPI 1.0 and baseTopic. And then it tries to map it to concepts 😄 but yeah, enough with my brain 😄
Can we then improve the example and not put
https
protocol but something that will not cause brains to fart?For example your
amqp
example. I guess there are different patterns, as I thought people prefer to splitpublic/private
orpublic/partners
by host as then they do different authentication, rate limits and stuff like that. But yeah, it is flexible and people can do it as they wish.or best would be to have 2 examples, one with path and one without it:
api.gemini.com
andpathname
isv1
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
also the origin of my question
was that you use
path
but it is not described in the spec in your PR.also side note: I think it should be
pathname
as we do not want to allow search (query params) here -> https://url.spec.whatwg.org/#dom-url-pathnameThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Absolutely. I'll add two examples.
True. My fault. Adding it.
Good catch. Changing it 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@derberg have a look again, please.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You added here
Server Variable Object Example
but that describes the whole Server Object, here only the:should be written.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think in this case it's justified to have the whole Server Object. Otherwise, there would be no context of where this
demo
variable is coming from.