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

Document pagination behavior for query-based endpoints #241

Open
jpahm opened this issue Nov 21, 2024 · 12 comments
Open

Document pagination behavior for query-based endpoints #241

jpahm opened this issue Nov 21, 2024 · 12 comments
Assignees
Labels
L1 A task suitable for someone who is comfortable helping with basic issues.

Comments

@jpahm
Copy link
Contributor

jpahm commented Nov 21, 2024

Currently, the paginated nature of the query-based endpoints (i.e. /course, /course/sections) is not explicitly documented in the OpenAPI spec, which is a considerable flaw. We should add the offset query parameter to the OpenAPI doc comments (via @param) on the relevant controllers and properly document its behavior. The details of how the pagination behaves and the usage of the offset parameter should be referenced in the description of the endpoint as well.

@jpahm jpahm added the L1 A task suitable for someone who is comfortable helping with basic issues. label Nov 21, 2024
@justinschwerdtfeger
Copy link

I would like to work on this issue

@justinschwerdtfeger
Copy link

/course/sections is not currently paginated as far as I can tell, but I'm assuming its supposed to be. I think I can fix it, but should it be a separate issue?

@justinschwerdtfeger
Copy link

I misunderstood how the query-based endpoints work, but I still have some questions about their behavior.

How would an API user know what their offset should be to get their desired results?
If the offset was set to 20 and the query had more than 20 results, how would an API user know what the offset should be to see the rest of the results?

Should the number 20 be in the documentation? There's also the LIMIT environment variable, which might not make sense if 20 is in the documentation.

I don't know if this is a very good solution, but I am probably still misunderstanding.

@jpahm
Copy link
Contributor Author

jpahm commented Feb 2, 2025

You're not wrong, and this is something that should be resolved before these endpoints go out to prod. We should look into perhaps adding some information about the offset into the response objects or performing some other means of letting the user know where they are in the pagination. I'll open an issue for this soon.

@mikehquan19
Copy link
Contributor

Is there a possibility that we can put offset in both former and latter thing of the endpoints? For example, course/sections can have both courseOffset and sectionOffset. That way the users can have more control over the result. I was thinking about that when I wrote those endpoints because like @justinschwerdtfeger said, we don’t have something unpredictable.

@jpahm
Copy link
Contributor Author

jpahm commented Feb 3, 2025

I would agree, having pagination for both is likely the best course of action here. We'll need to be clear in the documentation about the fact that two separate offsets are being used, since this will be a deviation from the behavior of the standard endpoints.

@mikehquan19
Copy link
Contributor

That sounds great. Since it's related to tasks #230 and #227, which I'm in charge of, I would try rewriting the code and then @justinschwerdtfeger can write the documentation based off it. Or maybe you could assign the task to other members?

@justinschwerdtfeger
Copy link

justinschwerdtfeger commented Feb 3, 2025

I think I could just make a PR for only /course /professor and /section and only address those with this issue. How does that sound?

@mikehquan19
Copy link
Contributor

It sounds great too, we can try that. But it's up to @jpahm really.

@jpahm
Copy link
Contributor Author

jpahm commented Feb 3, 2025

@mikehquan19 Let's have you adjust your code accordingly and then @justinschwerdtfeger can just focus on documenting the normal endpoints for now. I'll open up a separate issue for documenting the revised endpoints at a later date.

@jpahm
Copy link
Contributor Author

jpahm commented Feb 6, 2025

@mohammadmehrab Please reference the above and make the appropriate pagination changes to your PR as well if possible. Let me know if this is a task I need to open up for someone else.

@mohammadmehrab
Copy link
Contributor

@jpahm I'll take a look and make the appropriate changes. I'll let you know if I have any trouble, thanks for alerting me

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
L1 A task suitable for someone who is comfortable helping with basic issues.
Projects
None yet
Development

No branches or pull requests

4 participants