8000 BED-5845 - Rework DAWGS Value Mapping by zinic · Pull Request #1411 · SpecterOps/BloodHound · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

BED-5845 - Rework DAWGS Value Mapping #1411

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

Merged
merged 5 commits into from
May 12, 2025
Merged

BED-5845 - Rework DAWGS Value Mapping #1411

merged 5 commits into from
May 12, 2025

Conversation

zinic
Copy link
Contributor
@zinic zinic commented Apr 29, 2025

Description

Generic value mapping in DAWGS is poorly supported and was hacked together in order to support the first iteration of generic Cypher search for users.

Originally this logic was written for Neo4j with its caveats and quirks. Addition of the PostgreSQL driver introduced significant drift and technical debt in several of these paths.

The PostgreSQL driver for DAWGS currently targets upstream OID matching to utilize the object mapping cache found in the pgx library. This can be examined in the Scan(...) function of the PostgreSQL DAWGS driver type. The performance benefit of this optimization is not great enough to justify both the Neo4j and PostgreSQL driver deviating on mapping to DAWGS types. This deviation also made generic mapping of query results far more difficult.

The Result and Scanner interface have been refactored to remove the current ValueMapper idiom and value mapping between both the Neo4j and PostgreSQL driver is now unified.

Motivation and Context

Resolves BED-5845

Value mapping in DAWGS contains a complexity trap related to certain query return shapes. This is most prevalent when using the generic Cypher search functionality. This trap leads to CPU saturation.

How Has This Been Tested?

This refactor is covered by our existing tests.

Types of changes

  • Chore (a change that does not modify the application functionality)

Checklist:

@zinic zinic force-pushed the BED-5845 branch 5 times, most recently from 7078c51 to 657448e Compare April 29, 2025 20:44
Copy link
Contributor
@superlinkx superlinkx left a comment

Choose a reason for hiding this comment

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

Mostly nits, really appreciated having the context specific commits. Made it really easy to see what was changing without feeling overloaded with context, and gave me a good idea of your process too!

Copy link
Contributor
@superlinkx superlinkx left a comment

Choose a reason for hiding this comment

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

Changes look good, there's still quite a few places with the renamed graph.Result that name the variable scanner, may be worth the cleanup for consistency, but that's all that I found

Copy link
Contributor
@superlinkx superlinkx left a comment

Choose a reason for hiding this comment

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

Thanks for all the extra diligence, love this changeset

@zinic zinic merged commit b6d33c3 into main May 12, 2025
8 checks passed
@zinic zinic deleted the BED-5845 branch May 12, 2025 18:02
@github-actions github-actions bot locked and limited conversation to collaborators May 12, 2025
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants
0