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

[paulbalomiri]

I formulated this directly as a PR, because i could not find an easy way to enhance OpenAPI.Processor.Operation with the additional stuct fields needed (Operation.request_header_parameters and Operation.request_cookie_parameters) for handling :cookie and :header parameters.

I think a way to add additional info, or a way to access the spec right down to the renderer would be helpful, if this was to be implemented as a plugin. I really appreciate the clean separation of concern in open-api-generator, though. This does however hamper creating plugins which need spec info in custom renderer implementations.

request parameters with :cookie and :header location

To handle the ory kratos spec, at least the :header location needed an implementation, but i decided to also implement :cookie parameters.

The generated code in operation.ex for a spec containing :header and :cookie parameters, generated from the test spec in my fork looks like this:

defmodule Operations do
  @moduledoc """
  Provides API endpoint related to operations
  """

  @default_client Client

  @doc """
  Example endpoint

  Example endpoint
  """
  @spec example(keyword) :: {:ok, String.t()} | :error
  def example(opts \\ []) do
    client = opts[:client] || @default_client

    header_parameters =
      Keyword.take(opts, [:header_param])
      |> Enum.map(fn {:header_param, value} -> {"X-Header-Param", value} end)

    cookie_parameters =
      Keyword.take(opts, [:cookie_param])
      |> Enum.map(fn {:cookie_param, value} -> {"cookie-param", value} end)

    client.request(%{
      args: [],
      call: {Operations, :example},
      url: "/req-headers",
      method: :get,
      headers: header_parameters,
      cookie: cookie_parameters,
      response: [{200, {:string, :generic}}],
      opts: opts
    })
  end
end

• parameter names are expected as atoms, and are then mapped to their respective spec keys when sending to client request.
• The mapper function is generated with additional clauses for each additional header or cookie parameter passed via option
  The header_parameters= line with 4 params looks like so:

    header_parameters =
      Keyword.take(opts, [:header_param, :header_param2, :authorization, :no_x_header_param])
      |> Enum.map(fn
        {:header_param, value} -> {"X-Header-Param", value}
        {:header_param2, value} -> {"X-Header-Param2", value}
        {:authorization, value} -> {"authorization", value}
        {:no_x_header_param, value} -> {"no-x-Header-Param", value}
      end)

I will not include here also the cookie mapping for multiple parameters as it's similar, but the X- (or x- prefix is not stripped.

Test suite

I also created a test suite, which can be used to test output files generated using specs in test/fixture/*.{yml|yaml}

The functionality of this PR is tested ( in /test/open_api/renderer_test.exs using OpenAPI.Test.RecorderCase

EDIT corrected some links

[paulbalomiri]

@aj-foster , are you interested in this pr, perhaps?

If yes i'd appreciate a review.

[aj-foster]

Hi there,

Thanks for opening this! I'm generally supportive of making additional information available to plugins. Currently a bit ill, so I haven't had a chance to review the code fully, but will do so soon.

[paulbalomiri]

I wish you well & hope you'll get better soon.

Thank you so much more for the ping, here, given your state!

[paulbalomiri]

I corrected the links which were previously broken:

The generated code in operation.ex for a spec containing :header and :cookie parameters, generated from the test spec in my fork looks like this:

and

The functionality of this PR is tested ( in /test/open_api/renderer_test.exs using OpenAPI.Test.RecorderCase

[aj-foster]

Thanks again for opening this. I've taken a look at the code (and the example spec; thank you for including it) and I'd like to iterate on this a little more.

First, some things we agree on:

• Making cookie and header params available in the operation struct (so that you can implement this as a plugin) is a great idea. I'd like to move that portion of the code out to a separate PR so we can merge it (and potentially unblock you) while we work on the rest.
• Furthermore, I want to investigate making the original spec available in certain situations as well, like you mentioned. This is a great way to enable rich and creative plugins.
• Integration tests are necessary and important. Thanks for pushing me to finally address this.

It looks like the header parameters used by the ORY spec are related to authorization and session management (X-Session-Token, Cookie, etc.). We have similar headers available for the GitHub API client (though not always listed in the spec) and I took a different route to include this kind of information in the requests. I wonder if you might be able to accomplish this same thing by including the desired information as generic opts that get passed to the client.

Taking a step back, with every change to the generator, we have to think about the relationship between the generated code and the client library code. I had a few thoughts when thinking through this:

• Like I mentioned above, using opts to pass additional data to the client is always an option. We use an auth option to pass authorization header information to the GitHub client, which is okay as long as we document it well (since it won't appear in the docstrings of individual operation functions).
• However, this same suggestion could be said for query params. If the spec took the time to define a param, we probably shouldn't ignore it just because it's not in the path or query.
• Formatting the headers (ex. choosing uppercase/lowercase, removing and re-adding the X- prefix, etc.) feels like the responsibility of the client library code. However, I understand your code in this PR is only making the parameters "nice" to use and otherwise preserving the original format given in the spec.

I also thought about the HTTP client Req, and how it recently changed to use maps for headers instead of keyword lists like many other HTTP clients. It made me wonder if a headers option, which accepts a map, would be a good practice. It seems likely that someone might want to merge additional headers on top of the ones defined by the spec, and a map might be better for this purpose.

In summary, I'd like to talk about this more. I don't think it is necessary for all of the generated functions to have code handling a Cookie header param like what is in the ORY spec; this feels like something the client library should implement through the general opts and document outside of the generated code. If other specs specify the authorization header, for example, I don't think it would be good to include that code in every generated function.

---

Regarding testing, I like the way we're able to look through the generated code. Instead of using an Agent, I wonder if we can utilize the process dictionary to avoid the agent being a bottleneck (not that we have many tests) and to avoid copying potentially large rendered binaries across a process boundary. I would also like to investigate saving the rendered AST as well, and letting us search it, but that will be a separate branch for me to work on.

[paulbalomiri]

Making cookie and header params available in the operation struct (so that you can implement this as a plugin) is a great idea. I'd like to move that portion of the code out to a separate PR so we can merge it (and potentially unblock you) while we work on the rest.

I can open a separate PR for this, if it's ok with you

I would also like to investigate saving the rendered AST as well, and letting us search it, but that will be a separate branch for me to work on.

Would you also consider this for the renderer? Right now overriding the function generation is an all-or-nothing option: You either implement it yourself & loose all the defp functions the reference implementation relies upon (reference sugar 😄 ), or you deal with it in the request function.

What do you think of making AST transformations available for

• the function parameter list
• the argument/s for the client.request call
• a function head section, injecting quoted snippets into the operation before request
• offer an accumulator argument for docs to each of those AST transformation functions, which is presented after the body ast is generated to the docstring generator
• Introduce a :rename_operation config option:

config :oapi_generator, naming: [
 ...
  rename_operation: [
    {OperationModule, "Prefix_function_as_string", "function_str"},
    {OperationModule, ~r/Prefix_(.*)", "\\1"},
    # and maybe some special handling
    {OperationModule, :underscore}, # underscore all ops within the tag
    {OperationModule, "OtherOperationFunction", :underscore} #underscore a specific function
  ]
]

I realize this devolves into a discussion in a PR, so, let me just

• create a new PR with request_options the fields Operation.request_header_parameters and Operation.request_cookie_parameters
• publish my implemetation libs (oapi_kratos, oapi_hydra, oapi_oathkeeper planned)
• publish my client library, which is distinct and relies on tesla middlewares.

Regarding testing, I like the way we're able to look through the generated code. Instead of using an Agent, I wonder if we can utilize the process dictionary to avoid the agent being a bottleneck (not that we have many tests) and to avoid copying potentially large rendered binaries across a process boundary

The main reason for using an Agent is because i thought messages could be sent back for assertions if you were later to expand the test suite. An Agent can be more easily replaced by a GenServer without then introducing the handling of message passing.

In any case i do not insist on it. If you want i can change it to use &Process.get/1 and Process.put/2 & create a second PR [using Agent or Process].

Shall I also create a separate PR for the test suite? I can write the output to disk as well. can you recommend a place for this? Basically this would be throwaway output, which should be included in .gitignore. I chose the agent because of it being easier to read IMHO. I do not insist on it. Structures sent locally via message passing are not copied in OTP, it instead copies on write (if memory serves me well).

Is it ok to use :oapi_kratos e.t.c. for hex.pm? Perhaps a recommendation would be in order here in the Readme to avoid nasty renamings, after implementation.

[paulbalomiri]

Taking a step back, with every change to the generator, we have to think about the relationship between the generated code and the client library code. I had a few thoughts when thinking through this:

Would you be open to a discussion (not here) on this? I think that the priority should be to make the output clear to the end user, who will look at the generated code. The client as in http-client is replaceable itself.

I also thought about the HTTP client Req, and how it recently changed to use maps for headers instead of keyword lists like many other HTTP clients. It made me wonder if a headers option, which accepts a map

Headers can be set more than once for a key! So, this would be a client author's decision. Generally, I'd argue if we were to have a discussion on this 👀 that prio #1 is how intuitive for elixir users the clients generated are, and how well documented the mapping from the openapi spec to @spec is.

On that note: the mapper function, if we were to go forward with this (and I realize you're not inclined) would be better generated as:

 header_parameters =
      Keyword.take(opts, [
      :header_param, :header_param2, :authorization, :no_x_header_param, 
      "X-Header-Param", "X-Header-Param2","authorization",  "no-x-Header-Param"
      ])
      |> Enum.map(fn
        {:header_param, value} -> {"X-Header-Param", value}
        {:header_param2, value} -> {"X-Header-Param2", value}
        {:authorization, value} -> {"authorization", value}
        {:no_x_header_param, value} -> {"no-x-Header-Param", value}
        {key, value} -> {key,value}
      end)

This way the parameters are accepted as atoms and exactly as in the openapi spec, using String header keys too.
I would generally stay away from http protocol related notions such as headers and body when talking about the exposed function, as the user of the clients. is concerned with how to pass parameters, not with the method of transport used by the client.

There are still name clashes to address e.t.c. but the generator has the "luxury" of not being for end consumption, but for client libs authors' consumption. Overridability is more important there.

I see Openapi as a method to transport documentation to the user from the openapi spec writer, while the client author & his tools are best when they cognitively "disappear". [note to self: PR is not for discussions 🤌 ]

[aj-foster]

I can open a separate PR for this, if it's ok with you

If you have a chance, please do! I have to work on other things tonight, else I'll get around to it this week. One small note: when you add items to keyword lists, would you mind alphabetizing them?

What do you think of making AST transformations available for...

I'm in favor. As a first step, I'd like to make all of the functions public. Then we can identify callbacks (you listed a few, and I think there may be more). If the new callbacks are called from the existing callbacks, then it won't be a breaking change.

Shall I also create a separate PR for the test suite?

Let's wait a moment on that. I've been thinking about an idea related to testing, which may be worth a discussion topic. In short, it is: I would like to collect example schemas and create a GitHub Action that generates the code for every schema and commits it to a separate git branch. Then, when a PR is opened, it generates a diff of all the example code and comments on the PR with a link.

I think we would still want an in-test renderer for unit tests, but maybe these things could work together.

Is it ok to use :oapi_kratos e.t.c. for hex.pm?

Certainly, go ahead. If you don't mind linking back to this project from your readme, that would be appreciated (but not required).

Would you be open to a discussion (not here) on this? I think that the priority should be to make the output clear to the end user, who will look at the generated code. The client as in http-client is replaceable itself.

Absolutely!

Here's my current thinking: we definitely want some support for cookie and header params in the default implementation of the code generator. I'm not 100% sure that parsing them as options is the best route for everyone, but I still want the generator to support you via plugins.

What we probably should do for everyone is include information about these params in the generated docstring. So as a first step, we could modify the default implementation of the operation docstring to incorporate this information.

[paulbalomiri]

This might take some days for me, but i'll push to do it this week.

I've come to rely a lot on ory auth software, so i want to have a lib with ory generator, customizations, a separate one with the client (existing), and 4-5 clients doing generation for each project.

What we probably should do for everyone is include information about these params in the generated docstring. So as a first step, we could modify the default implementation of the operation docstring to incorporate this information.

A list of docs as return from the doc override would be great, where some entries could be tuples, e.g. {:argument_binding, %{argument: {openapi_spec_name:: String.t, description}. The doc rendering to String would then create beautiful markdown.

The reason i'm saying this, is because the info abt the client arg bindings (also function renames) should be in a familiar reference format for all api calls. It benefits the end user if client authors opt in to use them.

[paulbalomiri]

@aj-foster just a short notice, this PR is not abandoned, I'm just in deployment work handling by now the unknown unkowns 😅 . I'll come back to this PR & our discussions hopefully by the end of next week.

[aj-foster]

Closing this for now, but we can definitely discuss this more!