dbos-inc / dbos-transact

The TypeScript framework for backends that scale
https://docs.dbos.dev
MIT License
335 stars 22 forks source link

Consider safe guarding against JSON.parse #415

Closed demetris-manikas closed 3 months ago

demetris-manikas commented 3 months ago

Hi,

JSON.parse() can throw an exception and as well pointed out here it is a good idea to wrap the call with a safe one.

I don't know what will happen if an exception is thrown at say throw deserializeError(JSON.parse(rows[0].error));

kraftp commented 3 months ago

Thanks for bringing this to our attention! Have you directly experienced this issue or know a way to reproduce it? To my knowledge, every call to JSON.parse() is parsing a string which was created by JSON.stringify() and saved to the database. If such a call fails, it indicates a serious underlying data corruption issue that the framework cannot safely recover from.

demetris-manikas commented 3 months ago

No, I have not faced the problem while running the app. By safeguarding one makes sure that if the parsing fails then the program will not crash. Maybe a user updated an entry and by mistake introduced a faulty error message. Assuming that all changes in the db go through an api call is to my experience a wrong assumption. Having records with faulty error messages in the db is not a reason to crash the app or is it? Your call.

kraftp commented 3 months ago

Got it! All workflows (from which JSON.parse may be called) are wrapped in try blocks, exactly to solve problems like this. For example:

https://github.com/dbos-inc/dbos-transact/blob/45c93ea3c3191dee670354235429b79f5eaa764c/src/dbos-executor.ts#L427-L433

If an error is thrown, it will fail the workflow (which is the correct behavior in my opinion--such an error isn't recoverable), but the program won't crash. Does that sound reasonable to you?

demetris-manikas commented 3 months ago

Surely.

kraftp commented 3 months ago

Not necessary per conversation above--exceptions are handled safely elsewhere.