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

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

Open
begedin opened this issue Oct 26, 2016 · 7 comments
Open

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

begedin opened this issue Oct 26, 2016 · 7 comments
Assignees
Labels

Comments

@begedin
Copy link
Contributor

begedin commented Oct 26, 2016

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 Fix comment endpoints to match ember #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
Copy link
Contributor Author

begedin commented Oct 26, 2016

@joshsmith Worth a discussion, I think.

@joshsmith
Copy link
Contributor

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

@begedin
Copy link
Contributor Author

begedin commented Oct 27, 2016

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.

@joshsmith
Copy link
Contributor

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
Copy link
Contributor

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
Copy link
Contributor

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

@begedin
Copy link
Contributor Author

begedin commented Jan 2, 2018

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

2 participants