feat: unify all referencing mechanisms #852

fmvilas · 2022-10-13T17:57:31Z

Description

This PR unifies all the referencing mechanisms in the spec. So far, only the security requirements object had to be changed.

Related issues

#829

sonarcloud · 2022-10-13T17:57:49Z

Kudos, SonarCloud Quality Gate passed!

0 Bugs
0 Vulnerabilities
0 Security Hotspots
0 Code Smells

No Coverage information
0.0% Duplication

fmvilas · 2022-10-13T18:05:12Z

spec/asyncapi.md

@@ -373,7 +372,7 @@ Field Name | Type | Description
 <a name="serverObjectProtocolVersion"></a>protocolVersion | `string` | The version of the protocol used for connection. For instance: AMQP `0.9.1`, HTTP `2.0`, Kafka `1.0.0`, etc.
 <a name="serverObjectDescription"></a>description | `string` | An optional string describing the host designated by the URL. [CommonMark syntax](https://spec.commonmark.org/) MAY be used for rich text representation.
 <a name="serverObjectVariables"></a>variables | Map[`string`, [Server Variable Object](#serverVariableObject) \| [Reference Object](#referenceObject)]] | A map between a variable name and its value.  The value is used for substitution in the server's URL template.
-<a name="serverObjectSecurity"></a>security | [[Security Requirement Object](#securityRequirementObject)] | A declaration of which security mechanisms can be used with this server. The list of values includes alternative security requirement objects that can be used. Only one of the security requirement objects need to be satisfied to authorize a connection or operation.
+<a name="serverObjectSecurity"></a>security | [[Security Scheme Object](#securitySchemeObject) \| [Reference Object](#referenceObject)] | A declaration of which security schemes can be used with this server. The list of values includes alternative [security scheme objects](#securitySchemeObject) that can be used. Only one of the security scheme objects need to be satisfied to authorize a connection or operation.


Instead of having a weird syntax like:

security: - httpBearerToken: []

My proposal is that we do the following:

... security: - $ref: '#/components/securitySchemes/httpBearerToken' ... components: securitySchemes: httpBearerToken: type: http scheme: bearer

The array of the current syntax is just for OAuth2 and OpenIDConnect schemes so I created a new property called scopes in the Security Scheme Object. These will be the mandatory scopes needed to connect or perform an operation. It offers less reusability in the security property but you can anyway create as many Security Scheme Objects inside components so, yeah, not an issue.

Consider that in the security field we have 3 options:

defining one security (as in your case)

defining several security that must be met at once:

... security: - - $ref: '#/components/securitySchemes/1schema' - $ref: '#/components/securitySchemes/2schema'

defining several security, for which only one security must be satisfied.

... security: - $ref: '#/components/securitySchemes/1schema' - $ref: '#/components/securitySchemes/2schema'

So as you can see, we have problem with second case (several securities have to met at once). We have even issue for that #828

The array of the current syntax is just for OAuth2 and OpenIDConnect schemes so I created a new property called scopes in the Security Scheme Object. These will be the mandatory scopes needed to connect or perform an operation. It offers less reusability in the security property but you can anyway create as many Security Scheme Objects inside components so, yeah, not an issue.

Why not to create security item shape as:

security: - scheme: $ref: '#/components/securitySchemes/1schema' scopes: [...]

?

Why not to create security item shape as:

security: - scheme: $ref: '#/components/securitySchemes/1schema' scopes: [...]

?

Why should we? :) The proposed syntax looks much cleaner IMHO. It only has the problem of meeting multiple security schemes at once but I'm not sure it should be a requirement. Left a comment at #828 (comment).

As I remember, we have issue for that (or mention about it in another issue, I don't know), but I'm not involving with it. cc @smoya

@magicmatatjahu you raised it as thoughts here: #778 (comment)

I don't think it has been discussed anywhere else 🧐

Or are we planning not to support merging an object when using $ref alongside other properties?

It's not that we are planning not to support it, but we have not planned or had a discussion about whether that should be a feature I think 🤔

Interesting. So maybe it's time to have it 😄 This could simplify life in cases like this. And tooling is already behaving this way, I think.

And tooling is already behaving this way, I think.

That is only because the tooling has bugs, it is not intended behavior... 🙂

Adding this functionality will definitely complicate it from a spec perspective, but yea simplify it in this case 😆

Bug or feature? 😛 To be honest, this is a thing I see many people doing/trying expecting it to work. But yeah, that's not a discussion for this thread sorry for the noise.

I strongly encourage folks to file thoughts around potential reference merging in AsyncAPI as issues at the unified referencing discussion repo, where we can consider them alongside, for example, the limited merging of summary and description that OpenAPI 3.1 does outside of JSON Schema references.

This wouldn't mean AsyncAPI is blocked from taking other action, but unified referencing will only be unified if we know what's going on with each project.

fmvilas · 2022-10-13T18:06:02Z

spec/asyncapi.md

-          "description": "This environment is meant for developers to run their own tests"
-        }
-      ]
+  "url": "{username}.gigantic-server.com:{port}/{basePath}",


Here I'm just removing context information in the example. Just to keep it the same as in the rest of the spec.

fmvilas · 2022-10-13T18:07:58Z

spec/asyncapi.md

@@ -2442,106 +2401,32 @@ Field Name | Type | Applies To | Description
 <a name="oauthFlowAuthorizationUrl"></a>authorizationUrl | `string` | `oauth2` (`"implicit"`, `"authorizationCode"`) | **REQUIRED**. The authorization URL to be used for this flow. This MUST be in the form of an absolute URL.
 <a name="oauthFlowTokenUrl"></a>tokenUrl | `string` | `oauth2` (`"password"`, `"clientCredentials"`, `"authorizationCode"`) | **REQUIRED**. The token URL to be used for this flow. This MUST be in the form of an absolute URL.
 <a name="oauthFlowRefreshUrl"></a>refreshUrl | `string` | `oauth2` | The URL to be used for obtaining refresh tokens. This MUST be in the form of an absolute URL.
-<a name="oauthFlowScopes"></a>scopes | Map[`string`, `string`] | `oauth2` | **REQUIRED**. The available scopes for the OAuth2 security scheme. A map between the scope name and a short description for it.
+<a name="oauthFlowScopes"></a>availableScopes | Map[`string`, `string`] | `oauth2` | **REQUIRED**. The available scopes for the OAuth2 security scheme. A map between the scope name and a short description for it.


Renaming the scopes property of the OAuthFlow Object to availableScopes to differentiate from the other one added in this PR.

spec/asyncapi.md

fmvilas · 2022-12-17T16:45:29Z

@derberg @dalelane @char0n @magicmatatjahu @jonaslagoni would you mind having a look and approving if you like it? I want to start getting rid of these small PRs.

handrews

This consistency is great to see!

Is discriminator being considered as well? It uses property values as implicit references. I believe OpenAPI 4 is likely to deprecate it in favor of JSON Schema's forthcoming propertyDependencies (which uses $ref normally). So that would be one option (even with AsyncAPI 3.0 staying on draft-07, you could import that keyword as an extension in place of discriminator).

BTW, I am not demanding that it be addressed in this PR or this release, I'm just pointing out that it's an implicit reference as currently defined. Fixing security scheme is definitely higher-value!

handrews · 2023-01-10T19:14:28Z

spec/asyncapi.md

@@ -373,7 +372,7 @@ Field Name | Type | Description
 <a name="serverObjectProtocolVersion"></a>protocolVersion | `string` | The version of the protocol used for connection. For instance: AMQP `0.9.1`, HTTP `2.0`, Kafka `1.0.0`, etc.
 <a name="serverObjectDescription"></a>description | `string` | An optional string describing the host designated by the URL. [CommonMark syntax](https://spec.commonmark.org/) MAY be used for rich text representation.
 <a name="serverObjectVariables"></a>variables | Map[`string`, [Server Variable Object](#serverVariableObject) \| [Reference Object](#referenceObject)]] | A map between a variable name and its value.  The value is used for substitution in the server's URL template.
-<a name="serverObjectSecurity"></a>security | [[Security Requirement Object](#securityRequirementObject)] | A declaration of which security mechanisms can be used with this server. The list of values includes alternative security requirement objects that can be used. Only one of the security requirement objects need to be satisfied to authorize a connection or operation.
+<a name="serverObjectSecurity"></a>security | [[Security Scheme Object](#securitySchemeObject) \| [Reference Object](#referenceObject)] | A declaration of which security schemes can be used with this server. The list of values includes alternative [security scheme objects](#securitySchemeObject) that can be used. Only one of the security scheme objects need to be satisfied to authorize a connection or operation.


I strongly encourage folks to file thoughts around potential reference merging in AsyncAPI as issues at the unified referencing discussion repo, where we can consider them alongside, for example, the limited merging of summary and description that OpenAPI 3.1 does outside of JSON Schema references.

This wouldn't mean AsyncAPI is blocked from taking other action, but unified referencing will only be unified if we know what's going on with each project.

derberg

lgtm, just resolve conflicts and I'm ready to merge

sonarcloud · 2023-01-16T18:02:21Z

Kudos, SonarCloud Quality Gate passed!

0 Bugs
0 Vulnerabilities
0 Security Hotspots
0 Code Smells

No Coverage information
0.0% Duplication

fmvilas · 2023-01-16T18:04:02Z

👍 Done!

fmvilas · 2023-01-17T15:34:38Z

/rtm

asyncapi-bot · 2023-01-17T15:36:13Z

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

The release is available on GitHub release

Your semantic-release bot 📦🚀

feat: unify all referencing mechanisms

2a7acb2

fmvilas requested review from derberg, dalelane, smoya, char0n and asyncapi-bot-eve as code owners October 13, 2022 17:57

fmvilas commented Oct 13, 2022

View reviewed changes

This was referenced Oct 13, 2022

Work on 3.0 release #691

Closed

Unify all referencing mechanisms in v3 #829

Closed

fmvilas commented Dec 5, 2022

View reviewed changes

spec/asyncapi.md Show resolved Hide resolved

fmvilas added the 💡 Proposal (RFC 1) RFC Stage 1 (See CONTRIBUTING.md) label Dec 17, 2022

dalelane approved these changes Jan 2, 2023

View reviewed changes

handrews reviewed Jan 10, 2023

View reviewed changes

derberg reviewed Jan 16, 2023

View reviewed changes

Merge branch 'next-major-spec' into unify-all-referencing-mechanisms

939f7c1

derberg approved these changes Jan 17, 2023

View reviewed changes

asyncapi-bot added the ready-to-merge label Jan 17, 2023

asyncapi-bot merged commit 36061cd into asyncapi:next-major-spec Jan 17, 2023

asyncapi-bot added the released on @next-major-spec label Jan 17, 2023

GreenRover pushed a commit to GreenRover/asyncapi-spec that referenced this pull request Jan 18, 2023

feat: unify all referencing mechanisms (asyncapi#852)

36fd1a3

fmvilas deleted the unify-all-referencing-mechanisms branch January 18, 2023 15:19

magicmatatjahu mentioned this pull request Jan 28, 2023

feat: support AsyncAPI v3.0.0 asyncapi/converter-js#163

Closed

This was referenced Feb 6, 2023

feat: define new security mechanism asyncapi/spec-json-schemas#316

Merged

feat: channel's servers field allows references asyncapi/spec-json-schemas#318

Merged

jonaslagoni mentioned this pull request Mar 15, 2023

Spec 3.0 Meeting, 16:00 UTC Wednesday March 15th 2023 asyncapi/community#624

Closed

alequetzalli mentioned this pull request Mar 15, 2023

[📑 Docs]: Create docs for spec 3.0 release asyncapi/website#1433

Closed

9 tasks

smoya mentioned this pull request Mar 31, 2023

[2.0.0] Discussion how to release 2.0.0 parser-js asyncapi/parser-js#585

Closed

6 tasks

alequetzalli mentioned this pull request Apr 4, 2023

docs: unified referencing behavior (using $ref instead of string) asyncapi/website#1437

Closed

fmvilas mentioned this pull request Apr 4, 2023

Implement support for $ref in security object asyncapi/parser-js#746

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: unify all referencing mechanisms #852

feat: unify all referencing mechanisms #852

fmvilas commented Oct 13, 2022

sonarcloud bot commented Oct 13, 2022

fmvilas Oct 13, 2022

jonaslagoni Oct 14, 2022

magicmatatjahu Oct 17, 2022

magicmatatjahu Oct 17, 2022

fmvilas Oct 17, 2022

jonaslagoni Oct 20, 2022

fmvilas Oct 20, 2022

jonaslagoni Oct 20, 2022

fmvilas Oct 21, 2022

handrews Jan 10, 2023

fmvilas Oct 13, 2022

fmvilas Oct 13, 2022

fmvilas commented Dec 17, 2022 •

edited

handrews left a comment

handrews Jan 10, 2023

derberg left a comment

sonarcloud bot commented Jan 16, 2023

fmvilas commented Jan 16, 2023

fmvilas commented Jan 17, 2023

asyncapi-bot commented Jan 17, 2023

feat: unify all referencing mechanisms #852

feat: unify all referencing mechanisms #852

Conversation

fmvilas commented Oct 13, 2022

Description

Related issues

sonarcloud bot commented Oct 13, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fmvilas commented Dec 17, 2022 • edited

handrews left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

derberg left a comment

Choose a reason for hiding this comment

sonarcloud bot commented Jan 16, 2023

fmvilas commented Jan 16, 2023

fmvilas commented Jan 17, 2023

asyncapi-bot commented Jan 17, 2023

fmvilas commented Dec 17, 2022 •

edited