Conversation
kellnerd
left a comment
kellnerd
left a comment
There was a problem hiding this comment.
First off, I really appreciate that you have provided documentation with examples. That makes it a lot easier to understand what you are trying to achieve.
I also like that you have kept the API standalone and independent from the web app, this will make testing and the initial development iterations a lot easier.
Personally I wouldn't integrate the API into the web app until it is ready and the endpoints are reasonably stable.
And if we do that, the API definitely shouldn't start a second web server (on a different port) from the web app's main module, it should be mounted into the existing server of the Fresh app. (Once Harmony's been migrated to Fresh 2.0, composition of middlewares will be a lot easier, similar to Express.js etc.)
I haven't really thought much about how an API for Harmony should look like, so I am open to suggestions by people who would use the API.
Nevertheless I've been confused by two of your decisions:
- There are many endpoints which effectively do the same thing (a release lookup), but return different parts of the resulting data. I would have thought one endpoint
/api/v1/release/lookupthat returns all data (or accepts parameters) should be sufficient?
Especially/api/v1/matchlooks oddly-specific with thecandidatesandbestproperty which always return the same single release. - I don't really see the need for
serializeRelease... The existingHarmonyReleasetype is already fully JSON-compatible and your serialization deviates only in minor aspects.Most of the differences feel like your personal preferences to me. But if you have well-reasoned improvement suggestions, I would rather changeHarmonyReleaseand have one consistent format.
We should probably go back to #107 to collect ideas for the initial API endpoints where everyone who is interested has a chance to participate?
P.S. I also wanted to mention that deploying an API is currently not scalable, but that isn't specific to your implementation. There are mainly two concerns, one is inherent to the concept of Harmony, the other is related to the current architecture:
- The web app alone is already hitting the limits of the underlying APIs quite frequently nowadays. Hosting an API on the same server would only make this much worse.
- All the outgoing API requests are currently cached indefinitely in order to guarantee that the permalinks in edit notes continue to work and provide evidence using the original/historic data that was used to import the release.
TheSnapStoragecache therefore accumulates a lot of (mostly JSON) data (about 57GB in 18 months). While this is acceptable for the Release Lookup page which has a high probability of resulting in a MB import, it is wasteful for Release Actions already and almost pointless for most API requests that I would expect.
So before I feel confident to deploy an API into production, at least the second concern has to be resolved to avoid cluttering the SnapStorage with tons of data. Therefore Harmony needs a second, more traditional caching layer which discards data after a certain time (probably 24 hours). Only Release Lookup (or requests which explicitly need a permalink) should write to the permanent SnapStorage cache.
If the first concern turns out to be an issue, we can always turn off the API to improve the situation while it is hard to impossible to get rid of the useless API snapshots.
| // Primary location: release group MBID (populated by MBID resolver) | ||
| if (release.releaseGroup?.mbid) { | ||
| return release.releaseGroup.mbid; | ||
| } | ||
|
|
||
| // Fallback: check if MusicBrainz provider returned data | ||
| const mbProvider = release.info.providers.find((p) => p.internalName === 'musicbrainz'); | ||
| if (mbProvider?.id) { | ||
| return mbProvider.id; | ||
| } |
There was a problem hiding this comment.
This is wrong, the first is a release group MBID, the second is a release MBID.
| const trackCounts = release.media.map((m) => m.tracklist.length); | ||
| const totalTracks = trackCounts.reduce((sum, c) => sum + c, 0); | ||
| return { | ||
| mediaCount: release.media.length, | ||
| trackCounts, | ||
| totalTracks, | ||
| }; |
There was a problem hiding this comment.
I feel like this is redundant data and should be computed by the client?
| score: 1.0, | ||
| evidence: ['harmony-lookup'], |
There was a problem hiding this comment.
What's the purpose of these properties (and the "match" endpoint in general)?
| ```json | ||
| { | ||
| "ok": true, | ||
| "version": "mvp-20", |
There was a problem hiding this comment.
What's this? Shouldn't this rather return the Harmony release version?
| function formatDuration(ms: number): string { | ||
| const totalSeconds = Math.floor(ms / 1000); | ||
| const minutes = Math.floor(totalSeconds / 60); | ||
| const seconds = totalSeconds % 60; | ||
| return `${minutes}:${seconds.toString().padStart(2, '0')}`; | ||
| } |
There was a problem hiding this comment.
I don't think that the API should format durations, but if it does, there is already an existing utility for that.
| # Standalone | ||
| deno task api |
| ### Supported Providers | ||
|
|
||
| - Spotify | ||
| - Deezer | ||
| - iTunes / Apple Music | ||
| - Bandcamp (URL lookup only, no GTIN) | ||
| - Beatport | ||
| - Tidal | ||
| - MusicBrainz | ||
| - mora (URL lookup only, no GTIN) | ||
| - OTOTOY (URL lookup only, no GTIN) |
There was a problem hiding this comment.
This isn't API specific and shouldn't be mentioned here. The web app already generates this information for the About page.
kellnerd
added
feature
2 participants
First PR, hopefully I didn't screw up too badly.
This adds an API to Harmony. I've included an API.md file available in project root for review. The PR consists of a slightly modified server/main.ts and a new server/api.ts. I formatted everything with deno fmt and deno task ok shows no issues.