OpenAPI.NET icon indicating copy to clipboard operation
OpenAPI.NET copied to clipboard

Reference loading, the async challenge

Open darrelmiller opened this issue 8 years ago • 4 comments

Currently the DOM is designed so that strong types such as OpenAPISchema can be marked as a reference. This enables the model to know that these types are explicitly being re-used from the components collection.

Currently references are loaded immediately upon parsing a document so that references are always represented by the correct types and those instances contain all the referenced data. This is convenient when working with the object model as referenced objects and non-referenced can be interacted with transparently.

When it comes to loading references from external documents, this approach presents some challenges:

  • Loading from an external document should be async. Transparently loading external references would push async all the way through the model to the main load even if there were no external references in a particular document.
  • What if someone wanted to process a set of documents that contained external references but didn't need to load them to perform the updates? Should we not support loading documents without dereferencing external references? If so how should those references be represented in the model?
  • How do you construct a DOM with an external reference? Do you need an instance of the external DOM to do it?

It feels like we cannot get away from having a model type that can have a OpenApiReference instance but be in an "unresolved" state.

   var unresolvedSchema = new OpenApiSchema {
      Reference = new OpenApiReference("common.json#/components/schema/cat")
   };

Does this mean that we need to put a guard clause in every get accessor that prevents reading the regular properties? How do we construct a reusable object to be added to Components? Do we create it without the reference and then when adding it to the components collection a reference gets added automatically?

Or do we change the approach to the way we deal with references and instead of using the same object in the components collection and the reference to it, we create a new SchemaReference type that does aggregation and delegation.

e.g.

   content.Schema = new SchemaReference("cat",doc.Components.Schemas["cat"]);

This would require the SchemaReference to re-surface the entire Schema API. This would provide a nice place to put the guard against derefencing an unresolved reference, but it would also mean either creating an ISchema interface, or deriving from Schema and making all the properties virtual. Both of which are high maintenance approaches.

darrelmiller avatar Nov 27 '17 14:11 darrelmiller

Darrel,

  • I am trying to understand how bad it is to put in a "guard clause" in the accessor. This is similar to what all SerializeAsVX is doing - write Reference and nothing else if Reference is not null. But in general scenarios, the accessor can theoretically try to access any value, and it would first need to check if this is an unresolved schema (object), so the check would be more complicated. If we provide a flag in the Reference object or in the Schema object that determines whether the reference is resolved, that should make the guard clause simple with just one boolean check.

  • For Components, can we add the unresolved object there first? This should be just like a normal reusable object, and can be updated once we have the external file loaded. Would this work?

  • The alternative you proposed might be difficult to work with. I assume this means that in an object that contains a Schema object, that property would be declared as ISchema. When we need to use it as a reference, say to get the Reference property from that schema, it will require another conversion, which doesn't seem simple to me. Maybe I'm misunderstanding your proposal.

PerthCharern avatar Dec 01 '17 00:12 PerthCharern

@PerthCharern The guard clause isn't necessarily bad, it's just a whole lot of boilerplate code to maintain. Everywhere where we currently have just get;set property declarations would need to be fleshed out with an if statement.

If a component references an external file, then yes, we could defer resolving it. However, deferred resolution introduces the new problem of who manages the external files. During loading the ParsingContext could do that. If we defer loading, then we no longer have a ParsingContext.

If you look how XmlSchema does it, it has a notion of an XmlSchemaSet that holds all the documents that work together to provide a complete definition. I hadn't planned on taking that route, especially because an OpenAPI reference can point to a document that just contains a JSON/YAML fragment (I wish that wasn't the case, to be honest).

If we went with ISchema and SchemaReference object then I am assuming that it would still contain an property of type OpenAPIReference to identify the original source of the document.

Anyway, I will continue fighting with this. Thanks for your input.

darrelmiller avatar Dec 01 '17 02:12 darrelmiller

@darrelmiller

It's a little difficult for me to wrap my head around this without getting my hands on it, since there are many moving parts. I understand the concern about the manager of the reference file but I don't have any good suggestions at the moment.

To reduce # of guards, maybe we change the way we store any Referenceable object. Instead of

class OpenApiSchema : IOpenApiReferenceable {
   string Type,
   string Format,
   ...
   OpenApiReference Reference
}

, we can use

class OpenApiSchemaReference : IOpenApiReferenceable {
    OpenApiSchema Schema,
    OpenApiReference Reference
}

class OpenApiSchema {
   string Type,
   string Format,
   ...
}

The class SchemaReference goes into other classes instead of the Schema class.

Is this similar to what you suggested with SchemaReference? I still don't fully understand the exact picture of your suggestion.

PerthCharern avatar Dec 01 '17 18:12 PerthCharern

I made progress on this. Hopefully will be able to share something soon.

darrelmiller avatar Dec 04 '17 18:12 darrelmiller