anvilistas / anvil-labs

MIT License
9 stars 3 forks source link

Batch call tweaks #135

Open s-cork opened 1 year ago

s-cork commented 1 year ago

@jshaffstall

you can try this out just paste the batcher module from this PR into a client module then use it like this on the server

from . import batcher

@batcher.callable
def say_hello(name):
    ...

# or equivalently

@anvil.server.callable
@batcher.batchable
def say_hello(name):
    ....

then on the client


with batcher.batch_call() as c:
    c.call("say_hello", "foo")
    c.call("say_hello", "bar")

print(c.result)

TODO

jshaffstall commented 1 year ago

Thanks, I'll give that a try.

meatballs commented 1 year ago

@jshaffstall Did this work out ok for you? Shall we merge it?

jshaffstall commented 1 year ago

Sorry, end of the semester crunch here, I haven't had a chance to try this. Grades are due first of next week, I'll get a chance to try it then.

jshaffstall commented 1 year ago

If I have a server function that requires authentication, the order of the decorators seems to matter. This works as expected:

@batcher.batchable
@anvil.server.callable(require_user=True)

while this allows the call even if I'm not logged in:

@anvil.server.callable(require_user=True)
@batcher.batchable

I don't know if that's just a documentation issue, or something else.

I also tested with a custom decorator, and it also worked only if the @batcher.batchable decorator was first.

s-cork commented 1 year ago

yeah this seems like an oversight on my part - i'll have a think about that and ping you for an update.

I can't really see a way around this with the current api here. I don't think either order works with batchable and require_user=True. That's because the batchable decorator doesn't have direct access to the the function registered with anvil.server.callable.

My initial thinking is that i'll just have to reimplement the require_user logic 😔.

anvil.server.callable functions do exist in a private register, but it's private so relying on directly accessing these seems like a bad idea!

jshaffstall commented 1 year ago

Here's the clone link for the app where I was testing these: https://anvil.works/build#clone:2J2YQGYHOHU54UEK=YCIW4AZS5UTZASCOYEASOUCN

You can play with the order of decorators there. The one order does seem to work just fine, throwing the authentication exception when I'm not logged in and allowing the call when I am.

s-cork commented 1 year ago

@jshaffstall you can try the latest version now - just copy paste the batched module

note that I've removed the @batcher.batchable api in favour of @batched.callable(...)

this makes sure that any callable that has require_user=True in the batched calls will check permissions

jshaffstall commented 1 year ago

While that helps with the specific case of require_user, the order of decorators is still important for mixing with other decorators. This works:

@batched.callable
@test_complicated_decorator
def do_third_thing(foo, bar):
    return f"third thing done for {foo} and {bar}"

while this does not:

@test_complicated_decorator
@batched.callable
def do_third_thing(foo, bar):
    return f"third thing done for {foo} and {bar}"

Not a big deal, as long as it's documented. You can see the working version in the clone I posted above.

s-cork commented 1 year ago

Noted.

I don't really see how this example is different if you replace batched.callable with anvil.server.callable though.

The behaviour differs depending on the order there too.

Or am I missing something?

ie


@anvil.server.callable
@test_complicated_decorator

@test_complicated_decorator
@anvil.server.callable

Are different in the same way as the @batched.callable examples are different.

jshaffstall commented 1 year ago

As I say, the order being important isn't a big deal, but should be documented. The order being important for anvil.server.callable wasn't that I found when I started writing my own decorators for common pre-checks.

jshaffstall commented 1 year ago

One side effect of batched.callable is that the Anvil autocompleter doesn't know about the server calls now for direct anvil.server.call. The call works, you just have to type out the function name instead of picking it from the autocompleter.

s-cork commented 1 year ago

Good point. Then I think the recommendation might be to duplicate rather than replace the decorator. Putting both decorators on the callable doesn't impact how the callable works. But you'll get autocompletion this way.

jshaffstall commented 1 year ago

So something like:

@batched.callable(require_user=True)
@test_complicated_decorator
@anvil.server.callable(require_user=True)

where if the require_user=True is not repeated, then one of the ways of calling the function will not require authentication? As long as that's documented, it seems fine to me.

s-cork commented 1 year ago

I think the documentation would say that call signature should match exactly. batched.callable is a wrapper around anvil.server.callable. And adding 2 of them would behave in the same way as having 2 anvil.server.callable decorators. The top decorator will override the behaviour of the lower decorators.

jshaffstall commented 1 year ago

Ah, so this works fine:

@batched.callable(require_user=True)
@test_complicated_decorator
@anvil.server.callable

The anvil.server.callable exists only for the autocompleter. As long as the docs have an example of more complicated usage, I think people will be fine.