ADR: REST API Guidelines - #12544
Conversation
Dependency Review✅ No vulnerabilities or license issues or OpenSSF Scorecard issues found.Scanned FilesNone |
|
|
||
| Prefer purpose-built endpoints for specific UI needs over stretching a generic endpoint with a narrow filter. A dedicated endpoint documents intent, keeps its response shape minimal, and can be optimized independently. See [Write model vs Read models](/contributing/ADRs/back-end/write-model-vs-read-models) for the internal read/write split this endpoint pattern reflects. | ||
|
|
||
| ### Shadowing dynamic path segments |
There was a problem hiding this comment.
This whole section is new. Original version had just "Stop using shadowing." part, but didn't give us any ideas what to do instead. Maybe it won't be too hard to discuss here.
There was a problem hiding this comment.
I agree with avoiding shadowing, but documentation alone doesn't protects us very well. When adding /projects/foo, it's quite hard for the developer to know that somewhere else /projects/:projectId exists, especially once routes live in different controllers.
Could we make this mechanically detectable instead?
For example, we could add a route lint/test that compares registered routes and flags if you're shadowing an existing route.
There was a problem hiding this comment.
I'm ok with you creating a project for it. If I try solving it now, I won't close this PR this year :D
gastonfournier
left a comment
gastonfournier
left a comment
There was a problem hiding this comment.
I think I like the overall proposal, but my main concern is that the URL would encode two independent dimensions: the resource and its intended audience. Those have different reasons for change. The resource may remain the same while the audience evolves, and encoding both in the path means an audience change can unnecessarily become an API/URL change. That's why I kind of prefer the other alternative (#11264) when it comes to audiences, but regardless, this ADR does add a lot of valuable things.
A suggestion to avoid making the audience targeting a blocker would be to split that part out as a different conversation and keep this one focused in guidelines for APIs (regardless of the audience)
|
|
||
| Prefer purpose-built endpoints for specific UI needs over stretching a generic endpoint with a narrow filter. A dedicated endpoint documents intent, keeps its response shape minimal, and can be optimized independently. See [Write model vs Read models](/contributing/ADRs/back-end/write-model-vs-read-models) for the internal read/write split this endpoint pattern reflects. | ||
|
|
||
| ### Shadowing dynamic path segments |
There was a problem hiding this comment.
I agree with avoiding shadowing, but documentation alone doesn't protects us very well. When adding /projects/foo, it's quite hard for the developer to know that somewhere else /projects/:projectId exists, especially once routes live in different controllers.
Could we make this mechanically detectable instead?
For example, we could add a route lint/test that compares registered routes and flags if you're shadowing an existing route.
Co-authored-by: Gastón Fournier <gaston@getunleash.io>
chriswk
left a comment
chriswk
left a comment
There was a problem hiding this comment.
I think this agrees with what I've been thinking regarding our REST APIs so I'm going to vote a +1 here.
Some inline comments regarding the importance of thorough reviews (for new SQL) and consideration (if adding endpoints to existing server/browser SDK contexts).
|
|
||
| This is the response-side counterpart to [Separation of request and response schemas](/contributing/ADRs/overarching/separation-request-response-schemas): responses are tight and precise; request schemas can be more forgiving. | ||
|
|
||
| ### Filter in SQL, not JS |
There was a problem hiding this comment.
👍 - This has been a pet peeve for me for some of our existing endpoints, filtering in SQL also saves data transfer and increases response speed. Because we no longer transfer data out and then just use CPU time to toss it on the floor.
Does mean that the SQL will require thorough reviewing though.
| ### Positive | ||
|
|
||
| * New list endpoints paginate by default and behave the same way from the caller's perspective. | ||
| * Frontend and API consumers can predict the query params for search, pagination, and sorting without reading each endpoint's docs. |
There was a problem hiding this comment.
Having a common PaginationParameters that can be used for collection endpoints is great
|
|
||
| ### Query parameter conventions | ||
|
|
||
| Reuse existing names before inventing new ones: |
github-project-automation
Bot
moved this from In Progress
to Approved PRs
in Issues and PRs
3 participants
About the changes
Introducing a Guideline Document for creating new REST endpoints.
It's an ADR, but it doesn't cover everything this kind of document should.
From here we can either iterate on it or change it's name to be more specific. (Ideas?)
Important files
Discussion points
OSS PR checklist