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

Add 'table' safeMode specs #98

Merged
merged 2 commits into from
Aug 13, 2018
Merged

Add 'table' safeMode specs #98

merged 2 commits into from
Aug 13, 2018

Conversation

etpinard
Copy link
Contributor

I missed out on table traces in #55, this PR adds them in.

cc @nicolaskruchten


if (cont.type === 'table' && isPlainObj(cont.cells)) {
l = Math.max(l, findMaxArrayLength(cont.cells))
}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

table doesn't have any arrays in the top level container, does it? I might have put this in estimateDataLength then, alongside the similar logic for dimensions. Also this way we'd keep looking to recurse, trace.cells.cells.cells...

That said, the fact that this combines with the top level instead of replacing it is nice. Just imagining malicious users, the simple test if (Array.isArray(trace.dimensions)) as it is with an immediate return could be used on a trace that doesn't use dimensions to mask the fact that the real data arrays are huge.

So we could add a type qualifier to dimensions, but perhaps the most robust solution (and future-proof as we add more trace types so we don't need to worry too much about updating the logic here) would be to look at the top level AND in cells AND in dimensions regardless of trace type, and find the max length of everything. I don't think anyone is going to be bothered by the fact that their useless length-1e8 cells.junk array in a scatter trace got rejected 😈

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

would be to look at the top level AND in cells AND in dimensions regardless of trace type, and find the max length of everything.

I like this solution a lot. Done in 2b5cf94

- look up the trace's "top" level and "dimensions" and "cells"
  regardless of trace type
@alexcjohnson
Copy link
Collaborator

Nice job! 💃

@etpinard etpinard merged commit c15ccf0 into master Aug 13, 2018
@etpinard etpinard deleted the table-safe-mode branch August 13, 2018 20:40
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.

2 participants