From d09cef479b4c673d1a4ce26edad9f8f7ab66775b Mon Sep 17 00:00:00 2001 From: Justin Donn Date: Thu, 9 Sep 2021 12:28:43 -0700 Subject: [PATCH 01/13] feat: add pre-update hooks to BaseLocalDAO Introduce infrastructure for pre-update hooks, motivated by the need for a pre-update hook to remove duplicate elements from certain aspects. --- .../linkedin/metadata/dao/BaseLocalDAO.java | 52 +++++++++++++++---- .../metadata/dao/BaseLocalDAOTest.java | 31 +++++++++++ 2 files changed, 74 insertions(+), 9 deletions(-) diff --git a/dao-api/src/main/java/com/linkedin/metadata/dao/BaseLocalDAO.java b/dao-api/src/main/java/com/linkedin/metadata/dao/BaseLocalDAO.java index 7c45e2410..93ada7ee6 100644 --- a/dao-api/src/main/java/com/linkedin/metadata/dao/BaseLocalDAO.java +++ b/dao-api/src/main/java/com/linkedin/metadata/dao/BaseLocalDAO.java @@ -83,7 +83,11 @@ static class AddResult { // Maps an aspect class to the corresponding retention policy private final Map, Retention> _aspectRetentionMap = new HashMap<>(); - // Maps as aspect class to a list of post-update hooks + // Maps an aspect class to a list of pre-update hooks + private final Map, List>> _aspectPreUpdateHooksMap = + new HashMap<>(); + + // Maps an aspect class to a list of post-update hooks private final Map, List>> _aspectPostUpdateHooksMap = new HashMap<>(); @@ -155,6 +159,30 @@ public Retention getRetention(@Nonnull ClassThe hook will be invoked with the latest value of an aspect before it's updated. There's no guarantee on the + * order of invocation when multiple hooks are added for a single aspect. Adding the same hook again will result in + * {@link IllegalArgumentException} thrown. Hooks are invoked in the order they're registered. + */ + public void addPreUpdateHook(@Nonnull Class aspectClass, + @Nonnull BiConsumer preUpdateHook) { + + checkValidAspect(aspectClass); + // TODO: Also validate Urn once we convert all aspect models to PDL with proper annotation + + final List> hooks = + _aspectPreUpdateHooksMap.getOrDefault(aspectClass, new LinkedList<>()); + + if (hooks.contains(preUpdateHook)) { + throw new IllegalArgumentException("Adding an already-registered hook"); + } + + hooks.add((BiConsumer) preUpdateHook); + _aspectPreUpdateHooksMap.put(aspectClass, hooks); + } + /** * Registers a post-update hook for a specific aspect. * @@ -276,24 +304,30 @@ public ASPECT add(@Nonnull URN urn, @Nonnull Cla final ASPECT oldValue = latest == null ? null : latest.getAspect(); final ASPECT newValue = updateLambda.apply(Optional.ofNullable(oldValue)); checkValidAspect(newValue.getClass()); + if (_modelValidationOnWrite) { validateAgainstSchema(newValue); } - // 2. Skip saving if there's no actual change + // 2. Invoke pre-update hooks, if any + if (_aspectPreUpdateHooksMap.containsKey(aspectClass)) { + _aspectPreUpdateHooksMap.get(aspectClass).forEach(hook -> hook.accept(urn, newValue)); + } + + // 3. Skip saving if there's no actual change if (oldValue != null && equalityTester.equals(oldValue, newValue)) { return new AddResult<>(oldValue, oldValue); } - // 3. Save the newValue as the latest version + // 4. Save the newValue as the latest version long largestVersion = saveLatest(urn, aspectClass, oldValue, latest == null ? null : latest.getExtraInfo().getAudit(), newValue, auditStamp); - // 4. Apply retention policy + // 5. Apply retention policy applyRetention(urn, aspectClass, getRetention(aspectClass), largestVersion); - // 5. Save to local secondary index + // 6. Save to local secondary index if (_enableLocalSecondaryIndex) { updateLocalIndex(urn, newValue, largestVersion); } @@ -304,19 +338,19 @@ public ASPECT add(@Nonnull URN urn, @Nonnull Cla final ASPECT oldValue = result.getOldValue(); final ASPECT newValue = result.getNewValue(); - // 6. Produce MAE after a successful update + // 7. Produce MAE after a successful update if (_alwaysEmitAuditEvent || oldValue != newValue) { _producer.produceMetadataAuditEvent(urn, oldValue, newValue); } - // TODO: Replace step 6 with step 6.1 after pipeline is fully migrated to aspect specific events. - // 6.1 Produce aspect specific MAE after a successful update + // TODO: Replace step 7 with step 7.1 after pipeline is fully migrated to aspect specific events. + // 7.1 Produce aspect specific MAE after a successful update if (_emitAspectSpecificAuditEvent) { if (_alwaysEmitAspectSpecificAuditEvent || oldValue != newValue) { _producer.produceAspectSpecificMetadataAuditEvent(urn, oldValue, newValue); } } - // 7. Invoke post-update hooks if there's any + // 8. Invoke post-update hooks if there's any if (_aspectPostUpdateHooksMap.containsKey(aspectClass)) { _aspectPostUpdateHooksMap.get(aspectClass).forEach(hook -> hook.accept(urn, newValue)); } diff --git a/dao-api/src/test/java/com/linkedin/metadata/dao/BaseLocalDAOTest.java b/dao-api/src/test/java/com/linkedin/metadata/dao/BaseLocalDAOTest.java index 30f150075..e406e7744 100644 --- a/dao-api/src/test/java/com/linkedin/metadata/dao/BaseLocalDAOTest.java +++ b/dao-api/src/test/java/com/linkedin/metadata/dao/BaseLocalDAOTest.java @@ -228,6 +228,37 @@ public void testMAEEmissionNoValueChange() throws URISyntaxException { verifyNoMoreInteractions(_mockEventProducer); } + @Test + public void testAddSamePreUpdateHookTwice() { + BiConsumer hook = (urn, foo) -> { + // do nothing; + }; + + _dummyLocalDAO.addPreUpdateHook(AspectFoo.class, hook); + + try { + _dummyLocalDAO.addPreUpdateHook(AspectFoo.class, hook); + } catch (IllegalArgumentException e) { + // expected + return; + } + + fail("No IllegalArgumentException thrown"); + } + + @Test + public void testPreUpdateHookInvoked() throws URISyntaxException { + FooUrn urn = new FooUrn(1); + AspectFoo foo = new AspectFoo().setValue("foo"); + BiConsumer hook = mock(BiConsumer.class); + + _dummyLocalDAO.addPreUpdateHook(AspectFoo.class, hook); + _dummyLocalDAO.add(urn, foo, _dummyAuditStamp); + + verify(hook, times(1)).accept(urn, foo); + verifyNoMoreInteractions(hook); + } + @Test public void testAddSamePostUpdateHookTwice() { BiConsumer hook = (urn, foo) -> { From d70fd61a93f112694215150049facd142d053512 Mon Sep 17 00:00:00 2001 From: Justin Donn Date: Fri, 10 Sep 2021 13:55:23 -0700 Subject: [PATCH 02/13] simplified adding pre- and post-update hooks to one helper method. --- .../linkedin/metadata/dao/BaseLocalDAO.java | 50 ++++++++----------- 1 file changed, 22 insertions(+), 28 deletions(-) diff --git a/dao-api/src/main/java/com/linkedin/metadata/dao/BaseLocalDAO.java b/dao-api/src/main/java/com/linkedin/metadata/dao/BaseLocalDAO.java index 93ada7ee6..cc7a081ab 100644 --- a/dao-api/src/main/java/com/linkedin/metadata/dao/BaseLocalDAO.java +++ b/dao-api/src/main/java/com/linkedin/metadata/dao/BaseLocalDAO.java @@ -160,51 +160,45 @@ public Retention getRetention(@Nonnull ClassThe hook will be invoked with the latest value of an aspect before it's updated. There's no guarantee on the - * order of invocation when multiple hooks are added for a single aspect. Adding the same hook again will result in - * {@link IllegalArgumentException} thrown. Hooks are invoked in the order they're registered. + *

The hook will be invoked with the latest value of an aspect before (pre-) or after (post-) it's updated. There's + * no guarantee on the order of invocation when multiple hooks are added for a single aspect. Adding the same hook + * again will result in {@link IllegalArgumentException} thrown. Hooks are invoked in the order they're registered. */ - public void addPreUpdateHook(@Nonnull Class aspectClass, - @Nonnull BiConsumer preUpdateHook) { + public void addHook(@Nonnull Class aspectClass, + @Nonnull BiConsumer hook, + @Nonnull Map, List>> hooksMap) { checkValidAspect(aspectClass); // TODO: Also validate Urn once we convert all aspect models to PDL with proper annotation final List> hooks = - _aspectPreUpdateHooksMap.getOrDefault(aspectClass, new LinkedList<>()); + hooksMap.getOrDefault(aspectClass, new LinkedList<>()); - if (hooks.contains(preUpdateHook)) { + if (hooks.contains(hook)) { throw new IllegalArgumentException("Adding an already-registered hook"); } - hooks.add((BiConsumer) preUpdateHook); - _aspectPreUpdateHooksMap.put(aspectClass, hooks); + hooks.add((BiConsumer) hook); + hooksMap.put(aspectClass, hooks); } /** - * Registers a post-update hook for a specific aspect. - * - *

The hook will be invoked with the latest value of an aspect after it's updated. There's no guarantee on the - * order of invocation when multiple hooks are added for a single aspect. Adding the same hook again will result in - * {@link IllegalArgumentException} thrown. Hooks are invoked in the order they're registered. + * Add a pre-update hook for a specific aspect type. Warning: pre-update hooks are run within a transaction, so avoid + * creating time- and/or resource-consuming pre-update hooks. + */ + public void addPreUpdateHook(@Nonnull Class aspectClass, + @Nonnull BiConsumer preUpdateHook) { + addHook(aspectClass, preUpdateHook, _aspectPreUpdateHooksMap); + } + + /** + * Add a post-update hook for a specific aspect type. */ public void addPostUpdateHook(@Nonnull Class aspectClass, @Nonnull BiConsumer postUpdateHook) { - - checkValidAspect(aspectClass); - // TODO: Also validate Urn once we convert all aspect models to PDL with proper annotation - - final List> hooks = - _aspectPostUpdateHooksMap.getOrDefault(aspectClass, new LinkedList<>()); - - if (hooks.contains(postUpdateHook)) { - throw new IllegalArgumentException("Adding an already-registered hook"); - } - - hooks.add((BiConsumer) postUpdateHook); - _aspectPostUpdateHooksMap.put(aspectClass, hooks); + addHook(aspectClass, postUpdateHook, _aspectPostUpdateHooksMap); } /** From 61de2664b37abf011c2e64da855ce4e1231035df Mon Sep 17 00:00:00 2001 From: Justin Donn Date: Wed, 15 Sep 2021 16:45:41 -0700 Subject: [PATCH 03/13] docs: add getting started section to contributing doc --- docs/CONTRIBUTING.md | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/docs/CONTRIBUTING.md b/docs/CONTRIBUTING.md index c332d9238..43952dcdf 100644 --- a/docs/CONTRIBUTING.md +++ b/docs/CONTRIBUTING.md @@ -15,6 +15,15 @@ If you find a bug: 1. If the issue still reproduces or has not yet been reported, try to isolate the problem before opening an issue. +## Getting Started + +If you haven't done so already, fork and clone the repo by following [GitHub's quickstart guide](https://docs.github.com/en/get-started/quickstart/fork-a-repo). +Make sure to go through the [steps](https://docs.github.com/en/get-started/quickstart/fork-a-repo#configuring-git-to-sync-your-fork-with-the-original-repository) +to add the upstream repo via git remote. + +To contribute to the project, push local changes to your fork (i.e. github.com//datahub-gma instead of github.com/linkedin/datahub-gma), +then follow the below steps for creating a pull request. + ## Submitting a Pull Request (PR) Before you submit your Pull Request (PR), consider the following guidelines: From 64781e9855fc8764d4d190059472a7897f2642da Mon Sep 17 00:00:00 2001 From: Justin Donn Date: Mon, 13 Sep 2021 15:51:49 -0700 Subject: [PATCH 04/13] fixed build style check failure --- docs/CONTRIBUTING.md | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/docs/CONTRIBUTING.md b/docs/CONTRIBUTING.md index 43952dcdf..1bc3ecd2e 100644 --- a/docs/CONTRIBUTING.md +++ b/docs/CONTRIBUTING.md @@ -17,12 +17,13 @@ If you find a bug: ## Getting Started -If you haven't done so already, fork and clone the repo by following [GitHub's quickstart guide](https://docs.github.com/en/get-started/quickstart/fork-a-repo). -Make sure to go through the [steps](https://docs.github.com/en/get-started/quickstart/fork-a-repo#configuring-git-to-sync-your-fork-with-the-original-repository) +If you haven't done so already, fork and clone the repo by following +[GitHub's quickstart guide](https://docs.github.com/en/get-started/quickstart/fork-a-repo). Make sure to go through the +[steps](https://docs.github.com/en/get-started/quickstart/fork-a-repo#configuring-git-to-sync-your-fork-with-the-original-repository) to add the upstream repo via git remote. -To contribute to the project, push local changes to your fork (i.e. github.com//datahub-gma instead of github.com/linkedin/datahub-gma), -then follow the below steps for creating a pull request. +To contribute to the project, push local changes to your fork (i.e. github.com//datahub-gma instead of +github.com/linkedin/datahub-gma), then follow the below steps for creating a pull request. ## Submitting a Pull Request (PR) From e71ee943c927ae87814b9a49cc338656ef5eb322 Mon Sep 17 00:00:00 2001 From: Justin Donn Date: Mon, 18 Oct 2021 02:00:46 -0700 Subject: [PATCH 05/13] check if urn(s) exists when calling getInternalNonEmpty --- .../com/linkedin/metadata/dao/BaseLocalDAO.java | 7 +++++++ .../linkedin/metadata/dao/BaseLocalDAOTest.java | 5 +++++ .../com/linkedin/metadata/dao/EbeanLocalDAO.java | 4 ++++ .../linkedin/metadata/dao/EbeanLocalDAOTest.java | 15 +++++++++++++++ .../metadata/restli/BaseEntityResource.java | 5 ++++- .../metadata/restli/BaseEntityResourceTest.java | 3 +++ .../restli/BaseEntitySimpleKeyResourceTest.java | 3 +++ .../restli/BaseSearchableEntityResourceTest.java | 4 ++++ ...BaseSearchableEntitySimpleKeyResourceTest.java | 4 ++++ 9 files changed, 49 insertions(+), 1 deletion(-) diff --git a/dao-api/src/main/java/com/linkedin/metadata/dao/BaseLocalDAO.java b/dao-api/src/main/java/com/linkedin/metadata/dao/BaseLocalDAO.java index cc7a081ab..47522c822 100644 --- a/dao-api/src/main/java/com/linkedin/metadata/dao/BaseLocalDAO.java +++ b/dao-api/src/main/java/com/linkedin/metadata/dao/BaseLocalDAO.java @@ -594,6 +594,13 @@ protected abstract long getNextVersion(@Nonnull protected abstract void save(@Nonnull URN urn, @Nonnull RecordTemplate value, @Nonnull AuditStamp auditStamp, long version, boolean insert); + /** + * Returns a boolean representing if an Urn has any Aspects associated with it (i.e. if it exists in the DB). + * @param urn {@link Urn} for the entity + * @return boolean representing if entity associated with Urn exists + */ + public abstract boolean exists(@Nonnull URN urn); + /** * Applies version-based retention against a specific aspect type for an entity. * diff --git a/dao-api/src/test/java/com/linkedin/metadata/dao/BaseLocalDAOTest.java b/dao-api/src/test/java/com/linkedin/metadata/dao/BaseLocalDAOTest.java index e406e7744..e6a090ee6 100644 --- a/dao-api/src/test/java/com/linkedin/metadata/dao/BaseLocalDAOTest.java +++ b/dao-api/src/test/java/com/linkedin/metadata/dao/BaseLocalDAOTest.java @@ -77,6 +77,11 @@ protected void save(FooUrn urn, RecordTemplate value, AuditStamp auditStamp, lon } + @Override + public boolean exists(FooUrn urn) { + return true; + } + @Override protected void applyVersionBasedRetention(Class aspectClass, FooUrn urn, VersionBasedRetention retention, long largestVersion) { diff --git a/dao-impl/ebean-dao/src/main/java/com/linkedin/metadata/dao/EbeanLocalDAO.java b/dao-impl/ebean-dao/src/main/java/com/linkedin/metadata/dao/EbeanLocalDAO.java index 7615c7412..d9b8fb10f 100644 --- a/dao-impl/ebean-dao/src/main/java/com/linkedin/metadata/dao/EbeanLocalDAO.java +++ b/dao-impl/ebean-dao/src/main/java/com/linkedin/metadata/dao/EbeanLocalDAO.java @@ -560,6 +560,10 @@ protected void applyTimeBasedRetention(@Nonnull return result; } + public boolean exists(@Nonnull URN urn) { + return _server.find(EbeanMetadataAspect.class).where().eq(URN_COLUMN, urn.toString()).exists(); + } + public boolean existsInLocalIndex(@Nonnull URN urn) { return _server.find(EbeanMetadataIndex.class).where().eq(URN_COLUMN, urn.toString()).exists(); } diff --git a/dao-impl/ebean-dao/src/test/java/com/linkedin/metadata/dao/EbeanLocalDAOTest.java b/dao-impl/ebean-dao/src/test/java/com/linkedin/metadata/dao/EbeanLocalDAOTest.java index 49a3729fa..c1c0a8a87 100644 --- a/dao-impl/ebean-dao/src/test/java/com/linkedin/metadata/dao/EbeanLocalDAOTest.java +++ b/dao-impl/ebean-dao/src/test/java/com/linkedin/metadata/dao/EbeanLocalDAOTest.java @@ -1458,6 +1458,21 @@ record = getRecordFromLocalIndex(recordId); assertEquals(record.getStringVal(), "valFoo"); } + @Test + void testExists() { + // given + EbeanLocalDAO dao = createDao(FooUrn.class); + FooUrn urn = makeFooUrn(1); + + assertFalse(dao.exists(urn)); + + // add metadata + AspectFoo fooV1 = new AspectFoo().setValue("foo"); + addMetadata(urn, AspectFoo.class.getCanonicalName(), 1, fooV1); + + assertTrue(dao.exists(urn)); + } + @Test void testExistsInLocalIndex() { EbeanLocalDAO dao = createDao(BarUrn.class); diff --git a/restli-resources/src/main/java/com/linkedin/metadata/restli/BaseEntityResource.java b/restli-resources/src/main/java/com/linkedin/metadata/restli/BaseEntityResource.java index 84ac93627..4917edfee 100644 --- a/restli-resources/src/main/java/com/linkedin/metadata/restli/BaseEntityResource.java +++ b/restli-resources/src/main/java/com/linkedin/metadata/restli/BaseEntityResource.java @@ -625,8 +625,10 @@ protected Map getInternal(@Nonnull Collection urns, @Nonnull protected Map getInternalNonEmpty(@Nonnull Collection urns, @Nonnull Set> aspectClasses) { + BaseLocalDAO dao = getLocalDAO(); return getUrnAspectMap(urns, aspectClasses).entrySet() .stream() + .filter(e -> dao.exists(e.getKey())) .filter(e -> !e.getValue().isEmpty()) .collect(Collectors.toMap(Map.Entry::getKey, e -> toValue(newSnapshot(e.getKey(), e.getValue())))); } @@ -634,6 +636,7 @@ protected Map getInternalNonEmpty(@Nonnull Collection urns, @Nonnull private Map> getUrnAspectMap(@Nonnull Collection urns, @Nonnull Set> aspectClasses) { + BaseLocalDAO dao = getLocalDAO(); // Construct the keys to retrieve latest version of all supported aspects for all URNs. final Set> keys = urns.stream() .map(urn -> aspectClasses.stream() @@ -645,7 +648,7 @@ private Map> getUrnAspectMap(@Nonnull Collection u final Map> urnAspectsMap = urns.stream().collect(Collectors.toMap(Function.identity(), urn -> new ArrayList<>())); - getLocalDAO().get(keys) + dao.get(keys) .forEach((key, aspect) -> aspect.ifPresent( metadata -> urnAspectsMap.get(key.getUrn()).add(ModelUtils.newAspectUnion(_aspectUnionClass, metadata)))); diff --git a/restli-resources/src/test/java/com/linkedin/metadata/restli/BaseEntityResourceTest.java b/restli-resources/src/test/java/com/linkedin/metadata/restli/BaseEntityResourceTest.java index bd432fcb0..5978889fe 100644 --- a/restli-resources/src/test/java/com/linkedin/metadata/restli/BaseEntityResourceTest.java +++ b/restli-resources/src/test/java/com/linkedin/metadata/restli/BaseEntityResourceTest.java @@ -143,6 +143,7 @@ public void testGet() { when(_mockLocalDAO.get(new HashSet<>(Arrays.asList(aspect1Key, aspect2Key)))).thenReturn( Collections.singletonMap(aspect1Key, Optional.of(foo))); + when(_mockLocalDAO.exists(urn)).thenReturn(true); EntityValue value = runAndWait(_resource.get(makeResourceKey(urn), null)); @@ -178,6 +179,7 @@ public void testGetSpecificAspect() { when(_mockLocalDAO.get(new HashSet<>(Arrays.asList(aspect1Key)))).thenReturn( Collections.singletonMap(aspect1Key, Optional.of(foo))); + when(_mockLocalDAO.exists(urn)).thenReturn(true); EntityValue value = runAndWait(_resource.get(makeResourceKey(urn), aspectNames)); assertEquals(value.getFoo(), foo); @@ -194,6 +196,7 @@ public void testGetSpecificAspectNotFound() { } catch (RestLiServiceException e) { assertEquals(e.getStatus(), HttpStatus.S_404_NOT_FOUND); verify(_mockLocalDAO, times(1)).get(Collections.singleton(aspect1Key)); + verify(_mockLocalDAO, times(1)).exists(urn); verifyNoMoreInteractions(_mockLocalDAO); return; } diff --git a/restli-resources/src/test/java/com/linkedin/metadata/restli/BaseEntitySimpleKeyResourceTest.java b/restli-resources/src/test/java/com/linkedin/metadata/restli/BaseEntitySimpleKeyResourceTest.java index f120dbcc9..5c1fe973e 100644 --- a/restli-resources/src/test/java/com/linkedin/metadata/restli/BaseEntitySimpleKeyResourceTest.java +++ b/restli-resources/src/test/java/com/linkedin/metadata/restli/BaseEntitySimpleKeyResourceTest.java @@ -58,6 +58,7 @@ public void testGet() { when(_mockLocalDAO.get(new HashSet<>(Arrays.asList(aspect1Key, aspect2Key)))) .thenReturn(Collections.singletonMap(aspect1Key, Optional.of(foo))); + when(_mockLocalDAO.exists(urn)).thenReturn(true); EntityValue value = runAndWait(_resource.get(id, null)); @@ -95,6 +96,7 @@ public void testGetSpecificAspect() { when(_mockLocalDAO.get(new HashSet<>(Collections.singletonList(aspect1Key)))) .thenReturn(Collections.singletonMap(aspect1Key, Optional.of(foo))); + when(_mockLocalDAO.exists(urn)).thenReturn(true); EntityValue value = runAndWait(_resource.get(id, aspectNames)); assertEquals(value.getFoo(), foo); @@ -113,6 +115,7 @@ public void testGetSpecificAspectNotFound() { } catch (RestLiServiceException e) { assertEquals(e.getStatus(), HttpStatus.S_404_NOT_FOUND); verify(_mockLocalDAO, times(1)).get(Collections.singleton(aspect1Key)); + verify(_mockLocalDAO, times(1)).exists(urn); verifyNoMoreInteractions(_mockLocalDAO); } } diff --git a/restli-resources/src/test/java/com/linkedin/metadata/restli/BaseSearchableEntityResourceTest.java b/restli-resources/src/test/java/com/linkedin/metadata/restli/BaseSearchableEntityResourceTest.java index db158a4fc..ab08abac4 100644 --- a/restli-resources/src/test/java/com/linkedin/metadata/restli/BaseSearchableEntityResourceTest.java +++ b/restli-resources/src/test/java/com/linkedin/metadata/restli/BaseSearchableEntityResourceTest.java @@ -151,6 +151,8 @@ public void testSearch() { String[] aspectNames = new String[]{ModelUtils.getAspectName(AspectFoo.class)}; when(_mockLocalDAO.get(ImmutableSet.of(aspectKey1, aspectKey2))).thenReturn( ImmutableMap.of(aspectKey1, Optional.of(foo), aspectKey2, Optional.empty())); + when(_mockLocalDAO.exists(urn1)).thenReturn(true); + when(_mockLocalDAO.exists(urn2)).thenReturn(true); CollectionResult searchResult = runAndWait(_resource.search("bar", aspectNames, EMPTY_FILTER, null, new PagingContext(1, 2))); @@ -207,6 +209,8 @@ public void testGetAll() { String[] aspectNames = new String[]{ModelUtils.getAspectName(AspectFoo.class)}; when(_mockLocalDAO.get(ImmutableSet.of(aspectKey1, aspectKey2))).thenReturn( ImmutableMap.of(aspectKey1, Optional.of(foo), aspectKey2, Optional.of(foo))); + when(_mockLocalDAO.exists(urn1)).thenReturn(true); + when(_mockLocalDAO.exists(urn2)).thenReturn(true); // test with null filter and null sort criterion List values = diff --git a/restli-resources/src/test/java/com/linkedin/metadata/restli/BaseSearchableEntitySimpleKeyResourceTest.java b/restli-resources/src/test/java/com/linkedin/metadata/restli/BaseSearchableEntitySimpleKeyResourceTest.java index dff297301..4dcbc02e2 100644 --- a/restli-resources/src/test/java/com/linkedin/metadata/restli/BaseSearchableEntitySimpleKeyResourceTest.java +++ b/restli-resources/src/test/java/com/linkedin/metadata/restli/BaseSearchableEntitySimpleKeyResourceTest.java @@ -74,6 +74,8 @@ public void testSearch() { String[] aspectNames = new String[]{ModelUtils.getAspectName(AspectFoo.class)}; when(_mockLocalDAO.get(ImmutableSet.of(aspectKey1, aspectKey2))).thenReturn( ImmutableMap.of(aspectKey1, Optional.of(foo), aspectKey2, Optional.empty())); + when(_mockLocalDAO.exists(urn1)).thenReturn(true); + when(_mockLocalDAO.exists(urn2)).thenReturn(true); CollectionResult searchResult = runAndWait(_resource.search("bar", aspectNames, filter, null, new PagingContext(1, 2))); @@ -133,6 +135,8 @@ public void testGetAll() { String[] aspectNames = new String[]{ModelUtils.getAspectName(AspectFoo.class)}; when(_mockLocalDAO.get(ImmutableSet.of(aspectKey1, aspectKey2))).thenReturn( ImmutableMap.of(aspectKey1, Optional.of(foo), aspectKey2, Optional.of(foo))); + when(_mockLocalDAO.exists(urn1)).thenReturn(true); + when(_mockLocalDAO.exists(urn2)).thenReturn(true); // test with null filter and null sort criterion List values = From d442491b9bdd7b8ec93024f4aad3e502d308d1fc Mon Sep 17 00:00:00 2001 From: Justin Donn Date: Mon, 18 Oct 2021 13:34:07 -0700 Subject: [PATCH 06/13] cleaned up some code --- .../com/linkedin/metadata/restli/BaseEntityResource.java | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/restli-resources/src/main/java/com/linkedin/metadata/restli/BaseEntityResource.java b/restli-resources/src/main/java/com/linkedin/metadata/restli/BaseEntityResource.java index 4917edfee..c700e4ca6 100644 --- a/restli-resources/src/main/java/com/linkedin/metadata/restli/BaseEntityResource.java +++ b/restli-resources/src/main/java/com/linkedin/metadata/restli/BaseEntityResource.java @@ -625,10 +625,9 @@ protected Map getInternal(@Nonnull Collection urns, @Nonnull protected Map getInternalNonEmpty(@Nonnull Collection urns, @Nonnull Set> aspectClasses) { - BaseLocalDAO dao = getLocalDAO(); return getUrnAspectMap(urns, aspectClasses).entrySet() .stream() - .filter(e -> dao.exists(e.getKey())) + .filter(e -> getLocalDAO().exists(e.getKey())) .filter(e -> !e.getValue().isEmpty()) .collect(Collectors.toMap(Map.Entry::getKey, e -> toValue(newSnapshot(e.getKey(), e.getValue())))); } @@ -636,7 +635,6 @@ protected Map getInternalNonEmpty(@Nonnull Collection urns, @Nonnull private Map> getUrnAspectMap(@Nonnull Collection urns, @Nonnull Set> aspectClasses) { - BaseLocalDAO dao = getLocalDAO(); // Construct the keys to retrieve latest version of all supported aspects for all URNs. final Set> keys = urns.stream() .map(urn -> aspectClasses.stream() @@ -648,7 +646,7 @@ private Map> getUrnAspectMap(@Nonnull Collection u final Map> urnAspectsMap = urns.stream().collect(Collectors.toMap(Function.identity(), urn -> new ArrayList<>())); - dao.get(keys) + getLocalDAO().get(keys) .forEach((key, aspect) -> aspect.ifPresent( metadata -> urnAspectsMap.get(key.getUrn()).add(ModelUtils.newAspectUnion(_aspectUnionClass, metadata)))); From f6ba022b98eecdefe9c2897d6e6dd2853d512458 Mon Sep 17 00:00:00 2001 From: Justin Donn Date: Mon, 18 Oct 2021 15:03:39 -0700 Subject: [PATCH 07/13] cleaner approach --- .../com/linkedin/metadata/restli/BaseEntityResource.java | 6 ++++-- .../linkedin/metadata/restli/BaseEntityResourceTest.java | 5 ++--- .../metadata/restli/BaseSearchableEntityResourceTest.java | 4 ---- 3 files changed, 6 insertions(+), 9 deletions(-) diff --git a/restli-resources/src/main/java/com/linkedin/metadata/restli/BaseEntityResource.java b/restli-resources/src/main/java/com/linkedin/metadata/restli/BaseEntityResource.java index c700e4ca6..29730a909 100644 --- a/restli-resources/src/main/java/com/linkedin/metadata/restli/BaseEntityResource.java +++ b/restli-resources/src/main/java/com/linkedin/metadata/restli/BaseEntityResource.java @@ -147,7 +147,10 @@ public Task get(@Nonnull KEY id, return RestliUtils.toTask(() -> { final URN urn = toUrn(id); - final VALUE value = getInternalNonEmpty(Collections.singleton(urn), parseAspectsParam(aspectNames)).get(urn); + if (!getLocalDAO().exists(urn)) { + throw RestliUtils.resourceNotFoundException(); + } + final VALUE value = getInternal(Collections.singleton(urn), parseAspectsParam(aspectNames)).get(urn); if (value == null) { throw RestliUtils.resourceNotFoundException(); } @@ -627,7 +630,6 @@ protected Map getInternalNonEmpty(@Nonnull Collection urns, @Nonnull Set> aspectClasses) { return getUrnAspectMap(urns, aspectClasses).entrySet() .stream() - .filter(e -> getLocalDAO().exists(e.getKey())) .filter(e -> !e.getValue().isEmpty()) .collect(Collectors.toMap(Map.Entry::getKey, e -> toValue(newSnapshot(e.getKey(), e.getValue())))); } diff --git a/restli-resources/src/test/java/com/linkedin/metadata/restli/BaseEntityResourceTest.java b/restli-resources/src/test/java/com/linkedin/metadata/restli/BaseEntityResourceTest.java index 5978889fe..a77aae10f 100644 --- a/restli-resources/src/test/java/com/linkedin/metadata/restli/BaseEntityResourceTest.java +++ b/restli-resources/src/test/java/com/linkedin/metadata/restli/BaseEntityResourceTest.java @@ -141,9 +141,9 @@ public void testGet() { AspectKey aspect1Key = new AspectKey<>(AspectFoo.class, urn, LATEST_VERSION); AspectKey aspect2Key = new AspectKey<>(AspectBar.class, urn, LATEST_VERSION); + when(_mockLocalDAO.exists(urn)).thenReturn(true); when(_mockLocalDAO.get(new HashSet<>(Arrays.asList(aspect1Key, aspect2Key)))).thenReturn( Collections.singletonMap(aspect1Key, Optional.of(foo))); - when(_mockLocalDAO.exists(urn)).thenReturn(true); EntityValue value = runAndWait(_resource.get(makeResourceKey(urn), null)); @@ -177,9 +177,9 @@ public void testGetSpecificAspect() { AspectKey aspect1Key = new AspectKey<>(AspectFoo.class, urn, LATEST_VERSION); String[] aspectNames = {AspectFoo.class.getCanonicalName()}; + when(_mockLocalDAO.exists(urn)).thenReturn(true); when(_mockLocalDAO.get(new HashSet<>(Arrays.asList(aspect1Key)))).thenReturn( Collections.singletonMap(aspect1Key, Optional.of(foo))); - when(_mockLocalDAO.exists(urn)).thenReturn(true); EntityValue value = runAndWait(_resource.get(makeResourceKey(urn), aspectNames)); assertEquals(value.getFoo(), foo); @@ -196,7 +196,6 @@ public void testGetSpecificAspectNotFound() { } catch (RestLiServiceException e) { assertEquals(e.getStatus(), HttpStatus.S_404_NOT_FOUND); verify(_mockLocalDAO, times(1)).get(Collections.singleton(aspect1Key)); - verify(_mockLocalDAO, times(1)).exists(urn); verifyNoMoreInteractions(_mockLocalDAO); return; } diff --git a/restli-resources/src/test/java/com/linkedin/metadata/restli/BaseSearchableEntityResourceTest.java b/restli-resources/src/test/java/com/linkedin/metadata/restli/BaseSearchableEntityResourceTest.java index ab08abac4..db158a4fc 100644 --- a/restli-resources/src/test/java/com/linkedin/metadata/restli/BaseSearchableEntityResourceTest.java +++ b/restli-resources/src/test/java/com/linkedin/metadata/restli/BaseSearchableEntityResourceTest.java @@ -151,8 +151,6 @@ public void testSearch() { String[] aspectNames = new String[]{ModelUtils.getAspectName(AspectFoo.class)}; when(_mockLocalDAO.get(ImmutableSet.of(aspectKey1, aspectKey2))).thenReturn( ImmutableMap.of(aspectKey1, Optional.of(foo), aspectKey2, Optional.empty())); - when(_mockLocalDAO.exists(urn1)).thenReturn(true); - when(_mockLocalDAO.exists(urn2)).thenReturn(true); CollectionResult searchResult = runAndWait(_resource.search("bar", aspectNames, EMPTY_FILTER, null, new PagingContext(1, 2))); @@ -209,8 +207,6 @@ public void testGetAll() { String[] aspectNames = new String[]{ModelUtils.getAspectName(AspectFoo.class)}; when(_mockLocalDAO.get(ImmutableSet.of(aspectKey1, aspectKey2))).thenReturn( ImmutableMap.of(aspectKey1, Optional.of(foo), aspectKey2, Optional.of(foo))); - when(_mockLocalDAO.exists(urn1)).thenReturn(true); - when(_mockLocalDAO.exists(urn2)).thenReturn(true); // test with null filter and null sort criterion List values = From 9123e2577faba44e508d50fdd80f954b6cd5294c Mon Sep 17 00:00:00 2001 From: Justin Donn Date: Mon, 18 Oct 2021 15:58:49 -0700 Subject: [PATCH 08/13] cleaned up code, added a todo for batch get --- .../java/com/linkedin/metadata/restli/BaseEntityResource.java | 1 + .../com/linkedin/metadata/restli/BaseEntityResourceTest.java | 2 +- .../metadata/restli/BaseEntitySimpleKeyResourceTest.java | 1 - .../restli/BaseSingleAspectEntitySimpleKeyResourceTest.java | 1 + 4 files changed, 3 insertions(+), 2 deletions(-) diff --git a/restli-resources/src/main/java/com/linkedin/metadata/restli/BaseEntityResource.java b/restli-resources/src/main/java/com/linkedin/metadata/restli/BaseEntityResource.java index 29730a909..dca166b51 100644 --- a/restli-resources/src/main/java/com/linkedin/metadata/restli/BaseEntityResource.java +++ b/restli-resources/src/main/java/com/linkedin/metadata/restli/BaseEntityResource.java @@ -166,6 +166,7 @@ public Task get(@Nonnull KEY id, public Task> batchGet( @Nonnull Set ids, @QueryParam(PARAM_ASPECTS) @Optional @Nullable String[] aspectNames) { + // TODO: META-15492 - discuss and sync with get()'s intended behavior (check if urn exists). return RestliUtils.toTask(() -> { final Map urnMap = ids.stream().collect(Collectors.toMap(id -> toUrn(id), Function.identity())); diff --git a/restli-resources/src/test/java/com/linkedin/metadata/restli/BaseEntityResourceTest.java b/restli-resources/src/test/java/com/linkedin/metadata/restli/BaseEntityResourceTest.java index a77aae10f..5f0c2769b 100644 --- a/restli-resources/src/test/java/com/linkedin/metadata/restli/BaseEntityResourceTest.java +++ b/restli-resources/src/test/java/com/linkedin/metadata/restli/BaseEntityResourceTest.java @@ -195,7 +195,7 @@ public void testGetSpecificAspectNotFound() { runAndWait(_resource.get(makeResourceKey(urn), aspectNames)); } catch (RestLiServiceException e) { assertEquals(e.getStatus(), HttpStatus.S_404_NOT_FOUND); - verify(_mockLocalDAO, times(1)).get(Collections.singleton(aspect1Key)); + verify(_mockLocalDAO, times(1)).exists(urn); verifyNoMoreInteractions(_mockLocalDAO); return; } diff --git a/restli-resources/src/test/java/com/linkedin/metadata/restli/BaseEntitySimpleKeyResourceTest.java b/restli-resources/src/test/java/com/linkedin/metadata/restli/BaseEntitySimpleKeyResourceTest.java index 5c1fe973e..290002b1e 100644 --- a/restli-resources/src/test/java/com/linkedin/metadata/restli/BaseEntitySimpleKeyResourceTest.java +++ b/restli-resources/src/test/java/com/linkedin/metadata/restli/BaseEntitySimpleKeyResourceTest.java @@ -114,7 +114,6 @@ public void testGetSpecificAspectNotFound() { runAndWait(_resource.get(id, aspectNames)); } catch (RestLiServiceException e) { assertEquals(e.getStatus(), HttpStatus.S_404_NOT_FOUND); - verify(_mockLocalDAO, times(1)).get(Collections.singleton(aspect1Key)); verify(_mockLocalDAO, times(1)).exists(urn); verifyNoMoreInteractions(_mockLocalDAO); } diff --git a/restli-resources/src/test/java/com/linkedin/metadata/restli/BaseSingleAspectEntitySimpleKeyResourceTest.java b/restli-resources/src/test/java/com/linkedin/metadata/restli/BaseSingleAspectEntitySimpleKeyResourceTest.java index 764e998f4..22f6763e6 100644 --- a/restli-resources/src/test/java/com/linkedin/metadata/restli/BaseSingleAspectEntitySimpleKeyResourceTest.java +++ b/restli-resources/src/test/java/com/linkedin/metadata/restli/BaseSingleAspectEntitySimpleKeyResourceTest.java @@ -61,6 +61,7 @@ public void testGet() throws URISyntaxException { SingleAspectEntityUrn urn = new SingleAspectEntityUrn(id1); AspectBar aspect = new AspectBar().setValue(field1); AspectKey aspectKey = new AspectKey<>(AspectBar.class, urn, LATEST_VERSION); + when(_mockLocalDao.exists(urn)).thenReturn(true); when(_mockLocalDao.get(Collections.singleton(aspectKey))).thenReturn( Collections.singletonMap(aspectKey, Optional.of(aspect))); From dfe10d39bc7e073c89e7aac03e1cbdd9e2214d1c Mon Sep 17 00:00:00 2001 From: Justin Donn Date: Mon, 18 Oct 2021 16:05:18 -0700 Subject: [PATCH 09/13] cleaned up some tests --- .../metadata/restli/BaseEntitySimpleKeyResourceTest.java | 4 ++-- .../restli/BaseSearchableEntitySimpleKeyResourceTest.java | 4 ---- 2 files changed, 2 insertions(+), 6 deletions(-) diff --git a/restli-resources/src/test/java/com/linkedin/metadata/restli/BaseEntitySimpleKeyResourceTest.java b/restli-resources/src/test/java/com/linkedin/metadata/restli/BaseEntitySimpleKeyResourceTest.java index 290002b1e..9dd787dea 100644 --- a/restli-resources/src/test/java/com/linkedin/metadata/restli/BaseEntitySimpleKeyResourceTest.java +++ b/restli-resources/src/test/java/com/linkedin/metadata/restli/BaseEntitySimpleKeyResourceTest.java @@ -56,9 +56,9 @@ public void testGet() { AspectKey aspect1Key = new AspectKey<>(AspectFoo.class, urn, LATEST_VERSION); AspectKey aspect2Key = new AspectKey<>(AspectBar.class, urn, LATEST_VERSION); + when(_mockLocalDAO.exists(urn)).thenReturn(true); when(_mockLocalDAO.get(new HashSet<>(Arrays.asList(aspect1Key, aspect2Key)))) .thenReturn(Collections.singletonMap(aspect1Key, Optional.of(foo))); - when(_mockLocalDAO.exists(urn)).thenReturn(true); EntityValue value = runAndWait(_resource.get(id, null)); @@ -94,9 +94,9 @@ public void testGetSpecificAspect() { AspectKey aspect1Key = new AspectKey<>(AspectFoo.class, urn, LATEST_VERSION); String[] aspectNames = {AspectFoo.class.getCanonicalName()}; + when(_mockLocalDAO.exists(urn)).thenReturn(true); when(_mockLocalDAO.get(new HashSet<>(Collections.singletonList(aspect1Key)))) .thenReturn(Collections.singletonMap(aspect1Key, Optional.of(foo))); - when(_mockLocalDAO.exists(urn)).thenReturn(true); EntityValue value = runAndWait(_resource.get(id, aspectNames)); assertEquals(value.getFoo(), foo); diff --git a/restli-resources/src/test/java/com/linkedin/metadata/restli/BaseSearchableEntitySimpleKeyResourceTest.java b/restli-resources/src/test/java/com/linkedin/metadata/restli/BaseSearchableEntitySimpleKeyResourceTest.java index 4dcbc02e2..dff297301 100644 --- a/restli-resources/src/test/java/com/linkedin/metadata/restli/BaseSearchableEntitySimpleKeyResourceTest.java +++ b/restli-resources/src/test/java/com/linkedin/metadata/restli/BaseSearchableEntitySimpleKeyResourceTest.java @@ -74,8 +74,6 @@ public void testSearch() { String[] aspectNames = new String[]{ModelUtils.getAspectName(AspectFoo.class)}; when(_mockLocalDAO.get(ImmutableSet.of(aspectKey1, aspectKey2))).thenReturn( ImmutableMap.of(aspectKey1, Optional.of(foo), aspectKey2, Optional.empty())); - when(_mockLocalDAO.exists(urn1)).thenReturn(true); - when(_mockLocalDAO.exists(urn2)).thenReturn(true); CollectionResult searchResult = runAndWait(_resource.search("bar", aspectNames, filter, null, new PagingContext(1, 2))); @@ -135,8 +133,6 @@ public void testGetAll() { String[] aspectNames = new String[]{ModelUtils.getAspectName(AspectFoo.class)}; when(_mockLocalDAO.get(ImmutableSet.of(aspectKey1, aspectKey2))).thenReturn( ImmutableMap.of(aspectKey1, Optional.of(foo), aspectKey2, Optional.of(foo))); - when(_mockLocalDAO.exists(urn1)).thenReturn(true); - when(_mockLocalDAO.exists(urn2)).thenReturn(true); // test with null filter and null sort criterion List values = From 7fbe76854fd7806af307be01fadc629c085a8214 Mon Sep 17 00:00:00 2001 From: Justin Donn Date: Tue, 2 Nov 2021 14:10:52 -0700 Subject: [PATCH 10/13] added more tests to cover new expected behavior --- .../BaseEntitySimpleKeyResourceTest.java | 42 +++++++++++++------ 1 file changed, 30 insertions(+), 12 deletions(-) diff --git a/restli-resources/src/test/java/com/linkedin/metadata/restli/BaseEntitySimpleKeyResourceTest.java b/restli-resources/src/test/java/com/linkedin/metadata/restli/BaseEntitySimpleKeyResourceTest.java index 9dd787dea..64baed454 100644 --- a/restli-resources/src/test/java/com/linkedin/metadata/restli/BaseEntitySimpleKeyResourceTest.java +++ b/restli-resources/src/test/java/com/linkedin/metadata/restli/BaseEntitySimpleKeyResourceTest.java @@ -67,21 +67,38 @@ public void testGet() { } @Test - public void testGetNotFound() { + public void testGetUrnNotFound() { long id = 1234; Urn urn = makeUrn(id); + when(_mockLocalDAO.exists(urn)).thenReturn(false); + + try { + runAndWait(_resource.get(id, new String[0])); + fail("An exception should've been thrown!"); + } catch (RestLiServiceException e) { + assertEquals(e.getStatus(), HttpStatus.S_404_NOT_FOUND); + } + } + + @Test + public void testGetWithEmptyAspects() { + long id = 1234; + Urn urn = makeUrn(id); + AspectFoo foo = new AspectFoo().setValue("foo"); AspectKey aspect1Key = new AspectKey<>(AspectFoo.class, urn, LATEST_VERSION); AspectKey aspect2Key = new AspectKey<>(AspectBar.class, urn, LATEST_VERSION); - when(_mockLocalDAO.get(new HashSet<>(Arrays.asList(aspect1Key, aspect2Key)))).thenReturn( - Collections.emptyMap()); + when(_mockLocalDAO.exists(urn)).thenReturn(true); + when(_mockLocalDAO.get(new HashSet<>(Arrays.asList(aspect1Key, aspect2Key)))) + .thenReturn(Collections.singletonMap(aspect1Key, Optional.of(foo))); try { - runAndWait(_resource.get(id, new String[0])); + EntityValue value = runAndWait(_resource.get(id, new String[0])); + assertFalse(value.hasFoo()); + assertFalse(value.hasBar()); } catch (RestLiServiceException e) { - assertEquals(e.getStatus(), HttpStatus.S_404_NOT_FOUND); - return; + fail("No exception should be thrown!"); } } @@ -89,7 +106,6 @@ public void testGetNotFound() { public void testGetSpecificAspect() { long id = 1234; Urn urn = makeUrn(id); - AspectFoo foo = new AspectFoo().setValue("foo"); AspectKey aspect1Key = new AspectKey<>(AspectFoo.class, urn, LATEST_VERSION); String[] aspectNames = {AspectFoo.class.getCanonicalName()}; @@ -107,15 +123,17 @@ public void testGetSpecificAspect() { public void testGetSpecificAspectNotFound() { long id = 1234; Urn urn = makeUrn(id); - AspectKey aspect1Key = new AspectKey<>(AspectFoo.class, urn, LATEST_VERSION); String[] aspectNames = {AspectFoo.class.getCanonicalName()}; + + when(_mockLocalDAO.exists(urn)).thenReturn(true); + try { - runAndWait(_resource.get(id, aspectNames)); + EntityValue value = runAndWait(_resource.get(id, aspectNames)); + assertFalse(value.hasFoo()); + assertFalse(value.hasBar()); } catch (RestLiServiceException e) { - assertEquals(e.getStatus(), HttpStatus.S_404_NOT_FOUND); - verify(_mockLocalDAO, times(1)).exists(urn); - verifyNoMoreInteractions(_mockLocalDAO); + fail("No exception should be thrown!"); } } From 31e3315576824ef0273f5a8937ece78b02e4f5a0 Mon Sep 17 00:00:00 2001 From: Justin Donn Date: Tue, 2 Nov 2021 14:36:26 -0700 Subject: [PATCH 11/13] added override tag for exists method --- .../src/main/java/com/linkedin/metadata/dao/EbeanLocalDAO.java | 1 + 1 file changed, 1 insertion(+) diff --git a/dao-impl/ebean-dao/src/main/java/com/linkedin/metadata/dao/EbeanLocalDAO.java b/dao-impl/ebean-dao/src/main/java/com/linkedin/metadata/dao/EbeanLocalDAO.java index d9b8fb10f..3ce09f1e6 100644 --- a/dao-impl/ebean-dao/src/main/java/com/linkedin/metadata/dao/EbeanLocalDAO.java +++ b/dao-impl/ebean-dao/src/main/java/com/linkedin/metadata/dao/EbeanLocalDAO.java @@ -560,6 +560,7 @@ protected void applyTimeBasedRetention(@Nonnull return result; } + @Override public boolean exists(@Nonnull URN urn) { return _server.find(EbeanMetadataAspect.class).where().eq(URN_COLUMN, urn.toString()).exists(); } From 76eaec2d0d0dc9a5c9e52cef7d29c4b2a8904a8c Mon Sep 17 00:00:00 2001 From: Justin Donn Date: Wed, 3 Nov 2021 15:35:51 -0700 Subject: [PATCH 12/13] removed reference to internal ticket --- .../java/com/linkedin/metadata/restli/BaseEntityResource.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/restli-resources/src/main/java/com/linkedin/metadata/restli/BaseEntityResource.java b/restli-resources/src/main/java/com/linkedin/metadata/restli/BaseEntityResource.java index dca166b51..070674a78 100644 --- a/restli-resources/src/main/java/com/linkedin/metadata/restli/BaseEntityResource.java +++ b/restli-resources/src/main/java/com/linkedin/metadata/restli/BaseEntityResource.java @@ -166,7 +166,7 @@ public Task get(@Nonnull KEY id, public Task> batchGet( @Nonnull Set ids, @QueryParam(PARAM_ASPECTS) @Optional @Nullable String[] aspectNames) { - // TODO: META-15492 - discuss and sync with get()'s intended behavior (check if urn exists). + // TODO: discuss and sync with get()'s intended behavior (check if urn exists). https://github.com/linkedin/datahub-gma/issues/136 return RestliUtils.toTask(() -> { final Map urnMap = ids.stream().collect(Collectors.toMap(id -> toUrn(id), Function.identity())); From 3adab08fa3cefc0de70ccb48c56970590fc798c8 Mon Sep 17 00:00:00 2001 From: Justin Donn Date: Wed, 3 Nov 2021 16:43:45 -0700 Subject: [PATCH 13/13] added test cases for BaseEntityResourceTest --- .../restli/BaseEntityResourceTest.java | 38 ++++++++++++++----- .../BaseEntitySimpleKeyResourceTest.java | 3 +- 2 files changed, 29 insertions(+), 12 deletions(-) diff --git a/restli-resources/src/test/java/com/linkedin/metadata/restli/BaseEntityResourceTest.java b/restli-resources/src/test/java/com/linkedin/metadata/restli/BaseEntityResourceTest.java index 5f0c2769b..5e4fca45e 100644 --- a/restli-resources/src/test/java/com/linkedin/metadata/restli/BaseEntityResourceTest.java +++ b/restli-resources/src/test/java/com/linkedin/metadata/restli/BaseEntityResourceTest.java @@ -152,22 +152,40 @@ public void testGet() { } @Test - public void testGetNotFound() { + public void testGetUrnNotFound() { FooUrn urn = makeFooUrn(1234); AspectKey aspect1Key = new AspectKey<>(AspectFoo.class, urn, LATEST_VERSION); AspectKey aspect2Key = new AspectKey<>(AspectBar.class, urn, LATEST_VERSION); + when(_mockLocalDAO.exists(urn)).thenReturn(false); when(_mockLocalDAO.get(new HashSet<>(Arrays.asList(aspect1Key, aspect2Key)))).thenReturn(Collections.emptyMap()); try { runAndWait(_resource.get(makeResourceKey(urn), new String[0])); + fail("An exception should've been thrown!"); } catch (RestLiServiceException e) { assertEquals(e.getStatus(), HttpStatus.S_404_NOT_FOUND); - return; } + } - fail("No exception thrown"); + @Test + public void testGetWithEmptyAspects() { + FooUrn urn = makeFooUrn(1234); + + AspectKey aspect1Key = new AspectKey<>(AspectFoo.class, urn, LATEST_VERSION); + AspectKey aspect2Key = new AspectKey<>(AspectBar.class, urn, LATEST_VERSION); + + when(_mockLocalDAO.exists(urn)).thenReturn(true); + when(_mockLocalDAO.get(new HashSet<>(Arrays.asList(aspect1Key, aspect2Key)))).thenReturn(Collections.emptyMap()); + + try { + EntityValue value = runAndWait(_resource.get(makeResourceKey(urn), new String[0])); + assertFalse(value.hasFoo()); + assertFalse(value.hasBar()); + } catch (RestLiServiceException e) { + fail("No exception should be thrown!"); + } } @Test @@ -189,17 +207,17 @@ public void testGetSpecificAspect() { @Test public void testGetSpecificAspectNotFound() { FooUrn urn = makeFooUrn(1234); - AspectKey aspect1Key = new AspectKey<>(AspectFoo.class, urn, LATEST_VERSION); String[] aspectNames = {AspectFoo.class.getCanonicalName()}; + + when(_mockLocalDAO.exists(urn)).thenReturn(true); + try { - runAndWait(_resource.get(makeResourceKey(urn), aspectNames)); + EntityValue value = runAndWait(_resource.get(makeResourceKey(urn), aspectNames)); + assertFalse(value.hasFoo()); + assertFalse(value.hasBar()); } catch (RestLiServiceException e) { - assertEquals(e.getStatus(), HttpStatus.S_404_NOT_FOUND); - verify(_mockLocalDAO, times(1)).exists(urn); - verifyNoMoreInteractions(_mockLocalDAO); - return; + fail("No exception should be thrown!"); } - fail("No exception thrown"); } @Test diff --git a/restli-resources/src/test/java/com/linkedin/metadata/restli/BaseEntitySimpleKeyResourceTest.java b/restli-resources/src/test/java/com/linkedin/metadata/restli/BaseEntitySimpleKeyResourceTest.java index 64baed454..3222b4ac4 100644 --- a/restli-resources/src/test/java/com/linkedin/metadata/restli/BaseEntitySimpleKeyResourceTest.java +++ b/restli-resources/src/test/java/com/linkedin/metadata/restli/BaseEntitySimpleKeyResourceTest.java @@ -91,7 +91,7 @@ public void testGetWithEmptyAspects() { when(_mockLocalDAO.exists(urn)).thenReturn(true); when(_mockLocalDAO.get(new HashSet<>(Arrays.asList(aspect1Key, aspect2Key)))) - .thenReturn(Collections.singletonMap(aspect1Key, Optional.of(foo))); + .thenReturn(Collections.emptyMap()); try { EntityValue value = runAndWait(_resource.get(id, new String[0])); @@ -123,7 +123,6 @@ public void testGetSpecificAspect() { public void testGetSpecificAspectNotFound() { long id = 1234; Urn urn = makeUrn(id); - AspectKey aspect1Key = new AspectKey<>(AspectFoo.class, urn, LATEST_VERSION); String[] aspectNames = {AspectFoo.class.getCanonicalName()}; when(_mockLocalDAO.exists(urn)).thenReturn(true);