Skip to content

perf: eliminate N+1 queries in __make_cre_links and __get_all_nodes_and_cres - #850

Closed
shiwani42 wants to merge 1 commit into
OWASP:mainfrom
shiwani42:fix/n-plus-one-query-patterns
Closed

shiwani42 wants to merge 1 commit into
OWASP:mainfrom
shiwani42:fix/n-plus-one-query-patterns

Conversation

@shiwani42

Copy link
Copy Markdown
Contributor

Fixes #848

What changed

__make_cre_links

Replaced the per-link Node fetch with a single JOIN query. Previously, for a CRE with L links this method issued 1 + L queries. It now issues one query regardless of link count.

Before:

for link in self.session.query(Links).filter(Links.cre == cre.id).all():
    node = self.session.query(Node).filter(Node.id == link.node).first()

After:

rows = (
    self.session.query(Links, Node)
    .join(Node, Node.id == Links.node)
    .filter(Links.cre == cre.id)
    .all()
)
for link, node in rows:
    ...

__get_all_nodes_and_cres

Replaced the IDs-first fetch pattern with direct ORM object queries. For CREs, the previous code called get_cre_by_db_id per record, which added a SELECT external_id FROM cre WHERE id = ? round-trip before reaching get_CREs. Fetching full ORM objects and passing the already-known external_id directly cuts that out. The same fix is applied to the node branch.

Tests

All existing tests pass. The changed methods are exercised by test_get_CREs, test_export, test_get_root_cres, and test_get_cre_hierarchy.

…nd_cres

__make_cre_links was issuing a separate SELECT for each linked node —
one query to fetch all Links rows for a CRE, then one more per row to
fetch the corresponding Node. Replace with a single JOIN query so the
whole thing resolves in one round-trip regardless of how many links a
CRE has.

__get_all_nodes_and_cres was fetching IDs first (SELECT id FROM …)
and then re-querying each record individually. For CREs this chained
into get_cre_by_db_id, which added a further SELECT external_id per
CRE before finally calling get_CREs. Fetch full ORM objects directly
and call get_CREs(external_id=) using the already-known external_id,
cutting out that extra per-CRE round-trip. The same IDs-first pattern
is fixed for the node branch.
@northdpole

Copy link
Copy Markdown
Collaborator

Review: __make_cre_links JOIN fix is solid — real N+1 elimination.

Concern: __get_all_nodes_and_cres still does N+1 (query(Node).all() then get_nodes() per row). The old code had the same pattern but this doesn't fully deliver on the PR title.

Blockers: CONFLICTING on main — needs rebase. Suggest author trim to __make_cre_links only or finish the batch query for nodes/CREs before merge.

@shiwani42

Copy link
Copy Markdown
Contributor Author

For this too, I'll wait for GSoC PRs to get merged to avoid any conflicts and will incorporate the trim as suggested. Thanks!

@northdpole

Copy link
Copy Markdown
Collaborator

GSoC is over — we're going to land this ourselves rather than wait on another rebase cycle.

Plan: keep the __make_cre_links JOIN (the real N+1 win), drop or leave __get_all_nodes_and_cres as-is because that path is still N+1, rebase onto current main. We'll credit you as author. If you'd rather take it, say so here and we'll stop.

@northdpole

Copy link
Copy Markdown
Collaborator

Correction after reading current main: we should not land this PR as-is.

__make_cre_links is already gone. CRE hydration is _hydrate_cres_batch, which bulk-loads Links + Node in two queries — that is the N+1 fix.

The remaining __get_all_nodes_and_cres hunk still calls get_nodes / get_CREs per row, so it does not deliver the title. Applying it would be churn on db.py for no win.

Proposal: close this PR as superseded. If we still want faster export(), that is a new batch-hydrate of __get_all_nodes_and_cres, not this diff. @shiwani42 shout if you want to take that follow-up.

@northdpole

Copy link
Copy Markdown
Collaborator

Landed the actual remaining N+1 work as #1100 (batch-hydrate __get_all_nodes_and_cres via _hydrate_cres_batch + new _hydrate_nodes_batch).

This PR's JOIN on __make_cre_links is already on main; the __get_all_nodes_and_cres hunk here still per-row get_nodes/get_CREs. Closing as superseded by #1100.

@northdpole northdpole closed this Sep 13, 2026
northdpole added a commit that referenced this pull request Sep 16, 2026
Graph load still called get_cre_by_db_id/get_nodes once per row. Reuse _hydrate_cres_batch and add a matching node batch so #850's JOIN goal actually lands.
northdpole added a commit that referenced this pull request Sep 16, 2026
Graph load still called get_cre_by_db_id/get_nodes once per row. Reuse _hydrate_cres_batch and add a matching node batch so #850's JOIN goal actually lands.
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.

N+1 query patterns in __make_cre_links and __get_all_nodes_and_cres

2 participants