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

[BUG] The bundle command does not bundle referenced files #1323

Open
2 tasks done
mfroberg opened this issue Apr 3, 2024 · 26 comments
Open
2 tasks done

[BUG] The bundle command does not bundle referenced files #1323

mfroberg opened this issue Apr 3, 2024 · 26 comments
Labels
bug Something isn't working

Comments

@mfroberg
Copy link

mfroberg commented Apr 3, 2024

Describe the bug.

The CLI tool now accepts asyncapi bundle for v3 documents (thanks to fix of issue #1137). However, it does not work as I expected, but of course I could be missing something.

I created a new default document with asyncapi new and moved the single message to a file in the same directory. Added a $ref in the main file. When I ran asyncapi bundle main.yaml on the new file, I expected it to merge the “message file”. But it didn’t.

main.yaml

asyncapi: 3.0.0
info:
  title: Account Service
  version: 1.0.0
  description: This service is in charge of processing user signups
channels:
  userSignedUp:
    address: user/signedup
    messages:
      UserSignedUp:
        $ref: '#/components/messages/UserSignedUp'
operations:
  onUserSignUp:
    action: receive
    channel:
      $ref: '#/channels/userSignedUp'
    messages:
      - $ref: '#/channels/userSignedUp/messages/UserSignedUp'
components:
  messages:
    UserSignedUp:
      $ref: './messages.yaml#/UserSignedUp'

messages.yaml

UserSignedUp:
  payload:
    type: object
    properties:
      displayName:
        type: string
        description: Name of the user
      email:
        type: string
        format: email
        description: Email of the user

Expected behavior

One yaml file containing the whole specification.

expected_bundle.yaml

asyncapi: 3.0.0
info:
  title: Account Service
  version: 1.0.0
  description: This service is in charge of processing user signups
channels:
  userSignedUp:
    address: user/signedup
    messages:
      UserSignedUp:
        $ref: '#/components/messages/UserSignedUp'
operations:
  onUserSignUp:
    action: receive
    channel:
      $ref: '#/channels/userSignedUp'
    messages:
      - $ref: '#/channels/userSignedUp/messages/UserSignedUp'
components:
  messages:
    UserSignedUp:
      payload:
        type: object
        properties:
          displayName:
            type: string
            description: Name of the user
          email:
            type: string
            format: email
            description: Email of the user

Screenshots

Screenshot

How to Reproduce

  1. Extract main.yaml and messages.yaml to the same directory
  2. In that directory, execute asyncapi bundle main.yaml

🥦 Browser

None

👀 Have you checked for similar open issues?

  • I checked and didn't find similar issue

🏢 Have you read the Contributing Guidelines?

Are you willing to work on this issue ?

None

@mfroberg mfroberg added the bug Something isn't working label Apr 3, 2024
Copy link
Contributor

github-actions bot commented Apr 3, 2024

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.

@francocm
Copy link

I know this is not too helpful, but wanted to share that I am observing the same problem here as well, using asyncapi/cli:1.8.4. The referenced files remain referenced, and not bundled up.

@peter-rr
Copy link
Member

@mfroberg @francocm
Thanks for reporting, I'm having a look at this 👀

@francocm
Copy link

francocm commented Apr 24, 2024

@Amzani and @peter-rr I've raised a draft PR that replicates this problem: #1389

I can try giving a look later at the root cause too, but in the meantime I hope that's helpful.

Also, existing assertions that validated the output spec were not actually asserting for to.be.true.

@peter-rr
Copy link
Member

@Souvikns @Shurtu-gal
The issue occurs just with v3 documents. I've been doing some research but haven't managed to find the root cause. Would you have any clue about it? 🤔

francocm added a commit to francocm/asyncapi-cli that referenced this issue Apr 26, 2024
francocm added a commit to francocm/asyncapi-cli that referenced this issue Apr 26, 2024
@francocm
Copy link

@peter-rr I've updated the related PR slightly regarding the way assertions are done, and also which referenced files etc are used for comparison.

I spent some time trying to get to the root cause, but I am suspecting it's originating from https://github.com/asyncapi/bundler. This repo uses 0.4.0. I noticed that 0.5.0 is available, but the upgrade to 0.5.0 is not straightforward. A lot of tests started failing after upgrading, as well as I noticed that 0.5.0 changed the signature and configuration options.

So this requires deeper investigation and also a plan for upgrade.

@peter-rr
Copy link
Member

peter-rr commented Apr 29, 2024

Hey @francocm , thanks for the updates on the PR 🙌 I'm having a look at them!

Yeah, the root cause must be definitely at the @asyncapi/bundler repo. Regarding the latest available version 0.5.0, it should be correctly updated at CLI repo since last week but there might be an issue related to the bot in charge of bumping to latest versions. Here is the info I have about it so you can take a look 👀 @Souvikns @Shurtu-gal @Amzani
I can also create a PR to update the version manually in case the auto-update is not working properly.

@Shurtu-gal
Copy link
Collaborator

Hey @peter-rr I am the one who reverted it. The bundler has to be migrated separately as there were some changes made in 0.5.0 due to which the bundle.command was not working properly.

Ref: #1372

@peter-rr
Copy link
Member

Cool @Shurtu-gal 👍 Thanks for the info!
Let me know if you need some help in order to solve the issue :)

@peter-rr peter-rr moved this from Backlog to Ready in CLI - Kanban May 2, 2024
@francocm
Copy link

francocm commented May 7, 2024

I have pulled from upstream into https://github.com/francocm/asyncapi-cli/tree/fix/1323/bundleNotBundlingExternalFiles

and went from 2 failing tests under test/integration/bundle/bundle.test.ts to 20, with the errors now including:

  20) validate
       with no context file
         throws error message if no context file exists:

      AssertionError: expected 'ContextError: No context is set as cu…' to equal 'error locating AsyncAPI document: The…'
      + expected - actual

      -ContextError: No context is set as current, please set a current context.
      +error locating AsyncAPI document: These are your options to specify in the CLI what AsyncAPI file should be used:
      + - You can provide a path to the AsyncAPI file: asyncapi <command> path/to/file/asyncapi.yml
      + - You can provide URL to the AsyncAPI file: asyncapi <command> https://example.com/path/to/file/asyncapi.yml
      + - You can also pass a saved context that points to your AsyncAPI file: asyncapi <command> context-name
      + - In case you did not specify a context that you want to use, the CLI checks if there is a default context and uses it. To set default context run: asyncapi config context use mycontext
      + - In case you did not provide any reference to AsyncAPI file and there is no default context, the CLI detects if in your current working directory you have files like asyncapi.json, asyncapi.yaml, asyncapi.yml. Just rename your file accordingly.
      +
      
      at Context.<anonymous> (test/integration/validate.test.ts:164:31)
      at Object.run (node_modules/fancy-test/lib/base.js:44:38)
      at Context.run (node_modules/fancy-test/lib/base.js:68:36)

It looks like this is a result of some error messaging/handling changes.

Nonetheless, with the state of 'broken' tests currently in master, these were not detected. I can refactor the tests to align with the updated output, but:

  1. I have not spent enough time in the recent changes to know for sure I'm not simply working around the problem rather than solving it.
  2. We would still end up with 2 broken tests (this bug), that the updated test/integration/bundle/bundle.test.ts captures.

I can't spend too much time on it right now, but happy to do any small changes in my branch if you wish.

Any thoughts @peter-rr @Shurtu-gal

@francocm
Copy link

francocm commented May 9, 2024

Thanks @aeworxet this issue seems to be resolved through your change.

I've noticed a new potential gap, which I've detailed in a comment in my tests PR here: #1389 (comment)

@peter-rr
Copy link
Member

@francocm @mfroberg
After the @asyncapi/bundler dependency upgrade, the file we are getting when running asyncapi bundle main.yaml looks like:

asyncapi: 3.0.0
info:
  title: Account Service
  version: 1.0.0
  description: This service is in charge of processing user signups
channels:
  userSignedUp:
    address: user/signedup
    messages:
      UserSignedUp:
        payload:
          type: object
          properties:
            displayName:
              type: string
              description: Name of the user
            email:
              type: string
              format: email
              description: Email of the user
operations:
  onUserSignUp:
    action: receive
    channel:
      $ref: '#/channels/userSignedUp'
    messages:
      - $ref: '#/channels/userSignedUp/messages/UserSignedUp'
components:
  messages:
    UserSignedUp:
      payload:
        type: object
        properties:
          displayName:
            type: string
            description: Name of the user
          email:
            type: string
            format: email
            description: Email of the user

Could you confirm this is the expected bundled file? I'm not sure since the messages.yaml content is shown duplicated, meaning the referenced object has been dereferenced twice after bundle process 🤔

@aeworxet
Copy link
Contributor

@peter-rr
Hey. Me again. 👋
I modified Optimizer (v1.0.0+) some time ago (simultaneously with Bundler v0.5.0) exactly for these cases.
After Optimizer v1.0.2 (current master) with

{
  output: 'YAML',
  rules: {
    reuseComponents: true,
    removeComponents: true,
    moveAllToComponents: true,
    moveDuplicatesToComponents: false,
  },
  disableOptimizationFor: {
    schema: true,
  },
}

the output file looks like this:

asyncapi: 3.0.0
info:
  title: Account Service
  version: 1.0.0
  description: This service is in charge of processing user signups
channels:
  userSignedUp:
    $ref: '#/components/channels/userSignedUp'
operations:
  onUserSignUp:
    $ref: '#/components/operations/onUserSignUp'
components:
  messages:
    UserSignedUp:
      payload:
        type: object
        properties:
          displayName:
            type: string
            description: Name of the user
          email:
            type: string
            format: email
            description: Email of the user
  operations:
    onUserSignUp:
      action: receive
      channel:
        $ref: '#/channels/userSignedUp'
      messages:
        - $ref: '#/channels/userSignedUp/messages/UserSignedUp'
  channels:
    userSignedUp:
      address: user/signedup
      messages:
        UserSignedUp:
          $ref: '#/components/messages/UserSignedUp'

Does this suit your needs better?

@KhudaDad414
How would the above option object look in the form of a command for asyncapi optimize? I haven't delved into that code yet.

@KhudaDad414
Copy link
Member

@aeworxet currently there is no way to ignore schemas in CLI. I have opened an issue here: #1432

btw, I don't think it's a good solution for the issue that @peter-rr reported. #1323 (comment)

The problem is that the bundler de-references the local reference in channels/userSignedUp/messages/UserSignedUp. not sure If this is an expected behaviour.

@aeworxet
Copy link
Contributor

@KhudaDad414

The problem is that the bundler de-references the local reference in channels/userSignedUp/messages/UserSignedUp. not sure If this is an expected behaviour.

I was going from the v3.0.0 Spec: channels/channel/messages/message is supposed to be dereferenced. 
Bundler does this and $ref: '#/channels/userSignedUp/messages/UserSignedUp' is pointing to the correct location at that time.
Then Optimizer moves channels/userSignedUp to components but doesn't add components to the $ref, which now should be $ref: '#/components/channels/userSignedUp/messages/UserSignedUp'.

What else should be changed after this issue is fixed?

currently there is no way to ignore schemas in CLI.

Maybe format of

disableOptimizationFor: {
  schema: true,
},

should be rethought in a way that makes both options (in standalone and CLI) consistent?

Like

{ disableOptimizationForSchema: true } // in the standalone Optimizer

--disableOptimizationForSchema // in CLI's command line, as the very presence of the cmd switch already
                               // means 'true' and its absence - 'false'

{ disableOptimizationForSchema: flags.disableOptimizationForSchema } // in CLI's 'optimize.ts'

?

@aeworxet
Copy link
Contributor

The issue with updating the $refs of the AsyncAPI Document after moving the AsyncAPI components to the components section by prefixing the corresponding JSON Pointers with the characters components/ will be solved by asyncapi/optimizer#263.

@KhudaDad414
Copy link
Member

I was going from the v3.0.0 Spec: channels/channel/messages/message is supposed to be dereferenced.

@aeworxet if we leave the optimizer out of the picture for now, which of these options should be the output of the bundler?

asyncapi: 3.0.0
info:
  title: Account Service
  version: 1.0.0
  description: This service is in charge of processing user signups
channels:
  userSignedUp:
    address: user/signedup
    messages:
      UserSignedUp:
        payload:
          type: object
          properties:
            displayName:
              type: string
              description: Name of the user
            email:
              type: string
              format: email
              description: Email of the user
operations:
  onUserSignUp:
    action: receive
    channel:
      $ref: '#/channels/userSignedUp'
    messages:
      - $ref: '#/channels/userSignedUp/messages/UserSignedUp'
components:
  messages:
    UserSignedUp:
      payload:
        type: object
        properties:
          displayName:
            type: string
            description: Name of the user
          email:
            type: string
            format: email
            description: Email of the user
asyncapi: 3.0.0
info:
  title: Account Service
  version: 1.0.0
  description: This service is in charge of processing user signups
channels:
  userSignedUp:
    address: user/signedup
    messages:
      UserSignedUp:
        $ref: '#/components/messages/UserSignedUp'
operations:
  onUserSignUp:
    action: receive
    channel:
      $ref: '#/channels/userSignedUp'
    messages:
      - $ref: '#/channels/userSignedUp/messages/UserSignedUp'
components:
  messages:
    UserSignedUp:
      payload:
        type: object
        properties:
          displayName:
            type: string
            description: Name of the user
          email:
            type: string
            format: email
            description: Email of the user

I think option 2 should be the result. as a user of bundler I don't expect bundler to de-reference my local references. If there is disagreement here, I think we should discuss this in a separate issue and see what the community wants.

should be rethought in a way that makes both options (in standalone and CLI) consistent?

Agreed.

@aeworxet
Copy link
Contributor

I'll ask here since this issue already has the intended audience.

Should there be a separate mode in Bundler that will dereference only external $refs, leaving internal $refs intact (previous behavior?)

@KhudaDad414
Copy link
Member

Should there be a separate mode in Bundler that will dereference only external $refs, leaving internal $refs intact (previous behavior?)

cc: @Souvikns @derberg @peter-rr @mfroberg @francocm

@mfroberg
Copy link
Author

I think it is reasonable use case to just dereference external refs and leave local ones intact. My original need is to get rid of the external refs (by bundling that content) so that I am able to visualize the file using Async Studio web tool.

@peter-rr
Copy link
Member

Should there be a separate mode in Bundler that will dereference only external $refs, leaving internal $refs intact (previous behavior?)

IMO, that should be the default behaviour. In case we want to de-reference also the local refs, then we could add an option to bundle command for that purpose.

@aeworxet
Copy link
Contributor

I will be able to implement bundle after the onBundle callback with the similar functionality as onDereference is introduced in @apidevtools/json-schema-ref-parser (I still need to do stuff after bundling and keep cmd switches consistent across versions.)

@Amzani Amzani moved this from Ready to In progress in CLI - Kanban May 23, 2024
Copy link
Contributor

This issue has been automatically marked as stale because it has not had recent activity 😴

It will be closed in 120 days if no further activity occurs. To unstale this issue, add a comment with a detailed explanation.

There can be many reasons why some specific issue has no activity. The most probable cause is lack of time, not lack of interest. AsyncAPI Initiative is a Linux Foundation project not owned by a single for-profit company. It is a community-driven initiative ruled under open governance model.

Let us figure out together how to push this issue forward. Connect with us through one of many communication channels we established here.

Thank you for your patience ❤️

@github-actions github-actions bot added the stale label Sep 20, 2024
@peter-rr
Copy link
Member

peter-rr commented Sep 26, 2024

Still relevant.

Waiting for #1389 to be solved.

@github-actions github-actions bot removed the stale label Sep 27, 2024
@zbigniew-malcherczyk-tg

I'm looking forward to the fix as well. Currently it also kills the https://github.com/event-catalog/eventcatalog asyncapi plugin

@aeworxet
Copy link
Contributor

@zbigniew-malcherczyk-tg
Hey. I am one of Bundler's maintainers. Could you please point to the open issue?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
Status: In progress
Development

No branches or pull requests

7 participants