Skip to content
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.

Sign up for GitHub

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

Fix migration bug where using -- as an actual string value gets lost #28

Merged
merged 3 commits into from
Apr 30, 2017
Merged

Fix migration bug where using -- as an actual string value gets lost #28

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
ac62c4b
Fix migration bug where using -- as an actual string value gets lost
theogravity Apr 11, 2017
a573bd6
Simplify trim fixes
theogravity Apr 18, 2017
93b86e5
Simplify removal of comments
theogravity Apr 25, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 9 additions & 0 deletions migrations/003-test-cert.sql
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
-- Up
CREATE TABLE whatever ( certificate TEXT );
INSERT INTO whatever ( certificate ) VALUES (
'-----BEGIN CERTIFICATE-----
some contents
-----END CERTIFICATE-----');

-- Down
DROP TABLE whatever;
4 changes: 2 additions & 2 deletions src/Database.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -167,8 +167,8 @@ class Database {
reject(new Error(message));
} else {
/* eslint-disable no-param-reassign */
migration.up = up.replace(/^--.*?$/gm, '').trim(); // Remove comments
migration.down = down.replace(/^--.*?$/gm, '').trim(); // and trim whitespaces
migration.up = up.replace(/^-- .*?$/gm, '').trim();// Remove comments
Copy link

Choose a reason for hiding this comment

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

Thanks @theogravity 😁

Quick quesiton: do we really need to remove comments? Simply doing migration.up = up.trim() (same as migration.down) would prevent future issues like the one I just had with TLS certificates ;)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

i think @koistya could prob chime in on that one

migration.down = down.trim(); // and trim whitespaces
/* eslint-enable no-param-reassign */
resolve();
}
Expand Down
7 changes: 6 additions & 1 deletion test/spec.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -149,11 +149,16 @@ it('Should migrate the database', (done) => {
let p = db.open(':memory:');
p = p.then(() => db.migrate());
p = p.then(() => db.all('SELECT id, name FROM migrations').then((result) => {
expect(result).to.be.deep.equal([{ id: 1, name: 'initial' }, { id: 2, name: 'some-feature' }]);
expect(result).to.be.deep.equal([{ id: 1, name: 'initial' }, { id: 2, name: 'some-feature' }, { id: 3, name: 'test-cert' }]);
}));
p = p.then(() => db.all('SELECT * FROM Category').then((result) => {
expect(result).to.be.deep.equal([{ id: 1, name: 'Test' }]);
}));

p = p.then(() => db.all('SELECT certificate from whatever').then((result) => {
expect(result[0].certificate).to.be.equal('-----BEGIN CERTIFICATE-----\nsome contents\n-----END CERTIFICATE-----');
}));

p = p.then(() => db.close());
p.then(done, done);
});