From a25f0fe2cfd3a94fcf426276d9afd21aa8412502 Mon Sep 17 00:00:00 2001 From: QIU Date: Wed, 26 Aug 2026 03:44:59 +0000 Subject: [PATCH] fix(sources): a custom URL now replaces the built-in sources Switching "Custom TLE URL" on used to mean "my source AND yours". The map overwrote only the value keyed "All" and the other 26 built-in sources were still fetched, so pointing Look4Sat at a mirror, a filtered subset, an offline server or a URL reachable on a censored network did not stop it hitting Celestrak 26 more times. On a blocked link the real behaviour was 26 failing requests. This was a fossil rather than a decision: upstream has satelliteDataUrls as a plain list of six URLs with no keys and no custom-URL concept, all fetched unconditionally. The fork turned the list into a keyed map and bolted the override onto one key. Three things made replacing safe to choose, all checked rather than assumed. Stored satellites do not disappear, because insertEntries is OnConflictStrategy.REPLACE and updateFromRemote deletes nothing first, so rows the new source does not mention survive. The selection is a plain id list and is untouched. What degrades is the type filter for the skipped keys, which goes stale rather than empty - the last known membership, not a claim about the current fetch. The key is now customSourceType ("Other"), not "All". setSatelliteTypeIds early-returns on "All", so indexing there was always a no-op - satellites from a custom URL were never reachable by the type filter at all. "Other" is what manual file import already uses, which is the same meaning: satellites from a source the operator supplied. The existing test asserted the "All" index, which means it was asserting a no-op; it now checks that no built-in source is fetched and that the entries land somewhere the filter can see. A second test covers the switch-off path, which must fetch every built-in source exactly as before. --- .../core/data/repository/DatabaseRepo.kt | 38 ++++++++---- .../core/data/repository/DatabaseRepoTest.kt | 62 ++++++++++++++++++- 2 files changed, 86 insertions(+), 14 deletions(-) diff --git a/core/data/src/main/java/com/rtbishop/look4sat/core/data/repository/DatabaseRepo.kt b/core/data/src/main/java/com/rtbishop/look4sat/core/data/repository/DatabaseRepo.kt index 42b637b7..6ac5b62e 100644 --- a/core/data/src/main/java/com/rtbishop/look4sat/core/data/repository/DatabaseRepo.kt +++ b/core/data/src/main/java/com/rtbishop/look4sat/core/data/repository/DatabaseRepo.kt @@ -67,17 +67,33 @@ class DatabaseRepo( override suspend fun updateFromRemote() = withContext(dispatcher) { val dataSourcesSettings = settingsRepo.dataSourcesSettings.value - val tleUrls = buildMap { - putAll(Sources.satelliteDataUrls) - // Switch on + non-empty URL -> All uses the custom URL; otherwise the default URL (online-update default source) - put("All", if (dataSourcesSettings.useCustomTLE && dataSourcesSettings.tleUrl.isNotBlank()) - dataSourcesSettings.tleUrl else Sources.defaultTleUrl) - }.filterValues { it.isNotBlank() } - val radioUrls = buildMap { - putAll(Sources.transceiversDataUrls) - put("SatNOGS", if (dataSourcesSettings.useCustomTransceivers && dataSourcesSettings.transceiversUrl.isNotBlank()) - dataSourcesSettings.transceiversUrl else Sources.defaultTransceiversUrl) - }.filterValues { it.isNotBlank() } + // A custom URL REPLACES the built-in sources rather than joining them. The previous map + // overwrote only the "All" value and still fetched the other 26, so switching this on meant + // "my source AND yours" - which defeats the reasons for setting one: a mirror, a filtered + // subset, an offline server, or a network where Celestrak is unreachable. On a blocked link + // the real behaviour was 26 failing requests. + // + // Satellites already stored do not disappear: insertEntries is OnConflictStrategy.REPLACE + // and nothing is deleted before the insert, so rows the new source does not mention survive. + // The type index for the skipped keys goes stale rather than empty, which is the honest + // outcome - it is the last known membership, not a claim about this fetch. + // + // The key is customSourceType, not "All": setSatelliteTypeIds early-returns on "All", so + // indexing under it was always a no-op, and "Other" is what manual file import already + // uses. Same meaning - satellites from a source the operator supplied - and it makes them + // reachable by the type filter, which they were not before. + val tleUrls = if (dataSourcesSettings.useCustomTLE && dataSourcesSettings.tleUrl.isNotBlank()) { + mapOf(customSourceType to dataSourcesSettings.tleUrl) + } else { + Sources.satelliteDataUrls.filterValues { it.isNotBlank() } + } + val radioUrls = if (dataSourcesSettings.useCustomTransceivers && + dataSourcesSettings.transceiversUrl.isNotBlank() + ) { + mapOf("SatNOGS" to dataSourcesSettings.transceiversUrl) + } else { + Sources.transceiversDataUrls.filterValues { it.isNotBlank() } + } // launch all network requests concurrently val tleJobs = tleUrls.values.map { url -> async { url to remoteSource.getNetworkStream(url) } } val radioJobs = radioUrls.values.map { url -> async { url to remoteSource.getNetworkStream(url) } } diff --git a/core/data/src/test/java/com/rtbishop/look4sat/core/data/repository/DatabaseRepoTest.kt b/core/data/src/test/java/com/rtbishop/look4sat/core/data/repository/DatabaseRepoTest.kt index 58194d23..4e927707 100644 --- a/core/data/src/test/java/com/rtbishop/look4sat/core/data/repository/DatabaseRepoTest.kt +++ b/core/data/src/test/java/com/rtbishop/look4sat/core/data/repository/DatabaseRepoTest.kt @@ -18,6 +18,7 @@ package com.rtbishop.look4sat.core.data.repository import com.rtbishop.look4sat.core.domain.model.DataSourcesSettings +import com.rtbishop.look4sat.core.domain.source.Sources import com.rtbishop.look4sat.core.domain.model.DatabaseState import com.rtbishop.look4sat.core.domain.model.OtherSettings import com.rtbishop.look4sat.core.domain.model.PassesSettings @@ -102,8 +103,57 @@ class DatabaseRepoTest { repository.updateFromRemote() assertTrue(localSource.insertedEntries.any { it.catnum == 25544 }) - // New semantics: switch on + non-empty URL -> the All source uses the custom URL, data lands in the All type - assertEquals(listOf(25544), settingsRepo.satelliteTypeIdsByType["All"]) + // A custom URL replaces the built-in TLE sources: none of them is requested. The + // transceivers group is separate and its own switch is off here, so it still fetches. + val builtInTle = Sources.satelliteDataUrls.values.filter { it.isNotBlank() } + assertTrue( + "no built-in TLE source may be fetched, got " + remoteSource.requestedUrls, + builtInTle.none { it in remoteSource.requestedUrls } + ) + assertTrue( + "the operator's URL must be fetched", + customCsvUrl in remoteSource.requestedUrls + ) + // Indexed under "Other", the key manual file import uses. It used to be indexed under + // "All", where setSatelliteTypeIds early-returns - so the type filter never saw these + // satellites at all and this assertion was checking a no-op. + assertEquals(listOf(25544), settingsRepo.satelliteTypeIdsByType["Other"]) + assertEquals(null, settingsRepo.satelliteTypeIdsByType["All"]) + } + + /** The switch-off path must be untouched: all built-in sources, exactly as before. */ + @Test + fun `without a custom source every built-in source is fetched`() = runTest(dispatcher) { + val localSource = FakeLocalSource() + val remoteSource = FakeRemoteSource().apply { + // Every built-in TLE source has to answer, or updateFromRemote throws because all of + // them failed, which would mask what this test checks. The transceivers group is left + // unanswered on purpose: org.json is compileOnly in core:domain, so DataParser cannot + // parse a radio payload on the JVM anyway. + Sources.satelliteDataUrls.values.filter { it.isNotBlank() } + .forEach { networkStreams[it] = { validCsvStream() } } + } + val settingsRepo = FakeSettingsRepo( + dataSources = DataSourcesSettings( + useCustomTLE = false, + useCustomTransceivers = false, + tleUrl = "https://example.com/ignored.csv", + transceiversUrl = "" + ) + ) + val repository = DatabaseRepo(dispatcher, dataParser, localSource, remoteSource, settingsRepo) + + repository.updateFromRemote() + + val expected = Sources.satelliteDataUrls.values.filter { it.isNotBlank() } + assertTrue( + "expected all built-in sources, got " + remoteSource.requestedUrls.size, + expected.all { it in remoteSource.requestedUrls } + ) + assertTrue( + "the custom URL must not be fetched when the switch is off", + "https://example.com/ignored.csv" !in remoteSource.requestedUrls + ) } private fun validCsvStream(): InputStream = """ @@ -122,9 +172,15 @@ private class FakeRemoteSource : IRemoteSource { val fileStreams: MutableMap InputStream> = mutableMapOf() val networkStreams: MutableMap InputStream> = mutableMapOf() + /** Every URL asked for, so a test can assert WHICH sources were fetched, not just the result. */ + val requestedUrls = mutableListOf() + override suspend fun getFileStream(uri: String): InputStream? = fileStreams[uri]?.invoke() - override suspend fun getNetworkStream(url: String): InputStream? = networkStreams[url]?.invoke() + override suspend fun getNetworkStream(url: String): InputStream? { + requestedUrls += url + return networkStreams[url]?.invoke() + } override suspend fun getAmSatCatalog(): String? = null