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.

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

Handling of parse errors #49

Open
pandapaul opened this issue May 22, 2017 · 3 comments
Open

Handling of parse errors #49

pandapaul opened this issue May 22, 2017 · 3 comments

Comments

@pandapaul
Copy link

Howdy. I'm wondering what you guys would recommend as the best way to handle parsing errors that may occur while fetching & parsing. For example, in my case I'm working with a MySQL database where the JSON columns are actually type TEXT, so invalid JSON can wind up there. When attempting to parse that I, of course, run into parsing errors. It seems like it might be useful to be able to provide error handling hooks to the plugin, but the lack of those makes me think I'm missing something. So what would you guys recommend as the best way to handle that?

As a side note, what actually led to this in my case was a strange circumstance in which the plugin was attempting to parse data that had somehow already been parsed. I've never seen that happen since starting to use the plugin, but maybe it had something to do with the join I was doing. Pretty weird.

Anyway thanks in advance for the help.

@ricardogama
Copy link
Collaborator

Hi @pandapaul, thanks for the report.

I understand your concern, but I'm not sure if it's the responsibility of this plugin to handle such errors. The assumption to use it is that you don't need (nor have) to handle JSON values elsewhere in your code.

So if you have malformed values on the database, I suggest to run a migration and fix those values so they can be fetched correctly.

If you already have stringified values when the plugin hook runs, you should debug your code and figure where that's being done, and prevent that from happening.

If that still doesn't suit your use case, there some ways we can handle this:

  • Throw a custom error that you can easily catch and handle;
  • Add an option to graciously handle such errors, maintaining the original values on error;
  • Add an option (let's say { handleJSON: false }) to .save(), .fetch() and all other methods that trigger the event hooks, so that you can skip the parsing operations in a particular case;

Let me know what are you thoughts about this, and also take a look at #50 and confirm that it doesn't fix this issue in particular.

@pandapaul
Copy link
Author

Sorry for the late reply... May 25th ended up being when my son was born! Huzzah for that. Now back to code stuff...

The situation I'm in is that we've got a MySQL database that gets written to by both my own team and others, and the JSON columns are type TEXT so they're not inherently validated. So it's entirely possible that someone not going through my team's API might write some invalid JSON in there. Not an ideal situation for sure, but throwing a custom error would be great. Seems like bookshelf already has a few such things, so it's not out of line with that theme.

Indeed #50 seems to enforce not parsing missing attributes, which does not address the parsing errors I'm referring to here in #49. But I can't otherwise attest to whether or not the changes in #50 are necessary.

@ricardogama
Copy link
Collaborator

Hi @pandapaul, just created #51 which adds a throwsJSONErrors option that enables throwing a custom error when parsing fails.

Although I'm not sure if this is useful, can you please check the branch out and tell me if it fits your needs? Since this has no breaking changes I'm ok with adding such feature if it helps anybody out.

Cheers!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants