open-api-spex / open_api_spex

Open API Specifications for Elixir Plug applications
Mozilla Public License 2.0
704 stars 184 forks source link

Ref #352 - Schema's being cached by 'title' causes malformed models #490

Open mtraynham opened 2 years ago

mtraynham commented 2 years ago

In #352, schemas are being cached to the spec's schema dictionary, so that subsequent lookups can be used to prevent recursive circular generation.

Schema.title is somewhat of an open-ended field according to the Spec. There's no restrictions on what can go into it.

Now that this is in place, and title is being used as the reference for schema, references might no longer be valid URI identifiers, https://datatracker.ietf.org/doc/html/draft-pbryan-zyp-json-ref-03#page-3.

If you attempt to use schemas with Open API Generator, it will start to complain with:

-attribute components.schemas.Schema name Foo Bar doesn't adhere to regular expression ^[a-zA-Z0-9.-_]+$

A cursory glance at this, it seems kind of wrong to use the Title for schema reference. A title & associated schema in one context could be completely different from a title & associated schema in another. For example, consider two models with properties that share the same Title.

Car -> Wheel Type Train-> Wheel Type

In this particular case, because the model Wheel Type is cached, if Car was processed first, Train would use the Schema of Car -> Wheel Type.

If circular references are an issue, maybe a custom identifier field would better resolve the problem?

This also floods the schema dictionary with properties of objects that may be simple types, such as strings. Maybe a secondary object not output with the schema would be a better source of cache. image

mbuhot commented 2 years ago

Yes, title is a hack going right back to the first public commit in OpenApiSpex :) https://github.com/open-api-spex/open_api_spex/blob/22fcbcd8421f826a721abe519b46659fea32f9fc/lib/open_api_spex/schema_resolver.ex#L118-L129

We could introduce a second callback to the Schema behaviour, eg @callback key() :: String.t() that will be used to build the path to the schema within the document and for cache lookups.

For compatibility it could default to title.

Even with a custom callback, it might be helpful to validate that two schema modules don't use the same key and that all keys result in compliant schema references.

vrcca commented 1 year ago

Hey, @mbuhot! One problem we are facing with this piece of code is that when someone declares two schemas with the same title, only one is picked. Do you know if there is a way to avoid this? 👀

mbuhot commented 1 year ago

@vrcca Not sure if there is any way to avoid this without updating OpenApiSpex to use a different identifier for placing schemas in the components/schemas object and generating JSON schema references.

You could try explicitly adding your schemas to the OpenApi document:

 defmodule MyAppWeb.ApiSpec do
  def spec do
    api = %OpenApiSpex.OpenApi{
      paths: OpenApiSpex.Paths.from_router(MyAppWeb.Router)
    }
    api = put_in(api.components.schemas["car_wheel"], MyApp.Schemas.CarWheel.schema())
    api = put_in(api.components.schemas["train_wheel"], MyApp.Schemas.TrainWheel.schema())
    OpenApiSpex.resolve_schema_modules(api)
  end
end

Then use explicit schema references rather than module names when referring to schemas:

%Reference{"$ref": "#/components/schemas/train_wheel"}