From 5ca3437b5945a9dc2364169d62be5fab8462874f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Julian=20T=C3=B6lle?= Date: Sat, 28 Nov 2020 17:55:19 +0100 Subject: [PATCH] fix(api): missed listens from spotify Listory could miss some listens when the Spotify Api misbehaves. Sometimes the listens added to the last-recently-played endpoint are showing up are out-of-order. Because of our optimization to only retrieve listens newer than the lastRefreshTime we would skip retrieving those out-of-order listens. By always retrieving the maximum 50 listens we can be reasonably sure that we got all the listens. We also had to improve the handling of duplicate listens, as we now have a lot of them, curtesy of removing the lastRefreshTime optimization. --- src/listens/dto/create-listen.dto.ts | 10 ++++- src/listens/listen.repository.ts | 39 ++++++++++++++++++- src/listens/listens.service.ts | 32 ++++++--------- .../spotify-api/spotify-api.service.ts | 4 -- src/sources/spotify/spotify.service.ts | 34 +++++++++------- 5 files changed, 76 insertions(+), 43 deletions(-) diff --git a/src/listens/dto/create-listen.dto.ts b/src/listens/dto/create-listen.dto.ts index ba54fb2..7d731f5 100644 --- a/src/listens/dto/create-listen.dto.ts +++ b/src/listens/dto/create-listen.dto.ts @@ -1,8 +1,16 @@ import { Track } from "../../music-library/track.entity"; import { User } from "../../users/user.entity"; +import { Listen } from "../listen.entity"; -export class CreateListenDto { +// tslint:disable max-classes-per-file + +export class CreateListenRequestDto { track: Track; user: User; playedAt: Date; } + +export class CreateListenResponseDto { + listen: Listen; + isDuplicate: boolean; +} diff --git a/src/listens/listen.repository.ts b/src/listens/listen.repository.ts index 49cd552..410494a 100644 --- a/src/listens/listen.repository.ts +++ b/src/listens/listen.repository.ts @@ -1,8 +1,12 @@ // tslint:disable: max-classes-per-file import { EntityRepository, Repository, SelectQueryBuilder } from "typeorm"; -import { User } from "../users/user.entity"; -import { Listen } from "./listen.entity"; import { Interval } from "../reports/interval"; +import { User } from "../users/user.entity"; +import { + CreateListenRequestDto, + CreateListenResponseDto, +} from "./dto/create-listen.dto"; +import { Listen } from "./listen.entity"; export class ListenScopes extends SelectQueryBuilder { /** @@ -32,4 +36,35 @@ export class ListenRepository extends Repository { get scoped(): ListenScopes { return new ListenScopes(this.createQueryBuilder("listen")); } + + async insertNoConflict({ + user, + track, + playedAt, + }: CreateListenRequestDto): Promise { + const result = await this.createQueryBuilder() + .insert() + .values({ + user, + track, + playedAt, + }) + .onConflict('("playedAt", "trackId", "userId") DO NOTHING') + .execute(); + + const [insertedRowIdentifier] = result.identifiers; + + if (!insertedRowIdentifier) { + // We did not insert a new listen, it already existed + return { + listen: await this.findOne({ where: { user, track, playedAt } }), + isDuplicate: true, + }; + } + + return { + listen: await this.findOne(insertedRowIdentifier.id), + isDuplicate: false, + }; + } } diff --git a/src/listens/listens.service.ts b/src/listens/listens.service.ts index 04cbdf3..c94d658 100644 --- a/src/listens/listens.service.ts +++ b/src/listens/listens.service.ts @@ -1,11 +1,13 @@ import { Injectable } from "@nestjs/common"; -import { getTime, parseISO, getUnixTime } from "date-fns"; import { IPaginationOptions, paginate, Pagination, } from "nestjs-typeorm-paginate"; -import { CreateListenDto } from "./dto/create-listen.dto"; +import { + CreateListenRequestDto, + CreateListenResponseDto, +} from "./dto/create-listen.dto"; import { GetListensDto } from "./dto/get-listens.dto"; import { Listen } from "./listen.entity"; import { ListenRepository, ListenScopes } from "./listen.repository"; @@ -18,26 +20,14 @@ export class ListensService { user, track, playedAt, - }: CreateListenDto): Promise { - const listen = this.listenRepository.create(); + }: CreateListenRequestDto): Promise { + const response = await this.listenRepository.insertNoConflict({ + user, + track, + playedAt, + }); - listen.user = user; - listen.track = track; - listen.playedAt = playedAt; - - try { - await this.listenRepository.save(listen); - } catch (err) { - if (err.code === "23505") { - return this.listenRepository.findOne({ - where: { user, track, playedAt }, - }); - } - - throw err; - } - - return listen; + return response; } async getListens( diff --git a/src/sources/spotify/spotify-api/spotify-api.service.ts b/src/sources/spotify/spotify-api/spotify-api.service.ts index ee85666..56c2311 100644 --- a/src/sources/spotify/spotify-api/spotify-api.service.ts +++ b/src/sources/spotify/spotify-api/spotify-api.service.ts @@ -18,10 +18,6 @@ export class SpotifyApiService { limit: 50, }; - if (lastRefreshTime) { - parameters.after = lastRefreshTime.getTime(); - } - const history = await this.httpService .get>(`v1/me/player/recently-played`, { headers: { Authorization: `Bearer ${accessToken}` }, diff --git a/src/sources/spotify/spotify.service.ts b/src/sources/spotify/spotify.service.ts index 76098fa..b022839 100644 --- a/src/sources/spotify/spotify.service.ts +++ b/src/sources/spotify/spotify.service.ts @@ -1,5 +1,5 @@ import { Injectable } from "@nestjs/common"; -import { Interval, Timeout } from "@nestjs/schedule"; +import { Interval } from "@nestjs/schedule"; import { ListensService } from "../../listens/listens.service"; import { Logger } from "../../logger/logger.service"; import { Album } from "../../music-library/album.entity"; @@ -80,19 +80,21 @@ export class SpotifyService { playHistory.map(async (history) => { const track = await this.importTrack(history.track.id); - this.listensService.createListen({ + const { isDuplicate } = await this.listensService.createListen({ user, track, playedAt: new Date(history.played_at), }); - this.logger.debug( - `New listen found! ${user.id} listened to "${ - track.name - }" by ${track.artists - ?.map((artist) => `"${artist.name}"`) - .join(", ")}` - ); + if (!isDuplicate) { + this.logger.debug( + `New listen found! ${user.id} listened to "${ + track.name + }" by ${track.artists + ?.map((artist) => `"${artist.name}"`) + .join(", ")}` + ); + } }) ); @@ -103,12 +105,14 @@ export class SpotifyService { .pop() ); - this.logger.debug( - `Updating last refresh time for user ${ - user.id - }, new time: ${newestPlayTime.toISOString()}` - ); - + /** + * lastRefreshTime was previously used to only get new listens from Spotify + * but the Spotify WEB Api was sometimes not adding the listens in the right + * order, causing us to miss some listens. + * + * The variable will still be set, in case we want to add the functionality + * again. + */ this.usersService.updateSpotifyConnection(user, { ...user.spotify, lastRefreshTime: newestPlayTime,