feat: replace server url with server host and pathname

[fmvilas]

Fix bug with Server's URL field

Related issue(s):

#547
#274

[sonarqubecloud]

Kudos, SonarCloud Quality Gate passed!   

0 Bugs
0 Vulnerabilities
0 Security Hotspots
0 Code Smells

No Coverage information
0.0% Duplication

[magicmatatjahu]

You added here Server Variable Object Example but that describes the whole Server Object, here only the:

        default: demo
        description: This value is assigned by the service provider, in this example `mycompany.com`.

should be written.

[fmvilas]

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.

[derberg]

I think title of PR should be more descriptive. Also PR should have short description so people do not have to read linked issues.

fix: rename server.url to server.host - something like that, to explain in one word what this change actually does, as later we use it in the change log

[derberg]

path ? where is this coming from?

[fmvilas]

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.

[fmvilas]

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 like kafka-secure (e.g., kafka-secure://hostname:port/path) and secure-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 include https and wss) and allow the user to specify it in the Security Requirement Object.

[derberg]

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 to url. The only use case for path 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 to host and path or, as majority of people in #274 wrote that we basically need to make description and examples better?

[fmvilas]

There are two things here:

1. We often show examples where the 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://).
2. We have a field called protocol to define the protocol we want to use but it's already in the URL (!) ☝️ So you can end up with url: kafka://broker.mycompany.com and protocol: 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 the protocol field.

You add now path example that is a typical REST example for me.

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 at amqp://broker.mycompany.com/public and another one at amqp://broker.mycompany.com/partners. Depending on the path (public or partners), 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 the url field to a host (hostname + port) one. Another approach is to remove protocol altogether as it's already in the URL field. A matter of preference, I guess.

so basically my question is, do we really need to change url to host and path or, as majority of people in #274 wrote that we basically need to make description and examples better?

We definitely have to improve the examples but this incongruency goes beyond showing examples or improving description.

[derberg]

Well one thing is "concepts" and one thing is "just a simple association"

When I see

"host": "{org}.mycompany.com",
"path": "/api/v1",
"protocol": "https"

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 split public/private or public/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:

• websocket from kraken provides different hosts (URL 😄 ) https://docs.kraken.com/websockets/#connectionDetails
• websocket from geminig (https://docs.gemini.com/websocket-api/#market-data) where host is api.gemini.com and pathname is v1

[derberg]

also the origin of my question

path ? where is this coming from?

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-pathname

[fmvilas]

Can we then improve the example and not put https protocol but something that will not cause brains to fart?

or best would be to have 2 examples, one with path and one without it:

Absolutely. I'll add two examples.

was that you use path but it is not described in the spec in your PR.

True. My fault. Adding it.

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-pathname

Good catch. Changing it 👍

[fmvilas]

@derberg have a look again, please.

[derberg]

added one comment as I see your idea is that variables should be supported in pathname. Sounds good to me but we need an update.

Also, sorry if my previous comment sounded like I was suggesting that Gemini is a good example for pathname. I do not think it is, at least I modeled it differently in the official example -> https://github.com/asyncapi/spec/blob/a8c95348745dbeb5158f241f5b04ce7a1bbb6289/examples/websocket-gemini.yml

[fmvilas]

Also, sorry if my previous comment sounded like I was suggesting that Gemini is a good example for pathname. I do not think it is, at least I modeled it differently in the official example -> https://github.com/asyncapi/spec/blob/a8c95348745dbeb5158f241f5b04ce7a1bbb6289/examples/websocket-gemini.yml

I think it's still a good example. The difference is that you're defining the whole API and I'm just defining one endpoint 😄 But I get your point. Do you think we should change it to avoid confusions with your example?

[derberg]

@fmvilas yeah, I'm afraid that 2 examples contradict each other. Maybe add amqp samples you mentioned in the other comment, with public and private to something like that

[fmvilas]

@derberg I think examples are much better now. Have a look, please.

[derberg]

LGTM 🚀

[derberg]

I only think if it is really a fix, and not feat. In the end we have new fields, different new behaviour, so it is more an enhancement, new feature than fix

[dalelane]

the protocol versions in the Kafka examples aren't valid Kafka versions - so I've added a couple of suggested fixes to replace them with Kafka version numbers

but approving as they're only examples and not super critical

[char0n]

LGTM.

The only issue I found is in here:

spec/spec/asyncapi.md

Line 1628 in 0d2c235

"url": "{stage}.gigantic-server.com:{port}",

Example is still using url field instead of new one.

[fmvilas]

@derberg @dalelane @char0n @smoya would please review and approve again? 🙏

[magicmatatjahu]

@fmvilas Do you want to add the json-schema changes yourself? I can help you

[fmvilas]

@magicmatatjahu feel free mate! 😄

[magicmatatjahu]

JSON Schema PR asyncapi/spec-json-schemas#329
ParserJS PR asyncapi/parser-js#708

[sonarqubecloud]

Kudos, SonarCloud Quality Gate passed!   

0 Bugs
0 Vulnerabilities
0 Security Hotspots
0 Code Smells

No Coverage information
0.0% Duplication

[fmvilas]

/rtm

[asyncapi-bot]

🎉 This PR is included in version 3.0.0-next-major-spec.9 🎉

The release is available on GitHub release

Your semantic-release bot 📦🚀