haskell-servant / servant

Main repository for the servant libraries — DSL for describing, serving, querying, mocking, documenting web applications and more!
https://docs.servant.dev/
1.79k stars 407 forks source link

swagger-codegen issues #1084

Open adamConnerSax opened 5 years ago

adamConnerSax commented 5 years ago

I'm trying to build a servant client to query an API for which I have a swagger file. I used the swagger-codegen tool to generate a servant API library.

TL;DR

Details:

I downloaded the swagger-codegen tool, built it, and ran

java -jar modules/swagger-codegen-cli/target/swagger-codegen-cli.jar generate -i http://api.open.fec.gov/swagger -l haskell -o OpenFEC-servant

then I entered the OpenFEC-servant directory (checked into github on master branch of https://github.com/adamConnerSax/swagger-codegen-servant) and ran stack build whic resulted in many errors, all of the form:

/Users/adam/DataScience/swagger-codegen/OpenFEC-servant/lib/OpenFEC/Types.hs:221:54: error:
    Not in scope: type constructor or class ‘Date’

Doing cabal new-build with ghc-8.4 (and thus much newer versions of all the libs since the cabal file doesn't have version bounds) yields the same error.

I can "fix" that by:

  1. Adding "time" to the build depends of the cabal file
  2. Adding import Data.Time.Calendar (Day) to lib/OpenFEC/Types.hs
  3. Adding data Date = Day to lib/OpenFEC/Types.hs
  4. Adding an export of Date to lib/OpenFEC/Types.hs (for API.hs to import)

(checked into same repo, on "FixDate" branch)

I've no idea if this is a correct mapping because though this allows this file to compile, there are more errors. See below. Also, the compilation of Types.hs, which is 4270 lines long, takes a long time on both the ghc which goes with the stack build, 8.0.2 I think, [7 min] and ghc-8.4.3 [33 min] (Mac OSX 10.14.1, 3.5 GHz, 6-core Xeon E5 with 32GB of RAM). I'm assuming that compile time scales with size of the Types.hs file, so I tried to have the swagger-codegen produce a smaller output by restricting the models or apis produced (using the -Dmodels=XXX or -Dapis=XXX) but I couldn't get that to work. I can only produce all or nothing. I don't know if that's due to the swagger input file or the haskell-servant codegen module.

So, using the provided stack.yaml and stack build, I get to the next set of errors, included in the repo in "stack_build_output.txt"

I get similar issues (I think) using cabal new-build. One more small thing needs fixing, the import of BaseUrl. Once that's fixed in API.hs the build errors out with representation issues as well. Though they are not exactly the same in location or error message. This version is in the same repo, on the "FixBaseUrl" branch. The errors are in the "cabal_new_build_output.txt"

I'm not sure where to start with figuring any of this out, but I'd be happy to help if there's work that needs doing.

alpmestan commented 5 years ago

(We've been discussing this on IRC, and it appears that the servant backend of the swagger-codegen tool is a bit buggy and out of date.)

phadej commented 5 years ago

I'm confused, Can servant maintainers do something for swagger-codegen? Shouldn't the issue be reported to maintainers of swagger-codegen?

adamConnerSax commented 5 years ago

swagger-codegen has a number of backends and I thought it might be faster to figure out how to update by coming through here. Actually, I started with IRC and then alpmestan asked me to post an issue here. Now I'm trying to fix the haskell-servant backend of swagger-codegen. Or at least get it to the errors I don't understand.

alpmestan commented 5 years ago

@phadej We can have the investigation take place here and then send them a patch once we've figured out the problem.

adamConnerSax commented 5 years ago

An update: I've fixed several things. But I can't guarantee that I haven't created new issues.

  1. I changed the date mapping to Data.Time.Calendar.Day

  2. I changed the date-time mapping to Data.Time.LocalTime.LocalTIme

  3. I changed things to use ClientM and Handler as appropriate.

  4. I changed the field naming so that for an endpoint with name endpointName and field with name field_name of type X, the resulting thing looks like:

    date EndpointName = EndpointName 
    {
    endpointName_field_name :: X
    ...
    }

    and the generated Aeson FromJSON and ToJSON instances correctly handle those prefixes. Though I guess I've only really checked the FromJSON. But they use the same function to remove the prefixes. This fixed an issue where "_" was changed to "'Underscore"

  5. I removed the use of QueryList--a type used to wrap query parameters taking multiple arguments-- everywhere. This is the one I am least certain about. Why was it there? I left the QueryList code in API.hs file in case it serves a purpose.

  6. I removed "List" from the set of Keywords. Since it isn't one.

  7. I removed a type mapping of "array" to "List" and made it "array" to "[]" which might work. I haven't checked yet. But it does fix a bug where "List" was getting renamed to "List_" and then failing to compile.

    1. Various import changes, etc. The current version resides at https://github.com/adamConnerSax/swagger-codegen/blob/UpdateHaskellServant/modules/swagger-codegen/src/main/java/io/swagger/codegen/languages/HaskellServantCodegen.java Things now compile with the OpenFEC swagger file as well as some "petstore" example file from a different issue (https://github.com/swagger-api/swagger-codegen/issues/8914). I haven't done much other testing. I can get data from an OpenFEC endpoint but the decoding fails, due I think to a swagger issue on the OpenFEC side.

I'm not sure where to go from here. Should I submit a PR to swagger-codegen? It feels woefully under-tested. OTOH, the other version failed to compile at all so maybe untested but compiling is stil an improvement?

jonschoning commented 5 years ago

re: coordination - there are also proposed changes here: https://github.com/OpenAPITools/openapi-generator/pull/1469

f-f commented 5 years ago

A clarification on the above: for a whole set of reasons swagger-codegen is not merging PRs since several months and most of the contributors have moved to openapi-codegen, which is quite active and more updated.

Now, the servant template was deeply outdated in both repos, so I patched it with the changes @jonschoning linked above (OpenAPITools/openapi-generator#1469), and we're running in production with the patches contained in that PR on two of our services since ~6months (and this is to say that they are quite tested).

After merging that PR most of these issues @adamConnerSax mentions here will be solved, so I'd propose we move the discussion there or to a new issue in that repo.

alpmestan commented 5 years ago

Yes, this issue was meant to be the home of the discussion until a solution was figured out. Thanks to @adamConnerSax @jonschoning and you @f-f for taking care of all these issues.

In light of this, I would like to turn this issue into one about advertising this work! Once your patch hits openapi-generator, would anyone be interested in writing perhaps a quick cookbook recipe or tutorial section about this? It doesn't have to be fancy, but I do think this should be mentionned somewhere on our readthedocs. It's such a cheap (in terms of time/resources) way to get some clients or server up and running from an API specification, I'm sure this will be of interest to many future readers.

If nobody feels like doing it, I might give it a shot myself, when I get a chance to (quite likely not anytime soon).