Fixes #24208: team ownership broken for special-character names and partial name matches#27453
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}
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
|
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 7 out of 7 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (1)
openmetadata-ui/src/main/resources/ui/src/components/Database/TableVersion/TableVersion.component.tsx:91
columnsLoadingis initialized totrueeven though columns are now derived locally fromcurrentVersionData(no async fetch). This makesVersionTablerender in a loading state on first paint and can cause a visible spinner/flicker despite having data available. Consider initializingcolumnsLoadingtofalse(orisVersionLoading) and/or computing the initialtableColumnsslice in the initial state so the table can render immediately without a loading phase.
// Pagination state for columns
const [tableColumns, setTableColumns] = useState<Column[]>([]);
const [columnsLoading, setColumnsLoading] = useState(true); // Start with loading state
const [changeDescription, setChangeDescription] = useState<ChangeDescription>(
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 7 out of 7 changed files in this pull request and generated 3 comments.
Comments suppressed due to low confidence (1)
openmetadata-ui/src/main/resources/ui/src/components/Database/TableVersion/TableVersion.component.tsx:149
searchTextis included in theuseMemodependency list but is not used in the memoized object. This creates unnecessary recomputation and is misleading for future maintenance. RemovesearchTextfrom the dependency array (or use it in the memo if there’s an intended coupling).
const paginationProps = useMemo(
() => ({
currentPage,
showPagination,
isLoading: columnsLoading,
isNumberBased: true,
pageSize,
paging,
pagingHandler: handleColumnsPageChange,
onShowSizeChange: handlePageSizeChange,
}),
[
currentPage,
showPagination,
columnsLoading,
searchText,
pageSize,
paging,
handleColumnsPageChange,
handlePageSizeChange,
]
);
| useEffect(() => { | ||
| if (tableFqn && !isVersionLoading) { | ||
| // Reset to first page when search changes | ||
| fetchPaginatedColumns(currentPage, searchText || undefined); | ||
| if (isVersionLoading) { | ||
| return; | ||
| } | ||
| }, [ | ||
| isVersionLoading, | ||
| tableFqn, | ||
| searchText, | ||
| fetchPaginatedColumns, | ||
| pageSize, | ||
| currentPage, | ||
| ]); | ||
| const offset = (currentPage - 1) * pageSize; | ||
| setTableColumns(filteredHistoricalColumns.slice(offset, offset + pageSize)); | ||
| handlePagingChange({ | ||
| offset, | ||
| limit: pageSize, | ||
| total: filteredHistoricalColumns.length, | ||
| }); | ||
| setColumnsLoading(false); | ||
| }, [isVersionLoading, filteredHistoricalColumns, currentPage, pageSize]); |
There was a problem hiding this comment.
useEffect calls handlePagingChange but it isn’t listed in the dependency array. This can cause stale-closure behavior (and will typically fail react-hooks/exhaustive-deps). Add handlePagingChange to the dependency list (or ensure usePaging returns a stable callback and explicitly document/disable the lint rule for this line).
| logger.warning( | ||
| f"Failed to resolve owner reference for '{name}' due to: {err}. " | ||
| "Skipping owner assignment." | ||
| ) |
There was a problem hiding this comment.
This warning is emitted for both team and user resolution, and also when is_owner=False, but the message always says "owner reference" / "Skipping owner assignment". Make the message accurate for all call paths (e.g., "Failed to resolve entity reference" and optionally include whether is_owner was requested).
| except Exception as err: | ||
| logger.debug(traceback.format_exc()) | ||
| logger.warning( | ||
| f"Failed to resolve owner reference for '{name}' due to: {err}. " | ||
| "Skipping owner assignment." | ||
| ) |
There was a problem hiding this comment.
The current approach logs the traceback separately at debug and then logs a warning without exception context. Prefer a single warning log with attached exception info (so stack traces are retained when needed) and avoid f-string interpolation in logger calls to keep logs structured and defer formatting costs.
|
There was a problem hiding this comment.
@miantalha45, the changes do not look related to the issue. Can we please revert these and the test file changes as well?
There was a problem hiding this comment.
Sure, let me revert these changes.
Code Review ✅ Approved 1 resolved / 1 findingsUpdates team ownership lookups to correctly handle special characters and exact name matching, resolving the API error propagation issue. No issues found. ✅ 1 resolved✅ Edge Case: get_by_name propagates non-404 APIError unlike old ES path
OptionsDisplay: compact → Showing less information. Comment with these commands to change:
Was this helpful? React with 👍 / 👎 | Gitar |
|
@aniketkatkar97 I have reverted all unnecessary changes. |
|
@aniketkatkar97 @harshsoni2024 Please have a look on this PR and let me know if any changes are required. |
|
@miantalha45, it's already approved. Thanks. |
That's great. |
|
@harshach sir, i would love if you can merge this PR today as today is last day of hackathon. |
|
Changes
Changes
Related
Fixes #24208
Type of change:
Checklist:
Fixes <issue-number>: <short explanation>Before
After
Summary by Gitar
TeamOwnershipSpecialCharacters.spec.tsas the E2E test file, which was previously added as part of the fix.This will update automatically on new commits.