From 03a2aa298750106c603413652e853bdb49f3c231 Mon Sep 17 00:00:00 2001 From: droideparanoico Date: Wed, 8 Jul 2026 15:51:26 +0200 Subject: [PATCH] Address all code review issues - XSS: use hx-confirm instead of onclick confirm() - Feed: per-artist type filtering in SQL with LIMIT/OFFSET - Cache: batch staleness check (single query) - Monitoring: null-guard + constant-first equals - REST stack: switch to reactive (quarkus-rest) - User-Agent: fix double https, update version - Settings: validate cache TTL before saving - Delete: transactional cascade - Docker: remove unused sqlite package - Explicit columns in findAllArtists --- Dockerfile | 1 - pom.xml | 10 +-- .../java/com/discdrop/db/DatabaseService.java | 89 ++++++++++++++++--- .../com/discdrop/service/ArtistService.java | 3 +- .../com/discdrop/service/CacheService.java | 12 ++- .../com/discdrop/service/FeedService.java | 60 +++++-------- .../com/discdrop/web/ArtistController.java | 13 +-- .../com/discdrop/web/SettingsController.java | 12 +++ src/main/resources/application.properties | 2 +- .../templates/fragments/artist-list.html | 2 +- 10 files changed, 136 insertions(+), 68 deletions(-) diff --git a/Dockerfile b/Dockerfile index ae2545c..a568589 100644 --- a/Dockerfile +++ b/Dockerfile @@ -11,7 +11,6 @@ RUN mvn package -q -DskipTests # ─── 2. Runtime ─────────────────────────────────── FROM eclipse-temurin:21-jre-alpine WORKDIR /app -RUN apk add --no-cache sqlite COPY --from=build /build/target/quarkus-app/ /app/ EXPOSE 8080 diff --git a/pom.xml b/pom.xml index 256a632..6b4c0eb 100644 --- a/pom.xml +++ b/pom.xml @@ -30,22 +30,22 @@ - + io.quarkus - quarkus-resteasy + quarkus-rest io.quarkus - quarkus-resteasy-qute + quarkus-rest-qute io.quarkus - quarkus-resteasy-client + quarkus-rest-client @@ -75,7 +75,7 @@ io.quarkus - quarkus-resteasy-client-jackson + quarkus-rest-client-jackson diff --git a/src/main/java/com/discdrop/db/DatabaseService.java b/src/main/java/com/discdrop/db/DatabaseService.java index bad87dc..510cca5 100644 --- a/src/main/java/com/discdrop/db/DatabaseService.java +++ b/src/main/java/com/discdrop/db/DatabaseService.java @@ -7,6 +7,7 @@ import java.sql.*; import java.time.Instant; import java.util.ArrayList; import java.util.List; +import java.util.Map; import java.util.Optional; import org.sqlite.SQLiteDataSource; import jakarta.annotation.PostConstruct; @@ -84,7 +85,7 @@ public class DatabaseService { public List findAllArtists() { var artists = new ArrayList(); - var sql = "SELECT * FROM tracked_artist ORDER BY name"; + var sql = "SELECT mbid, name, disambiguation, area_name, album_monitored, single_monitored, ep_monitored, broadcast_monitored, other_monitored, created_at, updated_at FROM tracked_artist ORDER BY name"; try (Connection conn = getConnection(); Statement stmt = conn.createStatement(); ResultSet rs = stmt.executeQuery(sql)) { @@ -184,6 +185,29 @@ public class DatabaseService { } } + public void deleteArtistCascading(String mbid) { + var sqlRg = "DELETE FROM release_group_cache WHERE artist_mbid = ?"; + var sqlAr = "DELETE FROM tracked_artist WHERE mbid = ?"; + try (Connection conn = getConnection()) { + conn.setAutoCommit(false); + try (PreparedStatement ps1 = conn.prepareStatement(sqlRg); + PreparedStatement ps2 = conn.prepareStatement(sqlAr)) { + ps1.setString(1, mbid); + ps1.executeUpdate(); + ps2.setString(1, mbid); + ps2.executeUpdate(); + conn.commit(); + } catch (SQLException e) { + conn.rollback(); + throw e; + } finally { + conn.setAutoCommit(true); + } + } catch (SQLException e) { + throw new RuntimeException(e); + } + } + // --- release_group_cache DAO --- public void upsertReleaseGroups(List groups) { @@ -213,16 +237,24 @@ public class DatabaseService { public record FeedResult(List entries, boolean hasMore) {} - public FeedResult getFeed(List artistMbids, List primaryTypes, int limit, int offset) { + public FeedResult getFeed(Map> artistTypes, int limit, int offset) { var groups = new ArrayList(); - if (artistMbids.isEmpty()) return new FeedResult(groups, false); + if (artistTypes.isEmpty()) return new FeedResult(groups, false); - var placeholders = String.join(",", artistMbids.stream().map(m -> "?").toList()); - var sql = "SELECT * FROM release_group_cache WHERE artist_mbid IN (" + placeholders + ")"; - if (!primaryTypes.isEmpty()) { - var typePlaceholders = String.join(",", primaryTypes.stream().map(t -> "?").toList()); - sql += " AND primary_type IN (" + typePlaceholders + ")"; + var conditions = new ArrayList(); + var params = new ArrayList(); + for (var entry : artistTypes.entrySet()) { + var types = entry.getValue(); + if (types.isEmpty()) continue; + var placeholders = String.join(",", types.stream().map(t -> "?").toList()); + conditions.add("(artist_mbid = ? AND primary_type IN (" + placeholders + "))"); + params.add(entry.getKey()); + params.addAll(types); } + + if (conditions.isEmpty()) return new FeedResult(groups, false); + + var sql = "SELECT * FROM release_group_cache WHERE " + String.join(" OR ", conditions); sql += " ORDER BY (CASE WHEN first_release_date IS NULL OR first_release_date = '' THEN 1 ELSE 0 END), first_release_date DESC"; boolean useLimit = limit > 0; @@ -231,8 +263,10 @@ public class DatabaseService { try (Connection conn = getConnection(); PreparedStatement ps = conn.prepareStatement(sql)) { int idx = 1; - for (var mbid : artistMbids) ps.setString(idx++, mbid); - for (var type : primaryTypes) ps.setString(idx++, type); + for (var p : params) { + if (p instanceof String s) ps.setString(idx++, s); + else ps.setObject(idx++, p); + } if (useLimit) { ps.setInt(idx++, limit + 1); ps.setInt(idx, offset); @@ -291,6 +325,41 @@ public class DatabaseService { return Optional.empty(); } + public java.util.Map getStaleness(List artistMbids, long ttlHours) { + var result = new java.util.LinkedHashMap(); + if (artistMbids.isEmpty()) return result; + + var placeholders = String.join(",", artistMbids.stream().map(m -> "?").toList()); + var sql = "SELECT artist_mbid, COUNT(*) as cnt, MAX(cached_at) as last_cached FROM release_group_cache WHERE artist_mbid IN (" + placeholders + ") GROUP BY artist_mbid"; + try (Connection conn = getConnection(); + PreparedStatement ps = conn.prepareStatement(sql)) { + int idx = 1; + for (var mbid : artistMbids) ps.setString(idx++, mbid); + try (ResultSet rs = ps.executeQuery()) { + var now = Instant.now(); + var cached = new java.util.HashMap(); + var lastCached = new java.util.HashMap(); + while (rs.next()) { + var mbid = rs.getString("artist_mbid"); + cached.put(mbid, rs.getLong("cnt")); + var ts = rs.getString("last_cached"); + if (ts != null) { + lastCached.put(mbid, Instant.parse(ts.replace(" ", "T") + "Z")); + } + } + for (var mbid : artistMbids) { + var cnt = cached.getOrDefault(mbid, 0L); + var lc = lastCached.get(mbid); + boolean stale = cnt == 0 || lc == null || lc.plus(ttlHours, java.time.temporal.ChronoUnit.HOURS).isBefore(now); + result.put(mbid, stale); + } + } + } catch (SQLException e) { + throw new RuntimeException(e); + } + return result; + } + // --- settings DAO --- public String getSetting(String key, String defaultValue) { diff --git a/src/main/java/com/discdrop/service/ArtistService.java b/src/main/java/com/discdrop/service/ArtistService.java index fde16b6..e83d0da 100644 --- a/src/main/java/com/discdrop/service/ArtistService.java +++ b/src/main/java/com/discdrop/service/ArtistService.java @@ -84,8 +84,7 @@ public class ArtistService { } public void removeArtist(String mbid) { - db.deleteReleaseGroupsForArtist(mbid); - db.deleteArtist(mbid); + db.deleteArtistCascading(mbid); } public void updateMonitoring(String mbid, MonitoringFlags flags) { diff --git a/src/main/java/com/discdrop/service/CacheService.java b/src/main/java/com/discdrop/service/CacheService.java index 57aec51..ec2d175 100644 --- a/src/main/java/com/discdrop/service/CacheService.java +++ b/src/main/java/com/discdrop/service/CacheService.java @@ -5,7 +5,10 @@ import jakarta.enterprise.context.ApplicationScoped; import jakarta.inject.Inject; import java.time.Instant; import java.time.temporal.ChronoUnit; +import java.util.List; +import java.util.Map; import java.util.Optional; +import java.util.stream.Collectors; @ApplicationScoped public class CacheService { @@ -25,12 +28,15 @@ public class CacheService { public boolean isStale(String artistMbid) { Optional lastCached = db.getLastCachedAt(artistMbid); if (lastCached.isEmpty()) return true; - - long ttlHours = getTtlHours(); - return lastCached.get().plus(ttlHours, ChronoUnit.HOURS).isBefore(Instant.now()); + return lastCached.get().plus(getTtlHours(), ChronoUnit.HOURS).isBefore(Instant.now()); } public boolean needsRefresh(String artistMbid) { return db.countReleaseGroupsForArtist(artistMbid) == 0 || isStale(artistMbid); } + + public Map needsRefresh(List artistMbids) { + var staleness = db.getStaleness(artistMbids, getTtlHours()); + return staleness; + } } diff --git a/src/main/java/com/discdrop/service/FeedService.java b/src/main/java/com/discdrop/service/FeedService.java index ef22d9e..b8fef28 100644 --- a/src/main/java/com/discdrop/service/FeedService.java +++ b/src/main/java/com/discdrop/service/FeedService.java @@ -6,8 +6,7 @@ import com.discdrop.db.Record.ReleaseGroup; import com.discdrop.db.Record.TrackedArtist; import jakarta.enterprise.context.ApplicationScoped; import jakarta.inject.Inject; -import java.util.ArrayList; -import java.util.List; +import java.util.*; @ApplicationScoped public class FeedService { @@ -29,55 +28,36 @@ public class FeedService { public List getAllFeedEntries() { var artists = artistService.findAll(); if (artists.isEmpty()) return List.of(); - - var artistMbids = artistMbidsWithRefresh(artists); - var allTypes = List.of("Album", "Single", "EP", "Broadcast", "Other"); - var allEntries = db.getFeed(artistMbids, allTypes, 0, 0).entries(); - return filterByArtistMonitoring(allEntries, artists); + refreshStaleCaches(artists); + return db.getFeed(buildArtistTypesMap(artists), 0, 0).entries(); } public FeedResult getFeedEntries(int page) { var artists = artistService.findAll(); if (artists.isEmpty()) return new FeedResult(List.of(), false); - - var artistMbids = artistMbidsWithRefresh(artists); - var allTypes = List.of("Album", "Single", "EP", "Broadcast", "Other"); - var allEntries = db.getFeed(artistMbids, allTypes, 0, 0).entries(); - var filtered = filterByArtistMonitoring(allEntries, artists); - - var fromIndex = (page - 1) * PAGE_SIZE; - if (fromIndex >= filtered.size()) return new FeedResult(List.of(), false); - var toIndex = Math.min(fromIndex + PAGE_SIZE, filtered.size()); - var pageEntries = filtered.subList(fromIndex, toIndex); - var hasMore = toIndex < filtered.size(); - return new FeedResult(pageEntries, hasMore); + refreshStaleCaches(artists); + var offset = (page - 1) * PAGE_SIZE; + return db.getFeed(buildArtistTypesMap(artists), PAGE_SIZE, offset); } - private List artistMbidsWithRefresh(List artists) { - var allTypes = List.of("Album", "Single", "EP", "Broadcast", "Other"); - var mbids = new ArrayList(); + private Map> buildArtistTypesMap(List artists) { + var map = new LinkedHashMap>(); for (var a : artists) { - if (cacheService.needsRefresh(a.mbid())) { + var types = getMonitoredTypes(a); + if (!types.isEmpty()) map.put(a.mbid(), types); + } + return map; + } + + private void refreshStaleCaches(List artists) { + var allTypes = List.of("Album", "Single", "EP", "Broadcast", "Other"); + var mbids = artists.stream().map(TrackedArtist::mbid).toList(); + var staleness = cacheService.needsRefresh(mbids); + for (var a : artists) { + if (staleness.getOrDefault(a.mbid(), false)) { releaseGroupService.fetchAndCache(a.mbid(), a.name(), allTypes); } - mbids.add(a.mbid()); } - return mbids; - } - - private List filterByArtistMonitoring(List entries, List artists) { - var flags = new java.util.HashMap>(); - for (var a : artists) { - flags.put(a.mbid(), java.util.Set.copyOf(getMonitoredTypes(a))); - } - var result = new ArrayList(); - for (var e : entries) { - var artistFlags = flags.get(e.artistMbid()); - if (artistFlags != null && artistFlags.contains(e.primaryType())) { - result.add(e); - } - } - return result; } private List getMonitoredTypes(TrackedArtist a) { diff --git a/src/main/java/com/discdrop/web/ArtistController.java b/src/main/java/com/discdrop/web/ArtistController.java index 71b9ef5..0dda8ac 100644 --- a/src/main/java/com/discdrop/web/ArtistController.java +++ b/src/main/java/com/discdrop/web/ArtistController.java @@ -103,12 +103,15 @@ public class ArtistController { @QueryParam("type") String type) { var existing = artistService.findByMbid(mbid) .orElseThrow(() -> new NotFoundException("Artist not found")); + if (type == null || type.isBlank()) { + throw new jakarta.ws.rs.BadRequestException("type parameter is required"); + } var flags = new MonitoringFlags( - type.equals("album") ? !existing.albumMonitored() : existing.albumMonitored(), - type.equals("single") ? !existing.singleMonitored() : existing.singleMonitored(), - type.equals("ep") ? !existing.epMonitored() : existing.epMonitored(), - type.equals("broadcast") ? !existing.broadcastMonitored() : existing.broadcastMonitored(), - type.equals("other") ? !existing.otherMonitored() : existing.otherMonitored() + "album".equals(type) ? !existing.albumMonitored() : existing.albumMonitored(), + "single".equals(type) ? !existing.singleMonitored() : existing.singleMonitored(), + "ep".equals(type) ? !existing.epMonitored() : existing.epMonitored(), + "broadcast".equals(type) ? !existing.broadcastMonitored() : existing.broadcastMonitored(), + "other".equals(type) ? !existing.otherMonitored() : existing.otherMonitored() ); artistService.updateMonitoring(mbid, flags); var allArtists = artistService.findAll(); diff --git a/src/main/java/com/discdrop/web/SettingsController.java b/src/main/java/com/discdrop/web/SettingsController.java index 63d61de..69ed8d2 100644 --- a/src/main/java/com/discdrop/web/SettingsController.java +++ b/src/main/java/com/discdrop/web/SettingsController.java @@ -36,6 +36,18 @@ public class SettingsController { public Response saveSettings( @FormParam("cache_ttl_hours") @DefaultValue("8") String cacheTtl, @FormParam("default_types") List defaultTypes) { + try { + var ttl = Integer.parseInt(cacheTtl); + if (ttl < 1 || ttl > 168) { + return Response.status(400) + .entity("
Cache TTL must be between 1 and 168 hours
") + .build(); + } + } catch (NumberFormatException e) { + return Response.status(400) + .entity("
Invalid cache TTL value
") + .build(); + } db.setSetting("cache_ttl_hours", cacheTtl); db.setSetting("default_types", defaultTypes != null ? String.join(",", defaultTypes) : "Album"); return Response.ok("
Settings saved
") diff --git a/src/main/resources/application.properties b/src/main/resources/application.properties index 8bb1f2f..1571f35 100644 --- a/src/main/resources/application.properties +++ b/src/main/resources/application.properties @@ -17,4 +17,4 @@ discdrop.cache.ttl-hours=${DISC_DROP_CACHE_TTL:8} discdrop.base-url=${DISC_DROP_BASE_URL:http://localhost:8080} # User-Agent sent to MusicBrainz API (env: DISC_DROP_USER_AGENT) -discdrop.user-agent=${DISC_DROP_USER_AGENT:DiscDrop/0.1 (https://https://github.com/droideparanoico/discdrop)} +discdrop.user-agent=${DISC_DROP_USER_AGENT:DiscDrop/1.0 (https://github.com/droideparanoico/discdrop)} diff --git a/src/main/resources/templates/fragments/artist-list.html b/src/main/resources/templates/fragments/artist-list.html index 0c09fac..f5d42f9 100644 --- a/src/main/resources/templates/fragments/artist-list.html +++ b/src/main/resources/templates/fragments/artist-list.html @@ -77,7 +77,7 @@ hx-delete="/api/artists/{artist.mbid}" hx-target="#artist-list" hx-swap="innerHTML" - onclick="return confirm('Remove {artist.name}?')"> + hx-confirm="Remove {artist.name}?">