#349: Generalize graph write queries #1038

achantavy · 2022-11-28T18:20:47Z

Addresses #349, also a good step working toward #1024.

Provides module authors with an automated way to load data to the graph so that they don't have to hand write Neo4j queries. Makes the process significantly less error prone by generating queries, and helps with reliability because transactions and batching are abstracted away.
Defines schema objects so that we can automate index creation and cleanup job creation in a future PR.
Demonstrates this functionality by having the AWS EMR sync adopt it.
Allows module authors to connect relationships to nodes using one or more attributes.

Why not use an existing ORM? Existing ones:

don't use UNWIND + MERGE for speed and batching
don't allow configuring Neo4j driver details such as connection timeouts, which are absolutely necessary if you must send traffic across a load balancer
don't run Neo4j managed transaction functions for automatic retries
in some cases use their own custom driver (or even require you to run a separate webserver for IPC!) and not the official Neo4j driver

.pre-commit-config.yaml

achantavy · 2022-11-28T18:42:17Z

cartography/intel/aws/emr.py

 ) -> None:
-    query = """


This PR refactors the EMR sync to demonstrate using the new functionality. Rather than hand write queries, module authors now define schema objects and call build_ingestion_query() and load_graph_data().

Note that the existing EMR integration tests still pass.

cartography/graph/model.py

ramonpetgrave64

Still not yet finished reviewing, and I want to add a helper function to make it a bit easier to create a CartographyNodeSchema . like this, using the types library. But this can come later.

create_node_schema(
  label='EMRCluster',
  properties=create_node_properties(
    arn='ClusterArn',
    auto_terminate='AutoTerminate',
    ...
  ),
  ...
)

cartography/graph/model.py

ramonpetgrave64 · 2022-12-01T22:36:32Z

cartography/intel/aws/emr.py

+@dataclass
+class EMRClusterSchema(CartographyNodeSchema):
+    label: str = 'EMRCluster'
+    properties: EMRClusterNodeProperties = EMRClusterNodeProperties()


Suggested change

properties: EMRClusterNodeProperties = EMRClusterNodeProperties()

properties: CartographyNodeProperties = EMRClusterNodeProperties()

Shouldn't we typehint with the specific type and not the generic one?

I think it's helpful just as a clue to devs on what the supertype should be. When editing in an IDE, though, the effect is the same.

By doing this we're saying that EMRClusterSchema.properties will accept any CartographyNodeProperties type, which is not correct, so I think we should keep this as-is.

cartography/graph/querybuilder.py

ramonpetgrave64

Looks great! The type tricks I mentioned can come later.

tests/unit/cartography/graph/test_querybuilder_complex.py

Co-authored-by: Ramon Petgrave <[email protected]>

…nto generalizequerywrites

…tiple node labels

ramonpetgrave64

Looks good, but that one test case needs to fix.

tests/integration/cartography/graph/test_querybuilder_rel_subsets.py

…f#1038) * Build ingest query * Linter * Save cleanup query for another PR * Implement schema * bump mypy to 0.981 for python/mypy#13398 * linter * make load_graph_data interface make more sense * fix comment * Docs and some better names * add a todo * Doc updates, rename some fields * Fix pre-commit * Code commment suggestions Co-authored-by: Ramon Petgrave <[email protected]> * Stackoverflow comment for clarity) * Support ingesting only parts of a schema without breaking the others * Doc comment * Linter * Support matching on one or more properties * Correctly name test * Change key_refs to TargetNodeMatcher to enforce it as a mandatory field * Remove use of hacky default_field() * Support subset of schema relationships for query generation, test multiple node labels * Docstrings * Comments in tests * Better comments * Test for exception conditions * Remove irrelevant comment Co-authored-by: Ramon Petgrave <[email protected]>

Alex Chantavy added 12 commits August 15, 2022 14:12

Build ingest query

45ec55e

Linter

a641ad0

Save cleanup query for another PR

5b3c83c

Implement schema

71edc8b

Merge latest master

41c9a24

bump mypy to 0.981 for python/mypy#13398

519cce9

linter

b900146

make load_graph_data interface make more sense

05973fb

fix comment

b6f3faf

Docs and some better names

aafa38d

add a todo

1293b2e

Doc updates, rename some fields

b810d75

achantavy mentioned this pull request Nov 28, 2022

Generalize graph write queries #895

Closed

Fix pre-commit

be30b3a

achantavy changed the title ~~Generalize graph write queries~~ #349: Generalize graph write queries Nov 28, 2022

achantavy requested review from ramonpetgrave64, kunaals, marco-lancini and ryan-lane November 28, 2022 18:30

achantavy commented Nov 28, 2022

View reviewed changes

.pre-commit-config.yaml Outdated Show resolved Hide resolved

achantavy commented Nov 28, 2022

View reviewed changes

achantavy mentioned this pull request Nov 28, 2022

mypy to 0.981, use flake8 github #1039

Merged

Merge branch 'master' into generalizequerywrites

7458af7

kunaals reviewed Nov 30, 2022

View reviewed changes

cartography/graph/model.py Show resolved Hide resolved

skiptomyliu reviewed Dec 1, 2022

View reviewed changes

cartography/graph/model.py Show resolved Hide resolved

ramonpetgrave64 requested changes Dec 5, 2022

View reviewed changes

ramonpetgrave64 previously approved these changes Dec 7, 2022

View reviewed changes

achantavy commented Dec 8, 2022

View reviewed changes

tests/unit/cartography/graph/test_querybuilder_complex.py Show resolved Hide resolved

Code commment suggestions

8021520

Co-authored-by: Ramon Petgrave <[email protected]>

achantavy dismissed ramonpetgrave64’s stale review via 8021520 December 9, 2022 22:53

Alex Chantavy added 6 commits December 9, 2022 15:21

Merge branch 'master' into generalizequerywrites

2e3dd8e

Merge branch 'generalizequerywrites' of github.com:lyft/cartography i…

d2e4794

…nto generalizequerywrites

Stackoverflow comment for clarity)

5bcb915

Support ingesting only parts of a schema without breaking the others

52a1d29

Doc comment

b5875a9

Linter

fa40b09

ramonpetgrave64 mentioned this pull request Dec 12, 2022

Add RDSSnapshot type #1046

Merged

Alex Chantavy added 5 commits December 12, 2022 16:21

Support matching on one or more properties

80b6ff5

Correctly name test

346ad50

Change key_refs to TargetNodeMatcher to enforce it as a mandatory field

2e65877

Remove use of hacky default_field()

cb233c3

Support subset of schema relationships for query generation, test mul…

3be472b

…tiple node labels

ramonpetgrave64 requested changes Dec 14, 2022

View reviewed changes

Alex Chantavy added 5 commits December 14, 2022 09:51

Docstrings

5dd60a2

Comments in tests

f6fea75

Better comments

fea0308

Test for exception conditions

7b3a268

Remove irrelevant comment

ab0af7b

ramonpetgrave64 approved these changes Dec 16, 2022

View reviewed changes

achantavy merged commit 9179f59 into master Dec 16, 2022

achantavy deleted the generalizequerywrites branch December 16, 2022 19:39

This was referenced Dec 16, 2022

Design a way to more quickly write AWS sync plugins #349

Closed

Automatic cleanup jobs #1052

Merged

Update dev docs on how to use new object model #1054

Merged

VishalAgarW mentioned this pull request Dec 30, 2022

EMRCluster node don't have firstseen property #1064

Closed

achantavy mentioned this pull request Jan 30, 2023

Consolidate Power :) #1100

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

#349: Generalize graph write queries #1038

#349: Generalize graph write queries #1038

achantavy commented Nov 28, 2022 •

edited

Loading

achantavy Nov 28, 2022 •

edited

Loading

ramonpetgrave64 left a comment

ramonpetgrave64 Dec 1, 2022

achantavy Dec 9, 2022 •

edited

Loading

ramonpetgrave64 Dec 12, 2022

achantavy Dec 13, 2022

ramonpetgrave64 left a comment

ramonpetgrave64 left a comment

	properties: EMRClusterNodeProperties = EMRClusterNodeProperties()
	properties: CartographyNodeProperties = EMRClusterNodeProperties()

#349: Generalize graph write queries #1038

#349: Generalize graph write queries #1038

Conversation

achantavy commented Nov 28, 2022 • edited Loading

achantavy Nov 28, 2022 • edited Loading

Choose a reason for hiding this comment

ramonpetgrave64 left a comment

Choose a reason for hiding this comment

ramonpetgrave64 Dec 1, 2022

Choose a reason for hiding this comment

achantavy Dec 9, 2022 • edited Loading

Choose a reason for hiding this comment

ramonpetgrave64 Dec 12, 2022

Choose a reason for hiding this comment

achantavy Dec 13, 2022

Choose a reason for hiding this comment

ramonpetgrave64 left a comment

Choose a reason for hiding this comment

ramonpetgrave64 left a comment

Choose a reason for hiding this comment

achantavy commented Nov 28, 2022 •

edited

Loading

achantavy Nov 28, 2022 •

edited

Loading

achantavy Dec 9, 2022 •

edited

Loading