Prevent DAG crashes because of empty service name string #656
Conversation
Signed-off-by: Ruben Vargas <ruben.vp8510@gmail.com>
Codecov Report
@@ Coverage Diff @@
## master #656 +/- ##
=======================================
Coverage 94.22% 94.22%
=======================================
Files 228 228
Lines 5919 5922 +3
Branches 1488 1489 +1
=======================================
+ Hits 5577 5580 +3
Misses 303 303
Partials 39 39
Continue to review full report at Codecov.
|
| if (!nodeMap[d.parent]) { | ||
| nodes.push({ data: { id: d.parent } }); | ||
| nodeMap[d.parent] = true; | ||
| if (d.parent.trim().length !== 0 && d.child.trim().length !== 0) { |
There was a problem hiding this comment.
What are the semantics of d.parent and d.child? Could we not ensure elsewhere that they are not empty?
Could you add a unit test to demonstrate that data leads to an issue you're trying to fix?
There was a problem hiding this comment.
The way I was able to reproduce the issue is instrumenting a service with a space, (or empty string)
The context of this is d.parent and d.child are service names, I don't think there are other places where we ensure that the service name is not empty, indeed I can see the empty service name in the search bar
Is this considered an instrumentation error? Should we indicate it in some way? If this is consider an error in the instrumentation, may be we should do this validation in other place, more close to when we process the query service response.
I'll add a couple of tests.
There was a problem hiding this comment.
imo empty or whitespace service name should be considered an instrumentation error, and we could build a sanitizer to fix it during ingestion.
However, w.r.t. this code that looks things up in a map, an empty or whitespace string is still a string, so why is the code failing on that?
There was a problem hiding this comment.
I agree that we should build a sanitizer that fix this during ingestion, but also this view should not fail even if there are empty strings.
The error is not on this part, but on cytoscape, which doesn't allow empty strings nor white spaces as node IDs, I'm just filtering those cases here before calling cytoscape a couple of lines below.
rubenvp8510
force-pushed
the
Issue-634
branch
2 times, most recently
from
cec0ebe to
1244857
Compare
Signed-off-by: Ruben Vargas <ruben.vp8510@gmail.com>
rubenvp8510
force-pushed
the
Issue-634
branch
from
1244857 to
9a5b510
Compare
|
Thanks! |
…ng#656) * Prevent DAG crashes because of empty service name string Signed-off-by: Ruben Vargas <ruben.vp8510@gmail.com> * Adding tests for empty service names Signed-off-by: Ruben Vargas <ruben.vp8510@gmail.com> Signed-off-by: vvvprabhakar <vvvprabhakar@gmail.com>
…ng#656) * Prevent DAG crashes because of empty service name string Signed-off-by: Ruben Vargas <ruben.vp8510@gmail.com> * Adding tests for empty service names Signed-off-by: Ruben Vargas <ruben.vp8510@gmail.com> Signed-off-by: vvvprabhakar <vvvprabhakar@gmail.com>
…ng#656) * Prevent DAG crashes because of empty service name string Signed-off-by: Ruben Vargas <ruben.vp8510@gmail.com> * Adding tests for empty service names Signed-off-by: Ruben Vargas <ruben.vp8510@gmail.com> Signed-off-by: vvvprabhakar <vvvprabhakar@gmail.com>
Signed-off-by: Ruben Vargas ruben.vp8510@gmail.com
Which problem is this PR solving?
Short description of the changes