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

Batch call tweaks #135

Draft
wants to merge 3 commits into
base: main
Choose a base branch
from
Draft

Batch call tweaks #135

wants to merge 3 commits into from

Conversation

s-cork
Copy link
Contributor

@s-cork s-cork commented Apr 17, 2023

@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

  • docs

@jshaffstall
Copy link

Thanks, I'll give that a try.

@meatballs
Copy link
Member

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

@jshaffstall
Copy link

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
Copy link

jshaffstall commented May 8, 2023

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
Copy link
Contributor Author

s-cork commented May 9, 2023

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
Copy link

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
Copy link
Contributor Author

s-cork commented May 9, 2023

@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
Copy link

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
Copy link
Contributor Author

s-cork commented May 9, 2023

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
Copy link

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.

@s-cork s-cork marked this pull request as draft May 9, 2023 14:24
@jshaffstall
Copy link

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
Copy link
Contributor Author

s-cork commented May 9, 2023

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
Copy link

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
Copy link
Contributor Author

s-cork commented May 9, 2023

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
Copy link

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.

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.

3 participants