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

Disable schemas by default; implement switch/property to intentionally enable them #237

Closed
aeworxet opened this issue Mar 26, 2024 · 8 comments

Comments

@aeworxet
Copy link
Collaborator

Clone of asyncapi/bundler#141 (comment)

When working on asyncapi/bundler#141

I tried the modified Optimizer (moveAllToComponents: true) on

this AsyncAPI Document, which is the result from the modified `Bundler`
asyncapi: 3.0.0
info:
  title: Comments Service
  version: 1.0.0
  description: This service is in charge of processing all the events related to comments.
servers:
  mosquitto:
    host: test.mosquitto.org
    protocol: mqtt
    tags:
      - name: env:production
        description: This environment is meant for production use case
      - name: kind:remote
        description: This server is a remote server. Not exposed by the application
      - name: visibility:public
        description: This resource is public and available to everyone
    bindings:
      mqtt:
        clientId: comment-service
channels:
  commentLiked:
    address: comment/liked
    messages:
      commentLiked:
        x-origin: ./messages.yaml#/commentLiked
        description: Message that is being sent when a comment has been liked by someone.
        payload:
          x-origin: ./schemas.yaml#/commentLikedPayload
          type: object
          title: commentLikedPayload
          additionalProperties: false
          properties:
            commentId:
              allOf:
                - type: string
                - description: Id of the comment that was liked
    description: >-
      Updates the likes count in the database and sends the new count to the
      broker.
  commentCountChange:
    address: comment/{commentId}/changed
    messages:
      commentChanged:
        x-origin: ./messages.yaml#/commentChanged
        description: Message that is being sent when a comment have been updated.
        payload:
          x-origin: ./schemas.yaml#/commentChangedPayload
          type: object
          title: commentChangedPayload
          additionalProperties: false
          properties:
            commentId:
              allOf:
                - type: string
                - description: >-
                    Id of the comment that was changed, such as when someone
                    liked it.
            likeCount:
              type: integer
              description: The new like count of how many have liked the comment.
    description: >-
      Sends the new count to the broker after it has been updated in the
      database.
    parameters:
      commentId:
        x-origin: ./parameters.yaml#/commentId
        description: ID of the comment
operations:
  receiveCommentLiked:
    action: receive
    channel:
      $ref: '#/channels/commentLiked'
    messages:
      - $ref: '#/channels/commentLiked/messages/commentLiked'
  sendCommentChange:
    action: send
    channel:
      $ref: '#/channels/commentCountChange'
    messages:
      - $ref: '#/channels/commentCountChange/messages/commentChanged'

and received

this
asyncapi: 3.0.0
info:
  title: Comments Service
  version: 1.0.0
  description: This service is in charge of processing all the events related to comments.
servers:
  mosquitto:
    $ref: '#/components/servers/mosquitto'
channels:
  commentLiked:
    $ref: '#/components/channels/commentLiked'
  commentCountChange:
    $ref: '#/components/channels/commentCountChange'
operations:
  receiveCommentLiked:
    $ref: '#/components/operations/receiveCommentLiked'
  sendCommentChange:
    $ref: '#/components/operations/sendCommentChange'
components:
  schemas:
    commentCountChange:
      x-origin: ./parameters.yaml#/commentId
      description: ID of the comment
    commentLiked:
      allOf:
        - $ref: '#/components/schemas/commentLiked'
        - $ref: '#/components/schemas/commentLiked'
    commentChangedPayload:
      x-origin: ./schemas.yaml#/commentChangedPayload
      type: object
      title: commentChangedPayload
      additionalProperties: false
      properties:
        commentId:
          $ref: '#/components/schemas/commentCountChange'
        likeCount:
          $ref: '#/components/schemas/commentCountChange'
    commentLikedPayload:
      x-origin: ./schemas.yaml#/commentLikedPayload
      type: object
      title: commentLikedPayload
      additionalProperties: false
      properties:
        commentId:
          $ref: '#/components/schemas/commentLiked'
  messages:
    commentChanged:
      x-origin: ./messages.yaml#/commentChanged
      description: Message that is being sent when a comment have been updated.
      payload:
        $ref: '#/components/schemas/commentChangedPayload'
    commentLiked:
      x-origin: ./messages.yaml#/commentLiked
      description: Message that is being sent when a comment has been liked by someone.
      payload:
        $ref: '#/components/schemas/commentLikedPayload'
  operations:
    receiveCommentLiked:
      action: receive
      channel:
        $ref: '#/channels/commentLiked'
      messages:
        - $ref: '#/channels/commentLiked/messages/commentLiked'
    sendCommentChange:
      action: send
      channel:
        $ref: '#/channels/commentCountChange'
      messages:
        - $ref: '#/channels/commentCountChange/messages/commentChanged'
  channels:
    commentCountChange:
      address: comment/{commentId}/changed
      messages:
        commentChanged:
          $ref: '#/components/messages/commentChanged'
      description: >-
        Sends the new count to the broker after it has been updated in the
        database.
      parameters:
        commentId:
          $ref: '#/components/schemas/commentCountChange'
    commentLiked:
      address: comment/liked
      messages:
        commentLiked:
          $ref: '#/components/messages/commentLiked'
      description: >-
        Updates the likes count in the database and sends the new count to the
        broker.
  servers:
    mosquitto:
      host: test.mosquitto.org
      protocol: mqtt
      tags:
        - name: env:production
          description: This environment is meant for production use case
        - name: kind:remote
          description: This server is a remote server. Not exposed by the application
        - name: visibility:public
          description: This resource is public and available to everyone
      bindings:
        mqtt:
          clientId: comment-service

then I turned off schemas, received

this
asyncapi: 3.0.0
info:
  title: Comments Service
  version: 1.0.0
  description: This service is in charge of processing all the events related to comments.
servers:
  mosquitto:
    $ref: '#/components/servers/mosquitto'
channels:
  commentLiked:
    $ref: '#/components/channels/commentLiked'
  commentCountChange:
    $ref: '#/components/channels/commentCountChange'
operations:
  receiveCommentLiked:
    $ref: '#/components/operations/receiveCommentLiked'
  sendCommentChange:
    $ref: '#/components/operations/sendCommentChange'
components:
  messages:
    commentChanged:
      x-origin: ./messages.yaml#/commentChanged
      description: Message that is being sent when a comment have been updated.
      payload:
        x-origin: ./schemas.yaml#/commentChangedPayload
        type: object
        title: commentChangedPayload
        additionalProperties: false
        properties:
          commentId:
            allOf:
              - type: string
              - description: >-
                  Id of the comment that was changed, such as when someone liked
                  it.
          likeCount:
            type: integer
            description: The new like count of how many have liked the comment.
    commentLiked:
      x-origin: ./messages.yaml#/commentLiked
      description: Message that is being sent when a comment has been liked by someone.
      payload:
        x-origin: ./schemas.yaml#/commentLikedPayload
        type: object
        title: commentLikedPayload
        additionalProperties: false
        properties:
          commentId:
            allOf:
              - type: string
              - description: Id of the comment that was liked
  operations:
    receiveCommentLiked:
      action: receive
      channel:
        $ref: '#/channels/commentLiked'
      messages:
        - $ref: '#/channels/commentLiked/messages/commentLiked'
    sendCommentChange:
      action: send
      channel:
        $ref: '#/channels/commentCountChange'
      messages:
        - $ref: '#/channels/commentCountChange/messages/commentChanged'
  channels:
    commentCountChange:
      address: comment/{commentId}/changed
      messages:
        commentChanged:
          $ref: '#/components/messages/commentChanged'
      description: >-
        Sends the new count to the broker after it has been updated in the
        database.
      parameters:
        commentId:
          x-origin: ./parameters.yaml#/commentId
          description: ID of the comment
    commentLiked:
      address: comment/liked
      messages:
        commentLiked:
          $ref: '#/components/messages/commentLiked'
      description: >-
        Updates the likes count in the database and sends the new count to the
        broker.
  servers:
    mosquitto:
      host: test.mosquitto.org
      protocol: mqtt
      tags:
        - name: env:production
          description: This environment is meant for production use case
        - name: kind:remote
          description: This server is a remote server. Not exposed by the application
        - name: visibility:public
          description: This resource is public and available to everyone
      bindings:
        mqtt:
          clientId: comment-service

and I have a feeling that users would like the latter more.

Thus, should schemas be turned off by default, and users would need to specifically ENABLE them, for example, with the command-line switch --include-schemas?
It may be the opposite way around, but I believe that the possibility of changing the appearance/invisibility of schemas should definitely exist.

Copy link

Welcome to AsyncAPI. Thanks a lot for reporting your first issue. Please check out our contributors guide and the instructions about a basic recommended setup useful for opening a pull request.
Keep in mind there are also other channels you can use to interact with AsyncAPI community. For more details check out this issue.

@aeworxet
Copy link
Collaborator Author

Should I include this behavior in the scope of asyncapi/bundler#141 because I think it would improve the UX?

@derberg
Copy link
Member

derberg commented Mar 26, 2024

IMHO and also from the perspective of reusability that we always promote -> we should have schemas there by default and if someone wants to have them directly in message.payload then they need to explicitly configure that.

@KhudaDad414
Copy link
Member

@aeworxet If I am being honest, how the schema currently works bothered me for some time. A lot of times it overdoes the re-use and the file becomes ugly to read for humans.

Maybe we can find some middle ground for how optimiser works with schemas.

  1. Focus schema optimization on top-level schemas like payloads and headers, and not get into the nested properties of each Schema.

  2. Introduce a threshold to determine if optimization is beneficial. we can decide that based on the number of fields in the schema object. for example, I wouldn't want:

    commentCountChange:
      x-origin: ./parameters.yaml#/commentId
      description: ID of the comment

to be optimised since it only has one field and by optimising it, we are making it harder to read.

  1. As I think @derberg means having exclude flags and having them on by default.

I am wondering what you think.

@aeworxet
Copy link
Collaborator Author

@derberg
I'm not talking about disabling schemas completely, I'm talking about giving users an option. So that when they run through Optimizer a dereferenced by Bundler AsyncAPI Document (#1 in #237 (comment),) they would get a predicted #3 and not an unpleasant surprise in the form of #2 without the possibility of disabling the schemas' injection.

Talking about: okay, schemas will be there by default, but there will also be an option noSchemas: true, and if users need a cleaner AsyncAPI Document without schemas injected all over it, they'll use it.

@KhudaDad414
Also talking about including the abovementioned behavior in Optimizer during resolution of asyncapi/bundler#141 because this idea is not about schemas themselves or how they work; it's about giving users a sane replacement for Bundler's referenceIntoComponents: true in order not to disappoint them and release 1.0.0 with such behavior.
And resolution of #88 will be included in 1.0.1 as a separate and independent issue.

THIS is what I mean.

@KhudaDad414
Copy link
Member

schemas will be there by default, but there will also be an option noSchemas: true, and if users need a cleaner AsyncAPI Document without schemas injected all over it, they'll use it.

I agree with this approach. having them on by default and having the option to disable it.

@derberg
Copy link
Member

derberg commented Apr 9, 2024

yup, agree with @KhudaDad414

@KhudaDad414
Copy link
Member

closing as resolved in #216.

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

No branches or pull requests

3 participants