Support link generation #152

Altai-man · 2021-08-26T12:25:01Z

No description provided.

jnthn · 2021-08-27T12:45:08Z

lib/Cro/HTTP/Router.pm6

@@ -58,6 +65,12 @@ module Cro::HTTP::Router {
       has Str $.id is required;
    }

+    our $link-plugin is export(:link) = router-plugin-register('link');


Does it have to be our, given we're exporting it?

[Cro::HTTP] ===SORRY!=== Error while compiling /home/koto/Work/cro/cro-http/lib/Cro/HTTP/Router.pm6 (Cro::HTTP::Router) [Cro::HTTP] Can't apply trait 'is export' on a my scoped variable. Only our scoped variables are supported. [Cro::HTTP] at /home/koto/Work/cro/cro-http/lib/Cro/HTTP/Router.pm6 (Cro::HTTP::Router):68

jnthn · 2021-08-27T12:50:11Z

lib/Cro/HTTP/Router.pm6

+                        # my $*CRO-ROUTER-ROUTE-HANDLER = $handler;
+                        # my %*URLS = $handler ~~ DelegateHandler ?? %() !! router-plugin-get-configs($link-plugin);


Leftover commented code

jnthn · 2021-08-27T13:04:00Z

lib/Cro/HTTP/Router.pm6

                for $includee.handlers() {
-                    @!handlers.push: .copy-adding(:@prefix, :@!body-parsers, :@!body-serializers,
-                        :@!before-matched, :@!after-matched, :@!around, :%!plugin-config);
+                    my $key = ($name-prefix ?? $name-prefix ~ '.' !! '') ~ ($_.name // '');


Can just be .name here, although I wonder if we really need a key if there is no .name?

It cannot, here we check if names from our includes (optionally prefixed) are conflicting with each other and with what we have, the checking is bogus if we omit the prefix.

jnthn · 2021-08-27T13:04:52Z

lib/Cro/HTTP/Router.pm6

-                    @!handlers.push: .copy-adding(:@prefix, :@!body-parsers, :@!body-serializers,
-                        :@!before-matched, :@!after-matched, :@!around, :%!plugin-config);
+                    my $key = ($name-prefix ?? $name-prefix ~ '.' !! '') ~ ($_.name // '');
+                    if $name-prefix && $key && (%urls{$key}:exists) {


Why does it matter if we have a $name-prefix here? A conflict of route names in a route block that doesn't itself have a name is still a problem?

We need it because if the include route ... is anonymous, then we don't have access to its inner names and we don't need to check it for conflicts. Added a comment.

jnthn · 2021-08-27T13:05:59Z

lib/Cro/HTTP/Router.pm6

+                    my $outer-handler = .copy-adding(:@prefix, :@!body-parsers, :@!body-serializers,
+                        :@!before-matched, :@!after-matched, :@!around, :%!plugin-config, :$name-prefix, :from-include);
+                    if $outer-handler.name {
+                        my $link-config = $outer-handler.get-innermost-plugin-configs($link-plugin)[0];


Do we know that the outer handler will always have a config set up?

Yes, we have a call to generate-urls before this loop and it adds the link plugin state unconditionally.

jnthn · 2021-08-27T13:08:57Z

lib/Cro/HTTP/Router.pm6

+            }
+            @options.push: |$links.link-generators.keys;
+        }
+        warn "Called the make-link subroutine with $route-name but no such route defined, options are: @options.join('/')";


Why warning rather than error?

I don't consider a typo in an url name, which is easy to fix, as something which should crash the whole application.
A working page with just one URL being bogus with a warning for the dev >>> the whole application crashing.

jnthn · 2021-08-27T13:09:57Z

t/http-router-named-urls.t

+}
+
+{
+    my $*CRO-ROOT-URL = 'https://foobar.com';


Hm, I think we should probably do this by looking at the Host header of the HTTP request...though maybe a fallback is useful too.

No idea, I was piggybacking the test written by @vendethiel here.
The logic should probably be dynamic var // host header value?

jnthn · 2021-08-27T13:10:31Z

t/http-router-named-urls.t

+
+test-route-urls route :name<main>, {
+    get :name<home>, -> {
+        is make-link('main.home'), '/', 'Basic call of a generator by a short name is correct';


Think I'd call this a qualified name, not a short name.

jnthn · 2021-08-27T13:11:24Z

t/http-router-named-urls.t

+    }
+}, X::Cro::HTTP::Router::DuplicateLinkName, message => "Conflicting link name: main.home";
+
+# XXX the test intention is bogus?


Arguably yes; at the very least, if we do build a qualified table, it's a separate thing from the unqualified one.

Removed it.

jnthn · 2021-08-27T13:12:47Z

t/http-router-named-urls.t

+
+test-route-urls route {
+    get -> {
+        say make-link('hello', 'world');


Debug output in test

Resolve issues we had due to information loss at handlers being flattened after inclusion, make tests pass again as the conflict detection is more strict and at the same time smart now.

Here we do three things: * Before that, URLs constructed for a route had no idea they can be used in some outer route block, so they generated `/foo` instead of `/included-prefix/foo`. To overcome this, we add an url-prefix attribute when we include a handler we rewrite it to an appropriate value. * Before that, we suffered at referencing the correct names due to route flattening. Inner anonymous routes had their handlers not present in the outermost hash, and given the hash was implemented as an attribute on the route set, they were in fact unable to refer to proper names. To overcome this, instaed of using an attribute we rely on the plugin system which allows us to separately store the data of inner route blocks. * This commit also adapts tests to accommodate to removal of the urls attribute available before. There is now a fallback method, though it's possible it will be removed as well in next commits due to its redundancy.

jnthn · 2021-11-10T16:09:02Z

Doing some testing. Some findings:

my $application = route {
    get :name<home>, -> {
        content 'text/plain', join "\n",
            # Fine
            abs-link('home'),
            abs-link('lit'),
            abs-link('qs', 'tools', query => 'abc?!'),
            abs-link('qs', 'tools'),
            # Fails to % encode the space
            abs-link('segs', 42, 'foo bar.jpg'),
            # Should not consider `is cookie` and `is header`
            abs-link('noqs'),
            '';
    }

    get :name<lit>, -> 'foo', 'bar' { }

    get :name<segs>, -> 'product', $id, 'docs', $file { }

    get :name<qs>, -> 'search', $category, :$query {}

    get :name<noqs>, -> 'baz', :$foo! is cookie, :$bar! is header {}
}

jnthn · 2021-11-10T16:17:57Z

If I change the above example to:

route :name<root>, {

Then all of the examples stop working, complaining that they want the qualified name, however qualification should be optional.

jnthn · 2021-11-10T16:26:57Z

Also expanded with some non-working ones for included routes:

my $application = route {
    get :name<home>, -> {
        content 'text/plain', join "\n",
            # Fine
            abs-link('home'),
            abs-link('lit'),
            abs-link('qs', 'tools', query => 'abc?!'),
            abs-link('qs', 'tools'),
            # Fails to % encode the space
            abs-link('segs', 42, 'foo bar.jpg'),
            # Should not consider `is cookie` and `is header`
#            abs-link('noqs'),
            # These don't work (routes not found)           
            abs-link('blog.home'),
            abs-link('blog.post', 42),
            '';
    }       
    
    get :name<lit>, -> 'foo', 'bar' { }
    
    get :name<segs>, -> 'product', $id, 'docs', $file { }
    
    get :name<qs>, -> 'search', $category, :$query {}
    
    get :name<noqs>, -> 'baz', :$foo! is cookie, :$bar! is header {}
    
    include 'blog' => route :name<blog>, {
        get :name<home>, -> {}
        get :name<post>, -> 'post', $id {}
    }
}

jnthn · 2021-11-10T17:59:41Z

lib/Cro/HTTP/Router/LinkGenerator.pm6

+        next if $param ~~ Cro::HTTP::Router::Header;
+        next if $param ~~ Cro::HTTP::Router::Cookie;


Ooh, right, we should do also the same if it's Cro::HTTP::Router::Auth too.

Ah, and if the parameter type does Cro::HTTP::Auth also.

jnthn reviewed Aug 27, 2021

View reviewed changes

Altai-man force-pushed the link-gen-no-fork branch from 0d2da6a to 13e7019 Compare August 27, 2021 15:57

vendethiel and others added 8 commits November 10, 2021 16:22

Link generation from route definition

6f68c78

Update META6.json with new files

e014103

Streamline route naming story

278bd0c

Resolve issues we had due to information loss at handlers being flattened after inclusion, make tests pass again as the conflict detection is more strict and at the same time smart now.

Remove obsolete rewritten names

9274dd5

WIP

43902e0

Code cleanup (debug leftover, missing comments)

40a14fb

Rework API a bit and do some minor cleanup

dca1047

jnthn force-pushed the link-gen-no-fork branch from 13e7019 to dca1047 Compare November 10, 2021 15:22

Tweak link generator API and module name

9f47a63

Altai-man added 2 commits November 10, 2021 18:31

A couple of tweaks for link generator

af011e2

Better joiner for a warning

c30fcbe

jnthn reviewed Nov 10, 2021

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support link generation #152

Support link generation #152

Altai-man commented Aug 26, 2021

jnthn Aug 27, 2021

Altai-man Aug 27, 2021

jnthn Aug 27, 2021

jnthn Aug 27, 2021

Altai-man Aug 27, 2021

jnthn Aug 27, 2021

Altai-man Aug 27, 2021

jnthn Aug 27, 2021

Altai-man Aug 27, 2021

jnthn Aug 27, 2021

Altai-man Aug 27, 2021 •

edited

Loading

jnthn Aug 27, 2021

Altai-man Aug 27, 2021

jnthn Aug 27, 2021

Altai-man Aug 27, 2021

jnthn Aug 27, 2021

Altai-man Aug 27, 2021

jnthn Aug 27, 2021

jnthn commented Nov 10, 2021

jnthn commented Nov 10, 2021

jnthn commented Nov 10, 2021

jnthn Nov 10, 2021

jnthn Nov 10, 2021

		# my $*CRO-ROUTER-ROUTE-HANDLER = $handler;
		# my %*URLS = $handler ~~ DelegateHandler ?? %() !! router-plugin-get-configs($link-plugin);

		next if $param ~~ Cro::HTTP::Router::Header;
		next if $param ~~ Cro::HTTP::Router::Cookie;

Support link generation #152

Are you sure you want to change the base?

Support link generation #152

Conversation

Altai-man commented Aug 26, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Altai-man Aug 27, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jnthn commented Nov 10, 2021

jnthn commented Nov 10, 2021

jnthn commented Nov 10, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Altai-man Aug 27, 2021 •

edited

Loading