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

Fixed : don't crash when stat graph setting is broken #45

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

kevin-frugier
Copy link
Contributor

@kevin-frugier kevin-frugier commented Mar 9, 2017

For some reason, after returning to ForestAdmin UI after a while, some settings of my graphs were broken without having changed anything.

That's a bummer in itself (why did ForestAdmin loose my config out of the blue?), but the real problem is that this case was not properly handled by forest-express-mongoose hence resulting into a crash of my app.

error: UnhandledRejection: exception: $sort must have at least one sort key
error: MongoError: exception: $sort must have at least one sort key
error: Node app stopping.
[forest] 🌳🌳🌳  exception: $sort must have at least one sort key

The change of line-stat-getter.js fixes the crash.
The change of pie-stat-getter.js changes the cryptic
[forest] 🌳🌳🌳 Cannot read property 'reference' of undefined
into
[forest] 🌳🌳🌳 Missing param `group_by_field`

None of that would be required if the graph setting never broke for no reason (it's indeed impossible to create a graph without proper settings).

@arnaudbesnier
Copy link
Member

Hi @kevin-frugier, I am not a big fan of the idea of hiding the Forest issue you pointed by adding protections on the liana.

The best for us would be to have a simple way to reproduce your charts "destruction" and then fix this issue. Do you have any reproduction steps?

Thanks!

@kevin-frugier
Copy link
Contributor Author

Hi @arnaudbesnier,

Personally I'm not a big fan of an update of Forest crashing my PRODUCTION server either.
But it doesn't mean both sides can't be reinforced.

Anyway the liana is already quite robust to failures because it doesn't crash on exceptions.
It currently crashes on unhandledRejection which is what I prevent with the fix of line-stat-getter.js (I prevent the rejection to be raised in the first place).
You could simply handle unhandled rejections the same way you handle exceptions if you don't want to fine tune every bits of (potentially) broken code.

As for reproduction steps, the only thing I can tell you is that I migrated from Node 0.10.46 to 6.9.5 (at last!) but none of the model declaration has changed and the graph anyway is not declared at all on the serverside (only in Forest) and uses standard data.

Luckily, it's during the validation phase of this migration (to 6.9.5) that I noticed that Forest had lost some of my graphs settings (like 'group_by').

@roopakv
Copy link
Contributor

roopakv commented Oct 14, 2018

would actually love if there was some progress on this.

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

Successfully merging this pull request may close these issues.

3 participants