jcheruvelil / MusicRecsAPI

1 stars 0 forks source link

Code Review Comments Xia Webster #1

Open iemxia opened 1 month ago

iemxia commented 1 month ago

In your user.py, you are not using parameter binding in your create_user function which can leave you vulnerable to sql injection attacks since the username passed in is from the outside world and will get executed directly as SQL. I would suggest switching all instances of using the format string to use parameter binding like you have done in login_user. (also not parameter binding in playlist.py, ratings.py)

iemxia commented 1 month ago

In your search.py search_tracks function, you can remove the != "" in lines 38, 41, 44 and just replace them with if track: which will do the same thing since an empty string in python is considered false.

iemxia commented 1 month ago

Instead of checking manually if the rating for a particular song by a particular user is already there, then if it is update and if not, insert, use ON CONFLICT (user_id, track_id) DO UPDATE SET rating = EXCLUDED.rating which will do that for you all at once and update the review to be the value that was initially trying to be inserted. Essentially letting a user re-review a song.

iemxia commented 1 month ago

In your search.py, assuming you want to track peope's searches, your code right now does not add any record into the search history table and only returns the results. Same for recs.py

iemxia commented 1 month ago

In playlist.py remove_song_from_playlist you don't have to bind the connection.execute to a variable, and you can just run it as is without the variable assignment.

iemxia commented 1 month ago

In ratings.py set_rating fcn, the last if statement, you are mixing parameter binding and format string? Should just use parameter binding here to be safe

iemxia commented 1 month ago

For checking if things already exist in the table (like in ratings.py and playlist.py), isntead of doing the COUNT(*) you can just select one attribute from the table where the id matches, and check if a result is none or not and decide what to do from there.

iemxia commented 1 month ago

In ratings.py where you are checking if the rating exists in the table already, I do not think it is necessary to do a join with the users table since your ratings table already has a ratings.user_id which is a foreign key reference to users.id so you can just check WHERE ratings.user_id = user_id and it should work the same.

iemxia commented 1 month ago

In playlist.py it seems as if you can add the same song multiple times to a playlist, is this a case you want to allow?

iemxia commented 1 month ago

I would add error handling for when someone tries to get a recommendation for a song thats not in your database.

iemxia commented 1 month ago

In places where you have VARCHAR(255), I assume you only want the field to be 255 characters or less? As of right now, there is no error checking against this and I can make a playlist name as long as I want. Same with username.

iemxia commented 1 month ago

Throughout your code, you use variable name 'result' a lot within the same function (ex: set_rating in ratings.py), and yes it still runs and works but renaming these to be more specific can help improve readability.