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/10/16 12:15:51 UTC
[GitHub] [apisix] Jaycean opened a new pull request #2440: plugin(consumer-restriction): use draft7 way to rewrite the JSON Schema.
Jaycean opened a new pull request #2440:
URL: https://github.com/apache/apisix/pull/2440
### What this PR does / why we need it:
- fix: #2402
### Pre-submission checklist:
* [x] Did you explain what problem does this PR solve? Or what new features have been added?
* [x] Have you added corresponding test cases?
* [ ] Have you modified the corresponding document?
* [x] Is this PR backward compatible?
----------------------------------------------------------------
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] juzhiyuan commented on pull request #2440: WIP:plugin(consumer-restriction): use draft7 way to rewrite the JSON Schema.
Posted by GitBox <gi...@apache.org>.
juzhiyuan commented on pull request #2440:
URL: https://github.com/apache/apisix/pull/2440#issuecomment-710761426
> > hi please see here [apache/apisix-dashboard#495](https://github.com/apache/apisix-dashboard/issues/495)
>
> so you means apisix should not return disable filed until get request when FE needed? @juzhiyuan
>
> it seems that when the schema of the plugin does not have a `properties` property in the first level of the json schema, the interface returns the data with `disable` field.
> related code: https://github.com/membphis/apisix/blob/133b52ec78665b7292af62bab6af6bf981363a42/apisix/plugin.lua#L81
> which doesn't seem reasonable, especially after json schema changed from draft5 to draft7.
> in json schema draft7, the `oneof` `anyof` and `allof` type of schema e.g the schema in pr #2440 does not contain `properties` in first level
> cc @Jaycean
>
> the #2439 may also have something to do with it.
hi @membphis, What do you think?
----------------------------------------------------------------
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] juzhiyuan commented on pull request #2440: plugin(consumer-restriction): use draft7 way to rewrite the JSON Schema.
Posted by GitBox <gi...@apache.org>.
juzhiyuan commented on pull request #2440:
URL: https://github.com/apache/apisix/pull/2440#issuecomment-710094623
@Jaycean Please have a look at CI.
----------------------------------------------------------------
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] Jaycean edited a comment on pull request #2440: WIP:plugin(consumer-restriction): use draft7 way to rewrite the JSON Schema.
Posted by GitBox <gi...@apache.org>.
Jaycean edited a comment on pull request #2440:
URL: https://github.com/apache/apisix/pull/2440#issuecomment-710769509
> > > hi please see here [apache/apisix-dashboard#495](https://github.com/apache/apisix-dashboard/issues/495)
> >
> >
> > so you means apisix should not return disable filed until get request when FE needed? @juzhiyuan
> > it seems that when the schema of the plugin does not have a `properties` property in the first level of the json schema, the interface returns the data with `disable` field.
> > related code: https://github.com/membphis/apisix/blob/133b52ec78665b7292af62bab6af6bf981363a42/apisix/plugin.lua#L81
> > which doesn't seem reasonable, especially after json schema changed from draft5 to draft7.
> > in json schema draft7, the `oneof` `anyof` and `allof` type of schema e.g the schema in pr #2440 does not contain `properties` in first level
> > cc @Jaycean
> > the #2439 may also have something to do with it.
>
> hi @membphis, What do you think?
@liuxiran @juzhiyuan @membphis
Yes, Lua code is in `apisix/apisix/plugin.lua`,`apisix/schema_def.lua`
I think it's the code's judgment on the current schema structure that needs to be changed
However, I may not have a deep understanding of the whole project and the role of the disable field, so I have not thought about how to change the code logic
```
# `apisix/apisix/plugin.lua`
local function load_plugin(name, plugins_list, is_stream_plugin)
# line82-88
if plugin.schema and plugin.schema.type == "object" then
if not plugin.schema.properties or
core.table.nkeys(plugin.schema.properties) == 0
then
plugin.schema.properties = core.schema.plugin_disable_schema
end
end
# This is where the disables field is added. It seems that all schemas use this function
apisix/schema_def.lua
# function
_M.plugin_disable_schema = {
disable = {type = "boolean"}
```
Because in the JSON schema Draft 7, some structures do not contain `properties` in the first layer, but there are `properties` in the second layer, but the logic fails to judge. As a result, some schemas do not need to add the` disable` field
In fact, this problem currently affects all related schemas
Because there is still a difference between Lua's test framework and Web UI, the Lua test framework has no impact. From the user's point of view, I think that the sudden appearance of this field will make users very confused
----------------------------------------------------------------
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] liuxiran edited a comment on pull request #2440: WIP:plugin(consumer-restriction): use draft7 way to rewrite the JSON Schema.
Posted by GitBox <gi...@apache.org>.
liuxiran edited a comment on pull request #2440:
URL: https://github.com/apache/apisix/pull/2440#issuecomment-710740234
> According to the previous JSON schema data format, without changing the logic, the current writing method is not the same as the data obtained by apisix-dashboard. I need to discuss with @liuxiran again tomorrow
@Jaycean
with your newest json schema
```json
{
"oneOf": [{
"properties": {
"blacklist": {
"items": {
"type": "string"
},
"minItems": 1,
"type": "array"
},
"rejected_code": {
"default": 403,
"minimum": 200,
"type": "integer"
},
"type": {
"default": "consumer_name",
"enum": ["consumer_name", "service_id"],
"type": "string"
}
},
"required": ["blacklist"],
"title": "blacklist"
}, {
"properties": {
"rejected_code": {
"default": 403,
"minimum": 200,
"type": "integer"
},
"type": {
"default": "consumer_name",
"enum": ["consumer_name", "service_id"],
"type": "string"
},
"whitelist": {
"items": {
"type": "string"
},
"minItems": 1,
"type": "array"
}
},
"required": ["whitelist"],
"title": "whitelist"
}],
"properties": {
"disable": {
"type": "boolean"
}
},
"type": "object"
}
```
dashboard can create a route with `key-auth` plugin and `consumer-restriction` plugin successfully. and the `consumer-restriction` plugin can work correctly with the configuration.
If you can change the keys **order** in properties one from current:
`blacklist, rejected_code, type` to expected `rejected_code, type, blacklist`, that would be better :)
----------------------------------------------------------------
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] Jaycean commented on pull request #2440: plugin(consumer-restriction): use draft7 way to rewrite the JSON Schema.
Posted by GitBox <gi...@apache.org>.
Jaycean commented on pull request #2440:
URL: https://github.com/apache/apisix/pull/2440#issuecomment-710121446
> @Jaycean Please have a look at CI.
OK, I'll take a look at the reason. It passed the local test
----------------------------------------------------------------
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] liuxiran commented on pull request #2440: WIP:plugin(consumer-restriction): use draft7 way to rewrite the JSON Schema.
Posted by GitBox <gi...@apache.org>.
liuxiran commented on pull request #2440:
URL: https://github.com/apache/apisix/pull/2440#issuecomment-710740234
> According to the previous JSON schema data format, without changing the logic, the current writing method is not the same as the data obtained by apisix-dashboard. I need to discuss with @liuxiran again tomorrow
@Jaycean
with the newest json schema
```json
{
"oneOf": [{
"properties": {
"blacklist": {
"items": {
"type": "string"
},
"minItems": 1,
"type": "array"
},
"rejected_code": {
"default": 403,
"minimum": 200,
"type": "integer"
},
"type": {
"default": "consumer_name",
"enum": ["consumer_name", "service_id"],
"type": "string"
}
},
"required": ["blacklist"],
"title": "blacklist"
}, {
"properties": {
"rejected_code": {
"default": 403,
"minimum": 200,
"type": "integer"
},
"type": {
"default": "consumer_name",
"enum": ["consumer_name", "service_id"],
"type": "string"
},
"whitelist": {
"items": {
"type": "string"
},
"minItems": 1,
"type": "array"
}
},
"required": ["whitelist"],
"title": "whitelist"
}],
"properties": {
"disable": {
"type": "boolean"
}
},
"type": "object"
}
```
dashboard can create a route with `key-auth` plugin and `consumer-restriction` plugin successfully. and the `consumer-restriction` plugin can work correctly with the configuration.
If you can change the keys **order** in properties one from current:
`blacklist, rejected_code, type` to expected `rejected_code, type, blacklist`, that would be better :)
----------------------------------------------------------------
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] juzhiyuan commented on pull request #2440: WIP:plugin(consumer-restriction): use draft7 way to rewrite the JSON Schema.
Posted by GitBox <gi...@apache.org>.
juzhiyuan commented on pull request #2440:
URL: https://github.com/apache/apisix/pull/2440#issuecomment-710776224
the `disable` filed is an injection for every plugin's jsonschema, just waiting for @membphis's reply :)
----------------------------------------------------------------
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] juzhiyuan commented on pull request #2440: WIP:plugin(consumer-restriction): use draft7 way to rewrite the JSON Schema.
Posted by GitBox <gi...@apache.org>.
juzhiyuan commented on pull request #2440:
URL: https://github.com/apache/apisix/pull/2440#issuecomment-710752368
> > According to the previous JSON schema data format, without changing the logic, the current writing method is not the same as the data obtained by apisix-dashboard. I need to discuss with @liuxiran again tomorrow
>
> @Jaycean
> with your newest json schema
>
> ```json
> {
> "oneOf": [{
> "properties": {
> "blacklist": {
> "items": {
> "type": "string"
> },
> "minItems": 1,
> "type": "array"
> },
> "rejected_code": {
> "default": 403,
> "minimum": 200,
> "type": "integer"
> },
> "type": {
> "default": "consumer_name",
> "enum": ["consumer_name", "service_id"],
> "type": "string"
> }
> },
> "required": ["blacklist"],
> "title": "blacklist"
> }, {
> "properties": {
> "rejected_code": {
> "default": 403,
> "minimum": 200,
> "type": "integer"
> },
> "type": {
> "default": "consumer_name",
> "enum": ["consumer_name", "service_id"],
> "type": "string"
> },
> "whitelist": {
> "items": {
> "type": "string"
> },
> "minItems": 1,
> "type": "array"
> }
> },
> "required": ["whitelist"],
> "title": "whitelist"
> }],
> "properties": {
> "disable": {
> "type": "boolean"
> }
> },
> "type": "object"
> }
> ```
>
> dashboard can create a route with `key-auth` plugin and `consumer-restriction` plugin successfully. and the `consumer-restriction` plugin can work correctly with the configuration.
>
> And what is the purpose of the disable property below, which is not mentioned both in the [doc](https://github.com/apache/apisix/blob/master/doc/zh-cn/plugins/consumer-restriction.md) and in [test cases](https://github.com/apache/apisix/blob/master/t/plugin/consumer-restriction.t). it would be better to remove if it is no use:)
> cc @juzhiyuan @membphis
>
> ```json
> "properties": {
> "disable": {
> "type": "boolean"
> }
> }
> ```
hi please see here https://github.com/apache/apisix-dashboard/issues/495
----------------------------------------------------------------
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] liuxiran commented on pull request #2440: WIP:plugin(consumer-restriction): use draft7 way to rewrite the JSON Schema.
Posted by GitBox <gi...@apache.org>.
liuxiran commented on pull request #2440:
URL: https://github.com/apache/apisix/pull/2440#issuecomment-710756225
> hi please see here apache/apisix-dashboard#495
so you means apisix should not return disable filed until get request when FE needed? @juzhiyuan
it seems that when the schema of the plugin does not have a `properties` property in the first level of the json schema, the interface returns the data with `disable` field.
related code: https://github.com/membphis/apisix/blob/133b52ec78665b7292af62bab6af6bf981363a42/apisix/plugin.lua#L81
which doesn't seem reasonable, especially after json schema changed from draft5 to draft7.
in json schema draft7, the `oneof` `anyof` and `allof` type of schema e.g the schema in pr https://github.com/apache/apisix/pull/2440,whice does not contain `properties` in first level
cc @Jaycean
the https://github.com/apache/apisix/issues/2439 may also have something to do with it.
----------------------------------------------------------------
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] Jaycean edited a comment on pull request #2440: WIP:plugin(consumer-restriction): use draft7 way to rewrite the JSON Schema.
Posted by GitBox <gi...@apache.org>.
Jaycean edited a comment on pull request #2440:
URL: https://github.com/apache/apisix/pull/2440#issuecomment-710769509
> > > hi please see here [apache/apisix-dashboard#495](https://github.com/apache/apisix-dashboard/issues/495)
> >
> >
> > so you means apisix should not return disable filed until get request when FE needed? @juzhiyuan
> > it seems that when the schema of the plugin does not have a `properties` property in the first level of the json schema, the interface returns the data with `disable` field.
> > related code: https://github.com/membphis/apisix/blob/133b52ec78665b7292af62bab6af6bf981363a42/apisix/plugin.lua#L81
> > which doesn't seem reasonable, especially after json schema changed from draft5 to draft7.
> > in json schema draft7, the `oneof` `anyof` and `allof` type of schema e.g the schema in pr #2440 does not contain `properties` in first level
> > cc @Jaycean
> > the #2439 may also have something to do with it.
>
> hi @membphis, What do you think?
@liuxiran @juzhiyuan @membphis
Yes, Lua code is in `apisix/apisix/plugin.lua`
I think it's the code's judgment on the current schema structure that needs to be changed
However, I may not have a deep understanding of the whole project and the role of the disable field, so I have not thought about how to change the code logic
```
local function load_plugin(name, plugins_list, is_stream_plugin)
# line82-88
if plugin.schema and plugin.schema.type == "object" then
if not plugin.schema.properties or
core.table.nkeys(plugin.schema.properties) == 0
then
plugin.schema.properties = core.schema.plugin_disable_schema
end
end
# This is where the disables field is added. It seems that all schemas use this function
apisix/schema_def.lua
# function
_M.plugin_disable_schema = {
disable = {type = "boolean"}
```
Because in the JSON schema Draft 7, some structures do not contain `properties` in the first layer, but there are `properties` in the second layer, but the logic fails to judge. As a result, some schemas do not need to add the` disable` field
----------------------------------------------------------------
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] Jaycean edited a comment on pull request #2440: WIP:plugin(consumer-restriction): use draft7 way to rewrite the JSON Schema.
Posted by GitBox <gi...@apache.org>.
Jaycean edited a comment on pull request #2440:
URL: https://github.com/apache/apisix/pull/2440#issuecomment-710769509
> > > hi please see here [apache/apisix-dashboard#495](https://github.com/apache/apisix-dashboard/issues/495)
> >
> >
> > so you means apisix should not return disable filed until get request when FE needed? @juzhiyuan
> > it seems that when the schema of the plugin does not have a `properties` property in the first level of the json schema, the interface returns the data with `disable` field.
> > related code: https://github.com/membphis/apisix/blob/133b52ec78665b7292af62bab6af6bf981363a42/apisix/plugin.lua#L81
> > which doesn't seem reasonable, especially after json schema changed from draft5 to draft7.
> > in json schema draft7, the `oneof` `anyof` and `allof` type of schema e.g the schema in pr #2440 does not contain `properties` in first level
> > cc @Jaycean
> > the #2439 may also have something to do with it.
>
> hi @membphis, What do you think?
@liuxiran @juzhiyuan @membphis
Yes, Lua code is in `apisix/apisix/plugin.lua`
I think it's the code's judgment on the current schema structure that needs to be changed
However, I may not have a deep understanding of the whole project and the role of the disable field, so I have not thought about how to change the code logic
```
local function load_plugin(name, plugins_list, is_stream_plugin)
# line82-88
if plugin.schema and plugin.schema.type == "object" then
if not plugin.schema.properties or
core.table.nkeys(plugin.schema.properties) == 0
then
plugin.schema.properties = core.schema.plugin_disable_schema
end
end
```
Because in the JSON schema Draft 7, some structures do not contain `properties` in the first layer, but there are `properties` in the second layer, but the logic fails to judge. As a result, some schemas do not need to add the` disable` field
----------------------------------------------------------------
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] Jaycean edited a comment on pull request #2440: WIP:plugin(consumer-restriction): use draft7 way to rewrite the JSON Schema.
Posted by GitBox <gi...@apache.org>.
Jaycean edited a comment on pull request #2440:
URL: https://github.com/apache/apisix/pull/2440#issuecomment-710769509
> > > hi please see here [apache/apisix-dashboard#495](https://github.com/apache/apisix-dashboard/issues/495)
> >
> >
> > so you means apisix should not return disable filed until get request when FE needed? @juzhiyuan
> > it seems that when the schema of the plugin does not have a `properties` property in the first level of the json schema, the interface returns the data with `disable` field.
> > related code: https://github.com/membphis/apisix/blob/133b52ec78665b7292af62bab6af6bf981363a42/apisix/plugin.lua#L81
> > which doesn't seem reasonable, especially after json schema changed from draft5 to draft7.
> > in json schema draft7, the `oneof` `anyof` and `allof` type of schema e.g the schema in pr #2440 does not contain `properties` in first level
> > cc @Jaycean
> > the #2439 may also have something to do with it.
>
> hi @membphis, What do you think?
@liuxiran @juzhiyuan @membphis
Yes, Lua code is in `apisix/apisix/plugin.lua`
I think it's the code's judgment on the current schema structure that needs to be changed
However, I may not have a deep understanding of the whole project and the role of the disable field, so I have not thought about how to change the code logic
```
local function load_plugin(name, plugins_list, is_stream_plugin)
# line82-88
if plugin.schema and plugin.schema.type == "object" then
if not plugin.schema.properties or
core.table.nkeys(plugin.schema.properties) == 0
then
plugin.schema.properties = core.schema.plugin_disable_schema
end
end
```
Because in the JSON schema Draft 7, some structures do not contain `properties` in the first layer, but there are `properties` in the second layer, but the logic fails to judge. As a result, some schemas do not need to add the` disable` field
----------------------------------------------------------------
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] Jaycean edited a comment on pull request #2440: WIP:plugin(consumer-restriction): use draft7 way to rewrite the JSON Schema.
Posted by GitBox <gi...@apache.org>.
Jaycean edited a comment on pull request #2440:
URL: https://github.com/apache/apisix/pull/2440#issuecomment-710316191
According to the previous JSON schema data format, without changing the logic, the current writing method is not the same as the data obtained by apisix-dashboard. I need to discuss with @liuxiran again tomorrow
----------------------------------------------------------------
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] Jaycean commented on pull request #2440: WIP:plugin(consumer-restriction): use draft7 way to rewrite the JSON Schema.
Posted by GitBox <gi...@apache.org>.
Jaycean commented on pull request #2440:
URL: https://github.com/apache/apisix/pull/2440#issuecomment-710769509
> > > hi please see here [apache/apisix-dashboard#495](https://github.com/apache/apisix-dashboard/issues/495)
> >
> >
> > so you means apisix should not return disable filed until get request when FE needed? @juzhiyuan
> > it seems that when the schema of the plugin does not have a `properties` property in the first level of the json schema, the interface returns the data with `disable` field.
> > related code: https://github.com/membphis/apisix/blob/133b52ec78665b7292af62bab6af6bf981363a42/apisix/plugin.lua#L81
> > which doesn't seem reasonable, especially after json schema changed from draft5 to draft7.
> > in json schema draft7, the `oneof` `anyof` and `allof` type of schema e.g the schema in pr #2440 does not contain `properties` in first level
> > cc @Jaycean
> > the #2439 may also have something to do with it.
>
> hi @membphis, What do you think?
Yes, Lua code is in `apisix/apisix/plugin.lua`
I think it's the code's judgment on the current schema structure that needs to be changed
However, I may not have a deep understanding of the whole project and the role of the disable field, so I have not thought about how to change the code logic
```
local function load_plugin(name, plugins_list, is_stream_plugin)
# line82-88
if plugin.schema and plugin.schema.type == "object" then
if not plugin.schema.properties or
core.table.nkeys(plugin.schema.properties) == 0
then
plugin.schema.properties = core.schema.plugin_disable_schema
end
end
```
Because in the JSON schema Draft 7, some structures do not contain `properties` in the first layer, but there are `properties` in the second layer, but the logic fails to judge. As a result, some schemas do not need to add the` disable` field
----------------------------------------------------------------
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] liuxiran edited a comment on pull request #2440: WIP:plugin(consumer-restriction): use draft7 way to rewrite the JSON Schema.
Posted by GitBox <gi...@apache.org>.
liuxiran edited a comment on pull request #2440:
URL: https://github.com/apache/apisix/pull/2440#issuecomment-710740234
> According to the previous JSON schema data format, without changing the logic, the current writing method is not the same as the data obtained by apisix-dashboard. I need to discuss with @liuxiran again tomorrow
@Jaycean
with your newest json schema
```json
{
"oneOf": [{
"properties": {
"blacklist": {
"items": {
"type": "string"
},
"minItems": 1,
"type": "array"
},
"rejected_code": {
"default": 403,
"minimum": 200,
"type": "integer"
},
"type": {
"default": "consumer_name",
"enum": ["consumer_name", "service_id"],
"type": "string"
}
},
"required": ["blacklist"],
"title": "blacklist"
}, {
"properties": {
"rejected_code": {
"default": 403,
"minimum": 200,
"type": "integer"
},
"type": {
"default": "consumer_name",
"enum": ["consumer_name", "service_id"],
"type": "string"
},
"whitelist": {
"items": {
"type": "string"
},
"minItems": 1,
"type": "array"
}
},
"required": ["whitelist"],
"title": "whitelist"
}],
"properties": {
"disable": {
"type": "boolean"
}
},
"type": "object"
}
```
dashboard can create a route with `key-auth` plugin and `consumer-restriction` plugin successfully. and the `consumer-restriction` plugin can work correctly with the configuration.
And what is the purpose of the disable property below, which is not mentioned both in the [doc](https://github.com/apache/apisix/blob/master/doc/zh-cn/plugins/consumer-restriction.md) and in [test cases](https://github.com/apache/apisix/blob/master/t/plugin/consumer-restriction.t). it would be better to remove if it is no use:)
```json
"properties": {
"disable": {
"type": "boolean"
}
}
```
----------------------------------------------------------------
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] Jaycean edited a comment on pull request #2440: WIP:plugin(consumer-restriction): use draft7 way to rewrite the JSON Schema.
Posted by GitBox <gi...@apache.org>.
Jaycean edited a comment on pull request #2440:
URL: https://github.com/apache/apisix/pull/2440#issuecomment-710769509
> > > hi please see here [apache/apisix-dashboard#495](https://github.com/apache/apisix-dashboard/issues/495)
> >
> >
> > so you means apisix should not return disable filed until get request when FE needed? @juzhiyuan
> > it seems that when the schema of the plugin does not have a `properties` property in the first level of the json schema, the interface returns the data with `disable` field.
> > related code: https://github.com/membphis/apisix/blob/133b52ec78665b7292af62bab6af6bf981363a42/apisix/plugin.lua#L81
> > which doesn't seem reasonable, especially after json schema changed from draft5 to draft7.
> > in json schema draft7, the `oneof` `anyof` and `allof` type of schema e.g the schema in pr #2440 does not contain `properties` in first level
> > cc @Jaycean
> > the #2439 may also have something to do with it.
>
> hi @membphis, What do you think?
@liuxiran @juzhiyuan @membphis
Yes, Lua code is in `apisix/apisix/plugin.lua`,`apisix/schema_def.lua`
I think it's the code's judgment on the current schema structure that needs to be changed
However, I may not have a deep understanding of the whole project and the role of the disable field, so I have not thought about how to change the code logic
```
# `apisix/apisix/plugin.lua`
local function load_plugin(name, plugins_list, is_stream_plugin)
# line82-88
if plugin.schema and plugin.schema.type == "object" then
if not plugin.schema.properties or
core.table.nkeys(plugin.schema.properties) == 0
then
plugin.schema.properties = core.schema.plugin_disable_schema
end
end
# This is where the disables field is added. It seems that all schemas use this function
apisix/schema_def.lua
# function
_M.plugin_disable_schema = {
disable = {type = "boolean"}
```
Because in the JSON schema Draft 7, some structures do not contain `properties` in the first layer, but there are `properties` in the second layer, but the logic fails to judge. As a result, some schemas do not need to add the` disable` field
----------------------------------------------------------------
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 merged pull request #2440: plugin(consumer-restriction): use draft7 way to rewrite the JSON Schema.
Posted by GitBox <gi...@apache.org>.
membphis merged pull request #2440:
URL: https://github.com/apache/apisix/pull/2440
----------------------------------------------------------------
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 #2440: plugin(consumer-restriction): use draft7 way to rewrite the JSON Schema.
Posted by GitBox <gi...@apache.org>.
membphis commented on pull request #2440:
URL: https://github.com/apache/apisix/pull/2440#issuecomment-711113065
> Do you mean that the current function can directly remove this field?
You haven't added this field yet? I do not find it in the changed files: https://github.com/apache/apisix/pull/2440/files
----------------------------------------------------------------
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] Jaycean edited a comment on pull request #2440: plugin(consumer-restriction): use draft7 way to rewrite the JSON Schema.
Posted by GitBox <gi...@apache.org>.
Jaycean edited a comment on pull request #2440:
URL: https://github.com/apache/apisix/pull/2440#issuecomment-711140040
> > Do you mean that the current function can directly remove this field?
>
> You haven't added this field yet? I do not find it in the changed files: https://github.com/apache/apisix/pull/2440/files
My current PR does not modify the logic of this disable field. I intend to submit a new PR for modification
After all, the logic related to the disable field has nothing to do with the current PR, which first deals with the schema format of the plugin(consumer-restriction)
----------------------------------------------------------------
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] Jaycean commented on pull request #2440: plugin(consumer-restriction): use draft7 way to rewrite the JSON Schema.
Posted by GitBox <gi...@apache.org>.
Jaycean commented on pull request #2440:
URL: https://github.com/apache/apisix/pull/2440#issuecomment-711140040
> > Do you mean that the current function can directly remove this field?
>
> You haven't added this field yet? I do not find it in the changed files: https://github.com/apache/apisix/pull/2440/files
My current PR does not modify the logic of this disable field. I intend to submit a new PR for modification
----------------------------------------------------------------
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] liuxiran commented on pull request #2440: plugin(consumer-restriction): use draft7 way to rewrite the JSON Schema.
Posted by GitBox <gi...@apache.org>.
liuxiran commented on pull request #2440:
URL: https://github.com/apache/apisix/pull/2440#issuecomment-711265925
> My current PR does not modify the logic of this disable field. I intend to submit a new PR for modification
>
> After all, the logic related to the disable field has nothing to do with the current PR, which first deals with the schema format of the plugin(consumer-restriction)
Eagerly awaiting for a change in logic of this disable field😍, which will allow the dashboard better to use.
----------------------------------------------------------------
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] liuxiran edited a comment on pull request #2440: WIP:plugin(consumer-restriction): use draft7 way to rewrite the JSON Schema.
Posted by GitBox <gi...@apache.org>.
liuxiran edited a comment on pull request #2440:
URL: https://github.com/apache/apisix/pull/2440#issuecomment-710740234
> According to the previous JSON schema data format, without changing the logic, the current writing method is not the same as the data obtained by apisix-dashboard. I need to discuss with @liuxiran again tomorrow
@Jaycean
with your newest json schema
```json
{
"oneOf": [{
"properties": {
"blacklist": {
"items": {
"type": "string"
},
"minItems": 1,
"type": "array"
},
"rejected_code": {
"default": 403,
"minimum": 200,
"type": "integer"
},
"type": {
"default": "consumer_name",
"enum": ["consumer_name", "service_id"],
"type": "string"
}
},
"required": ["blacklist"],
"title": "blacklist"
}, {
"properties": {
"rejected_code": {
"default": 403,
"minimum": 200,
"type": "integer"
},
"type": {
"default": "consumer_name",
"enum": ["consumer_name", "service_id"],
"type": "string"
},
"whitelist": {
"items": {
"type": "string"
},
"minItems": 1,
"type": "array"
}
},
"required": ["whitelist"],
"title": "whitelist"
}],
"properties": {
"disable": {
"type": "boolean"
}
},
"type": "object"
}
```
dashboard can create a route with `key-auth` plugin and `consumer-restriction` plugin successfully. and the `consumer-restriction` plugin can work correctly with the configuration.
And what is the purpose of the disable property below, which is not mentioned both in the [doc](https://github.com/apache/apisix/blob/master/doc/zh-cn/plugins/consumer-restriction.md) and in [test cases](https://github.com/apache/apisix/blob/master/t/plugin/consumer-restriction.t). it would be better to remove if it is no use:)
cc @juzhiyuan @membphis
```json
"properties": {
"disable": {
"type": "boolean"
}
}
```
----------------------------------------------------------------
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] Jaycean commented on pull request #2440: WIP:plugin(consumer-restriction): use draft7 way to rewrite the JSON Schema.
Posted by GitBox <gi...@apache.org>.
Jaycean commented on pull request #2440:
URL: https://github.com/apache/apisix/pull/2440#issuecomment-710316191
According to the previous JSON schema data format, without changing the logic, the current writing method is not the same as the data obtained by the front end. I need to discuss with @liuxiran again tomorrow
----------------------------------------------------------------
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 #2440: plugin(consumer-restriction): use draft7 way to rewrite the JSON Schema.
Posted by GitBox <gi...@apache.org>.
membphis commented on pull request #2440:
URL: https://github.com/apache/apisix/pull/2440#issuecomment-711147574
thx 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] juzhiyuan commented on pull request #2440: WIP:plugin(consumer-restriction): use draft7 way to rewrite the JSON Schema.
Posted by GitBox <gi...@apache.org>.
juzhiyuan commented on pull request #2440:
URL: https://github.com/apache/apisix/pull/2440#issuecomment-710719221
> > @Jaycean Please have a look at CI.
>
> OK, I'll take a look at the reason. It passed the local test
Well, I will convert this PR to draft.
----------------------------------------------------------------
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] liuxiran edited a comment on pull request #2440: WIP:plugin(consumer-restriction): use draft7 way to rewrite the JSON Schema.
Posted by GitBox <gi...@apache.org>.
liuxiran edited a comment on pull request #2440:
URL: https://github.com/apache/apisix/pull/2440#issuecomment-710740234
> According to the previous JSON schema data format, without changing the logic, the current writing method is not the same as the data obtained by apisix-dashboard. I need to discuss with @liuxiran again tomorrow
@Jaycean
with your newest json schema
```json
{
"oneOf": [{
"properties": {
"blacklist": {
"items": {
"type": "string"
},
"minItems": 1,
"type": "array"
},
"rejected_code": {
"default": 403,
"minimum": 200,
"type": "integer"
},
"type": {
"default": "consumer_name",
"enum": ["consumer_name", "service_id"],
"type": "string"
}
},
"required": ["blacklist"],
"title": "blacklist"
}, {
"properties": {
"rejected_code": {
"default": 403,
"minimum": 200,
"type": "integer"
},
"type": {
"default": "consumer_name",
"enum": ["consumer_name", "service_id"],
"type": "string"
},
"whitelist": {
"items": {
"type": "string"
},
"minItems": 1,
"type": "array"
}
},
"required": ["whitelist"],
"title": "whitelist"
}],
"properties": {
"disable": {
"type": "boolean"
}
},
"type": "object"
}
```
dashboard can create a route with `key-auth` plugin and `consumer-restriction` plugin successfully. and the `consumer-restriction` plugin can work correctly with the configuration.
----------------------------------------------------------------
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 #2440: WIP:plugin(consumer-restriction): use draft7 way to rewrite the JSON Schema.
Posted by GitBox <gi...@apache.org>.
membphis commented on pull request #2440:
URL: https://github.com/apache/apisix/pull/2440#issuecomment-711012871
> the `disable` filed is an injection for every plugin's jsonschema, just waiting for @membphis's reply :)
we do not need to add this `disable` field in schema.
----------------------------------------------------------------
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] Jaycean commented on pull request #2440: plugin(consumer-restriction): use draft7 way to rewrite the JSON Schema.
Posted by GitBox <gi...@apache.org>.
Jaycean commented on pull request #2440:
URL: https://github.com/apache/apisix/pull/2440#issuecomment-711109884
> > the `disable` filed is an injection for every plugin's jsonschema, just waiting for @membphis's reply :)
>
> we do not need to add this `disable` field in schema.
OK, this field has no effect on the current pr. it affects all schemas. Do you mean that the current function can directly remove this field? If it can be removed, I will submit a new PR later.
----------------------------------------------------------------
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