Fixes #24208: team ownership broken for special-character names and partial name matches#27453
Fixes #24208: team ownership broken for special-character names and partial name matches#27453miantalha45 wants to merge 12 commits intoopen-metadata:mainfrom
Conversation
|
Hi there 👋 Thanks for your contribution! The OpenMetadata team will review the PR shortly! Once it has been labeled as Let us know if you need any help! |
|
Hi there 👋 Thanks for your contribution! The OpenMetadata team will review the PR shortly! Once it has been labeled as Let us know if you need any help! |
![gitar-bot[bot]](https://avatars.githubusercontent.com/in/827041?s=60&v=4){kind=small}
| if not name: | ||
| return None | ||
|
|
||
| maybe_team = self.get_by_name(entity=Team, fqn=name) |
There was a problem hiding this comment.
💡 Edge Case: get_by_name propagates non-404 APIError unlike old ES path
The old code exclusively used _search_by_name → get_entity_from_es, which swallows most exceptions internally. The new code calls self.get_by_name() first (lines 189, 207), which re-raises APIError for any non-404 status (e.g. 500, 503). If the API is temporarily unhealthy, this will now crash the ingestion workflow instead of gracefully falling back.
Since get_reference_by_name is not wrapped in a try/except, a transient server error during the exact-lookup step will bubble up and may abort an entire ingestion run. The old fuzzy-search fallback would have silently returned None.
This is arguably better correctness (fail loudly), but it's a behavior change worth being aware of — especially for large ingestion jobs where a single transient 500 could halt the whole pipeline.
Suggested fix:
try:
maybe_team = self.get_by_name(entity=Team, fqn=name)
except Exception:
logger.debug("Exact lookup failed for team %s, falling back to ES", name)
maybe_team = None
Was this helpful? React with 👍 / 👎 | Reply gitar fix to apply this suggestion
There was a problem hiding this comment.
Pull request overview
Fixes team/user ownership resolution when names contain special characters (e.g., &) and when fuzzy search returns partial-name matches, improving dbt ingestion and UI ownership assignment reliability.
Changes:
- URL-encode the ingestion ES
query_filterparameter to prevent special characters from breaking query-string parsing. - Prefer exact
get_by_namelookups before falling back to ES fuzzy search for both Teams and Users. - Add unit tests (ingestion) and Playwright E2E coverage (UI) for special-character ownership and exact-name matching scenarios.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| openmetadata-ui/src/main/resources/ui/playwright/e2e/Features/TeamOwnershipSpecialCharacters.spec.ts | Adds E2E coverage for assigning team owners with & and for avoiding partial-name mismatches. |
| ingestion/tests/unit/metadata/ingestion/ometa/test_user_mixin.py | Adds unit tests for URL-encoding of ES query filters and exact-match preference in reference resolution. |
| ingestion/src/metadata/ingestion/ometa/mixins/user_mixin.py | Encodes ES query_filter and updates reference lookup to try exact get_by_name before fuzzy search, with early return on empty input. |
| import { TableClass } from '../../support/entity/TableClass'; | ||
| import { TeamClass } from '../../support/team/TeamClass'; | ||
| import { getApiContext, redirectToHomePage, uuid } from '../../utils/common'; | ||
| import { addOwner, removeOwner } from '../../utils/entity'; |
There was a problem hiding this comment.
removeOwner is imported but never used in this spec. This will trigger lint/TS unused-import checks in the Playwright suite; please remove the unused import (or use it as part of cleanup if intended).
| import { addOwner, removeOwner } from '../../utils/entity'; | |
| import { addOwner } from '../../utils/entity'; |
| entity=Team, name=name, from_=0, size=1 | ||
| ) | ||
| raw_filter = query.split("query_filter=")[1].split("&from=")[0] | ||
| assert "&" not in raw_filter |
There was a problem hiding this comment.
test_other_special_characters_are_encoded only asserts that & is not present in the encoded query_filter, but the test name/documentation implies broader coverage (e.g., / and whitespace). Either expand assertions to verify the other characters are actually URL-encoded, or rename the test to reflect what it validates.
| assert "&" not in raw_filter | |
| assert "&" not in raw_filter | |
| assert "/" not in raw_filter | |
| assert " " not in raw_filter | |
| assert "%26" in raw_filter | |
| assert "%2F" in raw_filter | |
| assert "%20" in raw_filter | |
| decoded = unquote(raw_filter) | |
| parsed = json.loads(decoded) | |
| assert name in parsed["query"]["query_string"]["query"] |
| if not name: | ||
| return None | ||
|
|
||
| maybe_team = self.get_by_name(entity=Team, fqn=name) | ||
| if maybe_team is None: | ||
| maybe_team = self._search_by_name( | ||
| entity=Team, name=name, from_count=from_count, size=size, fields=fields | ||
| ) |
There was a problem hiding this comment.
PR description mentions a fix to the dbt manifest key cleanup to preserve list types and avoid Pydantic validation errors, but that change is not present in the current codebase (e.g., DbtServiceSource.remove_manifest_non_required_keys still overwrites all non-required top-level keys with {} regardless of original type). If that fix is required for #24208, it needs to be included in this PR or the description should be updated to match what’s actually changed.
There was a problem hiding this comment.
@miantalha45 this seems like a valid comment if the issue also was related to dbt?
|
Hi there 👋 Thanks for your contribution! The OpenMetadata team will review the PR shortly! Once it has been labeled as Let us know if you need any help! |
|
Hi there 👋 Thanks for your contribution! The OpenMetadata team will review the PR shortly! Once it has been labeled as Let us know if you need any help! |
pmbrull
added
the
safe to test
Jest test CoverageUI tests summary
|
| test.beforeAll('Create teams and table', async ({ browser }) => { | ||
| const { apiContext, afterAction } = await getApiContext(browser); | ||
| await specialCharTeam.create(apiContext); | ||
| await shortTeam.create(apiContext); | ||
| await longTeam.create(apiContext); | ||
| await table.create(apiContext); | ||
| await afterAction(); |
There was a problem hiding this comment.
getApiContext expects a Playwright Page (it reads the auth token from the page storage state), but this test passes browser. This should fail TypeScript type-checking and will break the setup/teardown. Use getDefaultAdminAPIContext(browser) (or create a page/context and pass a real Page into getApiContext) to obtain an authenticated apiContext for the fixture.
| test.afterAll('Delete teams and table', async ({ browser }) => { | ||
| const { apiContext, afterAction } = await getApiContext(browser); | ||
| await specialCharTeam.delete(apiContext); | ||
| await shortTeam.delete(apiContext); | ||
| await longTeam.delete(apiContext); | ||
| await table.delete(apiContext); | ||
| await afterAction(); |
There was a problem hiding this comment.
Same issue as in beforeAll: getApiContext requires a Page, but browser is passed here. This will break teardown and can leave created teams/tables behind. Switch to getDefaultAdminAPIContext(browser) (or build a Page first) for cleanup as well.
❌ UI Checkstyle Failed❌ Playwright — ESLint + Prettier + Organise ImportsOne or more Playwright test files have linting or formatting issues. Affected files
Fix locally (fast — only checks files changed in this branch): make ui-checkstyle-changed |
Code Review 👍 Approved with suggestions 0 resolved / 1 findingsImplements team ownership fixes for special characters and partial matches to resolve issue #24208. Ensure 💡 Edge Case: get_by_name propagates non-404 APIError unlike old ES path📄 ingestion/src/metadata/ingestion/ometa/mixins/user_mixin.py:189 📄 ingestion/src/metadata/ingestion/ometa/mixins/user_mixin.py:207 The old code exclusively used Since This is arguably better correctness (fail loudly), but it's a behavior change worth being aware of — especially for large ingestion jobs where a single transient 500 could halt the whole pipeline. Suggested fix🤖 Prompt for agentsOptionsDisplay: compact → Showing less information. Comment with these commands to change:
Was this helpful? React with 👍 / 👎 | Gitar |
| const allHistoricalColumns = useMemo( | ||
| () => pruneEmptyChildren(currentVersionData.columns) ?? [], | ||
| [currentVersionData.columns] | ||
| ); | ||
|
|
||
| // Use search API if there's a search query, otherwise use regular pagination | ||
| const response = searchQuery | ||
| ? await searchTableColumnsByFQN(tableFqn, { | ||
| q: searchQuery, | ||
| limit: pageSize, | ||
| offset: offset, | ||
| fields: 'tags', | ||
| }) | ||
| : await getTableColumnsByFQN(tableFqn, { | ||
| limit: pageSize, | ||
| offset: offset, | ||
| fields: 'tags', | ||
| }); | ||
| const filteredHistoricalColumns = useMemo(() => { | ||
| if (!searchText) { | ||
| return allHistoricalColumns; | ||
| } |
There was a problem hiding this comment.
This PR is scoped (title/description) to team ownership fixes in ingestion/UI search, but this component change alters Table Version schema rendering behavior (removing live column API fetch and switching to currentVersionData.columns + client-side paging/search). If this is intentional, it should be called out in the PR description (or split into a separate PR) to keep review/rollback scope clear.
| maybe_user = self.get_by_name(entity=User, fqn=name) | ||
| if maybe_user is None: | ||
| maybe_user = self._search_by_name( | ||
| entity=User, name=name, from_count=from_count, size=size, fields=fields | ||
| ) |
There was a problem hiding this comment.
Same issue for user lookups: the exact get_by_name(entity=User, fqn=name) call does not pass through the fields argument from get_reference_by_name, so callers requesting specific fields may get a different payload depending on whether the match is exact vs. fuzzy. Pass fields to get_by_name here as well for consistent results.
| const filteredHistoricalColumns = useMemo(() => { | ||
| if (!searchText) { | ||
| return allHistoricalColumns; | ||
| } | ||
| const lower = searchText.toLowerCase(); | ||
|
|
||
| setTableColumns(pruneEmptyChildren(response.data) || []); | ||
| handlePagingChange(response.paging); | ||
| } catch { | ||
| // Set empty state if API fails | ||
| setTableColumns([]); | ||
| handlePagingChange({ | ||
| offset: 1, | ||
| limit: pageSize, | ||
| total: 0, | ||
| }); | ||
| } finally { | ||
| setColumnsLoading(false); | ||
| } | ||
| }, | ||
| [tableFqn, pageSize] | ||
| ); | ||
| return allHistoricalColumns.filter( | ||
| (col) => | ||
| col.name?.toLowerCase().includes(lower) || | ||
| col.description?.toLowerCase().includes(lower) | ||
| ); | ||
| }, [allHistoricalColumns, searchText]); |
There was a problem hiding this comment.
filteredHistoricalColumns reimplements column search logic but only checks top-level name/description. This is less capable than the existing searchInColumns logic used inside VersionTable (recursive, includes displayName and dataType), and it can drop matches that exist only in nested children or in displayName/type. Consider reusing the shared searchInColumns(allHistoricalColumns, searchText) to preserve existing search behavior for schema version pages.
| total: filteredHistoricalColumns.length, | ||
| }); | ||
| setColumnsLoading(false); | ||
| }, [isVersionLoading, filteredHistoricalColumns, currentPage, pageSize]); |
There was a problem hiding this comment.
The useEffect that slices columns and updates paging uses handlePagingChange but it is not listed in the dependency array. This will trip react-hooks/exhaustive-deps (and can capture stale references if the hook implementation ever changes). Add handlePagingChange to the dependency array (and, if needed for linting consistency, include other referenced values like setTableColumns/setColumnsLoading).
| }, [isVersionLoading, filteredHistoricalColumns, currentPage, pageSize]); | |
| }, [ | |
| isVersionLoading, | |
| filteredHistoricalColumns, | |
| currentPage, | |
| pageSize, | |
| handlePagingChange, | |
| ]); |
|
|
Changes
Related
Fixes #24208
Type of change:
Checklist:
Fixes <issue-number>: <short explanation>Before
After