code-corps-api icon indicating copy to clipboard operation
code-corps-api copied to clipboard

Several of our endpoints allow fetching all records of that type.

Open begedin opened this issue 9 years ago • 7 comments

Several of our endpoints allow fetching all records. We should really handle this in some way.

The way I see it, we have two choices

  1. Enforce query requirements. If those query requirements are not met (for example, a list of ids), we can
    • Raise a custom error and return that error as a response
    • Return 0 results
  2. Limit index/all requests to X records

List of endpoints that allow this

  • category
  • comment (after merging #372, could've fixed there, but figured we should handle it all after a discussion)
  • donation_goal
  • organization
  • project_category
  • project_controller
  • project_skill
  • role
  • role_skill
  • skill
  • task
  • user
  • user_category
  • user_role
  • user_skill

Obviously, not all of these are as troublesome, but we should probably handle them all as consistently as it makes sense.

begedin avatar Oct 26 '16 09:10 begedin

@JoshSmith Worth a discussion, I think.

begedin avatar Oct 26 '16 09:10 begedin

Agree. Maybe we could ask Alan how he handles this in most of his APIs? Would you mind asking him?

joshsmith avatar Oct 26 '16 15:10 joshsmith

I spoke to Alan and he said in most cases, they have some sort of default pagination, meaning they limit the number of records they return. He also mentioned that in some cases, where we're supposed to provide query arguments, it would make sense to render an error instead.

Really, we need to think about our endpoints and decide on our own.

The way I see it, the "join" tables should not have an "all" endpoint. The index there is meant purely for coalesced id requests. If there is no id filter, we should render an error, something along the lines of RequestTooBroad.

For the other tables, we should consider and weigh our options. Do we want a comments endpoint that returns pages of all comments, for example? It's not needed right now, but it might make more sense than limiting the /commentsendpoint to coalesced id requests only.

begedin avatar Oct 27 '16 07:10 begedin

I don't know on many of these endpoints why we'd be paginating. Is there any sort of convention around the kind of error returned for too broad a request by a client?

joshsmith avatar Oct 27 '16 15:10 joshsmith

The only endpoints we might want to paginante are category, organization, role, and skill. Task could make sense to paginate in the context of not returning too many results in our new UI, but isn't that already paginated?

joshsmith avatar Oct 27 '16 15:10 joshsmith

Is this still relevant? I think we've basically figured out an approach for this now.

joshsmith avatar Dec 31 '17 16:12 joshsmith

Most of our index endpoints still could potentially return all records of that type. The main issue is that we have index endpoints which exist purely to support "coalesced id" requests from Ember. These will default to all records if no id filter is provided.

Some of these could benefit from being scoped to the current user, but that might not be enough for a lot of them and would only help in short term for most of them.

Some sort of hard limit or pagination support would definitely be useful eventually.

Category, Comment, DonationGoal, GithubAppInstallation, 
GithubIssue, GithubPullRequest, GithubRepo, Organization, 
OrganizationGithubAppInstallation, Project, ProjectCategory, 
ProjectSkill, ProjectUser, Role, RoleSkill, Skill, TaskList, TaskSkill,
User, UserCategory, UserRole, UserSkill, UserTask

Task specifically is, by default, scoped by the archived field, but outside of that is still likely to return a lot of data.

These are scoped, but could return a lot of data for admin users. As we grow, regular users might be problematic to. We will likely add pagination here anyway:

Conversation, ConversationPart, Message

begedin avatar Jan 02 '18 13:01 begedin