Skip to content
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: add missed "metadata" fields in the Server/Channel/Operation Objects #796

Merged

Conversation

magicmatatjahu
Copy link
Member

@magicmatatjahu magicmatatjahu commented May 12, 2022


title: add missed "metadata" fields in the Server/Channel/Operation Objects


Related issue(s):
Resolves #795
Part of #750


This PR introduces:

  • add title, summary, externalDocs fields in Server Object,
  • add title, summary fields in Channel Object,
  • add title field in Operation Object and Operation Trait Object,

cc @fmvilas @derberg @jonaslagoni @smoya @dalelane

spec/asyncapi.md Outdated

#### <a name="metadataObject"></a>Metadata Object

The object provides metadata about the described object. May contain previously non defined fields.
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have a problem with May contain previously non defined fields. sentence, because I don't know if we should allow only defining extensions and not non defined field. With this possibility there will be problems with validation on tooling side, by extension we can support it, by non defined fields no.

@magicmatatjahu magicmatatjahu changed the title chore: add metadata object chore: add metadata object, server trait and channel trait May 26, 2022
@magicmatatjahu
Copy link
Member Author

@fmvilas @jonaslagoni @jessemenning New way of applying Metadata Object was introduces alongside with Server and Channel Traits in latest commits. Feedback are more that welcome :)

@sonarcloud
Copy link

sonarcloud bot commented May 26, 2022

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
No Duplication information No Duplication information

@smoya
Copy link
Member

smoya commented Jun 17, 2022

I agree with some folks from https://www.youtube.com/watch?v=uoKEet0jcSc to not having a Metadata Object but have all those fields inline on those objects that don't have them.
Regarding implementation on the spec, I'm up for duplicating the fields on the objects rather than creating the new "Metadata Object in there and talking about inheritance. I think this will overcomplicate the spec and its readability, adding no real value to the final user.

What I'm concerned about right now is the fact name is not clear to me once we have ids on each Object. Ok, Channel doesn't have ID but could have it, so why would we want to keep name field anymore? Could we then deprecate it?

@magicmatatjahu
Copy link
Member Author

magicmatatjahu commented Jun 27, 2022

@smoya

What I'm concerned about right now is the fact name is not clear to me once we have ids on each Object. Ok, Channel doesn't have ID but could have it, so why would we want to keep name field anymore? Could we then deprecate it?

We don't have id on each object, please don't look on mentioned issue :)

Regarding implementation on the spec, I'm up for duplicating the fields on the objects rather than creating the new "Metadata Object in there and talking about inheritance. I think this will overcomplicate the spec and its readability, adding no real value to the final user.

For me it doesn't make the specification more complex, but let's see what other people say about it.

@smoya
Copy link
Member

smoya commented Jun 27, 2022

We don't have id on each object, please don't look on mentioned issue :)

By id I'm referring to operationId and messageId

@magicmatatjahu
Copy link
Member Author

magicmatatjahu commented Jun 27, 2022

@smoya

But operationId and messageId are unique id for object, name is a name, you can have multiple these same names in spec and it should be still valid. I don't see any problems with that.

@smoya
Copy link
Member

smoya commented Jun 27, 2022

@smoya

But operationId and messageId are unique id for object, name is a name, you can have multiple these same names in spec and it should be still valid. I don't see any problems with that.

Let's check the definition for messageId field at https://www.asyncapi.com/docs/reference/specification/v2.4.0#messageObject:

A machine-friendly name for the message.

Now let's think, Isn't the messageId right made for that purpose?

My suggestion is to not only not add the name field but to go further and deprecate it.

@magicmatatjahu
Copy link
Member Author

@smoya

Now let's think, Isn't the messageId right made for that purpose?

I don't think so. As I wrote, id should be one in spec, names you can have several. A good analogy would probably be a dictionary. You can only have unique keys but the given keys can have the same values. That's how I've always understood it.

@smoya
Copy link
Member

smoya commented Jun 27, 2022

@smoya

Now let's think, Isn't the messageId right made for that purpose?

I don't think so. As I wrote, id should be one in spec, names you can have several. A good analogy would probably be a dictionary. You can only have unique keys but the given keys can have the same values. That's how I've always understood it.

Is there a clear use case for declaring the same name on different messages?

@magicmatatjahu
Copy link
Member Author

@smoya I don't know. I'm not the creator of this spec :trollface:

@smoya
Copy link
Member

smoya commented Jun 27, 2022

@smoya I don't know. I'm not the creator of this spec :trollface:

Let's ping them cc @fmvilas.

@fmvilas
Copy link
Member

fmvilas commented Jul 4, 2022

I have a few concerns with this PR:

  1. This PR is doing too much. Please let's keep PRs focused on one thing at a time. Server and Channel traits have not much to do with the metadata object.
  2. I see the metadata object as premature optimization. We are prematurely assuming this information is common to multiple objects but the reality is that not all these fields apply equally to all the objects where metadata is applied. Instead, we're forcing the meaning and usefulness of the properties into the target object.
  3. I think the metadata object is not really providing any value and potentially causing harm (see point 2).
  4. I have the feeling that you're trying to define things in the wrong place. Especially, this: This object inherits the fields from [Metadata Object](#metadataObject). That sounds like you're defining how the parser should work. Like you're trying to apply programming concepts (i.e., inheritance) to a specification's text file. During the call you linked above, I explained how you can still have the "Metadata" object in your code and make everything else inherit it but I don't think this is necessarily useful for the user. It's actually worse because it's not clear at first sight that these objects can have name, description, etc.

@magicmatatjahu
Copy link
Member Author

@fmvilas thanks! So tl;dr; 😄

  • create separate PR for new traits,
  • add all fields from Metadata Object to the destination Objects?

@fmvilas
Copy link
Member

fmvilas commented Jul 4, 2022

@fmvilas thanks! So tl;dr; 😄

* create separate PR for new traits,

* add all fields from Metadata Object to the destination Objects?

Yes, but only the fields that make sense or are providing real value.

@jonaslagoni jonaslagoni mentioned this pull request Jul 18, 2022
@magicmatatjahu magicmatatjahu changed the title chore: add metadata object, server trait and channel trait feat: add missed "metadata" fields in the Server/Channel/Operation/Schema Objects Oct 3, 2022
@magicmatatjahu
Copy link
Member Author

Ok, traits removed, all fields from Metadata Object (which is also removed) are inlined, updated examples.

cc @derberg @fmvilas @dalelane

spec/asyncapi.md Outdated
@@ -371,32 +391,54 @@ Field Name | Type | Description
<a name="serverObjectUrl"></a>url | `string` | **REQUIRED**. A URL to the target host. This URL supports Server Variables and MAY be relative, to indicate that the host location is relative to the location where the AsyncAPI document is being served. Variable substitutions will be made when a variable is named in `{`braces`}`.
<a name="serverObjectProtocol"></a>protocol | `string` | **REQUIRED**. The protocol this URL supports for connection. Supported protocol include, but are not limited to: `amqp`, `amqps`, `http`, `https`, `ibmmq`, `jms`, `kafka`, `kafka-secure`, `anypointmq`, `mqtt`, `secure-mqtt`, `solace`, `stomp`, `stomps`, `ws`, `wss`, `mercure`, `googlepubsub`.
<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="serverObjectName"></a>name | `string` | A machine-friendly name for the server.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Isn't the server id (the key in servers object) already a machine-friendly name for the server? Why do we need one more property here? 🤔

Copy link
Member Author

@magicmatatjahu magicmatatjahu Oct 7, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for review! This is one field I wonder if it makes sense. Of course, we can treat the key in servers/channels/operations as an id/name of object, but in v3, servers/channels/operations/messages will be easily referenced, hence this id between references can change, and someone, for example, needs to give a "static" name for an object that will not change between refs. This makes a lot of sense for model generation and reusability of models.

If, on the other hand, the name doesn't make sense, we should probably remove it also from the Message Object - the messageId could be such a name for message. What do you think?

cc @derberg @jonaslagoni @smoya @dalelane

spec/asyncapi.md Outdated
@@ -612,12 +654,15 @@ Field Name | Type | Description
---|:---:|---
<a name="channelObjectAddress"></a>address | `string` \| `null` | An optional string representation of this channel's address. The address is typically the "topic name", "routing key", "event type", or "path". When `null` or absent, it MUST be interpreted as unknown. This is useful when the address is generated dynamically at runtime or can't be known upfront. It MAY contain [Channel Address Expressions](#channelAddressExpressions).
<a name="channelObjectMessages"></a>messages | [Messages Object](#messagesObject) | A map of the messages that will be sent to this channel by any application at any time. **Every message sent to this channel MUST be valid against one, and only one, of the [message objects](#messageObject) defined in this map.**
<a name="channelObjectName"></a>name | `string` | A machine-friendly name for the server.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same as with server above. Isn't the channel id already a machine-friendly name?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we end up leaving it, let's not forget to fix the typo:

Suggested change
<a name="channelObjectName"></a>name | `string` | A machine-friendly name for the server.
<a name="channelObjectName"></a>name | `string` | A machine-friendly name for the channel.

spec/asyncapi.md Outdated
@@ -612,12 +654,15 @@ Field Name | Type | Description
---|:---:|---
<a name="channelObjectAddress"></a>address | `string` \| `null` | An optional string representation of this channel's address. The address is typically the "topic name", "routing key", "event type", or "path". When `null` or absent, it MUST be interpreted as unknown. This is useful when the address is generated dynamically at runtime or can't be known upfront. It MAY contain [Channel Address Expressions](#channelAddressExpressions).
<a name="channelObjectMessages"></a>messages | [Messages Object](#messagesObject) | A map of the messages that will be sent to this channel by any application at any time. **Every message sent to this channel MUST be valid against one, and only one, of the [message objects](#messageObject) defined in this map.**
<a name="channelObjectName"></a>name | `string` | A machine-friendly name for the server.
<a name="channelObjectTitle"></a>title | `string` | A human-friendly title for the server.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
<a name="channelObjectTitle"></a>title | `string` | A human-friendly title for the server.
<a name="channelObjectTitle"></a>title | `string` | A human-friendly title for the channel.

spec/asyncapi.md Outdated
@@ -808,6 +855,8 @@ Field Name | Type | Description
---|:---:|---
<a name="operationObjectAction"></a>action | `string` | **Required**. Allowed values are `send` and `receive`. Use `send` when it's expected that the application will send a message to the given [`channel`](#operationObjectChannel), and `receive` when the application should expect receiving messages from the given [`channel`](#operationObjectChannel).
<a name="operationObjectChannel"></a>channel | [Reference Object](#referenceObject) | **Required**. A `$ref` pointer to the definition of the channel in which this operation is performed. Please note the `channel` property value MUST be a [Reference Object](#referenceObject) and, therefore, MUST NOT contain a [Channel Object](#channelObject). However, it is RECOMMENDED that parsers (or other software) dereference this property for a better development experience.
<a name="operationObjectName"></a>name | `string` | A machine-friendly name for the operation.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same as with server and channel above. Isn't it enough with the operation id?

Copy link
Member

@fmvilas fmvilas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM but you got conflicts.

@magicmatatjahu
Copy link
Member Author

@fmvilas Thanks for review! Done :)

@sonarcloud
Copy link

sonarcloud bot commented Nov 14, 2022

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
No Duplication information No Duplication information

Copy link
Collaborator

@dalelane dalelane left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM 👍

@@ -2316,7 +2362,7 @@ type: userPassword
```

```yaml
type: apiKey,
type: apiKey
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nice catch :-)

@magicmatatjahu magicmatatjahu changed the title feat: add missed "metadata" fields in the Server/Channel/Operation/Schema Objects feat: add missed "metadata" fields in the Server/Channel/Operation Objects Nov 22, 2022
@magicmatatjahu
Copy link
Member Author

magicmatatjahu commented Nov 22, 2022

@derberg @smoya @char0n I need one more approval :)

@magicmatatjahu
Copy link
Member Author

PR in the parser-js asyncapi/parser-js#677

@fmvilas
Copy link
Member

fmvilas commented Dec 7, 2022

@derberg @smoya @char0n can we help unlock this PR?

Copy link
Collaborator

@char0n char0n left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Nice work!

@magicmatatjahu
Copy link
Member Author

@derberg @smoya @char0n @fmvilas @dalelane Ok, I have 3 approvals. Someone from you, feel free to merge when you have time :) Thanks!

@magicmatatjahu
Copy link
Member Author

/rtm

@asyncapi-bot asyncapi-bot merged commit c46448f into asyncapi:next-major-spec Dec 19, 2022
@magicmatatjahu
Copy link
Member Author

Sorry! I had forgotten that automerge works here 😅

@magicmatatjahu magicmatatjahu deleted the next-major/metadata-object branch December 19, 2022 11:02
@asyncapi-bot
Copy link
Contributor

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

The release is available on GitHub release

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants