Fix parsing empty strings #50
Conversation
|
Thanks @zhongzhi107 for the contribution. However, this doesn't seem to fix #49, as @pandapaul explained the error is thrown when a value was already stringified on save, or is malformed in the database and it fails when parsing on fetch. Also, you seem to be fixing |
This reverts commit b10f43a.
|
Hi @ricardogama , I add test case.
|
There was a problem hiding this comment.
Sorry @zhongzhi107 for the late review, this seems good to merge once you review my comments.
If you don't mind I'll change the PR name and description to something more meaningful to keep the history clean.
Cheers!
src/index.js
Outdated
| @@ -103,7 +103,9 @@ export default Bookshelf => { | |||
| // Parse JSON columns. | |||
| Object.keys(attributes).forEach(attribute => { | |||
| if (this.constructor.jsonColumns.includes(attribute)) { | |||
| model.attributes[attribute] = JSON.parse(model.attributes[attribute]); | |||
| if (model.attributes[attribute]) { | |||
There was a problem hiding this comment.
Please update to the following to keep consistency with other conditionals like this:
if (this.constructor.jsonColumns.includes(attribute) && attributes[attribute]) {
model.attributes[attribute] = JSON.parse(model.attributes[attribute]);
}
test/mysql/index.js
Outdated
| @@ -140,6 +140,19 @@ describe('with MySQL client', () => { | |||
| sinon.restore(ModelPrototype); | |||
| }); | |||
|
|
|||
| it('should not stringify \'\' values on update with `patch` option', async () => { | |||
There was a problem hiding this comment.
This test does not assert that the value has not been parsed, use something like this instead:
it('should keep an empty string on update with `patch` option', async () => {
const model = await Model.forge({ foo: 'bar' }).save();
await model.save({ foo: '' }, { patch: true });
model.get('foo').should.equal('');
});There was a problem hiding this comment.
Also, please add the same test to the SQLite3 suite.
ricardogama
changed the title
|
Thanks @ricardogama for the advices. |
dist/index.js
Outdated
| @@ -111,7 +111,9 @@ exports.default = Bookshelf => { | |||
| // Parse JSON columns. | |||
| Object.keys(attributes).forEach(attribute => { | |||
| if (this.constructor.jsonColumns.indexOf(attribute) !== -1) { | |||
| model.attributes[attribute] = JSON.parse(model.attributes[attribute]); | |||
There was a problem hiding this comment.
All good, but please remove changes on this file as the dist files are only added on the release commit.
|
I'm sorry for my carelessness. The changes have been discarded. |
|
Thanks @zhongzhi107, released as 2.1.1. |
This PR fixes a bug when using empty strings on update with the
patchoption. Since the values aren't being stringified, an error was raised when parsing them.