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

Inlining in 1.3 is only partially useful #805

Closed
darrelmiller opened this issue Mar 27, 2022 · 5 comments
Closed

Inlining in 1.3 is only partially useful #805

darrelmiller opened this issue Mar 27, 2022 · 5 comments
Assignees
Labels
type:enhancement Enhancement request targeting an existing experience
Milestone

Comments

@darrelmiller
Copy link
Member

Inline local works, but in the process you lose the name of the component and therefore passing this code generator will end up generating a different type for every usage. It should be useful for scenarios like Power Platform though.

Inline external will pull in components from external files. However, if those components have local references in the external file, those local references don't get brought into the inlined file. Merging of components during writing seems like a bad approach. Having the writer render a reference either as a $ref or as the complete object seemed fairly harmless for a writer as it did not change the semantics of the document significantly. However, merging components is a much bigger task. e.g. What if there is a naming conflict?

In v2 of OpenAPI.NET reference objects and target components are going to be distinct concepts and therefore changing which will be rendered on the fly is going to be too much responsibility for the writer. I suspect the alternative will be to use a visitor to inline references. This will establish the precedent of inlining being a mutation to the document. At that point we can do things like merge the components of external files.

In the meanwhile, if you want to inline external files successfully, you will need to inlineLocal and inlineExternal.

@darrelmiller darrelmiller added the type:enhancement Enhancement request targeting an existing experience label Mar 27, 2022
@darrelmiller darrelmiller added this to the 2.0 milestone Mar 27, 2022
@darrelmiller
Copy link
Member Author

@MaggieKimani1 @irvinesunday @baywet

As a follow up to our conversation on Tuesday, I have a proposal.

  • Creating an OpenAPIDocument via a public ctor will always create an OpenAPIWorkspace. That document will be considered an "entry" document.
  • All model objects will have an optional ctor parameter called hostDocument of type OpenAPIDocument
  • If the model object is instantiated without a hostDocument, then reference resolution will fail. That's OK.
  • The OpenAPIWorkspace will own an instance of a "ComponentRegistry" for any component that exists in any document in the OpenAPIWorkspace.
  • Having a ComponentRegistry per OpenAPIWorkspace allows us to avoid using the global SchemaRegistry and keeps JsonSchema.Net's registry as an implementation detail.
  • When parsing an entry document. All components must be registered in the ComponentRegistry with a URI as the identifier. i.e. We should treat all components like JsonSchema.Net treats Schemas. This also allows us to treat tags and security schemes in a way that is consistent with how $id works for Schemas.
  • After parsing the entry document, if reader settings indicate that external references should be loaded, we will walk the entry document looking for external references. All documents that host external references will be loaded into the same OpenAPIWorkspace and their components will be registered in the same component registry.
  • Non-schema Components will be registered with a URI that is unique to the hostdocument and the type of component to avoid clashes.
  • Attempts to resolve a reference should call a resolve method on the host document with the local identifier. The host document should process the reference based on its identity and then call the ComponentRegistry in the OpenAPIWorkspace to return the component.
  • This means we are using $ref values as keys into the component registry. We are no longer going to use them as JSON Pointer locators.

This approach will allow us to efficiently support fragment references which we have never been able to do before. This is important to Microsoft because Azure OpenAPI descriptions use fragment references quite often and referencing JSONSchema files is quite common.

  • External references that are identified as fragment references can load the external document in a special OpenApiFragmentDocument and parsed as a tree of JsonNodes. The targeted fragment can be deserialized into a model object and added as a Component to the OpenAPIWorkspace component registry.
  • Future external fragment references that target a document that has already been parsed as an OpenApiFragment document can reuse the existing loaded document and add new Components to the registry. This avoids the problem of trying to parse non-OpenAPI fragment documents. This will allow pointing to JSON Schema documents and not have to reload the JSON Schema file on every fragment reference.

This approach sets us up for success with the current plans for moonwalk. Moonwalk is trying to move to a model where external components are imported from documents within defined namespaces. The OpenAPI document then uses the namespace qualified component name to reference the component from external files. Moonwalk will be able to use the component registry in the OpenAPIWorkspace to hold the list of namespace qualified components.

An open item to discuss is the structure of the URI for identifying components. Here's one suggestion...

The documentId could just be a GUID that we generate upon creation of an OpenAPIDocument. We also have to decide if $Id values replace the entire URL or if they are encoded into the {name} value.

@baywet
Copy link
Member

baywet commented Feb 26, 2024

Since you always create a workspace? Why not pass the workspace around instead and delegate ALL reference resolution to the workspace? (local, remote, etc...)
This way you'd have a single control logic, which also happens to be the place where you can load additional documents.
The only drawback I can see from doing that are:

  • back references (but your proposition was doing the same)
  • async propagation on the API surface (same rationale)

@darrelmiller
Copy link
Member Author

@baywet That was my first thought too. However, we need to qualify the identifier of the reference with an id of the document because you could have two references in two different documents that have the same $ref value but point to completely different objects.

@darrelmiller
Copy link
Member Author

I was reminded by @irvinesunday that proxy reference objects already accept the hostdocument so there may be no need to add hostDocument as an optional constructor to all the base models.

@irvinesunday irvinesunday self-assigned this Mar 5, 2024
@irvinesunday
Copy link
Contributor

irvinesunday commented Mar 5, 2024

I just want to share initial code snippets on how I think we can go about this:

In the workspace, we register two registry dictionaries:

private IDictionary<string, IOpenApiReferenceable> _referenceableRegistry = new Dictionary<string, IOpenApiReferenceable>();
private IDictionary<string, IBaseDocument> _schemaRegistry = new Dictionary<string, IBaseDocument>();

Then have two overloaded methods to register components with unique keys (should the key be Uri or uri string?)

  1. Overloaded method that registers JsonSchema
public void RegisterComponent(Uri uri, IBaseDocument baseDocument)
{
    // Check nullability of uri
    // Check nullability of baseDocument
   
    if (_schemaRegistry.ContainsKey(uri.ToString()))
    {
        throw new InvalidOperationException($"Key already exists. {nameof(uri)} needs to be unique");
    }
    else
    {
        _schemaRegistry.Add(uri.OriginalString, baseDocument);
    }
}
  1. Overloaded method that registers OpenApiReferenceable components.
public void RegisterComponent(Uri uri, IOpenApiReferenceable referenceable)
{
    // Check nullability of uri
    // Check nullability of referenceable

    if (_referenceableRegistry.ContainsKey(uri.OriginalString))
    {
        throw new InvalidOperationException($"Key already exists. {nameof(uri)} needs to be unique");
    }
    else
    {
        _referenceableRegistry.Add(uri.OriginalString, referenceable);
    }
}

And a generic method to retrieve a component value from either of the two registries:

public bool TryRetrieveComponent<TValue>(Uri uri, out TValue value)
{
    if (uri == null)
    {
        value = default;
        return false;
    }
    
    if ((typeof(TValue) == typeof(IBaseDocument)))
    {
        _schemaRegistry.TryGetValue(uri.OriginalString, out IBaseDocument schema);
        if (schema != null)
        {
            value = (TValue)schema;
            return true;
        }
    }
    else if(typeof(TValue) == typeof(IOpenApiReferenceable))
    {
        _referenceableRegistry.TryGetValue(uri.OriginalString, out IOpenApiReferenceable referenceable);
        if (referenceable != null)
        {
            value = (TValue)referenceable;
            return true;
        }
    }

    value = default;
    return false;
}

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type:enhancement Enhancement request targeting an existing experience
Projects
None yet
Development

No branches or pull requests

4 participants