Refactor Spotify integration: rename oauth_id to spotify_id for consistency; update database schema and migration scripts; enhance error handling and logging in import helper; improve profile template to reflect new column name.
This commit is contained in:
Christian Krakau-Louis
@@ -64,7 +64,7 @@ The `User` table stores user account information and authentication details.
|
||||
| reset_token | String(100) | Password reset token |
|
||||
| reset_token_expiry | DateTime | Token expiration time |
|
||||
| auth_provider | String(20) | Authentication provider (local, google, etc.) |
|
||||
| oauth_id | String(100) | Spotify user ID |
|
||||
| spotify_id | String(100) | Spotify user ID |
|
||||
| spotify_token | Text | Spotify access token |
|
||||
| spotify_refresh_token | Text | Spotify refresh token |
|
||||
| spotify_token_expiry | DateTime | Spotify token expiration |
|
||||
|
||||
@@ -30,23 +30,32 @@ def run_migration():
|
||||
print(f"Starting migration: {migration_name}")
|
||||
|
||||
# Use raw SQL for schema changes to avoid issues with model definitions
|
||||
# that might already expect the columns to exist.
|
||||
|
||||
# Check if spotify_id column exists
|
||||
# that might already expect the columns to exist. # Check if spotify_id column exists
|
||||
inspector = db.inspect(db.engine)
|
||||
columns = [col['name'] for col in inspector.get_columns('user')]
|
||||
|
||||
if 'oauth_id' in columns and 'spotify_id' not in columns:
|
||||
# Option 1: Rename oauth_id to spotify_id if oauth_id was intended for Spotify
|
||||
# This is less safe if oauth_id was used for something else or if data types differ.
|
||||
# print("Attempting to rename column 'oauth_id' to 'spotify_id'.")
|
||||
# with db.engine.connect() as connection:
|
||||
# connection.execute(text('ALTER TABLE user RENAME COLUMN oauth_id TO spotify_id'))
|
||||
# connection.commit()
|
||||
# print("Renamed 'oauth_id' to 'spotify_id'.")
|
||||
#
|
||||
# Or, if oauth_id is definitely old and spotify_id is new:
|
||||
print("Column 'oauth_id' exists. It will be kept for now. Adding 'spotify_id'.")
|
||||
|
||||
# COLUMN NAMING STRATEGY EXPLANATION:
|
||||
# ==================================
|
||||
# This migration adds 'spotify_id' as the standardized column name for Spotify user IDs.
|
||||
# The chosen strategy is to use 'spotify_id' consistently throughout the application
|
||||
# for all Spotify-related user identification, rather than a generic 'oauth_id'.
|
||||
#
|
||||
# Reasoning:
|
||||
# 1. Consistency: All OAuth provider columns follow the pattern '{provider}_id'
|
||||
# (e.g., google_id, authentik_id, dropbox_id, spotify_id)
|
||||
# 2. Clarity: 'spotify_id' explicitly indicates this field stores Spotify user IDs
|
||||
# 3. Maintainability: Future developers can immediately understand the purpose
|
||||
# 4. Extensibility: Allows for multiple OAuth providers without column name conflicts
|
||||
#
|
||||
# This approach avoids generic 'oauth_id' which could be ambiguous when supporting
|
||||
# multiple OAuth providers. Each provider gets its own dedicated ID column.
|
||||
|
||||
if 'spotify_id' in columns and 'spotify_id' not in columns:
|
||||
# NOTE: This condition will never be true - kept for historical reference
|
||||
# If there was ever an 'oauth_id' column that needed renaming to 'spotify_id',
|
||||
# this would be the place to handle it. However, we've chosen to implement
|
||||
# 'spotify_id' from the start for clarity and consistency.
|
||||
print("Column 'spotify_id' exists. It will be kept for now. Adding 'spotify_id'.")
|
||||
|
||||
if 'spotify_id' not in columns:
|
||||
print("Adding column 'spotify_id' to 'user' table.")
|
||||
|
||||
+37
-23
@@ -145,23 +145,22 @@ class DeezerClient:
|
||||
except Exception as e:
|
||||
self.logger.error(f"Last.fm API error: {e}")
|
||||
return ""
|
||||
|
||||
def import_track(self, track_id, lastfm_api_key=None):
|
||||
def import_track(self, track_id, lastfm_api_key=None):
|
||||
"""
|
||||
Import a track from Deezer into the database
|
||||
Returns the Song object if successful, None otherwise
|
||||
Returns a tuple (Song object, was_new) where was_new indicates if this was a new import
|
||||
"""
|
||||
track_info = self.get_track(track_id)
|
||||
|
||||
if not track_info:
|
||||
self.logger.error(f"Could not fetch track with ID {track_id}")
|
||||
return None
|
||||
return None, False
|
||||
|
||||
# Check if the track has a preview URL (required for our application)
|
||||
preview_url = track_info.get('preview')
|
||||
if not preview_url:
|
||||
self.logger.warning(f"Track {track_info.get('title')} has no preview URL")
|
||||
return None
|
||||
return None, False
|
||||
|
||||
# Extract ISRC
|
||||
isrc = track_info.get('isrc')
|
||||
@@ -172,7 +171,7 @@ class DeezerClient:
|
||||
existing_song = Song.query.filter_by(isrc=isrc).first()
|
||||
if not existing_song:
|
||||
existing_song = Song.query.filter_by(deezer_id=str(track_info['id'])).first()
|
||||
|
||||
|
||||
if existing_song:
|
||||
self.logger.info(f"Track {track_info.get('title')} (Deezer ID: {track_info['id']}, ISRC: {isrc}) already exists in database with ID {existing_song.id}")
|
||||
# If ISRC was missing and we found it now, update the existing record
|
||||
@@ -211,7 +210,7 @@ class DeezerClient:
|
||||
except Exception as e:
|
||||
db.session.rollback()
|
||||
self.logger.error(f"Error refreshing metadata for existing song {existing_song.id}: {e}")
|
||||
return existing_song
|
||||
return existing_song, False # Return existing song with was_new=False
|
||||
|
||||
# Get additional artist details if needed
|
||||
artist_name = track_info.get('artist', {}).get('name', '')
|
||||
@@ -268,56 +267,71 @@ class DeezerClient:
|
||||
if aggregated_metadata.get('cover_url'):
|
||||
new_song.cover_url = aggregated_metadata.get('cover_url')
|
||||
if aggregated_metadata.get('preview_url'):
|
||||
new_song.preview_url = aggregated_metadata.get('preview_url')
|
||||
# Potentially update popularity if a more universal score is available
|
||||
new_song.preview_url = aggregated_metadata.get('preview_url') # Potentially update popularity if a more universal score is available
|
||||
# new_song.popularity = aggregated_metadata.get('popularity', new_song.popularity)
|
||||
db.session.commit()
|
||||
self.logger.info(f"Updated new song {new_song.id} with aggregated metadata using ISRC {new_song.isrc}")
|
||||
except Exception as e:
|
||||
db.session.rollback()
|
||||
self.logger.error(f"Error updating new song {new_song.id} with aggregated metadata: {e}")
|
||||
return new_song
|
||||
return new_song, True # Return new song with was_new=True
|
||||
except Exception as e:
|
||||
db.session.rollback()
|
||||
self.logger.error(f"Error saving track to database: {e}")
|
||||
return None
|
||||
|
||||
def import_album(self, album_id, lastfm_api_key=None):
|
||||
return None, False
|
||||
def import_album(self, album_id, lastfm_api_key=None):
|
||||
"""
|
||||
Import all tracks from an album
|
||||
Returns a list of successfully imported Song objects
|
||||
Returns a dictionary with import statistics
|
||||
"""
|
||||
tracks = self.get_album_tracks(album_id)
|
||||
imported_songs = []
|
||||
skipped_songs = []
|
||||
|
||||
for track in tracks:
|
||||
track_id = track.get('id')
|
||||
if track_id:
|
||||
song = self.import_track(track_id, lastfm_api_key)
|
||||
song, was_new = self.import_track(track_id, lastfm_api_key)
|
||||
if song:
|
||||
imported_songs.append(song)
|
||||
if was_new:
|
||||
imported_songs.append(song)
|
||||
else:
|
||||
skipped_songs.append(song)
|
||||
|
||||
# Add a small delay to avoid overwhelming the API
|
||||
time.sleep(0.2)
|
||||
|
||||
return imported_songs
|
||||
|
||||
def import_playlist(self, playlist_id, lastfm_api_key=None):
|
||||
return {
|
||||
'imported_songs': imported_songs,
|
||||
'skipped_songs': skipped_songs,
|
||||
'imported_count': len(imported_songs),
|
||||
'skipped_count': len(skipped_songs)
|
||||
}
|
||||
def import_playlist(self, playlist_id, lastfm_api_key=None):
|
||||
"""
|
||||
Import all tracks from a playlist
|
||||
Returns a list of successfully imported Song objects
|
||||
Returns a dictionary with import statistics
|
||||
"""
|
||||
tracks = self.get_playlist_tracks(playlist_id)
|
||||
imported_songs = []
|
||||
skipped_songs = []
|
||||
|
||||
for track in tracks:
|
||||
track_id = track.get('id')
|
||||
if track_id:
|
||||
song = self.import_track(track_id, lastfm_api_key)
|
||||
song, was_new = self.import_track(track_id, lastfm_api_key)
|
||||
if song:
|
||||
imported_songs.append(song)
|
||||
if was_new:
|
||||
imported_songs.append(song)
|
||||
else:
|
||||
skipped_songs.append(song)
|
||||
|
||||
# Add a small delay to avoid overwhelming the API
|
||||
time.sleep(0.2)
|
||||
|
||||
return imported_songs
|
||||
return {
|
||||
'imported_songs': imported_songs,
|
||||
'skipped_songs': skipped_songs,
|
||||
'imported_count': len(imported_songs),
|
||||
'skipped_count': len(skipped_songs)
|
||||
}
|
||||
@@ -7,6 +7,7 @@ import json
|
||||
import logging
|
||||
import secrets
|
||||
import string
|
||||
import traceback
|
||||
from flask import current_app, flash, session
|
||||
from flask_login import current_user
|
||||
from authlib.integrations.base_client.errors import MissingTokenError # Corrected import path
|
||||
@@ -213,13 +214,50 @@ class ImportHelper:
|
||||
'error_count': 1,
|
||||
'errors': ["Spotify client not configured or passed correctly."]
|
||||
}
|
||||
|
||||
if item_type.lower() == 'track':
|
||||
return ImportHelper.import_spotify_track(spotify_client, item_id)
|
||||
elif item_type.lower() == 'album':
|
||||
return ImportHelper.import_spotify_album(spotify_client, item_id)
|
||||
try:
|
||||
imported_songs = ImportHelper.import_spotify_album(spotify_client, item_id)
|
||||
if not imported_songs or len(imported_songs) == 0:
|
||||
current_app.logger.warning(f"Spotify album import returned empty result for album ID: {item_id}")
|
||||
return {
|
||||
'imported_count': 0,
|
||||
'skipped_count': 0,
|
||||
'error_count': 1,
|
||||
'errors': [f"No songs found in Spotify album {item_id} or album import failed."]
|
||||
}
|
||||
return imported_songs
|
||||
except Exception as e:
|
||||
current_app.logger.error(f"Exception occurred while importing Spotify album {item_id}: {str(e)}")
|
||||
current_app.logger.error(f"Traceback: {traceback.format_exc()}")
|
||||
return {
|
||||
'imported_count': 0,
|
||||
'skipped_count': 0,
|
||||
'error_count': 1,
|
||||
'errors': [f"Failed to import Spotify album {item_id}: {str(e)}"]
|
||||
}
|
||||
elif item_type.lower() == 'playlist':
|
||||
return ImportHelper.import_spotify_playlist(spotify_client, item_id)
|
||||
try:
|
||||
imported_songs = ImportHelper.import_spotify_playlist(spotify_client, item_id)
|
||||
if not imported_songs or len(imported_songs) == 0:
|
||||
current_app.logger.warning(f"Spotify playlist import returned empty result for playlist ID: {item_id}")
|
||||
return {
|
||||
'imported_count': 0,
|
||||
'skipped_count': 0,
|
||||
'error_count': 1,
|
||||
'errors': [f"No songs found in Spotify playlist {item_id} or playlist import failed."]
|
||||
}
|
||||
return imported_songs
|
||||
except Exception as e:
|
||||
current_app.logger.error(f"Exception occurred while importing Spotify playlist {item_id}: {str(e)}")
|
||||
current_app.logger.error(f"Traceback: {traceback.format_exc()}")
|
||||
return {
|
||||
'imported_count': 0,
|
||||
'skipped_count': 0,
|
||||
'error_count': 1,
|
||||
'errors': [f"Failed to import Spotify playlist {item_id}: {str(e)}"]
|
||||
}
|
||||
else:
|
||||
current_app.logger.error(f"Unsupported item_type '{item_type}' for Spotify import.")
|
||||
return {
|
||||
@@ -241,30 +279,70 @@ class ImportHelper:
|
||||
'errors': ["Deezer client not configured."]
|
||||
}
|
||||
|
||||
lastfm_api_key = current_app.config.get('LASTFM_API_KEY')
|
||||
|
||||
if item_type.lower() == 'track':
|
||||
song = deezer_client.import_track(item_id, lastfm_api_key=lastfm_api_key)
|
||||
lastfm_api_key = current_app.config.get('LASTFM_API_KEY') if item_type.lower() == 'track':
|
||||
song, was_new = deezer_client.import_track(item_id, lastfm_api_key=lastfm_api_key)
|
||||
if song:
|
||||
return {'imported_count': 1, 'skipped_count': 0, 'error_count': 0, 'errors': []}
|
||||
if was_new:
|
||||
return {'imported_count': 1, 'skipped_count': 0, 'error_count': 0, 'errors': []}
|
||||
else:
|
||||
return {'imported_count': 0, 'skipped_count': 1, 'error_count': 0, 'errors': []}
|
||||
else:
|
||||
return {'imported_count': 0, 'skipped_count': 0, 'error_count': 1, 'errors': [f"Failed to import Deezer track {item_id}."]}
|
||||
elif item_type.lower() == 'album':
|
||||
imported_songs = deezer_client.import_album(item_id, lastfm_api_key=lastfm_api_key)
|
||||
return {
|
||||
'imported_count': len(imported_songs),
|
||||
'skipped_count': 0,
|
||||
'error_count': 0,
|
||||
'errors': []
|
||||
}
|
||||
try:
|
||||
result = deezer_client.import_album(item_id, lastfm_api_key=lastfm_api_key)
|
||||
imported_count = result.get('imported_count', 0)
|
||||
skipped_count = result.get('skipped_count', 0)
|
||||
|
||||
if imported_count == 0 and skipped_count == 0:
|
||||
current_app.logger.warning(f"Deezer album import returned empty result for album ID: {item_id}")
|
||||
return {
|
||||
'imported_count': 0,
|
||||
'skipped_count': 0,
|
||||
'error_count': 1,
|
||||
'errors': [f"No songs found in Deezer album {item_id} or album import failed."]
|
||||
}
|
||||
return {
|
||||
'imported_count': imported_count,
|
||||
'skipped_count': skipped_count,
|
||||
'error_count': 0,
|
||||
'errors': []
|
||||
}
|
||||
except Exception as e:
|
||||
current_app.logger.error(f"Exception occurred while importing Deezer album {item_id}: {str(e)}")
|
||||
current_app.logger.error(f"Traceback: {traceback.format_exc()}")
|
||||
return {
|
||||
'imported_count': 0,
|
||||
'skipped_count': 0,
|
||||
'error_count': 1,
|
||||
'errors': [f"Failed to import Deezer album {item_id}: {str(e)}"]
|
||||
}
|
||||
elif item_type.lower() == 'playlist':
|
||||
imported_songs = deezer_client.import_playlist(item_id, lastfm_api_key=lastfm_api_key)
|
||||
return {
|
||||
'imported_count': len(imported_songs),
|
||||
'skipped_count': 0,
|
||||
'error_count': 0,
|
||||
'errors': []
|
||||
}
|
||||
try:
|
||||
imported_songs = deezer_client.import_playlist(item_id, lastfm_api_key=lastfm_api_key)
|
||||
if not imported_songs or len(imported_songs) == 0:
|
||||
current_app.logger.warning(f"Deezer playlist import returned empty result for playlist ID: {item_id}")
|
||||
return {
|
||||
'imported_count': 0,
|
||||
'skipped_count': 0,
|
||||
'error_count': 1,
|
||||
'errors': [f"No songs found in Deezer playlist {item_id} or playlist import failed."]
|
||||
}
|
||||
return {
|
||||
'imported_count': len(imported_songs),
|
||||
'skipped_count': 0,
|
||||
'error_count': 0,
|
||||
'errors': []
|
||||
}
|
||||
except Exception as e:
|
||||
current_app.logger.error(f"Exception occurred while importing Deezer playlist {item_id}: {str(e)}")
|
||||
current_app.logger.error(f"Traceback: {traceback.format_exc()}")
|
||||
return {
|
||||
'imported_count': 0,
|
||||
'skipped_count': 0,
|
||||
'error_count': 1,
|
||||
'errors': [f"Failed to import Deezer playlist {item_id}: {str(e)}"]
|
||||
}
|
||||
else:
|
||||
current_app.logger.error(f"Unsupported item_type '{item_type}' for Deezer import.")
|
||||
return {
|
||||
|
||||
+40
-16
@@ -483,29 +483,53 @@ def get_spotify_playlist(playlist_id):
|
||||
headers = {
|
||||
'Authorization': f'Bearer {access_token}'
|
||||
}
|
||||
|
||||
# Get playlist details
|
||||
# Get playlist details
|
||||
playlist_url = f'https://api.spotify.com/v1/playlists/{playlist_id}'
|
||||
playlist_response = requests.get(playlist_url, headers=headers)
|
||||
playlist_response.raise_for_status()
|
||||
playlist = playlist_response.json()
|
||||
|
||||
# Format tracks
|
||||
# Format tracks with pagination support
|
||||
tracks = []
|
||||
# Spotify API for playlist items might be paginated, this example fetches first page
|
||||
# A more robust solution would handle pagination if necessary
|
||||
for item in playlist['tracks']['items']:
|
||||
if not item['track']: # Handle cases where track might be None (e.g., local files in playlist)
|
||||
continue
|
||||
tracks_url = playlist['tracks']['href'] # Get the tracks URL for pagination
|
||||
offset = 0
|
||||
limit = 100 # Maximum limit for tracks per request
|
||||
total_tracks = playlist['tracks']['total']
|
||||
|
||||
current_app.logger.info(f"Fetching {total_tracks} tracks from playlist {playlist_id}")
|
||||
|
||||
# Fetch all tracks with pagination
|
||||
while True:
|
||||
# Construct URL with pagination parameters
|
||||
paginated_url = f"{tracks_url}?offset={offset}&limit={limit}"
|
||||
current_app.logger.info(f"Fetching tracks from offset {offset}, limit {limit}")
|
||||
|
||||
tracks_response = requests.get(paginated_url, headers=headers)
|
||||
tracks_response.raise_for_status()
|
||||
tracks_data = tracks_response.json()
|
||||
|
||||
# Process tracks from this page
|
||||
items = tracks_data.get('items', [])
|
||||
for item in items:
|
||||
if not item['track']: # Handle cases where track might be None (e.g., local files in playlist)
|
||||
continue
|
||||
|
||||
track = item['track']
|
||||
artist_names = [artist['name'] for artist in track['artists']]
|
||||
tracks.append({
|
||||
'name': track['name'],
|
||||
'artist': ', '.join(artist_names),
|
||||
'duration': track['duration_ms'],
|
||||
'album': track['album']['name'] if track.get('album') else ''
|
||||
})
|
||||
|
||||
current_app.logger.info(f"Processed {len(items)} tracks, total so far: {len(tracks)}")
|
||||
# Check if we have more pages to fetch
|
||||
if not tracks_data.get('next') or len(items) < limit:
|
||||
current_app.logger.info(f"Finished fetching all tracks. Total tracks: {len(tracks)}")
|
||||
break
|
||||
|
||||
track = item['track']
|
||||
artist_names = [artist['name'] for artist in track['artists']]
|
||||
tracks.append({
|
||||
'name': track['name'],
|
||||
'artist': ', '.join(artist_names),
|
||||
'duration': track['duration_ms'],
|
||||
'album': track['album']['name'] if track.get('album') else ''
|
||||
})
|
||||
offset += limit
|
||||
|
||||
# Format playlist response
|
||||
playlist_data = {
|
||||
|
||||
@@ -149,12 +149,11 @@
|
||||
<div class="flex items-center mb-3">
|
||||
<i class="fab fa-spotify text-[#1DB954] text-2xl mr-3"></i>
|
||||
<div>
|
||||
<h4 class="font-medium">Spotify</h4>
|
||||
<p class="text-sm text-gray-600">
|
||||
<h4 class="font-medium">Spotify</h4> <p class="text-sm text-gray-600">
|
||||
{% if current_user.spotify_token %}
|
||||
Connected
|
||||
{% if current_user.oauth_id %}
|
||||
as {{ current_user.oauth_id }}
|
||||
{% if current_user.spotify_id %}
|
||||
as {{ current_user.spotify_id }}
|
||||
{% endif %}
|
||||
{% else %}
|
||||
Not connected
|
||||
@@ -363,10 +362,9 @@
|
||||
{% endif %}
|
||||
</td>
|
||||
</tr>
|
||||
{% if current_user.spotify_token %}
|
||||
<tr class="border-b border-gray-200">
|
||||
{% if current_user.spotify_token %} <tr class="border-b border-gray-200">
|
||||
<td class="py-2 font-medium">Spotify ID:</td>
|
||||
<td class="py-2">{{ current_user.oauth_id or 'Unknown' }}</td>
|
||||
<td class="py-2">{{ current_user.spotify_id or 'Unknown' }}</td>
|
||||
</tr>
|
||||
<tr class="border-b border-gray-200">
|
||||
<td class="py-2 font-medium">Token Expiry:</td>
|
||||
|
||||
Reference in New Issue
Block a user