Improvements to Router syntax about brookframework HOT 36 CLOSED

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

from brookframework.

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.

from brookframework.

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

from brookframework.

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

from brookframework.

I'll fix the test cases now ...

from brookframework.

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.

from brookframework.

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

from brookframework.

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.

from brookframework.

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

from brookframework.

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.

from brookframework.

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.

from brookframework.

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)

from brookframework.

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.

from brookframework.

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. 👍

from brookframework.

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! :)

from brookframework.

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.

from brookframework.

I'll test all your changes now ...

from brookframework.

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

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

from brookframework.

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.

from brookframework.

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.

from brookframework.

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');

:/

from brookframework.

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

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

Regex doesn't creates variables.

from brookframework.

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

from brookframework.

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.

from brookframework.

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

This way?:

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

from brookframework.

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

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

But, calling:

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

Returns 404. :/

from brookframework.

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.

from brookframework.

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?

from brookframework.

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.

from brookframework.

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.

from brookframework.

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?

from brookframework.

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.

from brookframework.

... 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).

from brookframework.

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).

from brookframework.

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

from brookframework.

Yes, let's close it.

from brookframework.