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

feat: get schema by schema_type #2509

Merged
merged 7 commits into from
Oct 29, 2020

Conversation

Jaycean
Copy link
Member

@Jaycean Jaycean commented Oct 24, 2020

What this PR does / why we need it:

Pre-submission checklist:

  • Did you explain what problem does this PR solve? Or what new features have been added?
  • Have you added corresponding test cases?
  • Have you modified the corresponding document?
  • Is this PR backward compatible?

@Jaycean Jaycean force-pushed the get_schema_by_schema_type branch from f83527b to 8c15430 Compare October 24, 2020 08:29
@Jaycean Jaycean changed the title feat: get schema by schema_type WIP: feat: get schema by schema_type Oct 24, 2020
@Jaycean Jaycean force-pushed the get_schema_by_schema_type branch from 8c15430 to 150336f Compare October 26, 2020 01:41
@Jaycean Jaycean force-pushed the get_schema_by_schema_type branch from 6218bf6 to 89c87e2 Compare October 26, 2020 02:31
@Jaycean Jaycean force-pushed the get_schema_by_schema_type branch from 2c962be to cfbf93d Compare October 26, 2020 03:00
@Jaycean Jaycean changed the title WIP: feat: get schema by schema_type feat: get schema by schema_type Oct 26, 2020
@Jaycean
Copy link
Member Author

Jaycean commented Oct 26, 2020

@membphis PTAL,Thx

@Jaycean
Copy link
Member Author

Jaycean commented Oct 26, 2020

@membphis I have some questions to discuss with you.

The method I currently implement is to directly determine whether to return consumer_schema through whether the query field has schema_type=consumer in apisix/admin/plugins.lua.

    local arg = get_uri_args()
    local json_schema = plugin.schema
    if arg and arg["schema_type"] == "consumer" then
        json_schema = plugin.consumer_schema
    end

#2441 (comment)
I thought of another implementation method to obtain the corresponding schema directly through the value of schema_type, but if Obtain directly through the value of schema_type, then you need to pass the corresponding complete schema_type:/apisix/admin/schema/plugins/{plugin_name}?schema_type=consumer_schema
Instead of: /apisix/admin/schema/plugins/{plugin_name}?schema_type=consumer

@membphis
Copy link
Member

/apisix/admin/schema/plugins/{plugin_name}?schema_type=consumer

I think this way is simpler and enough.

Copy link
Member

@juzhiyuan juzhiyuan left a comment

Choose a reason for hiding this comment

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

LGTM

@liuxiran
Copy link
Contributor

LGTM

tips:
after this pr merged, jwt-auth also needs to add consumer_schema to the _M @Jaycean

@Jaycean
Copy link
Member Author

Jaycean commented Oct 29, 2020

LGTM

tips:
after this pr merged, jwt-auth also needs to add consumer_schema to the _M @Jaycean

OK, I will submit a new PR unified addition to consumer_schema support

@juzhiyuan juzhiyuan merged commit d2aa1d8 into apache:master Oct 29, 2020
@juzhiyuan
Copy link
Member

LGTM

tips:
after this pr merged, jwt-auth also needs to add consumer_schema to the _M @Jaycean

  • TODO this issue

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

plugin(hmac-auth): update the schema
4 participants