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

url_for functionality for justpy routing using starlette #478

Closed
WolfgangFahl opened this issue Aug 30, 2022 · 13 comments
Closed

url_for functionality for justpy routing using starlette #478

WolfgangFahl opened this issue Aug 30, 2022 · 13 comments
Assignees
Labels
core enhancement New feature or request
Milestone

Comments

@WolfgangFahl
Copy link
Collaborator

WolfgangFahl commented Aug 30, 2022

@sandeep-gh in #389 suggested that the following Unit Test should work:

'''
Created on 2022-08-30

@author: 
@author: wf
'''
import justpy as jp
from starlette.testclient import TestClient

@jp.SetRoute("/bye", name = "bye")
def bye_function(_request):
    wp = jp.WebPage()
    wp.add(jp.P(text='Hello there!', classes='text-5xl m-2'))
    return wp

@jp.SetRoute("/hello", name = "hello")
def hello_function(request):
    wp = jp.WebPage()
    wp.add(jp.P(text='Hello there!', classes='text-5xl m-2'))
    print("request  = ", request.url_for("bye"))
    return wp

app = jp.app
client = TestClient(app)
client.get("/hello")
@WolfgangFahl WolfgangFahl added enhancement New feature or request core labels Aug 30, 2022
@WolfgangFahl WolfgangFahl added this to the 0.4.0 milestone Aug 30, 2022
@WolfgangFahl WolfgangFahl self-assigned this Aug 30, 2022
@WolfgangFahl
Copy link
Collaborator Author

Which currently fails with:

starlette.routing.NoMatchFound: No route exists for name "bye" and params "".

due to the decision to evade the startlette Routing System.
As far as i understand the design decision was to be able to check functions assigned to routes

 for route in Route.instances:
            func = route.matches(request['path'], request)
            if func:
                func_to_run = func
                break

@WolfgangFahl
Copy link
Collaborator Author

So i think we need to refactor the routing system to tap into starlette routing's system and still allow to check the function that is assigned to a route. I assume this might be possible by using inheritance instead of the current cut&paste+modify approach. It's also a good opportunity to use design for testability.

WolfgangFahl added a commit that referenced this issue Aug 30, 2022
@WolfgangFahl WolfgangFahl reopened this Aug 30, 2022
@WolfgangFahl
Copy link
Collaborator Author

Interface is used wrongly - downstream match returns a tuple

@WolfgangFahl
Copy link
Collaborator Author

url_for still does not work since starlette needs a routing table at startup now see:

https://www.starlette.io/routing/

@sandeep-gh
Copy link

Whats the status on this? Should we even have justpy own's Route class? What additional functionality will it provide? Plus, there is more advanced aspect of routing related to mounts/etc. If we roll our own Route, would have eventually deal with mounts and related issues as well.

@sandeep-gh
Copy link

My current fix is not to use a decorator but a function called castAsEndpoint. You can check it out the solution here.

@WolfgangFahl
Copy link
Collaborator Author

WolfgangFahl commented Sep 1, 2022

@sandeep-gh
IMHO we should use startlette's functionality as much as we can to avoid cut&paste and complicated code.
The SetRoute is still in place for compatibilty Route has been renamed to JpRoute and inherits from starlettes route. We now have to initialized jp.app correctly and should be able to use standard starlette features again. JpRoute might even not be necessary any more if we refactor properly.

from starlette.routing import Route, Match
import typing

class JpRoute(Route):
    '''
    extends starlette Routing
    
    see 
       https://www.starlette.io/routing/
    
       https://github.com/encode/starlette/blob/master/starlette/routing.py
    '''
    # map for all routes that are defined
    routesByPath={}
    
    @classmethod
    def reset(cls):
        JpRoute.routesByPath={}
        
    @classmethod
    def getFuncForRequest(cls,request):
        '''
        get the function for the given request
        
        Args:
            request: the starlette request
            
        Returns:
            Callable: the function that is bound to the path of the given request
        '''
        scope=request.scope
        return JpRoute.getFuncForScope(scope)
    
    @classmethod
    def getFuncForScope(cls,scope):
        '''
        get the function (endpoint in starlette jargon) for the given scope
        
        Args:
            path: the path to check
        Returns:
            Callable: the function that is bound to the given path 
        '''
        for _path,route in JpRoute.routesByPath.items():
            match,_matchScope=route.matches(scope)
            if match is not Match.NONE:
                func_to_run=route.endpoint
                return func_to_run
        return None
     
    def __init__(self, path: str, endpoint: typing.Callable,**kwargs):
        '''
        constructor
        '''
        # call super constructor
        Route.__init__(self, path=path,endpoint=endpoint,**kwargs)
        # remember my routes 
        JpRoute.routesByPath[path]=self
        
    def __repr__(self):
        return f'{self.__class__.__name__}(name: {self.name}, path: {self.path}, format: {self.path_format}, func: {self.endpoint.__name__}, regex: {self.path_regex})'
  
class SetRoute:
    '''
    Justpy specific route annotation
    '''

    def __init__(self, route, **kwargs):
        '''
        constructor
        
        Args:
            route(Route): the starlette route to set
            **kwargs: Arbitrary keyword arguments.
        '''
        self.route = route
        self.kwargs = kwargs

    def __call__(self, fn, **_instance_kwargs):
        '''
        Args:
            fn(Callable): the function
            **_instance_kwargs: Arbitrary keyword arguments (ignored).
        
        '''
        # create a new route
        JpRoute(path=self.route, endpoint=fn,  name=self.kwargs.get('name', None))
        return fn

@sandeep-gh
Copy link

Hi Wolfgang,

Have you settled on a approach for this? What would the final interface look like to the end programmer?

@WolfgangFahl
Copy link
Collaborator Author

#502 is in the works. Your "castAsEndpoint" idea IMHO is on the right track. Currently i am in the process or refactoring the HomePage Class to a JustpyEndpoint in jpcore. The nasty namespace problem of #486 is making progress very slow.

We'll have to define the approach we want to take to let endusers know which style of routing we recommend.
using the https://github.com/MrPigss/DecoRouter/blob/main/src/decoRouter/decoRouter.py approach might be a way. It's a pitty that the starlette designers have been inconsistent in their design choices and many people have followed the decorator style (some inspired by flask) ...

@sandeep-gh
Copy link

On import *:

Its safer to not use import *, otherwise we have to reason out all the bad implications of import * which is going to be very cumbersome. I understand that user can do "jp." in the IDE and discover what all keywords/features are present in jp. This is basically flatting and inverse of a hierarchy. Maybe there is some other way to provide this functionality -- something like jp.allkw.* -- In case user wants to quickly search and zero in on a keyword.

On routing:

Lets spell out positive/drawbacks of using castAsEndpoint approach vs. using own rolled out JpRoute.
CastAsEndpoint is a less code approach and completely relies on Starlette for all is routing. So in that sense its limited in functionality via Starlette.
Is the JpRoute approach more flexible then the CastAsEndpoint approach?

on DecoRouter

I like this approach. Its less typing for enduser and keeps relevant things in one place.

@WolfgangFahl
Copy link
Collaborator Author

WolfgangFahl commented Sep 13, 2022

The JpRoute approach doesn't even work properly. The refactoring is now at a point where we should be able get your buildResponse/castAsEndPoint approach integrated since there is now a JustpyEndpoint class on which we can focus.

I'd love to get this done in very smalls steps to make sure we don't break anything. I see e.g. that you add beautification and other aspects such as BasicAuthentication. Would you please open issues for each of these separately so that we can make progress towards a common solution.

@sandeep-gh
Copy link

I was still playing around with the BasicAuthentication. Let me open an issue and we can discuss what aspects we want to pull in.
What beautification you noticed?

@WolfgangFahl
Copy link
Collaborator Author

@sandeep-gh part of the discussion here has to move e.g. if you'd like to propse the DecoRouter approach. The beautifier code if found was in your link:

import jsbeautifier
import json
opts = jsbeautifier.default_options()
...

and i think it was used again later in the code. If you'd like this beautifying please open a separate issue. This issue is closed for release 0.9.0

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
core enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

3 participants