fix: escape CSV fields and neutralise formulas in both exporters (#1052) - #1057
fix: escape CSV fields and neutralise formulas in both exporters (#1052)#1057MOHITKOURAV01 wants to merge 2 commits into
Conversation
…tya8369#1052) Both CSV writers built a row with `values.join(delimiter)`, which is correct only while no value contains the delimiter, a quote or a newline. A pollutant labelled `PM2.5, respirable` produced one extra field on that row and nothing else -- no parse error, every column after it simply read one place left. On the compliance export that is a document going to a regulator with values under the wrong headings. Adds `src/utils/csv.js` and routes both exporters through it: - a field is quoted when it contains the delimiter, a quote, CR or LF, and embedded quotes are doubled (RFC 4180 s2.7) - a value beginning `=`, `+`, `-`, `@`, tab or CR is prefixed with an apostrophe, which every major spreadsheet reads as "this cell is text". Quoting alone does not help here: the quotes are consumed by the parser before the cell text is examined, so `=HYPERLINK(...)` in a station name was a live formula in whoever opened the file - null, undefined and NaN become an empty cell rather than the word `undefined` - a Date is written as ISO rather than via toString(), which is locale-dependent and contains commas `exportToCSV` also no longer throws when `report.exceedances` is absent -- that was a TypeError reaching the user as a Download button that did nothing -- and its "Total Exceedances" line is now counted from the rows it actually wrote, so the header cannot disagree with the table under it. Fixes Aditya8369#1052
|
@MOHITKOURAV01 is attempting to deploy a commit to the Aditya Mahajan's projects Team on Vercel. A member of the Team first needs to authorize it. |
Thank You for Your Contribution! 🎉Hi @MOHITKOURAV01, Thank you for opening this Pull Request and contributing to our project. We truly appreciate your efforts.
The maintainer @Aditya8369 will review your PR shortly! Happy Contributing! 🚀 |
|
@MOHITKOURAV01 conflicts |
# Conflicts: # src/services/historicalDataService.js
|
Merged One conflict, in import { formatRow, formatTable } from '../utils/csv';and #1020 landed on import { localDayKey } from '../utils/localDay';on the same line. Both kept — the two changes are independent (CSV field escaping vs. local-day bucket keys), and both imports are still used, three call sites each.
|
Fixes #1052
The problem
Both CSV writers built a row the same way:
which is correct only while no value contains the delimiter, a quote or a newline. A pollutant labelled
PM2.5, respirableproduces one extra field on that row and nothing else — no parse error, no warning; every column after it simply reads one place left. On the compliance export that is a document going to a regulator with values under the wrong headings.Choosing
;for comma-decimal locales (#736) fixed one class of value that can contain the delimiter. It does nothing for a;inside a value, or a quote, or a newline.The fix
A shared writer at
src/utils/csv.js, used byreportExporter.exportToCSVandhistoricalDataService.formatHistoricalCSV.Quoting (RFC 4180). A field is quoted when it contains the delimiter, a
", CR or LF; embedded quotes are doubled. Quoting is applied only when needed, so an ordinary export still reads as an ordinary file in a text editor.Formula neutralisation. A cell whose text starts with
=,+,-,@, tab or CR is evaluated by Excel, LibreOffice and Google Sheets alike. A station name or severity note beginning=HYPERLINK(...)was a live formula in whoever opened the file.This is a separate concern from quoting, and worth being explicit about: quoting does not prevent it. The quotes are consumed by the parser before the cell text is examined. The fix is an apostrophe prefix, which every major spreadsheet reads as "the rest of this cell is text" — a human sees the original value, the application does not run it. Tab and CR are in the trigger list because a leading one is stripped during import, which exposes whatever follows.
Absent values.
null,undefinedandNaNbecome an empty cell. They were template-interpolated, so a report missinggeneratedAtexported the literal stringundefined— worse than blank, because it looks like data. A genuine0is preserved; it is a reading like any other.Dates are written as ISO rather than through
toString(), whose output is locale- and timezone-dependent and contains commas.Also in
exportToCSVexceedancesis missing.report.exceedances.map(...)on a report the API returned without the key was aTypeError, reaching the user as a Download button that did nothing. A report with no exceedances and a report missing the field are both "no rows to write".report.totalExceedances, so the header cannot disagree with the table under it.Report ID: ...and friends were built by interpolation and broke in the same way an unescaped field breaks a row. Label and value are now emitted as two cells, so the pair survives as data rather than one string to split by eye.exportToJSON(undefined)returns'null'rather thanundefined— the latter is not a string, and reachesnew Blob([undefined])as the text "undefined".Verification
src/utils/csv.test.js— 25 tests. The assertions round-trip through a small RFC 4180 reader rather than checking the output string, because splitting on the delimiter is exactly the bug under test: a correctly quoted row would still split inside its quotes and look broken.src/utils/reportExporter.test.js— 13 tests, at the level a reviewer of the compliance export cares about: every data row is the header's width,PM2.5, respirablecomes back out of column 2 intact, a recorded0survives, a formula is defanged, a missingexceedancesarray does not throw.The 14 existing
historicalDataService.test.jstests pass against the new writer, and its export now escapes:One caveat on CI:
historicalDataService.test.jscannot collect onmainright now — #1049 makes that module throw on import. The 14 tests above were run with #1054 applied locally; they will go green on this branch as soon as #1054 lands. Nothing in this PR causes or worsens that failure.Also removed an unused
catch (e)binding ingetDelimiterForLocale— a pre-existing ESLint error in a file this PR already touches.npx eslintis clean on all five changed files.