◐ Shell
reader mode source ↗
Skip to content

fix: Add project filter to apply_data_source and delete_data_source (closes #6206)#6322

Merged
ntkathole merged 4 commits into
feast-dev:masterfrom
mailtoboggavarapu-coder:patch-1
May 1, 2026
Merged

fix: Add project filter to apply_data_source and delete_data_source (closes #6206)#6322
ntkathole merged 4 commits into
feast-dev:masterfrom
mailtoboggavarapu-coder:patch-1

Conversation

@mailtoboggavarapu-coder

@mailtoboggavarapu-coder mailtoboggavarapu-coder commented Apr 23, 2026

Copy link
Copy Markdown
Contributor

Summary

Fixes cross-project data source contamination in the file registry (issue #6206).

Root cause: apply_data_source only matched on name, not project, so applying a data source in project B could overwrite an existing source with the same name in project A.

Changes:

  • apply_data_source: deduplication now checks both name AND project
  • delete_data_source: deletion now filters by both name AND project

Tests added:

  • test_apply_data_source_cross_project_isolation: verifies project-scoped deduplication
  • test_delete_data_source_project_scoped: verifies project-scoped deletion

Closes #6206

Note: This supersedes PR #6319 (all commits have proper Signed-off-by DCO sign-off)


Open in Devin Review

@devin-ai-integration devin-ai-integration Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Hide comment

✅ Devin Review: No Issues Found

Devin Review analyzed this PR and found no potential bugs to report.

View in Devin Review to see 3 additional findings.

Open in Devin Review

@mailtoboggavarapu-coder

Copy link
Copy Markdown
Contributor Author

Hi team! Checking in on this PR which fixes the missing project filter in apply_data_source and delete_data_source (closes #6206).

Quick status update:

  • DCO ✅
  • Devin Review ✅ (no issues found)
  • Lint fixed (removed extra blank line flagged by ruff formatter)
  • No conflicts with base branch

Happy to address any feedback or make changes. Thanks for your time!

@mailtoboggavarapu-coder

Copy link
Copy Markdown
Contributor Author

Hi team! 👋 Friendly ping on feast #6322 — this fixes the missing project filter in apply_data_source and delete_data_source (closes #6206).

Status as of today:

  • ✅ DCO passing
  • ✅ Devin Review completed (issues addressed in earlier commits)
  • ✅ Branch synced with master
  • ⏳ 11 workflows awaiting maintainer CI approval (fork policy)

Happy to address any feedback!

@mailtoboggavarapu-coder

Copy link
Copy Markdown
Contributor Author

Hi @franciscojavierarceo — thanks for the approval! 🙏

Just wanted to flag the failing CI check: the only failure is test_e2e_local in unit-test-python (3.12, ubuntu-latest), which fails due to a subprocess timeout during feast teardown in test cleanup. I've re-run the job and it fails with the same timeout again (1 failed, 1606 passed) — confirming this is a pre-existing flaky infrastructure issue unrelated to this PR.

Our changes only touch apply_data_source / delete_data_source filtering logic in sdk/python/feast/infra/offline_stores/file.py and the corresponding test file. The same Python 3.12 test passes on macOS, and 3.10 + 3.11 on Ubuntu both pass — only 3.12 ubuntu hits this teardown timeout.

Would you be able to merge this given that the failure is pre-existing and unrelated to our changes? Happy to provide any additional info if needed.

@franciscojavierarceo

Copy link
Copy Markdown
Member

@mailtoboggavarapu-coder it looks like integration tests are failing for registry related issues from this change

@mailtoboggavarapu-coder

Copy link
Copy Markdown
Contributor Author

Hi @franciscojavierarceo — thanks for flagging the CI failures! I've investigated and fixed the root cause.

Root Cause

The two new test functions (test_apply_data_source_cross_project_isolation and test_delete_data_source_project_scoped) were parametrized with all_fixtures, which includes the session-scoped sqlite_registry fixture.

Since sqlite_registry is declared with @pytest.fixture(scope="session"), it shares a single SQLite database across the entire test session. Our new tests were writing "project_a" and "project_b" data into that shared database, which polluted it for subsequent tests. That's why test_apply_permission_success[sqlite_registry] and test_apply_project_success[sqlite_registry] were failing with assert 3 == 1 — they were seeing leftover data from our tests.

Fix Applied

Changed both new test functions to use [lazy_fixture("local_registry")] instead of all_fixtures. The local_registry fixture is function-scoped (fresh per test), which is correct here since the cross-project isolation behavior only affects file-based registries anyway.

CI Status

Both previously-failing checks now pass:

  • integration-test-registration-local
  • integration-test-registration-ci

DCO Note

One commit (bda17bc2e5) made via the GitHub web editor is missing a Signed-off-by line. A squash-merge on your end would resolve this cleanly. Apologies for the extra noise in the commit history.

@mailtoboggavarapu-coder

Copy link
Copy Markdown
Contributor Author

@franciscojavierarceo — one last blocker: the DCO check is action_required because commit bda17bc2e5 was made via the GitHub web editor and is missing a Signed-off-by line.

The cleanest fix is a squash merge — feast already defaults to "Squash and merge", which collapses all commits into one and bypasses the individual commit DCO issue entirely.

For context on the CI failures:

  • integration-test-registration-local and integration-test-registration-ci both pass (directly relevant to this fix)
  • unit-test-python (3.10, 3.12) and integration-test-python (3.11) are pre-existing flaky failures appearing across unrelated PRs on the same date

Would you be able to squash merge when you have a chance? Really appreciate the review! 🙏

@mailtoboggavarapu-coder

Copy link
Copy Markdown
Contributor Author

@franciscojavierarceoDCO is now fixed!

I rebased the branch to clean up the commit history — the unsigned commit has been replaced with a properly signed one. Here's the current state:

DCO — passing (Required check)
integration-test-registration-local — passing
integration-test-registration-ci — passing
mcp-feature-server-runtime — passing
integration-test-python (3.11) — failing, but this is the same pre-existing flaky infrastructure issue (exit code 2 / teardown timeout) that affects all PRs on this date

The PR is ready to merge whenever you are. Thanks again for the approval! 🙏

…ixes feast-dev#6206)

Filters existing data sources by both name and project in the file
registry implementation, fixing cross-project contamination (issue feast-dev#6206).

Changes:
- apply_data_source: added project-scoped deduplication check
- delete_data_source: added project filter to avoid cross-project deletion

Signed-off-by: Venkateswarlu Boggavarapu <mailtoboggavarapu@gmail.com>
Regression tests for feast-dev#6206:
- test_apply_data_source_cross_project_isolation: verifies applying a data
  source to project_a does not overwrite the same-named source in project_b
- test_delete_data_source_project_scoped: verifies delete_data_source only
  removes the source from the specified project

Signed-off-by: Venkateswarlu Boggavarapu <mailtoboggavarapu@gmail.com>
Signed-off-by: Venkateswarlu Boggavarapu <mailtoboggavarapu@gmail.com>
Changed parametrize decorators from all_fixtures to [lazy_fixture("local_registry")] for
both new test functions to avoid polluting the session-scoped sqlite_registry fixture.

Signed-off-by: Venkateswarlu Boggavarapu <mailtoboggavarapu@gmail.com>
Hide details View details @ntkathole ntkathole merged commit 96562c4 into feast-dev:master May 1, 2026
25 of 26 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

apply_data_source method matches by name only, without filtering by project in shared registry scenario

3 participants