New Feature: Path based parameters #119

Tiberriver256 · 2018-05-06T03:46:29Z

*Note this should also fix the matching issue described at the bottom of issue #116

This allows for the following scenario:

New-PolarisGetRoute -Path "/Hello/{Name}" -ScriptBlock {
                $Response.Send("Hello $($Parameters.Name)")
            }

$Result = Invoke-WebRequest -Uri "http://localhost:$Port/hello/Testing" -UseBasicParsing
$Result.Content | Should Be 'Hello Testing'

It introduces a new $Parameters PSCustomObject representing any path based parameters and it also makes route matching more strict as it is now converts paths to a regular expression using Convert-PathParameters function.

At the time a request is made the above mentioned path /Hello/{Name} would get converted to a regular expression of ^/Hello/(?<Name>.*)$ for matching against the client's requested route.

A more basic example would be that /Hello would be converted to ^/Hello$ to guarantee an exact path match. I've also added some test cases to ensure we are throwing 404 errors for paths that don't match exactly.

Tiberriver256 · 2018-05-06T03:54:47Z

Appveyor is still drunk, but tests appear to be passing. Take a look at the changes and let me know what you think @tylerl0706

https://ci.appveyor.com/project/PowerShell/polaris/build/job/0qxc0cy0p5bd54g9

TylerLeonhardt · 2018-05-06T05:19:24Z

I'll look at this :) p.s. tweeted at AppVeyor to save us! https://twitter.com/TylerLeonhardt/status/992997043433099264

Tiberriver256 · 2018-05-06T12:45:53Z

Thinking about this a bit more last night. If you like the concept I may just look into porting the module expressjs uses. It would give a more familiar syntax and theirs is pretty robust.

https://github.com/pillarjs/path-to-regexp

TylerLeonhardt · 2018-05-07T01:42:16Z

@Tiberriver256 THIS IS AWESOME AND EXACTLY WHAT POLARIS NEEDS. Adopting what expressjs does sounds awesome for this - that's one of my favorite features of express actually.

TylerLeonhardt · 2018-05-07T02:37:06Z

Do you still want me to review this or do you want to take a crack at following what expressjs does 😀

Tiberriver256 · 2018-05-07T12:08:22Z

I’ll take a crack at the expressjs :) doesn’t look too bad.

…

On Sun, May 6, 2018 at 10:37 PM Tyler James Leonhardt < ***@***.***> wrote: Do you still want me to review this or do you want to take a crack at following what expressjs does 😀 — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#119 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AGqmtAXySXg5JN1DmMJrQeWMDGVNQ5cpks5tv7NSgaJpZM4Tz4dA> .

TimCurwick · 2018-05-07T23:33:29Z

Nice enhancement, but the implementation makes certain assumptions about our desired end state that may make sense in the context of the this enhancement, but may not be valid for other desired behaviors. I would prefer if we take the time to discuss and agree on a set of behaviors and design requirements, so that we can design an elegant, comprehensive solution, rather than hoping we get there through a series of small breaking bug fixes and enhancements taking us in random directions.

Tiberriver256 · 2018-05-08T00:02:58Z

@TimCurwick - Agreed, I think the goal of my PR here (which was to get the discussion started) is not really standard. I opened #120 to start the discussion. Really glad people are getting into this!

Tiberriver256 · 2018-05-10T05:45:20Z

I updated the code here as a first stab at RFC001 (#120). Everything is working and has a test case for the examples I gave. It does not have a method for switching from case sensitive to being insensitive yet.

*EDIT: Looks like I've actually got one test to fix on the linux side but the code should still be okay to look at.

TylerLeonhardt · 2018-05-14T14:38:11Z

I'm waiting on how the RFC turns out before reviewing :) just so you know I haven't forgotten about this!

TylerLeonhardt

@Tiberriver256 I owe you an apology for disappearing! Some other work has taken priority but I still deeply care about this project.

If fact, I'm going to be giving a talk that uses Polaris in the coming months :) ... more on that later.

I have a few comments on this PR. This is the last thing I want to get in before I push it out to the Gallery. I've got the signing process almost set up... and then in the future, it will be 100% automated and we'll be able to iterate much faster.

Again, I'm sorry for disappearing and hope that you still have an interest in Polaris!

PS. The RFC time period is over and I think we should get this in.

TylerLeonhardt · 2018-06-29T19:30:04Z

lib/Polaris.Class.ps1

@@ -48,7 +48,8 @@ class Polaris {
                # Run middleware in the order in which it was added
                foreach ($Middleware in $Polaris.RouteMiddleware) {
 					$InformationVariable += $Polaris.InvokeRoute(
-							$Middleware.Scriptblock,
+                            $Middleware.Scriptblock,
+                            $Null,


nit: some weird indenting!

TylerLeonhardt · 2018-06-29T19:33:52Z

lib/Polaris.Class.ps1

@@ -68,7 +75,8 @@ class Polaris {
                    try {

                        $InformationVariable += $Polaris.InvokeRoute(
-							$Routes[$MatchingRoute][$Request.Method],
+                            $Routes[$MatchingRoute][$Request.Method],
+                            $Parameters,


nit some more weird indents

TylerLeonhardt · 2018-06-29T19:34:45Z

lib/Polaris.Class.ps1

@@ -192,6 +201,23 @@ class Polaris {
        return $SanitizedPath
    }

+    static [RegEx] ConvertPathToRegex([string]$Path){
+        Write-Debug "Path: $path"
+        $path = $path -replace '\.', '\.'


Is this replace doing anything?

TylerLeonhardt · 2018-06-29T19:35:22Z

lib/Polaris.Class.ps1

+        $path = $path -replace '-', '\-'
+        $path = $path -replace '\*', '.*'
+        $path = "^$path$"
+        $path = $path -replace ":(\w+)(\?{0,1})", '(?<$1>.+)$2'


can you add a couple comments in this function due to the complexity?

TylerLeonhardt · 2018-06-29T19:36:44Z

lib/Polaris.Class.ps1

+                # Searching for the first route that matches by the most specific route paths first.
+                #
+                $MatchingRoute = $Routes.keys | Sort-Object -Property Length -Descending | Where-Object { $Route -match [Polaris]::ConvertPathToRegex($_) } | Select-Object -First 1
+                $Request.Parameters = ([PSCustomObject]$Matches)


Where does $Matches get defined?

$Matches is an automatic variable that gets defined every time you run a -match comparison.

See:
https://docs.microsoft.com/en-us/powershell/module/microsoft.powershell.core/about/about_automatic_variables?view=powershell-6#matches
https://mcpmag.com/articles/2015/09/30/regex-groups-with-powershell.aspx

TylerLeonhardt · 2018-06-29T19:44:33Z

Also that test failure is bogus...

Expected strings to be the same, but they were different.
String lengths are both 47.
Strings differ at index 2.
Expected: '@{bookId=123; userId=12; 0=/users/12/books/123}'
But was:  '@{userId=12; bookId=123; 0=/users/12/books/123}'

Maybe do a different kind of comparison rather than a string compare.

Tiberriver256 · 2018-06-29T19:45:57Z

Yeah, no worries on the timing. My life kind of blew up for a bit there too but seems to be close to returning to normalcy. It might be a bit before I can get to cleaning this up but super excited to do it.

TylerLeonhardt · 2018-07-03T18:31:26Z

@Tiberriver256 Got all the release stuff done so as soon as we get this PR in I'll throw Polaris up on the Gallery!

TylerLeonhardt · 2018-08-06T15:07:51Z

Gentle ping 😊

I'd like to get this on the Gallery.

Tiberriver256 · 2018-08-06T15:19:27Z

Thanks Tyler. I haven't forgotten. I'll see if I can get this up tonight.

…

On Mon, Aug 6, 2018 at 11:07 AM Tyler James Leonhardt < ***@***.***> wrote: Gentle ping 😊 I'd like to get this on the Gallery. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#119 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AGqmtNIfKHATErdrW4MslM4U0P4enUbPks5uOFvKgaJpZM4Tz4dA> .

Tiberriver256 · 2018-08-07T00:12:21Z

Should be all set sir :)

TylerLeonhardt

LGTM

TylerLeonhardt · 2018-08-08T14:02:56Z

Whooooooo!!

Micah Rairdon added 3 commits May 5, 2018 23:35

Removing mandatory to allow function to run with defaults

1e39000

Path based parameters functionality

4f62889

Removing test for invalid path as parameter

39afe19

Tiberriver256 requested a review from TylerLeonhardt May 6, 2018 03:54

Tiberriver256 mentioned this pull request May 7, 2018

Update Polaris.Class.ps1 #118

Closed

Tiberriver256 mentioned this pull request May 8, 2018

RFC 001: Path Matching/Resolution Strategy #120

Closed

Tiberriver256 added 4 commits May 10, 2018 01:20

Route matching for RFC001

f2fb7da

Updating static route for new route matching strategy

515c11d

Fix for parameters filepath not matching

1d3d82e

Updated routing syntax in tests

5a8a1a8

TylerLeonhardt approved these changes Jun 29, 2018

View reviewed changes

Micah Rairdon added 2 commits August 6, 2018 19:40

Added comments for clarity on regular expressions

2474996

Corrected the tests to be more reliabilities 🐱‍👤

1f605f3

TylerLeonhardt approved these changes Aug 8, 2018

View reviewed changes

TylerLeonhardt merged commit 6156823 into PowerShell:master Aug 8, 2018

New Feature: Path based parameters #119

New Feature: Path based parameters #119

Uh oh!

Conversation

Tiberriver256 commented May 6, 2018

Uh oh!

Tiberriver256 commented May 6, 2018

Uh oh!

TylerLeonhardt commented May 6, 2018

Uh oh!

Tiberriver256 commented May 6, 2018

Uh oh!

TylerLeonhardt commented May 7, 2018

Uh oh!

TylerLeonhardt commented May 7, 2018

Uh oh!

Tiberriver256 commented May 7, 2018 via email

Uh oh!

TimCurwick commented May 7, 2018

Uh oh!

Tiberriver256 commented May 8, 2018

Uh oh!

Tiberriver256 commented May 10, 2018

Uh oh!

TylerLeonhardt commented May 14, 2018

Uh oh!

TylerLeonhardt left a comment

Choose a reason for hiding this comment

Uh oh!

TylerLeonhardt Jun 29, 2018

Choose a reason for hiding this comment

Uh oh!

TylerLeonhardt Jun 29, 2018

Choose a reason for hiding this comment

Uh oh!

TylerLeonhardt Jun 29, 2018

Choose a reason for hiding this comment

Uh oh!

TylerLeonhardt Jun 29, 2018

Choose a reason for hiding this comment

Uh oh!

TylerLeonhardt Jun 29, 2018

Choose a reason for hiding this comment

Uh oh!

Tiberriver256 Jun 29, 2018

Choose a reason for hiding this comment

Uh oh!

TylerLeonhardt commented Jun 29, 2018

Uh oh!

Tiberriver256 commented Jun 29, 2018

Uh oh!

TylerLeonhardt commented Jul 3, 2018

Uh oh!

TylerLeonhardt commented Aug 6, 2018

Uh oh!

Tiberriver256 commented Aug 6, 2018 via email

Uh oh!

Tiberriver256 commented Aug 7, 2018

Uh oh!

TylerLeonhardt left a comment

Choose a reason for hiding this comment

Uh oh!

TylerLeonhardt commented Aug 8, 2018

Uh oh!

Uh oh!