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

TokenRouter needs explicit GET when POST is used #1

Open
dustinmoris opened this issue Feb 9, 2018 · 4 comments
Open

TokenRouter needs explicit GET when POST is used #1

dustinmoris opened this issue Feb 9, 2018 · 4 comments
Labels
bug Something isn't working help wanted Extra attention is needed

Comments

@dustinmoris
Copy link
Member

Originally created by @forki in giraffe-fsharp/Giraffe#158


Reminder for @gerardtoconnor ;-)

sent you actual routes in screenshot. It's along the lines of:

 router notfound [
      subRoute "/api" [
           // get routes
           route "/hey" f
           POST [
              route "/there" f2
           ]
 ]

not working. but following is:

 router notfound [
      subRoute "/api" [
           GET [
               route "/hey" f
           ]
           POST [
              route "/there" f2
           ]
 ]
@dustinmoris
Copy link
Member Author

Posted by @gerardtoconnor


I'll look into this, thanks for raising.

The issue relates to it being able to partially match the non-filtered root before realizing it's a dead-end but there may have been a route possible in filtered routes earlier in the tree.

There are a few ways to fix but I want to make sure it's performant & clean so please bear with me and use the explicit route filtering as you have for now thanks.

@dustinmoris
Copy link
Member Author

Posted by @gerardtoconnor


@forki I know breaking changes are annoying but when I created the method filter functions I overlooked the fact that many people like to do filter as last leg in chain eg:

router notfound [
      subRoute "/api" [
           subRoute "/hey" [
                GET f1
                POST f2
           ]
           POSTGRP [
              route "/there" f3
           ]
 ]

So I am wondering how I handle it and if I have two separate functions for each method eg for GET:

  1. GET : HttpHandler -> Node -> Node
  2. GETGRP : (Node -> Node) list -> Node

not changing anything yet but along with this bug fix want to make sure fixes against this issue as

router notfound [
      subRoute "/api" [
           GET [ 
                route "/hey" f1 
           ]
           POST [ 
                route "/hey" f2
           ]
           POST [
              route "/there" f3
           ]
 ]

Is bit repetitive and frustrating for user having to write route "/hey" twice?

@dustinmoris dustinmoris added the bug Something isn't working label Feb 9, 2018
@dustinmoris dustinmoris added the help wanted Extra attention is needed label Aug 19, 2018
@danieljsummers
Copy link

This seems to be more generalized than that. I just hit an issue where I had a GET section defined, and then one defined outside a verb list (because it needs to hand HEAD and OPTIONS too), and my GET wasn't found. I can work around it, so no worries; I'll be keeping an eye on this, though, so I can remove my workaround once I upgrade.

(FWIW, I'd be fine with an API change if necessary; as a manager I used to work with liked to say, "We reserve the right to get smarter." :) )

@gerardtoconnor
Copy link
Member

Given there is always a verb required with list being relatively stable, rather then grouping, I was thinking of instead adopting the Saturn function naming scheme of get/getf, post/postf etc, and it's what I actually use in Zebra router which is a copy tweaked version of this router. It would be a drastic API change so not sure if it's a starter for most given the messy upgrade work invloved?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working help wanted Extra attention is needed
Projects
None yet
Development

No branches or pull requests

3 participants