feat(conditional-writes): misc updates and fixes #10901
Conversation
* Non-existent aspect validated against version `-1` * Fix scroll sort order (added test) * Documentation updates (batchGet, `-1` version)
WalkthroughThe recent changes introduce new functionalities for batch operations and conditional writes, focusing on API enhancements and validation logic. Key updates include allowing aspects to have a Changes
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (invoked as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
github-actions
bot
added
docs
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Outside diff range, codebase verification and nitpick comments (2)
metadata-service/openapi-servlet/src/main/java/io/datahubproject/openapi/v3/controller/EntityController.java (1)
129-129: Issue Found: MissingdefaultVersionparameter in function calls tobuildEntityVersionedAspectList.The
defaultVersionparameter is not included in the function calls tobuildEntityVersionedAspectListacross multiple files. This can lead to incorrect functionality.
GenericEntitiesController.javaEntityController.javain bothv2andv3packagesPlease ensure that the
defaultVersionparameter is correctly handled in all instances wherebuildEntityVersionedAspectListis called.Analysis chain
LGTM! But ensure the function usage in the codebase.
The use of
LinkedHashMapforurnAspectVersionsand the additional argumentdefaultVersionare appropriate.However, ensure that all function calls to
buildEntityVersionedAspectListhandleLinkedHashMapanddefaultVersioncorrectly.
Verify the handling of the
defaultVersionparameter in all function calls tobuildEntityVersionedAspectList.We need to ensure that the
defaultVersionparameter is correctly handled in all instances wherebuildEntityVersionedAspectListis called.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify all function calls to `buildEntityVersionedAspectList` handle `LinkedHashMap` and `defaultVersion` correctly. # Test: Search for the function usage. Expect: Only occurances of the new usage. rg --type java -A 5 $'buildEntityVersionedAspectList'Length of output: 5041
Script:
#!/bin/bash # Description: Verify if the new `defaultVersion` parameter is correctly handled in all instances where `buildEntityVersionedAspectList` is called. # Searching for calls to `buildEntityVersionedAspectList` to check for `defaultVersion` parameter. rg --type java -A 5 'buildEntityVersionedAspectList'Length of output: 5041
docs/api/openapi/openapi-usage-guide.md (1)
604-604: Consider a shorter alternative to avoid wordiness.Consider a shorter alternative to avoid wordiness in the phrase "in order to".
- in order to view the current versions of the aspects. + to view the current versions of the aspects.Tools
LanguageTool
[style] ~604-~604: Consider a shorter alternative to avoid wordiness.
Context: ...the url parametersystemMetadata=truein order to view the current versions of the aspec...(IN_ORDER_TO_PREMIUM)
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (8)
- docs/advanced/mcp-mcl.md (2 hunks)
- docs/api/openapi/openapi-usage-guide.md (1 hunks)
- entity-registry/src/main/java/com/linkedin/metadata/aspect/validation/ConditionalWriteValidator.java (3 hunks)
- entity-registry/src/test/java/com/linkedin/metadata/aspect/validators/ConditionalWriteValidatorTest.java (13 hunks)
- metadata-service/openapi-servlet/src/main/java/io/datahubproject/openapi/controller/GenericEntitiesController.java (5 hunks)
- metadata-service/openapi-servlet/src/main/java/io/datahubproject/openapi/v2/controller/EntityController.java (2 hunks)
- metadata-service/openapi-servlet/src/main/java/io/datahubproject/openapi/v3/controller/EntityController.java (4 hunks)
- metadata-service/openapi-servlet/src/test/java/io/datahubproject/openapi/v3/controller/EntityControllerTest.java (1 hunks)
Additional context used
LanguageTool
docs/api/openapi/openapi-usage-guide.md
[typographical] ~597-~597: The word “however” is an adverb that can’t be used like a conjunction, and therefore needs to be separated from the sentence.
Context: ...trieve a specific version of the aspects, however it defaults to the latest aspect versio...(HOWEVER_SENTENCE)
[style] ~604-~604: Consider a shorter alternative to avoid wordiness.
Context: ...the url parametersystemMetadata=truein order to view the current versions of the aspec...(IN_ORDER_TO_PREMIUM)
[grammar] ~693-~693: An apostrophe ‘s’ denotes possession. Did you mean to use the plural form of the noun (no apostrophe)?
Context: ...provided" } } } ] ``` Next let's mutateglobalTagsfor the second URN ...(NOUN_APOSTROPHE_S_VERB)
Additional comments not posted (18)
entity-registry/src/main/java/com/linkedin/metadata/aspect/validation/ConditionalWriteValidator.java (2)
45-45: LGTM! Constant name change toUNVERSIONED_ASPECT_VERSIONis appropriate.The new name is more descriptive, and the value
-1aligns with the documentation updates regarding non-existent aspects.
Line range hint
133-146:
LGTM! Usage ofUNVERSIONED_ASPECT_VERSIONaligns with intended behavior.The introduction of
UNVERSIONED_ASPECT_VERSIONin the switch case forCREATEandCREATE_ENTITYchange types, and as a fallback in the default case, is appropriate.docs/advanced/mcp-mcl.md (2)
186-188: LGTM! Documentation update regarding aspect versions is clear and helpful.The note clarifies that the version for non-existent aspects is
-1, aligning with the code changes.
201-208: LGTM! Documentation update introducing new change types is well-documented.The new change types
CREATEandCREATE_ENTITYare clearly explained, providing clear guidance on their usage.metadata-service/openapi-servlet/src/test/java/io/datahubproject/openapi/v3/controller/EntityControllerTest.java (1)
75-168: LGTM! New test case for sorting order is comprehensive.The test case
testSearchOrderPreservedcovers both ascending and descending order scenarios, ensuring the search order is preserved.metadata-service/openapi-servlet/src/main/java/io/datahubproject/openapi/v2/controller/EntityController.java (2)
191-191: LGTM! Method signature update forbuildEntityVersionedAspectListis appropriate.The update to use
LinkedHashMapensures order is preserved, and thedefaultVersionargument provides flexibility in aspect resolution.
196-196: LGTM! Method signature update forresolveAspectNamesis appropriate.The update to use
LinkedHashMapensures order is preserved, and thedefaultVersionargument provides flexibility in aspect resolution.metadata-service/openapi-servlet/src/main/java/io/datahubproject/openapi/v3/controller/EntityController.java (2)
202-206: LGTM! But ensure the function usage in the codebase.The use of
LinkedHashMapforitemsis appropriate for preserving the insertion order.However, ensure that all function calls to
toEntityVersionRequesthandleLinkedHashMapcorrectly.Verification successful
LGTM! The function usage has been verified and no issues have been found.
The
LinkedHashMapreturned bytoEntityVersionRequestis used correctly in the subsequent code for stream operations and authorization checks.
- Verified at:
metadata-service/openapi-servlet/src/main/java/io/datahubproject/openapi/v3/controller/EntityController.javaScripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify all function calls to `toEntityVersionRequest` handle `LinkedHashMap` correctly. # Test: Search for the function usage. Expect: Only occurances of the new usage. rg --type java -A 5 $'toEntityVersionRequest'Length of output: 1968
Script:
#!/bin/bash # Description: Inspect the usage of `requestMap` after being assigned the result from `toEntityVersionRequest`. # Search for `requestMap` usage in `EntityController.java`. rg --type java -A 10 $'LinkedHashMap<Urn, Map<String, Long>> requestMap = toEntityVersionRequest' metadata-service/openapi-servlet/src/main/java/io/datahubproject/openapi/v3/controller/EntityController.javaLength of output: 790
Script:
#!/bin/bash # Description: Extract more lines of code following the assignment of `requestMap` to verify its correct usage. # Search for `requestMap` usage in `EntityController.java` and extract more lines for full context. rg --type java -A 20 $'LinkedHashMap<Urn, Map<String, Long>> requestMap = toEntityVersionRequest' metadata-service/openapi-servlet/src/main/java/io/datahubproject/openapi/v3/controller/EntityController.javaLength of output: 1181
84-84: LGTM! But ensure the function usage in the codebase.The use of
LinkedHashMapforrequestMapis appropriate for preserving the insertion order.However, ensure that all function calls to
getEntityBatchhandleLinkedHashMapcorrectly.Verification successful
Let's search for all references to
requestMapwithin thev3EntityControllerto ensure that theLinkedHashMapis handled correctly.
Let's search for the implementations of
OperationContext.asSession,AuthUtil.isAPIAuthorizedEntityUrns, andbuildEntityVersionedAspectListto ensure they handleLinkedHashMapcorrectly.
Let's refine our search to locate the complete implementations and additional context for
asSession,isAPIAuthorizedEntityUrns, andbuildEntityVersionedAspectList.
Let's further refine the search for
buildEntityVersionedAspectListimplementation to ensure it correctly handlesLinkedHashMap.
Function usage verified and correctly handles
LinkedHashMap.The usage of
LinkedHashMapin thegetEntityBatchfunction and its related methods (asSession,isAPIAuthorizedEntityUrns, andbuildEntityVersionedAspectList) has been verified to correctly handle the insertion order.
OperationContext.asSessionconstructs a new context, preserving the order.AuthUtil.isAPIAuthorizedEntityUrnsprocesses collections derived fromrequestMap.buildEntityVersionedAspectListstreamsurnAspectVersionskeys and maps them to entities, preserving order.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify all function calls to `getEntityBatch` handle `LinkedHashMap` correctly. # Test: Search for the function usage. Expect: Only occurances of the new usage. rg --type java -A 5 $'getEntityBatch'Length of output: 3686
Script:
#!/bin/bash # Description: Verify all references to `requestMap` in the v3 EntityController. # Search for all references to `requestMap` in the v3 EntityController. rg --type java --context 5 'requestMap' metadata-service/openapi-servlet/src/main/java/io/datahubproject/openapi/v3/controller/EntityController.javaLength of output: 1607
Script:
#!/bin/bash # Search for `OperationContext.asSession` implementation. rg --type java 'public static OperationContext asSession' -A 10 # Search for `AuthUtil.isAPIAuthorizedEntityUrns` implementation. rg --type java 'public static boolean isAPIAuthorizedEntityUrns' -A 10 # Search for `buildEntityVersionedAspectList` implementation. rg --type java 'public List<GenericEntityV3> buildEntityVersionedAspectList' -A 10Length of output: 3152
Script:
#!/bin/bash # Search for `OperationContext.asSession` implementation with more context. rg --type java 'public static OperationContext asSession' -A 20 -B 5 # Search for `AuthUtil.isAPIAuthorizedEntityUrns` implementation with more context. rg --type java 'public static boolean isAPIAuthorizedEntityUrns' -A 20 -B 5 # Search for `buildEntityVersionedAspectList` implementation with more context. rg --type java 'public List<GenericEntityV3> buildEntityVersionedAspectList' -A 20 -B 5Length of output: 6833
Script:
#!/bin/bash # Search for `buildEntityVersionedAspectList` implementation with more context. rg --type java 'buildEntityVersionedAspectList' -A 20 -B 5Length of output: 18666
metadata-service/openapi-servlet/src/main/java/io/datahubproject/openapi/controller/GenericEntitiesController.java (2)
Line range hint
120-139: LGTM! But ensure the function usage in the codebase.The use of
LinkedHashMapforversionMapand the additional argumentdefaultVersionare appropriate.However, ensure that all function calls to
buildEntityListhandleLinkedHashMapanddefaultVersioncorrectly.Verification successful
To ensure the
buildEntityListfunction calls are correctly handlingLinkedHashMapanddefaultVersion, we need to review the context around each usage found in the search results:
- metadata-service/openapi-servlet/src/main/java/io/datahubproject/openapi/v3/controller/EntityController.java
- metadata-service/openapi-servlet/src/main/java/io/datahubproject/openapi/v2/controller/EntityController.java
- metadata-service/openapi-servlet/src/main/java/io/datahubproject/openapi/controller/GenericEntitiesController.java
Let's examine the context around these usages to confirm they are appropriately updated.
Ensure correct handling of
LinkedHashMapanddefaultVersioninbuildEntityListcalls.From the context provided, it appears that the
buildEntityListfunction calls have been updated to handle the new parametersLinkedHashMapanddefaultVersioncorrectly. Specifically:
- metadata-service/openapi-servlet/src/main/java/io/datahubproject/openapi/v3/controller/EntityController.java
- metadata-service/openapi-servlet/src/main/java/io/datahubproject/openapi/v2/controller/EntityController.java
- metadata-service/openapi-servlet/src/main/java/io/datahubproject/openapi/controller/GenericEntitiesController.java
All these files show the correct usage of
LinkedHashMapand the handling ofdefaultVersion.
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify all function calls to `buildEntityList` handle `LinkedHashMap` and `defaultVersion` correctly. # Test: Search for the function usage. Expect: Only occurances of the new usage. rg --type java -A 5 $'buildEntityList'Length of output: 9535
Script:
#!/bin/bash # Extracting the context around each `buildEntityList` usage to verify handling of `LinkedHashMap` and `defaultVersion`. # Extracting 10 lines before and after each usage to ensure we get enough context. rg --type java -A 10 -B 10 $'buildEntityList' metadata-service/openapi-servlet/src/main/java/io/datahubproject/openapi/v3/controller/EntityController.java rg --type java -A 10 -B 10 $'buildEntityList' metadata-service/openapi-servlet/src/main/java/io/datahubproject/openapi/v2/controller/EntityController.java rg --type java -A 10 -B 10 $'buildEntityList' metadata-service/openapi-servlet/src/main/java/io/datahubproject/openapi/controller/GenericEntitiesController.javaLength of output: 7789
655-702: LGTM! But ensure the function usage in the codebase.The use of
LinkedHashMapforrequestedAspectNamesand the additional argumentdefaultValueare appropriate.However, ensure that all function calls to
resolveAspectNameshandleLinkedHashMapanddefaultValuecorrectly.Verification successful
Verification Successful!
All function calls to
resolveAspectNameshandleLinkedHashMapanddefaultValuecorrectly.
metadata-service/openapi-servlet/src/main/java/io/datahubproject/openapi/v2/controller/EntityController.javametadata-service/openapi-servlet/src/main/java/io/datahubproject/openapi/v3/controller/EntityController.javametadata-service/openapi-servlet/src/main/java/io/datahubproject/openapi/controller/GenericEntitiesController.javaScripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify all function calls to `resolveAspectNames` handle `LinkedHashMap` and `defaultValue` correctly. # Test: Search for the function usage. Expect: Only occurances of the new usage. rg --type java -A 5 $'resolveAspectNames'Length of output: 3641
Script:
#!/bin/bash # Extract context around invocations of resolveAspectNames in EntityController.java (v2) rg --type java -A 10 -B 10 'resolveAspectNames' metadata-service/openapi-servlet/src/main/java/io/datahubproject/openapi/v2/controller/EntityController.java # Extract context around invocations of resolveAspectNames in EntityController.java (v3) rg --type java -A 10 -B 10 'resolveAspectNames' metadata-service/openapi-servlet/src/main/java/io/datahubproject/openapi/v3/controller/EntityController.java # Extract context around invocations of resolveAspectNames in GenericEntitiesController.java rg --type java -A 10 -B 10 'resolveAspectNames' metadata-service/openapi-servlet/src/main/java/io/datahubproject/openapi/controller/GenericEntitiesController.javaLength of output: 3719
docs/api/openapi/openapi-usage-guide.md (2)
767-767: LGTM!The documentation for example responses is clear and provides necessary information.
693-693: Correct the plural form of the noun.An apostrophe ‘s’ denotes possession. Use the plural form "lets" instead of "let's".
- Next let's mutate `globalTags` for the second URN + Next lets mutate `globalTags` for the second URNLikely invalid or redundant comment.
Tools
LanguageTool
[grammar] ~693-~693: An apostrophe ‘s’ denotes possession. Did you mean to use the plural form of the noun (no apostrophe)?
Context: ...provided" } } } ] ``` Next let's mutateglobalTagsfor the second URN ...(NOUN_APOSTROPHE_S_VERB)
entity-registry/src/test/java/com/linkedin/metadata/aspect/validators/ConditionalWriteValidatorTest.java (5)
93-93: LGTM! Check consistency of version-1usage.The change aligns with the PR objective to validate non-existent aspects against version
-1.Ensure this logic is consistently applied within other test methods.
Verification successful
Verified: Consistent usage of version
-1in test methodsThe change aligns with the PR objective to validate non-existent aspects against version
-1. The logic is consistently applied within other test methods inConditionalWriteValidatorTest.java.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the consistency of version `-1` usage in the test file. # Test: Search for `HTTP_HEADER_IF_VERSION_MATCH` set to `-1`. Expect: Consistent usage across test methods. rg --type java -A 5 $'HTTP_HEADER_IF_VERSION_MATCH, "-1"'Length of output: 3487
334-334: LGTM! Check consistency of version-1usage.The change aligns with the PR objective to validate non-existent aspects against version
-1.Ensure this logic is consistently applied within other test methods.
Verification successful
LGTM! The usage of
HTTP_HEADER_IF_VERSION_MATCHset to-1is consistent across the test methods.The change aligns with the PR objective to validate non-existent aspects against version
-1.
- Instances found in
ConditionalWriteValidatorTest.javashow consistent usage.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the consistency of version `-1` usage in the test file. # Test: Search for `HTTP_HEADER_IF_VERSION_MATCH` set to `-1`. Expect: Consistent usage across test methods. rg --type java -A 5 $'HTTP_HEADER_IF_VERSION_MATCH, "-1"'Length of output: 3487
140-170: LGTM! Verify correct handling ofDELETE,CREATE, andCREATE_ENTITYchange types.The changes align with the PR objective to handle aspect versions correctly.
Ensure the correct handling of these change types across test methods.
6-6: Ensure correct usage ofresetfrom Mockito.The
resetmethod is useful for resetting mocks. Ensure it is used correctly within the test methods to reset the state of the mock objects.Verification successful
Verified: The
resetmethod from Mockito is used correctly within the test methods.The
resetmethod is consistently used to reset the state of the mock objects, followed by appropriate configurations or setups.
- Lines 1-6: Correct usage.
- Lines 7-12: Correct usage.
- Lines 13-18: Correct usage.
- Lines 19-24: Correct usage.
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the usage of `reset` in the test file. # Test: Search for `reset` usage. Expect: Correct usage within test methods. rg --type java -A 5 $'reset(mockRetrieverContext.getAspectRetriever())'Length of output: 72
Script:
#!/bin/bash # Description: Verify the usage of `reset` in the test file. # Test: Search for all occurrences of `reset` in the test file. rg --type java -A 5 'reset(' entity-registry/src/test/java/com/linkedin/metadata/aspect/validators/ConditionalWriteValidatorTest.javaLength of output: 198
Script:
#!/bin/bash # Description: Verify the usage of `reset` in the test file. # Test: Search for all occurrences of `reset` in the test file. rg --type java -A 5 'reset\(' entity-registry/src/test/java/com/linkedin/metadata/aspect/validators/ConditionalWriteValidatorTest.javaLength of output: 1229
Line range hint
242-272: LGTM! Verify correct handling of version lookup based on schema metadata.The changes align with the PR objective to handle aspect versions correctly.
Ensure the correct handling of version lookups based on schema metadata across test methods.
There was a problem hiding this comment.
Actionable comments posted: 2
Outside diff range, codebase verification and nitpick comments (1)
docs/api/openapi/openapi-usage-guide.md (1)
604-604: Consider shortening the phrase "in order to"The phrase "in order to" can be shortened to "to" for conciseness.
- the url parameter `systemMetadata=true` in order to view the current versions of the aspects. + the url parameter `systemMetadata=true` to view the current versions of the aspects.Tools
LanguageTool
[style] ~604-~604: Consider a shorter alternative to avoid wordiness.
Context: ...the url parametersystemMetadata=truein order to view the current versions of the aspec...(IN_ORDER_TO_PREMIUM)
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (8)
- docs/advanced/mcp-mcl.md (2 hunks)
- docs/api/openapi/openapi-usage-guide.md (1 hunks)
- entity-registry/src/main/java/com/linkedin/metadata/aspect/validation/ConditionalWriteValidator.java (3 hunks)
- entity-registry/src/test/java/com/linkedin/metadata/aspect/validators/ConditionalWriteValidatorTest.java (13 hunks)
- metadata-service/openapi-servlet/src/main/java/io/datahubproject/openapi/controller/GenericEntitiesController.java (5 hunks)
- metadata-service/openapi-servlet/src/main/java/io/datahubproject/openapi/v2/controller/EntityController.java (2 hunks)
- metadata-service/openapi-servlet/src/main/java/io/datahubproject/openapi/v3/controller/EntityController.java (4 hunks)
- metadata-service/openapi-servlet/src/test/java/io/datahubproject/openapi/v3/controller/EntityControllerTest.java (1 hunks)
Additional context used
LanguageTool
docs/api/openapi/openapi-usage-guide.md
[typographical] ~597-~597: The word “however” is an adverb that can’t be used like a conjunction, and therefore needs to be separated from the sentence.
Context: ...trieve a specific version of the aspects, however it defaults to the latest aspect versio...(HOWEVER_SENTENCE)
[style] ~604-~604: Consider a shorter alternative to avoid wordiness.
Context: ...the url parametersystemMetadata=truein order to view the current versions of the aspec...(IN_ORDER_TO_PREMIUM)
[uncategorized] ~692-~692: Possible missing comma found.
Context: ...n-id-provided" } } } ] ``` Next let's mutateglobalTagsfor the secon...(AI_HYDRA_LEO_MISSING_COMMA)
[grammar] ~693-~693: An apostrophe ‘s’ denotes possession. Did you mean to use the plural form of the noun (no apostrophe)?
Context: ...provided" } } } ] ``` Next let's mutateglobalTagsfor the second URN ...(NOUN_APOSTROPHE_S_VERB)
Additional comments not posted (40)
entity-registry/src/main/java/com/linkedin/metadata/aspect/validation/ConditionalWriteValidator.java (3)
45-45: Renamed constant and updated value.The constant
DEFAULT_ASPECT_VERSIONhas been renamed toUNVERSIONED_ASPECT_VERSIONwith a value of "-1". This change is appropriate for representing unversioned aspects.
133-133: Updated logic for versioning.The updated logic correctly uses the new constant
UNVERSIONED_ASPECT_VERSIONforCREATEandCREATE_ENTITYchange types. This ensures that unversioned aspects are handled appropriately.Also applies to: 146-146
Line range hint
186-188:
Added documentation note for-1version.The note clearly explains that the version is
-1for non-existing aspects, which can be used to create an aspect only if it doesn't exist. This is a helpful addition.docs/advanced/mcp-mcl.md (2)
186-188: Added documentation note for-1version.The note clearly explains that the version is
-1for non-existing aspects, which can be used to create an aspect only if it doesn't exist. This is a helpful addition.
201-208: Added section on change types for conditional writes.The new section clearly explains the
CREATEandCREATE_ENTITYchange types and their usage for conditional writes. This is a valuable addition to the documentation.metadata-service/openapi-servlet/src/test/java/io/datahubproject/openapi/v3/controller/EntityControllerTest.java (2)
75-168: Added new test cases for searching and sorting entities.The new test cases for the
EntityControllerclass thoroughly cover the intended functionality of searching and sorting entities. Mocking services and testing the order of search results based on URN values are well-implemented.
170-204: Added test configuration forEntityControllerTest.The new test configuration class
EntityControllerTestConfigis correctly set up to support the test cases. Mocking the necessary beans and providing required configurations are appropriately handled.metadata-service/openapi-servlet/src/main/java/io/datahubproject/openapi/v2/controller/EntityController.java (2)
191-191: Updated variable to useLinkedHashMap.The variable
urnAspectVersionsis now declared asLinkedHashMap. This change is appropriate for maintaining insertion order and is correctly implemented.
196-196: Updated method call with additional argument.The method call to
resolveAspectNamesnow includes an additional argument0L. This change is correctly implemented and ensures compatibility with the updated method signature.metadata-service/openapi-servlet/src/main/java/io/datahubproject/openapi/v3/controller/EntityController.java (6)
47-47: Import LinkedHashMapThe import statement for
LinkedHashMapis added. This is necessary for the changes made in the code to useLinkedHashMapinstead ofMap.
84-84: Use LinkedHashMap for requestMapThe
requestMapvariable is now aLinkedHashMapinstead of aMap. This change ensures that the order of insertion is preserved, which can be crucial for certain operations.
129-129: Use LinkedHashMap for urnAspectVersionsThe
urnAspectVersionsparameter in thebuildEntityVersionedAspectListmethod is now aLinkedHashMap. This ensures that the order of URNs and their associated aspect versions is maintained.
137-137: Pass default value to resolveAspectNamesThe method
resolveAspectNamesnow includes an additional argument0Lto set a default value for aspect versions. This ensures that if an aspect version is not specified, it defaults to0L.
202-202: Use LinkedHashMap for toEntityVersionRequestThe
toEntityVersionRequestmethod now returns aLinkedHashMap. This change ensures that the order of URNs and their associated aspect versions is maintained when converting the entity version request.
206-206: Use LinkedHashMap for items in toEntityVersionRequestThe
itemsvariable is now aLinkedHashMap. This ensures that the order of insertion is preserved when adding URNs and their associated aspect versions.metadata-service/openapi-servlet/src/main/java/io/datahubproject/openapi/controller/GenericEntitiesController.java (5)
121-121: Use LinkedHashMap for versionMapThe
versionMapvariable is now aLinkedHashMapinstead of aMap. This change ensures that the order of insertion is preserved, which can be crucial for certain operations.
131-138: Collect to LinkedHashMapThe
collectoperation now collects the entries into aLinkedHashMap. This ensures that the order of insertion is preserved when creating theversionMap.
145-145: Use LinkedHashMap for urnAspectVersionsThe
urnAspectVersionsparameter in thebuildEntityVersionedAspectListmethod is now aLinkedHashMap. This ensures that the order of URNs and their associated aspect versions is maintained.
655-656: Use LinkedHashMap in resolveAspectNamesThe
resolveAspectNamesmethod now uses aLinkedHashMapfor therequestedAspectNamesparameter. This ensures that the order of URNs and their associated aspect versions is maintained during aspect name resolution.
661-702: Collect to LinkedHashMap in resolveAspectNamesThe
collectoperation now collects the entries into aLinkedHashMap. This ensures that the order of insertion is preserved when creating the resolved aspect names map.docs/api/openapi/openapi-usage-guide.md (3)
591-592: Add reference to MetadataChangeProposal sectionA reference to the
MetadataChangeProposalsection has been added for the use of headers to support conditional writes semantics. This addition improves the documentation by providing a clear link to relevant information.
593-599: Document batch get endpointsNew documentation for batch get endpoints has been added. This includes details on fetching entity and aspects in bulk and using the
If-Version-Matchheader to retrieve specific aspect versions. The documentation is comprehensive and provides clear examples.Tools
LanguageTool
[typographical] ~597-~597: The word “however” is an adverb that can’t be used like a conjunction, and therefore needs to be separated from the sentence.
Context: ...trieve a specific version of the aspects, however it defaults to the latest aspect versio...(HOWEVER_SENTENCE)
693-693: Use plural form instead of possessiveThe phrase "globalTags's" should use the plural form "globalTags".
- Next let's mutate `globalTags` for the second URN by adding a new tag. + Next let's mutate `globalTags` for the second URN by adding a new tag.Likely invalid or redundant comment.
Tools
LanguageTool
[grammar] ~693-~693: An apostrophe ‘s’ denotes possession. Did you mean to use the plural form of the noun (no apostrophe)?
Context: ...provided" } } } ] ``` Next let's mutateglobalTagsfor the second URN ...(NOUN_APOSTROPHE_S_VERB)
entity-registry/src/test/java/com/linkedin/metadata/aspect/validators/ConditionalWriteValidatorTest.java (17)
6-6: Approved: ImportMockito.reset.The addition of
Mockito.resetis appropriate for resetting mocked objects in the tests.
93-93: Approved: Handling non-existent aspects with version-1.The update to include a header with
HTTP_HEADER_IF_VERSION_MATCHset to "-1" aligns with the PR objectives and ensures correct handling of non-existent aspects.
140-165: Approved: Resetting mocks and handling headers for different change types.The use of
resetensures proper mock state management, and the header updates align with the new versioning logic.
168-194: Approved: Handling non-existent aspects with version-1.The update to include a header with
HTTP_HEADER_IF_VERSION_MATCHset to "-1" aligns with the PR objectives and ensures correct handling of non-existent aspects.
Line range hint
242-272:
Approved: Resetting mocks and handling headers for non-existent aspects.The use of
resetensures proper mock state management, and the header updates align with the new versioning logic for non-existent aspects.
334-334: Approved: Handling non-existent aspects with version-1.The update to include a header with
HTTP_HEADER_IF_VERSION_MATCHset to "-1" aligns with the PR objectives and ensures correct handling of non-existent aspects.
371-386: Approved: Handling incorrect version expectations.The updates ensure that incorrect version expectations are properly tested and validated against the new versioning logic.
444-451: Approved: Handling incorrect version expectations.The updates ensure that incorrect version expectations are properly tested and validated against the new versioning logic.
506-506: Approved: Handling incorrect version expectations for non-existent aspects.The updates ensure that incorrect version expectations for non-existent aspects are properly tested and validated.
586-586: Approved: Handling incorrect version expectations for non-existent aspects.The updates ensure that incorrect version expectations for non-existent aspects are properly tested and validated.
665-665: Approved: Handling incorrect version expectations for non-existent aspects.The updates ensure that incorrect version expectations for non-existent aspects are properly tested and validated.
665-665: Approved: HandlingIF_UNMODIFIED_SINCEandIF_MODIFIED_SINCEheaders.The updates ensure that the
IF_UNMODIFIED_SINCEandIF_MODIFIED_SINCEheaders are properly tested and validated.
665-665: Approved: HandlingIF_UNMODIFIED_SINCEandIF_MODIFIED_SINCEheaders.The updates ensure that the
IF_UNMODIFIED_SINCEandIF_MODIFIED_SINCEheaders are properly tested and validated.
665-665: Approved: HandlingIF_UNMODIFIED_SINCEandIF_MODIFIED_SINCEheaders.The updates ensure that the
IF_UNMODIFIED_SINCEandIF_MODIFIED_SINCEheaders are properly tested and validated.
665-665: Approved: HandlingIF_UNMODIFIED_SINCEandIF_MODIFIED_SINCEheaders.The updates ensure that the
IF_UNMODIFIED_SINCEandIF_MODIFIED_SINCEheaders are properly tested and validated.
665-665: Approved: HandlingIF_UNMODIFIED_SINCEandIF_MODIFIED_SINCEheaders.The updates ensure that the
IF_UNMODIFIED_SINCEandIF_MODIFIED_SINCEheaders are properly tested and validated.
665-665: Approved: HandlingIF_UNMODIFIED_SINCEandIF_MODIFIED_SINCEheaders.The updates ensure that the
IF_UNMODIFIED_SINCEandIF_MODIFIED_SINCEheaders are properly tested and validated.
| Map.Entry::getKey, | ||
| Map.Entry::getValue, | ||
| (a, b) -> { | ||
| throw new IllegalStateException("Duplicate key"); |
There was a problem hiding this comment.
If it has the same value then it's likely okay, do we still want to throw an error for this case? Or can we put in a comment why the default map merger function is not appropriate?
There was a problem hiding this comment.
My understanding is that this is the default behavior of the map classes. Unfortunately the steaming interface doesn't allow deferring to the default behavior and forces me to re-implement the merge
| Map.Entry::getKey, | ||
| Map.Entry::getValue, | ||
| (a, b) -> { | ||
| throw new IllegalStateException("Duplicate key"); |
-1-1version)Checklist
Summary by CodeRabbit
Documentation
Refactor
LinkedHashMapfor better order preservation.Tests