Proposal: Advanced Field Type Annotations

[aj-foster]

I'm looking for feedback on top topic of how we represent field types in the generated code. Truthfully, I'm not satisfied with any of the proposals below, so ideas are very welcome!

Problem

Today, field types in the generated code are expressed using tuples:

  @type t ::
          :binary
          | :boolean
          | :integer
          | :map
          | :number
          | :string
          | :null
          | :unknown
          | {:array, t}
          | {:union, [t]}
          | {:nullable, t}
          | {module, atom}

This represents the primitive types available in OpenAPI and several of the common container types (array, union, etc.). It also makes nullability easy to pattern match. Unfortunately, it doesn't represent all of the data available in the spec:

• String format information, like dates and times
• Enums with an array of possible values
• Required vs. non-required fields

This information could be useful for clients, especially when additional processing needs to take place (like with datetimes).

Proposals

I propose a breaking change to the way we represent field types in the generated schemas. I have the following goals in mind:

1. Keep code succinct and elegant
2. Make it easy to pattern match on each aspect of the type, including nullability, requiredness, and format
3. Preferably do not introduce any additional imports or aliases in the schema modules, though this is okay if necessary

For easy pattern matching, maps are often helpful because the order of the keys does not matter:

def my_function(%{type: :string, nullable: false, required: true}), do: ...

However, we then have to consider whether the keys need to always be present (like a struct) or if we will sometimes have a type that is simply %(type: :string}. It isn't possible to match on the non-existence of keys, so it seems likely that every map will need the same keys — hence, a struct. But this could get cumbersome:

%{type: :union, nullable: false, required: true, values: [%{type: :string}, ...]}

Perhaps, then, some containers should remain tuples:

{:union, [%{type: :string, enum: true, values: ["one", "two"]}, ...]}

This brings up another option: keep using tuples like we are, but with an additional element that contains metadata as a map.

{:union, [{:string, %{enum: ["one", "two"]}}, ...], %{required: true}}

This would allow for pattern matching while keeping the code slightly more succinct, but still runs into the questions of whether we always put the same keys in the metadata.

[feynmanliang]

Pardon my ignorance, but what is the use case for pattern matching on field types?

[paulbalomiri]

Pattern matching with a map is, i think "the Elixir way" to match parameters. How about splitting the parameters into required and not required params, where opts would hold any optional params, regardless of location. All required and defaulted params are then kept in a struct, like so:

@doc """
...

### Argument binding
The doc should have a chapter appended created by the generator:
The request is sent using `GET` method using the path `/operation/:req_path_param`, 
* Arguments
  * `req_query_param`: **required**, sent by query `req-query-param`
  * `req_cookie_param`: **required**, sent by cookie `REQ-cooKie-PARAM`
  * `req_path_param`: **required, sent by parameter `:req-path-param` in path
  * `req_defaulted_param`: optional, defaults to `"some default from spec"` sent via header `X-Req-Defaulted-Param`
* Options
  * `some_optional_header_parameter`: sent via header `X-Some-Optional-Header-Parameter`  
 ...
"""

@operation_defaults %{
  req_defaulted_param: "some default from spec"
}
# this clause contains only required, but not defaulted params
def operation(%{
    req_path_param:  req_path_param,
    req_query_param:  req_query_param,
    req_cookie_param: req_query_param
  }=params, opts)
do
  @operation_defaults
  |>Map.merge(params)
  |> operation(
       opts
       # This saves the info about which params have been defaulted 
       |> Keyword.put(:defaulted_params, Map.keys(@operation_defaults)  -- Map.keys(params)  ) 
   )
end
# this clause pattern matches all required and all defaulted arguments
def operation(%{ 
  req_defaulted_param: req_defaulted_param ,
  req_path_param:  req_path_param,
  req_query_param:  req_query_param,
  req_cookie_param: req_query_param
},
opts
) do
# ... skipping the param transformation for clarity & going directly to `client.request(%{...})`
client.request(%{
    # parameters is replacing the args field here
    parameters : %{
       # this is keyed by Processor.Operation.Param.location()
       path: [req_path_param: req_path_param],
       cookie:  [ req_cookie_param: req_cookie_param ],
       header: [req_defaulted_param: req_defaulted_param],
       query:  [req_query_param: req_query_param]
     }
   
    call: {Operations, :operation},
    url: "/operation",
    method: :get,
    # :headers field should become part of the :parameters field (:header)
    #headers: header_parameters,
    response: [
     #...
    ],
    opts: 
     opts
     |> Keyword.put(
         :parameter_types,  %{             
            req_path_param: {:string, :generic, spec_key: {:path, ":req-path-param"}, ...]},
            req_defaulted_param: {:string, generic, spec_key: {:header, "X-Req-Defaulted-Param"}, default:  "some default from spec"}
            ... # not expanding now on this
          })
    })
end

What do you think abt. this?
I know & presume there is a lot this snippet left out, especially:

• handling of body params: are there cases (e.g. required params) where merging of required args and body args is preferable to a separate argument? What do we do with optional body arguments ....
• the parameter_types option proposed in the snippet which would make it possible to infer types to input maps, so as to be more lenient towards the lib user. With this info the user could supply the argument value in any format the client understands (e.g. Map with keys instead of struct)
• making sure that all atom keys are sent together with the string key from the spec. (e.g. this PR does this by supplying header params in {"X-Spec-key", "string-value"} Implementation proposal for operation parameters with :cookie and :header location and test case for rendered code #28 )
  • Also, on this point: the mapping between atoms and spec key is not sent to the client. should it be part of something like :parameter_types using the :opts key ?
• EDIT:: a heads up: I have not compiled the snippet 😃 . I can do this if the discussion does expand around it.

Initially i wanted to expand on the code snippet, but i think that the snippet is more concise, than redoubling with words (and losing clarity 🤔).

Are you open to a discussion about this? I think that it's only slightly off topic, as the way advanced type annotations should be sent is contingent on how to capture them and supply them in most useful format to the client.

I have already written a dirty POC of a client with tesla using middlewares, but I'm leaning to writing a tesla client where each parameter location (as in spec) gets it's own middleware. I can publish and expand on this if there's interest. For my project, i needed a quick fix for header params, so i created this PR:

• Implementation proposal for operation parameters with :cookie and :header location and test case for rendered code #28

That PR uses only the opts argument for setting params in header and cookie, which can be improved upon, I'm sure.

I'll leave it here, but I'll keep an eye and try to be responsive here if there's interest.

[paulbalomiri]

I edited and cleaned up this part of the snippet

    opts: 
     opts
     |> Keyword.put(
         :parameter_types,  %{             
            req_path_param: {:string, :generic, spec_key: {:path, ":req-path-param"}, ...]},
            req_defaulted_param: {:string, generic, spec_key: {:header, "X-Req-Defaulted-Param"}, default:  "some default from spec"}
            ... # not expanding now on this
          })
    })

The opts entry :defaulted_params is only set by the first clause of &operation/1. What is conceptually missing is whether optional and defaulted fields should be handled in the struct too.

According to this comment ( more concise than the openapi promoted proposal 003 [link included because the comment link is broken]) i think that

• spec parameters which are required or which have a default should be in params
• typed spec parameters which are not defaulted should be
  • in params if nullable spec is missing or false, because they are required implicitely
  • in opts if nullable: true
• typed params which have a default should be in params (struct) and be initialized in the first clause

What is missing, is applying the defaults to the opts. Given a macro var @optional_parameter_defaults, and using pattern matching for runtime efficiency, this is what i can come up with:

opts=
  |> Enum.map( fn 
    {key, nil} when is_map_key(@optional_parameter_defaults, key) ->
       {key, @optional_parameter_defaults|> Map.get(key) }
    other-> other 
  )

This should happen in the 2nd clause (the one with the client.request/1 call.

EDIT: perhaps the best way to address this is to make the required/optional decision in the Processor, by creating a required field on the Operation passed to the Renderer, for each parameter 🤔

[paulbalomiri]

For easy pattern matching, maps are often helpful because the order of the keys does not matter:

def my_function(%{type: :string, nullable: false, required: true}), do: ...

However, we then have to consider whether the keys need to always be present (like a struct) or if we will sometimes have a type that is simply %(type: :string}. It isn't possible to match on the non-existence of keys, so it seems likely that every map will need the same keys — hence, a struct. But this could get cumbersome:

How about using the argument

{:string, :discriminator, nullable: false, required: true}
#which is equivalent to 
{:string, :discriminator, [nullable: false, required: true]}

This could then be easily pattern matched in the client:

def my_function({:string, :date, opts}), do: ...
def my_function({My.Module.Struct, :t, opts}), do: ...

Options are then implementation details, which fall in the reponsibility of the clause matching on {:string,:generic,_} or even just{:string,_,_}.

This should already be familiar from Config

import Config

config :some_app,
  key1: "value1",
  key2: "value2"

import_config "#{config_env()}.exs"

also from the app child spec:

%{
  id: Counter,
  start: {Counter, :start_link, [0]}
}

EDIT: accidentally deleted the prior comment on the same topic, so here we go again 😢