Parquery / swagger-to

Generate server and client code from Swagger (OpenAPI 2.0) specification.
MIT License
57 stars 14 forks source link

Invalid Elm generated when `operationId` values are invalid Elm identifiers #107

Closed abingham closed 3 years ago

abingham commented 3 years ago

Consider this spec:

basePath: /
consumes:
- application/json
info:
  description: An API
  title: An API
  version: '1.0'
paths:
  /:
    get:
      operationId: get.foo.bar
      responses:
        '200':
          description: Success
produces:
- application/json
swagger: '2.0'

The operationId value is valid swagger, but it's not a valid Elm identifier (i.e. Elm identifiers can't have "." in them). However, swagger-to uses this value unmodified when producing Elm function names. For example, here's what it's producing for the above YAML:

get.foo.barRequest : String -> Maybe Time.Time -> Bool -> Http.Request String
get.foo.barRequest prefix maybeTimeout withCredentials =
mristin commented 3 years ago

Pardon my lack of Elm (a colleague of mine, @TeodoroFilippini, actually implemented the Elm generator) -- is there a way to escape the method name somehow? Or would you expect swagger-to to give an error here?

mristin commented 3 years ago

Please see also the issue https://github.com/Parquery/swagger-to/issues/88 -- maybe I should make swagger-to-elm simply use characters such as '-' and '.' as delimiters and silently generate a valid function name?

abingham commented 3 years ago

Issue #88 seems to be essentially the same issue.

is there a way to escape the method name somehow? Or would you expect swagger-to to give an error here?

There's no way to escape the names that I know of, and I definitely wouldn't want swagger-to to raise an error. My intuition here is that swagger-to should silently transform the names into valid Elm identifiers, e.g. 'foo.bar' could become 'foo_bar'. You could also do some package nesting or something so that 'foo' becomes a package and 'bar' is the function name; this is arguably cleaner and less susceptible to name collisions.

FWIW, the need to have these kinds of dotted names comes from my use of connexion which maps operationIds to Python identifiers on the server side. So I can't really get away from the dotted names, and that's why I need swagger-to to accept these names and do something useful with them.

mristin commented 3 years ago

Let's keep it simple for the moment and use . and - as delimiters in swagger-to-elm (foo.bar -> foo_bar). Package nesting seems like an overkill and given my limited knowledge of Elm would be also hard to implement.

abingham commented 3 years ago

At first glance, this looks like it should be pretty easy to fix. I could modify elm_client._write_request() to simply replace '.' with '_' after the call to swagger_to.camel_case().

But it feels more appropriate to actually modify swagger_to.camel_case() itself to do the same thing.

I'm happy to put together a PR for this. Do you have a preference for which approach I should take?

mristin commented 3 years ago

@abingham it would be great if you could set up a PR! It would be best if you could mimic what I did in the PRs https://github.com/Parquery/swagger-to/pull/89 and https://github.com/Parquery/swagger-to/pull/91.

abingham commented 3 years ago

PR #113 attempts to address this.

mristin commented 3 years ago

@abingham , I assume you tested #113 on your schema? Should we close the issue?

abingham commented 3 years ago

This seems to be working properly. Thanks!