Merge pull request #181 from FoxxMD/malojaDuplicateHandling

fix(maloja): Improve handling for Maloja warnings-as-errors
FoxxMD · Aug 27, 2024 · 8369939 · 8369939
2 parents ce95c3a + 08056a6
commit 8369939
Show file tree

Hide file tree

Showing 5 changed files with 88 additions and 50 deletions.
diff --git a/src/backend/common/vendor/maloja/interfaces.ts b/src/backend/common/vendor/maloja/interfaces.ts
@@ -74,7 +74,7 @@ export interface MalojaScrobbleV3RequestData extends MalojaScrobbleRequestData {
     nofix?: boolean
 }
 
-interface MalojaScrobbleWarning {
+export interface MalojaScrobbleWarning {
     type: string
     value: string[] | string
     desc: string

diff --git a/src/backend/scrobblers/AbstractScrobbleClient.ts b/src/backend/scrobblers/AbstractScrobbleClient.ts
@@ -153,6 +153,7 @@ export default abstract class AbstractScrobbleClient extends AbstractComponent i
             this.logger.debug('Refreshing recent scrobbles');
             const recent = await this.getScrobblesForRefresh(limit);
             this.logger.debug(`Found ${recent.length} recent scrobbles`);
+            this.recentScrobbles = recent;
             if (this.recentScrobbles.length > 0) {
                 const [{data: {playDate: newestScrobbleTime = dayjs()} = {}} = {}] = this.recentScrobbles.slice(-1);
                 const [{data: {playDate: oldestScrobbleTime = dayjs()} = {}} = {}] = this.recentScrobbles.slice(0, 1);

diff --git a/src/backend/scrobblers/MalojaScrobbler.ts b/src/backend/scrobblers/MalojaScrobbler.ts
@@ -19,7 +19,7 @@ import {
     MalojaScrobbleRequestData,
     MalojaScrobbleV2RequestData,
     MalojaScrobbleV3RequestData,
-    MalojaScrobbleV3ResponseData,
+    MalojaScrobbleV3ResponseData, MalojaScrobbleWarning,
     MalojaV2ScrobbleData,
     MalojaV3ScrobbleData,
 } from "../common/vendor/maloja/interfaces.js";
@@ -59,7 +59,7 @@ export default class MalojaScrobbler extends AbstractScrobbleClient {
                 // when the track was scrobbled
                 time: mTime,
                 track: {
-                    artists: mArtists,
+                    artists: mArtists = [],
                     title: mTitle,
                     album: mAlbum,
                     // length of the track
@@ -77,14 +77,14 @@ export default class MalojaScrobbler extends AbstractScrobbleClient {
                 const {
                     albumtitle,
                     name: mAlbumName,
-                    artists: albumArtists
+                    artists: albumArtists = []
                 } = mAlbum || {};
                 album = albumtitle ?? mAlbumName;
             }
         } else {
             // scrobble data structure for v2 and below
             const {
-                artists: mArtists,
+                artists: mArtists = [],
                 title: mTitle,
                 album: mAlbum,
                 duration: mDuration,
@@ -438,7 +438,11 @@ export default class MalojaScrobbler extends AbstractScrobbleClient {
                     }
                     if(warnings.length > 0) {
                         for(const w of warnings) {
-                            this.logger.warn(`Maloja Warning: ${w.desc} => ${JSON.stringify(w.value)}`)
+                            const warnStr = buildWarningString(w);
+                            if(warnStr.includes('The submitted scrobble was not added')) {
+                                throw new UpstreamError(`Maloja returned a warning but MS treating as error: ${warnStr}`, {showStopper: false});
+                            }
+                            this.logger.warn(`Maloja Warning: ${warnStr}`);
                         }
                     }
                 } else {
@@ -517,3 +521,19 @@ const buildErrorString = (body: MalojaResponseV3CommonData) => {
     }
     return `Maloja API returned ${status} of type ${type} "${desc}"${valString !== undefined ? `: ${valString}` : ''}`;
 }
+
+const buildWarningString = (w: MalojaScrobbleWarning): string => {
+    const parts: string[] = [`${typeof w.type === 'string' ? `(${w.type}) ` : ''}${w.desc ?? ''}`];
+    let vals: string[] = [];
+    if(w.value !== null && w.value !== undefined) {
+        if(Array.isArray(w.value)) {
+            vals = w.value;
+        } else {
+            vals.push(w.value);
+        }
+    }
+    if(vals.length > 0) {
+        parts.push(vals.join(' | '));
+    }
+    return parts.join(' => ');
+}
diff --git a/src/backend/tests/scrobbler/TestScrobbler.ts b/src/backend/tests/scrobbler/TestScrobbler.ts
@@ -6,16 +6,19 @@ import { Notifiers } from "../../notifier/Notifiers.js";
 import AbstractScrobbleClient from "../../scrobblers/AbstractScrobbleClient.js";
 
 export class TestScrobbler extends AbstractScrobbleClient {
-    protected async getScrobblesForRefresh(limit: number): Promise<PlayObject[]> {
-        return [];
-    }
+
+    testRecentScrobbles: PlayObject[] = [];
 
     constructor() {
         const logger = loggerTest;
         const notifier = new Notifiers(new EventEmitter(), new EventEmitter(), new EventEmitter(), logger);
         super('test', 'Test', {name: 'test'}, notifier, new EventEmitter(), logger);
     }
 
+    protected async getScrobblesForRefresh(limit: number): Promise<PlayObject[]> {
+        return this.testRecentScrobbles;
+    }
+
     doScrobble(playObj: PlayObject): Promise<PlayObject> {
         return Promise.resolve(playObj);
     }

diff --git a/src/backend/tests/scrobbler/scrobblers.test.ts b/src/backend/tests/scrobbler/scrobblers.test.ts
@@ -458,66 +458,80 @@ describe('Detects duplicate and unique scrobbles using actively tracked scrobble
     });
 });
 
-describe('Detects when upstream scrobbles should be refreshed', function() {
+describe('Upstream Scrobbles', function() {
+
+    it('Stores upstream scrobbles on refresh', async function () {
+        const scrobbler = generateTestScrobbler();
+        scrobbler.testRecentScrobbles = normalizedWithMixedDur;
+        assert.isEmpty(scrobbler.recentScrobbles);
+        await scrobbler.refreshScrobbles();
+        assert.isNotEmpty(scrobbler.recentScrobbles);
+    });
 
-    const normalizedClose = normalizePlays(withDurPlays, {initialDate: dayjs().subtract(100, 'seconds')});
+    describe('Detects when upstream scrobbles should be refreshed', function() {
 
-    beforeEach(function () {
-        testScrobbler = generateTestScrobbler();
-        testScrobbler.recentScrobbles = normalizedWithMixedDur;
-        testScrobbler.newestScrobbleTime = normalizedWithMixedDur[0].data.playDate;
-        testScrobbler.lastScrobbleCheck = dayjs().subtract(60, 'seconds');
-        testScrobbler.queuedScrobbles = [];
-        testScrobbler.config.options = {};
-    });
+        const normalizedClose = normalizePlays(withDurPlays, {initialDate: dayjs().subtract(100, 'seconds')});
 
-    it('Detects queued scrobble date is newer than last scrobble refresh', async function() {
-        const newScrobble = generatePlay({
-            playDate: dayjs()
+        beforeEach(function () {
+            testScrobbler = generateTestScrobbler();
+            testScrobbler.recentScrobbles = normalizedWithMixedDur;
+            testScrobbler.newestScrobbleTime = normalizedWithMixedDur[0].data.playDate;
+            testScrobbler.lastScrobbleCheck = dayjs().subtract(60, 'seconds');
+            testScrobbler.queuedScrobbles = [];
+            testScrobbler.config.options = {};
         });
 
-        testScrobbler.queueScrobble(newScrobble, 'test');
-        assert.isTrue(testScrobbler.shouldRefreshScrobble());
-    });
-
-    it('Detects queued scrobble date is older than newest scrobble', async function() {
-        testScrobbler.recentScrobbles = normalizedClose;
-        testScrobbler.newestScrobbleTime = normalizedClose[0].data.playDate;
+        it('Detects queued scrobble date is newer than last scrobble refresh', async function() {
+            const newScrobble = generatePlay({
+                playDate: dayjs()
+            });
 
-        const newScrobble = generatePlay({
-            playDate: dayjs().subtract(120, 'seconds')
+            testScrobbler.queueScrobble(newScrobble, 'test');
+            assert.isTrue(testScrobbler.shouldRefreshScrobble());
         });
 
-        testScrobbler.queueScrobble(newScrobble, 'test');
-        assert.isTrue(testScrobbler.shouldRefreshScrobble());
-    });
+        it('Detects queued scrobble date is older than newest scrobble', async function() {
+            testScrobbler.recentScrobbles = normalizedClose;
+            testScrobbler.newestScrobbleTime = normalizedClose[0].data.playDate;
 
-    it('Forces refresh if refreshStaleAfter is set', async function() {
-        testScrobbler.recentScrobbles = normalizedClose;
-        testScrobbler.newestScrobbleTime = normalizedClose[0].data.playDate;
-        testScrobbler.config.options = { refreshStaleAfter: 10 };
+            const newScrobble = generatePlay({
+                playDate: dayjs().subtract(120, 'seconds')
+            });
 
-        const newScrobble = generatePlay({
-            playDate: dayjs().subtract(80, 'seconds')
+            testScrobbler.queueScrobble(newScrobble, 'test');
+            assert.isTrue(testScrobbler.shouldRefreshScrobble());
         });
 
-        testScrobbler.queueScrobble(newScrobble, 'test');
-        assert.isTrue(testScrobbler.shouldRefreshScrobble());
-    });
+        it('Forces refresh if refreshStaleAfter is set', async function() {
+            testScrobbler.recentScrobbles = normalizedClose;
+            testScrobbler.newestScrobbleTime = normalizedClose[0].data.playDate;
+            testScrobbler.config.options = { refreshStaleAfter: 10 };
 
-    it('Does not refresh if scrobble is older than last check but newer than newest upstream scrobble', async function() {
-        testScrobbler.recentScrobbles = normalizedClose;
-        testScrobbler.newestScrobbleTime = normalizedClose[0].data.playDate;
+            const newScrobble = generatePlay({
+                playDate: dayjs().subtract(80, 'seconds')
+            });
 
-        const newScrobble = generatePlay({
-            playDate: dayjs().subtract(80, 'seconds')
+            testScrobbler.queueScrobble(newScrobble, 'test');
+            assert.isTrue(testScrobbler.shouldRefreshScrobble());
         });
 
-        testScrobbler.queueScrobble(newScrobble, 'test');
-        assert.isFalse(testScrobbler.shouldRefreshScrobble());
+        it('Does not refresh if scrobble is older than last check but newer than newest upstream scrobble', async function() {
+            testScrobbler.recentScrobbles = normalizedClose;
+            testScrobbler.newestScrobbleTime = normalizedClose[0].data.playDate;
+
+            const newScrobble = generatePlay({
+                playDate: dayjs().subtract(80, 'seconds')
+            });
+
+            testScrobbler.queueScrobble(newScrobble, 'test');
+            assert.isFalse(testScrobbler.shouldRefreshScrobble());
+        });
     });
+
 });
 
+
+
 describe('Scrobble client uses transform plays correctly', function() {
 
     beforeEach(async function() {