aj-foster/open-api-generator

Failure to properly resolve unreferenced schemas?

xadhoom opened this issue · 5 comments

When a schema is unreferenced into an operation response, the generator fails to properly define the expected response.

Consider the following spec Test Spec, where the User schema has been unreferenced in the operation response.

The generated operation is

  @spec get_users(keyword) :: {:ok, [map]} | {:error, S2SClient.Error.t()}
  def get_users(opts \\ []) do
    client = opts[:client] || @default_client

    client.request(%{
      call: {S2SClient.API, :get_users},
      url: "/api/v1/users",
      method: :get,
      response: [{200, {:array, :map}}],
      opts: opts
    })
  end

And the User schema is not generated at all into schemas.

After digging into the code, seems that all boils down to OpenAPI.Generator.Naming.schema_to_module/1 which matches only if the schema is a reference and not defined inline.

As a quick test I've added another clause:

  defp schema_to_module(%Schema{title: schema_name}) when is_binary(schema_name) do
    module =
      schema_name
      |> String.replace("-", "_")
      |> Macro.camelize()

    {:ok, {module, :t}}
  end

Which correctly generates the schema module if it has a valid title, and the resulting generated code is:

  @spec get_users(keyword) :: {:ok, [S2SClient.User.t()]} | {:error, S2SClient.Error.t()}
  def get_users(opts \\ []) do
    client = opts[:client] || @default_client

    client.request(%{
      call: {S2SClient.API, :get_users},
      url: "/api/v1/users",
      method: :get,
      response: [{200, {:array, {S2SClient.User, :t}}}],
      opts: opts
    })
  end

As a side effect, this also works if the schema is only defined in the response (with a valid title). After all the components.schemas is meant to hold reusable schemas, so If one schema is used only in one place I think is fine to generate a struct for it.

The only "drawback" I can see here is that a schema title is not defined as "unique" by the openAPI specification, so for locally-generated schemas no one guarantees that we may not have conflicts.

Good catch @xadhoom — I'd noticed this happening before but didn't look into the cause. In the case of non-unique titles, I wonder if there's a good way to merge the schemas (we would need to decide an alternative to :t as the subtype from the information available in the schema).

Well, I'm not sure that merging may be a good idea, because of the generic meaning of title in the spec.

The "title" after all is just a descriptive field, and nothing says that I cannot use the same title on all schemas. What is unique is just the operation id and the key of spec.components.schemas which hold the shared schemas.

When a schema is "local", ie declared in a operation, maybe the best approach is to prefix it with the operation id in someway? And yes we can use the title (if present) as struct name.

An additional complexity may arise when an operation declares multitple media types (for both request and response), and again we can define different schemas, even locally, even with same title, for different mediatypes.

I don't know what can be the pragmatic approach here. To be honest I still have to see an openAPI spec that uses something different from json, maybe we can ignore that for now?

Or use the media type along with the operation id to create the structure name.

From my example spec above, the nested User schema can be rendered as GetUsers.Json.User if we consider the media type or just GetUsers.User if we ignore that.

what do you think?

I agree that we can mostly ignore types other than JSON for now. The only other content types I have encountered so far are plain text and markdown, both of which returned plain strings and not objects. URL/form encoded data is the most likely type besides JSON to return an object, but I would be surprised if the same endpoint returned both.

We are mostly talking about local schemas defined in the responses of operations, but there are also schemas defined as inputs / request bodies. Do you agree that these could be output as @types rather than their own schema modules?

For local response schemas, I agree with creating a schema based on the operation name. It would be nice to find a way to organize these files so that operations and non-local schemas remain relatively easy to find in the code.

We are mostly talking about local schemas defined in the responses of operations, but there are also schemas defined as inputs / request bodies. Do you agree that these could be output as @types rather than their own schema modules?

Well, forgot about these. I expect if a request payload is defined locally (at least as an object), it should have a corresponding struct, like responses? what do you mean to output them as @type ?

For local response schemas, I agree with creating a schema based on the operation name. It would be nice to find a way to organize these files so that operations and non-local schemas remain relatively easy to find in the code.

Expanding on that maybe one way can be:

  • for local response scheme, output something like <Operation>.Response.LocalScheme
  • for local request scheme, output something like <Operation>.Request.LocalScheme
  • for request: LocalScheme can be inferred by the title, if present, otherwise fallback to just .Request` ? Can we have multiple request payloads?
  • for response: LocalScheme can be inferred by the title, if present, otherwise what can be done?

The files organization may follow standard practice, like creating subdirs following the module name, so if we have Operation.Response.LocalScheme is translated to operation/response/local_scheme.ex

Hello there 👋🏼

It took a long time, but I finally addressed generating schemas for response types and sub-fields. Release 0.1.0-rc.4 has some fairly significant changes to help with all of this. If you have a chance, please let me know how it works for you.