Skip to content

Fix houses.zip file creation in Web.API project - #56

Open
skorpionreser wants to merge 8 commits into
devfrom
fix/43/fix-houses.zip-file-creation-in-Web.API
Open

Fix houses.zip file creation in Web.API project#56
skorpionreser wants to merge 8 commits into
devfrom
fix/43/fix-houses.zip-file-creation-in-Web.API

Conversation

@skorpionreser

@skorpionreser skorpionreser commented Aug 13, 2026

Copy link
Copy Markdown
Contributor

JIRA

Code reviewers

Second Level Review

Summary of issue

The Ukrposhta postal dataset was tracked as both the houses.zip archive and the unpacked Streetcode/Streetcode.DAL/houses.csv file. The Docker build depended on a stale repository copy of the dataset even though the application downloads the latest data from Ukrposhta at runtime.

The archive and extraction paths were also tied to the application's working environment.

Summary of change

The tracked dataset files were removed from the current project state. Their blobs remain in the existing Git history. Removing them requires a coordinated history rewrite and is outside the scope of this change. The latest archive is now downloaded from Ukrposhta to temporary storage at runtime and removed after processing. The obsolete houses.zip copy instruction was also removed from the Dockerfile.

The current data.csv workflow is preserved between parsing jobs within the same application deployment. Persistence across rebuilds or container recreation is not guaranteed and is outside the scope of this change.

CHECK LIST

  • СI passed
  • Сode coverage >=95%
  • PR is reviewed manually again (to make sure you have 100% ready code)
  • All reviewers agreed to merge the PR
  • I've checked new feature as logged in and logged out user if needed
  • PR meets all conventions

Closes #43

@DrFaust555 DrFaust555 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

The problem is scale. houses.zip was 5,490,356 bytes. Streetcode/Streetcode.DAL/houses.csv is still tracked at 57,009,829 bytes - the same data unpacked, ten times bigger. Until now it could at least be picked up, since extractTo pointed at the Streetcode.DAL folder and the lookup is Directory.GetFiles(extractTo).First(f => f.EndsWith("houses.csv")). After this change nothing reads it at all. The description says the goal is to stop carrying a large binary in the repository, so leaving 57 MB behind doesn't get there - either delete it here or split it into its own task and say so.

On persistence: "data.csv is preserved for subsequent parsing runs" holds within a running app, but AppContext.BaseDirectory is the build output folder - /app in the container - so a rebuild or a recreated container resets it. That matters because SaveToponymsToDbAsync truncates the Toponyms table on every run and remainsToParse is capped at Take(20), so the first run in a fresh container wipes the toponyms and writes at most twenty rows back. Not a regression - the old path was just as ephemeral - but either soften the claim or put the folder somewhere that survives a redeploy.

Small one: neither .gitignore rule matches a path the code actually uses. The archive goes to Path.GetTempPath(), and the data folder sits under bin/, which is already ignored.

@DrFaust555
DrFaust555 self-requested a review August 14, 2026 10:29

@DrFaust555 DrFaust555 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

The stated goal is not reached by deleting at the tip. The description says the file "increased the repository size". Deleting it in a commit does not remove it — both blobs stay in history and every clone still transfers them. So either raise it as its own task, or restate the goal to what this change does deliver: the build stops shipping a stale binary and the data is fetched fresh at runtime. As written the summary promises something the diff cannot do.

A failed run now reports success. ProcessCsvFileAsync locates the dataset with Directory.GetFiles(extractTo).First(fName => fName.EndsWith("houses.csv")). Until this change extractTo pointed at Streetcode.DAL, where a copy was committed, so the lookup found a file no matter what the download did. The folder now starts empty on every run. If the archive layout changes — a nested folder, a renamed entry — GetFiles is not recursive, First() throws InvalidOperationException, and the catch (Exception ex) { Console.WriteLine(...) } in ParseZipFileFromWebAsync swallows it. Hangfire records the job as succeeded and the monthly schedule moves on. Use FirstOrDefault with an explicit failure, and let ParseZipFileFromWebAsync rethrow so the job fails and retries instead of logging into an empty container stream.

Per-run isolation is applied to the archive but not to the extraction. zipPath gets a Guid, dataDirectory is a fixed path. Program.cs registers both BackgroundJob.Schedule(..., TimeSpan.FromMinutes(1)) and RecurringJob.AddOrUpdate(..., Cron.Monthly), so a restart during the monthly run gives two concurrent executions. They extract into the same directory with overwriteFiles: true, both write data.csv, and each deletes houses.csv at the end via deleteFile: true — the loser of that race hits the First() above or reads a half-overwritten file. Either make the extraction directory per-run as well, or put [DisableConcurrentExecution] on the method.

Quality Gate is failing: 0.0% coverage on new code against a required 80%. There is no test file for WebParsingUtils anywhere in Streetcode.XUnitTest. The path resolution is currently inlined in ParseZipFileFromWebAsync, which cannot be tested without hitting the network; pulling it into a separate member makes the new logic coverable and is most of what the gate is asking for. Two checklist items — CI passed and Code coverage >=95% — are still unticked, and they match reality.

Smaller: AppContext.BaseDirectory is the publish output — /app in the container, bin/... locally — so the app writes its working data into its own binary directory. The description settles the question of how long that data lives; the choice of location is separate, and IHostEnvironment.ContentRootPath or a configured path keeps runtime data out of the build output. The branch is 13 commits behind dev. The body starts with a stray dev line above the ## JIRA heading.

@DrFaust555
DrFaust555 self-requested a review August 16, 2026 04:05
DrFaust555
DrFaust555 previously approved these changes Aug 16, 2026

@DrFaust555 DrFaust555 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Fix before merge:

  1. WebParsingUtilsTests.cs sits in Utils/ but declares namespace Streetcode.XUnitTest.UtilsTests. Rename the namespace to match the folder.
  2. Add a trailing newline to WebParsingUtilsTests.cs and .gitignore.

@DrFaust555
DrFaust555 self-requested a review August 21, 2026 16:52
DrFaust555
DrFaust555 previously approved these changes Aug 21, 2026

@DrFaust555 DrFaust555 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

  1. WebParsingUtilsTests.cs, lines 1-4: three open Sonar issues. Add the
    file header and move the using directives inside the namespace block,
    the same way the other test files in Streetcode.XUnitTest do.

  2. WebParsingUtils.cs, lines 100-104 and 128-132: catch (Exception) that
    only writes ex.Message to Console and rethrows. Remove both blocks;
    Hangfire logs the failed job with the full exception and stack trace.

@DrFaust555 DrFaust555 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Fresh deployment can destroy Toponyms data.
data.csv is stored in non-persistent ContentRoot/Data/WebParsing, and the job is run a minute after each non-Local start.
When data.csv is missing, the code takes only 20 rows including the header, i.e. a maximum of 19 addresses. Then it deletes all Toponyms and commits the deletion separately, after which it tries to insert these 19 records. In case of an error, the table may remain empty.
Simply writing in the PR that persistence between rebuilds is not guaranteed is not enough: the database survives the redeploy, but the progress file does not.

Geocoding errors can also clear the table.
After two unsuccessful attempts, the coordinates remain null, but the row is still written to data.csv. After clearing the table, decimal.Parse crashes, the exception is swallowed, and Toponym is not restored.

The new tests only check path construction and file search. A script with an empty runtime directory is needed, which proves that an incomplete or invalid download does not clear the database.

@sonarqubecloud

Copy link
Copy Markdown

Quality Gate Failed Quality Gate failed

Failed conditions
0.0% Coverage on New Code (required ≥ 80%)

See analysis details on SonarQube Cloud

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.

[Bug] Fix houses.zip file creation in Web.API project

2 participants