Address all code review issues
Build & Deploy / build (push) Successful in 55s

- 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
This commit is contained in:
2026-07-08 15:51:26 +02:00
parent ba82c0b6ff
commit 03a2aa2987
10 changed files with 136 additions and 68 deletions
-1
View File
@@ -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
+5 -5
View File
@@ -30,22 +30,22 @@
</dependencyManagement>
<dependencies>
<!-- REST server (resteasy-reactive successor) -->
<!-- REST server (reactive) -->
<dependency>
<groupId>io.quarkus</groupId>
<artifactId>quarkus-resteasy</artifactId>
<artifactId>quarkus-rest</artifactId>
</dependency>
<!-- Qute templating -->
<dependency>
<groupId>io.quarkus</groupId>
<artifactId>quarkus-resteasy-qute</artifactId>
<artifactId>quarkus-rest-qute</artifactId>
</dependency>
<!-- REST Client -->
<dependency>
<groupId>io.quarkus</groupId>
<artifactId>quarkus-resteasy-client</artifactId>
<artifactId>quarkus-rest-client</artifactId>
</dependency>
<!-- CDI -->
@@ -75,7 +75,7 @@
</dependency>
<dependency>
<groupId>io.quarkus</groupId>
<artifactId>quarkus-resteasy-client-jackson</artifactId>
<artifactId>quarkus-rest-client-jackson</artifactId>
</dependency>
<!-- Test -->
@@ -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<Record.TrackedArtist> findAllArtists() {
var artists = new ArrayList<Record.TrackedArtist>();
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<Record.ReleaseGroup> groups) {
@@ -213,16 +237,24 @@ public class DatabaseService {
public record FeedResult(List<Record.ReleaseGroup> entries, boolean hasMore) {}
public FeedResult getFeed(List<String> artistMbids, List<String> primaryTypes, int limit, int offset) {
public FeedResult getFeed(Map<String, List<String>> artistTypes, int limit, int offset) {
var groups = new ArrayList<Record.ReleaseGroup>();
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<String>();
var params = new ArrayList<Object>();
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<String, Boolean> getStaleness(List<String> artistMbids, long ttlHours) {
var result = new java.util.LinkedHashMap<String, Boolean>();
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<String, Long>();
var lastCached = new java.util.HashMap<String, Instant>();
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) {
@@ -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) {
@@ -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<Instant> 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<String, Boolean> needsRefresh(List<String> artistMbids) {
var staleness = db.getStaleness(artistMbids, getTtlHours());
return staleness;
}
}
@@ -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<ReleaseGroup> 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<String> artistMbidsWithRefresh(List<TrackedArtist> artists) {
var allTypes = List.of("Album", "Single", "EP", "Broadcast", "Other");
var mbids = new ArrayList<String>();
private Map<String, List<String>> buildArtistTypesMap(List<TrackedArtist> artists) {
var map = new LinkedHashMap<String, List<String>>();
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<TrackedArtist> 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<ReleaseGroup> filterByArtistMonitoring(List<ReleaseGroup> entries, List<TrackedArtist> artists) {
var flags = new java.util.HashMap<String, java.util.Set<String>>();
for (var a : artists) {
flags.put(a.mbid(), java.util.Set.copyOf(getMonitoredTypes(a)));
}
var result = new ArrayList<ReleaseGroup>();
for (var e : entries) {
var artistFlags = flags.get(e.artistMbid());
if (artistFlags != null && artistFlags.contains(e.primaryType())) {
result.add(e);
}
}
return result;
}
private List<String> getMonitoredTypes(TrackedArtist a) {
@@ -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();
@@ -36,6 +36,18 @@ public class SettingsController {
public Response saveSettings(
@FormParam("cache_ttl_hours") @DefaultValue("8") String cacheTtl,
@FormParam("default_types") List<String> defaultTypes) {
try {
var ttl = Integer.parseInt(cacheTtl);
if (ttl < 1 || ttl > 168) {
return Response.status(400)
.entity("<div class=\"alert alert-error\">Cache TTL must be between 1 and 168 hours</div>")
.build();
}
} catch (NumberFormatException e) {
return Response.status(400)
.entity("<div class=\"alert alert-error\">Invalid cache TTL value</div>")
.build();
}
db.setSetting("cache_ttl_hours", cacheTtl);
db.setSetting("default_types", defaultTypes != null ? String.join(",", defaultTypes) : "Album");
return Response.ok("<div class=\"alert alert-success\">Settings saved</div>")
+1 -1
View File
@@ -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)}
@@ -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}?">
<svg class="icon-sm" viewBox="0 0 24 24" fill="none" stroke="currentColor" stroke-width="2"><polyline points="3 6 5 6 21 6"/><path d="M19 6v14a2 2 0 01-2 2H7a2 2 0 01-2-2V6m3 0V4a2 2 0 012-2h4a2 2 0 012 2v2"/></svg>
</button>
</td>