Skip to content

Better recognize urls#93

Merged
julianrubisch merged 3 commits into
stimulusreflex:masterfrom
rickychilcott:better-route-recognition
May 4, 2021
Merged

Better recognize urls#93
julianrubisch merged 3 commits into
stimulusreflex:masterfrom
rickychilcott:better-route-recognition

Conversation

@rickychilcott

@rickychilcott rickychilcott commented Apr 29, 2021

Copy link
Copy Markdown
Contributor

Bug fix

Description

This passes in the full url to the Rails.application.routes.recognize_path method and try multiple HTTP methods to try to match a URL.

Note: While this does fix some bugs, it doesn't appear the controller and action_name are currently being passed through correctly to the underlying controller. I don't have failing tests for that, but it doesn't look like the code I wrote some time ago works properly.

This PR does no harm -- it makes it better -- but it doesn't fully address the intended functionality. However, it does fix #89

Checklist

  • My code follows the style guidelines of this project
  • Checks (StandardRB & Prettier-Standard) are passing

@rickychilcott

Copy link
Copy Markdown
Contributor Author

@julianrubisch - give this a review and see what you think. We might need to pair or take another crack at fully building up our request object and building the correct sequence of objects for the renderer. I noticed that the SR team changed, slightly, the way that the controllers and renderer is built up and implemented Middleware functionality.

@rickychilcott

Copy link
Copy Markdown
Contributor Author

Do let me know if this is not clear enough to review or you need more information.

@julianrubisch

Copy link
Copy Markdown
Contributor

Thanks! Will do, will have to defer this until Monday though

Comment thread lib/futurism/resolver/controller/renderer.rb Outdated
@julianrubisch

Copy link
Copy Markdown
Contributor

One question: In what circumstance would you expect a POST/PUT etc. route to appear in the URL? I'm not sure I totally get it...

@rickychilcott

Copy link
Copy Markdown
Contributor Author

Good question. Short answer is that I have no intention for that to happen, but it "could" happen, so I provided the option.

The longer answer:

Right now I can't use futurism in my app because the current implementation only passes the path (i.e. without the subdomain) to the recognize_path method. Because most of my routes of a subdomain constraint, many of my routes are unrecognized and the code path will raise an exception (ActionController::RoutingError).

The routes that I need it to recognize are currently all GET requests, but in theory, someone could have a POST/PUT/PATCH/DELETE. So... the way I implemented this is that it loops through all possible HTTP verbs to try to infer the correct route.

@julianrubisch

Copy link
Copy Markdown
Contributor

Understood. Just wanted to make sure we don't overlook some security concern here.

@julianrubisch
julianrubisch merged commit 5b64da4 into stimulusreflex:master May 4, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Better Error Handling if routes are not "recognized"

2 participants