-
Notifications
You must be signed in to change notification settings - Fork 123
fix: guarantee db is empty before performing a replay #1374
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Merged
Merged
Changes from all commits
Commits
Show all changes
10 commits
Select commit
Hold shift + click to select a range
2e9e56c
feat: add event-replay test suite
rafaelcr 8d8200d
test: bns genesis block
rafaelcr 292fa27
chore: rename tsv to mainnet
rafaelcr 814a767
Merge branch 'beta' into feat/event-replay-tests
rafaelcr 18188bc
test: export import cycle
rafaelcr 169f631
chore: reduce mainnet tsv size
rafaelcr c45b252
fix: db has data check
rafaelcr f275171
fix: drop views first, tables second
rafaelcr a178bed
feat: friendly error when migration cycle failed
rafaelcr c60d9ca
Merge branch 'develop' into feat/event-replay-tests
rafaelcr File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,15 @@ | ||
module.exports = { | ||
preset: 'ts-jest', | ||
rootDir: 'src', | ||
testMatch: ['<rootDir>/tests-event-replay/**/*.ts'], | ||
testPathIgnorePatterns: [ | ||
'<rootDir>/tests-event-replay/setup.ts', | ||
'<rootDir>/tests-event-replay/teardown.ts', | ||
], | ||
collectCoverageFrom: ['<rootDir>/**/*.ts'], | ||
coveragePathIgnorePatterns: ['<rootDir>/tests*'], | ||
coverageDirectory: '../coverage', | ||
globalSetup: '<rootDir>/tests-event-replay/setup.ts', | ||
globalTeardown: '<rootDir>/tests-event-replay/teardown.ts', | ||
testTimeout: 20000, | ||
}; |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,101 @@ | ||
import * as fs from 'fs'; | ||
import { exportEventsAsTsv, importEventsFromTsv } from '../event-replay/event-replay'; | ||
import { PgWriteStore } from '../datastore/pg-write-store'; | ||
import { dangerousDropAllTables, runMigrations } from '../datastore/migrations'; | ||
import { databaseHasData } from '../datastore/event-requests'; | ||
import { getPgClientConfig } from '../datastore/connection-legacy'; | ||
|
||
describe('import/export tests', () => { | ||
let db: PgWriteStore; | ||
|
||
beforeEach(async () => { | ||
process.env.PG_DATABASE = 'postgres'; | ||
db = await PgWriteStore.connect({ | ||
usageName: 'tests', | ||
withNotifier: false, | ||
skipMigrations: true, | ||
}); | ||
}); | ||
|
||
test('event import and export cycle', async () => { | ||
// Import from mocknet TSV | ||
await importEventsFromTsv('src/tests-event-replay/tsv/mocknet.tsv', 'archival', true, true); | ||
const chainTip = await db.getUnanchoredChainTip(); | ||
expect(chainTip.found).toBe(true); | ||
expect(chainTip.result?.blockHeight).toBe(28); | ||
expect(chainTip.result?.indexBlockHash).toBe( | ||
'0x76cd67a65c0dfd5ea450bb9efe30da89fa125bfc077c953802f718353283a533' | ||
); | ||
expect(chainTip.result?.blockHash).toBe( | ||
'0x7682af212d3c1ef62613412f9b5a727269b4548f14eca2e3f941f7ad8b3c11b2' | ||
); | ||
|
||
// Export into temp TSV | ||
const tmpDir = 'src/tests-event-replay/.tmp'; | ||
try { | ||
fs.mkdirSync(tmpDir); | ||
} catch (error: any) { | ||
if (error.code != 'EEXIST') throw error; | ||
} | ||
const tmpTsvPath = `${tmpDir}/export.tsv`; | ||
await exportEventsAsTsv(tmpTsvPath, true); | ||
|
||
// Re-import with exported TSV and check that chain tip matches. | ||
try { | ||
await importEventsFromTsv(`${tmpDir}/export.tsv`, 'archival', true, true); | ||
const newChainTip = await db.getUnanchoredChainTip(); | ||
expect(newChainTip.found).toBe(true); | ||
expect(newChainTip.result?.blockHeight).toBe(28); | ||
expect(newChainTip.result?.indexBlockHash).toBe( | ||
'0x76cd67a65c0dfd5ea450bb9efe30da89fa125bfc077c953802f718353283a533' | ||
); | ||
expect(newChainTip.result?.blockHash).toBe( | ||
'0x7682af212d3c1ef62613412f9b5a727269b4548f14eca2e3f941f7ad8b3c11b2' | ||
); | ||
} finally { | ||
fs.rmSync(tmpDir, { force: true, recursive: true }); | ||
} | ||
}); | ||
|
||
test('import with db wipe options', async () => { | ||
// Migrate first so we have some data. | ||
const clientConfig = getPgClientConfig({ usageName: 'cycle-migrations' }); | ||
await runMigrations(clientConfig, 'up', {}); | ||
await expect( | ||
importEventsFromTsv('src/tests-event-replay/tsv/mocknet.tsv', 'archival', false, false) | ||
).rejects.toThrowError('contains existing data'); | ||
|
||
// Create strange table | ||
await db.sql`CREATE TABLE IF NOT EXISTS test (a varchar(10))`; | ||
await expect( | ||
importEventsFromTsv('src/tests-event-replay/tsv/mocknet.tsv', 'archival', true, false) | ||
).rejects.toThrowError('migration cycle failed'); | ||
|
||
// Force and test | ||
await expect( | ||
importEventsFromTsv('src/tests-event-replay/tsv/mocknet.tsv', 'archival', true, true) | ||
).resolves.not.toThrow(); | ||
}); | ||
|
||
test('db contains data', async () => { | ||
const clientConfig = getPgClientConfig({ usageName: 'cycle-migrations' }); | ||
await runMigrations(clientConfig, 'up', {}); | ||
|
||
// Having tables counts as having data as this may change across major versions. | ||
await expect(databaseHasData()).resolves.toBe(true); | ||
|
||
// Dropping all tables removes everything. | ||
await dangerousDropAllTables({ acknowledgePotentialCatastrophicConsequences: 'yes' }); | ||
await expect(databaseHasData()).resolves.toBe(false); | ||
|
||
// Cycling migrations leaves the `pgmigrations` table. | ||
await runMigrations(clientConfig, 'up', {}); | ||
await runMigrations(clientConfig, 'down', {}); | ||
await expect(databaseHasData()).resolves.toBe(true); | ||
await expect(databaseHasData({ ignoreMigrationTables: true })).resolves.toBe(false); | ||
}); | ||
|
||
afterEach(async () => { | ||
await db?.close(); | ||
}); | ||
}); |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,11 @@ | ||
import { loadDotEnv } from '../helpers'; | ||
|
||
// ts-unused-exports:disable-next-line | ||
export default (): void => { | ||
console.log('Jest - setup..'); | ||
if (!process.env.NODE_ENV) { | ||
process.env.NODE_ENV = 'test'; | ||
} | ||
loadDotEnv(); | ||
console.log('Jest - setup done'); | ||
}; |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,5 @@ | ||
// ts-unused-exports:disable-next-line | ||
export default (): void => { | ||
console.log('Jest - teardown'); | ||
console.log('Jest - teardown done'); | ||
}; |
Large diffs are not rendered by default.
Oops, something went wrong.
Large diffs are not rendered by default.
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there a way to constrain this query to the database specified in
clientConfig.database
? Otherwise, it seems like this could drop everything from the entire postgres instance rather than only the specific database.The previous code was marked as "dangerous"/"unsafe" because it's conceivable that devs might also use the same database for their app-specific data, e.g. something like a
stacks_blockchain_api
db with tables like:So the idea behind requiring that
--force
flag for wiping is that devs might think twice about this situation. However, it seems like the above code would drop data even if devs put their app-specific data in a separate database yet still in the same postgres instance.But maybe that is prevented because of implicit database scoping setup in the sql connection? Idk, just want to verify the behavior here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The
pg_class
system catalog is actually database-specific. There are some which are shared butpg_class
is not:https://www.postgresql.org/docs/14/catalogs-overview.html
This is a great point, though, that I think we should definitely discourage somehow. The difficulty with this is that when we change schemas across API version we'd need to somehow keep track of historical migrations to validate that we're only dropping things the API created as opposed to the developer.
I could expand the warning message we display during replay to explain this situation (i.e. check for empty DB, if not, ask the user to use --wipe-db --force but also explain they shouldn't keep other data in the same DB)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Got it, I think in that case this PR is good to go. The extra log warning sounds helpful but doesn't have to be in the PR imo since there doesn't seem to be extra danger introduced with this change.