-
Notifications
You must be signed in to change notification settings - Fork 8.6k
[Contextual Security] add relationship node type for entity graph #249479
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
Changes from all commits
f936d35
fa5edfa
dea41f1
a51ff8e
c1e88b7
d316264
31cc552
5f24bcd
7f9f3e0
5599a55
af5bfdd
cf99142
0538f11
287e3c4
dda90fb
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -88,7 +88,12 @@ export const REACHED_NODES_LIMIT = 'REACHED_NODES_LIMIT'; | |
| export const graphResponseSchema = () => | ||
| schema.object({ | ||
| nodes: schema.arrayOf( | ||
| schema.oneOf([entityNodeDataSchema, groupNodeDataSchema, labelNodeDataSchema]) | ||
| schema.oneOf([ | ||
| entityNodeDataSchema, | ||
| groupNodeDataSchema, | ||
| labelNodeDataSchema, | ||
| relationshipNodeDataSchema, | ||
| ]) | ||
| ), | ||
| edges: schema.arrayOf(edgeDataSchema), | ||
| messages: schema.maybe(schema.arrayOf(schema.oneOf([schema.literal(REACHED_NODES_LIMIT)]))), | ||
|
|
@@ -115,6 +120,7 @@ export const nodeShapeSchema = schema.oneOf([ | |
| schema.literal('diamond'), | ||
| schema.literal('label'), | ||
| schema.literal('group'), | ||
| schema.literal('relationship'), | ||
| ]); | ||
|
|
||
| export const nodeBaseDataSchema = schema.object({ | ||
|
|
@@ -164,6 +170,14 @@ export const labelNodeDataSchema = schema.allOf([ | |
| }), | ||
| ]); | ||
|
|
||
| export const relationshipNodeDataSchema = schema.allOf([ | ||
| nodeBaseDataSchema, | ||
| schema.object({ | ||
| shape: schema.literal('relationship'), | ||
| parentId: schema.maybe(schema.string()), | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm not sure we would like to stack relationships - worth to double check with design
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. left a question.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. decided to support stacked nodes for relationships as well.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. depending on the proposed design - we could add more fields such as
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @kfirpeled as discussed in the sync - we are going to show relationship nodes in stacked groupes as well.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Since then I think we reverted that decision not to stack.. anyway. Lets leave it here for now
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. yes, I would leave it for now to see if all cases are covered - if not ill remove it. |
||
| }), | ||
| ]); | ||
|
|
||
| export const edgeDataSchema = schema.object({ | ||
| id: schema.string(), | ||
| source: schema.string(), | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just a side note, the naming convention here is not aligned with the current design. However, I'm thinking to propose a new change to the existing node's names.
Instead of shape, we will have element type: relationship/entity/event/stack.
So I'm ok with keeping it now as it is.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"relationship" sounds very business-domain specific to me, though I don't have a better suggestion.
On the other hand, I like the idea of future renaming label nodes to event, since we also have labels in each node
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree with both comments - right now we are coupling shape with node type. As I see it we should have the following:
relationship/entity/event/stack.this would also require us to return additional property for each node - type.
For now i would leave current behavior and do it in a separate task.