Skip to content

Commit 2c2dd7f

Browse files
committed
Address third round of review feedback
- Fix the header probe's completeness check. It tested the probe for any line break, but blank lines before the header contribute breaks of their own, so a header running past the 1 MB probe could still look terminated and be handed back truncated, silently yielding wrong column names. The check now asks whether the header line's own terminator is present, via a new `hasCompleteHeaderLine`. The line scan is factored into a shared `headerLineBounds` so that and `firstDelimitedTextLine` cannot drift. - Expose `DelimitedTextBody.rows` as the generator itself rather than a `() => Generator` factory. The factory shape read as if it restarted the scan, when calling it twice would in fact split rows between the two consumers; returning the generator makes the single-use contract visible at each call site.
1 parent 16f4e63 commit 2c2dd7f

3 files changed

Lines changed: 63 additions & 12 deletions

File tree

apps/geolibre-desktop/src/lib/delimited-text.ts

Lines changed: 42 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -87,7 +87,7 @@ export function parseDelimitedTextLayer(
8787
}
8888
if (!wantsLatitude && !wantsLongitude) {
8989
const tableFeatures: Feature<Geometry | null, GeoJsonProperties>[] = [];
90-
for (const row of body.rows()) {
90+
for (const row of body.rows) {
9191
tableFeatures.push({
9292
type: "Feature",
9393
geometry: null,
@@ -124,7 +124,7 @@ export function parseDelimitedTextLayer(
124124
let totalRows = 0;
125125
const features: Feature<Point, GeoJsonProperties>[] = [];
126126

127-
for (const row of body.rows()) {
127+
for (const row of body.rows) {
128128
totalRows += 1;
129129
// For a projected CRS, treat a bare comma as a thousands separator (large
130130
// easting/northing) rather than a decimal point.
@@ -188,7 +188,7 @@ export function parseDelimitedTextRows(
188188

189189
const fields = body.fields;
190190
const rows: Record<string, string>[] = [];
191-
for (const row of body.rows()) {
191+
for (const row of body.rows) {
192192
const record: Record<string, string> = {};
193193
fields.forEach((field, index) => {
194194
record[field] = (row[index] ?? "").trim();
@@ -315,12 +315,14 @@ function* iterateDelimitedRows(text: string, delimiter: string): Generator<strin
315315
* dropped. Returns `null` when the text has no header row or no data rows, so
316316
* each caller can raise its own error.
317317
*
318-
* `rows()` streams from the same underlying scan and so can only be consumed
319-
* once; that is what keeps a large file from being held in memory twice.
318+
* `rows` is the live generator over the same underlying scan, not a factory that
319+
* restarts it: consuming it twice would split the rows between the consumers
320+
* rather than replay them. It is exposed as the generator itself so that
321+
* single-use contract is visible at every call site.
320322
*/
321323
interface DelimitedTextBody {
322324
fields: string[];
323-
rows: () => Generator<string[]>;
325+
rows: Generator<string[]>;
324326
}
325327

326328
function readDelimitedTextBody(text: string, delimiter: string): DelimitedTextBody | null {
@@ -338,10 +340,10 @@ function readDelimitedTextBody(text: string, delimiter: string): DelimitedTextBo
338340

339341
return {
340342
fields: uniqueFieldNames(headerRow.map((field) => field.trim())),
341-
rows: function* () {
343+
rows: (function* () {
342344
yield firstDataRow;
343345
for (let row = nextRow(); row !== null; row = nextRow()) yield row;
344-
},
346+
})(),
345347
};
346348
}
347349

@@ -464,6 +466,35 @@ export function countDelimitedTextRows(text: string, delimiter: string): number
464466
* blank.
465467
*/
466468
export function firstDelimitedTextLine(text: string): string {
469+
const bounds = headerLineBounds(text);
470+
return bounds ? text.slice(bounds.start, bounds.end) : "";
471+
}
472+
473+
/**
474+
* Whether `text` holds a *complete* header line, meaning the first non-blank
475+
* line ends at a line break within `text` rather than running off its end.
476+
*
477+
* A caller that read only a prefix of a file uses this to tell "the header fits
478+
* in what I read" from "my prefix was cut mid-header". Merely testing the
479+
* prefix for a line break is not equivalent: leading blank lines contribute
480+
* line breaks of their own, so a header that overruns the prefix can still look
481+
* terminated.
482+
*
483+
* @param text - A prefix of a delimited file, optionally starting with a BOM.
484+
* @returns True when the header line's own terminator is present.
485+
*/
486+
export function hasCompleteHeaderLine(text: string): boolean {
487+
return headerLineBounds(text)?.terminated ?? false;
488+
}
489+
490+
/** The first non-blank line's span, and whether its terminator was reached. */
491+
interface HeaderLineBounds {
492+
start: number;
493+
end: number;
494+
terminated: boolean;
495+
}
496+
497+
function headerLineBounds(text: string): HeaderLineBounds | null {
467498
let start = contentStart(text);
468499
while (start < text.length) {
469500
// Blankness is tracked during the same scan that finds the terminator, so
@@ -477,14 +508,14 @@ export function firstDelimitedTextLine(text: string): string {
477508
if (!hasContent && !isTrimmableCode(code, text, end)) hasContent = true;
478509
end += 1;
479510
}
480-
if (hasContent) return text.slice(start, end);
481-
if (end >= text.length) return "";
511+
if (hasContent) return { start, end, terminated: end < text.length };
512+
if (end >= text.length) return null;
482513
start =
483514
text.charCodeAt(end) === CARRIAGE_RETURN_CODE && text.charCodeAt(end + 1) === LINE_FEED_CODE
484515
? end + 2
485516
: end + 1;
486517
}
487-
return "";
518+
return null;
488519
}
489520

490521
/**

apps/geolibre-desktop/src/lib/tauri-io.ts

Lines changed: 5 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -26,6 +26,7 @@ import {
2626
detectCoordinateFields,
2727
detectDelimitedTextDelimiter,
2828
firstDelimitedTextLine,
29+
hasCompleteHeaderLine,
2930
parseDelimitedTextFields,
3031
parseDelimitedTextLayer,
3132
} from "./delimited-text";
@@ -654,7 +655,10 @@ const DELIMITED_TEXT_HEADER_PROBE_BYTES = 1024 * 1024;
654655
async function readDelimitedHeaderText(file: File): Promise<string> {
655656
if (file.size <= DELIMITED_TEXT_HEADER_PROBE_BYTES) return file.text();
656657
const probe = await file.slice(0, DELIMITED_TEXT_HEADER_PROBE_BYTES).text();
657-
return probe.includes("\n") ? probe : file.text();
658+
// Deliberately not "does the probe contain a line break": blank lines before
659+
// the header contribute breaks of their own, so a header that overruns the
660+
// probe would still look terminated and be handed back truncated.
661+
return hasCompleteHeaderLine(probe) ? probe : file.text();
658662
}
659663

660664
/**

tests/data-parsing.test.ts

Lines changed: 16 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -6,6 +6,7 @@ import {
66
detectCoordinateFields,
77
detectDelimitedTextDelimiter,
88
firstDelimitedTextLine,
9+
hasCompleteHeaderLine,
910
parseCoordinate,
1011
parseDelimitedTextFields,
1112
parseDelimitedTextLayer,
@@ -367,6 +368,21 @@ describe("delimited text row counting and header slicing", () => {
367368
assert.equal(firstDelimitedTextLine("a,b"), "a,b");
368369
});
369370

371+
it("reports whether a prefix holds the header's own terminator", () => {
372+
assert.equal(hasCompleteHeaderLine("a,b\n1,2"), true);
373+
assert.equal(hasCompleteHeaderLine("a,b\r\n1,2"), true);
374+
assert.equal(hasCompleteHeaderLine("a,b\r1,2"), true);
375+
// Cut mid-header: the prefix ends before any terminator.
376+
assert.equal(hasCompleteHeaderLine("a,b,c"), false);
377+
// The trap this exists for: the blank line supplies a line break, but the
378+
// header after it is still unterminated, so a plain newline test would
379+
// wrongly call this prefix complete.
380+
assert.equal(hasCompleteHeaderLine("\n\na,b,c"), false);
381+
assert.equal(hasCompleteHeaderLine("\n\na,b,c\n1,2,3"), true);
382+
assert.equal(hasCompleteHeaderLine(""), false);
383+
assert.equal(hasCompleteHeaderLine("\n \n"), false);
384+
});
385+
370386
it("ends the header at a bare CR, so a classic-Mac file is not one long line", () => {
371387
const bareCr = "name,longitude,latitude\rA,-78.6,35.7\rB,-80.1,36.2\r";
372388
assert.equal(firstDelimitedTextLine(bareCr), "name,longitude,latitude");

0 commit comments

Comments
 (0)