Fix UserDAO missing parameter domainEntityType #27190#27252
Fix UserDAO missing parameter domainEntityType #27190#27252aniruddhaadak80 wants to merge 7 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! |
![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 PostgreSQL user listing failures caused by unbound named parameters (e.g., :domainEntityType) when ListFilter injects additional bind params during condition building.
Changes:
- Adds
@BindMapparams support toCollectionDAOuser listing queries (listCount,listBefore,listAfter) and passesfilter.getQueryParams()through. - Introduces additional tag-pruning logic in
TableRepository.addDataModelfor table and column tags (automated tags not present in the incoming model).
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
openmetadata-service/src/main/java/org/openmetadata/service/jdbi3/CollectionDAO.java |
Binds ListFilter query params via @BindMap to avoid “Missing named parameter” errors (e.g., domain filtering). |
openmetadata-service/src/main/java/org/openmetadata/service/jdbi3/TableRepository.java |
Adds logic to remove certain automated tags during DataModel application (potentially unrelated to the PR’s stated bugfix). |
| return listCount( | ||
| getTableName(), mySqlCondition, postgresCondition, team, Relationship.HAS.ordinal()); | ||
| getTableName(), mySqlCondition, postgresCondition, team, Relationship.HAS.ordinal(), filter.getQueryParams()); | ||
| } |
There was a problem hiding this comment.
Passing filter.getQueryParams() into the new @BindMap will also bind the raw "team" query param (if present). Since this method already binds :team via @BindFQN (hashed), the later @BindMap binding can override it and break team filtering (te.nameHash = :team). Consider passing a sanitized copy of queryParams that removes keys already bound explicitly (at least "team").
| if (table.getTags() != null) { | ||
| java.util.List<String> incomingTags = dataModel.getTags() != null | ||
| ? dataModel.getTags().stream().map(org.openmetadata.schema.type.TagLabel::getTagFQN).collect(java.util.stream.Collectors.toList()) | ||
| : java.util.Collections.emptyList(); | ||
| mergedTableTags.removeIf(t -> t.getLabelType() == org.openmetadata.schema.type.TagLabel.LabelType.AUTOMATED && !incomingTags.contains(t.getTagFQN())); | ||
| } |
There was a problem hiding this comment.
This pruning logic removes all AUTOMATED tags whenever dataModel.getTags() is null/empty (incomingTags becomes empty), even though mergeTagsWithIncomingPrecedence() intentionally preserves existing tags when there are no incoming tags. That changes behavior and can unintentionally drop automated/derived tags on addDataModel calls that don't include tags. Only prune automated tags when the request explicitly provides tags (e.g., guard on dataModel.getTags() != null / not empty).
| if (stored.getTags() != null) { | ||
| java.util.List<String> incomingColTags = modelColumn.getTags() != null | ||
| ? modelColumn.getTags().stream().map(org.openmetadata.schema.type.TagLabel::getTagFQN).collect(java.util.stream.Collectors.toList()) | ||
| : java.util.Collections.emptyList(); | ||
| mergedColumnTags.removeIf(t -> t.getLabelType() == org.openmetadata.schema.type.TagLabel.LabelType.AUTOMATED && !incomingColTags.contains(t.getTagFQN())); | ||
| } |
There was a problem hiding this comment.
Same issue at column level: when modelColumn.getTags() is null/empty, incomingColTags becomes empty and this removes all existing AUTOMATED column tags despite mergeTagsWithIncomingPrecedence() returning the existing tags in that case. This can lead to unintended tag loss; only apply this pruning when modelColumn.getTags() is explicitly provided.
| List<TagLabel> mergedTableTags = | ||
| mergeTagsWithIncomingPrecedence(table.getTags(), dataModel.getTags()); | ||
| if (table.getTags() != null) { | ||
| java.util.List<String> incomingTags = dataModel.getTags() != null | ||
| ? dataModel.getTags().stream().map(org.openmetadata.schema.type.TagLabel::getTagFQN).collect(java.util.stream.Collectors.toList()) | ||
| : java.util.Collections.emptyList(); | ||
| mergedTableTags.removeIf(t -> t.getLabelType() == org.openmetadata.schema.type.TagLabel.LabelType.AUTOMATED && !incomingTags.contains(t.getTagFQN())); | ||
| } |
There was a problem hiding this comment.
The PR description is focused on fixing missing JDBI bindings for domainEntityType in user listing, but this hunk introduces additional tag-removal behavior in TableRepository.addDataModel. If this is intentional, it should be called out in the PR description and justified; otherwise it should be reverted/split to keep the PR scoped to the reported issue.
|
@aniruddhaadak80 thanks for the PR. However tests are must for every change, we won't be able to merge without it |
…gs wipe and fqns
|
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! |
| limit, | ||
| beforeName, | ||
| beforeId, | ||
| Relationship.HAS.ordinal()); | ||
| Relationship.HAS.ordinal(), | ||
| filter.getQueryParams()); |
There was a problem hiding this comment.
Same potential bind-name collision as in listCount: filter.getQueryParams() is likely to include team, and binding it via @BindMap can override the explicit @BindFQN("team") binding for :team, causing incorrect results when team filtering is used. Recommend removing team from the map you pass to listBefore(...).
| limit, | ||
| afterName, | ||
| afterId, | ||
| Relationship.HAS.ordinal()); | ||
| Relationship.HAS.ordinal(), | ||
| filter.getQueryParams()); |
There was a problem hiding this comment.
Same potential bind-name collision as in listCount: filter.getQueryParams() may contain team, and binding it via @BindMap can override the explicit @BindFQN("team") binding for :team. Please pass a params map with team removed (or otherwise ensure no duplicate binding names) when calling listAfter(...).
| List<TagLabel> mergedColumnTags = | ||
| mergeTagsWithIncomingPrecedence(stored.getTags(), modelColumn.getTags()); | ||
| if (stored.getTags() != null && modelColumn.getTags() != null) { | ||
| List<String> incomingColTags = modelColumn.getTags().stream().map(TagLabel::getTagFQN).collect(Collectors.toList()); | ||
| mergedColumnTags.removeIf(t -> t.getLabelType() == TagLabel.LabelType.AUTOMATED && !incomingColTags.contains(t.getTagFQN())); | ||
| } |
There was a problem hiding this comment.
Similarly, this changes column tag-merging semantics (removing existing AUTOMATED tags not present in the incoming DataModel column tags), which appears unrelated to the PR’s stated purpose. Please split this change into a separate PR or update the PR description with the rationale/expected behavior change.
| List<String> incomingTags = dataModel.getTags().stream().map(TagLabel::getTagFQN).collect(Collectors.toList()); | ||
| mergedTableTags.removeIf(t -> t.getLabelType() == TagLabel.LabelType.AUTOMATED && !incomingTags.contains(t.getTagFQN())); |
There was a problem hiding this comment.
incomingTags is built as a List and then used with contains(...) inside removeIf, which makes the overall operation O(n*m). Converting incomingTags to a Set would avoid repeated linear scans and is more appropriate for membership checks. Also, these stream chains look unformatted compared to surrounding code and may fail Spotless/google-java-format checks—please reformat to match the file’s style.
| List<String> incomingColTags = modelColumn.getTags().stream().map(TagLabel::getTagFQN).collect(Collectors.toList()); | ||
| mergedColumnTags.removeIf(t -> t.getLabelType() == TagLabel.LabelType.AUTOMATED && !incomingColTags.contains(t.getTagFQN())); |
There was a problem hiding this comment.
Same as the table-level tags: incomingColTags is used for repeated membership checks via contains(...) inside removeIf. Prefer a Set for membership lookups to keep this linear, and reformat the chained stream calls to match the surrounding style (and to satisfy Spotless).
|
I've addressed the checkstyle 120-character line length formatting issues caused by the recent TableRepository changes. Regarding the tests missing for the new UserDAO parameter and TableRepository edge cases, this PR currently cannot run any test suites to confirm additional coverage since the safe to test label is missing (giving an action_required CI block). Could a maintainer please approve run the workflows or advise which test module they prefer these specific backend integration tests in (Python API SDK or Java DropWizard ResourceTest)? |
|
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! |
|
Hello. I am participating in the WeMakeDevs hackathon. Could a maintainer please assign the |
|
Hi. Requesting the |
|
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! |
| int listCount( | ||
| @Define("table") String table, | ||
| @Define("mysqlCond") String mysqlCond, | ||
| @Define("postgresCond") String postgresCond, | ||
| @BindFQN("team") String team, | ||
| @Bind("relation") int relation); | ||
| @Bind("relation") int relation, | ||
| @BindMap java.util.Map<String, String> params); |
There was a problem hiding this comment.
@BindMap is added after @BindFQN("team") in the method signature. Since filter.getQueryParams() commonly contains a team entry (e.g., new ListFilter(...).addQueryParam("team", parentTeam)), the bind map may overwrite the hashed :team binding from @BindFQN, causing team filtering to silently return wrong results. Consider either removing team from the passed params map or reordering parameters so the explicit @BindFQN("team") bind happens after the bind map.
| List<String> listBefore( | ||
| @Define("table") String table, | ||
| @Define("mysqlCond") String mysqlCond, | ||
| @Define("postgresCond") String postgresCond, | ||
| @BindFQN("team") String team, | ||
| @Bind("limit") int limit, | ||
| @Bind("beforeName") String beforeName, | ||
| @Bind("beforeId") String beforeId, | ||
| @Bind("relation") int relation); | ||
| @Bind("relation") int relation, | ||
| @BindMap java.util.Map<String, String> params); |
There was a problem hiding this comment.
Same potential bind override issue as in listCount: the new @BindMap params comes after @BindFQN("team"), and the filter params map may contain team, overwriting the hashed bind and breaking team filtering for pagination queries.
| List<String> listAfter( | ||
| @Define("table") String table, | ||
| @Define("mysqlCond") String mysqlCond, | ||
| @Define("postgresCond") String postgresCond, | ||
| @BindFQN("team") String team, | ||
| @Bind("limit") int limit, | ||
| @Bind("afterName") String afterName, | ||
| @Bind("afterId") String afterId, | ||
| @Bind("relation") int relation); | ||
| @Bind("relation") int relation, | ||
| @BindMap java.util.Map<String, String> params); |
There was a problem hiding this comment.
Same potential bind override issue as in listCount: the new @BindMap params comes after @BindFQN("team"), and the filter params map may contain team, overwriting the hashed bind and breaking team filtering for pagination queries.
| if (table.getTags() != null && dataModel.getTags() != null) { | ||
| List<String> incomingTags = | ||
| dataModel.getTags().stream().map(TagLabel::getTagFQN).collect(Collectors.toList()); | ||
| mergedTableTags.removeIf( | ||
| t -> | ||
| t.getLabelType() == TagLabel.LabelType.AUTOMATED | ||
| && !incomingTags.contains(t.getTagFQN())); | ||
| } |
There was a problem hiding this comment.
This introduces new tag-removal behavior (dropping existing AUTOMATED tags not present in the incoming DataModel tags) that is not described in the PR title/description, which focuses on UserDAO bind params. Please either split this into a separate PR or update the PR description to explain the behavioral change and rationale (and ideally add/adjust tests for the new tag semantics).
| if (stored.getTags() != null && modelColumn.getTags() != null) { | ||
| List<String> incomingColTags = | ||
| modelColumn.getTags().stream().map(TagLabel::getTagFQN).collect(Collectors.toList()); | ||
| mergedColumnTags.removeIf( | ||
| t -> | ||
| t.getLabelType() == TagLabel.LabelType.AUTOMATED | ||
| && !incomingColTags.contains(t.getTagFQN())); | ||
| } |
There was a problem hiding this comment.
This introduces new tag-removal behavior for column tags (dropping existing AUTOMATED tags not present in the incoming modelColumn tags) that is unrelated to the PR’s stated goal (UserDAO bind params). Please split this change out or update the PR description to cover it and ensure expected behavior is validated.
|
@aniruddhaadak80 can you make sure to comment on the github issue so that we can assign this to you. |
|
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! |
|
I have addressed the checkstyle formatting issues caused by the recent TableRepository changes. Regarding the tests missing for the new UserDAO parameter and TableRepository edge cases, this PR currently cannot run any test suites to confirm additional coverage since the safe to test label is missing giving an action_required CI block. Could a maintainer please approve or run the workflows or advise which test module they prefer these specific backend integration tests in. |
…gs wipe and fqns
…gs wipe and fqns
aniruddhaadak80
force-pushed
the
fix-userdao-listcount-param
branch
from
f73cf60 to
d4b6511
Compare
|
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! |
| if (table.getTags() != null && dataModel.getTags() != null) { | ||
| List<String> incomingTags = | ||
| dataModel.getTags().stream().map(TagLabel::getTagFQN).collect(Collectors.toList()); | ||
| mergedTableTags.removeIf( | ||
| t -> | ||
| t.getLabelType() == TagLabel.LabelType.AUTOMATED | ||
| && !incomingTags.contains(t.getTagFQN())); | ||
| } |
There was a problem hiding this comment.
💡 Edge Case: Null dataModel tags skip AUTOMATED tag cleanup
When dataModel.getTags() is null (as opposed to an empty list), the guard if (table.getTags() != null && dataModel.getTags() != null) prevents the stale AUTOMATED tag removal from running. This means if a dbt model previously had tags but now the tags field is entirely absent from the incoming payload, previously-applied AUTOMATED tags will linger.
This may be intentional (null = "not specified" vs empty list = "explicitly no tags"), but it's worth confirming the semantics match the dbt integration contract. The same pattern applies to the column-level logic at line 1447.
Suggested fix:
If null should also trigger cleanup, change the guard to:
if (table.getTags() != null) {
List<String> incomingTags =
dataModel.getTags() == null
? Collections.emptyList()
: dataModel.getTags().stream()
.map(TagLabel::getTagFQN)
.collect(Collectors.toList());
mergedTableTags.removeIf(
t -> t.getLabelType() == TagLabel.LabelType.AUTOMATED
&& !incomingTags.contains(t.getTagFQN()));
}
Was this helpful? React with 👍 / 👎 | Reply gitar fix to apply this suggestion
Code Review 👍 Approved with suggestions 3 resolved / 4 findingsFixes the missing domainEntityType parameter in UserDAO and resolves the unintended removal of automated tags and encoding issues in tests. Consider updating the cleanup logic to include null dataModel tags to avoid skipping AUTOMATED tag removal. 💡 Edge Case: Null dataModel tags skip AUTOMATED tag cleanup📄 openmetadata-service/src/main/java/org/openmetadata/service/jdbi3/TableRepository.java:1422-1429 📄 openmetadata-service/src/main/java/org/openmetadata/service/jdbi3/TableRepository.java:1447-1454 When This may be intentional (null = "not specified" vs empty list = "explicitly no tags"), but it's worth confirming the semantics match the dbt integration contract. The same pattern applies to the column-level logic at line 1447. Suggested fix✅ 3 resolved✅ Edge Case: Null/empty incoming tags removes ALL automated tags
✅ Quality: Use imports instead of fully-qualified class names
✅ Bug: DummyBackendTest.java is UTF-16LE encoded; javac expects UTF-8
🤖 Prompt for agentsOptionsDisplay: compact → Showing less information. Comment with these commands to change:
Was this helpful? React with 👍 / 👎 | Gitar |
| limit, | ||
| beforeName, | ||
| beforeId, | ||
| Relationship.HAS.ordinal()); | ||
| Relationship.HAS.ordinal(), | ||
| filter.getQueryParams()); | ||
| } |
There was a problem hiding this comment.
Same binding conflict as listCount: filter.getQueryParams() can contain team, but :team is also bound via @BindFQN("team"). Passing the full map via @BindMap risks overriding/duplicating the explicit binding. Pass a cleaned copy of the params map with team removed.
| limit, | ||
| afterName, | ||
| afterId, | ||
| Relationship.HAS.ordinal()); | ||
| Relationship.HAS.ordinal(), | ||
| filter.getQueryParams()); | ||
| } |
There was a problem hiding this comment.
Same binding conflict as listCount/listBefore: filter.getQueryParams() may contain team while :team is also bound via @BindFQN("team"). Passing the full map via @BindMap risks overriding/duplicating the explicit binding; pass a cleaned params map (remove team).
| @BindFQN("team") String team, | ||
| @Bind("relation") int relation); | ||
| @Bind("relation") int relation, | ||
| @BindMap java.util.Map<String, String> params); |
There was a problem hiding this comment.
Minor consistency: elsewhere in this file @BindMap parameters use the imported Map type (e.g., CollectionDAO.java:4629), but here the signature uses java.util.Map. Consider using Map<String, String> (with imports) for consistency/readability.
| @BindMap java.util.Map<String, String> params); | |
| @BindMap Map<String, String> params); |
| String user2Name = ns.prefix("domainFilterUser2"); | ||
| CreateUser request2 = | ||
| new CreateUser() | ||
| .withName(user2Name) | ||
| .withEmail(toValidEmail(user2Name)) | ||
| .withDescription("User not in domain"); | ||
| createEntity(request2); | ||
|
|
||
| assertTrue( | ||
| findUserInPaginatedResults(user1.getId(), "domain", testDomain().getName()), | ||
| "User in domain should be in filtered list"); | ||
|
|
||
| // Also assert that we can search for users without any domain (domain=null) | ||
| // This specifically tests the issue #27190 where filtering without a domain was throwing a 500 error | ||
| assertTrue( | ||
| findUserInPaginatedResults(user2.getId(), "domain", "null"), | ||
| "User without domain should be in filtered list with domain=null"); |
There was a problem hiding this comment.
user2 is never assigned (the second user is created without storing the returned entity), but it’s referenced in the new assertion (user2.getId()). This will not compile. Assign the result of createEntity(request2) to a User user2 variable and use that id in the assertion.
This PR fixes issue #27190 for the WeMakeDevs hackathon. UserDAO.listCount, listBefore, and listAfter were missing the BindMap params argument needed by ListFilter for domainEntityType filtering.
Summary by Gitar
TableRepositoryto remove staleAUTOMATEDtags that are absent from incoming schema definitions.This will update automatically on new commits.