Add courses aggregate endpoint to professor endpoint

[mikehquan19]

Hi!
I've added the course endpoints in the professor's routes that return the array of courses taught by the queried professors (or single professor). I basically just filter the professors first and then, through several stages of the aggregation pipelines, sort of efficiently obtain the list of distinct courses by those professors.

Let me know ASAP if there's anything I need to fix.
Thank you.

[mikehquan19]

I also completed task #227 and added the section aggregate endpoint to the course endpoint in this pull request.

Thank you.

[jpahm]

I will review this shortly, thanks much!

[jpahm]

Sorry for the wait on this review, overall this looks very good!

My only concern is that we should probably have pagination for the query-based endpoints (like the standard query endpoints) using the same limit and offset system. As of right now, there's nothing stopping the user from running a raw query like /course/section and essentially just pulling all of the sections in the database.

Also, as a small change, it would be nice it we could change the routes to use the words sections and courses instead of section and course to make it abundantly clear to the user that these endpoints return a collection of sections and courses, respectively.

Thank you!

[mikehquan19]

Got it! I will fix it ASAP.

[mikehquan19]

Hi @jpahm, I have 2 questions regarding the limit and offset system.

I played around with the API a little bit, and here's what I learned. For example, if I call /professor?first_name=John, I will only get 20 documents, even though there are 42 professors named John in the database, because the default limit is 20. So if I want to get all the documents, I will have to call /professor?first_name=John with offset=20, 40, 60,... until I get all of them. Is my understanding correct?

For aggregate endpoints, i.e professor/courses?, do you think it'd be a better idea if I set offset and limit on the professors or on the courses pulled from those professors ?

[jpahm]

You are correct! Also, as far as pagination is concerned, I think it would be best to paginate the professors before you pull their courses so as to allow for getting all of a professor's courses at a time without needing to pull all professors at once. We will need to make sure this functionality is clearly documented so as to not cause confusion for our users, though.

Also, after doing some further testing, there is unfortunately a problem with the current implementation:
replaceWith aggregations such as bson.D{{Key: "$replaceWith", Value: "$courses"}} cause a panic if $courses does not evaluate to an object, which happens for any professor that doesn't teach any courses. As of right now, a query such as /professor/courses?first_name=John causes a panic with this message:
Executor error during getMore :: caused by :: 'replacement document' must evaluate to an object, but resulting value was: MISSING. Type of resulting value: 'missing'. Input document: {_id: 6704e56fef056d3fccfbbafd}
due to the fact that the professor with the mentioned _id has an empty array of courses.

[mikehquan19]

That's great. Let me rephrase to make sure I understand it.

We paginate the professors so that when we call the endpoint we will only get all the courses of 20 professors at the time, and then we can call the endpoint again with offset=20 if we want all the courses of the next 20 professors. Is it correct?

As for the problem in aggregation, I caught it too. Fortunately, the solution is easy as I just need to set preserveNullAndEmptyArrays to false in the $unwind stage of the pipeline, which gets rid of those empty arrays.

[jpahm]

You understand correctly!

Glad to hear the problem was easy to fix.

[mikehquan19]

I've fixed the pagination and renamed the endpoints. Luckily enough, mongo pipeline has $skip and $limit stages which does exactly what SetSkip() & SetLimit() of FindOptions does. I added GetAggregateLimit() in setup.go which just returns offset and limit. However, I wonder if we can combine GetAggregateLimit() and GetOptionsLimit() together since they share a lot in common and only returns different types.

Thank you!