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

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

Draft
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

moznion
Copy link

@moznion moznion commented Aug 22, 2024

Original discussion: #328

@pirj
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?

@moznion
Copy link
Author

moznion commented Aug 22, 2024

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.

@dvandersluis
Copy link
Member

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

@moznion
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?

@dvandersluis
Copy link
Member

Putting it under Testing would make sense to me!

@moznion
Copy link
Author

moznion commented Aug 23, 2024

@dvandersluis Thank you, I moved it: 1c4cf0e

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.

3 participants