Improvements to Router syntax

[uaply]

Currently action routing supports such syntax:

• /page – static page
• /get/:id/ – parameterized page
• /more/*/all – single-part asterisk
• /file//download** – multi-part asterisk (would match /file/a/.../z/download)

Single-part asterisk seems redundant to me, because you can always replace it with
/more/*/all → /more/:dummy/all and do not use value of dummy parameter.

Multi-part asterisk implementation is buggy, because it doesn't check suffix,
so /file/x-files/top-secret.pdf/denyaccess will also match /file//download** pattern. Besides being buggy, it is nearly useless, because action should parse whole URL to extract pathname of file. It would be great if the action somehow received x-files/top-secret.pdf as a parameter.

I propose slightly adjust router syntax:
(this is much like http://backbonetutorials.com/what-is-a-router/)

• /page – static page
• /get/:id/ – parameterized page
• */file/path

With such modification asterisk will match anything after /file/ and place substring to parameter named path. So Action could easily use path and open needed file.

I could push some modifications right now, but we already have a lot of commits in working branch so I better hold down before current changes will be merged to master branch.

[silvioprog]

You can apply the change. Then we check if it brokes old codes, running all test cases.

[uaply]

For sure it will break some test cases.

I'm also experimenting now with another modification – optional parameter that could be passed to Action.
For example if I have special action for serving single file, I can pass filename during registration like this:

TSingleStaticFileAction.register('/favicon.ico',False,'favicon.ico');
TSingleStaticFileAction.register('/',False,'main.html');

So I can reuse the same action for different resources.

[silvioprog]

You can apply the changes separately. Thus, if there is error, it will be easier for us to fix. :)

[uaply]

Test unit testbrookrouter.pas is broken now because we removed Router.CurrentAction property recently.

[silvioprog]

I'll fix the test cases now ...

[silvioprog]

Done. Test case is OK now.

I need to test your changes in router. The router code is the most critical part of the Brook. We have to be very careful when changing the logic there.

[silvioprog]

You want to remove support to "*_/foo"? If yes, "_foo" will provide support for it?

[uaply]

It is tricky part. If I can read the code correct, '/foo' is not supported now. It just equivalent to ''

function TBrookRouter.MatchPattern
[...]
    if VPt = AK + AK then
      Exit;

My implementation don't remove '/*_', but also don't support '/_path/foo' on itself. If we want to compare ending '/foo' and reject routes that don't have such suffix, we still need to implement it.

[silvioprog]

And any change in router must be documented (with demos), like: https://github.com/silvioprog/brookframework/blob/master/core/brookaction.pas#L94.

[silvioprog]

I forgot to comment...

Single-part asterisk seems redundant to me, because you can always replace it with
/more/*/all → /more/:dummy/all and do not use value of dummy parameter.

But "/more/*/all" don't will create a variable.

[silvioprog]

Yes. Your changes will be accepted, as long as the old examples (you can change examples too for use new feature) continue working. And that must be documented, for better comprehension that new feature provides.

[silvioprog]

Seems a bug in new router code. See the test below:

unit Unit1;

{$mode objfpc}{$H+}

interface

uses
  BrookAction;

type
  TMyAction = class(TBrookAction)
  public
    procedure Get; override;
  end;

implementation

procedure TMyAction.Get;
begin
  Write(Values.AsJSON);
end;

initialization
  TMyAction.Register('/home/:var/*download/');

end.

Calling the URL http://localhost/cgi-bin/cgi1/home/file/download:

In old router: 404 - Page not found (Correct!)
In new router: { "var" : "file", "download" : "download" } (Error! Because "*download/" isn't a variable)

[uaply]

Error! Because "*download/" isn't a variable

The whole point of my modifications is for "_download" to _be* a variable.
See what I mean:
/home/:var – will match "/home/value" and set {"var": "value"}. Only single-level before slash.
*/home/file – will match "/home/dir/subdir/.../filename" and set {'file": "dir/subdir/.../filename" }. Almost the same as above, but multi-level.

So your example with "/home/:var/*download/" behaves as I expected.

There are some questions with trailing slash in my code, so I should check it. Possible suffixes could be something like
*/home/file/ or
*/home/file/download (Not yet implemented, but I will).

Anything like "/home/*file/:var/" is not supposed to work because it is ambiguous.

[silvioprog]

I'll test all your changes after you commit it. Please document all details and sample of it in TBrookAction.

And see other nice feature that will be implemented here: #52. We can implement it soon as possible. 👍

[silvioprog]

It will match patterns like too?:

TMyAction.Register('/home/:user/**/download/:fileid');

Call: http://localhost/cgi-bin/cgi1/home/1/a/b/download/2

Result: { "user": 1, "fileid": 2 }

If yes, it will be very nice! :)

[uaply]

Yeah, with last commit it's possible with such syntax:

TMyAction.Register('/home/:user/*path/download/:fileid');

Call
http://localhost/cgi-bin/cgi1/home/1/a/b/download/2

Result will be

{ "user": "1", "path": "a/b", "fileid": "2"}

I think old-style /*/ and /**/ should not be used. You can always replace it with /*path/ which do the same job.

...Working on documentation.

[silvioprog]

I'll test all your changes now ...

[silvioprog]

Before it is moved to the master branch we have a backup here:

http://brookframework.org/backup/brookframework-20130818-1546.zip

[silvioprog]

Some detected problems...

The first is simple. For this test:

procedure TMyAction.Get;
begin
  Write(Values.AsJSON);
end;

initialization
  TMyAction.Register('/home/:user/*path/download/:fileid');

And this URL: http://localhost/cgi-bin/cgi1/home/1/a/b/download/2

The result is: { "user" : "1", "fileid" : "2", "path" : "a\/b" }

So, should not be? { "user" : "1", "path" : "a\/b", "fileid" : "2" }

There is no error, so if it is too difficult to change or use more processing, we can leave it as is.

Now the second:

In new implementation we can use only /home/:user/*path/download/:fileid instead of /home/:user/*/download/:fileid and /home/:user/*path/download/:fileid, but, using new implementation, how I validate this situation?:

http://localhost/cgi-bin/cgi1/home/1/a/download/1 OK
http://localhost/cgi-bin/cgi1/home/1/a/b/download/1 Error (404)

In old router I wore it: /home/:user/*/download/:fileid.

[uaply]

Values in JSON dictionary is not sorted anyway, so there is no point in fixing first issue. One never should rely on order of keys in json.

Second issue can be solved this way. You just specify:
/home/:user/:dummy/download/:fileid instead of old /home/:user/*/download/:fileid.
Or even /home/:user/:/download/:fileid without variable name (It would be empty-string-named variable).

Honestly I can't imagine situation when you will need to match some part of url with /*/ and not use matched value.

We can also hack the syntax to make /:/ = // (if variable name is not specified), but this would be confusing, because in all other cases * will match *any number of levels, not only one.

[silvioprog]

The only utilizade of * and ** is to allow multi-levels but without creating variables.

In old implementation I can:

TMyAction.Register('/home/:user/**/download/:fileid/*');

and call:

http://localhost/cgi-bin/cgi1/home/1/a/download/2/stable

or:

http://localhost/cgi-bin/cgi1/home/1/a/b/download/2/release

But, new implementation follows?:

TMyAction.Register('/home/:user/*dummy1/download/:fileid/*dummy2');

:/

[silvioprog]

Ah, using regex I can solve it. E.g.:

/home/:user/(my regex 1)/download/:fileid/(my regex 2)

Regex doesn't creates variables.

[silvioprog]

If new implementation allow /home/:user/*dummy1/download/:fileid/*dummy2 the problem will solved.

[uaply]

If new implementation allow /home/:user/*dummy1/download/:fileid/*dummy2 the problem will solved.

It is ambiguous, how such pattern /home/:user/***/download/:fileid/*** could be reliably matched?

Now possible to use /home/:user/*dummy1/download/:fileid/:dummy2
It equals to old /home/:user/**/download/:fileid/*

You just replace // → /:/ and /__/ → // to convert from old syntax to new.

[silvioprog]

You just replace // → /:/ and /__/ → // to convert from old syntax to new.

This way?:

TMyAction.Register('/home/:user/*/download/:fileid/*');

[silvioprog]

So, if /*/ → //, then:

TMyAction.Register('/home/:user/*/download/:fileid/*');

But, calling:

http://localhost/cgi-bin/cgi1/home/1/a/download/2

Returns 404. :/

[uaply]

If you want old-style
TMyAction.Register('/home/:user/**/download/:fileid/*');
it should be written in new syntax as
TMyAction.Register('/home/:user/*/download/:fileid/:);

Last ":" at the end will match only one level, of course.

[silvioprog]

Hm... something weird or I really I don't understand. :/

Using this pattern:

TMyAction.Register('/home/:user/*');

I can:

http://localhost/cgi-bin/cgi1/home/1/a

or:

http://localhost/cgi-bin/cgi1/home/1/a/b

So, with:

TMyAction.Register('/home/:user/*/id/:id/*');

Why http://localhost/cgi-bin/cgi1/home/John/directorship/id/1/computers return 404?

[uaply]

It doesn't match because only first // is treated as a pattern. Second / is not a pattern in current implementation. It is just not obvious how to implement it, so I process only first one. Second is treated as literal "_", so this will match only
http://localhost/cgi-bin/cgi1/home/John/directorship/id/1/_.

I am aware of this limitation, and even documented it: "@bold(@italic(NOTE:)) Only one multi-level variable can be specified per URL."

I think it will lead to confusion if something like /a/*/p/*/z/* will be allowed. Example:

How would you parse /a/p/p/p/z/z/z/e with above pattern?
Is it /a/ + (p/p) + /p/ + (z/z) +/z/ + (e)?
Or maybe /a/ + (p) + /p/ + (p/z/z) +/z/ + (e)?
Or even /a/ + (p) + /p/ + (p) + /z/ + (z/z/e)?
All seems to match, but not in unique way.

[silvioprog]

It doesn't match because only first // is treated as a pattern. Second / is not a pattern in current implementation. It is just not obvious how to implement it, so I process only first one. Second is treated as literal "_", so this will match only
http://localhost/cgi-bin/cgi1/home/John/directorship/id/1/_.

I am aware of this limitation, and even documented it: "@bold(@italic(NOTE:)) Only one multi-level variable can be specified per URL."

But you have plans to implement it, or we'll use only a single level?

I think it will lead to confusion if something like /a/*/p/*/z/* will be allowed. Example:

How would you parse /a/p/p/p/z/z/z/e with above pattern?
Is it /a/ + (p/p) + /p/ + (z/z) +/z/ + (e)?
Or maybe /a/ + (p) + /p/ + (p/z/z) +/z/ + (e)?
Or even /a/ + (p) + /p/ + (p) + /z/ + (z/z/e)?
All seems to match, but not in unique way.

I don't know too. :( How Backbone treats it?

After we close this issue, I'll do a merge to master.

[uaply]

Backbone is using RegExp behind the scenes. It is possible to use multiple '*splat' parameters in Backbone, but it is not documented. They use non-greedy pattern, so it will match minimal possible substring, just like /a/ + (p) + /p/ + (p) + /z/ + (z/z/e) in my examples. This behavior is also not documented.

I was not planning to support it because it is hard to implement. It will require too much efforts and testing for a feature almost no one will use. Maybe it will be easier to implement it using regexpr library, but regexps usually are much slower. Such a simple syntax and so many complications.

Also I completely forgot about syntax like /open/pg:num/. Does anybody ever planned to use it with Brook?

[silvioprog]

Backbone is using RegExp behind the scenes. It is possible to use multiple '*splat' parameters in Backbone, but it is not documented. They use non-greedy pattern, so it will match minimal possible substring, just like /a/ + (p) + /p/ + (p) + /z/ + (z/z/e) in my examples. This behavior is also not documented.

Sorry, but is not documented in Backbone or in Brook?

I was not planning to support it because it is hard to implement. It will require too much efforts and testing for a feature almost no one will use. Maybe it will be easier to implement it using regexpr library, but regexps usually are much slower. Such a simple syntax and so many complications.

OK. No problems. We keep only the current implementation. This already meets the majority of cases. 👍

Also I completely forgot about syntax like /open/pg:num/. Does anybody ever planned to use it with Brook?

What does this do? This is already implemented in Brook? If yes, it needs to be document.

[silvioprog]

... I forgot to say,

Also I completely forgot about syntax like /open/pg:num/. Does anybody ever planned to use it with Brook?

This is optional, so if the implementation for it decrease a little the performance, we can leave as already, because it can be validated in a constraint (I'll release the constraints soon).

[uaply]

Multiple *splat cases are not documented in Backbone, but they works there. And it seems to me that developers aimed only most common case when /_splat is located at the end of path. At least there is no examples like /_file/download in their documentation.

Syntax /open/pg:num/ supposed to match /open/pg1/. You are right – it easily implemented with Constrains, or with RegExp validation we discussed in #52. So it is optional (and not supported in Brook).

[silvioprog]

We can close this issue? :) If yes, I'll do a clean and then do a merge.

[uaply]

Yes, let's close it.