mirror of
https://github.com/abhigyanpatwari/GitNexus.git
synced 2026-09-29 01:41:42 +00:00
fix(python): walk ancestors for multi-segment dotted imports (#1241)
* fix(python): walk ancestors for multi-segment dotted imports (#1240) Single-segment Python imports (`from middleware import X`) already get an ancestor-directory walk in `resolvePythonImportInternal`, so they resolve correctly when the importer and the imported module share a parent directory (e.g. both under `backend/`). Multi-segment dotted imports (`from services.sync import X`) were only resolved against the workspace root. In a `backend/`-prefixed repo, `from services.sync import X` from `backend/routers/cron.py` would not resolve because `services/sync.py` does not exist at the workspace root — only `backend/services/sync.py` does. The IMPORTS edge was dropped, the imported names were never bound, and downstream CALLS edges to those names were silently lost. The fix mirrors the single-segment ancestor walk for multi-segment paths in `resolveAbsoluteFromFiles`, and widens `hasRepoCandidate` to accept nested `/segment/` matches so it does not bail before the walk runs. Includes a new fixture and 5 integration tests covering: - IMPORTS resolution for `from services.sync`, `from services.alerts`, `from routers.alerts` from `backend/routers/cron.py`. - CALLS edge counts for every multi-segment-imported callee. - Regression check: single-segment ancestor walk (`from auth_utils import …`) still resolves correctly. The django-app-imports regression suite (which prevents `accounts.apps` from spuriously matching a local `apps.py`) continues to pass — the new nested-namespace check in `hasRepoCandidate` is bounded by an explicit `/segment/` substring, and the workspace-root candidate check still runs first. * fix(python): scope hasRepoCandidate widening to importer ancestors + tighten ancestor-walk loop Address review findings from PR #1241: 1. hasRepoCandidate's nested check now requires the matching directory to sit on an ancestor of the importer. Previously any nested /SEGMENT/ path satisfied the gate, which would let a vendored copy of an external package (e.g. vendor/django/urls.py) gate-pass an external import like 'from django.urls import path' issued from app/main.py. 2. Loop bound in resolveAbsoluteFromFiles tightened from 'i >= 0' to 'i > 0' to skip a redundant root-candidate recheck (the workspace-root direct check above already covers that case). 3. Doc-comment in resolveAbsoluteFromFiles now states the precedence order explicitly: workspace root > closest ancestor > suffix fallback. Tests added: - Vendored-external false-positive guard (vendor/django/urls.py must not resolve from app/main.py). - Workspace-root vs ancestor precedence (root services/sync.py wins over backend/services/sync.py for a backend/routers/cron.py importer). 215/215 python integration tests pass (+4 from this change). tsc --noEmit green.
This commit is contained in:
parent
489985488f
commit
efed04af51
9 changed files with 344 additions and 20 deletions
|
|
@ -61,37 +61,81 @@ export function resolvePythonImportTarget(
|
|||
const pathLike = parsedImport.targetRaw.replace(/\./g, '/');
|
||||
if (pathLike.includes('/')) {
|
||||
const [leadingSegment] = pathLike.split('/').filter(Boolean);
|
||||
if (!leadingSegment || !hasRepoCandidate(leadingSegment, ctx.allFilePaths)) {
|
||||
if (!leadingSegment || !hasRepoCandidate(leadingSegment, ctx.allFilePaths, ctx.fromFile)) {
|
||||
return null;
|
||||
}
|
||||
}
|
||||
|
||||
// Multi-segment absolute resolve: try exact paths first, then suffix
|
||||
// match in nested repos. Using direct `Set.has` + `endsWith` instead of
|
||||
// `suffixResolve`'s shared helper because that helper requires a
|
||||
// pre-built `SuffixIndex` to disambiguate ties — without one it falls
|
||||
// back to an O(files) scan that silently picks the wrong file when
|
||||
// the last segment collides across directories (e.g. `accounts.models`
|
||||
// matching `billing/models.py` when both files exist).
|
||||
return resolveAbsoluteFromFiles(pathLike, ctx.allFilePaths);
|
||||
// Multi-segment absolute resolve: try exact paths first, then ancestor
|
||||
// walk (mirrors the single-segment ancestor walk in
|
||||
// `resolvePythonImportInternal`), then a suffix match in nested repos.
|
||||
// Using direct `Set.has` + `endsWith` instead of `suffixResolve`'s shared
|
||||
// helper because that helper requires a pre-built `SuffixIndex` to
|
||||
// disambiguate ties — without one it falls back to an O(files) scan that
|
||||
// silently picks the wrong file when the last segment collides across
|
||||
// directories (e.g. `accounts.models` matching `billing/models.py` when
|
||||
// both files exist).
|
||||
return resolveAbsoluteFromFiles(pathLike, ctx.allFilePaths, ctx.fromFile);
|
||||
}
|
||||
|
||||
/**
|
||||
* Resolve `package/sub/module` style paths (already dot-flattened) to a
|
||||
* concrete file in `allFilePaths`. Tries the exact path first, then the
|
||||
* `__init__.py` variant, then a suffix match for nested layouts.
|
||||
* concrete file in `allFilePaths`. Tries the exact path first, then walks
|
||||
* ancestors of `fromFile` looking for `<ancestor>/<pathLike>.py` (or
|
||||
* `__init__.py`), then falls back to a suffix match for nested layouts.
|
||||
* Returns the original (un-normalized) path from the set.
|
||||
*
|
||||
* Precedence order:
|
||||
* 1. Workspace-root direct hit (`<pathLike>.py`, `<pathLike>/__init__.py`).
|
||||
* 2. Closest-ancestor match walking up from the importer's directory.
|
||||
* 3. Suffix fallback (first match).
|
||||
*
|
||||
* Root wins over ancestor by construction — if both `services/sync.py` and
|
||||
* `backend/services/sync.py` exist, `backend/routers/cron.py`'s
|
||||
* `from services.sync import X` resolves to the root file. This mirrors
|
||||
* Python's `sys.path` semantics where the project root is searched first.
|
||||
*
|
||||
* The ancestor walk mirrors the single-segment behavior in
|
||||
* `resolvePythonImportInternal`. For `from services.sync import X` in
|
||||
* `backend/routers/cron.py`, walk up: `backend/routers/services/sync.py` →
|
||||
* `backend/services/sync.py` ✓.
|
||||
*/
|
||||
function resolveAbsoluteFromFiles(pathLike: string, allFilePaths: Set<string>): string | null {
|
||||
function resolveAbsoluteFromFiles(
|
||||
pathLike: string,
|
||||
allFilePaths: Set<string>,
|
||||
fromFile: string,
|
||||
): string | null {
|
||||
const directFile = `${pathLike}.py`;
|
||||
const directPkg = `${pathLike}/__init__.py`;
|
||||
|
||||
// Direct hit at workspace root.
|
||||
if (allFilePaths.has(directFile)) return directFile;
|
||||
if (allFilePaths.has(directPkg)) return directPkg;
|
||||
|
||||
// Ancestor walk — match the single-segment resolver's behavior at
|
||||
// multi-segment granularity. Closest match wins. Stop at `i > 0` because
|
||||
// `i === 0` would re-check the workspace-root candidates already covered
|
||||
// by the direct check above.
|
||||
const importerDir = fromFile.replace(/\\/g, '/').split('/').slice(0, -1).join('/');
|
||||
if (importerDir) {
|
||||
const dirParts = importerDir.split('/').filter(Boolean);
|
||||
for (let i = dirParts.length; i > 0; i--) {
|
||||
const ancestor = dirParts.slice(0, i).join('/');
|
||||
const prefix = `${ancestor}/`;
|
||||
const candidateFile = `${prefix}${directFile}`;
|
||||
const candidatePkg = `${prefix}${directPkg}`;
|
||||
if (allFilePaths.has(candidateFile)) return candidateFile;
|
||||
if (allFilePaths.has(candidatePkg)) return candidatePkg;
|
||||
}
|
||||
}
|
||||
|
||||
// Existing suffix-match fallback (preserved for monorepo/nested-repo
|
||||
// layouts that don't share a directory ancestor with the importer).
|
||||
const suffixFile = `/${directFile}`;
|
||||
const suffixPkg = `/${directPkg}`;
|
||||
|
||||
let suffixMatch: string | null = null;
|
||||
for (const raw of allFilePaths) {
|
||||
const f = raw.replace(/\\/g, '/');
|
||||
if (f === directFile || f === directPkg) return raw;
|
||||
if (suffixMatch === null && (f.endsWith(suffixFile) || f.endsWith(suffixPkg))) {
|
||||
suffixMatch = raw;
|
||||
}
|
||||
|
|
@ -100,21 +144,54 @@ function resolveAbsoluteFromFiles(pathLike: string, allFilePaths: Set<string>):
|
|||
}
|
||||
|
||||
/**
|
||||
* Does the repo contain a module/package named `leadingSegment` at the top
|
||||
* level? Used to guard against false-positive suffix matches on external
|
||||
* dotted imports (e.g. `django.apps` matching a local `accounts/apps.py`).
|
||||
* Does the repo contain a module/package named `leadingSegment` somewhere
|
||||
* the importer can plausibly reach?
|
||||
*
|
||||
* Checks, in order: `<segment>.py` root file, `<segment>/__init__.py`
|
||||
* regular package, or any `<segment>/**.py` file (namespace package).
|
||||
* Used to guard against false-positive suffix matches on external dotted
|
||||
* imports (e.g. `django.apps` matching a local `accounts/apps.py`).
|
||||
*
|
||||
* Checks, in order:
|
||||
* 1. `SEGMENT.py` root file or `SEGMENT/__init__.py` regular package.
|
||||
* 2. Any `SEGMENT/...py` file at the workspace root (namespace package).
|
||||
* 3. Any `<importer-ancestor>/SEGMENT/...py` file (nested namespace
|
||||
* package the importer could reach via an ancestor walk, e.g.
|
||||
* `backend/services/sync.py` from `backend/routers/cron.py`).
|
||||
*
|
||||
* The nested case is bounded to the importer's own ancestors so a
|
||||
* vendored copy of an external package (e.g. `vendor/django/urls.py`)
|
||||
* does not gate-pass external imports like `from django.urls import path`
|
||||
* issued from `app/main.py`. Files inside the vendored tree itself
|
||||
* (importer under `vendor/django/...`) still resolve correctly because
|
||||
* the ancestor walk includes their own parents.
|
||||
*/
|
||||
function hasRepoCandidate(leadingSegment: string, allFilePaths: Set<string>): boolean {
|
||||
function hasRepoCandidate(
|
||||
leadingSegment: string,
|
||||
allFilePaths: Set<string>,
|
||||
fromFile: string,
|
||||
): boolean {
|
||||
const prefix = `${leadingSegment}/`;
|
||||
const rootFile = `${leadingSegment}.py`;
|
||||
const initFile = `${leadingSegment}/__init__.py`;
|
||||
|
||||
// Build importer-ancestor prefixes: for `backend/routers/cron.py`,
|
||||
// produces `["backend/routers/services/", "backend/services/"]` for
|
||||
// segment `services` (closest first, root excluded — covered above).
|
||||
const importerDir = fromFile.replace(/\\/g, '/').split('/').slice(0, -1).join('/');
|
||||
const dirParts = importerDir ? importerDir.split('/').filter(Boolean) : [];
|
||||
const ancestorPrefixes: string[] = [];
|
||||
for (let i = dirParts.length; i > 0; i--) {
|
||||
ancestorPrefixes.push(`${dirParts.slice(0, i).join('/')}/${leadingSegment}/`);
|
||||
}
|
||||
|
||||
for (const raw of allFilePaths) {
|
||||
const f = raw.replace(/\\/g, '/');
|
||||
if (f === rootFile || f === initFile) return true;
|
||||
if (f.startsWith(prefix) && f.endsWith('.py')) return true;
|
||||
if (f.endsWith('.py')) {
|
||||
for (const ap of ancestorPrefixes) {
|
||||
if (f.startsWith(ap)) return true;
|
||||
}
|
||||
}
|
||||
}
|
||||
return false;
|
||||
}
|
||||
|
|
|
|||
|
|
@ -0,0 +1,6 @@
|
|||
def verify_cron_secret(token):
|
||||
return token == "expected"
|
||||
|
||||
|
||||
def get_org_id_from_header(headers):
|
||||
return headers.get("x-org-id") if headers else None
|
||||
|
|
@ -0,0 +1,2 @@
|
|||
def send_daily_alerts():
|
||||
return "sent"
|
||||
|
|
@ -0,0 +1,24 @@
|
|||
from auth_utils import verify_cron_secret, get_org_id_from_header
|
||||
from services.sync import _start_cron_run, _complete_cron_run
|
||||
from services.alerts import _create_ops_alert
|
||||
from routers.alerts import send_daily_alerts
|
||||
|
||||
|
||||
def handler_a():
|
||||
if verify_cron_secret("x"):
|
||||
org = get_org_id_from_header(None)
|
||||
_start_cron_run("a")
|
||||
_create_ops_alert("a")
|
||||
send_daily_alerts()
|
||||
_complete_cron_run("a")
|
||||
return org
|
||||
|
||||
|
||||
def handler_b():
|
||||
_start_cron_run("b")
|
||||
_create_ops_alert("b")
|
||||
send_daily_alerts()
|
||||
|
||||
|
||||
def handler_c():
|
||||
_start_cron_run("c")
|
||||
|
|
@ -0,0 +1,2 @@
|
|||
def _create_ops_alert(name):
|
||||
return f"alert:{name}"
|
||||
|
|
@ -0,0 +1,6 @@
|
|||
def _start_cron_run(name):
|
||||
return f"started:{name}"
|
||||
|
||||
|
||||
def _complete_cron_run(name):
|
||||
return f"completed:{name}"
|
||||
|
|
@ -416,6 +416,213 @@ describe('Python ancestor directory import resolution (Issue #417)', () => {
|
|||
});
|
||||
});
|
||||
|
||||
// ---------------------------------------------------------------------------
|
||||
// Multi-segment ancestor walk: `from services.sync import X` style imports
|
||||
// from a sibling sub-package nested under a shared root directory.
|
||||
//
|
||||
// Before this fix, single-segment ancestor walks worked (`from middleware
|
||||
// import X` from `backend/services/auth.py` → `backend/middleware.py`) but
|
||||
// multi-segment dotted imports were only resolved against the workspace
|
||||
// root. In a `backend/`-prefixed repo, `from services.sync import X` from
|
||||
// `backend/routers/cron.py` would silently drop because `services/sync.py`
|
||||
// does not exist at the workspace root — only `backend/services/sync.py`
|
||||
// does. The fix mirrors the single-segment ancestor walk for multi-segment
|
||||
// paths.
|
||||
// ---------------------------------------------------------------------------
|
||||
|
||||
describe('Python multi-segment ancestor directory import resolution', () => {
|
||||
let result: PipelineResult;
|
||||
|
||||
beforeAll(async () => {
|
||||
result = await runPipelineFromRepo(
|
||||
path.join(FIXTURES, 'python-multi-segment-ancestor-import'),
|
||||
() => {},
|
||||
);
|
||||
}, 60000);
|
||||
|
||||
it('resolves from services.sync import to backend/services/sync.py via ancestor walk', () => {
|
||||
const imports = getRelationships(result, 'IMPORTS');
|
||||
const syncImport = imports.find(
|
||||
(i) =>
|
||||
i.sourceFilePath === 'backend/routers/cron.py' &&
|
||||
i.targetFilePath === 'backend/services/sync.py',
|
||||
);
|
||||
expect(syncImport).toBeDefined();
|
||||
});
|
||||
|
||||
it('resolves from services.alerts import to backend/services/alerts.py via ancestor walk', () => {
|
||||
const imports = getRelationships(result, 'IMPORTS');
|
||||
const alertsImport = imports.find(
|
||||
(i) =>
|
||||
i.sourceFilePath === 'backend/routers/cron.py' &&
|
||||
i.targetFilePath === 'backend/services/alerts.py',
|
||||
);
|
||||
expect(alertsImport).toBeDefined();
|
||||
});
|
||||
|
||||
it('resolves from routers.alerts import to backend/routers/alerts.py (sibling sub-package)', () => {
|
||||
const imports = getRelationships(result, 'IMPORTS');
|
||||
const routerImport = imports.find(
|
||||
(i) =>
|
||||
i.sourceFilePath === 'backend/routers/cron.py' &&
|
||||
i.targetFilePath === 'backend/routers/alerts.py',
|
||||
);
|
||||
expect(routerImport).toBeDefined();
|
||||
});
|
||||
|
||||
it('emits CALLS edges for every multi-segment-imported callee', () => {
|
||||
const calls = getRelationships(result, 'CALLS').filter(
|
||||
(c) => c.sourceFilePath === 'backend/routers/cron.py',
|
||||
);
|
||||
|
||||
const startCronRunCalls = calls.filter((c) => c.target === '_start_cron_run');
|
||||
expect(startCronRunCalls.length).toBe(3);
|
||||
expect(startCronRunCalls.every((c) => c.targetFilePath === 'backend/services/sync.py')).toBe(
|
||||
true,
|
||||
);
|
||||
|
||||
const completeCronRunCalls = calls.filter((c) => c.target === '_complete_cron_run');
|
||||
expect(completeCronRunCalls.length).toBe(1);
|
||||
expect(completeCronRunCalls[0].targetFilePath).toBe('backend/services/sync.py');
|
||||
|
||||
const opsAlertCalls = calls.filter((c) => c.target === '_create_ops_alert');
|
||||
expect(opsAlertCalls.length).toBe(2);
|
||||
expect(opsAlertCalls.every((c) => c.targetFilePath === 'backend/services/alerts.py')).toBe(
|
||||
true,
|
||||
);
|
||||
|
||||
const sendDailyCalls = calls.filter((c) => c.target === 'send_daily_alerts');
|
||||
expect(sendDailyCalls.length).toBe(2);
|
||||
expect(sendDailyCalls.every((c) => c.targetFilePath === 'backend/routers/alerts.py')).toBe(
|
||||
true,
|
||||
);
|
||||
});
|
||||
|
||||
it('preserves single-segment ancestor walk (regression check for from auth_utils import X)', () => {
|
||||
const calls = getRelationships(result, 'CALLS').filter(
|
||||
(c) => c.sourceFilePath === 'backend/routers/cron.py',
|
||||
);
|
||||
|
||||
const verifyCalls = calls.filter((c) => c.target === 'verify_cron_secret');
|
||||
expect(verifyCalls.length).toBe(1);
|
||||
expect(verifyCalls[0].targetFilePath).toBe('backend/auth_utils.py');
|
||||
|
||||
const orgCalls = calls.filter((c) => c.target === 'get_org_id_from_header');
|
||||
expect(orgCalls.length).toBe(1);
|
||||
expect(orgCalls[0].targetFilePath).toBe('backend/auth_utils.py');
|
||||
});
|
||||
});
|
||||
|
||||
// ---------------------------------------------------------------------------
|
||||
// Negative case for `hasRepoCandidate` widening: a vendored copy of an
|
||||
// external package (e.g. `vendor/django/urls.py`) must not cause an external
|
||||
// import like `from django.urls import path` issued from an unrelated file
|
||||
// (`app/main.py`) to be treated as a local candidate. The ancestor-bounded
|
||||
// nested check rejects vendored matches that don't sit on the importer's
|
||||
// own ancestor path.
|
||||
// ---------------------------------------------------------------------------
|
||||
|
||||
describe('Python multi-segment widening: vendored external package false-positive guard', () => {
|
||||
let repoDir: string;
|
||||
let result: PipelineResult;
|
||||
|
||||
beforeAll(async () => {
|
||||
repoDir = fs.mkdtempSync(path.join(os.tmpdir(), 'gn-python-vendored-django-'));
|
||||
writeFixtureRepo(repoDir, {
|
||||
'app/main.py': `from django.urls import path
|
||||
|
||||
def boot():
|
||||
path("/")
|
||||
`,
|
||||
'vendor/django/__init__.py': '',
|
||||
'vendor/django/urls.py': `def path(p):
|
||||
return p
|
||||
`,
|
||||
});
|
||||
result = await runPipelineFromRepo(repoDir, () => {});
|
||||
}, 60000);
|
||||
|
||||
afterAll(() => {
|
||||
if (repoDir !== undefined) fs.rmSync(repoDir, { recursive: true, force: true });
|
||||
});
|
||||
|
||||
it('does not resolve from django.urls to vendor/django/urls.py from an unrelated importer', () => {
|
||||
const imports = getRelationships(result, 'IMPORTS');
|
||||
const stray = imports.find(
|
||||
(i) => i.sourceFilePath === 'app/main.py' && i.targetFilePath === 'vendor/django/urls.py',
|
||||
);
|
||||
expect(stray).toBeUndefined();
|
||||
});
|
||||
|
||||
it('does not emit a CALLS edge from app/main.py:boot to vendor/django/urls.py:path', () => {
|
||||
const calls = getRelationships(result, 'CALLS').filter(
|
||||
(c) => c.sourceFilePath === 'app/main.py',
|
||||
);
|
||||
const stray = calls.find(
|
||||
(c) => c.target === 'path' && c.targetFilePath === 'vendor/django/urls.py',
|
||||
);
|
||||
expect(stray).toBeUndefined();
|
||||
});
|
||||
});
|
||||
|
||||
// ---------------------------------------------------------------------------
|
||||
// Workspace-root precedence: when both `services/sync.py` (root) and
|
||||
// `backend/services/sync.py` (ancestor) exist, an importer at
|
||||
// `backend/routers/cron.py` doing `from services.sync import X` resolves to
|
||||
// the root file. Mirrors Python's `sys.path` semantics where the project
|
||||
// root is searched before package-local namespaces.
|
||||
// ---------------------------------------------------------------------------
|
||||
|
||||
describe('Python multi-segment resolution: workspace root wins over ancestor', () => {
|
||||
let repoDir: string;
|
||||
let result: PipelineResult;
|
||||
|
||||
beforeAll(async () => {
|
||||
repoDir = fs.mkdtempSync(path.join(os.tmpdir(), 'gn-python-root-precedence-'));
|
||||
writeFixtureRepo(repoDir, {
|
||||
'services/__init__.py': '',
|
||||
'services/sync.py': `def root_marker():
|
||||
return "root"
|
||||
`,
|
||||
'backend/__init__.py': '',
|
||||
'backend/services/__init__.py': '',
|
||||
'backend/services/sync.py': `def ancestor_marker():
|
||||
return "ancestor"
|
||||
`,
|
||||
'backend/routers/__init__.py': '',
|
||||
'backend/routers/cron.py': `from services.sync import root_marker
|
||||
|
||||
def handler():
|
||||
return root_marker()
|
||||
`,
|
||||
});
|
||||
result = await runPipelineFromRepo(repoDir, () => {});
|
||||
}, 60000);
|
||||
|
||||
afterAll(() => {
|
||||
if (repoDir !== undefined) fs.rmSync(repoDir, { recursive: true, force: true });
|
||||
});
|
||||
|
||||
it('resolves the import edge to the root services/sync.py, not backend/services/sync.py', () => {
|
||||
const imports = getRelationships(result, 'IMPORTS').filter(
|
||||
(i) => i.sourceFilePath === 'backend/routers/cron.py',
|
||||
);
|
||||
const rootEdge = imports.find((i) => i.targetFilePath === 'services/sync.py');
|
||||
expect(rootEdge).toBeDefined();
|
||||
|
||||
const ancestorEdge = imports.find((i) => i.targetFilePath === 'backend/services/sync.py');
|
||||
expect(ancestorEdge).toBeUndefined();
|
||||
});
|
||||
|
||||
it('binds the imported name to the root file, not the ancestor copy', () => {
|
||||
const calls = getRelationships(result, 'CALLS').filter(
|
||||
(c) => c.sourceFilePath === 'backend/routers/cron.py' && c.target === 'root_marker',
|
||||
);
|
||||
expect(calls.length).toBe(1);
|
||||
expect(calls[0].targetFilePath).toBe('services/sync.py');
|
||||
});
|
||||
});
|
||||
|
||||
// ---------------------------------------------------------------------------
|
||||
// Re-export chain: from .base import X barrel pattern via __init__.py
|
||||
// ---------------------------------------------------------------------------
|
||||
|
|
|
|||
Loading…
Add table
Reference in a new issue