Skip to content

feat: add WSS ZIP cache - #487

Merged
AndrewBrown-USDA merged 23 commits into
masterfrom
for473-2
Oct 28, 2026
Merged

AndrewBrown-USDA merged 23 commits into
masterfrom
for473-2

Conversation

@AndrewBrown-USDA

@AndrewBrown-USDA AndrewBrown-USDA commented Sep 18, 2026 •

Copy link
Copy Markdown
Collaborator

This PR introduces a caching system for Web Soil Survey ZIP archives in the downloadSSURGO() workflow, along with new utility functions for cache management. For #473

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Copilot review overview

🟡 Changes recommended

Critical download and cache-safety issues remain unresolved.

Review effort: Lite
Findings: 2 High severity · 2 Medium severity · 1 Low severity

Open (5)
What changed in this PR

Adds persistent fiscal-year-organized caching for Web Soil Survey ZIP downloads.

Changes:

  • Adds cache management, listing, cleanup, and download utilities.
  • Integrates caching with downloadSSURGO() and createSSURGO().
  • Updates tests, documentation, exports, and release notes.

Unresolved issues include unsafe forced replacement, incomplete-download handling, cache deduplication, fallback behavior, and documentation corrections.

File Description
vignettes/​local-ssurgo.Rmd Documents persistent cache usage.
tests/​testthat/​test-downloadSSURGO-cache.R Tests cache behavior and fallback scenarios.
R/​WSS_cache.R Implements cache management and download utilities.
R/​createSSURGO.R Integrates caching into download and database workflows.
NEWS.md Records the new cache functionality.
NAMESPACE Exports cache helper functions.
man/​list_WSS_cache.Rd Documents cache inventory.
man/​downloadSSURGO.Rd Documents updated download arguments.
man/​createSSURGO.Rd Updates extraction argument documentation.
man/​clear_WSS_cache.Rd Documents cache cleanup.
Files not reviewed (4)
  • man/clear_WSS_cache.Rd: Generated file
  • man/createSSURGO.Rd: Generated file
  • man/downloadSSURGO.Rd: Generated file
  • man/list_WSS_cache.Rd: Generated file

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Comment thread R/WSS_cache.R Resolved
Comment thread R/createSSURGO.R Resolved
Comment thread R/WSS_cache.R Resolved
Comment thread R/createSSURGO.R Resolved
Comment thread R/createSSURGO.R Resolved
@dylanbeaudette

Copy link
Copy Markdown
Member

I'd like the other soilDB authors to weigh-in on the topic of accepting any genAI suggested changes.

@AndrewBrown-USDA

Copy link
Copy Markdown
Collaborator Author

I will note that having an agent review a PR is not the same as blindly accepting the proposed changes.

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Copilot review overview

🟡 Changes recommended

Unresolved cache reliability and extraction-isolation issues include a critical risk of mixing outputs across requests.

Review effort: Lite
Findings: 1 High severity

Open (1)
Resolved since last review (5)
Files not reviewed (4)
  • man/clear_WSS_cache.Rd: Generated file
  • man/createSSURGO.Rd: Generated file
  • man/downloadSSURGO.Rd: Generated file
  • man/list_WSS_cache.Rd: Generated file
Previously missed (4)

In code that hasn't changed since last review

Medium severity Failed initial downloads leave corrupt destination files

R/​WSS_cache.R:160

For a first download (force = FALSE and no existing destination), curl_download() writes directly to destfile. If the transfer fails after creating a partial file, the error path does not remove it because tmp_destfile equals destfile; the next call then returns that corrupt ZIP at line 153 without retrying. Always download to a temporary file and rename it into place only after success, as is already done for forced replacements.

Medium severity Multiple fiscal years are matched incorrectly

R/​WSS_cache.R:219

fiscal_year is normalized element-wise, but == only works correctly for a single requested year; passing multiple years (for example list_WSS_cache(fiscal_year = c(2023, 2024))) recycles the shorter vector and can select the wrong rows. Use set membership for the normalized values so all requested fiscal years are honored.

Medium severity Fallback cache selection does not support LIKE predicates

R/​createSSURGO.R:166

The documented WHERE form areasymbol LIKE 'KS%' cannot use this fallback: .extract_wss_areasymbols() returns KS%, but .wss_cache_select() treats areasymbols as exact values, so cached KS### entries are never selected when the remote metadata query fails. Please make the fallback evaluate supported WHERE predicates against the cache inventory (at least SQL LIKE), or limit this path to exact area-symbol inputs and report that limitation.

Low severity Document NULL exdir behavior in createSSURGO

R/​createSSURGO.R:308

Making exdir optional changes the public behavior, but the @param exdir documentation still describes only an explicitly supplied path and does not say that NULL selects the latest fiscal-year WSS cache directory (or errors when none exists). Update the roxygen text so the generated createSSURGO help documents this new default.

Comment thread R/createSSURGO.R Resolved

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Copilot review overview

🟡 Changes recommended

Critical API and area-filtering issues, plus unresolved cache fallback and replacement problems, block approval.

Review effort: Lite
Findings: 2 High severity · 1 Medium severity

Open (3)
Resolved since last review (1)
Files not reviewed (4)
  • man/clear_WSS_cache.Rd: Generated file
  • man/createSSURGO.Rd: Generated file
  • man/downloadSSURGO.Rd: Generated file
  • man/list_WSS_cache.Rd: Generated file

Comment thread R/createSSURGO.R Resolved
Comment thread R/createSSURGO.R Resolved
Comment thread R/WSS_cache.R Resolved
@AndrewBrown-USDA

Copy link
Copy Markdown
Collaborator Author

Test failures are unrelated to this PR (fetchSCAN() failures); those seem to be persisting so I will merge this shortly and focus on some improvements to SCAN testing in another PR

@StephenRoecker-USDA

Copy link
Copy Markdown
Member

I'd like the other soilDB authors to weigh-in on the topic of accepting any genAI suggested changes.
@dylanbeaudette

I don't have a problem with genAI changes in general. I think all the same best practices apply. Targeted changes, presumed ownership of individual contributors functions, etc.. Where it gets sticky is functions that are reused internally by packages, such as uncode, that have to work. In such cases as usual it requires sufficient testing and eyeballs.

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🟡 Changes recommended

Fallback and filtering can select incorrect or incomplete surveys, and database writes can bypass the caller’s append setting.

1 open issue
4 previously missed issues

In code that hasn't changed since last review

Medium severity Validate all requested areas are covered by fallback cache

R/​createSSURGO.R:168

When metadata lookup fails, requesting CA067 and CA077 succeeds with only CA067 if that is the only cached area. This bypasses the partial-download check and can produce an incomplete database. Check coverage of explicitly requested area symbols before accepting the fallback and report any missing symbols.

Medium severity Select newest cached archive per area for latest_only

R/​createSSURGO.R:169

latest_only = TRUE keeps one ZIP per area and fiscal year. If CA067 has FY23 and FY24 archives, metadata lookup failure selects both: default extraction errors about multiple directories, while an explicit exdir extracts both versions into the same destination. Select the newest cached archive per area here, preserving the per-year behavior used by cache cleanup.

Medium severity Match extraction paths relative to exdir

R/​createSSURGO.R:258

createSSURGO() passes full paths here, so every ancestor directory becomes an area identifier. With exdir = '/exports/CA067', filtering for CA067 also matches /exports/CA067/CA077/tabular/mapunit.txt through the common extraction root and imports an unrelated survey. Pass paths relative to exdir to the matcher, excluding the extraction root and its ancestors.

Medium severity Prevent unintended appends after table initialization failure

R/​createSSURGO.R:712

i > 1 permits appending to tables that existed before this call even when append = FALSE. With a supplied connection, an existing table, and overwrite = FALSE, the first write fails inside try(), but later files are appended and committed anyway. Track successful table initialization within this invocation and auto-append only after it succeeds, or when append = TRUE is explicit.

3 resolved since last review
4 files not reviewed
  • man/clear_WSS_cache.Rd: Generated file
  • man/createSSURGO.Rd: Generated file
  • man/downloadSSURGO.Rd: Generated file
  • man/list_WSS_cache.Rd: Generated file

Review effort: Balanced

Comment thread R/createSSURGO.R Resolved
@AndrewBrown-USDA
AndrewBrown-USDA merged commit c63c955 into master Oct 28, 2026
5 checks passed
@AndrewBrown-USDA
AndrewBrown-USDA deleted the for473-2 branch October 31, 2026 01:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

downloadSSURGO(): add support for local WSS ZIP cache

4 participants