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

Convert functions to accept RequestHandlers #311

Merged
merged 9 commits into from
Oct 25, 2020
Merged

Convert functions to accept RequestHandlers #311

merged 9 commits into from
Oct 25, 2020

Conversation

jaidevd
Copy link
Member

@jaidevd jaidevd commented Oct 14, 2020

See #304
This PR adds a decorator: gramex.handlers.functionhandler.add_handler which wraps any callable to:

  1. get it's args and kwargs from handler.path_args and handler.request.body
  2. return a JSON object
  3. enforce any type annotations found in the callable

ToDo:

  • Not exhaustively tested for POST / DELETE

@jaidevd jaidevd requested a review from sanand0 October 14, 2020 05:24
def wrapper(handler):
args = handler.path_args
if handler.request.method == 'GET':
kwargs = {k: v[0] for k, v in handler.args.items()}
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

handler.get_arg does this.

Copy link
Member Author

@jaidevd jaidevd Oct 14, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • Test all HTTP methods.
  • Test all native types including None.
  • Test what happens with default named arguments.
  • ?x=1&x=2&x=3 should be doable. Perform the typecasting from the hints.

def wrapper(handler):
args = handler.path_args
if handler.request.method == 'GET':
kwargs = {k: v[0] for k, v in handler.args.items()}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perhaps {k: v if len(v) > 1 else v[0] for k, v in handler.args.items()} to handle ?x=1&x=2&y=3?

Copy link
Member Author

@jaidevd jaidevd Oct 15, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@sanand0 This ^ works, obviously. handler.get_arg('x') only gets the last value. Is that intended or is it a bug? Sounds to me a bit like what's at the heart of #312

args = [argtype(k) for k in args]
else:
kwargs[arg] = argtype(kwargs[arg])
return json.dumps(func(*args, **kwargs))
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thoughts on using pydantic? See this example https://pydantic-docs.helpmanual.io/usage/validation_decorator/

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@pratapvardhan this is great... I can offload all the typechecking / casting off to pydantic.validate_arguments.
Type validation should be on by default, and in that case, pydantic should do it. But users should also be able to turn it off if required, like:

@add_handler(validate=False)
def slow_sum(*numbers: float, delay: int):
   s = 0
   for i in numbers:
       time.sleep(delay)
       s += 0
       yield s

This ^ snippet won't complain when numbers is not floats, say.

@sanand0 Thoughts?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No inputs -- and we can defer this.

@jaidevd
Copy link
Member Author

jaidevd commented Oct 17, 2020

@sanand0 items from the last review are done. Some notes on usage are in the docstring for add_handler. Please take a look.

@sanand0
Copy link
Contributor

sanand0 commented Oct 21, 2020

  • Move add_handler as gramex.transforms.handler
  • Fix typos in docstring
    • greet.,py -> greet.py
    • funtion -> function
    • andd -> and
    • Please check for others
  • Change usage to:
pattern: /$YAMLURL/greet
handler: Function
kwargs:
    function: greet.birthday
  • This test case should
def total_list(items: List[float], start: float) -> float:
    return sum(items) + start
  • In POST and PUT requests, URL parameters and path arguments should be used. Add test cases

@sanand0
Copy link
Contributor

sanand0 commented Oct 21, 2020

These scenarios don't yet work.

url:
  power:
    pattern: /power
    handler: FunctionHandler
    kwargs:
      function: alpha.power
      kwargs:
        x: 3

  message:
    pattern: /message
    handler: FunctionHandler
    kwargs:
      function: alpha.messages
@handler
def power(x: int, y:float) -> float:
    return y ** x


@handler
def messages(s: list):
    for msg in s:
        yield msg

@jaidevd
Copy link
Member Author

jaidevd commented Oct 21, 2020

@sanand0 Added these two cases, and tests for them.

@sanand0
Copy link
Contributor

sanand0 commented Oct 25, 2020

I made these changes:

  • Make all handler names, URL patterns and function names identical
  • Move @handler methods from test_function.py to functionutils.py. This is to allow manual testing. Specifically, if cd tests and run gramex to visit localhost:9999, Gramex can't import tests.test_function because it has a line from . import TestGramex.
  • Split tests into a test_functionhandler.TestWrapper class so we can run them independently
  • If ?x=1&x=2 are passed to func(x: int), then the last parameter is used, not the first
  • POST body is processed as JSON only if Content-Type: application/json. This is consistent with FormHandler. It also retains the URL query parameters apart from the body
  • Modify FunctionHandler to return list, int, float, etc.
  • If we call /func/total/10/20?items=30&items=40 it will add 10+20+30+40=100. This required a rewrite of the code base.

@sanand0 sanand0 merged commit 8257915 into master Oct 25, 2020
@sanand0 sanand0 deleted the jd-add-handler branch October 25, 2020 12:08
@sanand0
Copy link
Contributor

sanand0 commented Oct 25, 2020

Note: Checks failed because Travis couldn't access the Microsoft R repository. Even master fails despite the same commit passing earlier.

This is because requests.get('https://cran.microsoft.com/src/contrib/PACKAGES') fails due to an SSL issue.

So I've gone ahead and merged.

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.

None yet

3 participants