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

fix(schema) infer shorthands #6352

Closed
wants to merge 1 commit into from
Closed

fix(schema) infer shorthands #6352

wants to merge 1 commit into from

Conversation

bungle
Copy link
Member

@bungle bungle commented Sep 16, 2020

Summary

In issue #6272 it was reported that some of our new shorthands that were added to 2.1.x (whitelist/blacklist -> allow/deny) do not behave correctly with application/x-www-form-urlencoded or multipart/form-data.

I considered several options to fix these, including:

  1. Fix each shorthands function to change the types (to arrays in this case):
    felt like just repetitive code, and a bug waiting to happen again
  2. Change arguments parser to call shorthand functions and infer results:
    would fix it only for code that uses arguments parser
  3. Infer shorthands return values according to schema field:
    Does always the inferring for shorthands return values if there is a matching field
  4. Change plugins api to do inferring there only to config table values:
    is not universal solution, and makes plugins config behave differently than rest

Out of those I went with 3.

While inferring values in general should happen only for application/x-www-form-urlencoded and multipart/form-data, I don't think inferring the shorthand function return values is all that bad. It is a little bit magical, but I could not think it being problematic.

Issues Resolved

Fix #6272

kong/meta.lua Outdated Show resolved Hide resolved
### Summary

In issue #6272 it was reported that some of our new shorthands that were added to
`2.1.x` (whitelist/blacklist -> allow/deny) do not behave correctly with
`application/x-www-form-urlencoded` or `multipart/form-data`.

I considered several options to fix these, including:

1. Fix each shorthands function to change the types (to arrays in this case):
   felt like just repetitive code, and a bug waiting to happen again
2. Change arguments parser to call shorthand functions and infer results:
   would fix it only for code that uses arguments parser
3. Infer shorthands return values according to schema field:
   Does always the inferring for shorthands return values if there is a
   matching field
4. Change plugins api to do inferring there only to config table values:
   is not universal solution, and makes plugins config behave differently
   than rest

 Out of those I went with 3.

 While inferring values in general should happen only for
 `application/x-www-form-urlencoded` and `multipart/form-data`, I don't think
 inferring the shorthand function return values is all that bad. It is a little
 bit magical, but I could not think it being problematic.

 ### Issues Resolved

 Fix #6272
@hishamhm
Copy link
Contributor

While inferring values in general should happen only for application/x-www-form-urlencoded and multipart/form-data, I don't think inferring the shorthand function return values is all that bad. It is a little bit magical, but I could not think it being problematic.

I am concerned about this. It loosens the type checking for shorthands all over the place (declarative config loading, db_import, etc.) and might bite us back in the future in an unrelated area because of something that was done only because of application/x-www-form-urlencoded or multipart/form-data in the Admin API. I think the effects of inference should be restricted to that. The option that would achieve this is option 2, right?

@bungle
Copy link
Member Author

bungle commented Sep 16, 2020

I am concerned about this. It loosens the type checking for shorthands all over the place (declarative config loading, db_import, etc.) and might bite us back in the future in an unrelated area because of something that was done only because of application/x-www-form-urlencoded or multipart/form-data in the Admin API. I think the effects of inference should be restricted to that. The option that would achieve this is option 2, right?

Yes, it loosens the type checking as shorthands by definition are loosened. There is no field definition for shorthands.
Shorthand is a quickie for devs, and some automation would probably be nice.

Option 2 is also a bit tricky. It copies same code to other place, and the order might then be different in admin api and that code is executed twice (it could affect error messages too), and in general dao api. It might as well bite us. It adds shorthands to admin api separately. Shorthands doesn't really play nice in different scenarios, its design feels off.

@hishamhm
Copy link
Contributor

Very fair points! Let me give this a little more thought and see if I can come up with any proposal.

@bungle
Copy link
Member Author

bungle commented Sep 18, 2020

This PR is superseded by #6364, thus closing it.

@bungle bungle closed this Sep 18, 2020
@bungle bungle deleted the fix/shorthands branch September 18, 2020 12:00
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.

After upgrading from 2.0.5 to 2.1.3 the deprecation system of “whitelist” -> “allow” does not work
3 participants