Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Lua] Add new Lua client generator #6252

Merged
merged 17 commits into from
Aug 10, 2017
Merged

[Lua] Add new Lua client generator #6252

merged 17 commits into from
Aug 10, 2017

Conversation

wing328
Copy link
Contributor

@wing328 wing328 commented Aug 6, 2017

PR checklist

  • Read the contribution guidelines.
  • Ran the shell script under ./bin/ to update Petstore sample so that CIs can verify the change. (For instance, only need to run ./bin/{LANG}-petstore.sh and ./bin/security/{LANG}-petstore.sh if updating the {LANG} (e.g. php, ruby, python, etc) code generator or {LANG} client's mustache templates). Windows batch files can be found in .\bin\windows\.
  • Filed the PR against the correct branch: master for non-breaking changes and 3.0.0 branch for breaking (non-backward compatible) changes.

Description of the PR

For #4794

Still WIP (based on #6244)

Prefix string
}

type Configuration struct {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@daurnimator here is the configuration class for Go API client. We may need something similar in Lua API client to store the authentication setting, default header, host, basePath, etc

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Don't we sort of have that already with the api class?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, and I'm not saying it's a must. My suggestion is to make it more consistent with other language generator. We can put it as a day 2 requirement for reconsideration.

return setmetatable(t, {{classname}}_mt)
end

local function new_{{classname}}({{#vars}}{{name}}{{#hasMore}}, {{/hasMore}}{{/vars}})
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@daurnimator in the constructor method signature, I've included all parameters (required, optional)

local function new_{{classname}}({{#vars}}{{name}}{{#hasMore}}, {{/hasMore}}{{/vars}})
return cast_pet({
{{#vars}}
{{name}} = {{name}};
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@daurnimator later we'll need to handle the situation when the JSON key name contains special character (e.g. date-time) (day 2 requirement)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For non-valid lua identifiers, the key syntax is ["f@0"]

return nil, err3
end
return {{{packagName}}}_{{classname}}.cast(result)
else
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@daurnimator for 4xx and 5xx status code, we usually throw an exception in other generators and the exception object contains 3 fields: http status code, header and body (error message). Not sure if we can do something similar in Lua.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can throw an error, but it's considered better practice to return nil, error, errno. Where error is usually a string, but could be some type of other object.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure, let's follow the best practice to return nil, error, errno.

if result == nil then
return nil, err3
end
return {{{packagName}}}_{{classname}}.cast(result)
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@daurnimator I notice that Lua can return a list of objects so I would suggest we return the following

  • result (e.g. Pet, string, array of Users)
  • http status code
  • http headers

as it's just a matter of time the developers need the status code and header.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lua-http follows the http2 convention where the status code is just another header (called :status): so no need for the extra status return value.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK. so just return result and headers then.

@wing328
Copy link
Contributor Author

wing328 commented Aug 6, 2017

@daurnimator please review my comments and the TODO in the code when you've time. Thanks!

// TODO: default basePath to {{{basePath}}}
return setmetatable({
host = host;
basePath = basePath;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could use basePath = basePath or {{{basePath}}} to close above TODO?

basePath = basePath;
schemes = schemes_map;
default_scheme = default_scheme;
}, {{{pacakgeName}}}_mt)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

typo? pacakgeName

if result == nil then
return nil, err3
end
return {{{packagName}}}_{{classname}}.cast(result)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lua-http follows the http2 convention where the status code is just another header (called :status): so no need for the extra status return value.

return nil, err3
end
return {{{packagName}}}_{{classname}}.cast(result)
else
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can throw an error, but it's considered better practice to return nil, error, errno. Where error is usually a string, but could be some type of other object.

local function new_{{classname}}({{#vars}}{{name}}{{#hasMore}}, {{/hasMore}}{{/vars}})
return cast_pet({
{{#vars}}
{{name}} = {{name}};
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For non-valid lua identifiers, the key syntax is ["f@0"]

@@ -0,0 +1,40 @@
# Compiled Lua sources
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure where you got this .gitignore file from.. it's tremendously broad. You probably don't need one at all..

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

end

local function new_api_response(code, type, message)
return cast_pet({
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You left cast_pet in here.

end

local function new_tag(id, name)
return cast_pet({
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

cast_pet

})

// set HTTP verb
req.headers:upsert(":method", "Post")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"Post" should be all caps I think?
Also this is indented oddly

local req = http_request.new_from_uri({
scheme = self.default_scheme;
host = self.host;
path = string.format("%s//store/order", self.basePath, petId);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

double //

local dkjson = require "dkjson"

// model import
local {{{packagName}}}_{{classname}} = require "{{{pacakgeName}}}.{{classname}}"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

two different typos: pacakgeName packagName

local {{{packagName}}}_{{classname}} = require "{{{pacakgeName}}}.{{classname}}"

local {{{packageName}}}= {}
local {{{pacakgeName}}}_mt = {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

pacakgeName

})

// set HTTP verb
req.headers:upsert(":method", "{{httpMethod}}")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

indentation

local var_accept = { {{#consumes}}"{{{mediaType}}}"{{#hasMore}}, {{/hasMore}}{{/consumes}} }
req.headers:upsert("accept", {{#consumes}}{{#-first}}"{{{mediaType}}}"{{/-first}}{{/consumes}})

// TODO: create a function to select proper content-type
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

selects content-type based on what?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

{{/authMethods}}

// make the HTTP call
local headers, stream, errno = req:go()
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FYI, we'll probably want to pass a timeout to :go()

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK. We can add another configurable property timeout for that.

@@ -91,20 +91,37 @@ function {{classname}}:{{operationId}}({{#allParams}}{{#required}}{{paramName}}{
return nil, stream, errno
end
local http_status = headers:get(":status")
if http_status == "200" then
if tonumber(http_status) >= 200 or tonumber(http_status) <= 299 then
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No need to convert to number (and that will have false positives, e.g. a status code of 2e2)
Instead consider if you just want to check the first character? In which case: if http_status:sub(1,1) == "2"

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure. will update that in the next push.

{{#vars}}
{{name}} = {{name}};
["{{baseName}}"] = {{name}};
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will this escape correctly if e.g. baseName include a double quote?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, it would be better to prefer the non-quoted form (saving the [] syntax only for invalid lua identifiers)

@wing328
Copy link
Contributor Author

wing328 commented Aug 8, 2017

I think these are the remaining items before a beta release:

  • add support for query parameters
  • add support for authentication
  • function to select a proper "accept", "content-type"

@daurnimator
Copy link
Contributor

add support for query parameters

Support how?

add support for authentication

What types of auth? Does this require anything more than setting the authorization header?

function to select a proper "accept", "content-type"

When/how should this happen?

@wing328
Copy link
Contributor Author

wing328 commented Aug 9, 2017

Support how?

How can we add query parameters (URL query string) to HTTP request in Lua?

What types of auth? Does this require anything more than setting the authorization header?

HTTP basic auth, API key (header or query), OAuth access token

function to select a proper "accept", "content-type"

We need to create 2 functions in Lua similar to https://github.com/swagger-api/swagger-codegen/blob/master/modules/swagger-codegen/src/main/resources/Java/libraries/jersey2/ApiClient.mustache#L435-L491

@wing328
Copy link
Contributor Author

wing328 commented Aug 10, 2017

@daurnimator I've tried to add support for query parameters and authentication via b63ab93. I also added 2 fake query parameters (will be removed before merge) to illustrate what the auto-generated Lua code for query parameters looks like.

Please review when you've time.

@wing328
Copy link
Contributor Author

wing328 commented Aug 10, 2017

Quick question: shall we put the API and model files under the sub-folder "api" and "model" respectively?

@@ -4,6 +4,7 @@
{{#operations}}
local http_request = require "http.request"
local http_util = require "http.util"
local url_encode = require "http.util".encodeURIComponent
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No need to make this a local: call it as http_util.encodeURIComponent when needed.

}, {{{packageName}}}_mt)
end

// for base64 encoding
local function base64_encode(s)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

schemes_map[v] = v
end
local default_scheme = schemes_map.https or schemes_map.http
-- TODO: default basePath to {{{basePath}}}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This TODO is done.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Removed

@wing328
Copy link
Contributor Author

wing328 commented Aug 10, 2017

@daurnimator from your perspective, do you think it's ready for a beta release so as to collect more feedback from the Lua community?

@daurnimator
Copy link
Contributor

daurnimator commented Aug 10, 2017

@daurnimator from your perspective, do you think it's ready for a beta release so as to collect more feedback from the Lua community?

Maybe. Though it's not like we don't have an empty TODO list.

Other things to add:

  • generate a .rockspec file.
  • Add lua linting to .travis.yml

@wing328
Copy link
Contributor Author

wing328 commented Aug 10, 2017

Btw, something else to add would be generating a .rockspec file

I think that's for LuaRocks (lua package manager) so sure let's add that. I'll include it in the enhancement list.

@daurnimator
Copy link
Contributor

I think that's for LuaRocks (lua package manager) so sure let's add that. I'll include it in enhancement list.

Where is this enhancement list?

@wing328
Copy link
Contributor Author

wing328 commented Aug 10, 2017

I'll create it after merging this PR into master shortly.

@@ -0,0 +1,8 @@
language: go
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This isn't go :)

{{#isApiKey}}- **Type**: API key

Example
```
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

README examples need updating

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yup, I've removed the auto-genreated README for now as it needs some work to output correct examples. Will add to the enhancement list for tracking.


local {{{packageName}}}= {}
local {{{packageName}}}_mt = {
__name = "{{{classname}}}";
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Subjective question: indentation.
Most lua is indented in tabs or 4 spaces; however some people use 3 spaces due to it being the default in emacs; and the rare person use 8 spaces. However, 2 spaces is pretty rare.
May I suggest swapping to tabs?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good suggestion. Is there any Lua style guide that we can follow?

Copy link
Contributor

@daurnimator daurnimator Aug 10, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can't think of a good one :P
Even the one I wrote I don't like any more

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's use tab then.

{{>partial_header}}
package {{packageName}}

import (
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure what this file is doing here?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Removed. (used by Go)

@@ -171,6 +171,17 @@ paths:
required: true
type: integer
format: int64
- name: size
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This needs removing before merging

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Removed. Thanks for the reminder

req.headers:upsert("authorization", "Bearer " .. self.access_token)
{{/isOAuth}}
{{/authMethods}}

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There seems to be lots of whitespace in generated files around this.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It shouldn't. It's a feature in mustache template library that the following will not result in any line or whitespace

    {{#thisIsFalse}}
    something here...
    {{/thisIsFalse}}

@@ -1,8 +1,6 @@
language: go
language: lua
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sadly travis doesn't support lua. Us lua devs use a tool 'hererocks' to run in travis
https://github.com/mpeterv/hererocks/#using-hererocks-to-set-up-automated-testing

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will add this to the enhancement list later

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sadly travis doesn't support lua.

Any other CI provides better support for Lua?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

None I know off the top of my head. Most lua devs will just use hererocks

--local var_accept = { "application/xml", "application/json" }
req.headers:upsert("content-type", "application/xml")


Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These are the empty lines I was talking about.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Removed.

if result == nil then
return nil, err3
end
return petstore_{}.cast(result), headers
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

petstore_{} doesn't seem like what we wanted?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right. That method returns an array of object. Not sure how we can deserialize the result back into an array of object in Lua

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's an already an array. We might just want to cast the elements of the array to the correct type.

for _, ob in ipairs(result) do
    cast_pet(ob)
end

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK. What if the result is a dictionary/hash/map? What should the code look like?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hrm? a dictionary/hash of what?
dkjson.decode will just decode the json object and return a lua table (arrays and dictionaries are the same data structure in lua)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

dkjson.decode will just decode the json object and return a lua table

That should work.

Hrm? a dictionary/hash of what?

Here is an example (the key is not fixed):
{
"cat" : 10,
"dog": 2,
"rabbit": 7
}

if result == nil then
return nil, err3
end
return petstore_pet.cast(result), headers
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@daurnimator should this be cast_pet(result), headers?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated to use cast_pet via 6d180af

@wing328 wing328 merged commit 06686d6 into master Aug 10, 2017
@wing328
Copy link
Contributor Author

wing328 commented Aug 10, 2017

Tweet: https://twitter.com/wing328/status/895703684079734784 to promote the new generator.

@wing328
Copy link
Contributor Author

wing328 commented Aug 10, 2017

@daurnimator here is the enhancement list: #6284

@wing328
Copy link
Contributor Author

wing328 commented Aug 10, 2017

Add @daurnimator as the template creator of Lua via f4de426

@wing328 wing328 deleted the lua_generator branch August 17, 2017 06:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants