Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@ class ImprovedSearchManagerImpl @Inject constructor(
) : ImprovedSearchManager {
override suspend fun autoCompleteSearch(term: String): List<SearchAutoCompleteItem> {
val response = autoCompleteSearchService.autoCompleteSearch(query = term, termsLimit = null, podcastsLimit = null)
return response.results.map {
return response.results.mapNotNull {
when (it) {
is AutoCompleteResult.TermResult -> SearchAutoCompleteItem.Term(term = it.value)

Expand All @@ -26,6 +26,8 @@ class ImprovedSearchManagerImpl @Inject constructor(
author = it.value.author.orEmpty(),
isExplicit = it.value.explicit == true,
)

AutoCompleteResult.Unknown -> null
}
}
}
Expand Down Expand Up @@ -53,6 +55,8 @@ class ImprovedSearchManagerImpl @Inject constructor(
publishedDate = it.publishedDate,
duration = it.duration.seconds,
)

CombinedResult.Unknown -> null
}
}
}
Expand Down
Original file line number Diff line number Diff line change
@@ -1,16 +1,21 @@
package au.com.shiftyjelly.pocketcasts.repositories.search

import au.com.shiftyjelly.pocketcasts.models.to.ImprovedSearchResultItem
import au.com.shiftyjelly.pocketcasts.models.to.SearchAutoCompleteItem
import au.com.shiftyjelly.pocketcasts.servers.podcast.PodcastCacheService
import au.com.shiftyjelly.pocketcasts.servers.search.AutoCompleteResponse
import au.com.shiftyjelly.pocketcasts.servers.search.AutoCompleteResult
import au.com.shiftyjelly.pocketcasts.servers.search.AutoCompleteSearchService
import au.com.shiftyjelly.pocketcasts.servers.search.CombinedResult
import au.com.shiftyjelly.pocketcasts.servers.search.CombinedSearchResponse
import au.com.shiftyjelly.pocketcasts.servers.search.PodcastResultValue
import java.util.Date
import kotlinx.coroutines.test.runTest
import org.junit.Assert.assertEquals
import org.junit.Assert.assertTrue
import org.junit.Test
import org.mockito.kotlin.any
import org.mockito.kotlin.anyOrNull
import org.mockito.kotlin.doReturn
import org.mockito.kotlin.mock
import org.mockito.kotlin.whenever
Expand Down Expand Up @@ -66,4 +71,47 @@ class ImprovedSearchManagerImplTest {
val episode = results.filterIsInstance<ImprovedSearchResultItem.EpisodeItem>().single()
assertEquals("Business Daily", episode.podcastTitle)
}

@Test
fun `combined search drops unknown result types`() = runTest {
whenever(combinedSearchService.combinedSearch(any())) doReturn CombinedSearchResponse(
results = listOf(
CombinedResult.PodcastResult(
uuid = "podcast-uuid",
title = "Big Sugar",
author = "Weekday Fun Productions",
slug = "big-sugar",
explicit = false,
),
CombinedResult.Unknown,
),
)

val results = manager.combinedSearch("big sugar")

assertEquals(listOf("podcast-uuid"), results.map { it.uuid })
}

@Test
fun `autocomplete search drops unknown result types`() = runTest {
whenever(autoCompleteSearchService.autoCompleteSearch(any(), anyOrNull(), anyOrNull())) doReturn AutoCompleteResponse(
results = listOf(
AutoCompleteResult.TermResult(value = "big sugar"),
AutoCompleteResult.PodcastResult(
value = PodcastResultValue(uuid = "podcast-uuid", title = "Big Sugar"),
),
AutoCompleteResult.Unknown,
),
)

val results = manager.autoCompleteSearch("big sugar")

assertEquals(
listOf<SearchAutoCompleteItem>(
SearchAutoCompleteItem.Term(term = "big sugar"),
SearchAutoCompleteItem.Podcast(uuid = "podcast-uuid", title = "Big Sugar", author = "", isExplicit = false),
),
results,
)
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -19,10 +19,13 @@ sealed class AutoCompleteResult {
val value: PodcastResultValue,
) : AutoCompleteResult()

data object Unknown : AutoCompleteResult()

companion object {
val jsonAdapter = PolymorphicJsonAdapterFactory.of(AutoCompleteResult::class.java, "type")
.withSubtype(TermResult::class.java, "term")
.withSubtype(PodcastResult::class.java, "podcast")
.withDefaultValue(Unknown)
}
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@ sealed interface CombinedResult {
val uuid: String,
val title: String? = null,
val author: String? = "",
val slug: String,
val slug: String = "",

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good change — slug and podcastSlug are read by nobody (ImprovedSearchManagerImpl.kt:37-61 ignores both, and there are no other consumers of CombinedResult), so requiring them was pure downside.

One caveat on how far a Kotlin default actually protects you: Moshi's generated adapter uses the default only when the key is absent. If the server sends "slug": null explicitly, codegen still hits Util.unexpectedNull(...) and throws JsonDataException: Non-null value 'slug' was null at $.results[N], which aborts the whole response exactly like the old bug. String? = null covers both the absent and explicit-null cases; String = "" covers only the first. Given the motivation here is "one weird result shouldn't wipe out the search", nullable is the stronger guarantee.

Related, and the same argument as slug: url (line 29) is still non-null-required and is likewise never read by any consumer. title/podcastUuid/podcastTitle are at least consumed, though it's a bit asymmetric that PodcastResult.title is nullable-and-filtered (ImprovedSearchManagerImpl.kt:40) while EpisodeResult.title hard-fails the response.

Not blocking — the PR's stated bug is fixed either way.

Fix this →

val explicit: Boolean? = null,
) : CombinedResult

Expand All @@ -33,12 +33,15 @@ sealed interface CombinedResult {
@Json(name = "podcast_title")
val podcastTitle: String,
@Json(name = "podcast_slug")
val podcastSlug: String,
val podcastSlug: String = "",
) : CombinedResult

data object Unknown : CombinedResult

companion object {
val jsonAdapter = PolymorphicJsonAdapterFactory.of(CombinedResult::class.java, "type")
.withSubtype(PodcastResult::class.java, "podcast")
.withSubtype(EpisodeResult::class.java, "episode")
.withDefaultValue(Unknown)
Comment thread
sztomek marked this conversation as resolved.
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,28 @@
package au.com.shiftyjelly.pocketcasts.servers.search

import com.squareup.moshi.Moshi
import org.junit.Assert.assertEquals
import org.junit.Test

class AutoCompleteResponseTest {
private val adapter = Moshi.Builder()
.add(AutoCompleteResult.jsonAdapter)
.build()
.adapter(AutoCompleteResponse::class.java)

@Test
fun `unknown result types map to Unknown without failing the whole response`() {
val response = adapter.fromJson(
"""
{"results":[
{"value":"freakonomics","type":"term"},
{"value":{"uuid":"p1","title":"Freakonomics Radio"},"type":"podcast"},
{"value":"anything","type":"network"}
]}
""".trimIndent(),
)

val types = response?.results?.map { it::class.simpleName }
assertEquals(listOf("TermResult", "PodcastResult", "Unknown"), types)
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,47 @@
package au.com.shiftyjelly.pocketcasts.servers.search

import com.squareup.moshi.Moshi
import com.squareup.moshi.adapters.Rfc3339DateJsonAdapter
import java.util.Date
import org.junit.Assert.assertEquals
import org.junit.Test

class CombinedSearchResponseTest {
private val adapter = Moshi.Builder()
.add(Date::class.java, Rfc3339DateJsonAdapter().nullSafe())
.add(CombinedResult.jsonAdapter)
.build()
.adapter(CombinedSearchResponse::class.java)

@Test
fun `unknown result types map to Unknown without failing the whole response`() {
val response = adapter.fromJson(
"""
{"results":[
{"uuid":"p1","title":"Freakonomics Radio","slug":"freakonomics-radio","type":"podcast"},
{"uuid":"n1","title":"Some Network","type":"network"},
{"uuid":"p2","title":"Another Show","slug":"another-show","type":"podcast"}
]}
""".trimIndent(),
)

val types = response?.results?.map { it::class.simpleName }
assertEquals(listOf("PodcastResult", "Unknown", "PodcastResult"), types)
}
Comment thread
sztomek marked this conversation as resolved.
Comment thread
sztomek marked this conversation as resolved.

@Test
fun `registered podcast and episode subtypes decode alongside the fallback`() {
val response = adapter.fromJson(
"""
{"results":[
{"uuid":"p1","title":"Freakonomics Radio","slug":"freakonomics-radio","type":"podcast"},
{"uuid":"e1","title":"Ep 1","url":"https://example.com/1.mp3","published_date":"2024-01-02T03:04:05Z","podcast_uuid":"p1","podcast_title":"Freakonomics Radio","podcast_slug":"freakonomics-radio","type":"episode"},
{"uuid":"n1","title":"Some Network","type":"network"}
]}
""".trimIndent(),
)

val types = response?.results?.map { it::class.simpleName }
assertEquals(listOf("PodcastResult", "EpisodeResult", "Unknown"), types)
}
}
Loading