From f71140ae2d2736282d97f55413bfa0143336e985 Mon Sep 17 00:00:00 2001 From: Rikki Tooley Date: Thu, 24 Aug 2017 01:12:10 +0100 Subject: [PATCH] Add RemoveFromCache and GetCachedCount methods to IScrobbler and SQLite implementation - change scrobbler behaviour to remove sucessfully scrobbled tracks from the cache - change return value of Scrobble methods - will always be either Successful, Cached. Actual failure reason may be stored by Scrobbler cache impl - Remove CacheEnabled property from IScrobbler. Just use new MemoryScrobbler class if needed - Remove Scrobbler class, add MemoryScrobbler. No migration path because the behaviour is different - and the old behaviour not that useful --- .../Commands/TrackScrobbleCommandTests.cs | 2 +- .../Scrobblers/ScrobblerTests.cs | 3 +- .../Scrobblers/ScrobblerTestsBase.cs | 38 +++------- src/IF.Lastfm.Core/Api/LastfmClient.cs | 2 +- src/IF.Lastfm.Core/Objects/Scrobble.cs | 42 +++++++++- src/IF.Lastfm.Core/Scrobblers/IScrobbler.cs | 4 +- .../Scrobblers/MemoryScrobbler.cs | 49 ++++++++++++ src/IF.Lastfm.Core/Scrobblers/Scrobbler.cs | 28 ------- .../Scrobblers/ScrobblerBase.cs | 46 ++++++----- .../SQLiteScrobblerTests.cs | 4 +- src/IF.Lastfm.SQLite/SQLiteScrobbler.cs | 76 +++++++++---------- 11 files changed, 164 insertions(+), 130 deletions(-) create mode 100644 src/IF.Lastfm.Core/Scrobblers/MemoryScrobbler.cs delete mode 100644 src/IF.Lastfm.Core/Scrobblers/Scrobbler.cs diff --git a/src/IF.Lastfm.Core.Tests.Integration/Commands/TrackScrobbleCommandTests.cs b/src/IF.Lastfm.Core.Tests.Integration/Commands/TrackScrobbleCommandTests.cs index c492852..61e918a 100644 --- a/src/IF.Lastfm.Core.Tests.Integration/Commands/TrackScrobbleCommandTests.cs +++ b/src/IF.Lastfm.Core.Tests.Integration/Commands/TrackScrobbleCommandTests.cs @@ -71,7 +71,7 @@ public async Task ScrobblesMultiple() var countingHandler = new CountingHttpClientHandler(); var httpClient = new HttpClient(countingHandler); - var scrobbler = new Scrobbler(Lastfm.Auth, httpClient) + var scrobbler = new MemoryScrobbler(Lastfm.Auth, httpClient) { MaxBatchSize = 2 }; diff --git a/src/IF.Lastfm.Core.Tests/Scrobblers/ScrobblerTests.cs b/src/IF.Lastfm.Core.Tests/Scrobblers/ScrobblerTests.cs index bc361f8..74f1f33 100644 --- a/src/IF.Lastfm.Core.Tests/Scrobblers/ScrobblerTests.cs +++ b/src/IF.Lastfm.Core.Tests/Scrobblers/ScrobblerTests.cs @@ -1,6 +1,5 @@ using IF.Lastfm.Core.Scrobblers; using System.Net.Http; -using Scrobbler = IF.Lastfm.Core.Scrobblers.Scrobbler; namespace IF.Lastfm.Core.Tests.Scrobblers { @@ -9,7 +8,7 @@ public class ScrobblerTests : ScrobblerTestsBase protected override ScrobblerBase GetScrobbler() { var httpClient = new HttpClient(FakeResponseHandler); - return new Scrobbler(MockAuth.Object, httpClient); + return new MemoryScrobbler(MockAuth.Object, httpClient); } } } diff --git a/src/IF.Lastfm.Core.Tests/Scrobblers/ScrobblerTestsBase.cs b/src/IF.Lastfm.Core.Tests/Scrobblers/ScrobblerTestsBase.cs index 657ebf8..99e5838 100644 --- a/src/IF.Lastfm.Core.Tests/Scrobblers/ScrobblerTestsBase.cs +++ b/src/IF.Lastfm.Core.Tests/Scrobblers/ScrobblerTestsBase.cs @@ -162,13 +162,7 @@ public async Task ScrobblesExistingCachedTracks() var scrobblesToCache = testScrobbles.Take(1); var scrobbleResponse1 = await ExecuteTestInternal(scrobblesToCache, responseMessage1); - - if (!Scrobbler.CacheEnabled) - { - Assert.AreEqual(LastResponseStatus.BadAuth, scrobbleResponse1.Status); - return; - } - + Assert.AreEqual(LastResponseStatus.Cached, scrobbleResponse1.Status); var scrobblesToSend = testScrobbles.Skip(1).Take(1); @@ -189,18 +183,11 @@ public async Task CorrectResponseWithBadAuth() var responseMessage = TestHelper.CreateResponseMessage(HttpStatusCode.Forbidden, TrackApiResponses.TrackScrobbleBadAuthError); var scrobbleResponse = await ExecuteTestInternal(testScrobbles, responseMessage, requestMessage); - if (Scrobbler.CacheEnabled) - { - Assert.AreEqual(LastResponseStatus.Cached, scrobbleResponse.Status); + Assert.AreEqual(LastResponseStatus.Cached, scrobbleResponse.Status); - // check actually cached - var cached = await Scrobbler.GetCachedAsync(); - TestHelper.AssertSerialiseEqual(testScrobbles, cached); - } - else - { - Assert.AreEqual(LastResponseStatus.BadAuth, scrobbleResponse.Status); - } + // check actually cached + var cached = await Scrobbler.GetCachedAsync(); + TestHelper.AssertSerialiseEqual(testScrobbles, cached); } [Test] @@ -212,18 +199,11 @@ public async Task CorrectResponseWhenRequestFailed() var responseMessage = TestHelper.CreateResponseMessage(HttpStatusCode.RequestTimeout, new byte[0]); var scrobbleResponse = await ExecuteTestInternal(testScrobbles, responseMessage, requestMessage); - if (Scrobbler.CacheEnabled) - { - Assert.AreEqual(LastResponseStatus.Cached, scrobbleResponse.Status); + Assert.AreEqual(LastResponseStatus.Cached, scrobbleResponse.Status); - // check actually cached - var cached = await Scrobbler.GetCachedAsync(); - TestHelper.AssertSerialiseEqual(testScrobbles, cached); - } - else - { - Assert.AreEqual(LastResponseStatus.RequestFailed, scrobbleResponse.Status); - } + // check actually cached + var cached = await Scrobbler.GetCachedAsync(); + TestHelper.AssertSerialiseEqual(testScrobbles, cached); } } } \ No newline at end of file diff --git a/src/IF.Lastfm.Core/Api/LastfmClient.cs b/src/IF.Lastfm.Core/Api/LastfmClient.cs index 10b073c..2300da6 100644 --- a/src/IF.Lastfm.Core/Api/LastfmClient.cs +++ b/src/IF.Lastfm.Core/Api/LastfmClient.cs @@ -25,7 +25,7 @@ public class LastfmClient : ApiBase public ScrobblerBase Scrobbler { - get { return _scrobbler ?? (_scrobbler = new Scrobbler(Auth, HttpClient)); } + get { return _scrobbler ?? (_scrobbler = new MemoryScrobbler(Auth, HttpClient)); } set { _scrobbler = value; } } diff --git a/src/IF.Lastfm.Core/Objects/Scrobble.cs b/src/IF.Lastfm.Core/Objects/Scrobble.cs index 1b90bdc..fdbb4ec 100644 --- a/src/IF.Lastfm.Core/Objects/Scrobble.cs +++ b/src/IF.Lastfm.Core/Objects/Scrobble.cs @@ -1,11 +1,19 @@ using System; using IF.Lastfm.Core.Api.Helpers; using Newtonsoft.Json.Linq; +using System.Collections.Generic; namespace IF.Lastfm.Core.Objects { - public class Scrobble + public class Scrobble : IEquatable { + /// + /// Not part of the Last.fm API. This is a convenience property allowing Scrobbles to have a unique ID. + /// IF.Lastfm.SQLite uses this field to store a primary key, if this Scrobble was cached. + /// Not used in Equals or GetHashCode implementations. + /// + public int Id { get; set; } + public string IgnoredReason { get; private set; } public string Artist { get; private set; } @@ -49,5 +57,37 @@ internal static Scrobble ParseJToken(JToken token) IgnoredReason = ignoredMessage }; } + + public override bool Equals(object obj) + { + return Equals(obj as Scrobble); + } + + public bool Equals(Scrobble other) + { + return other != null && + IgnoredReason == other.IgnoredReason && + Artist == other.Artist && + AlbumArtist == other.AlbumArtist && + Album == other.Album && + Track == other.Track && + TimePlayed.Equals(other.TimePlayed) && + ChosenByUser == other.ChosenByUser && + EqualityComparer.Default.Equals(Duration, other.Duration); + } + + public override int GetHashCode() + { + var hashCode = 417801827; + hashCode = hashCode * -1521134295 + EqualityComparer.Default.GetHashCode(IgnoredReason); + hashCode = hashCode * -1521134295 + EqualityComparer.Default.GetHashCode(Artist); + hashCode = hashCode * -1521134295 + EqualityComparer.Default.GetHashCode(AlbumArtist); + hashCode = hashCode * -1521134295 + EqualityComparer.Default.GetHashCode(Album); + hashCode = hashCode * -1521134295 + EqualityComparer.Default.GetHashCode(Track); + hashCode = hashCode * -1521134295 + EqualityComparer.Default.GetHashCode(TimePlayed); + hashCode = hashCode * -1521134295 + ChosenByUser.GetHashCode(); + hashCode = hashCode * -1521134295 + EqualityComparer.Default.GetHashCode(Duration); + return hashCode; + } } } \ No newline at end of file diff --git a/src/IF.Lastfm.Core/Scrobblers/IScrobbler.cs b/src/IF.Lastfm.Core/Scrobblers/IScrobbler.cs index 4b2086f..f842128 100644 --- a/src/IF.Lastfm.Core/Scrobblers/IScrobbler.cs +++ b/src/IF.Lastfm.Core/Scrobblers/IScrobbler.cs @@ -6,10 +6,8 @@ namespace IF.Lastfm.Core.Scrobblers { public interface IScrobbler { - bool CacheEnabled { get; } - Task> GetCachedAsync(); - + Task ScrobbleAsync(Scrobble scrobble); Task ScrobbleAsync(IEnumerable scrobbles); diff --git a/src/IF.Lastfm.Core/Scrobblers/MemoryScrobbler.cs b/src/IF.Lastfm.Core/Scrobblers/MemoryScrobbler.cs new file mode 100644 index 0000000..aec1da2 --- /dev/null +++ b/src/IF.Lastfm.Core/Scrobblers/MemoryScrobbler.cs @@ -0,0 +1,49 @@ +using System.Collections.Generic; +using System.Linq; +using System.Net.Http; +using System.Threading.Tasks; +using IF.Lastfm.Core.Api; +using IF.Lastfm.Core.Api.Enums; +using IF.Lastfm.Core.Objects; + +namespace IF.Lastfm.Core.Scrobblers +{ + public class MemoryScrobbler : ScrobblerBase + { + private readonly HashSet _scrobbles; + + public MemoryScrobbler(ILastAuth auth, HttpClient httpClient = null) : base(auth, httpClient) + { + _scrobbles = new HashSet(); + } + + public override Task> GetCachedAsync() + { + return Task.FromResult(_scrobbles.AsEnumerable()); + } + + public override Task RemoveFromCacheAsync(ICollection scrobbles) + { + foreach (var scrobble in scrobbles) + { + _scrobbles.Remove(scrobble); + } + + return Task.FromResult(0); + } + + public override Task GetCachedCountAsync() + { + return Task.FromResult(_scrobbles.Count); + } + + protected override Task CacheAsync(IEnumerable scrobbles, LastResponseStatus reason) + { + foreach (var scrobble in scrobbles) + { + _scrobbles.Add(scrobble); + } + return Task.FromResult(LastResponseStatus.Cached); + } + } +} \ No newline at end of file diff --git a/src/IF.Lastfm.Core/Scrobblers/Scrobbler.cs b/src/IF.Lastfm.Core/Scrobblers/Scrobbler.cs deleted file mode 100644 index 1b44333..0000000 --- a/src/IF.Lastfm.Core/Scrobblers/Scrobbler.cs +++ /dev/null @@ -1,28 +0,0 @@ -using System.Collections.Generic; -using System.Linq; -using System.Net.Http; -using System.Threading.Tasks; -using IF.Lastfm.Core.Api; -using IF.Lastfm.Core.Api.Enums; -using IF.Lastfm.Core.Objects; - -namespace IF.Lastfm.Core.Scrobblers -{ - public class Scrobbler : ScrobblerBase - { - public Scrobbler(ILastAuth auth, HttpClient httpClient = null) : base(auth, httpClient) - { - CacheEnabled = false; - } - - public override Task> GetCachedAsync() - { - return Task.FromResult(Enumerable.Empty()); - } - - protected override Task CacheAsync(IEnumerable scrobble, LastResponseStatus originalResponseStatus) - { - return Task.FromResult(originalResponseStatus); - } - } -} \ No newline at end of file diff --git a/src/IF.Lastfm.Core/Scrobblers/ScrobblerBase.cs b/src/IF.Lastfm.Core/Scrobblers/ScrobblerBase.cs index c901229..9bb7fa9 100644 --- a/src/IF.Lastfm.Core/Scrobblers/ScrobblerBase.cs +++ b/src/IF.Lastfm.Core/Scrobblers/ScrobblerBase.cs @@ -4,6 +4,7 @@ using IF.Lastfm.Core.Helpers; using System; using System.Collections.Generic; +using System.Collections.ObjectModel; using System.Linq; using System.Net.Http; using System.Threading.Tasks; @@ -15,8 +16,6 @@ public abstract class ScrobblerBase : ApiBase, IScrobbler { public event EventHandler AfterSend; - public bool CacheEnabled { get; protected set; } - internal int MaxBatchSize { get; set; } protected ScrobblerBase(ILastAuth auth, HttpClient httpClient = null) : base(httpClient) @@ -43,7 +42,7 @@ public Task SendCachedScrobblesAsync() public async Task ScrobbleAsyncInternal(IEnumerable scrobbles) { - var scrobblesList = scrobbles.ToList(); + var scrobblesList = new ReadOnlyCollection(scrobbles.ToList()); var cached = await GetCachedAsync(); var pending = scrobblesList.Concat(cached).OrderBy(s => s.TimePlayed).ToList(); if (!pending.Any()) @@ -64,7 +63,15 @@ public async Task ScrobbleAsyncInternal(IEnumerable try { var response = await command.ExecuteAsync(); - + + var acceptedMap = new HashSet(scrobblesList); + foreach (var ignored in response.Ignored) + { + acceptedMap.Remove(ignored); + } + + await RemoveFromCacheAsync(acceptedMap); + responses.Add(response); } catch (HttpRequestException httpEx) @@ -80,28 +87,15 @@ public async Task ScrobbleAsyncInternal(IEnumerable } else { - try - { - var firstBadResponse = responses.FirstOrDefault(r => !r.Success && r.Status != LastResponseStatus.Unknown); - var originalResponseStatus = firstBadResponse != null - ? firstBadResponse.Status - : LastResponseStatus.RequestFailed; // TODO check httpEx + var firstBadResponse = responses.FirstOrDefault(r => !r.Success && r.Status != LastResponseStatus.Unknown); + var originalResponseStatus = firstBadResponse?.Status ?? LastResponseStatus.RequestFailed; // TODO check httpEx - var cacheStatus = await CacheAsync(scrobblesList, originalResponseStatus); + var cacheStatus = await CacheAsync(scrobblesList, originalResponseStatus); - scrobblerResponse = new ScrobbleResponse(cacheStatus); - } - catch (Exception e) - { - scrobblerResponse = new ScrobbleResponse(LastResponseStatus.CacheFailed) - { - Exception = e - }; - } + scrobblerResponse = new ScrobbleResponse(cacheStatus); } - - var ignoredScrobbles = responses.SelectMany(r => r.Ignored); - scrobblerResponse.Ignored = ignoredScrobbles; + + scrobblerResponse.Ignored = responses.SelectMany(r => r.Ignored); AfterSend?.Invoke(this, scrobblerResponse); @@ -110,6 +104,10 @@ public async Task ScrobbleAsyncInternal(IEnumerable public abstract Task> GetCachedAsync(); - protected abstract Task CacheAsync(IEnumerable scrobble, LastResponseStatus originalResponseStatus); + public abstract Task RemoveFromCacheAsync(ICollection scrobbles); + + public abstract Task GetCachedCountAsync(); + + protected abstract Task CacheAsync(IEnumerable scrobble, LastResponseStatus reason); } } \ No newline at end of file diff --git a/src/IF.Lastfm.SQLite.Tests.Integration/SQLiteScrobblerTests.cs b/src/IF.Lastfm.SQLite.Tests.Integration/SQLiteScrobblerTests.cs index 1ec6fc3..94e832b 100644 --- a/src/IF.Lastfm.SQLite.Tests.Integration/SQLiteScrobblerTests.cs +++ b/src/IF.Lastfm.SQLite.Tests.Integration/SQLiteScrobblerTests.cs @@ -3,6 +3,7 @@ using System.Net.Http; using IF.Lastfm.Core.Scrobblers; using IF.Lastfm.Core.Tests.Scrobblers; +using SQLite; namespace IF.Lastfm.SQLite.Tests.Integration { @@ -12,7 +13,7 @@ public class SQLiteScrobblerTests : ScrobblerTestsBase public override void Initialise() { - var dbPath = Path.GetFullPath("test.db"); + var dbPath = Path.GetFullPath($"test-{DateTime.UtcNow.ToFileTimeUtc()}.db"); File.Delete(dbPath); using (File.Create(dbPath)) { @@ -25,6 +26,7 @@ public override void Initialise() public override void Cleanup() { + SQLiteAsyncConnection.ResetPool(); GC.Collect(); GC.WaitForPendingFinalizers(); File.Delete(_dbPath); diff --git a/src/IF.Lastfm.SQLite/SQLiteScrobbler.cs b/src/IF.Lastfm.SQLite/SQLiteScrobbler.cs index a4d709d..6b23cf8 100644 --- a/src/IF.Lastfm.SQLite/SQLiteScrobbler.cs +++ b/src/IF.Lastfm.SQLite/SQLiteScrobbler.cs @@ -6,67 +6,63 @@ using IF.Lastfm.Core.Api.Enums; using IF.Lastfm.Core.Objects; using IF.Lastfm.Core.Scrobblers; +using Newtonsoft.Json.Converters; using SQLite; namespace IF.Lastfm.SQLite { public class SQLiteScrobbler : ScrobblerBase { - public string DatabasePath { get; private set; } + public string DatabasePath { get; } public SQLiteScrobbler(ILastAuth auth, string databasePath, HttpClient client = null) : base(auth, client) { DatabasePath = databasePath; - - CacheEnabled = true; } - public override Task> GetCachedAsync() + public override async Task> GetCachedAsync() { - using (var db = new SQLiteConnection(DatabasePath, SQLiteOpenFlags.ReadOnly)) - { - var tableInfo = db.GetTableInfo(typeof (Scrobble).Name); - if (!tableInfo.Any()) - { - return Task.FromResult(Enumerable.Empty()); - } - - var cached = db.Query("SELECT * FROM Scrobble"); - db.Close(); - - return Task.FromResult(cached.AsEnumerable()); - } + var db = GetConnection(); + var tableInfo = db.Table(); + var cached = await tableInfo.ToListAsync(); + return cached; } - protected override Task CacheAsync(IEnumerable scrobbles, LastResponseStatus originalResponseStatus) + public override async Task RemoveFromCacheAsync(ICollection scrobbles) { - // TODO cache originalResponse - reason to cache - return Task.Run(() => + var db = GetConnection(); + await db.RunInTransactionAsync(connection => { - Cache(scrobbles); - return LastResponseStatus.Cached; - }); - } - - private void Cache(IEnumerable scrobbles) - { - using (var db = new SQLiteConnection(DatabasePath, SQLiteOpenFlags.ReadWrite)) - { - var tableInfo = db.GetTableInfo(typeof (Scrobble).Name); - if (!tableInfo.Any()) - { - db.CreateTable(); - } - - db.BeginTransaction(); foreach (var scrobble in scrobbles) { - db.Insert(scrobble); + connection.Delete(scrobble); } - db.Commit(); + }); - db.Close(); - } + await Task.WhenAll(scrobbles.Select(s => db.DeleteAsync(s)).ToArray()); + } + + public override async Task GetCachedCountAsync() + { + var db = GetConnection(); + var tableInfo = db.Table(); + var count = await tableInfo.CountAsync(); + return count; + } + + protected override async Task CacheAsync(IEnumerable scrobbles, LastResponseStatus reason) + { + // TODO cache reason + var db = GetConnection(); + await db.InsertAllAsync(scrobbles); + return LastResponseStatus.Cached; + } + + private SQLiteAsyncConnection GetConnection() + { + var db = new SQLiteAsyncConnection(DatabasePath, SQLiteOpenFlags.ReadWrite); + db.GetConnection().CreateTable(CreateFlags.AutoIncPK | CreateFlags.AllImplicit); + return db; } } }