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

Do not introspect operation function parameters #268

Closed

Conversation

MarcosEnevo
Copy link

@MarcosEnevo MarcosEnevo commented Aug 30, 2016

Hi, we are using Connexion at Enevo and we are quite happy, but there is something limiting us if we want to use it for more than simple endpoint-to-function mappings, which is introspection of target function parameters.

For example, if Connexion is introspecting the target function parameters we are not able to apply decorators with the usual (*args, **kwargs) since then nothing will be passed.

There is no reason to perform this extra check, if an input is defined in the OpenAPI file as required then the target function should have a required parameter, and if there is a optional input defined then the target function should have a parameter with a default (most likely None).

We think with this change Connexion will be more flexible and powerful, so please consider it.

Thanks.

@hjacobs
Copy link
Contributor

hjacobs commented Aug 30, 2016

@MarcosEnevo thanks for using Connexion. We'll look into it. Obviously the tests now fail, but I think we should discuss the actual change proposal first.

@MarcosEnevo
Copy link
Author

@hjacobs Yep, I just ran "py.test" locally and apparently that's not enough. But looks like easy fixes.

Please keep us in the loop.

Thanks.

@hjacobs
Copy link
Contributor

hjacobs commented Aug 30, 2016

@MarcosEnevo I think we should fix the argument inspection to honor *args and **kwargs instead of removing it altogether. We just need to check the parameter kind (https://docs.python.org/3/library/inspect.html#inspect.Parameter).

@MarcosEnevo
Copy link
Author

MarcosEnevo commented Aug 30, 2016

@hjacobs that would be nice, and would solve the specific problem we have now, even though it could not solve other potential issues trying to build more complex services.

I still see no point of doing that extra validation, since inputs are already perfectly defined in the OpenAPI file, what's the purpose of allowing operation functions that does not follow the spec? specially when we are trying to be OpenAPI first.

IMHO param introspection It is not adding any value, only limiting how the lib can be used.

Thanks again for your time.

@hjacobs
Copy link
Contributor

hjacobs commented Aug 30, 2016

@jmcs @rafaelcaricio any opinions on this?

@coveralls
Copy link

coveralls commented Aug 30, 2016

Coverage Status

Coverage remained the same at 100.0% when pulling cb303e6 on MarcosEnevo:not-introspect-op-function into d6c4d47 on zalando:master.

@coveralls
Copy link

coveralls commented Aug 30, 2016

Coverage Status

Coverage remained the same at 100.0% when pulling cb59b23 on MarcosEnevo:not-introspect-op-function into d6c4d47 on zalando:master.

1 similar comment
@coveralls
Copy link

Coverage Status

Coverage remained the same at 100.0% when pulling cb59b23 on MarcosEnevo:not-introspect-op-function into d6c4d47 on zalando:master.

@coveralls
Copy link

coveralls commented Aug 30, 2016

Coverage Status

Coverage remained the same at 100.0% when pulling d76a64b on MarcosEnevo:not-introspect-op-function into d6c4d47 on zalando:master.

@coveralls
Copy link

coveralls commented Aug 30, 2016

Coverage Status

Coverage remained the same at 100.0% when pulling d76a64b on MarcosEnevo:not-introspect-op-function into d6c4d47 on zalando:master.

@jmcs
Copy link
Contributor

jmcs commented Aug 30, 2016

You can get your decorators to expose the right signature with https://pypi.python.org/pypi/decorator I never tried it with connexion but I see no reason why it wouldn't work.

@MarcosEnevo
Copy link
Author

MarcosEnevo commented Aug 31, 2016

Oh course we could use 3rd party libraries to workaround the issue, we could also use compile(), ASTs or even eval(), or other 30 nasty workarounds to create functions dynamically with the parameters names Connexion is expecting.

But again, why Connexion is expecting those parameters there? why is that feature needed? Why not just relying in the OpenAPI definition (since this is OpenAPI-first)?

@hjacobs
Copy link
Contributor

hjacobs commented Aug 31, 2016

@MarcosEnevo one good reason IMHO to check the function signature is too allow having a small number of params in the function signature only (maybe only path params) and using the flask.request object to get parameters when needed.

I propose adding *args and **kwargs support, this would fix your issue and still allow the mentioned use case.

@MarcosEnevo
Copy link
Author

@hjacobs Ok, fair enough. So how should we continue to get *args, **kwargs support?

Should we create an issue?

Thanks.

@hjacobs
Copy link
Contributor

hjacobs commented Aug 31, 2016

@MarcosEnevo done: #270

@MarcosEnevo MarcosEnevo closed this Sep 1, 2016
@hjacobs
Copy link
Contributor

hjacobs commented Sep 1, 2016

@MarcosEnevo #271 should fix your issue with **kwargs.

@MarcosEnevo
Copy link
Author

@hjacobs Thanks a lot

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.

4 participants