Recursively refresh playlist tracks within smart playlist rules #3018

reillymc · 2024-05-12T00:13:14Z

This PR ensure that when a smart playlist is refreshed, the playlists used in its rules also have their tracks refreshed first.

This is my first time diving into the Navidrome codebase and using the Go language, so if there is anything I need to do to improve the code here please let me know and I'll be happy to fix it up!

github-actions · 2024-05-12T22:40:27Z

Download the artifacts for this pull request:

deluan

Good job for a first Go AND Navidrome contribution. I admit this codebase can be daunting.

Anyway, thanks and take a look at my comments below.

deluan · 2024-06-11T01:23:37Z

model/criteria/operators.go

+	case Any:
+		for _, rules := range rule {
+			ids = extractPlaylistIds(rules, ids)
+		}
+	case All:
+		for _, rules := range rule {
+			ids = extractPlaylistIds(rules, ids)
+		}
+	case InPlaylist:
+		if id, ok = rule["id"].(string); ok {
+			ids = append(ids, id)
+		}
+	case NotInPlaylist:
+		if id, ok = rule["id"].(string); ok {
+			ids = append(ids, id)
+		}


Suggested change

case Any:

for _, rules := range rule {

ids = extractPlaylistIds(rules, ids)

}

case All:

for _, rules := range rule {

ids = extractPlaylistIds(rules, ids)

}

case InPlaylist:

if id, ok = rule["id"].(string); ok {

ids = append(ids, id)

}

case NotInPlaylist:

if id, ok = rule["id"].(string); ok {

ids = append(ids, id)

}

case Any, All:

for _, rules := range rule {

ids = extractPlaylistIds(rules, ids)

}

case InPlaylist, NotInPlaylist:

if id, ok = rule["id"].(string); ok {

ids = append(ids, id)

}

Ah yes, I initially tried to do this however I hit an error. From reading up on it, using multiple cases in a type switch doesn't narrow the type of rule to Any/All but instead it remains as interface{} and I am then unable iterate over or index it. I wasn't able to find a nice solution unfortunately so I had to leave it verbose. Happy to fix up if there's a trick I've missed.

Hummm, yeah I think you are right. We can leave it like that for now.

I'll have to do a big refactor in Smart Playlists, due to the database schema changes in #2709, so if we merge this I'll think a bit about ways to remove this duplication.

deluan · 2024-06-11T01:29:09Z

model/criteria/operators.go

@@ -275,3 +283,29 @@ func inList(m map[string]interface{}, negate bool) (sql string, args []interface
 		return "media_file.id IN (" + subQText + ")", subQArgs, nil
 	}
 }
+
+func extractPlaylistIds(inputRule interface{}, ids []string) []string {
+	var id string


You are never returning the ids slice, you are basically mutating the slice passed. Try to not mutate the param and properly returning the new slice.

Thanks, I think this should be addressed now by using a named return type instead of an argument. Whoops!

deluan · 2024-06-11T01:30:34Z

model/criteria/operators.go

@@ -36,6 +40,10 @@ func (any Any) MarshalJSON() ([]byte, error) {
 	return marshalConjunction("any", any)
 }

+func (any Any) ChildPlaylistIds() (ids []string) {
+	return extractPlaylistIds(any, ids)


And that's why it is working, because you are passing the response as a param to extractPlaylistIds. If we went with this design, extractPlaylistIds wouldn't need to return anything. But I rather not mutate arguments when possible.

Ah thank you for that - definitely agree its nicer not to mutate the arguments.

deluan · 2024-06-11T01:31:54Z

persistence/playlist_repository.go

+			log.Error(r.ctx, "Error loading child playlist", "id", pls.ID, "childId", id, err)
+			return false
+		}
+		r.refreshSmartPlaylist(childPls)


I'm not too sure if we want to refresh all children playlists like this, this could have a performance impact. Did you test it with large playlists?

Definitely a fair point on performance. I did a little bit of testing that I'll include below. I could only manage to grab ~2500 songs for my test library - not sure what large libraries/playlists are in terms of how Navidrome is used but hopefully this is some useful data to go off! I used the logger to get the times from the running dev server.

Test 1:
A smart playlist with two playlist dependencies. Each picking a random 100 songs from a library of ~2500

Playlist Run 1 Run 2 Run 3 Average

Sub-playlist A 8.5 7.2 5.7 7.1

Sub-playlist B 7.9 13.4 6.5 9.3

Main playlist with changes 27.4 32.9 22.5 27.6

Main playlist without changes 5.6 27.2 4.5 12.4

Difference of ~15.2ms to fetch main playlist. This aligns with the ~16.4 total milliseconds taken to refresh the sub playlists.

Test 2:
A smart playlist with two playlist dependencies. Each picking a random 1000 songs from a library of ~2500

Playlist Run 1 Run 2 Run 3 Average

Sub-playlist A 10.0 8.6 10.0 9.5

Sub-playlist B 8.8 8.5 7.7 8.3

Main playlist with changes 43.7 45 43 43.9

Main playlist without changes 25.9 25.3 23.4 24.9

Difference of ~19ms to fetch main playlist. This aligns with the ~17.8 total milliseconds taken to refresh the sub playlists.

Overall the fetching time for the sub-playlists is basically added onto the main playlist which is expected. The additional time is pretty minor (at least at this scale) for the total refresh time to the user; however it does basically double the overall time which could be an issue at a much larger scale.

An alternative may be to map these recursive refreshes into the one SQL statement directly as sub-queries (although this is probably a bit beyond what I can wrangle, at least right now).

Otherwise perhaps a boolean flag on the playlist to indicate whether it should use the 'dynamic refresh' of sub-playlists functionality.

Can you post here the NSP files you used to run this tests? I can run it against my 83K songs library.

Wow thats a serious test library...
Here are my nsp files (as json files so github would allow the upload). Basically I used a bulk tagger to set half my library with a comment of A and the other half with B.

mainPlaylist.json
selectionB.json
selectionA.json

reillymc · 2024-06-19T10:40:06Z

Good job for a first Go AND Navidrome contribution. I admit this codebase can be daunting.

Anyway, thanks and take a look at my comments below.

Thanks for taking a look! I'd be interested in your thoughts on the performance side and if you think this will be a trade-off you'd be willing to make. This ability is the last thing from my old iTunes playlists that I haven't been able to bring over to Navidrome but otherwise they have been fantastic!

deluan · 2024-06-19T22:24:10Z

One more thing: It is missing tests. Can you please add tests for the logic you implemented?

deluan · 2024-07-25T22:26:38Z

conf/configuration.go

@@ -300,6 +301,7 @@ func init() {
 	viper.SetDefault("enableartworkprecache", true)
 	viper.SetDefault("autoimportplaylists", true)
 	viper.SetDefault("playlistspath", consts.DefaultPlaylistsPath)
+	viper.SetDefault("smartPlaylistRefreshTimeout", 5*time.Second)


Hey, thanks for this. Can you make the value all lowercase, like smartplaylistrefreshtimeout? I think Viper has an issue with this.

Oops my bad - should have picked that up from the other items. All fixed!

deluan · 2024-07-25T22:28:00Z

model/criteria/criteria_test.go

@@ -89,4 +90,94 @@ var _ = Describe("Criteria", func() {
 		gomega.Expect(newObj.OrderBy()).To(gomega.Equal("random() asc"))
 	})

+	It("extracts all child smart playlist IDs from All expression criteria", func() {
+		var topLevelInPlaylistID = uuid.New().String()


Nitpick: please use this style:

Suggested change

var topLevelInPlaylistID = uuid.New().String()

topLevelInPlaylistID := uuid.NewString()

Signed-off-by: reillymc <[email protected]>

…refetching Signed-off-by: reillymc <[email protected]>

sammbeller

couple nits, obviously feel free to ignore

sammbeller · 2024-09-11T12:10:37Z

persistence/playlist_repository_test.go

+				nestedPlsAfterParentGet, err := repo.Get(nestedPls.ID)
+				Expect(err).ToNot(HaveOccurred())
+
+				Expect(*nestedPlsAfterParentGet.EvaluatedAt).To(BeTemporally(">", *nestedPlsRead.EvaluatedAt))


I think it would be better to update the nested playlist and check the updated value is referenced correctly in the parent playlist, which is a more specific test about the expected behavior of the used Criteria that would additionally verify that this change does what it is expected to do. Does that make sense?

Yes, it does. This tests seems to assume the resolution of the Smart Playlist rules works as expected, so it only checks if the nested playlist was updated or not. I think it is a valid test.

IMO, what you are suggesting is more like an integration test, that checks all these parts working together, and we would need to setup data in the MediaFile repository for it to work. Of course it has its value, but requires a different setup. I'm personally ok with this test as is.

More as a matter of learning about your testing principles in this project I would like to make the following comment. The test as written is looking at behavior internal to the unit under test, specifically the line including the conditional that the delay is included on. We're already in the data layer so I'm not sure scope is increased that much by touching another repo. At the same time, there are other ways to test this exact behavior such as ensuring that refreshSmartPlaylist is called on all relevant child playlists. In general I don't think this side-effect testing test is super valuable.

If this discussion has outlived its usefulness, please let me know. As I said I'm just trying to learn about how this project thinks about testing.

sammbeller · 2024-09-11T12:11:23Z

persistence/playlist_repository.go

-	// Only refresh if it is a smart playlist and was not refreshed in the last 5 seconds
-	if !pls.IsSmartPlaylist() || (pls.EvaluatedAt != nil && time.Since(*pls.EvaluatedAt) < 5*time.Second) {
+	// Only refresh if it is a smart playlist and was not refreshed within the interval provided by the refresh timeout config
+	if !pls.IsSmartPlaylist() || (pls.EvaluatedAt != nil && time.Since(*pls.EvaluatedAt) < conf.Server.SmartPlaylistRefreshTimeout) {


nit: this name is confusing, this isn't really a timeout

Agree. I'm changing it to SmartPlaylistRefreshDelay, and changing the default to 1 minute.

…ease default value

deluan

Fixed small nits, hope you agree with my changes :)

Merging it now.

reillymc · 2024-09-15T23:19:07Z

Looks great! Thanks for all your help to get this merged!

reillymc force-pushed the smart-playlist-recursive-track-refresh branch from 08b12b8 to 2863c61 Compare May 12, 2024 07:12

deluan requested changes Jun 11, 2024

View reviewed changes

deluan reviewed Jul 25, 2024

View reviewed changes

reillymc force-pushed the smart-playlist-recursive-track-refresh branch from dde553e to 217e97b Compare July 27, 2024 03:01

reillymc added 3 commits September 2, 2024 01:30

Recursively refresh playlists within smart playlist rules

2f577e6

Signed-off-by: reillymc <[email protected]>

Clean up recursive smart playlist functions

4f6d56f

Signed-off-by: reillymc <[email protected]>

Add smart playlist refresh timeout config and tests for nested track …

521a9a3

…refetching Signed-off-by: reillymc <[email protected]>

reillymc force-pushed the smart-playlist-recursive-track-refresh branch from 217e97b to 521a9a3 Compare September 2, 2024 01:46

reillymc requested a review from deluan September 4, 2024 10:51

sammbeller reviewed Sep 11, 2024

View reviewed changes

Change SmartPlaylistRefreshTimeout to SmartPlaylistRefreshDelay, incr…

5177078

…ease default value

deluan approved these changes Sep 15, 2024

View reviewed changes

Revert smartPlaylistRefreshDelay default to 5 seconds

a365820

deluan merged commit d683688 into navidrome:master Sep 15, 2024
5 checks passed

BrewTestBot mentioned this pull request Sep 17, 2024

navidrome 0.53.0 Homebrew/homebrew-core#190903

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Recursively refresh playlist tracks within smart playlist rules #3018

Recursively refresh playlist tracks within smart playlist rules #3018

reillymc commented May 12, 2024

github-actions bot commented May 12, 2024 •

edited

Loading

deluan left a comment

deluan Jun 11, 2024

reillymc Jun 19, 2024

deluan Jun 19, 2024

deluan Jun 11, 2024

reillymc Jun 19, 2024

deluan Jun 11, 2024

reillymc Jun 19, 2024

deluan Jun 11, 2024

reillymc Jun 19, 2024

deluan Jun 19, 2024 •

edited

Loading

reillymc Jun 19, 2024

reillymc commented Jun 19, 2024

deluan commented Jun 19, 2024

deluan Jul 25, 2024

reillymc Jul 27, 2024

deluan Jul 25, 2024

sammbeller left a comment

sammbeller Sep 11, 2024

deluan Sep 15, 2024

sammbeller Sep 15, 2024

sammbeller Sep 11, 2024

deluan Sep 15, 2024

deluan left a comment

reillymc commented Sep 15, 2024

Playlist	Run 1	Run 2	Run 3	Average
Sub-playlist A	8.5	7.2	5.7	7.1
Sub-playlist B	7.9	13.4	6.5	9.3
Main playlist with changes	27.4	32.9	22.5	27.6
Main playlist without changes	5.6	27.2	4.5	12.4

Playlist	Run 1	Run 2	Run 3	Average
Sub-playlist A	10.0	8.6	10.0	9.5
Sub-playlist B	8.8	8.5	7.7	8.3
Main playlist with changes	43.7	45	43	43.9
Main playlist without changes	25.9	25.3	23.4	24.9

	var topLevelInPlaylistID = uuid.New().String()
	topLevelInPlaylistID := uuid.NewString()

Recursively refresh playlist tracks within smart playlist rules #3018

Recursively refresh playlist tracks within smart playlist rules #3018

Conversation

reillymc commented May 12, 2024

github-actions bot commented May 12, 2024 • edited Loading

deluan left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

deluan Jun 19, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

reillymc commented Jun 19, 2024

deluan commented Jun 19, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sammbeller left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

deluan left a comment

Choose a reason for hiding this comment

reillymc commented Sep 15, 2024

github-actions bot commented May 12, 2024 •

edited

Loading

deluan Jun 19, 2024 •

edited

Loading