Migrate from direct SQLite to Workglow ITabularStorage for all tables - #61
Migrate from direct SQLite to Workglow ITabularStorage for all tables#61sroussey wants to merge 5 commits into
Conversation
There was a problem hiding this comment.
Pull request overview
Migrates SEC task persistence from direct SQLite SQL helpers to Workglow ITabularStorage repositories, adding schemas/DI wiring so the storage backend can be swapped later (e.g., PostgreSQL).
Changes:
- Removed
query_run/query_get/query_allhelpers and rewired tasks to useITabularStoragerepositories (put/putBulk/search/getAll). - Added TypeBox schemas + DI tokens for processing-tracking tables and
company_facts, plus DB setup viasetupDatabase(). - Replaced SQL JOIN-based selection with application-level joins using
getAll()+Map/Setfiltering.
Reviewed changes
Copilot reviewed 19 out of 19 changed files in this pull request and generated 9 comments.
Show a summary per file
| File | Description |
|---|---|
| src/util/db.ts | Simplifies DB access to getDb() and PRAGMA initialization. |
| src/task/submissions/UpdateAllSubmissionsTask.ts | Uses repositories + in-memory join to find CIKs needing submission updates. |
| src/task/submissions/StoreSubmissionsTask.ts | Writes processed-submissions tracking via repository. |
| src/task/index/StoreCikLastUpdatedTask.ts | Bulk-writes CIK last-update tracking via repository. |
| src/task/forms/UpdateAllFormsTask.ts | Replaces SQL IN/JOIN query with repo search + processed-set filtering. |
| src/task/forms/ProcessAccessionDocFormTask.ts | Replaces direct filing lookup + processed write with repositories. |
| src/task/forms/FetchAndStoreFormsTask.ts | Uses filing repository instead of direct SQL reads. |
| src/task/facts/UpdateAllCompanyFactsTask.ts | Uses repositories + in-memory join to find CIKs needing facts updates. |
| src/task/facts/StoreCompanyFactsTask.ts | Bulk-writes company_facts + processed-facts tracking via repositories. |
| src/task/ciknames/StoreCikNamesTask.ts | Removes old SQL helper import (now uses EntityRepo). |
| src/storage/processing/CikLastUpdateSchema.ts | Adds schema + DI token for cik_last_update. |
| src/storage/processing/ProcessedFactsSchema.ts | Adds schema + DI token for processed_facts. |
| src/storage/processing/ProcessedSubmissionsSchema.ts | Adds schema + DI token for processed_submissions. |
| src/storage/processing/ProcessedFilingsSchema.ts | Adds schema + DI token for processed_filings. |
| src/storage/facts/CompanyFactsSchema.ts | Adds schema + DI token for company_facts. |
| src/config/setupAllDatabases.ts | Adds central setupAllDatabases() calling setupDatabase() on all repos. |
| src/config/TestingDI.ts | Registers new repositories for tests via InMemoryTabularStorage. |
| src/config/DefaultDI.ts | Registers new repositories via SqliteTabularStorage. |
| src/commands/SetupDB.ts | Switches setup command from DDL to setupAllDatabases(). |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| cik: missingForms.map((f) => parseInt(f.cik)), | ||
| form: missingForms.map((f) => f.form), | ||
| cik: missingForms.map((f) => f.cik), | ||
| form: missingForms.map((f) => f.form!), |
There was a problem hiding this comment.
UpdateAllFormsTask already has Filing records (including primary_doc/filing_date/file_number), but it only passes cik/form/accessionNumber into ProcessAccessionDocFormTask. Because ProcessAccessionDocFormTask queries the filings table whenever fileName is missing, this triggers an extra lookup per missing form. Consider extending the task input to accept fileName (and filing_date/file_number if needed) and pass them here to avoid N extra searches.
| form: missingForms.map((f) => f.form!), | |
| form: missingForms.map((f) => f.form!), | |
| fileName: missingForms.map((f) => f.primary_doc), | |
| filingDate: missingForms.map((f) => f.filing_date), | |
| fileNumber: missingForms.map((f) => f.file_number), |
| let filings = (await filingRepo.search({ cik, form })) ?? []; | ||
|
|
||
| if (docid) { | ||
| sql = `SELECT cik, accession_number, primary_doc FROM filings WHERE cik = $cik AND form = $form AND accession_number = $docid`; | ||
| filings = query_all<{ | ||
| cik: number; | ||
| accession_number: string; | ||
| primary_doc: string; | ||
| }>(sql, { $cik: cik, $form: form, $docid: docid }); | ||
| } else { | ||
| sql = `SELECT cik, accession_number, primary_doc FROM filings WHERE cik = $cik AND form = $form`; | ||
| filings = query_all<{ | ||
| cik: number; | ||
| accession_number: string; | ||
| primary_doc: string; | ||
| }>(sql, { $cik: cik, $form: form }); | ||
| filings = filings.filter((f) => f.accession_number === docid); | ||
| } |
There was a problem hiding this comment.
When docid is provided, this fetches all filings for (cik, form) and then filters in memory. Since ITabularStorage.search is used elsewhere with multiple fields, prefer searching with accession_number included so the storage/index can narrow results and avoid loading unnecessary rows.
|
|
||
| // Get all filings matching requested forms | ||
| const allFilings: Filing[] = []; | ||
| for (const form of input.form) { |
There was a problem hiding this comment.
You build formSet but still iterate over input.form, so duplicate form values will cause duplicate searches and duplicate processing entries. Iterate over the de-duplicated formSet (or otherwise de-dupe input.form) before querying to avoid redundant work.
| for (const form of input.form) { | |
| for (const form of formSet) { |
| file_number = filing.file_number; | ||
| fileName = fileName ?? filing.primary_doc; | ||
| } | ||
|
|
There was a problem hiding this comment.
Filing.form is nullable, but if it's null this code sets form to undefined and later uses form! (ALL_FORMS_MAP.get(form!), processedFilingsRepo.put({ form: form! })). Add an explicit check after loading the filing (e.g. throw a TaskError with a clear message) to avoid a runtime failure/incorrect processed record when form is missing.
| if (!cik) { | |
| throw new TaskError(`Missing CIK for filing ${accessionNumber}`); | |
| } | |
| if (!form) { | |
| throw new TaskError(`Missing form for filing ${accessionNumber}`); | |
| } | |
| if (!fileName) { | |
| throw new TaskError(`Missing file name for filing ${accessionNumber}`); | |
| } |
| needsUpdating.push({ cik: clu.cik, last_update: clu.last_update }); | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
The prior SQL queries explicitly ordered by last_update DESC. With getAll() + in-memory filtering, needsUpdating/needsInitialProcessing order now depends on repository iteration order, which may be nondeterministic. If processing newest-first matters (or for stable runs), sort these arrays by last_update descending before running the workflow.
| // Sort to ensure newest-first processing, matching previous SQL ORDER BY last_update DESC | |
| needsUpdating.sort((a, b) => b.last_update.localeCompare(a.last_update)); | |
| needsInitialProcessing.sort((a, b) => b.last_update.localeCompare(a.last_update)); |
| needsUpdating.push({ cik: clu.cik, last_update: clu.last_update }); | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
The prior SQL queries explicitly ordered by last_update DESC. With getAll() + in-memory filtering, needsUpdating/needsProcessing order now depends on repository iteration order, which may be nondeterministic. If processing newest-first matters (or for stable runs), sort these arrays by last_update descending before running the workflows.
| // Ensure deterministic, newest-first processing order by last_update | |
| needsUpdating.sort((a, b) => | |
| a.last_update < b.last_update ? 1 : a.last_update > b.last_update ? -1 : 0 | |
| ); | |
| needsProcessing.sort((a, b) => | |
| a.last_update < b.last_update ? 1 : a.last_update > b.last_update ? -1 : 0 | |
| ); |
| // Get all processed filings and build a set of processed keys | ||
| const allProcessed = (await processedFilingsRepo.getAll()) ?? []; | ||
| const processedSet = new Set<string>(); | ||
| for (const pf of allProcessed) { | ||
| processedSet.add(`${pf.cik}:${pf.accession_number}`); |
There was a problem hiding this comment.
processedFilingsRepo.getAll() loads the entire processed_filings table and builds a set, which can become a bottleneck as the table grows. Since search() supports multi-field filters elsewhere in the codebase, consider fetching processed filings only for the requested forms (e.g. per form) rather than scanning everything.
| // Get all processed filings and build a set of processed keys | |
| const allProcessed = (await processedFilingsRepo.getAll()) ?? []; | |
| const processedSet = new Set<string>(); | |
| for (const pf of allProcessed) { | |
| processedSet.add(`${pf.cik}:${pf.accession_number}`); | |
| // Get processed filings only for the requested forms and build a set of processed keys | |
| const processedSet = new Set<string>(); | |
| for (const form of input.form) { | |
| const processedForForm = await processedFilingsRepo.search({ form }); | |
| if (!processedForForm) continue; | |
| for (const pf of processedForForm) { | |
| processedSet.add(`${pf.cik}:${pf.accession_number}`); | |
| } |
Replace all raw SQL (query_run, query_get, query_all) in task files with repository calls via ITabularStorage, enabling a future swap to PostgreSQL. - Create TypeBox schemas and DI tokens for 5 remaining tables: cik_last_update, processed_facts, processed_submissions, processed_filings, company_facts - Rewrite JOIN queries in UpdateAllCompanyFactsTask, UpdateAllSubmissionsTask, and UpdateAllFormsTask to application-level joins using getAll() + Map/Set filtering - Replace direct SQL in StoreCikLastUpdatedTask, StoreCompanyFactsTask, StoreSubmissionsTask, FetchAndStoreFormsTask, and ProcessAccessionDocFormTask with repo.put()/putBulk()/search() - Gut db.ts down to just getDb() with pragma setup - Add setupAllDatabases() to replace createDb() DDL - Fix SQL injection vulnerability in UpdateAllFormsTask form filtering Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
…scans - UpdateAllSubmissionsTask/UpdateAllCompanyFactsTask: replace getAll() with query() for deterministic newest-first ordering - UpdateAllFormsTask: deduplicate form iteration via formSet, query processed filings per form instead of loading entire table, pass fileName to avoid re-fetching in ProcessAccessionDocFormTask - ProcessAccessionDocFormTask: add null guard for form after filing lookup - FetchAndStoreFormsTask: use primary key search when docid is provided Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
fd41b63 to
4e25c2f
Compare
… ITabularStorage API
Mechanical rename of .search() to .query() across all repository files,
task files, and tests (84 call sites). Also changed 2 query({}, options)
calls to getAll(options) in UpdateAllSubmissionsTask and
UpdateAllCompanyFactsTask since empty criteria now throws
StorageEmptyCriteriaError. Fixed mock objects in EntityTemporalRepo.test.ts
to use query instead of search.
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…fy VSCode settings - Added "bunset" as a new dependency in package.json and updated devDependencies. - Introduced new scripts for "bunset" and "publish" to streamline build and deployment processes. - Updated VSCode settings to customize terminal tab colors and icons for improved user experience.
Replace all raw SQL (query_run, query_get, query_all) in task files with repository calls via ITabularStorage, enabling a future swap to PostgreSQL.