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

New @http syntax: * for {proxy+} #969

Closed
ryanblock opened this issue Sep 29, 2020 · 11 comments
Closed

New @http syntax: * for {proxy+} #969

ryanblock opened this issue Sep 29, 2020 · 11 comments
Assignees
Labels
enhancement @package @architect/package
Milestone

Comments

@ryanblock
Copy link
Member

ryanblock commented Sep 29, 2020

Today you can capture dynamic paths in @http functions one of two ways:

  • get / – greedy, captures everything that wasn't captured by the other routes you define
    • Filesystem: src/http/get-index
  • [get|post|etc.] /path/:param where :param captures any request in that part of the URL
    • This is NOT greedy, so while it would capture a get request to /path/foo, if you requested /path/foo/bar, it would actually fall back to get /
    • Filesystem: src/http/get-path-000param

@kristoferjoseph recently suggested an idea that was echoed by @patcat in Architect chat: adding support for API Gateway's {proxy+} syntax.

Proposing it look something like this:

  • [get|post|etc.] /path/* where * is equivalent to {proxy+}
    • This would be greedy to that path, so it would it would capture a get request to /path/foo, as well as one to /path/foo/bar
    • Filesystem: src/http/get-path-catchall – would need to build conflict support in case there's also a get /path/catchall path defined

Since this would be the first new @http feature following the Chupacabra launch with HTTP APIs support, I'd propose this, and the upcoming @proxy pragma support be for HTTP APIs only. Thoughts?

@ryanblock ryanblock added enhancement @package @architect/package labels Sep 29, 2020
@patcat
Copy link

patcat commented Sep 30, 2020

I like this proposed concept! I think starting with HTTP APIs to start with is a good option, as it would enable more complex routing within lambdas, which was initially was prompted me looking into it.

@exalted
Copy link

exalted commented Sep 30, 2020

Question: Where [get|post|etc.] /path/* translates to [get|post|etc.] /path/{proxy+}; would [get|post|etc.] /* also translate to [get|post|etc.] /{proxy+} (i.e., is there a specific case for the root path?) My :two-cents: there shouldn't be. So, if I do this get / I expect this route to NOT be a catch-all, and instead only capture requests that are strictly coming to / and not /foo or /foo/bar… If I wanted to capture-all (à la current get /) I'd do get /*. Makes sense? What do you think?

Feedback:

Since this would be the first new @http feature following Chupacabra launch with HTTP APIs support, I'd propose this, and the upcoming @Proxy pragma support be for HTTP APIs only. Thoughts?

YES! I think it's okay to forcibly push folks to move on to the new stuff, unless there are legitimate reasons why someone couldn't migrate to the new and shiny because of a limit of the framework.

@ryanblock
Copy link
Member Author

I'm open to ideas on how to handle get /* – most straightforward path would be to just ignore it, though, because get / is already acting as get /* today. To change the behavior of get / to be strictly get requests to root would be a breaking change for all current Arc apps, so I'm not inclined to do that. What do y'all think?

@ryanblock ryanblock added this to the 7.1.0 milestone Sep 30, 2020
@ryanblock
Copy link
Member Author

Quick update: looking at making this feature Architect 7.1, shooting for an RC this week!

@ryanblock ryanblock self-assigned this Sep 30, 2020
@ryanblock
Copy link
Member Author

Another update: Architect 7.1.0 RC is now live!

@brianleroux
Copy link
Member

bug report for 7.1.0-RC.0 steps to reproduce:

  1. create an app.arc with the following:
@app
myapp
@http
get /*
  1. arc init and arc sandbox works as expected (all routes including get / are handled by catchall handler

  2. arc deploy and navigate to the generated api gateway url will result in 502 error (but adding paths to the url will route to catchall)

demo failing behavior: https://jwpm1ssy75.execute-api.us-east-1.amazonaws.com
but this works https://jwpm1ssy75.execute-api.us-east-1.amazonaws.com/foo

@ryanblock
Copy link
Member Author

Thank you, 7.1.0-RC.1!

@exalted
Copy link

exalted commented Oct 2, 2020

I'm open to ideas on how to handle get /* – most straightforward path would be to just ignore it, though, because get / is already acting as get /* today. To change the behavior of get / to be strictly get requests to root would be a breaking change for all current Arc apps, so I'm not inclined to do that. What do y'all think?

Yes, it's being a breaking change was exactly what I was also thinking while writing my previous message down. In all honesty, the "fix" for all apps would literally be adding a single * in their app.arc to keep the existing behavior. IMO it doesn't even compare with the coherent and expected behavior you'd get out of it as opposed to the exception for get /.


Also, on a somewhat related note, with the current behavior (i.e., get / being a special case for catch-all behavior) what happens if I were to define get /*? Do I end up with something like /{proxy+}{proxy+} on AWS's end? 🤷‍♂️

@ryanblock
Copy link
Member Author

ryanblock commented Oct 2, 2020

I somewhat misspoke previously, current get / behavior is actually more akin to any /*, so it's not quite as direct a parallel.

I think it's probably time for us to resolve the confusing root behavior though. This may require a breaking change.

@exalted
Copy link

exalted commented Oct 2, 2020

I somewhat misspoke previously, current get / behavior is actually more akin to any /*, so it's not quite as direct a parallel.

Oh… That's even more confusing then. 👍 Thanks for the update @ryanblock

I think it's probably time for us to resolve the confusing root behavior though. This may require a breaking change.

:two-cents: I know it's not ideal, but for whatever it's worth, I think it's okay to break things, again, since the "fix" is about only few characters in app.arc, if I am not mistaken.

@ryanblock
Copy link
Member Author

Mainlined and released tonight as 8.0!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement @package @architect/package
Development

No branches or pull requests

4 participants