From 4fedb5fd9ba1e06e4b21ebab0477b8cedc910149 Mon Sep 17 00:00:00 2001 From: Alberto Miorin <32617+amiorin@users.noreply.github.com> Date: Fri, 14 Jan 2022 21:19:19 +0100 Subject: [PATCH] Fix #2199: Follow-up of PR: Support for getting deleted entities in GET and LIST operations (#2206) * Improve tests for deleted entities, fix User->Teams->OwnsEntities * Improve doc for recursive deletion and improve UsageRepository * Remove listAfter and listBerfore without Include. Add Include.NON_DELETED to the resources without deletion like Bots, Metrics, and Reports. Improve UserRepository. * Add method to add all relationships for the get deleted entity test. Adapt the test for the entity User. * Add soft deletion to Webhook. Implement Include for Webhook. * Improve existing test to include a check for owner. * Fix populateService of Table * Fix Team setFields. --- .../catalog/CatalogHealthCheck.java | 3 +- .../java/org/openmetadata/catalog/Entity.java | 11 ++++ .../catalog/jdbi3/CollectionDAO.java | 35 +++++++++---- .../catalog/jdbi3/EntityRepository.java | 16 +----- .../catalog/jdbi3/FeedRepository.java | 7 ++- .../catalog/jdbi3/TableRepository.java | 6 ++- .../catalog/jdbi3/TeamRepository.java | 16 +++--- .../catalog/jdbi3/UsageRepository.java | 13 +++-- .../catalog/jdbi3/UserRepository.java | 12 +++-- .../catalog/resources/bots/BotsResource.java | 5 +- .../resources/events/WebhookResource.java | 37 ++++++++++---- .../catalog/resources/feeds/FeedResource.java | 3 +- .../resources/metrics/MetricsResource.java | 5 +- .../resources/reports/ReportResource.java | 3 +- .../catalog/resources/teams/UserResource.java | 33 +++++++++--- .../openmetadata/catalog/util/EntityUtil.java | 12 +++-- .../catalog/resources/EntityResourceTest.java | 51 +++++++++---------- .../dashboards/DashboardResourceTest.java | 1 + .../resources/events/WebhookResourceTest.java | 8 --- .../resources/teams/TeamResourceTest.java | 5 ++ .../resources/teams/UserResourceTest.java | 19 ++++--- 21 files changed, 184 insertions(+), 117 deletions(-) diff --git a/catalog-rest-service/src/main/java/org/openmetadata/catalog/CatalogHealthCheck.java b/catalog-rest-service/src/main/java/org/openmetadata/catalog/CatalogHealthCheck.java index f52ff94166a..5d863db21fe 100644 --- a/catalog-rest-service/src/main/java/org/openmetadata/catalog/CatalogHealthCheck.java +++ b/catalog-rest-service/src/main/java/org/openmetadata/catalog/CatalogHealthCheck.java @@ -21,6 +21,7 @@ import java.io.IOException; import org.jdbi.v3.core.Jdbi; import org.openmetadata.catalog.jdbi3.CollectionDAO; import org.openmetadata.catalog.jdbi3.UserRepository; +import org.openmetadata.catalog.type.Include; import org.openmetadata.catalog.util.EntityUtil; public class CatalogHealthCheck extends HealthCheck { @@ -36,7 +37,7 @@ public class CatalogHealthCheck extends HealthCheck { @Override protected Result check() throws Exception { try { - userRepository.listAfter(null, fields, null, 1, null); + userRepository.listAfter(null, fields, null, 1, null, Include.NON_DELETED); return Result.healthy(); } catch (IOException e) { LOG.error("Health check error {}", e.getMessage()); diff --git a/catalog-rest-service/src/main/java/org/openmetadata/catalog/Entity.java b/catalog-rest-service/src/main/java/org/openmetadata/catalog/Entity.java index e52ec7a2c3b..52c9056981b 100644 --- a/catalog-rest-service/src/main/java/org/openmetadata/catalog/Entity.java +++ b/catalog-rest-service/src/main/java/org/openmetadata/catalog/Entity.java @@ -147,6 +147,17 @@ public final class Entity { return entityRepository.getEntityInterface(entity); } + public static EntityInterface getEntityInterface(EntityReference entityReference) + throws IOException, ParseException { + if (entityReference == null) { + return null; + } + String entityName = entityReference.getType(); + EntityRepository entityRepository = getEntityRepository(entityName); + T entity = entityRepository.get(null, entityReference.getId().toString(), EntityUtil.Fields.EMPTY_FIELDS); + return entityRepository.getEntityInterface(entity); + } + /** * Retrieve the entity using id from given entity reference and fields * diff --git a/catalog-rest-service/src/main/java/org/openmetadata/catalog/jdbi3/CollectionDAO.java b/catalog-rest-service/src/main/java/org/openmetadata/catalog/jdbi3/CollectionDAO.java index 237ebec423c..03dc249ad75 100644 --- a/catalog-rest-service/src/main/java/org/openmetadata/catalog/jdbi3/CollectionDAO.java +++ b/catalog-rest-service/src/main/java/org/openmetadata/catalog/jdbi3/CollectionDAO.java @@ -13,6 +13,8 @@ package org.openmetadata.catalog.jdbi3; +import static org.openmetadata.catalog.util.EntityUtil.toBoolean; + import java.sql.ResultSet; import java.sql.SQLException; import java.util.Arrays; @@ -1133,17 +1135,19 @@ public interface CollectionDAO { String findByEmail(@Bind("email") String email); default int listCount(String team, Include include) { - return listCount(getTableName(), getNameColumn(), team, Relationship.HAS.ordinal()); + return listCount(getTableName(), getNameColumn(), team, Relationship.HAS.ordinal(), toBoolean(include)); } @Override default List listBefore(String team, int limit, String before, Include include) { - return listBefore(getTableName(), getNameColumn(), team, limit, before, Relationship.HAS.ordinal()); + return listBefore( + getTableName(), getNameColumn(), team, limit, before, Relationship.HAS.ordinal(), toBoolean(include)); } @Override default List listAfter(String team, int limit, String after, Include include) { - return listAfter(getTableName(), getNameColumn(), team, limit, after, Relationship.HAS.ordinal()); + return listAfter( + getTableName(), getNameColumn(), team, limit, after, Relationship.HAS.ordinal(), toBoolean(include)); } @SqlQuery( @@ -1152,13 +1156,16 @@ public interface CollectionDAO { + "FROM user_entity ue " + "LEFT JOIN entity_relationship er on ue.id = er.toId " + "LEFT JOIN team_entity te on te.id = er.fromId and er.relation = :relation " - + "WHERE (ue.deleted = false AND (te.name = :team OR :team IS NULL)) " + + "WHERE (te.name = :team OR :team IS NULL) " + + "AND (ue.deleted = :deleted OR :deleted IS NULL) " + + "AND (er.deleted = :deleted OR :deleted IS NULL OR (:team IS NULL AND er.deleted IS NULL)) " + "GROUP BY ue.id) subquery") int listCount( @Define("table") String table, @Define("nameColumn") String nameColumn, @Bind("team") String team, - @Bind("relation") int relation); + @Bind("relation") int relation, + @Bind("deleted") Boolean deleted); @SqlQuery( "SELECT json FROM (" @@ -1166,8 +1173,10 @@ public interface CollectionDAO { + "FROM user_entity ue " + "LEFT JOIN entity_relationship er on ue.id = er.toId " + "LEFT JOIN team_entity te on te.id = er.fromId and er.relation = :relation " - + "WHERE (ue.deleted = false AND (te.name = :team OR :team IS NULL)) AND " - + "ue. < :before " + + "WHERE (te.name = :team OR :team IS NULL) " + + "AND (ue.deleted = :deleted OR :deleted IS NULL) " + + "AND (er.deleted = :deleted OR :deleted IS NULL OR (:team IS NULL AND er.deleted IS NULL)) " + + "AND ue. < :before " + "GROUP BY ue., ue.json " + "ORDER BY ue. DESC " + "LIMIT :limit" @@ -1178,15 +1187,18 @@ public interface CollectionDAO { @Bind("team") String team, @Bind("limit") int limit, @Bind("before") String before, - @Bind("relation") int relation); + @Bind("relation") int relation, + @Bind("deleted") Boolean deleted); @SqlQuery( "SELECT ue.json " + "FROM user_entity ue " + "LEFT JOIN entity_relationship er on ue.id = er.toId " + "LEFT JOIN team_entity te on te.id = er.fromId and er.relation = :relation " - + "WHERE (ue.deleted = false AND (te.name = :team OR :team IS NULL)) AND " - + "ue. > :after " + + "WHERE (te.name = :team OR :team IS NULL) " + + "AND (ue.deleted = :deleted OR :deleted IS NULL) " + + "AND (er.deleted = :deleted OR :deleted IS NULL OR (:team IS NULL AND er.deleted IS NULL)) " + + "AND ue. > :after " + "GROUP BY ue.json " + "ORDER BY ue. " + "LIMIT :limit") @@ -1196,7 +1208,8 @@ public interface CollectionDAO { @Bind("team") String team, @Bind("limit") int limit, @Bind("after") String after, - @Bind("relation") int relation); + @Bind("relation") int relation, + @Bind("deleted") Boolean deleted); } interface ChangeEventDAO { diff --git a/catalog-rest-service/src/main/java/org/openmetadata/catalog/jdbi3/EntityRepository.java b/catalog-rest-service/src/main/java/org/openmetadata/catalog/jdbi3/EntityRepository.java index 04314964d79..38498c0277f 100644 --- a/catalog-rest-service/src/main/java/org/openmetadata/catalog/jdbi3/EntityRepository.java +++ b/catalog-rest-service/src/main/java/org/openmetadata/catalog/jdbi3/EntityRepository.java @@ -220,12 +220,6 @@ public abstract class EntityRepository { return withHref(uriInfo, setFields(dao.findEntityByName(fqn, include), fields)); } - @Transaction - public final ResultList listAfter(UriInfo uriInfo, Fields fields, String fqnPrefix, int limitParam, String after) - throws GeneralSecurityException, IOException, ParseException { - return listAfter(uriInfo, fields, fqnPrefix, limitParam, after, Include.NON_DELETED); - } - @Transaction public final ResultList listAfter( UriInfo uriInfo, Fields fields, String fqnPrefix, int limitParam, String after, Include include) @@ -251,12 +245,6 @@ public abstract class EntityRepository { return getResultList(entities, beforeCursor, afterCursor, total); } - @Transaction - public final ResultList listBefore(UriInfo uriInfo, Fields fields, String fqnPrefix, int limitParam, String before) - throws IOException, GeneralSecurityException, ParseException { - return listBefore(uriInfo, fields, fqnPrefix, limitParam, before, Include.NON_DELETED); - } - @Transaction public final ResultList listBefore( UriInfo uriInfo, Fields fields, String fqnPrefix, int limitParam, String before, Include include) @@ -406,7 +394,7 @@ public abstract class EntityRepository { @Transaction public final void delete(UUID id, boolean recursive) throws IOException { - // If an entity being deleted contains other children entities, it can't be deleted + // If an entity being deleted contains other **non-deleted** children entities, it can't be deleted List contains = daoCollection .relationshipDAO() @@ -496,7 +484,7 @@ public abstract class EntityRepository { EntityInterface entityInterface = getEntityInterface(entity); return supportsOwner && entity != null ? EntityUtil.populateOwner( - entityInterface.getId(), + entityInterface, entityName, daoCollection.relationshipDAO(), daoCollection.userDAO(), diff --git a/catalog-rest-service/src/main/java/org/openmetadata/catalog/jdbi3/FeedRepository.java b/catalog-rest-service/src/main/java/org/openmetadata/catalog/jdbi3/FeedRepository.java index 4af9b93c21c..ec4529a38fe 100644 --- a/catalog-rest-service/src/main/java/org/openmetadata/catalog/jdbi3/FeedRepository.java +++ b/catalog-rest-service/src/main/java/org/openmetadata/catalog/jdbi3/FeedRepository.java @@ -16,6 +16,7 @@ package org.openmetadata.catalog.jdbi3; import static org.openmetadata.catalog.util.EntityUtil.toBoolean; import java.io.IOException; +import java.text.ParseException; import java.util.ArrayList; import java.util.HashSet; import java.util.List; @@ -31,6 +32,7 @@ import org.openmetadata.catalog.resources.feeds.MessageParser.EntityLink.LinkTyp import org.openmetadata.catalog.type.EntityReference; import org.openmetadata.catalog.type.Include; import org.openmetadata.catalog.type.Post; +import org.openmetadata.catalog.util.EntityInterface; import org.openmetadata.catalog.util.EntityUtil; import org.openmetadata.catalog.util.JsonUtils; @@ -42,7 +44,7 @@ public class FeedRepository { } @Transaction - public Thread create(Thread thread) throws IOException { + public Thread create(Thread thread) throws IOException, ParseException { // Validate user creating thread UUID fromUser = thread.getPosts().get(0).getFrom(); dao.userDAO().findEntityById(fromUser); @@ -50,11 +52,12 @@ public class FeedRepository { // Validate about data entity is valid EntityLink about = EntityLink.parse(thread.getAbout()); EntityReference aboutRef = EntityUtil.validateEntityLink(about); + EntityInterface aboutEntityInterface = Entity.getEntityInterface(aboutRef); // Get owner for the addressed to Entity EntityReference owner = EntityUtil.populateOwner( - aboutRef.getId(), aboutRef.getType(), dao.relationshipDAO(), dao.userDAO(), dao.teamDAO()); + aboutEntityInterface, aboutRef.getType(), dao.relationshipDAO(), dao.userDAO(), dao.teamDAO()); // Insert a new thread dao.feedDAO().insert(JsonUtils.pojoToJson(thread)); diff --git a/catalog-rest-service/src/main/java/org/openmetadata/catalog/jdbi3/TableRepository.java b/catalog-rest-service/src/main/java/org/openmetadata/catalog/jdbi3/TableRepository.java index 475588ccc66..0bfc1887f52 100644 --- a/catalog-rest-service/src/main/java/org/openmetadata/catalog/jdbi3/TableRepository.java +++ b/catalog-rest-service/src/main/java/org/openmetadata/catalog/jdbi3/TableRepository.java @@ -351,16 +351,18 @@ public class TableRepository extends EntityRepository { } private void populateService(Table table) throws IOException { + Database database = daoCollection.databaseDAO().findEntityById(table.getDatabase().getId(), Include.ALL); + Include include = database.getDeleted() ? Include.DELETED : Include.NON_DELETED; // Find database service from the database that table is contained in String serviceId = daoCollection .relationshipDAO() .findFrom( - table.getDatabase().getId().toString(), + database.getId().toString(), Entity.DATABASE, Relationship.CONTAINS.ordinal(), Entity.DATABASE_SERVICE, - toBoolean(Include.NON_DELETED)) + toBoolean(include)) .get(0); DatabaseService service = daoCollection.dbServiceDAO().findEntityById(UUID.fromString(serviceId)); table.setService(new DatabaseServiceEntityInterface(service).getEntityReference()); diff --git a/catalog-rest-service/src/main/java/org/openmetadata/catalog/jdbi3/TeamRepository.java b/catalog-rest-service/src/main/java/org/openmetadata/catalog/jdbi3/TeamRepository.java index 675e94b67ee..457da8f879e 100644 --- a/catalog-rest-service/src/main/java/org/openmetadata/catalog/jdbi3/TeamRepository.java +++ b/catalog-rest-service/src/main/java/org/openmetadata/catalog/jdbi3/TeamRepository.java @@ -31,7 +31,6 @@ import org.openmetadata.catalog.exception.CatalogExceptionMessage; import org.openmetadata.catalog.resources.teams.TeamResource; import org.openmetadata.catalog.type.ChangeDescription; import org.openmetadata.catalog.type.EntityReference; -import org.openmetadata.catalog.type.Include; import org.openmetadata.catalog.util.EntityInterface; import org.openmetadata.catalog.util.EntityUtil; import org.openmetadata.catalog.util.EntityUtil.Fields; @@ -79,8 +78,8 @@ public class TeamRepository extends EntityRepository { if (!fields.contains("profile")) { team.setProfile(null); } - team.setUsers(fields.contains("users") ? getUsers(team.getId().toString()) : null); - team.setOwns(fields.contains("owns") ? getOwns(team.getId().toString()) : null); + team.setUsers(fields.contains("users") ? getUsers(team) : null); + team.setOwns(fields.contains("owns") ? getOwns(team) : null); return team; } @@ -128,11 +127,12 @@ public class TeamRepository extends EntityRepository { return new TeamUpdater(original, updated, patchOperation); } - private List getUsers(String id) throws IOException { + private List getUsers(Team team) throws IOException { List userIds = daoCollection .relationshipDAO() - .findTo(id, Entity.TEAM, Relationship.HAS.ordinal(), "user", toBoolean(Include.NON_DELETED)); + .findTo( + team.getId().toString(), Entity.TEAM, Relationship.HAS.ordinal(), "user", toBoolean(toInclude(team))); List users = new ArrayList<>(); for (String userId : userIds) { users.add(daoCollection.userDAO().findEntityReferenceById(UUID.fromString(userId))); @@ -140,10 +140,12 @@ public class TeamRepository extends EntityRepository { return users; } - private List getOwns(String teamId) throws IOException { + private List getOwns(Team team) throws IOException { // Compile entities owned by the team return EntityUtil.populateEntityReferences( - daoCollection.relationshipDAO().findTo(teamId, Entity.TEAM, OWNS.ordinal(), toBoolean(Include.NON_DELETED))); + daoCollection + .relationshipDAO() + .findTo(team.getId().toString(), Entity.TEAM, OWNS.ordinal(), toBoolean(toInclude(team)))); } public static class TeamEntityInterface implements EntityInterface { diff --git a/catalog-rest-service/src/main/java/org/openmetadata/catalog/jdbi3/UsageRepository.java b/catalog-rest-service/src/main/java/org/openmetadata/catalog/jdbi3/UsageRepository.java index c4daee84846..6c335ce326b 100644 --- a/catalog-rest-service/src/main/java/org/openmetadata/catalog/jdbi3/UsageRepository.java +++ b/catalog-rest-service/src/main/java/org/openmetadata/catalog/jdbi3/UsageRepository.java @@ -24,6 +24,7 @@ import org.jdbi.v3.core.mapper.RowMapper; import org.jdbi.v3.core.statement.StatementContext; import org.jdbi.v3.sqlobject.transaction.Transaction; import org.openmetadata.catalog.Entity; +import org.openmetadata.catalog.entity.data.Table; import org.openmetadata.catalog.type.DailyCount; import org.openmetadata.catalog.type.EntityReference; import org.openmetadata.catalog.type.EntityUsage; @@ -74,20 +75,18 @@ public class UsageRepository { dao.usageDAO().computePercentile(entityType, date); } - private void addUsage(String entityType, String entityId, DailyCount usage) { + private void addUsage(String entityType, String entityId, DailyCount usage) throws IOException { // Insert usage record dao.usageDAO().insert(usage.getDate(), entityId, entityType, usage.getCount()); // If table usage was reported, add the usage count to database if (entityType.equalsIgnoreCase(Entity.TABLE)) { + // we accept usage for deleted entities + Table table = dao.tableDAO().findEntityById(UUID.fromString(entityId), Include.ALL); + Include include = table.getDeleted() ? Include.DELETED : Include.NON_DELETED; List databaseIds = dao.relationshipDAO() - .findFrom( - entityId, - entityType, - Relationship.CONTAINS.ordinal(), - Entity.DATABASE, - toBoolean(Include.NON_DELETED)); + .findFrom(entityId, entityType, Relationship.CONTAINS.ordinal(), Entity.DATABASE, toBoolean(include)); dao.usageDAO().insertOrUpdateCount(usage.getDate(), databaseIds.get(0), Entity.DATABASE, usage.getCount()); } } diff --git a/catalog-rest-service/src/main/java/org/openmetadata/catalog/jdbi3/UserRepository.java b/catalog-rest-service/src/main/java/org/openmetadata/catalog/jdbi3/UserRepository.java index 129774a0188..e4b9984f5ae 100644 --- a/catalog-rest-service/src/main/java/org/openmetadata/catalog/jdbi3/UserRepository.java +++ b/catalog-rest-service/src/main/java/org/openmetadata/catalog/jdbi3/UserRepository.java @@ -16,11 +16,14 @@ package org.openmetadata.catalog.jdbi3; import static org.openmetadata.catalog.jdbi3.Relationship.FOLLOWS; import static org.openmetadata.catalog.jdbi3.Relationship.HAS; import static org.openmetadata.catalog.jdbi3.Relationship.OWNS; +import static org.openmetadata.catalog.type.Include.DELETED; +import static org.openmetadata.catalog.type.Include.NON_DELETED; import static org.openmetadata.catalog.util.EntityUtil.toBoolean; import com.fasterxml.jackson.core.JsonProcessingException; import java.io.IOException; import java.net.URI; +import java.text.ParseException; import java.util.ArrayList; import java.util.Collections; import java.util.List; @@ -94,13 +97,13 @@ public class UserRepository extends EntityRepository { } @Transaction - public User getByEmail(String email, Fields fields) throws IOException { + public User getByEmail(String email, Fields fields) throws IOException, ParseException { User user = EntityUtil.validate(email, daoCollection.userDAO().findByEmail(email), User.class); return setFields(user, fields); } @Override - public User setFields(User user, Fields fields) throws IOException { + public User setFields(User user, Fields fields) throws IOException, ParseException { user.setProfile(fields.contains("profile") ? user.getProfile() : null); user.setTeams(fields.contains("teams") ? getTeams(user) : null); user.setRoles(fields.contains("roles") ? getRoles(user) : null); @@ -112,7 +115,7 @@ public class UserRepository extends EntityRepository { @Override public void restorePatchAttributes(User original, User updated) {} - private List getOwns(User user) throws IOException { + private List getOwns(User user) throws IOException, ParseException { // Compile entities owned by the user List ownedEntities = daoCollection @@ -122,10 +125,11 @@ public class UserRepository extends EntityRepository { // Compile entities owned by the team the user belongs to List teams = user.getTeams() == null ? getTeams(user) : user.getTeams(); for (EntityReference team : teams) { + Include include = Entity.getEntityInterface(team).isDeleted() ? DELETED : NON_DELETED; ownedEntities.addAll( daoCollection .relationshipDAO() - .findTo(team.getId().toString(), Entity.TEAM, OWNS.ordinal(), toBoolean(Include.NON_DELETED))); + .findTo(team.getId().toString(), Entity.TEAM, OWNS.ordinal(), toBoolean(include))); } // Populate details in entity reference return EntityUtil.populateEntityReferences(ownedEntities); diff --git a/catalog-rest-service/src/main/java/org/openmetadata/catalog/resources/bots/BotsResource.java b/catalog-rest-service/src/main/java/org/openmetadata/catalog/resources/bots/BotsResource.java index 3cb66bc9396..246dffaa837 100644 --- a/catalog-rest-service/src/main/java/org/openmetadata/catalog/resources/bots/BotsResource.java +++ b/catalog-rest-service/src/main/java/org/openmetadata/catalog/resources/bots/BotsResource.java @@ -45,6 +45,7 @@ import org.openmetadata.catalog.jdbi3.CollectionDAO; import org.openmetadata.catalog.resources.Collection; import org.openmetadata.catalog.security.Authorizer; import org.openmetadata.catalog.security.SecurityUtil; +import org.openmetadata.catalog.type.Include; import org.openmetadata.catalog.util.EntityUtil.Fields; import org.openmetadata.catalog.util.RestUtil; import org.openmetadata.catalog.util.ResultList; @@ -97,9 +98,9 @@ public class BotsResource { ResultList list; if (before != null) { // Reverse paging - list = dao.listBefore(uriInfo, null, name, limitParam, before); + list = dao.listBefore(uriInfo, null, name, limitParam, before, Include.NON_DELETED); } else { // Forward paging or first page - list = dao.listAfter(uriInfo, null, name, limitParam, after); + list = dao.listAfter(uriInfo, null, name, limitParam, after, Include.NON_DELETED); } return list; } diff --git a/catalog-rest-service/src/main/java/org/openmetadata/catalog/resources/events/WebhookResource.java b/catalog-rest-service/src/main/java/org/openmetadata/catalog/resources/events/WebhookResource.java index f03c57da23e..1849144237c 100644 --- a/catalog-rest-service/src/main/java/org/openmetadata/catalog/resources/events/WebhookResource.java +++ b/catalog-rest-service/src/main/java/org/openmetadata/catalog/resources/events/WebhookResource.java @@ -52,6 +52,7 @@ import org.openmetadata.catalog.security.Authorizer; import org.openmetadata.catalog.security.SecurityUtil; import org.openmetadata.catalog.type.ChangeEvent; import org.openmetadata.catalog.type.EntityHistory; +import org.openmetadata.catalog.type.Include; import org.openmetadata.catalog.type.Webhook; import org.openmetadata.catalog.type.Webhook.Status; import org.openmetadata.catalog.util.EntityUtil.Fields; @@ -111,14 +112,20 @@ public class WebhookResource { String before, @Parameter(description = "Returns list of webhooks after this cursor", schema = @Schema(type = "string")) @QueryParam("after") - String after) + String after, + @Parameter( + description = "Include all, deleted, or non-deleted entities.", + schema = @Schema(implementation = Include.class)) + @QueryParam("include") + @DefaultValue("non-deleted") + Include include) throws IOException, ParseException, GeneralSecurityException { RestUtil.validateCursors(before, after); ResultList webhooks; if (before != null) { // Reverse paging - webhooks = dao.listBefore(uriInfo, Fields.EMPTY_FIELDS, null, limitParam, before); + webhooks = dao.listBefore(uriInfo, Fields.EMPTY_FIELDS, null, limitParam, before, include); } else { // Forward paging or first page - webhooks = dao.listAfter(uriInfo, Fields.EMPTY_FIELDS, null, limitParam, after); + webhooks = dao.listAfter(uriInfo, Fields.EMPTY_FIELDS, null, limitParam, after, include); } webhooks.getData().forEach(t -> dao.withHref(uriInfo, t)); return webhooks; @@ -138,11 +145,17 @@ public class WebhookResource { content = @Content(mediaType = "application/json", schema = @Schema(implementation = ChangeEvent.class))), @ApiResponse(responseCode = "404", description = "Entity for instance {id} is not found") }) - public Webhook getWebhook( + public Webhook get( @Context UriInfo uriInfo, - @Parameter(description = "webhook Id", schema = @Schema(type = "string")) @PathParam("id") String id) + @Parameter(description = "webhook Id", schema = @Schema(type = "string")) @PathParam("id") String id, + @Parameter( + description = "Include all, deleted, or non-deleted entities.", + schema = @Schema(implementation = Include.class)) + @QueryParam("include") + @DefaultValue("non-deleted") + Include include) throws IOException, GeneralSecurityException, ParseException { - return dao.get(uriInfo, id, Fields.EMPTY_FIELDS); + return dao.get(uriInfo, id, Fields.EMPTY_FIELDS, include); } @GET @@ -161,9 +174,15 @@ public class WebhookResource { public Webhook getByName( @Context UriInfo uriInfo, @Context SecurityContext securityContext, - @Parameter(description = "Name of the webhook", schema = @Schema(type = "string")) @PathParam("name") String fqn) + @Parameter(description = "Name of the webhook", schema = @Schema(type = "string")) @PathParam("name") String fqn, + @Parameter( + description = "Include all, deleted, or non-deleted entities.", + schema = @Schema(implementation = Include.class)) + @QueryParam("include") + @DefaultValue("non-deleted") + Include include) throws IOException, ParseException { - return dao.getByName(uriInfo, fqn, Fields.EMPTY_FIELDS); + return dao.getByName(uriInfo, fqn, Fields.EMPTY_FIELDS, include); } @GET @@ -280,7 +299,7 @@ public class WebhookResource { @Context UriInfo uriInfo, @Parameter(description = "webhook Id", schema = @Schema(type = "string")) @PathParam("id") String id) throws IOException, GeneralSecurityException, ParseException, InterruptedException { - dao.delete(id); + dao.delete(UUID.fromString(id), false); dao.deleteWebhookPublisher(UUID.fromString(id)); return Response.ok().build(); } diff --git a/catalog-rest-service/src/main/java/org/openmetadata/catalog/resources/feeds/FeedResource.java b/catalog-rest-service/src/main/java/org/openmetadata/catalog/resources/feeds/FeedResource.java index 82d29ca6178..019e678ae9a 100644 --- a/catalog-rest-service/src/main/java/org/openmetadata/catalog/resources/feeds/FeedResource.java +++ b/catalog-rest-service/src/main/java/org/openmetadata/catalog/resources/feeds/FeedResource.java @@ -20,6 +20,7 @@ import io.swagger.v3.oas.annotations.media.Content; import io.swagger.v3.oas.annotations.media.Schema; import io.swagger.v3.oas.annotations.responses.ApiResponse; import java.io.IOException; +import java.text.ParseException; import java.util.List; import java.util.Objects; import java.util.UUID; @@ -130,7 +131,7 @@ public class FeedResource { content = @Content(mediaType = "application/json", schema = @Schema(implementation = CreateThread.class))), @ApiResponse(responseCode = "400", description = "Bad request") }) - public Response create(@Context UriInfo uriInfo, @Valid CreateThread cr) throws IOException { + public Response create(@Context UriInfo uriInfo, @Valid CreateThread cr) throws IOException, ParseException { Thread thread = new Thread().withId(UUID.randomUUID()).withThreadTs(System.currentTimeMillis()).withAbout(cr.getAbout()); // For now redundantly storing everything in json (that includes fromEntity, addressedTo entity) diff --git a/catalog-rest-service/src/main/java/org/openmetadata/catalog/resources/metrics/MetricsResource.java b/catalog-rest-service/src/main/java/org/openmetadata/catalog/resources/metrics/MetricsResource.java index 7acbe49fd3b..baad8a80848 100644 --- a/catalog-rest-service/src/main/java/org/openmetadata/catalog/resources/metrics/MetricsResource.java +++ b/catalog-rest-service/src/main/java/org/openmetadata/catalog/resources/metrics/MetricsResource.java @@ -48,6 +48,7 @@ import org.openmetadata.catalog.jdbi3.CollectionDAO; import org.openmetadata.catalog.jdbi3.MetricsRepository; import org.openmetadata.catalog.resources.Collection; import org.openmetadata.catalog.security.Authorizer; +import org.openmetadata.catalog.type.Include; import org.openmetadata.catalog.util.EntityUtil.Fields; import org.openmetadata.catalog.util.RestUtil; import org.openmetadata.catalog.util.RestUtil.PutResponse; @@ -106,10 +107,10 @@ public class MetricsResource { Fields fields = new Fields(FIELD_LIST, fieldsParam); if (before != null) { // Reverse paging - return dao.listBefore(uriInfo, fields, null, limitParam, before); + return dao.listBefore(uriInfo, fields, null, limitParam, before, Include.NON_DELETED); } // Forward paging or first page - return dao.listAfter(uriInfo, fields, null, limitParam, after); + return dao.listAfter(uriInfo, fields, null, limitParam, after, Include.NON_DELETED); } @GET diff --git a/catalog-rest-service/src/main/java/org/openmetadata/catalog/resources/reports/ReportResource.java b/catalog-rest-service/src/main/java/org/openmetadata/catalog/resources/reports/ReportResource.java index 8004b701a64..b5a575b124f 100644 --- a/catalog-rest-service/src/main/java/org/openmetadata/catalog/resources/reports/ReportResource.java +++ b/catalog-rest-service/src/main/java/org/openmetadata/catalog/resources/reports/ReportResource.java @@ -45,6 +45,7 @@ import org.openmetadata.catalog.jdbi3.CollectionDAO; import org.openmetadata.catalog.jdbi3.ReportRepository; import org.openmetadata.catalog.resources.Collection; import org.openmetadata.catalog.security.Authorizer; +import org.openmetadata.catalog.type.Include; import org.openmetadata.catalog.util.EntityUtil.Fields; import org.openmetadata.catalog.util.RestUtil.PutResponse; import org.openmetadata.catalog.util.ResultList; @@ -92,7 +93,7 @@ public class ReportResource { String fieldsParam) throws IOException, GeneralSecurityException, ParseException { Fields fields = new Fields(FIELD_LIST, fieldsParam); - return dao.listAfter(uriInfo, fields, null, 10000, null); + return dao.listAfter(uriInfo, fields, null, 10000, null, Include.NON_DELETED); } @GET diff --git a/catalog-rest-service/src/main/java/org/openmetadata/catalog/resources/teams/UserResource.java b/catalog-rest-service/src/main/java/org/openmetadata/catalog/resources/teams/UserResource.java index 2e4e3f3e1d5..7b87d0d35b0 100644 --- a/catalog-rest-service/src/main/java/org/openmetadata/catalog/resources/teams/UserResource.java +++ b/catalog-rest-service/src/main/java/org/openmetadata/catalog/resources/teams/UserResource.java @@ -64,6 +64,7 @@ import org.openmetadata.catalog.resources.Collection; import org.openmetadata.catalog.security.Authorizer; import org.openmetadata.catalog.security.SecurityUtil; import org.openmetadata.catalog.type.EntityHistory; +import org.openmetadata.catalog.type.Include; import org.openmetadata.catalog.util.EntityUtil.Fields; import org.openmetadata.catalog.util.RestUtil; import org.openmetadata.catalog.util.RestUtil.PatchResponse; @@ -146,16 +147,22 @@ public class UserResource { String before, @Parameter(description = "Returns list of users after this cursor", schema = @Schema(type = "string")) @QueryParam("after") - String after) + String after, + @Parameter( + description = "Include all, deleted, or non-deleted entities.", + schema = @Schema(implementation = Include.class)) + @QueryParam("include") + @DefaultValue("non-deleted") + Include include) throws IOException, GeneralSecurityException, ParseException { RestUtil.validateCursors(before, after); Fields fields = new Fields(FIELD_LIST, fieldsParam); ResultList users; if (before != null) { // Reverse paging - users = dao.listBefore(uriInfo, fields, teamParam, limitParam, before); + users = dao.listBefore(uriInfo, fields, teamParam, limitParam, before, include); } else { // Forward paging or first page - users = dao.listAfter(uriInfo, fields, teamParam, limitParam, after); + users = dao.listAfter(uriInfo, fields, teamParam, limitParam, after, include); } Optional.ofNullable(users.getData()).orElse(Collections.emptyList()).forEach(u -> addHref(uriInfo, u)); return users; @@ -203,10 +210,16 @@ public class UserResource { description = "Fields requested in the returned resource", schema = @Schema(type = "string", example = FIELDS)) @QueryParam("fields") - String fieldsParam) + String fieldsParam, + @Parameter( + description = "Include all, deleted, or non-deleted entities.", + schema = @Schema(implementation = Include.class)) + @QueryParam("include") + @DefaultValue("non-deleted") + Include include) throws IOException, ParseException { Fields fields = new Fields(FIELD_LIST, fieldsParam); - User user = dao.get(uriInfo, id, fields); + User user = dao.get(uriInfo, id, fields, include); return addHref(uriInfo, user); } @@ -232,10 +245,16 @@ public class UserResource { description = "Fields requested in the returned resource", schema = @Schema(type = "string", example = FIELDS)) @QueryParam("fields") - String fieldsParam) + String fieldsParam, + @Parameter( + description = "Include all, deleted, or non-deleted entities.", + schema = @Schema(implementation = Include.class)) + @QueryParam("include") + @DefaultValue("non-deleted") + Include include) throws IOException, ParseException { Fields fields = new Fields(FIELD_LIST, fieldsParam); - User user = dao.getByName(uriInfo, name, fields); + User user = dao.getByName(uriInfo, name, fields, include); return addHref(uriInfo, user); } diff --git a/catalog-rest-service/src/main/java/org/openmetadata/catalog/util/EntityUtil.java b/catalog-rest-service/src/main/java/org/openmetadata/catalog/util/EntityUtil.java index 819be44d922..7c79429726d 100644 --- a/catalog-rest-service/src/main/java/org/openmetadata/catalog/util/EntityUtil.java +++ b/catalog-rest-service/src/main/java/org/openmetadata/catalog/util/EntityUtil.java @@ -15,6 +15,7 @@ package org.openmetadata.catalog.util; import static org.openmetadata.catalog.type.Include.ALL; import static org.openmetadata.catalog.type.Include.DELETED; +import static org.openmetadata.catalog.type.Include.NON_DELETED; import java.io.IOException; import java.util.ArrayList; @@ -209,13 +210,18 @@ public final class EntityUtil { // Get owner for a given entity public static EntityReference populateOwner( - UUID id, String entityType, EntityRelationshipDAO entityRelationshipDAO, UserDAO userDAO, TeamDAO teamDAO) + EntityInterface entityInterface, + String entityType, + EntityRelationshipDAO entityRelationshipDAO, + UserDAO userDAO, + TeamDAO teamDAO) throws IOException { + Include include = entityInterface.isDeleted() ? DELETED : NON_DELETED; List ids = entityRelationshipDAO.findFrom( - id.toString(), entityType, Relationship.OWNS.ordinal(), toBoolean(Include.NON_DELETED)); + entityInterface.getId().toString(), entityType, Relationship.OWNS.ordinal(), toBoolean(include)); if (ids.size() > 1) { - LOG.warn("Possible database issues - multiple owners {} found for entity {}", ids, id); + LOG.warn("Possible database issues - multiple owners {} found for entity {}", ids, entityInterface.getId()); } return ids.isEmpty() ? null : populateOwner(userDAO, teamDAO, ids.get(0)); } diff --git a/catalog-rest-service/src/test/java/org/openmetadata/catalog/resources/EntityResourceTest.java b/catalog-rest-service/src/test/java/org/openmetadata/catalog/resources/EntityResourceTest.java index 88ce2d4c181..8d4f70f6c8b 100644 --- a/catalog-rest-service/src/test/java/org/openmetadata/catalog/resources/EntityResourceTest.java +++ b/catalog-rest-service/src/test/java/org/openmetadata/catalog/resources/EntityResourceTest.java @@ -287,6 +287,11 @@ public abstract class EntityResourceTest extends CatalogApplicationTest { public abstract Object createRequest(String name, String description, String displayName, EntityReference owner) throws URISyntaxException; + // Add all possible relationship to check if the entity is missing any of them after deletion + public Object addAllRelationships(TestInfo test, Object create) throws HttpResponseException { + return create; + } + // Get container entity based on create request that has CONTAINS relationship to the entity created with this // request has . For table, it is database. For database, it is databaseService. See Relationship.CONTAINS for // details. @@ -322,8 +327,11 @@ public abstract class EntityResourceTest extends CatalogApplicationTest { Random rand = new Random(); int maxEntities = rand.nextInt(16) + 5; + List createdUUIDs = new ArrayList<>(); for (int i = 0; i < maxEntities; i++) { - createEntity(createRequest(getEntityName(test, i), null, null, null), adminAuthHeaders()); + createdUUIDs.add( + getEntityInterface(createEntity(createRequest(getEntityName(test, i), null, null, null), adminAuthHeaders())) + .getId()); } T entity = createEntity(createRequest(getEntityName(test, -1), null, null, null), adminAuthHeaders()); @@ -409,12 +417,11 @@ public abstract class EntityResourceTest extends CatalogApplicationTest { } } - // before running "deleted" delete all entries + // before running "deleted" delete all created entries otherwise the test doesn't work with just one element. if ("all".equals(include)) { - // delete all entries for (T e : allEntities.getData()) { EntityInterface toBeDeleted = getEntityInterface(e); - if (!toBeDeleted.isDeleted()) { + if (createdUUIDs.contains(toBeDeleted.getId()) && !toBeDeleted.isDeleted()) { deleteEntity(toBeDeleted.getId(), adminAuthHeaders()); } } @@ -480,8 +487,8 @@ public abstract class EntityResourceTest extends CatalogApplicationTest { @Test void get_entityIncludeDeleted_200(TestInfo test) throws HttpResponseException, URISyntaxException { - Object create = createRequest(getEntityName(test), "", "", null); - EntityReference container = getContainer(create); + Object create = + addAllRelationships(test, createRequest(getEntityName(test), "description", "displayName", USER_OWNER1)); // Create first time using POST T entity = createEntity(create, adminAuthHeaders()); EntityInterface entityInterface = getEntityInterface(entity); @@ -497,20 +504,16 @@ public abstract class EntityResourceTest extends CatalogApplicationTest { NOT_FOUND, entityNotFound(entityName, entityInterface.getFullyQualifiedName())); - Map queryParams = - new HashMap<>() { - { - put("include", "deleted"); - } - }; - checkContainer(container, getEntity(entityInterface.getId(), queryParams, null, adminAuthHeaders())); - checkContainer( - container, getEntityByName(entityInterface.getFullyQualifiedName(), queryParams, null, adminAuthHeaders())); - - queryParams.put("include", "all"); - checkContainer(container, getEntity(entityInterface.getId(), queryParams, null, adminAuthHeaders())); - checkContainer( - container, getEntityByName(entityInterface.getFullyQualifiedName(), queryParams, null, adminAuthHeaders())); + Map queryParams = new HashMap<>(); + for (String include : List.of("deleted", "all")) { + queryParams.put("include", include); + validateCreatedEntity( + getEntity(entityInterface.getId(), queryParams, allFields, adminAuthHeaders()), create, adminAuthHeaders()); + validateCreatedEntity( + getEntityByName(entityInterface.getFullyQualifiedName(), queryParams, allFields, adminAuthHeaders()), + create, + adminAuthHeaders()); + } queryParams.put("include", "non-deleted"); assertResponse( @@ -523,14 +526,6 @@ public abstract class EntityResourceTest extends CatalogApplicationTest { entityNotFound(entityName, entityInterface.getFullyQualifiedName())); } - protected void checkContainer(EntityReference container, T entity) { - EntityInterface entityInterface = getEntityInterface(entity); - if (container != null) { - assertNotNull(entityInterface.getContainer(), "Container is null"); - assertEquals(container.getId(), entityInterface.getContainer().getId(), "Containers are different"); - } - } - /////////////////////////////////////////////////////////////////////////////////////////////////////////////////// // Common entity tests for POST operations /////////////////////////////////////////////////////////////////////////////////////////////////////////////////// diff --git a/catalog-rest-service/src/test/java/org/openmetadata/catalog/resources/dashboards/DashboardResourceTest.java b/catalog-rest-service/src/test/java/org/openmetadata/catalog/resources/dashboards/DashboardResourceTest.java index b146b8516da..07e509983ce 100644 --- a/catalog-rest-service/src/test/java/org/openmetadata/catalog/resources/dashboards/DashboardResourceTest.java +++ b/catalog-rest-service/src/test/java/org/openmetadata/catalog/resources/dashboards/DashboardResourceTest.java @@ -294,6 +294,7 @@ public class DashboardResourceTest extends EntityResourceTest { List expectedChartReferences = expectedCharts.stream().map(EntityReference::getId).collect(Collectors.toList()); List actualChartReferences = new ArrayList<>(); + assertNotNull(dashboard.getCharts(), "dashboard should have charts"); dashboard .getCharts() .forEach( diff --git a/catalog-rest-service/src/test/java/org/openmetadata/catalog/resources/events/WebhookResourceTest.java b/catalog-rest-service/src/test/java/org/openmetadata/catalog/resources/events/WebhookResourceTest.java index 4e5a1f39195..0e3e49d7272 100644 --- a/catalog-rest-service/src/test/java/org/openmetadata/catalog/resources/events/WebhookResourceTest.java +++ b/catalog-rest-service/src/test/java/org/openmetadata/catalog/resources/events/WebhookResourceTest.java @@ -64,14 +64,6 @@ public class WebhookResourceTest extends EntityResourceTest { supportsPatch = false; } - // FIXME: This test is added to be able to merge the PR. We will open a new PR, fix the bug, and remove this test - @Test - void get_entityIncludeDeleted_200(TestInfo test) throws HttpResponseException, URISyntaxException {} - - // FIXME: This test is added to be able to merge the PR. We will open a new PR, fix the bug, and remove this test - @Test - void get_entityListWithPagination_200(TestInfo test) throws HttpResponseException, URISyntaxException {} - @Test void post_webhookEnabledStateChange(TestInfo test) throws URISyntaxException, IOException, InterruptedException { // diff --git a/catalog-rest-service/src/test/java/org/openmetadata/catalog/resources/teams/TeamResourceTest.java b/catalog-rest-service/src/test/java/org/openmetadata/catalog/resources/teams/TeamResourceTest.java index 0cc78dfa0b4..cfccbef8c3f 100644 --- a/catalog-rest-service/src/test/java/org/openmetadata/catalog/resources/teams/TeamResourceTest.java +++ b/catalog-rest-service/src/test/java/org/openmetadata/catalog/resources/teams/TeamResourceTest.java @@ -340,6 +340,11 @@ public class TeamResourceTest extends EntityResourceTest { return create(name).withDescription(description).withDisplayName(displayName).withProfile(PROFILE); } + @Override + public Object addAllRelationships(TestInfo test, Object create) throws HttpResponseException { + return ((CreateTeam) create).withUsers(List.of(USER1.getId())); + } + @Override public EntityReference getContainer(Object createRequest) throws URISyntaxException { return null; // No container entity diff --git a/catalog-rest-service/src/test/java/org/openmetadata/catalog/resources/teams/UserResourceTest.java b/catalog-rest-service/src/test/java/org/openmetadata/catalog/resources/teams/UserResourceTest.java index 43fb2ceb765..fa4c3a9f409 100644 --- a/catalog-rest-service/src/test/java/org/openmetadata/catalog/resources/teams/UserResourceTest.java +++ b/catalog-rest-service/src/test/java/org/openmetadata/catalog/resources/teams/UserResourceTest.java @@ -85,14 +85,6 @@ public class UserResourceTest extends EntityResourceTest { super(Entity.USER, User.class, UserList.class, "users", UserResource.FIELDS, false, false, false); } - // FIXME: This test is added to be able to merge the PR. We will open a new PR, fix the bug, and remove this test - @Test - void get_entityIncludeDeleted_200(TestInfo test) throws HttpResponseException, URISyntaxException {} - - // FIXME: This test is added to be able to merge the PR. We will open a new PR, fix the bug, and remove this test - @Test - void get_entityListWithPagination_200(TestInfo test) throws HttpResponseException, URISyntaxException {} - @Test void post_userWithoutEmail_400_badRequest(TestInfo test) { // Create user with mandatory email field null @@ -548,6 +540,17 @@ public class UserResourceTest extends EntityResourceTest { return create(name).withDescription(description).withDisplayName(displayName).withProfile(PROFILE); } + @Override + public Object addAllRelationships(TestInfo test, Object create) throws HttpResponseException { + TeamResourceTest teamResourceTest = new TeamResourceTest(); + Team team1 = createTeam(teamResourceTest.create(test), adminAuthHeaders()); + ((CreateUser) create).setTeams(List.of(team1.getId())); + RoleResourceTest roleResourceTest = new RoleResourceTest(); + Role role1 = createRole(roleResourceTest.create(test), adminAuthHeaders()); + ((CreateUser) create).setRoles(List.of(role1.getId())); + return create; + } + @Override public EntityReference getContainer(Object createRequest) throws URISyntaxException { return null; // No container entity