jcheruvelil / MusicRecsAPI

1 stars 0 forks source link

Schema/API Design comments Xia Webster #2

Open iemxia opened 5 months ago

iemxia commented 5 months ago

image image

Make sure your recommendation endpoint matches what you have in your APISpecs or that your APIspec matches your code. It looks like right now you have in your code (recs.py) the endpoint as /recs that takes in a track_id string as a parameter but your APISpec lists it as /recommendation/song that takes in a JSON object. The response objects are also different. Maybe this is still in progress, but just make sure that the two match since it is slightly confusing on what your code should be doing according to your APISpec. (Also your /history/recommendation endpoint in the code is listed backwards as /recommendation/history in APISpec)

iemxia commented 5 months ago

Schema.SQL file you are creating two tables with same name? In line 34, I think one is suppose to be ratings not playlist_tracks again.

iemxia commented 5 months ago

Under your RATINGS comment where I presume you meant to make a ratings table, is it referecing it's own key? Or is there another table it's referencing that exists for you guys but not in the schema.sql file? If the rating column in this table is meant to be an integer rating made by a single user, what exactly would it need to be referencing?

iemxia commented 5 months ago

Maybe could be useful to have a remove playlist endpoint as well? Just like how you can remove playlists in other listening apps such as Spotify.

iemxia commented 5 months ago

I think it would be useful when you are getting user library to also list the tracks that are in each playlist? So a JSON object for each playlist, which then has a list of tracks inside of it.

iemxia commented 5 months ago

Your APISpec includes endpoints that are not yet implemented, perhaps still in progress. (ex: /recommendation/song , /recommendation/artist , /recommendation/album , /album/{album_id}/rating)

iemxia commented 5 months ago

For your tracks table, what is the use case difference between id and track_id ? One is text and one is a big int, but it looks like in your code you only use track_id when querying and not id.

iemxia commented 5 months ago

You have no primary key for tracks table.

iemxia commented 5 months ago

It seems like you could delete either track_id or id from tracks table and just reference one of those in the rest of your tables since you don't need two unique identifiers for a single track. I would suggest using track_id since you are using that already in all your endpoints and querying and just change the schema.sql foreign key references to use track_id instead of id for the tracks table.

iemxia commented 5 months ago

You have two APISpec files in two different locations. I assume the one in the docs folder is most up to date, but you should probably remove the other one to avoid confusion.

iemxia commented 5 months ago

Looking at your search history end point that takes in a user_id, I'm assuming the intent is to return the history of searches a specific user has made, however when you run the search endpoint there is no no parameter for the user that ran that search, so how are you keeping track of who searches what? I would maybe suggest adding user_id as a parameter in the search endpoint so you can track who does what. Also in testing, search did not work as expected.

iemxia commented 5 months ago

What exactly does /login/ do practically?

iemxia commented 5 months ago

It looks like in your search/recommendation history endpoints your APISpec says it returns a JSON object with the songs that got recommended/searched but your code itself is returning a list of HistoryItems that contain the user_id and query?