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

Ambiguity in Operation.message field #588

Closed
char0n opened this issue Jul 26, 2021 · 17 comments · Fixed by #681
Closed

Ambiguity in Operation.message field #588

char0n opened this issue Jul 26, 2021 · 17 comments · Fixed by #681
Labels

Comments

@char0n
Copy link
Collaborator

char0n commented Jul 26, 2021

Hi everybody,

I've intercepted a possible ambiguity and un-clarity within the specification (2.1.0) regarding Operation.message fixed field.

Type of Operation.message fixed field is defined as [Message Object | Reference Object] which means that the field is going to be a list and either Message Object or Reference Object is allowed within it.

Inconsistent description

First sentence in the description then says A definition of the message that will be published or received on this channel. Other fields descriptions containing list of union types start with following sentence: A list of .... So I propose we change the first sentence of description to A list of Message Objects that will be published or received on this channel.

Allowing additional type in union not specified in Type field

Second sentence in description says: oneOf is allowed here to specify multiple messages. This has very serious implications. In order for this to be true we have to amend the Type of Operation.message to [Message Object | Reference Object | Schema Object]. From the perspective of usage this doesn't really make sense as we already have this mechanism in [Message Object | Reference Object]. I suggest to eliminate this sentence entirely to reduce complexity. The alternative ca be defining the type as [Message Object | Reference Object | Schema Object], but as argued there is no additional value in this.

Invalid example

This example contains invalid usage of Operation.message field. The value of the field in example is defined as Message Object. This shouldn't be possible according to the current spec.

{
  "operationId": "registerUser",
  "summary": "Action to sign a user up.",
  "description": "A longer description",
  "tags": [
    { "name": "user" },
    { "name": "signup" },
    { "name": "register" }
  ],
  "message": {
    "headers": {
      "type": "object",
      "properties": {
        "applicationInstanceId": {
          "description": "Unique identifier for a given instance of the publishing application",
          "type": "string"
        }
      }
    },
    "payload": {
      "type": "object",
      "properties": {
        "user": {
          "$ref": "#/components/schemas/userCreate"
        },
        "signup": {
          "$ref": "#/components/schemas/signup"
        }
      }
    }
  },
  "bindings": {
    "amqp": {
      "ack": false
    },
  },
  "traits": [
    { "$ref": "#/components/operationTraits/kafka" }
  ]
}

I can issue a PR with the changeset if we agree on these points.

@magicmatatjahu
Copy link
Member

magicmatatjahu commented Jul 26, 2021

Exactly, we have a bug with type of Operation.message field, but

So I propose we change the first sentence of description to A list of Message Objects that will be published or received on this channel.

we cannot use this description, because you can use here single Message Object or list of Message Objects, so:

A definition of single Message Object or list of Message Objects that will be published or received on this channel.
`oneOf` is allowed here to specify list of possible messages, however, a message MUST be valid only against one of the defined message objects.

would be better.

According to your suggestion with type:Message Object | Reference Object | Schema Object: it isn't correct, because using Schema Object you tell that you can also use the another keyword from Schema Object, like anyOf, allOf etc., which isn't true. I think that Message Object | Reference Object | { oneOf: [Message Object] } describe it clearer.

EDIT: { oneOf: [MessageObject] } should be described as Map["oneOf", [Message Object]]

@magicmatatjahu
Copy link
Member

magicmatatjahu commented Jul 26, 2021

On the other hand, I wonder why we are using the oneOf keyword here if we could use a normal array without using that keyword...

@fmvilas I need your help here. Why even from 1.1.0 version of spec we use the oneOf field to define multiple messages? Due to fact that message field can be extended by custom extension or another decision? Or maybe oneOf better describes that a message can be one of... in the case of a regular array this isn't so easy to understand?

@char0n
Copy link
Collaborator Author

char0n commented Jul 26, 2021

we cannot use this description, because you can use here single Message Object or list of Message Objects, so:

Then the Type definition should be Message Object | Reference Object | [Message Object | Reference Object] and then we need to extend the description field to say the it allows either list of Message Object or Message Object itself.

Map["oneOf", [Message Object]]

What would be the semantics of this? In order for this to be in spec, we have to define it's behavior. oneOf is a keyword from JSON Schema.

On the other hand, I wonder why we are using the oneOf keyword here if we could use a normal array without using that keyword...

IMHO it doesn't make any sense, as I already pointed out in my issue description and now you realizing it.

@magicmatatjahu
Copy link
Member

magicmatatjahu commented Jul 26, 2021

What would be the semantics of this? In order for this to be in spec, we have to define it's behavior. oneOf is a keyword from JSON Schema.

oneOf is already in spec, but we haven't any example of it in the spec, only a single difficult sentence to understand:

oneOf is allowed here to specify multiple messages, however, a message MUST be valid only against one of the referenced message objects.

Additionally, the use of the word oneOf doesn't indicate that it is a Scheme object. We can use the keyword oneOf to describe other logic.

Currently you can define message as:

message:
  payload: ...

or

message:
  oneOf:
    - payload: ... # message 1
    - payload: ... # message 2

so Map["oneOf", [Message Object | Reference Object]] is exactly the second case.

IMHO it doesn't make any sense, as I already pointed out in my issue description and now you realizing it.

TBH I don't know, maybe as I wrote, oneOf is easier to understand than the normal array itself, but let's wait for Fran's answer, because he defined it 2 years ago.

@char0n
Copy link
Collaborator Author

char0n commented Jul 26, 2021

oneOf is already in spec, but we haven't any example of it in the spec, only a single difficult sentence to understand:

Found an example here: https://github.com/asyncapi/spec/blob/master/spec/asyncapi.md#channel-item-object-example (third and forth example). I agree it's difficult to understand and figure it out, looking at currently defined Type.

TBH I don't know, maybe as I wrote, oneOf is easier to understand than the normal array itself, but let's wait for Fran's answer, because he defined it 2 years ago.

I assume that the intention was to indicate that only one of defined messages apply here and not all. Having it defined as an array ([Message Object | Reference Object]) doesn't indicate that. Let's wait for author to clarify.

@fmvilas
Copy link
Member

fmvilas commented Jul 26, 2021

Yes, the intention behind having oneOf is to indicate that the message will be one of the listed messages. The reason it's not an array is that we were considering adding anyOf as well, where the message must match 1 or more instances of the messages. With an array type, we could only do either oneOf or anyOf, therefore the verbosity.

I'd like to clarify that oneOf here is not taken from JSON Schema. Its name is the same but it's not implying you can use a Schema Object or a subset of it.

I think the definition should be something like Message Object | Reference Object | Map["oneOf", [Message Object | Reference Object]].

@char0n would you mind opening a PR clarifying it? Also explaining the meaning of oneOf so it's clearer.

@char0n
Copy link
Collaborator Author

char0n commented Jul 26, 2021

@fmvilas sure I'll handle it, now that I understand the original intention, it shouldn't be a problem.

@smoya
Copy link
Member

smoya commented Jul 27, 2021

I'd like to clarify that oneOf here is not taken from JSON Schema. Its name is the same but it's not implying you can use a Schema Object or a subset of it.

In practice for tooling implementations, can be translated to a Schema with only a oneOf field containing all those messages?

@magicmatatjahu
Copy link
Member

@smoya

In practice for tooling implementations, can be translated to a Schema with only a oneOf field containing all those messages?

I don't think so, because Message Object has different shape that JSON Schema. The behaviour is very similar, but another purpose. But to be clear: you can use the custom keywords inside JSON Schema so in theory you can use our Message Object, but from JSON Schema point of view, this use case is invalid.

@smoya
Copy link
Member

smoya commented Jul 27, 2021

I don't think so, because Message Object has different shape that JSON Schema

Sorry my bad. I meant for validating, to use a Schema with only a oneOf field containing all message payloads, which are actually Schemas.

@magicmatatjahu
Copy link
Member

You can do that, but this operation should retrieves all payloads from possible messages and concatenate them in the oneOf array. The other solution/use case I don't see.

@char0n
Copy link
Collaborator Author

char0n commented Jul 27, 2021

you can use the custom keywords inside JSON Schema so in theory you can use our Message Object, but from JSON Schema point of view, this use case is invalid.

Yep, exactly, JSON Schema should only include another JSON Schema; every unrecognized keyword is ignored. The situation changes in Draft 2020-12 where addition keywords can be given semantics by metaschemas. From the perspective of AsyncAPI spec it would be more than improper to include arbitrary object within JSON Schema, like Message Object; so we must clearly communicate that oneOf in this particular case is not a keyword from JSON Schema draft 07.

Sorry my bad. I meant for validating, to use a Schema with only a oneOf field containing all message payloads, which are actually Schemas.

I'm not sure this would be possible. oneOf MUST reference Message Objects and not one of its fields.

@char0n
Copy link
Collaborator Author

char0n commented Aug 4, 2021

PR provided: #603

@github-actions
Copy link

github-actions bot commented Oct 4, 2021

This issue has been automatically marked as stale because it has not had recent activity 😴
It will be closed in 60 days if no further activity occurs. To unstale this issue, add a comment with detailed explanation.
Thank you for your contributions ❤️

@github-actions github-actions bot added the stale label Oct 4, 2021
@char0n
Copy link
Collaborator Author

char0n commented Oct 4, 2021

The PR is waiting for approval. All the requested changes has been processed.

@derberg derberg removed the stale label Oct 4, 2021
@char0n
Copy link
Collaborator Author

char0n commented Jan 17, 2022

The PR has landed 2.3.0-2022-01-release.2. Should I close the issue or should I leave it to you (maintainers) to manage it's life cycle further?

@derberg
Copy link
Member

derberg commented Jan 17, 2022

hey, in theory, #681 should close it once merged and bot should add a comment here that it is included in 2.3 🤞🏼 😄

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

Successfully merging a pull request may close this issue.

5 participants