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

add support for user defined operators #590

Closed
wants to merge 1 commit into from

Conversation

porterjamesj
Copy link

Hi! Thank you for this project, I used it a while back to good effect. One thing I ran that wasn't supported out of the box was extending searching with "custom" operators. In my case, I was using PostGIS and geoalchemy, and needed to be able to query the API using geographic regions. I ended up hacking extra operators in using something like:

import flask_restless

def _monkey_patch_restless_search_ops(module):
    module.OPERATORS["my_favorite_operator"] = my_favorite_op_function

_monkey_patch_restless_search_ops(flask_restless.search)

This patch extends the project to support this sort of thing on a per-model basis via a custom_operators keyword argument to create_api_blueprint.

Is this something you'd be interested in having? No worries if not, just figured I'd throw it out there ^_^

This PR is mostly just to demonstrate that it's possible. I have no illusions that this is the best implementation, passing custom_operators all over the place was just the quickest way to make it work since the filtering code right now is blissfully unaware of the existence of APIBase, etc. I am happy to work up a better implementation if desired!

in any case, my definite TODOs if you'd like me to move forward with this are:

  1. more tests
  2. documentation

let me know, cheers!

@jfinkels
Copy link
Owner

jfinkels commented Oct 5, 2016

In general, this seems like a reasonable feature request to me.

passing custom_operators all over the place was just the quickest way to make it work since the filtering code right now is blissfully unaware of the existence of APIBase,

Yeah, this does seem like a lot of pass-throughs for no real benefit. (But I'm happy that the filtering code seems "blissfully unaware" to you, that was a design goal!) Perhaps just create a simple

def register_operator(name, function):
    OPERATORS[name] = function

in operators.py (basically the same as your monkey-patch solution). Then the user would call register_operator('mycooloperator', my_cool_func) anywhere before the Flask app runs. This solution is not model-dependent, but I don't feel all that bad about that. Keep it simple until a more complicated solution is necessary. What do you think?

In your geographic oriented use case, did you create a new operator or did you override an existing one? Was it important that the operator was model-dependent?

Thanks for your contribution!

@porterjamesj
Copy link
Author

I was going to say I'm dubious about register_operator, since in my case the operator only made sense for one model, since the operator only worked on geoalchemy types and only one of my models had those. Now that I think about it though, a similar situation obtains with the builtin operators—you can imagine only having one model with a numeric field, and yet ">" is builtin and useable on any model / field.

I can foresee maaaaybe wanting to scope operators to only be useable in certain APIs, as opposed to globally, but that seems like a niche enough use-case that register_operator will work for now. I will go ahead and implement that!

@porterjamesj
Copy link
Author

The one other question I have is around arity. The most basic implementation of this would just assume that all user defined operators are binary. A couple more sophisticated options:

  1. when you register an operator you do something like register_operator("cool", my_cool_op, arity=1) and then we know how to call it.
  2. we figure this out "automagically" using the inspect module. To me this seems like a bit too much and fraught with danger.

any thoughts on this? I think I'm leaning towards option 1 above, it seems not too hard to implement and a substantial improvement.

@jfinkels
Copy link
Owner

jfinkels commented Oct 7, 2016

I would start with the simplest implementation first, just register_operator(name, op). If there is an arity mismatch, it should cause an error which will be caught in the search subpackage and propagated up through the view back to the client; that's basically what happens now I think. If we need to modify it in the future, we'll make changes then.

@porterjamesj
Copy link
Author

that sounds good! i am pretty busy tomorrow, but i will try to do this over the weekend or next week

@jfinkels
Copy link
Owner

Pull request #620 has a simpler version of this with documentation. Please add any comments there.

@jfinkels jfinkels closed this Dec 26, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants