Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Sign up
Appearance settings

Add a style guide to suggest to pass a string literal as a URL to http method calls #359

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

Draft
moznion wants to merge 2 commits into rubocop:master
base: master
Choose a base branch
Loading
from moznion:http-url

Conversation

@moznion
Copy link

@moznion moznion commented Aug 22, 2024

Original discussion: #328

...p method calls
Original discussion: rubocop#328
Signed-off-by: moznion <moznion@mail.moznion.net>
Copy link
Member

pirj commented Aug 22, 2024

I’m in general in favour of this change, but I admit that I’m biased because I’m mostly working on APIs, and the general picture may not be as simple as the limited use case I got used to.

Rails guides say (about the use of path helpers in views):

patient_path(@patient) then the router will generate the path /patients/17. This reduces the brittleness of your view and makes your code easier to understand

A lot of to digest in this brief note:

  1. Reduces brittleness. Path helpers add an abstraction over a detail, the URL path. It’s a design decision whether those paths should be stable or can be bent at will. For the public API or pages that will be bookmarked, shared or referenced from other resources - it makes sense to keep stable. For internal APIs, APIs for own mobile apps or web - not really.
  2. Easier to understand. I don’t really take this argument. Pretty sure this can be stretched in a way that the URL will be easier to understand than a path helper.

What is the source of the brittleness?
A mistake causing a change to the URL. Or a deliberate routes refactoring.
It can indeed cause views and redirects to point to the outdated route.
But with tests?

Should we introduce a concept of route stability, and say that this only makes sense as a measure to ensure route stability? And otherwise path helpers are preferrable to reduce brittleness?

What do you think?

Copy link
Author

moznion commented Aug 22, 2024
edited
Loading

I appreciate your explanation.

Easier to understand. I don’t really take this argument. Pretty sure this can be stretched in a way that the URL will be easier to understand than a path helper.

Yeah, I’m on the same page, and the main motivation comes from this.

Anyway, my understanding of the term "brittleness" is related to path-traversal attacks, where malicious "URL-ish" arguments are injected. I completely agree with the approach of using helpers with URL escaping on the given argument to protect against this type of vulnerability.
However, in testing, this shouldn’t be a problem since it’s just a test. Personally, I believe that other issues, like breaking changes to endpoints, should be caught by the tests.

Copy link
Member

Based on #328 shouldn't this section mention that it applies to tests and not production code?

Copy link
Author

moznion commented Aug 23, 2024

@dvandersluis Thank you for your suggestion; it sounds great to me. However, what would be the best approach: should I include that in the description, or move it under the "Testing" section?

Copy link
Member

Putting it under Testing would make sense to me!

andyw8 reacted with thumbs up emoji

Signed-off-by: moznion <moznion@mail.moznion.net>
Copy link
Author

moznion commented Aug 23, 2024

@dvandersluis Thank you, I moved it: 1c4cf0e

Copy link
Contributor

andyw8 commented Oct 22, 2024

@moznion you might also want to mention tests in the PR title, I was confused the first time reading this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

AltStyle によって変換されたページ (->オリジナル) /