Working for updates?

[juandiegopalomino]

Hi there,

This is an awesome library, and the only fault I've encountered is that it doesn't seem to stringify the JSON columns when saving as an update. I've been toying with the "updating" and "updated" event emitters, but didn't get much. Any idea on how to perfect this (or if it's even possible)?

[ricardogama]

@juandiegopalomino Hey!

The update queries are included on the test suite, saving and saved events contemplate both insert and update queries.

Can you please provide a code snippet of your model method call and the thrown error?

Also, what Bookshelf & Knex versions are you using?

[juandiegopalomino]

@ricardogama Hi there,

We're currently using bookshelf v0.9.45 and knex v0.11.9
The code looks like the following:
preexistingUser.save({anAttribute: aSimpleJSONOrArray}, {patch: true});
And on the User model, we have jsonColumns:["anAttribute", ...]. If we set it upon creation, it seems to work, but if we try to update it as on top, we would need to stringify aSimpleJSONOrArray in order to get it to work.

[ebramanti]

I have this exact same problem. Could it be emitting a different event because of the patch option?

[jamesdixon]

@juandiegopalomino @jadengore can you read through #7 and see if this is the same issue?

[ricardogama]

Thanks for the reports guys. The problem is indeed when using the patch option, since Bookshelf uses the save argument instead of the model attributes which are the ones who get parsed on the events. I'll investigate and try to find a solution.

[ebramanti]

In the meantime, I'm calling JSON.parse on the data for that column before I serialize from my API.

[ricardogama]

I think I found a solution on #27, although it diverges a lot from the previous implementation of this plugin and I'll require further investigation before merging. As this can take a while I suggest you use the feature/update-with-patch-option branch and let me know if the issue is fixed.

[ricardogama]

Fix released as 1.2.0, cheers!

[ebramanti]

@ricardogama 👏 👏 👏 👏 👏

[jamesdixon]

Haven't gotten a chance to try this yet, but looking forward to it. Great work, @ricardogama!

[juandiegopalomino]

@ricardogama sorry, I lost track of the issue (need to check my notification settings) and just saw the fix. I'll get to using it immediately and thank you so much!

[jamesdixon]

@ricardogama just wanted to confirm that this does appear to work. Thanks a ton for this!!!!!

[jamesdixon]

I seem to be running into this issue again. This time, it's occurring specifically when inserting. I'm calling a function in the saving event that generates the JSON and then sets it on the model.

        this.on('saving', (model, attrs, options) => {
            const { pets, services } = model.attributes;

            // Generate and save the report card.
            return internals.generateReportCard(pets, services)
                .then((reportCard) => {
                    model.set('reportCard', reportCard);
                });
});

Anyone else having this issue?

[jamesdixon]

FYI that if I do the following, it works in both insert and update cases:

            return internals.generateReportCard(pets, services)
                .then((reportCard) => {

                    if (model.isNew()) {
                        reportCard = JSON.stringify(reportCard);
                    }

                    model.set('reportCard', reportCard);
                });

[ricardogama]

Hi James, I guess the trick here is with the patch option and not wether you're inserting or updating.

If you're using patch, the plugin task will be executed before the saving event is triggered, that's why you have to do it manually on your event.

Without patch, your saving event will occur before the plugin handles its own saving event, and that why you don't need to do it, since the plugin will take care of it.

I guess that the plugin behaviour diverged so much since the patch fix that now a lot of edge cases like this can happen, so I'm really considering moving all the behaviour to the save method instead of events, where they'll all occur after the job is done.

Anyway if you want to make your code more plain (and if it doesn't bring side effects), I think you can call the prototype initialize method before registering the events, and you'll have to stringify manually in all cases:

bookshelf.Model.extend({
  initialize() {
    bookshelf.Model.prototype.initialize.apply(this, arguments);

    this.on('saving', (model, attrs, options) => {
      const { pets, services } = model.attributes;

      // Generate and save the report card.
      return internals.generateReportCard(pets, services)
        .then(reportCard => {
            model.set('reportCard', JSON.stringify(reportCard));
        });
    });
  }
});

[jamesdixon]

@ricardogama thanks for the detailed explanation and sorry to keep being a pest about this issue!

I'll probably revert to specifically stringifying and parsing these fields for the time being. Again, I appreciate your hard work!