fix v1 data download fails (#196)

e-kotov · web-flow · commit ba135a87b626 · 2026-06-06T22:26:05.000+02:00
* Update version to 0.2.5.9000, enhance NEWS.md, and improve data handling in available-data and download_data functions

* Enhance metadata mapping and improve error handling in data retrieval functions

* Remove unnecessary package loading in test-regression-v1-meta.R

* Refactor metadata path retrieval and enhance tests for V1 data handling
diff --git a/DESCRIPTION b/DESCRIPTION
@@ -1,6 +1,6 @@
 Package: spanishoddata
 Title: Get Spanish Origin-Destination Data
-Version: 0.2.5
+Version: 0.2.5.9000
 Authors@R: c(
     person("Egor", "Kotov", , "kotov.egor@gmail.com", role = c("aut", "cre"),
            comment = c(ORCID = "0000-0001-6690-5345")),
diff --git a/NEWS.md b/NEWS.md
@@ -1,3 +1,9 @@
+# spanishoddata (development version)
+
+## Bug fixes
+
+* Improved Version 1 (2020-2021) metadata mapping to ensure redirected district files are correctly identified and added a check to alert the user if no data is found for the requested criteria.
+
 # spanishoddata 0.2.5
 
 ## Bug fixes
diff --git a/R/available-data.R b/R/available-data.R
@@ -227,6 +227,33 @@ spod_available_data_v1 <- function(
   ))
   # order by pub_ts
   files_table <- files_table[order(files_table$pub_ts, decreasing = TRUE), ]
+
+  # replace all municipal data download links with districts links
+  # this is to address the bugs described in detail in:
+  # https://www.ekotov.pro/mitma-data-issues/issues/011-v1-tpp-mismatch-zone-ids-in-table-and-spatial-data.html
+  # https://www.ekotov.pro/mitma-data-issues/issues/012-v1-tpp-district-files-in-municipality-folders.html
+  # the decision was to use distrcit data and aggregate it to replicate municipal data
+  # this substitution MUST happen before local_path generation to avoid mismatches
+  # that lead to spod_download skipping files
+
+  # create a flag to identify rows that were originally municipality entries
+  # this is needed to correctly handle file sizes later
+  files_table$is_redirected_muni <- grepl(
+    "mitma-municipios",
+    files_table$target_url
+  )
+
+  files_table$target_url <- gsub(
+    "mitma-municipios",
+    "mitma-distritos",
+    files_table$target_url
+  )
+  files_table$target_url <- gsub(
+    "mitma_municipio",
+    "mitma_distrito",
+    files_table$target_url
+  )
+
   files_table$local_path <- fs::path(
     data_dir,
     spod_subfolder_raw_data_cache(ver = 1),
@@ -278,22 +305,6 @@ spod_available_data_v1 <- function(
     files_table$local_path
   )
 
-  # replace all municipal data download links with districts links
-  # this is to address the bugs described in detail in:
-  # https://www.ekotov.pro/mitma-data-issues/issues/011-v1-tpp-mismatch-zone-ids-in-table-and-spatial-data.html
-  # https://www.ekotov.pro/mitma-data-issues/issues/012-v1-tpp-district-files-in-municipality-folders.html
-  # the decision was to use distrcit data and aggregate it to replicate municipal data
-  files_table$target_url <- gsub(
-    "mitma-municipios",
-    "mitma-distritos",
-    files_table$target_url
-  )
-  files_table$target_url <- gsub(
-    "mitma_municipio",
-    "mitma_distrito",
-    files_table$target_url
-  )
-
   files_table <- files_table |>
     dplyr::mutate(
       study = factor(
@@ -337,6 +348,7 @@ spod_available_data_v1 <- function(
 
       zones = factor(
         dplyr::case_when(
+          .data$is_redirected_muni ~ "municipalities",
           grepl("distrito", .data$target_url) ~ "districts",
           grepl("municipio", .data$target_url) ~ "municipalities",
           TRUE ~ NA_character_
@@ -348,15 +360,19 @@ spod_available_data_v1 <- function(
   # add known file sizes from cached data
   if (s3_successful) {
     # replace remote file sizes for v1
+    # this is to handle cases where we redirected a municipality URL to a district URL
+    # we need the size of the district file, not the municipality one
     replacement_file_sizes_distr <- files_table |>
-      dplyr::filter(grepl("mitma-distr", .data$local_path)) |>
+      dplyr::filter(!.data$is_redirected_muni) |>
       dplyr::select("target_url", "file_size_bytes")
+
     replaced_file_sizes_municip <- files_table |>
-      dplyr::filter(grepl("mitma-municip", .data$local_path)) |>
+      dplyr::filter(.data$is_redirected_muni) |>
       dplyr::select(-"file_size_bytes") |>
       dplyr::left_join(replacement_file_sizes_distr, by = "target_url")
+
     files_table_replaced_file_sizes <- files_table |>
-      dplyr::filter(!grepl("mitma-municip", .data$local_path)) |>
+      dplyr::filter(!.data$is_redirected_muni) |>
       dplyr::bind_rows(replaced_file_sizes_municip) |>
       dplyr::arrange(dplyr::desc(.data$pub_ts))
     files_table <- files_table_replaced_file_sizes
@@ -366,13 +382,19 @@ spod_available_data_v1 <- function(
       2
     )
 
-    file_sizes <- readRDS(
-      system.file(
-        "extdata",
-        "available_data_v1.rds",
-        package = "spanishoddata"
+    file_sizes_path <- spod_get_v1_meta_path()
+    if (file.exists(file_sizes_path)) {
+      file_sizes <- readRDS(file_sizes_path)
+    } else {
+      # Fallback to empty if not found
+      file_sizes <- tibble::tibble(
+        target_url = character(0),
+        etag = character(0),
+        true_etag = character(0),
+        true_remote_file_size_bytes = numeric(0)
       )
-    )
+    }
+
     files_table <- dplyr::left_join(
       files_table |> dplyr::select(-"file_size_bytes"),
       file_sizes |>
@@ -394,13 +416,19 @@ spod_available_data_v1 <- function(
       ) |>
       dplyr::select(-"true_etag")
   } else {
-    file_sizes <- readRDS(
-      system.file(
-        "extdata",
-        "available_data_v1.rds",
-        package = "spanishoddata"
+    file_sizes_path <- spod_get_v1_meta_path()
+    if (file.exists(file_sizes_path)) {
+      file_sizes <- readRDS(file_sizes_path)
+    } else {
+      # Fallback to empty if not found
+      file_sizes <- tibble::tibble(
+        target_url = character(0),
+        etag = character(0),
+        true_etag = character(0),
+        true_remote_file_size_bytes = numeric(0)
       )
-    )
+    }
+
     if ("file_size_bytes" %in% colnames(files_table)) {
       files_table_no_size <- files_table |> dplyr::select(-"file_size_bytes")
     } else {
@@ -497,6 +525,8 @@ spod_available_data_v1 <- function(
       )
   }
 
+  files_table$is_redirected_muni <- NULL
+
   return(files_table)
 }
 
diff --git a/R/download_data.R b/R/download_data.R
@@ -180,6 +180,13 @@ spod_download <- function(
     ]
   }
 
+  if (nrow(requested_files) == 0 && !isTRUE(ignore_missing_dates)) {
+    stop(
+      "No data files found for the requested criteria (type, zones, and dates) in the metadata.\n",
+      "Please check if the data for these dates is available in the selected format."
+    )
+  }
+
   # compare file sizes
   requested_files <- requested_files |>
     dplyr::mutate(
diff --git a/R/internal-utils.R b/R/internal-utils.R
@@ -592,3 +592,14 @@ spod_assert_package <- function(...) {
 
   rlang::is_installed(...)
 }
+
+#' Get the path to the V1 metadata RDS file
+#' @return A `character` string with the path to the RDS file, or `""` if not found.
+#' @keywords internal
+spod_get_v1_meta_path <- function() {
+  system.file(
+    "extdata",
+    "available_data_v1.rds",
+    package = "spanishoddata"
+  )
+}
diff --git a/tests/testthat/test-check-files.R b/tests/testthat/test-check-files.R
@@ -1,6 +1,3 @@
-library(testthat)
-library(spanishoddata)
-
 # Tests using real fixtures to verify ETag computation and consistency checking logic
 
 test_that("spod_check_files detects inconsistency in fixtures", {
diff --git a/tests/testthat/test-cite.R b/tests/testthat/test-cite.R
@@ -1,6 +1,3 @@
-# Load testthat
-library(testthat)
-
 # --- Tests for spod_cite function ---
 
 test_that("spod_cite with default arguments prints all citations in all formats", {
diff --git a/tests/testthat/test-download.R b/tests/testthat/test-download.R
@@ -1,6 +1,3 @@
-library(testthat)
-library(spanishoddata)
-
 # Use real data directory fixtures
 # Tests integration of metadata cache -> decision logic -> download call
 
diff --git a/tests/testthat/test-duckdb-helpers-coverage.R b/tests/testthat/test-duckdb-helpers-coverage.R
@@ -1,10 +1,7 @@
-library(testthat)
-library(spanishoddata)
-
 # Use setup_test_data_dir helper instead of mocks
 # This ensures we test against the real file structure and logic
 
-test_that("spod_duckdb_od validates version", {
+test_that("spod_duckdb_od creates a valid DuckDB view", {
   expect_error(spod_duckdb_od(ver = 3), "Invalid version number")
 })
 
diff --git a/tests/testthat/test-live-integration.R b/tests/testthat/test-live-integration.R
@@ -1,6 +1,3 @@
-library(testthat)
-library(spanishoddata)
-
 # This test file is designed for scheduled live integration tests on CI.
 # It hits real APIs and S3 buckets to ensure the data provider infrastructure
 # and the package's parsing logic remain compatible.
diff --git a/tests/testthat/test-quick-get-internals.R b/tests/testthat/test-quick-get-internals.R
@@ -1,5 +1,3 @@
-library(spanishoddata)
-
 test_that("spod_get_hmac_secret parses secret from HTML", {
   mock_html <- '
     <html>
diff --git a/tests/testthat/test-regression-v1-meta.R b/tests/testthat/test-regression-v1-meta.R
@@ -0,0 +1,150 @@
+test_that("V1 metadata correctly maps municipios to distritos in local_path", {
+  # Mock read_data_links_xml to return a row with a municipality URL
+
+  mock_meta <- tibble::tibble(
+    target_url = "https://opendata-movilidad.mitma.es/maestra1-mitma-municipios/ficheros-diarios/2021-02/20210201_maestra_1_mitma_municipio.txt.gz",
+    pub_ts = as.POSIXct("2021-03-01", tz = "UTC")
+  )
+
+  # Ensure SPANISH_OD_DATA_DIR is set to avoid warnings
+  withr::local_envvar(c("SPANISH_OD_DATA_DIR" = tempdir()))
+
+  testthat::with_mocked_bindings(
+    {
+      # Call the internal function directly
+      res <- spod_available_data_v1(
+        data_dir = tempdir(),
+        check_local_files = FALSE
+      )
+
+      # Check target_url was substituted
+      expect_match(res$target_url[1], "distrito")
+
+      # Check local_path was generated AFTER substitution (regression test)
+      expect_match(res$local_path[1], "distritos")
+      expect_false(grepl("municipio", res$local_path[1]))
+
+      # Ensure it correctly identifies as origin-destination and municipalities
+      expect_equal(as.character(res$type[1]), "origin-destination")
+      expect_equal(as.character(res$zones[1]), "municipalities")
+    },
+    read_data_links_xml = function(...) mock_meta,
+    .package = "spanishoddata"
+  )
+})
+
+test_that("spod_available_data_v1 handles missing metadata RDS gracefully", {
+  mock_meta <- tibble::tibble(
+    target_url = "https://example.com/test.txt.gz",
+    pub_ts = as.POSIXct("2021-03-01", tz = "UTC")
+  )
+
+  withr::local_envvar(c("SPANISH_OD_DATA_DIR" = tempdir()))
+
+  testthat::with_mocked_bindings(
+    {
+      # Mock spod_get_v1_meta_path to return a non-existent path
+      res <- spod_available_data_v1(data_dir = tempdir(), use_s3 = FALSE)
+
+      # Should still return a table even if sizes are missing
+      expect_s3_class(res, "data.frame")
+      expect_true(nrow(res) > 0)
+    },
+    read_data_links_xml = function(...) mock_meta,
+    spod_get_v1_meta_path = function() "/non/existent/path.rds",
+    .package = "spanishoddata"
+  )
+})
+
+test_that("spod_available_data_v1 correctly redirects file sizes in S3 block", {
+  # Create a dummy RDS file
+  test_dir <- withr::local_tempdir()
+  dummy_rds <- file.path(test_dir, "dummy.rds")
+  saveRDS(
+    tibble::tibble(
+      target_url = character(0),
+      etag = character(0),
+      true_etag = character(0),
+      true_remote_file_size_bytes = numeric(0)
+    ),
+    dummy_rds
+  )
+
+  # Mock S3 return with both a district and a municipality entry
+  mock_s3 <- tibble::tibble(
+    target_url = c(
+      "https://opendata-movilidad.mitma.es/maestra1-mitma-distritos/ficheros-diarios/2021-02/20210201_maestra_1_mitma_distrito.txt.gz",
+      "https://opendata-movilidad.mitma.es/maestra1-mitma-municipios/ficheros-diarios/2021-02/20210201_maestra_1_mitma_municipio.txt.gz"
+    ),
+    pub_ts = rep(as.POSIXct("2021-03-01", tz = "UTC"), 2),
+    file_size_bytes = c(2048, 1024),
+    etag = c("e1", "e2")
+  )
+
+  withr::local_envvar(c("SPANISH_OD_DATA_DIR" = tempdir()))
+
+  testthat::with_mocked_bindings(
+    {
+      res <- spod_available_data_v1(data_dir = tempdir(), use_s3 = TRUE)
+
+      # Both should now point to district URL
+      expect_true(all(grepl("distrito", res$target_url)))
+    },
+    spod_available_data_s3 = function(...) mock_s3,
+    spod_get_v1_meta_path = function() dummy_rds,
+    .package = "spanishoddata"
+  )
+})
+
+test_that("spod_download fails loudly when no data is found (safety switch)", {
+  test_dir <- withr::local_tempdir()
+  withr::local_envvar(c("SPANISH_OD_DATA_DIR" = test_dir))
+
+  # Mock available_data with expected columns but no rows
+  empty_meta <- tibble::tibble(
+    target_url = character(0),
+    local_path = character(0),
+    data_ymd = as.Date(character(0)),
+    file_size_bytes = numeric(0),
+    local_file_size = numeric(0)
+  )
+
+  testthat::with_mocked_bindings(
+    {
+      expect_error(
+        spod_download(
+          type = "od",
+          zones = "dist",
+          dates = "2021-02-01",
+          data_dir = test_dir,
+          ignore_missing_dates = FALSE,
+          quiet = TRUE
+        ),
+        "No data files found for the requested criteria"
+      )
+
+      # Should not error if ignore_missing_dates = TRUE
+      expect_silent(
+        spod_download(
+          type = "od",
+          zones = "dist",
+          dates = "2021-02-01",
+          data_dir = test_dir,
+          ignore_missing_dates = TRUE,
+          quiet = TRUE
+        )
+      )
+    },
+    spod_available_data = function(...) empty_meta,
+    spod_get_valid_dates = function(ver, ...) {
+      if (ver == 1) {
+        return(as.Date("2021-02-01"))
+      }
+      if (ver == 2) {
+        return(as.Date("2022-02-01"))
+      }
+      return(as.Date(character(0)))
+    },
+    .package = "spanishoddata"
+  )
+})

Original file line number	Diff line number	Diff line change
`@@ -180,6 +180,13 @@ spod_download <- function(`
`180`	`180`	`]`
`181`	`181`	`}`
`182`	`182`
	`183`	`+ if (nrow(requested_files) == 0 && !isTRUE(ignore_missing_dates)) {`
	`184`	`+ stop(`
	`185`	`+ "No data files found for the requested criteria (type, zones, and dates) in the metadata.\n",`
	`186`	`+ "Please check if the data for these dates is available in the selected format."`
	`187`	`+ )`
	`188`	`+ }`
	`189`	`+`
`183`	`190`	`# compare file sizes`
`184`	`191`	`requested_files <- requested_files \|>`
`185`	`192`	`dplyr::mutate(`