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: allow non-component references #132

Merged
merged 5 commits into from
Oct 20, 2023
Merged

feat: allow non-component references #132

merged 5 commits into from
Oct 20, 2023

Conversation

VisualBean
Copy link
Collaborator

No description provided.

@@ -67,9 +67,21 @@ public AsyncApiV2VersionService(AsyncApiDiagnostic diagnostic)
Id = reference,
};
}

var asyncApiReference = new AsyncApiReference();
if (reference.StartsWith("/"))
Copy link
Contributor

Choose a reason for hiding this comment

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

Probably worth noting that I believe the spec also supports "relative" paths. So they could conceivably start with . and .. as well.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This is already fully supported, gotta have to check for the case of having a fragment though.

Something like
./myjsonfile.json/fragment

that would simply fall under 'externalresource' and not isFragment: true

Copy link
Contributor

Choose a reason for hiding this comment

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

So, just to clarify, something like a schema stored as a yaml file separately using cloudevents or some other schema would be an externalResource and also a fragment. But a reference to an entire AsyncAPI doc would simply be an externalResource?

Copy link
Collaborator Author

@VisualBean VisualBean Oct 12, 2023

Choose a reason for hiding this comment

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

I think fragments are, when a subset is referenced inside another Json file. (Which is not what I stated above - it should have isFragment:true).

So jsonfile.json#/someLocation would be external AND a fragment
Like /whatever is the same, but in the same host document.

But I got dizzy from all of the RFCs involved.
I need to be determine 100% what "fragment" actually refers to.

I'll get back to you on that, once clarified, so we are 100% in line and on point regarding when, which is what.

Copy link
Collaborator Author

@VisualBean VisualBean Oct 12, 2023

Choose a reason for hiding this comment

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

After some investigating, ive come to the understanding that there are 3 types of references

  1. Fragments
    ./external-doc.json#/fragment different file document. (relative path)
    /fragments/myFragment same document.
    https://example.com/document.json#/fragment different external file document.
  2. Internal
    #/components/schemas/Pet (we already support those)
  3. 'Full' document Referenes
    ./external-doc.json We can choose to support this (although it technically does not align with the spec.')

and is a mix of
Json Reference
JSON Pointer
Uri

The main point is paragraph 3 and 4 of the Json Reference RFC

3. Syntax

A JSON Reference is a JSON object, which contains a member named
"$ref", which has a JSON string value. Example:

{ "$ref": "http://example.com/example.json#/foo/bar" }

If a JSON value does not have these characteristics, then it SHOULD
NOT be interpreted as a JSON Reference.

The "$ref" string value contains a URI [RFC3986], which identifies
the location of the JSON value being referenced. It is an error
condition if the string value does not conform to URI syntax rules.
Any members other than "$ref" in a JSON Reference object SHALL be
ignored.

4. Resolution

Resolution of a JSON Reference object SHOULD yield the referenced
JSON value. Implementations MAY choose to replace the reference with
the referenced value.

If the URI contained in the JSON Reference value is a relative URI,
then the base URI resolution MUST be calculated according to
[RFC3986], section 5.2. Resolution is performed relative to the
referring document.

If a URI contains a fragment identifier, then the fragment should be
resolved per the fragment resolution mechansim of the referrant
document. If the representation of the referrant document is JSON,
then the fragment identifier SHOULD be interpreted as a
[JSON-Pointer].

Copy link
Collaborator Author

@VisualBean VisualBean Oct 12, 2023

Choose a reason for hiding this comment

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

Ive pushed tests that should verify this behaviour.

So now it should be a matter of figuring out the Uri scheme, fetching the file/fragment accordingly, using the loader and insert values.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ok, interesting, definitely adds some clarity.

Especially so that "'Full' document References" aren't in this RFC because I've seen them used a lot on AsyncAPI slack to allow sharing of resources between OpenAPI schemas, AsyncAPI schemas and schema registry.

Is it significant that all the examples are to .json files? Or can we allow for .yaml as well?

Similar context in terms of spec intention vs facilitating actual usage: I'm never sure with this stuff because I'm sure there's a similar situation with servers where the AsyncAPI spec doesn't actually specify its contents can be $ref'd but I've seen a many members of the core AsyncAPI team themselves reffing the servers object in their config.

Copy link
Contributor

@dpwdec dpwdec Oct 13, 2023

Choose a reason for hiding this comment

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

Given the IReferenceLoader pattern on my PR we could actually just push this concern to the user. They can decide how they want to read when using external files. We could provide them with implementations for the canonical uses outlined. But if they want to do something extremely weird then they've got the interface to implement around their needs.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

My thoughts exactly

@@ -67,6 +95,21 @@ public void SerializeV2(IAsyncApiWriter writer)
writer.WriteEndObject();
}

private string GetExternalReferenceV2()
Copy link
Contributor

Choose a reason for hiding this comment

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

Not sure I understand what intention is with this method. If, its external why does it start pointing an id in components on the document?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

There is a case, where the reference is a full asyncapidocument, and we are pointing at a 'real' reference there.
So it sets up the Reference to match what the internal references look like, except it has the 'externalResource' set as the document path. made a test case for it

{
if (reference.IsExternal)
{
return new ()
Copy link
Contributor

Choose a reason for hiding this comment

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

So my understanding based on the comments on #129 is that this is where we would inject the code to read an external file or remote resource using the proposed ReferenceLoader?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

So basically take the reference loader in the ctor, and use that if isExternal. Yeah

@VisualBean VisualBean changed the title feat: allow any sort of reference (no external resolution) feat: allow any sort of reference Oct 20, 2023
@VisualBean VisualBean changed the title feat: allow any sort of reference feat: allow non-component references Oct 20, 2023
@VisualBean
Copy link
Collaborator Author

@dpwdec I will merge this - let me know if you have any trouble with it, in its current state

@VisualBean VisualBean merged commit 71fe571 into vnext Oct 20, 2023
9 checks passed
@VisualBean VisualBean deleted the alex/references branch October 20, 2023 12:11
@dpwdec
Copy link
Contributor

dpwdec commented Oct 20, 2023

@VisualBean Thanks! I haven't had time to review things properly yet and build off. Hopefully I'll have a chance next week to start working on referencing seriously. sorry for the delay.

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