You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@apisix.apache.org by GitBox <gi...@apache.org> on 2020/12/04 06:28:30 UTC
[GitHub] [apisix] spacewander opened a new pull request #2964: feat: route accroding to the graphql attributes
spacewander opened a new pull request #2964:
URL: https://github.com/apache/apisix/pull/2964
### What this PR does / why we need it:
<!--- Why is this change required? What problem does it solve? -->
<!--- If it fixes an open issue, please link to the issue here. -->
### Pre-submission checklist:
* [ ] Did you explain what problem does this PR solve? Or what new features have been added?
* [x] Have you added corresponding test cases?
* [x] Have you modified the corresponding document?
* [ ] Is this PR backward compatible? **If it is not backward compatible, please discuss on the [mailing list](https://github.com/apache/apisix/tree/master#community) first**
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] membphis commented on pull request #2964: feat: route accroding to the graphql attributes
Posted by GitBox <gi...@apache.org>.
membphis commented on pull request #2964:
URL: https://github.com/apache/apisix/pull/2964#issuecomment-740491882
@JanLi-air do you have time to look at this PR?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] membphis commented on a change in pull request #2964: feat: route accroding to the graphql attributes
Posted by GitBox <gi...@apache.org>.
membphis commented on a change in pull request #2964:
URL: https://github.com/apache/apisix/pull/2964#discussion_r538164446
##########
File path: t/lib/test_admin.lua
##########
@@ -108,14 +108,15 @@ function _M.comp_tab(left_tab, right_tab)
local err
dir_names = {}
+ local _
if type(left_tab) == "string" then
- left_tab, err = json.decode(left_tab)
+ left_tab, _, err = json.decode(left_tab)
Review comment:
got it, many thanks for your explain
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] membphis commented on a change in pull request #2964: feat: route accroding to the graphql attributes
Posted by GitBox <gi...@apache.org>.
membphis commented on a change in pull request #2964:
URL: https://github.com/apache/apisix/pull/2964#discussion_r538166296
##########
File path: apisix/core/ctx.lua
##########
@@ -15,22 +15,96 @@
-- limitations under the License.
--
local core_str = require("apisix.core.string")
+local core_tab = require("apisix.core.table")
+local request = require("apisix.core.request")
local log = require("apisix.core.log")
+local config_local = require("apisix.core.config_local")
local tablepool = require("tablepool")
local get_var = require("resty.ngxvar").fetch
local get_request = require("resty.ngxvar").request
local ck = require "resty.cookie"
+local gq_parse = require("graphql").parse
local setmetatable = setmetatable
local sub_str = string.sub
local rawset = rawset
+local ngx = ngx
local ngx_var = ngx.var
local re_gsub = ngx.re.gsub
+local ipairs = ipairs
local type = type
local error = error
-local ngx = ngx
+local pcall = pcall
local _M = {version = 0.2}
+local GRAPHQL_DEFAULT_MAX_SIZE = 1048576 -- 1MiB
+
+
+local function parse_graphql(ctx)
+ local local_conf, err = config_local.local_conf()
+ if not local_conf then
+ return nil, "failed to get local conf: " .. err
+ end
+
+ local max_size = GRAPHQL_DEFAULT_MAX_SIZE
+ local size = core_tab.try_read_attr(local_conf, "graphql", "max_size")
+ if size then
+ max_size = size
+ end
+
+ local body, err = request.get_body(max_size, ctx)
+ if not body then
+ return nil, "failed to read graphql body: " .. err
+ end
+
+ local ok, res = pcall(gq_parse, body)
+ if not ok then
+ return nil, "failed to parse graphql: " .. res .. " body: " .. body
+ end
+
+ if #res.definitions == 0 then
+ return nil, "empty graphql: " .. body
+ end
+
+ return res
+end
+
+
+local function get_parsed_graphql(ctx)
+ if not ctx._graphql then
Review comment:
code style
```lua
if ctx._graphql then
return ctx._graphql
end
... ...
```
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] membphis commented on a change in pull request #2964: feat: route accroding to the graphql attributes
Posted by GitBox <gi...@apache.org>.
membphis commented on a change in pull request #2964:
URL: https://github.com/apache/apisix/pull/2964#discussion_r538009201
##########
File path: apisix/core/ctx.lua
##########
@@ -84,6 +158,10 @@ do
key = re_gsub(key, "-", "_", "jo")
val = get_var(key, t._request)
+ elseif core_str.has_prefix(key, "graphql_") then
+ key = sub_str(key, 9)
Review comment:
magic number, need comment
##########
File path: apisix/core/ctx.lua
##########
@@ -15,22 +15,96 @@
-- limitations under the License.
--
local core_str = require("apisix.core.string")
+local core_tab = require("apisix.core.table")
+local request = require("apisix.core.request")
local log = require("apisix.core.log")
+local config_local = require("apisix.core.config_local")
local tablepool = require("tablepool")
local get_var = require("resty.ngxvar").fetch
local get_request = require("resty.ngxvar").request
local ck = require "resty.cookie"
+local gq_parse = require("graphql").parse
local setmetatable = setmetatable
local sub_str = string.sub
local rawset = rawset
+local ngx = ngx
local ngx_var = ngx.var
local re_gsub = ngx.re.gsub
+local ipairs = ipairs
local type = type
local error = error
-local ngx = ngx
+local pcall = pcall
local _M = {version = 0.2}
+local GRAPHQL_DEFAULT_MAX_SIZE = 1048576 -- 1MiB
+
+
+local function parse_graphql(ctx)
+ local local_conf, err = config_local.local_conf()
+ if not local_conf then
+ return nil, "failed to get local conf: " .. err
+ end
+
+ local max_size = GRAPHQL_DEFAULT_MAX_SIZE
+ local size = core_tab.try_read_attr(local_conf, "graphql", "max_size")
+ if size then
+ max_size = size
+ end
+
+ local body, err = request.get_body(max_size, ctx)
+ if not body then
+ return nil, "failed to read graphql body: " .. err
+ end
+
+ local ok, res = pcall(gq_parse, body)
+ if not ok then
+ return nil, "failed to parse graphql: " .. res .. " body: " .. body
+ end
+
+ if #res.definitions == 0 then
+ return nil, "empty graphql: " .. body
+ end
+
+ return res, nil
Review comment:
`return res` be fine
##########
File path: t/lib/test_admin.lua
##########
@@ -108,14 +108,15 @@ function _M.comp_tab(left_tab, right_tab)
local err
dir_names = {}
+ local _
if type(left_tab) == "string" then
- left_tab, err = json.decode(left_tab)
+ left_tab, _, err = json.decode(left_tab)
Review comment:
where to get this API manual?
the new way seems wrong. I think the correct style should be `local res, err = json.decode(str)`
##########
File path: apisix/core/ctx.lua
##########
@@ -15,22 +15,96 @@
-- limitations under the License.
--
local core_str = require("apisix.core.string")
+local core_tab = require("apisix.core.table")
+local request = require("apisix.core.request")
local log = require("apisix.core.log")
+local config_local = require("apisix.core.config_local")
local tablepool = require("tablepool")
local get_var = require("resty.ngxvar").fetch
local get_request = require("resty.ngxvar").request
local ck = require "resty.cookie"
+local gq_parse = require("graphql").parse
local setmetatable = setmetatable
local sub_str = string.sub
local rawset = rawset
+local ngx = ngx
local ngx_var = ngx.var
local re_gsub = ngx.re.gsub
+local ipairs = ipairs
local type = type
local error = error
-local ngx = ngx
+local pcall = pcall
local _M = {version = 0.2}
+local GRAPHQL_DEFAULT_MAX_SIZE = 1048576 -- 1MiB
+
+
+local function parse_graphql(ctx)
+ local local_conf, err = config_local.local_conf()
+ if not local_conf then
+ return nil, "failed to get local conf: " .. err
+ end
+
+ local max_size = GRAPHQL_DEFAULT_MAX_SIZE
+ local size = core_tab.try_read_attr(local_conf, "graphql", "max_size")
+ if size then
+ max_size = size
+ end
+
+ local body, err = request.get_body(max_size, ctx)
+ if not body then
+ return nil, "failed to read graphql body: " .. err
+ end
+
+ local ok, res = pcall(gq_parse, body)
+ if not ok then
+ return nil, "failed to parse graphql: " .. res .. " body: " .. body
+ end
+
+ if #res.definitions == 0 then
+ return nil, "empty graphql: " .. body
+ end
+
+ return res, nil
+end
+
+
+local function get_parsed_graphql(ctx)
+ if not ctx._graphql then
+ local res, err = parse_graphql(ctx)
+ if not res then
+ log.error(err)
+ ctx._graphql = {}
+
Review comment:
return directly
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] spacewander commented on a change in pull request #2964: feat: route accroding to the graphql attributes
Posted by GitBox <gi...@apache.org>.
spacewander commented on a change in pull request #2964:
URL: https://github.com/apache/apisix/pull/2964#discussion_r538018558
##########
File path: t/lib/test_admin.lua
##########
@@ -108,14 +108,15 @@ function _M.comp_tab(left_tab, right_tab)
local err
dir_names = {}
+ local _
if type(left_tab) == "string" then
- left_tab, err = json.decode(left_tab)
+ left_tab, _, err = json.decode(left_tab)
Review comment:
@membphis
For dkjson, the second argument is `pos`, not `err`.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] spacewander commented on a change in pull request #2964: feat: route accroding to the graphql attributes
Posted by GitBox <gi...@apache.org>.
spacewander commented on a change in pull request #2964:
URL: https://github.com/apache/apisix/pull/2964#discussion_r538189387
##########
File path: apisix/core/ctx.lua
##########
@@ -15,22 +15,96 @@
-- limitations under the License.
--
local core_str = require("apisix.core.string")
+local core_tab = require("apisix.core.table")
+local request = require("apisix.core.request")
local log = require("apisix.core.log")
+local config_local = require("apisix.core.config_local")
local tablepool = require("tablepool")
local get_var = require("resty.ngxvar").fetch
local get_request = require("resty.ngxvar").request
local ck = require "resty.cookie"
+local gq_parse = require("graphql").parse
local setmetatable = setmetatable
local sub_str = string.sub
local rawset = rawset
+local ngx = ngx
local ngx_var = ngx.var
local re_gsub = ngx.re.gsub
+local ipairs = ipairs
local type = type
local error = error
-local ngx = ngx
+local pcall = pcall
local _M = {version = 0.2}
+local GRAPHQL_DEFAULT_MAX_SIZE = 1048576 -- 1MiB
+
+
+local function parse_graphql(ctx)
+ local local_conf, err = config_local.local_conf()
+ if not local_conf then
+ return nil, "failed to get local conf: " .. err
+ end
+
+ local max_size = GRAPHQL_DEFAULT_MAX_SIZE
+ local size = core_tab.try_read_attr(local_conf, "graphql", "max_size")
+ if size then
+ max_size = size
+ end
+
+ local body, err = request.get_body(max_size, ctx)
+ if not body then
+ return nil, "failed to read graphql body: " .. err
+ end
+
+ local ok, res = pcall(gq_parse, body)
+ if not ok then
+ return nil, "failed to parse graphql: " .. res .. " body: " .. body
+ end
+
+ if #res.definitions == 0 then
+ return nil, "empty graphql: " .. body
+ end
+
+ return res
+end
+
+
+local function get_parsed_graphql(ctx)
+ if not ctx._graphql then
Review comment:
updated
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] moonming merged pull request #2964: feat: route accroding to the graphql attributes
Posted by GitBox <gi...@apache.org>.
moonming merged pull request #2964:
URL: https://github.com/apache/apisix/pull/2964
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org