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

DRAFT - feat: Add local external file refs #129

Closed
wants to merge 8 commits into from

Conversation

dpwdec
Copy link
Contributor

@dpwdec dpwdec commented Oct 9, 2023

About the PR

Currently, referenced objects in the AsyncAPI spec when read by the AsyncAPI.NET library can only be parsed using internal document references to the components object. While this is very useful, ideally we would utilise the full $ref functionality afforded to us by referencing external files, both locally and remotely. The advantages of facilitating references outside of a single AsyncAPI file are myriad, but most importantly it:

  • allows for re-use of referenced objects between AsyncAPI service files, for example, two services wanting to consume the same message can directly $ref a producer's definition of their message without need for duplication
  • allows for AsyncAPI files to be defined modularly according to users needs as projects defined using AsyncAPI scale

This PR aims to implement the functionality for allowing referencing of local references, i.e. other AsyncAPI objects defined in YAML that exist on the same local file system.

Challenges to solve

  • How can we facilitate allowing users to pass in their own file reader as a settings option so as to make the resolving of file paths agnostic within the project itself?
  • How can we propagate settings of readers down to external readers?
  • Do we need to generalise the patterning that we use for defining external references to lower code duplication?

Changelog

TODO

Related Issues

TODO

@github-actions
Copy link

github-actions bot commented Oct 9, 2023

We require all PRs to follow Conventional Commits specification.
More details 👇🏼

 No release type found in pull request title "DRAFT - feat: Add local external file refs". Add a prefix to indicate what kind of release this pull request corresponds to. For reference, see https://www.conventionalcommits.org/

Available types:
 - feat: A new feature
 - fix: A bug fix
 - docs: Documentation only changes
 - style: Changes that do not affect the meaning of the code (white-space, formatting, missing semi-colons, etc)
 - refactor: A code change that neither fixes a bug nor adds a feature
 - perf: A code change that improves performance
 - test: Adding missing tests or correcting existing tests
 - build: Changes that affect the build system or external dependencies (example scopes: gulp, broccoli, npm)
 - ci: Changes to our CI configuration files and scripts (example scopes: Travis, Circle, BrowserStack, SauceLabs)
 - chore: Other changes that don't modify src or test files
 - revert: Reverts a previous commit

@dpwdec dpwdec changed the title feat: Add local external file refs DRAFT - feat: Add local external file refs Oct 9, 2023
if (pointer.StartsWith("#")) return mapNode.GetReferencedObject<AsyncApiMessage>(ReferenceType.Message, pointer);

// how can we pass in settings and options for user to inject their own file reader
// is version matching important?
Copy link
Collaborator

@VisualBean VisualBean Oct 9, 2023

Choose a reason for hiding this comment

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

Version matching becomes important once we need support for both V2 and V3 which is coming very soon.
But important only on the read side, as we aren't looking to support writing to both version, but rather "upgrade" older versions to the newest.
I see, so multiple asyncapi structures within a single file.

return mapNode.GetReferencedObject<AsyncApiBindings<IChannelBinding>>(ReferenceType.ChannelBindings, pointer);
}

var referencedContent = File.ReadAllText(pointer);
Copy link
Collaborator

@VisualBean VisualBean Oct 10, 2023

Choose a reason for hiding this comment

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

I dont think the right place to implement this, would be here in the deserializers, but rather a mix of GetReferencedObject and the AsyncApiReferenceResolver.

There is also a method in the DocumentReader called "ResolveReferences" which is the beginning of the reference resolution story. - this effectively walks all of the references.
What you will notice is that AsyncApiDocument uses AsyncApiReferenceResolver - which might also be helpful in figuring out the resolution map up front.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@VisualBean Will do. Thanks for the context!

Copy link
Collaborator

@VisualBean VisualBean Oct 11, 2023

Choose a reason for hiding this comment

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

@dpwdec ive made some initial digging into this (just allowing any type of reference and correctly setting the values) without resolving anything (just marking them as unresolved to begin with).

I would also really appreciate if work could be done against the vnext branch, as this has some breaking changes (like changing from yamldotnet to system.text.json)

see #132.

resolving non internal references should feasibly happen in the jsondocumentreader

@VisualBean VisualBean marked this pull request as draft October 12, 2023 10:24
@VisualBean
Copy link
Collaborator

VisualBean commented Oct 12, 2023

A few thoughts ive had

How can we facilitate allowing users to pass in their own file reader as a settings option so as to make the resolving of file paths agnostic within the project itself?

Perhaps an IReferenceLoader could be used as a part of the AsyncApiReaderSettings() coupled with the ReferenceResolution enum (add another one for resolving 'all' references or whatever).

A default ReferenceLoader could be made that fetches http and local fragments.

The current resolution flow 'ends' with the following method in the AsyncApiReferenceResolver

private T ResolveReference<T>(AsyncApiReference reference) where T : class, IAsyncApiReferenceable
{
    try
    {
        return this.currentDocument.ResolveReference(reference) as T;
    }
    catch (AsyncApiException ex)
    {
        this.errors.Add(new AsyncApiReferenceError(ex));
        return null;
    }
}

Which could feasibly check if the reference IsExternal and apply the loader accordingly.
So basically feeding the ReferenceLoader in through the document.ResolveReferences() call from the Reader and down to the AsyncApiReferenceResolver

Do we need to generalise the patterning that we use for defining external references to lower code duplication?

I believe i have solved this in another PR

@VisualBean VisualBean force-pushed the main branch 3 times, most recently from 5ebfbf8 to 9063f4e Compare February 27, 2024 09:06
@dpwdec dpwdec closed this Apr 19, 2024
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.

2 participants