Commit 77f9726
authored
* Fix #27107: soft-deleted users still appear in experts/reviewers across all entities
Two bugs caused soft-deleted users to leak into experts/reviewers/owners/followers:
1. EntityRepository.resolveRelationshipEntityReferencesByType hardcoded Include.ALL
for the consolidated bulk list path, affecting ALL entities (DataProduct, Domain,
GlossaryTerm, Glossary, Table, Schema, Service, Container, etc.). Fixed by
switching to getEntityReferencesByIdsRespectingInclude(..., NON_DELETED).
2. EntityResource.getInternal / getByNameInternal built RelationIncludes(null, ...)
which defaulted to Include.ALL, causing single-entity GET on DataProduct, Domain,
EventSubscription, Query to return deleted users in relation fields. Fixed by
defaulting null include to Include.NON_DELETED for all resource endpoints.
Also cleaned up dead-code in DataProductRepository.batchFetchExperts and
DomainRepository.batchFetchExperts which used getEntityReferenceById that
silently overrides NON_DELETED to ALL.
Adds regression tests in DataProductResourceIT, DomainResourceIT,
GlossaryTermResourceIT covering both single-GET and list-endpoint paths.
* Thread Include through bulk relationship resolution instead of hardcoding NON_DELETED
- Add Include parameter to resolveRelationshipEntityReferencesByType, fetchAndSetRelationshipFieldsInBulk, and fetchAndSetFields (new 3-arg overload)
- Add setFieldsInBulk(fields, entities, Include) overload in base class; existing 2-arg delegates to NON_DELETED
- The Include is no longer hardcoded — callers with request context (e.g. include=all) can pass it through via the 3-arg setFieldsInBulk or fetchAndSetFields
Fixes: #27107
* Fix inherited experts/owners including soft-deleted users from parent Domain
DataProductRepository.setInheritedFields and DomainRepository.setInheritedFields
both fetched parent Domain entities with Include.ALL, causing the domain's
experts and owners fields to be resolved with ALL include — returning
soft-deleted users. Changed to NON_DELETED so inherited fields only carry
active users.
Fixes: #27107
* Fix soft-deleted users leaking into owners/experts/reviewers across all entities
Five additional bug sites using Include.ALL when resolving user relationships:
- EntityRepository.batchFetchOwners: getEntityReferencesByIds → getEntityReferencesByIdsRespectingInclude(NON_DELETED)
- EntityRepository.batchFetchReviewers: same fix
- EntityRepository.batchFetchExperts (base class): same fix
- TagRepository.setInheritedFields: fetch Classification with NON_DELETED (was ALL), preventing soft-deleted owners/reviewers from being inherited by Tags
- WorksheetRepository.setInheritedFields: fetch Spreadsheet with NON_DELETED (was ALL), preventing soft-deleted owners from being inherited by Worksheets
Fixes: #27107
* Fix test user email validation in soft-delete integration tests
ns.prefix() generates ~110-char strings; after stripping underscores the
local part exceeds RFC 5321's 64-char limit and the server's maxLength:127
constraint. Switch to ns.shortPrefix() (~27 chars total) which is already
alphanumeric+underscore and requires no sanitisation.
* Fix GlossaryTerm list test to filter by glossary ID
Without a glossary filter the list call returns the first 100 terms
across all glossaries; in a populated CI env the test term lands
outside that window and the search fails with 'GlossaryTerm not found
in list'. Add .addFilter("glossary", glossary.getId()) so the query
is scoped to only our test glossary.
* Fix fromId/toId swap in EntityRepository.batchFetchExperts
findToBatch(entityIds, EXPERT, USER) returns rows where fromId=entity
and toId=user (matching the write path in bulkInsertToRelationship).
The original code collected fromId as expert user IDs and toId as
entity IDs — both backwards. Method is preempted by the bulk path in
practice, but the logic should be correct.
* Add merge function to Collectors.toMap in batchFetchExperts
Prevents potential IllegalStateException if getEntityReferencesByIdsRespectingInclude
returns duplicate references for the same ID due to data inconsistency.
Consistent with the (a, b) -> a merge function used elsewhere.
* Handle soft-deleted parent in setInheritedFields for DataProduct and Domain
Entity.getEntity(..., NON_DELETED) throws EntityNotFoundException when
the parent domain/domain is soft-deleted while the relationship still
exists. Catch and skip inheritance for that parent rather than
propagating the exception and breaking the GET/list response.
* Remove dead setFieldsInBulk(Include) overload and collapse Include from private chain
The 3-arg setFieldsInBulk(Fields, List<T>, Include) overload had no callers
passing a non-default Include — the intent of this PR is to always filter
soft-deleted users from relationship fields, not to respect the caller's
include. Remove the overload and collapse the Include parameter out of
fetchAndSetFields and fetchAndSetRelationshipFieldsInBulk/
resolveRelationshipEntityReferencesByType, hardcoding NON_DELETED where
it matters.
* Paginate domain list to find test domain regardless of total count
* Revert WorksheetRepository Include change — out of scope for #27107
* Address PR review: add limit to DataProduct list test; align TagRepository bulk path to NON_DELETED
* Revert TagRepository Include changes — out of scope for #27107
* Remove unnecessary changes and compilation error
* Thread Include parameter through bulk relationship resolution path
setFieldsInBulk, fetchAndSetFields, fetchAndSetRelationshipFieldsInBulk, and
resolveRelationshipEntityReferencesByType now accept an Include parameter so
that include=all list queries correctly surface soft-deleted relationship
targets (owners, experts, reviewers) — matching the single-entity GET path
behaviour via RelationIncludes.
All existing 2-param overrides delegate to the new 3-param overloads with
NON_DELETED, so all subclass repositories and the update hydration path are
unchanged. All list/get callers with access to an Include value (listAfter,
listBefore, listAll, listAfterKeyset, listWithOffset, get, getByNames) now
forward that value through the chain.
* Fix subclass bypass: use ThreadLocal to carry Include through virtual setFieldsInBulk dispatch
The previous approach made callers invoke the 3-param setFieldsInBulk directly,
which bypassed all subclass overrides of the 2-param version (GlossaryTermRepository,
DatabaseRepository, TableRepository, etc. all override 2-param with essential
hydration logic like populateParentAndGlossaryReferencesInBulk). This caused a
regression where entities from those repos would be missing service references,
parent references, and other custom bulk-hydrated fields.
Fix: store the include in a static ThreadLocal before delegating to the virtual
2-param setFieldsInBulk (so all subclass overrides run), then the 2-param
fetchAndSetFields reads bulkInclude.get() so it reaches
fetchAndSetRelationshipFieldsInBulk with the correct value. The 3-param overload
is now final to prevent further accidental overrides.
* Fix null Include in setFieldsInBulk — default to NON_DELETED
Callers like DomainResource/DataProductResource/PersonaResource build
new ListFilter(null), so filter.getInclude() returns null. That null was
stored in the bulkInclude ThreadLocal and propagated into
getEntityReferencesByIdsRespectingInclude, bypassing the soft-delete
filter for USER/TEAM types and re-introducing soft-deleted users in
owners/experts/reviewers/followers on those list endpoints.
Resolve null to NON_DELETED before setting the ThreadLocal.
Add unit test covering null → NON_DELETED defaulting.
* Fix #27107: fix inverted ternary root cause; harden bulk-list NON_DELETED
Root cause: Entity.java had an inverted ternary in getEntityReferenceById and
getEntityReferencesByIds:
// WRONG — forces ALL when type supports soft delete
include = repository.supportsSoftDelete ? Include.ALL : include;
// CORRECT — respects caller's include; falls back to ALL only when the
// type has no deleted column and filtering isn't possible
include = repository.supportsSoftDelete ? include : Include.ALL;
PR #25284 worked around this by introducing getEntityReferenceByIdRespectingInclude /
getEntityReferencesByIdsRespectingInclude with the correct ternary, but the original
broken methods were never fixed — a permanent trap. This commit fixes the root and
deletes the RespectingInclude pair (now identical after the flip).
For the bulk-list path (GET /entities?fields=...) nested reference hydration is
hardcoded to NON_DELETED unconditionally in resolveRelationshipEntityReferencesByType
and all five batchFetch* methods (owners, followers, reviewers, experts, votes).
The ?include= query param controls which top-level entities appear in the list;
it does not change the semantics of nested relationship pointers, which should
always resolve to live entities.
For single-entity GET: EntityResource.getInternal/getByNameInternal had a latent
null-include bug for resources (Domain, DataProduct, EventSubscription, Query) that
don't declare @QueryParam("include"). Those null values defaulted to ALL inside
RelationIncludes, leaking soft-deleted nested refs. Fixed by defaulting null →
NON_DELETED before constructing RelationIncludes.
Remove ThreadLocal bulkInclude and the include-threading approach added earlier in
this branch — superseded by the root-cause fix and the NON_DELETED semantic above.
Delete EntityRepositoryIncludeThreadingTest (tested the rejected approach).
Tests: 7 integration tests covering single-GET and list endpoints for Domain,
DataProduct, and GlossaryTerm; verify soft-deleted experts/owners/reviewers are
absent from both paths, including with ?include=all on the list endpoint.
* Fix Copilot review: null-guard in batchFetchFollowers, remove dead 3-arg setFieldsInBulk, add follower/voter IT tests
- batchFetchFollowers: add null-check before adding followerRef to list.
When a follower is soft-deleted, getEntityReferencesByIds(NON_DELETED)
omits their ID from the result map, so followerRefs.get(followerId)
returns null. All other batchFetch* methods already guard this;
batchFetchFollowers was the only one missing the check.
- Remove the dead 3-arg setFieldsInBulk(Fields, List<T>, Include) overload
that accepted include but silently discarded it, delegating to the 2-arg
form. Revert all 6 call sites back to the 2-arg form. The overload was
misleading — callers appeared to plumb include through but bulk hydration
was never affected; NON_DELETED is hardcoded in resolveRelationship-
EntityReferencesByType.
- Add integration tests for followers and votes:
softDeletedFollower_notReturnedInListEndpoint and
softDeletedVoter_notReturnedInListEndpoint in DomainResourceIT.
Both pass (5/5 DomainResourceIT soft-delete tests green).
* fix: bump list limit to 1000000 in follower/voter IT tests to handle large CI datasets
* fix: remove unused Include param from listInternal and serializeJsons
1 parent df9e5e0 commit 77f9726
9 files changed
Lines changed: 416 additions & 74 deletions
File tree
- openmetadata-integration-tests/src/test/java/org/openmetadata/it/tests
- openmetadata-service/src/main/java/org/openmetadata/service
- jdbi3
- resources
Lines changed: 120 additions & 0 deletions
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
33 | 33 | | |
34 | 34 | | |
35 | 35 | | |
| 36 | + | |
36 | 37 | | |
37 | 38 | | |
38 | 39 | | |
| |||
41 | 42 | | |
42 | 43 | | |
43 | 44 | | |
| 45 | + | |
44 | 46 | | |
45 | 47 | | |
46 | 48 | | |
| |||
2884 | 2886 | | |
2885 | 2887 | | |
2886 | 2888 | | |
| 2889 | + | |
| 2890 | + | |
| 2891 | + | |
| 2892 | + | |
| 2893 | + | |
| 2894 | + | |
| 2895 | + | |
| 2896 | + | |
| 2897 | + | |
| 2898 | + | |
| 2899 | + | |
| 2900 | + | |
| 2901 | + | |
| 2902 | + | |
| 2903 | + | |
| 2904 | + | |
| 2905 | + | |
| 2906 | + | |
| 2907 | + | |
| 2908 | + | |
| 2909 | + | |
| 2910 | + | |
| 2911 | + | |
| 2912 | + | |
| 2913 | + | |
| 2914 | + | |
| 2915 | + | |
| 2916 | + | |
| 2917 | + | |
| 2918 | + | |
| 2919 | + | |
| 2920 | + | |
| 2921 | + | |
| 2922 | + | |
| 2923 | + | |
| 2924 | + | |
| 2925 | + | |
| 2926 | + | |
| 2927 | + | |
| 2928 | + | |
| 2929 | + | |
| 2930 | + | |
| 2931 | + | |
| 2932 | + | |
| 2933 | + | |
| 2934 | + | |
| 2935 | + | |
| 2936 | + | |
| 2937 | + | |
| 2938 | + | |
| 2939 | + | |
| 2940 | + | |
| 2941 | + | |
| 2942 | + | |
| 2943 | + | |
| 2944 | + | |
| 2945 | + | |
| 2946 | + | |
| 2947 | + | |
| 2948 | + | |
| 2949 | + | |
| 2950 | + | |
| 2951 | + | |
| 2952 | + | |
| 2953 | + | |
| 2954 | + | |
| 2955 | + | |
| 2956 | + | |
| 2957 | + | |
| 2958 | + | |
| 2959 | + | |
| 2960 | + | |
| 2961 | + | |
| 2962 | + | |
| 2963 | + | |
| 2964 | + | |
| 2965 | + | |
| 2966 | + | |
| 2967 | + | |
| 2968 | + | |
| 2969 | + | |
| 2970 | + | |
| 2971 | + | |
| 2972 | + | |
| 2973 | + | |
| 2974 | + | |
| 2975 | + | |
| 2976 | + | |
| 2977 | + | |
| 2978 | + | |
| 2979 | + | |
| 2980 | + | |
| 2981 | + | |
| 2982 | + | |
| 2983 | + | |
| 2984 | + | |
| 2985 | + | |
| 2986 | + | |
| 2987 | + | |
| 2988 | + | |
| 2989 | + | |
| 2990 | + | |
| 2991 | + | |
| 2992 | + | |
| 2993 | + | |
| 2994 | + | |
| 2995 | + | |
| 2996 | + | |
| 2997 | + | |
| 2998 | + | |
| 2999 | + | |
| 3000 | + | |
| 3001 | + | |
| 3002 | + | |
| 3003 | + | |
| 3004 | + | |
| 3005 | + | |
| 3006 | + | |
2887 | 3007 | | |
Lines changed: 206 additions & 0 deletions
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
29 | 29 | | |
30 | 30 | | |
31 | 31 | | |
| 32 | + | |
32 | 33 | | |
33 | 34 | | |
| 35 | + | |
34 | 36 | | |
| 37 | + | |
| 38 | + | |
35 | 39 | | |
36 | 40 | | |
| 41 | + | |
37 | 42 | | |
38 | 43 | | |
39 | 44 | | |
| 45 | + | |
40 | 46 | | |
41 | 47 | | |
42 | 48 | | |
| |||
1164 | 1170 | | |
1165 | 1171 | | |
1166 | 1172 | | |
| 1173 | + | |
| 1174 | + | |
| 1175 | + | |
| 1176 | + | |
| 1177 | + | |
| 1178 | + | |
| 1179 | + | |
| 1180 | + | |
| 1181 | + | |
| 1182 | + | |
| 1183 | + | |
| 1184 | + | |
| 1185 | + | |
| 1186 | + | |
| 1187 | + | |
| 1188 | + | |
| 1189 | + | |
| 1190 | + | |
| 1191 | + | |
| 1192 | + | |
| 1193 | + | |
| 1194 | + | |
| 1195 | + | |
| 1196 | + | |
| 1197 | + | |
| 1198 | + | |
| 1199 | + | |
| 1200 | + | |
| 1201 | + | |
| 1202 | + | |
| 1203 | + | |
| 1204 | + | |
| 1205 | + | |
| 1206 | + | |
| 1207 | + | |
| 1208 | + | |
| 1209 | + | |
| 1210 | + | |
| 1211 | + | |
| 1212 | + | |
| 1213 | + | |
| 1214 | + | |
| 1215 | + | |
| 1216 | + | |
| 1217 | + | |
| 1218 | + | |
| 1219 | + | |
| 1220 | + | |
| 1221 | + | |
| 1222 | + | |
| 1223 | + | |
| 1224 | + | |
| 1225 | + | |
| 1226 | + | |
| 1227 | + | |
| 1228 | + | |
| 1229 | + | |
| 1230 | + | |
| 1231 | + | |
| 1232 | + | |
| 1233 | + | |
| 1234 | + | |
| 1235 | + | |
| 1236 | + | |
| 1237 | + | |
| 1238 | + | |
| 1239 | + | |
| 1240 | + | |
| 1241 | + | |
| 1242 | + | |
| 1243 | + | |
| 1244 | + | |
| 1245 | + | |
| 1246 | + | |
| 1247 | + | |
| 1248 | + | |
| 1249 | + | |
| 1250 | + | |
| 1251 | + | |
| 1252 | + | |
| 1253 | + | |
| 1254 | + | |
| 1255 | + | |
| 1256 | + | |
| 1257 | + | |
| 1258 | + | |
| 1259 | + | |
| 1260 | + | |
| 1261 | + | |
| 1262 | + | |
| 1263 | + | |
| 1264 | + | |
| 1265 | + | |
| 1266 | + | |
| 1267 | + | |
| 1268 | + | |
| 1269 | + | |
| 1270 | + | |
| 1271 | + | |
| 1272 | + | |
| 1273 | + | |
| 1274 | + | |
| 1275 | + | |
| 1276 | + | |
| 1277 | + | |
| 1278 | + | |
| 1279 | + | |
| 1280 | + | |
| 1281 | + | |
| 1282 | + | |
| 1283 | + | |
| 1284 | + | |
| 1285 | + | |
| 1286 | + | |
| 1287 | + | |
| 1288 | + | |
| 1289 | + | |
| 1290 | + | |
| 1291 | + | |
| 1292 | + | |
| 1293 | + | |
| 1294 | + | |
| 1295 | + | |
| 1296 | + | |
| 1297 | + | |
| 1298 | + | |
| 1299 | + | |
| 1300 | + | |
| 1301 | + | |
| 1302 | + | |
| 1303 | + | |
| 1304 | + | |
| 1305 | + | |
| 1306 | + | |
| 1307 | + | |
| 1308 | + | |
| 1309 | + | |
| 1310 | + | |
| 1311 | + | |
| 1312 | + | |
| 1313 | + | |
| 1314 | + | |
| 1315 | + | |
| 1316 | + | |
| 1317 | + | |
| 1318 | + | |
| 1319 | + | |
| 1320 | + | |
| 1321 | + | |
| 1322 | + | |
| 1323 | + | |
| 1324 | + | |
| 1325 | + | |
| 1326 | + | |
| 1327 | + | |
| 1328 | + | |
| 1329 | + | |
| 1330 | + | |
| 1331 | + | |
| 1332 | + | |
| 1333 | + | |
| 1334 | + | |
| 1335 | + | |
| 1336 | + | |
| 1337 | + | |
| 1338 | + | |
| 1339 | + | |
| 1340 | + | |
| 1341 | + | |
| 1342 | + | |
| 1343 | + | |
| 1344 | + | |
| 1345 | + | |
| 1346 | + | |
| 1347 | + | |
| 1348 | + | |
| 1349 | + | |
| 1350 | + | |
| 1351 | + | |
| 1352 | + | |
| 1353 | + | |
| 1354 | + | |
| 1355 | + | |
| 1356 | + | |
| 1357 | + | |
| 1358 | + | |
| 1359 | + | |
| 1360 | + | |
| 1361 | + | |
| 1362 | + | |
| 1363 | + | |
| 1364 | + | |
| 1365 | + | |
| 1366 | + | |
| 1367 | + | |
| 1368 | + | |
| 1369 | + | |
| 1370 | + | |
| 1371 | + | |
| 1372 | + | |
1167 | 1373 | | |
0 commit comments