feat(system): support conditional write semantics #10868

david-leifker · 2024-07-08T23:10:37Z

Features:

Introduce If-Modified-Since, If-Unmodified-Since, If-Version-Match conditional writes
ConditionalWriteValidator added
MCP headers field added
SystemMetadata version field added
batchGet with If-Version-Match support OpenApi V3

Breaking Change (OpenAPI v3):

Example Global Tags Aspect:

{
  "tags": [
    {
      "tag": "string",
      "context": "string"
    }
  ]
}

New (optional fields systemMetadata and headers):

{
  "value": {
    "tags": [
      {
        "tag": "string",
        "context": "string"
      }
    ]
  },
  "systemMetadata": {},
  "headers": {}
}

batchGet OpenAPI v3

Notes:

Restli API and direct MCP producers should work out of the box by using MCP headers field, no API changes required.
GraphQL support is out of scope for this PR

Checklist

The PR conforms to DataHub's Contributing Guideline (particularly Commit Message Format)
Links to related issues (if applicable)
Tests for the changes have been added/updated (if applicable)
Docs related to the changes have been added/updated (if applicable). If a new feature has been added a Usage Guide has been added for the same.
For any breaking change/potential downtime/deprecation/big changes an entry has been made in Updating DataHub

Summary by CodeRabbit

New Features
- Introduced methods to handle system aspects, enhancing aspect versioning and metadata management.
- Added support for HTTP-like headers in metadata change proposals.
Refactor
- Replaced old methods with new streamlined versions for aspect retrieval and processing.
- Updated client interfaces to use new versions of request and response objects.
Bug Fixes
- Improved exception handling to include metric collection before throwing exceptions.
Documentation
- Updated field descriptions in metadata change proposal records for better clarity.

coderabbitai · 2024-07-08T23:10:42Z

Walkthrough

The updates introduce several key improvements: enhanced aspect version handling, refined system aspect management, streamlined exception handling, and enriched metadata changes. These changes add new methods for computing and retrieving aspects, update exception metrics, and improve validation and error reporting. Additionally, the changes encompass modifications to generic entity and aspect handling, updating method signatures and introducing new methods to support these features.

Changes

File(s)	Change Summary
`metadata-io/.../EntityServiceImpl.java`	Introduced `collectMetrics`, replaced `aspectDao` calls with `EntityUtils`, updated exception handling, refactored error messages
`metadata-io/.../EntityUtils.java`	Added imports, renamed variables, introduced new methods for aspect version calculations
`entity-registry/.../SystemAspect.java`	Added `getSystemMetadataVersion` method
`entity-registry/.../EnvelopedSystemAspect.java`	Introduced `EnvelopedSystemAspect` class with various methods
`entity-registry/.../MCPItem.java`	Updated `MCPItem` interface with new methods and change types
`entity-registry/.../MockAspectRetriever.java`	Added handling for `SystemAspect` and `TestSystemAspect`, introduced `getLatestSystemAspects` method
`metadata-models/.../MetadataChangeProposal.pdl`	Updated `systemMetadata` field description, added `headers` field
`metadata-service/.../OpenApiClient.java`	Updated method signatures and response mapping with `BatchGetUrnRequestV2` and `BatchGetUrnResponseV2`
`metadata-service/.../GenericAspect.java`	Introduced `GenericAspect` interface
`metadata-service/.../GenericEntity.java`	Modified `GenericEntity` interface to include a generic type parameter `T`
`metadata-service/.../GenericEntityScrollResult.java`	Included a type parameter `A` that extends `GenericAspect` in `GenericEntityScrollResult`
`metadata-service/.../BatchGetUrnResponseV2.java` & `GenericAspectV2.java`	Renamed `BatchGetUrnResponse` to `BatchGetUrnResponseV2` with type parameters, introduced `GenericAspectV2` class
`metadata-service/.../GenericEntitiesController.java`	Updated class declaration and method signatures to include `GenericAspect`, added method for handling versioned aspects
`metadata-io/.../DefaultAspectsUtil.java`	Added imports, created and modified `SystemMetadata` objects, updated `MetadataChangeProposal` fields
`metadata-io/.../EntityClientAspectRetriever.java` & `EntityServiceAspectRetriever.java`	Added `getLatestSystemAspects` method, updated method signatures
`metadata-io/.../StructuredPropertiesValidatorTest.java`	Replaced usage of `getExceptionKey().getKey()` with `getAspectGroup().getKey()` in test methods
`metadata-models/.../SystemMetadata.pdl`	Added optional `version` field to `SystemMetadata`
`metadata-utils/.../GenericRecordUtils.java`	Extended `entityResponseToAspectMap` method, added `entityResponseToSystemAspectMap` method

Poem

In the land where data flows,
New methods bloom and metrics grow.
Aspects dance in versioned glee,
Ensuring truth for all to see.
Changes weave a seamless thread,
In DataHub, where knowledge's spread.

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?

Share

Tips

Chat

There are 3 ways to chat with CodeRabbit:

Review comments: Directly reply to a review comment made by CodeRabbit. Example:
- I pushed a fix in commit <commit_id>.
- Generate unit testing code for this file.
- Open a follow-up GitHub issue for this discussion.
Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
- @coderabbitai generate unit testing code for this file.
- @coderabbitai modularize this function.
PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
- @coderabbitai generate interesting stats about this repository and render them as a table.
- @coderabbitai show all the console.log statements in this repository.
- @coderabbitai read src/utils.ts and generate unit testing code.
- @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.
- @coderabbitai help me debug CodeRabbit configuration file.

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)

@coderabbitai pause to pause the reviews on a PR.
@coderabbitai resume to resume the paused reviews.
@coderabbitai review to trigger an incremental review. This is useful when automatic reviews are disabled for the repository.
@coderabbitai full review to do a full review from scratch and review all the files again.
@coderabbitai summary to regenerate the summary of the PR.
@coderabbitai resolve resolve all the CodeRabbit review comments.
@coderabbitai configuration to show the current CodeRabbit configuration for the repository.
@coderabbitai help to get help.

Additionally, you can add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.

CodeRabbit Configration File (`.coderabbit.yaml`)

You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
Please see the configuration documentation for more information.
If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.json

Documentation and Community

Visit our Documentation for detailed information on how to use CodeRabbit.
Join our Discord Community to get help, request features, and share feedback.
Follow us on X/Twitter for updates and announcements.

...egistry/src/main/java/com/linkedin/metadata/aspect/validation/ConditionalWriteValidator.java

kevin1chun · 2024-07-08T23:20:20Z

...egistry/src/main/java/com/linkedin/metadata/aspect/validation/ConditionalWriteValidator.java

+      }
+
+      // Validate modified time conditional
+      if (hasTimePrecondition(item)) {


It is possible for a client to pass both a timestamp and version header, and this code will apply both checks.
Should we just prefer one over the other rather than applying both?

From the way I read the http specification, both headers are allowed at the same time. It actually allows you to specify a time range when using both.

While not implemented here, these headers can be used to time travel on a GET request for example. Give me objects that were modified before/after or within the given time range specified by the headers.

That's actually an interesting use-case

@david-leifker if i specify some time range, will it give me all versions that were created in that time range? or just the latest in that time range?

Per the specification, it would return versions in that range.

interesting! @ajoymajumdar this would be able to replace our queries against audit
Assuming the version retention in the OLTP suffices

...a/openlineage-converter/src/test/java/io/datahubproject/openlineage/HdfsPathDatasetTest.java

coderabbitai

Actionable comments posted: 8

Outside diff range and nitpick comments (4)

docs/advanced/mcp-mcl.md (2)
174-174: Fix noun/verb agreement.

There seems to be a noun/verb agreement error. Did you mean “writes” or “wrote”?
- Conditional write semantics use extra information contained in the MCP `headers` field to possibly avoid writing new aspects
+ Conditional writes semantics use extra information contained in the MCP `headers` field to possibly avoid writing new aspects
Tools

LanguageTool

[grammar] ~174-~174: There seems to be a noun/verb agreement error. Did you mean “writes” or “wrote”?
Context: ...es ### Conditional Writes Conditional write semantics use extra information contain...

(SINGULAR_NOUN_VERB_AGREEMENT)

188-188: Hyphenate 'version based'.

This expression is usually spelled with a hyphen.
- Similar to version based conditional writes
+ Similar to version-based conditional writes
Tools

LanguageTool

[uncategorized] ~188-~188: This expression is usually spelled with a hyphen.
Context: ...using http header semantics. Similar to version based conditional writes this method can be u...

(BASED_HYPHEN)
docs/api/openapi/openapi-usage-guide.md (1)
122-122: Add a comma for clarity.

A comma is needed to improve readability.
- In this example we've added a URL parameter `createEntityIfNotExists=true`
+ In this example, we've added a URL parameter `createEntityIfNotExists=true`
Tools

LanguageTool

[typographical] ~122-~122: It appears that a comma is missing.
Context: ...ead of overwriting the entity. In this example we've added a URL parameter `createEnti...

(DURING_THAT_TIME_COMMA)
docs/how/updating-datahub.md (1)
25-25: Simplify the phrase "with respect to".

Consider using a shorter alternative for "with respect to" to improve readability.
-  and the API is now symmetric with respect to input and outputs.
+  and the API is now symmetric for inputs and outputs.
Tools

LanguageTool

[style] ~25-~25: ‘with respect to’ might be wordy. Consider a shorter alternative.
Context: ...value` key and the API is now symmetric with respect to input and outputs. Example Global Tags...

(EN_WORDINESS_PREMIUM_WITH_RESPECT_TO)

Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

Commits

Files that changed from the base of the PR and between 609847f and c7b4002.

Files selected for processing (53)

docs/advanced/mcp-mcl.md (4 hunks)
docs/api/openapi/openapi-usage-guide.md (2 hunks)
docs/how/updating-datahub.md (1 hunks)
entity-registry/src/main/java/com/linkedin/metadata/aspect/AspectRetriever.java (2 hunks)
entity-registry/src/main/java/com/linkedin/metadata/aspect/EnvelopedSystemAspect.java (1 hunks)
entity-registry/src/main/java/com/linkedin/metadata/aspect/SystemAspect.java (2 hunks)
entity-registry/src/main/java/com/linkedin/metadata/aspect/batch/MCPItem.java (1 hunks)
entity-registry/src/main/java/com/linkedin/metadata/aspect/plugins/validation/AspectValidationException.java (1 hunks)
entity-registry/src/main/java/com/linkedin/metadata/aspect/plugins/validation/ValidationExceptionCollection.java (1 hunks)
entity-registry/src/main/java/com/linkedin/metadata/aspect/validation/ConditionalWriteValidator.java (1 hunks)
entity-registry/src/test/java/com/linkedin/metadata/aspect/validators/ConditionalWriteValidatorTest.java (1 hunks)
entity-registry/src/testFixtures/java/com/linkedin/test/metadata/aspect/MockAspectRetriever.java (4 hunks)
entity-registry/src/testFixtures/java/com/linkedin/test/metadata/aspect/batch/TestMCP.java (2 hunks)
entity-registry/src/testFixtures/java/com/linkedin/test/metadata/aspect/batch/TestSystemAspect.java (1 hunks)
metadata-integration/java/datahub-client/src/main/resources/MetadataChangeProposal.avsc (1 hunks)
metadata-integration/java/datahub-event/src/main/resources/MetadataChangeProposal.avsc (1 hunks)
metadata-integration/java/openlineage-converter/build.gradle (1 hunks)
metadata-integration/java/openlineage-converter/src/test/java/io/datahubproject/openlineage/HdfsPathDatasetTest.java (8 hunks)
metadata-io/metadata-io-api/src/main/java/com/linkedin/metadata/entity/ebean/batch/ChangeItemImpl.java (7 hunks)
metadata-io/src/main/java/com/linkedin/metadata/aspect/utils/DefaultAspectsUtil.java (3 hunks)
metadata-io/src/main/java/com/linkedin/metadata/client/EntityClientAspectRetriever.java (2 hunks)
metadata-io/src/main/java/com/linkedin/metadata/entity/EntityServiceAspectRetriever.java (2 hunks)
metadata-io/src/main/java/com/linkedin/metadata/entity/EntityServiceImpl.java (6 hunks)
metadata-io/src/main/java/com/linkedin/metadata/entity/EntityUtils.java (4 hunks)
metadata-io/src/test/java/com/linkedin/metadata/AspectGenerationUtils.java (3 hunks)
metadata-io/src/test/java/com/linkedin/metadata/entity/EntityServiceTest.java (14 hunks)
metadata-io/src/test/java/com/linkedin/metadata/structuredproperties/validators/StructuredPropertiesValidatorTest.java (2 hunks)
metadata-models-custom/src/main/java/com/linkedin/metadata/aspect/plugins/spring/validation/CustomDataQualityRulesValidator.java (2 hunks)
metadata-models-custom/src/main/java/com/linkedin/metadata/aspect/plugins/validation/CustomDataQualityRulesValidator.java (2 hunks)
metadata-models/src/main/pegasus/com/linkedin/mxe/MetadataChangeProposal.pdl (1 hunks)
metadata-models/src/main/pegasus/com/linkedin/mxe/SystemMetadata.pdl (1 hunks)
metadata-models/src/main/resources/entity-registry.yml (1 hunks)
metadata-operation-context/src/main/java/io/datahubproject/test/metadata/context/TestOperationContexts.java (2 hunks)
metadata-service/openapi-servlet/models/src/main/java/io/datahubproject/openapi/models/GenericAspect.java (1 hunks)
metadata-service/openapi-servlet/models/src/main/java/io/datahubproject/openapi/models/GenericEntity.java (1 hunks)
metadata-service/openapi-servlet/models/src/main/java/io/datahubproject/openapi/models/GenericEntityScrollResult.java (1 hunks)
metadata-service/openapi-servlet/models/src/main/java/io/datahubproject/openapi/v2/models/GenericAspectV2.java (1 hunks)
metadata-service/openapi-servlet/models/src/main/java/io/datahubproject/openapi/v2/models/GenericEntityScrollResultV2.java (1 hunks)
metadata-service/openapi-servlet/models/src/main/java/io/datahubproject/openapi/v2/models/GenericEntityV2.java (2 hunks)
metadata-service/openapi-servlet/models/src/main/java/io/datahubproject/openapi/v3/models/GenericAspectV3.java (1 hunks)
metadata-service/openapi-servlet/models/src/main/java/io/datahubproject/openapi/v3/models/GenericEntityScrollResultV3.java (1 hunks)
metadata-service/openapi-servlet/models/src/main/java/io/datahubproject/openapi/v3/models/GenericEntityV3.java (3 hunks)
metadata-service/openapi-servlet/src/main/java/io/datahubproject/openapi/controller/GenericEntitiesController.java (3 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/OpenAPIV3Generator.java (1 hunks)
metadata-service/openapi-servlet/src/main/java/io/datahubproject/openapi/v3/controller/EntityController.java (4 hunks)
metadata-service/restli-api/src/main/snapshot/com.linkedin.entity.aspects.snapshot.json (2 hunks)
metadata-service/restli-api/src/main/snapshot/com.linkedin.entity.entities.snapshot.json (1 hunks)
metadata-service/restli-api/src/main/snapshot/com.linkedin.entity.entitiesV2.snapshot.json (1 hunks)
metadata-service/restli-api/src/main/snapshot/com.linkedin.entity.entitiesVersionedV2.snapshot.json (1 hunks)
metadata-service/restli-client-api/src/main/java/com/linkedin/entity/client/EntityClient.java (3 hunks)
metadata-utils/src/main/java/com/linkedin/metadata/utils/GenericRecordUtils.java (2 hunks)
metadata-utils/src/main/java/com/linkedin/metadata/utils/metrics/ExceptionUtils.java (1 hunks)

Files skipped from review due to trivial changes (3)

metadata-models/src/main/pegasus/com/linkedin/mxe/MetadataChangeProposal.pdl
metadata-models/src/main/pegasus/com/linkedin/mxe/SystemMetadata.pdl
metadata-service/openapi-servlet/models/src/main/java/io/datahubproject/openapi/models/GenericAspect.java

Additional context used

LanguageTool

docs/advanced/mcp-mcl.md

[grammar] ~174-~174: There seems to be a noun/verb agreement error. Did you mean “writes” or “wrote”?
Context: ...es ### Conditional Writes Conditional write semantics use extra information contain...

(SINGULAR_NOUN_VERB_AGREEMENT)

[uncategorized] ~179-~179: Possible missing comma found.
Context: ...f-Version-Match Each time an aspect is updated a version is incremented to represent...

(AI_HYDRA_LEO_MISSING_COMMA)

[grammar] ~183-~183: The verb ‘write’ does not usually follow articles like ‘the’. Check that ‘write’ is spelled correctly; using ‘write’ as a noun may be non-standard.
Context: ...ctual version stored in the database, the write will fail. This prevents overwriting an...

(A_INFINITIVE)

[uncategorized] ~188-~188: This expression is usually spelled with a hyphen.
Context: ...using http header semantics. Similar to version based conditional writes this method can be u...

(BASED_HYPHEN)

[grammar] ~189-~189: The verb ‘write’ does not usually follow articles like ‘the’. Check that ‘write’ is spelled correctly; using ‘write’ as a noun may be non-standard.
Context: ...ites this method can be used to prevent the write if the target aspect was modified after...

(A_INFINITIVE)

[uncategorized] ~190-~190: Possible missing comma found.
Context: ...fter a reading the aspect. Per the http specification dates must comply with ISO-8601 standar...

(AI_HYDRA_LEO_MISSING_COMMA)

[uncategorized] ~192-~192: Loose punctuation mark.
Context: ...SO-8601 standard. If-Unmodified-Since: A writer can specify that the aspect mu...

(UNLIKELY_OPENING_PUNCTUATION)

docs/api/openapi/openapi-usage-guide.md

[typographical] ~119-~119: It seems that a comma is missing.
Context: ...ntity doesn't exist. If the entity does exist the command will return an error instea...

(IF_COMMA)

[typographical] ~122-~122: It appears that a comma is missing.
Context: ...ead of overwriting the entity. In this example we've added a URL parameter `createEnti...

(DURING_THAT_TIME_COMMA)

docs/how/updating-datahub.md

[style] ~25-~25: ‘with respect to’ might be wordy. Consider a shorter alternative.
Context: ...value` key and the API is now symmetric with respect to input and outputs. Example Global Tags...

(EN_WORDINESS_PREMIUM_WITH_RESPECT_TO)

Additional comments not posted (122)

metadata-service/openapi-servlet/models/src/main/java/io/datahubproject/openapi/models/GenericEntity.java (1)

5-6: Enhancement: Introduced Generic Type Parameter for Flexibility

The addition of the generic type parameter T to the GenericEntity interface and the updated return type of getAspects improve type safety and flexibility. This allows for more specific type handling in implementing classes.

metadata-service/openapi-servlet/models/src/main/java/io/datahubproject/openapi/models/GenericEntityScrollResult.java (1)

3-3: Enhancement: Added Type Parameters for Improved Type Specificity

The addition of type parameters A and T to the GenericEntityScrollResult interface enhances type specificity and safety. This change allows for more precise type handling in implementing classes.

metadata-service/openapi-servlet/models/src/main/java/io/datahubproject/openapi/v2/models/GenericEntityScrollResultV2.java (1)

10-11: Enhancement: Implemented Generic Interface with Specific Types

The GenericEntityScrollResultV2 class now implements the GenericEntityScrollResult interface with specific types GenericAspectV2 and GenericEntityV2. This improves type safety and specificity in handling scroll results.

metadata-service/openapi-servlet/models/src/main/java/io/datahubproject/openapi/v3/models/GenericEntityScrollResultV3.java (1)

10-11: Enhancement: Implemented Generic Interface with Specific Types

The GenericEntityScrollResultV3 class now implements the GenericEntityScrollResult interface with specific types GenericAspectV3 and GenericEntityV3. This improves type safety and specificity in handling scroll results.

metadata-service/openapi-servlet/models/src/main/java/io/datahubproject/openapi/v3/models/GenericAspectV3.java (2)

13-17: Annotations for immutability and serialization look good.

The annotations ensure the class is immutable and only non-null fields are included in JSON serialization.

18-21: Class definition and fields are correct.

The class implements GenericAspect and includes non-null value, nullable systemMetadata, and headers fields, which align with the PR objectives.

metadata-service/openapi-servlet/models/src/main/java/io/datahubproject/openapi/v2/models/GenericAspectV2.java (3)

9-9: Class definition and inheritance are correct.

The class extends LinkedHashMap and implements GenericAspect, which provides the necessary structure for aspect data.

11-13: Constructor looks good.

The constructor initializes the LinkedHashMap with the provided map.

15-19: getValue method is correct.

The method returns the LinkedHashMap instance itself as the value.

entity-registry/src/testFixtures/java/com/linkedin/test/metadata/aspect/batch/TestSystemAspect.java (2)

14-17: Annotations for immutability and equality look good.

The annotations ensure the class is immutable and provides proper equality checks.

18-26: Class definition and fields are correct.

The class implements SystemAspect and includes fields for urn, version, recordTemplate, systemMetadata, entitySpec, aspectSpec, createdOn, and createdBy, which align with the PR objectives.

entity-registry/src/main/java/com/linkedin/metadata/aspect/SystemAspect.java (2)

Line range hint 1-1:
Interface declaration is correct.

The interface SystemAspect extends ReadItem and includes methods for version, creation timestamp, and audit stamp.

28-38: New method getSystemMetadataVersion is correct.

The method correctly retrieves the version from the system metadata if it exists.

metadata-integration/java/openlineage-converter/build.gradle (1)

27-27: Approve the addition of the testng dependency.

The addition of the testng dependency is approved.

However, ensure that the use of testng is consistent with other test configurations in the project.

metadata-utils/src/main/java/com/linkedin/metadata/utils/metrics/ExceptionUtils.java (1)

11-42: Approve the collectMetrics method.

The collectMetrics method is well-structured and correctly uses MetricUtils to increment various counters based on exception details.

However, ensure that MetricUtils is correctly configured and used in the project.

entity-registry/src/main/java/com/linkedin/metadata/aspect/EnvelopedSystemAspect.java (1)

1-62: Approve the EnvelopedSystemAspect class.

The EnvelopedSystemAspect class is well-structured and provides correct implementations for all required methods.

entity-registry/src/main/java/com/linkedin/metadata/aspect/plugins/validation/AspectValidationException.java (1)

1-70: Approve the AspectValidationException class.

The AspectValidationException class is well-structured and provides correct implementations for creating exceptions with different subtypes and messages.

entity-registry/src/main/java/com/linkedin/metadata/aspect/AspectRetriever.java (2)

35-41: LGTM!

The method getLatestSystemAspect correctly retrieves the latest system aspect for a given URN and aspect name.

49-50: LGTM!

The method getLatestSystemAspects correctly retrieves the latest system aspects for a set of URNs and aspect names.

entity-registry/src/main/java/com/linkedin/metadata/aspect/plugins/validation/ValidationExceptionCollection.java (1)

23-23: LGTM!

The method addException uses computeIfAbsent to efficiently add exceptions to the collection.

entity-registry/src/main/java/com/linkedin/metadata/aspect/batch/MCPItem.java (3)

25-31: LGTM!

The method getHeaders correctly retrieves headers from the metadata change proposal, handling null values appropriately.

33-35: LGTM!

The method hasHeader correctly checks for the presence of a specific header.

37-42: LGTM!

The method getHeader correctly retrieves the value of a specific header.

metadata-service/openapi-servlet/models/src/main/java/io/datahubproject/openapi/v2/models/GenericEntityV2.java (2)

27-34: LGTM! The class correctly implements the interface.

The class GenericEntityV2 now implements GenericEntity<GenericAspectV2>, which aligns with the intended functionality.

Line range hint 40-62:
LGTM! The build method correctly processes the aspects.

The build method in GenericEntityV2Builder now correctly processes GenericAspectV2 and handles potential exceptions.

metadata-io/src/main/java/com/linkedin/metadata/entity/EntityServiceAspectRetriever.java (2)

4-4: LGTM! The new import is necessary and used correctly.

The import entityResponseToSystemAspectMap is necessary for the new method getLatestSystemAspects.

56-80: LGTM! The method correctly retrieves the latest system aspects.

The method getLatestSystemAspects correctly retrieves the latest system aspects for given URNs and aspect names and handles potential exceptions.

metadata-service/openapi-servlet/models/src/main/java/io/datahubproject/openapi/v3/models/GenericEntityV3.java (2)

28-39: LGTM! The class correctly implements the interface.

The class GenericEntityV3 now implements GenericEntity<GenericAspectV3>, which aligns with the intended functionality.

48-70: LGTM! The build method correctly processes the aspects.

The build method in GenericEntityV3Builder now correctly processes GenericAspectV3 and handles potential exceptions.

metadata-io/src/test/java/com/linkedin/metadata/AspectGenerationUtils.java (2)

34-38: LGTM! The method correctly handles the new version field.

The createSystemMetadata method now correctly handles the new version field.

47-55: LGTM! The method correctly handles the new version field.

The createSystemMetadata method now correctly handles the new version field.

metadata-models-custom/src/main/java/com/linkedin/metadata/aspect/plugins/validation/CustomDataQualityRulesValidator.java (2)

29-29: Validation ensures at least one rule is required.

The change correctly enforces that DataQualityRules must contain at least one rule, which is essential for validation.

58-59: Validation ensures field type consistency.

The change correctly validates that the field type remains consistent between the old and new DataQualityRules, which is essential for data integrity.

entity-registry/src/testFixtures/java/com/linkedin/test/metadata/aspect/MockAspectRetriever.java (3)

9-13: New imports for SystemAspect and SystemMetadata.

The new imports are necessary for the added functionality related to SystemAspect and SystemMetadata.

48-61: Initialization of systemData with SystemAspect.

The change ensures that systemData is properly initialized with SystemAspect objects, which is necessary for the new functionality.

84-92: New method getLatestSystemAspects.

The method correctly retrieves the latest system aspects based on the provided urnAspectNames. This is necessary for the new functionality.

metadata-models-custom/src/main/java/com/linkedin/metadata/aspect/plugins/spring/validation/CustomDataQualityRulesValidator.java (2)

52-52: Validation ensures at least one rule is required.

The change correctly enforces that DataQualityRules must contain at least one rule, which is essential for validation.

81-82: Validation ensures field type consistency.

The change correctly validates that the field type remains consistent between the old and new DataQualityRules, which is essential for data integrity.

metadata-utils/src/main/java/com/linkedin/metadata/utils/GenericRecordUtils.java (1)

94-115: New method entityResponseToSystemAspectMap.

The method correctly converts EntityResponse objects to maps of SystemAspect objects based on the provided EntityRegistry. This is necessary for the new functionality.

entity-registry/src/testFixtures/java/com/linkedin/test/metadata/aspect/batch/TestMCP.java (3)

23-24: Add import statements for Map and Optional.

The import statements for Map and Optional are necessary for the newly added headers functionality.

125-125: Addition of headers field.

The headers field has been added to the TestMCP class to store HTTP-like headers.

133-142: Implementation of getHeaders method.

The getHeaders method retrieves headers from the metadataChangeProposal if available; otherwise, it returns the default headers.

metadata-integration/java/datahub-client/src/main/resources/MetadataChangeProposal.avsc (3)

170-174: Addition of version field to SystemMetadata.

The version field has been added to store the aspect version. This change allows for more precise version tracking of aspects.

177-178: Documentation update for SystemMetadata.

The documentation for SystemMetadata has been updated to reflect the addition of the version field.

180-185: Addition of headers field to MetadataChangeProposal.

The headers field has been added to support HTTP-like headers for metadata change proposals. This change enhances the flexibility of metadata proposals.

metadata-integration/java/datahub-event/src/main/resources/MetadataChangeProposal.avsc (3)

170-174: Addition of version field to SystemMetadata.

The version field has been added to store the aspect version. This change allows for more precise version tracking of aspects.

177-178: Documentation update for SystemMetadata.

The documentation for SystemMetadata has been updated to reflect the addition of the version field.

180-185: Addition of headers field to MetadataChangeProposal.

The headers field has been added to support HTTP-like headers for metadata change proposals. This change enhances the flexibility of metadata proposals.

metadata-service/restli-api/src/main/snapshot/com.linkedin.entity.entitiesV2.snapshot.json (1)

149-153: Addition of version field to SystemMetadata.

The version field has been added to store the aspect version. This change allows for more precise version tracking of aspects.

metadata-service/openapi-servlet/src/main/java/io/datahubproject/openapi/v3/controller/EntityController.java (2)

189-194: Ensure proper handling of null headers.

The code correctly converts the headers to a map, but ensure that it handles cases where headers are not present.

202-206: LGTM! Ensure the retriever context is correctly passed.

The addition of headers in the ChangeItemImpl builder looks good.

However, ensure that the retriever context is correctly passed to the build method.
docs/advanced/mcp-mcl.md (1)
192-192: Remove loose punctuation mark.

There is a loose punctuation mark before A writer can specify.
- `If-Unmodified-Since`:
+ `If-Unmodified-Since`
Likely invalid or redundant comment.

Tools

LanguageTool

[uncategorized] ~192-~192: Loose punctuation mark.
Context: ...SO-8601 standard. If-Unmodified-Since: A writer can specify that the aspect mu...

(UNLIKELY_OPENING_PUNCTUATION)
entity-registry/src/main/java/com/linkedin/metadata/aspect/validation/ConditionalWriteValidator.java (6)
96-103: Ensure proper handling of version precondition validation.

The code correctly validates the version precondition, but ensure that all edge cases are handled.

106-119: Ensure proper handling of time precondition validation.

The code correctly validates the time precondition, but ensure that all edge cases are handled.

150-158: LGTM! Ensure accurate version comparison.

The version comparison logic looks good.

However, ensure that the comparison handles all potential version formats accurately.

181-204: LGTM! Ensure accurate time comparison.

The time comparison logic looks good.

However, ensure that the comparison handles all potential time formats accurately.

50-53: Simplify the method by using a single return statement.

The method hasTimePrecondition can be simplified by using a single return statement.
- return item.getHeader(HttpHeaders.IF_MODIFIED_SINCE).isPresent()
-        || item.getHeader(HttpHeaders.IF_UNMODIFIED_SINCE).isPresent();
+ return item.getHeader(HttpHeaders.IF_MODIFIED_SINCE).isPresent() || 
+        item.getHeader(HttpHeaders.IF_UNMODIFIED_SINCE).isPresent();
Likely invalid or redundant comment.

56-57: Simplify the method by using a single return statement.

The method hasVersionPrecondition can be simplified by using a single return statement.
- return item.getHeader(HTTP_HEADER_IF_VERSION_MATCH).isPresent();
+ return item.getHeader(HTTP_HEADER_IF_VERSION_MATCH).isPresent();
Likely invalid or redundant comment.
metadata-service/openapi-servlet/src/main/java/io/datahubproject/openapi/v2/controller/EntityController.java (3)

37-37: Import new models for type-safe handling.

The new imports for GenericAspectV2, GenericEntityV2, and GenericEntityScrollResultV2 align with the updated type-safe handling for aspects and entities.

72-72: Type-safe handling for aspects and entities.

The class EntityController now uses type parameters for GenericAspectV2, GenericEntityV2, and GenericEntityScrollResultV2, enhancing type safety and extensibility.

75-82: Build scroll result with type-safe aspects and entities.

The method buildScrollResult now returns a GenericEntityScrollResultV2 object, ensuring type-safe handling of aspects and entities.

metadata-integration/java/openlineage-converter/src/test/java/io/datahubproject/openlineage/HdfsPathDatasetTest.java (8)

16-17: Add imports for testing framework.

The new imports for Assert and Test from org.testng are necessary for the test cases.

45-45: Set environment for path specifications.

The env field is set using Optional.of("PROD"), ensuring the environment is correctly specified for path specifications.

92-92: Set environment for path specifications.

The env field is set using Optional.of("PROD"), ensuring the environment is correctly specified for path specifications.

124-124: Set environment for path specifications.

The env field is set using Optional.of("PROD"), ensuring the environment is correctly specified for path specifications.

157-157: Set environment and platform instance for path specifications.

The env and platformInstance fields are set using Optional.of("PROD") and Optional.of("s3Instance"), respectively, ensuring the environment and platform instance are correctly specified for path specifications.

190-190: Set environment for path specifications.

The env field is set using Optional.of("PROD"), ensuring the environment is correctly specified for path specifications.

222-222: Set environment for path specifications.

The env field is set using Optional.of("PROD"), ensuring the environment is correctly specified for path specifications.

252-252: Set environment for path specifications.

The env field is set using Optional.of("PROD"), ensuring the environment is correctly specified for path specifications.

metadata-io/metadata-io-api/src/main/java/com/linkedin/metadata/entity/ebean/batch/ChangeItemImpl.java (10)

7-7: Import StringMap for headers.

The new import for StringMap is necessary for handling headers in ChangeItemImpl.

27-29: Add imports for handling optional and stream operations.

The new imports for Optional and Collectors are necessary for handling optional values and stream operations in ChangeItemImpl.

76-76: Add system metadata field.

The new field systemMetadata is necessary for handling system metadata in ChangeItemImpl.

88-89: Add headers field.

The new field headers is necessary for handling headers in ChangeItemImpl.

90-99: Update next aspect version and system metadata.

The method setNextAspectVersion updates the next aspect version and the system metadata accordingly.

103-124: Generate system aspect with updated system metadata.

The method getSystemAspect generates a SystemAspect object with the updated system metadata.

144-146: Include headers in MetadataChangeProposal.

The method getMetadataChangeProposal includes headers in the MetadataChangeProposal if they are present.

151-160: Get headers from MetadataChangeProposal or default headers.

The method getHeaders returns the headers from the MetadataChangeProposal if they are present, otherwise it returns the default headers.

179-183: Initialize headers if null.

The builder method initializes the headers to an empty map if they are null.

207-208: Include headers in ChangeItemImpl builder.

The builder includes the headers field in the ChangeItemImpl object.

metadata-operation-context/src/main/java/io/datahubproject/test/metadata/context/TestOperationContexts.java (1)

273-279: Add method to retrieve latest system aspects.

The method getLatestSystemAspects retrieves the latest system aspects for the given URNs and aspect names.

metadata-io/src/main/java/com/linkedin/metadata/entity/EntityUtils.java (4)

37-37: Imports look good.

The addition of Set and Stream imports is necessary for the new methods.

Also applies to: 39-39

164-167: Parameter renaming in toSystemAspect.

The parameter aspectRetriever has been renamed to retrieverContext. This improves clarity since retrieverContext is more descriptive.

293-335: New method calculateNextVersions added.

This method calculates the next versions of aspects using both precalculated and database values. The logic is sound and efficiently merges the two sources. Ensure that the aspectDao.getNextVersions method handles missing aspect versions correctly.

343-351: New method convertSystemAspectToNextVersionMap added.

This method extracts the next version from a map of system aspects. The logic is straightforward and efficient, using streams and filtering. Ensure that aspects with version 0 and system metadata version are correctly processed.

metadata-io/src/main/java/com/linkedin/metadata/aspect/utils/DefaultAspectsUtil.java (1)

353-360: Handling of SystemMetadata in getProposalFromAspectForDefault.

The added block ensures that the SystemMetadata is copied and its version is removed before setting it in the proposal. This is a good practice to avoid unintended versioning issues.

metadata-models/src/main/resources/entity-registry.yml (1)

622-633: Addition of ConditionalWriteValidator.

The ConditionalWriteValidator class is added with support for multiple operations. This is a significant addition for handling conditional writes. Ensure that the validator is correctly implemented and integrated.

metadata-service/restli-client-api/src/main/java/com/linkedin/entity/client/EntityClient.java (1)

607-615: New method getLatestSystemAspect added.

This method retrieves the latest system aspects for a set of URNs and aspect names. The logic is sound, and the method provides a useful utility for fetching system aspects. Ensure that the entityResponseToSystemAspectMap handles the conversion correctly.

metadata-io/src/test/java/com/linkedin/metadata/structuredproperties/validators/StructuredPropertiesValidatorTest.java (2)

392-392: LGTM! Assertion addition improves test coverage.

The added assertion correctly checks the aspect group key and the exception message, which enhances the test coverage for immutable property validation.

487-487: LGTM! Assertion addition improves test coverage.

The added assertion correctly checks the aspect group key and the exception message, which enhances the test coverage for immutable property deletion validation.

docs/api/openapi/openapi-usage-guide.md (1)

586-591: LGTM! Documentation for OpenAPI v3 features is clear.

The added section clearly explains the support for conditional writes in OpenAPI v3.

metadata-service/openapi-servlet/src/main/java/io/datahubproject/openapi/controller/GenericEntitiesController.java (2)

82-84: LGTM! Enhanced type safety and extensibility.

The added generic type parameters improve type safety and extensibility of the GenericEntitiesController class.

288-288: LGTM! Consistent update to method signature.

The updated method signature with generic type parameters ensures consistency with the class changes.

metadata-service/openapi-servlet/src/main/java/io/datahubproject/openapi/v3/OpenAPIV3Generator.java (1)

511-534: LGTM! Verify consistency with overall system design.

The changes to include systemMetadata and headers properties in the buildAspectRefRequestSchema method look good. Ensure that these changes are consistent with the overall system design and usage.

entity-registry/src/test/java/com/linkedin/metadata/aspect/validators/ConditionalWriteValidatorTest.java (14)

70-130: LGTM!

The testNextVersionSuccess method comprehensively tests the ConditionalWriteValidator for various ChangeType values, ensuring no exceptions are thrown when the expected and actual versions match.

132-174: LGTM!

The testNoSystemMetadataNextVersionNextVersionSuccess method effectively tests the ConditionalWriteValidator when there is no previous system metadata, ensuring fallback to the version field.

176-237: LGTM!

The testNoPreviousVersionsLookupSchemaMetadataNextVersionSuccess method is well-structured and effectively tests the ConditionalWriteValidator with a mock lookup based on version.

239-299: LGTM!

The testNoPreviousVersionsLookupVersionNextVersionSuccess method effectively tests the ConditionalWriteValidator with a mock lookup based on version, ensuring no exceptions are thrown when the expected and actual versions match.

301-379: LGTM!

The testNextVersionFail method comprehensively tests the ConditionalWriteValidator for various ChangeType values, ensuring exceptions are thrown when the expected and actual versions do not match.

381-440: LGTM!

The testNoSystemMetadataNextVersionNextVersionFail method effectively tests the ConditionalWriteValidator when there is no previous system metadata, ensuring exceptions are thrown when the expected and actual versions do not match.

442-523: LGTM!

The testNoPreviousVersionsLookupSchemaMetadataNextVersionFail method is well-structured and effectively tests the ConditionalWriteValidator with a mock lookup based on version, ensuring exceptions are thrown when the expected and actual versions do not match.

525-599: LGTM!

The testNoPreviousVersionsLookupVersionNextVersionFail method effectively tests the ConditionalWriteValidator with a mock lookup based on version, ensuring exceptions are thrown when the expected and actual versions do not match.

601-674: LGTM!

The testModifiedSinceSuccess method comprehensively tests the ConditionalWriteValidator for various ChangeType values, ensuring no exceptions are thrown when the modified timestamp falls within the specified range.

676-749: LGTM!

The testNoPreviousLookupAuditStampModifiedSinceSuccess method is well-structured and effectively tests the ConditionalWriteValidator with a mock lookup based on audit stamp, ensuring no exceptions are thrown when the modified timestamp falls within the specified range.

751-836: LGTM!

The testModifiedSinceBeforeRangeFail method comprehensively tests the ConditionalWriteValidator for various ChangeType values, ensuring exceptions are thrown when the modified timestamp is before the specified range.

839-923: LGTM!

The testModifiedSinceAfterRangeFail method comprehensively tests the ConditionalWriteValidator for various ChangeType values, ensuring exceptions are thrown when the modified timestamp is after the specified range.

925-1010: LGTM!

The testNoPreviousLookupAuditStampModifiedSinceBeforeRangeFail method is well-structured and effectively tests the ConditionalWriteValidator with a mock lookup based on audit stamp, ensuring exceptions are thrown when the modified timestamp is before the specified range.

1012-1097: LGTM!

The testNoPreviousLookupAuditStampModifiedSinceAfterRangeFail method is well-structured and effectively tests the ConditionalWriteValidator with a mock lookup based on audit stamp, ensuring exceptions are thrown when the modified timestamp is after the specified range.

metadata-io/src/test/java/com/linkedin/metadata/entity/EntityServiceTest.java (8)

Line range hint 211-213: Ensure correct handling of additional parameters in SystemMetadata.

The SystemMetadata instances now include additional parameters. Verify that the createSystemMetadata method correctly handles these parameters and that the test assertions cover these new fields.

523-523: Ensure correct handling of additional parameters in SystemMetadata.

The SystemMetadata instance now includes a version parameter. Verify that the createSystemMetadata method correctly handles this parameter and that the test assertions cover this new field.

606-606: Ensure correct handling of additional parameters in SystemMetadata.

The SystemMetadata instance now includes a version parameter. Verify that the createSystemMetadata method correctly handles this parameter and that the test assertions cover this new field.

620-620: Ensure correct handling of additional parameters in SystemMetadata.

The SystemMetadata instance now includes a version parameter. Verify that the createSystemMetadata method correctly handles this parameter and that the test assertions cover this new field.

668-668: Ensure correct handling of additional parameters in SystemMetadata.

The SystemMetadata instance now includes a version parameter. Verify that the createSystemMetadata method correctly handles this parameter and that the test assertions cover this new field.

697-697: Ensure correct handling of additional parameters in SystemMetadata.

The SystemMetadata instance now includes a version parameter. Verify that the createSystemMetadata method correctly handles this parameter and that the test assertions cover this new field.

1187-1190: Ensure correct handling of additional parameters in SystemMetadata.

The SystemMetadata instances now include additional parameters. Verify that the createSystemMetadata method correctly handles these parameters and that the test assertions cover these new fields.

1299-1302: Ensure correct handling of additional parameters in SystemMetadata.

The SystemMetadata instances now include additional parameters. Verify that the createSystemMetadata method correctly handles these parameters and that the test assertions cover these new fields.

metadata-io/src/main/java/com/linkedin/metadata/entity/EntityServiceImpl.java (3)

13-13: Approved: Import statement for collectMetrics.

The import statement for collectMetrics is necessary for the new exception handling logic.

783-785: Approved: Refactored version calculation logic.

The change to use EntityUtils.calculateNextVersions instead of aspectDao.getNextVersions makes the code more modular and maintainable.

802-806: Approved: Enhanced logic for merging aspects and calculating next versions.

The additional logic for merging aspects and calculating next versions ensures the correctness and robustness of the ingestion process.

metadata-service/restli-api/src/main/snapshot/com.linkedin.entity.aspects.snapshot.json (3)

104-108: Addition of version field in SystemMetadata.

The version field has been added to the SystemMetadata record. Ensure that this field is used consistently across the codebase and that it is properly documented in the relevant sections.

4060-4062: Addition of systemMetadata field in MetadataChangeProposal.

The systemMetadata field is already present and correctly documented.

4063-4068: Addition of headers field in MetadataChangeProposal.

The headers field has been added to the MetadataChangeProposal record. Ensure that this field is used consistently across the codebase and that it is properly documented in the relevant sections.

metadata-service/restli-api/src/main/snapshot/com.linkedin.entity.entities.snapshot.json (1)

6472-6476: Addition of the version field in SystemMetadata looks good.

The new version field is correctly defined as a string and marked as optional, with a clear documentation string. Ensure that its usage is consistent throughout the codebase.

metadata-io/src/main/java/com/linkedin/metadata/client/EntityClientAspectRetriever.java

...penapi-servlet/models/src/main/java/io/datahubproject/openapi/v2/models/GenericAspectV2.java

docs/advanced/mcp-mcl.md

coderabbitai

Actionable comments posted: 0

Outside diff range, codebase verification and nitpick comments (9)

docs/advanced/mcp-mcl.md (6)
120-120: Add a comma.

Add a comma after 'timestamp' in the sentence to improve readability.
- updated timestamp.  8. headers     Optional headers which are meant to mimic http headers.
+ updated timestamp.  8. headers,     Optional headers which are meant to mimic http headers.
Tools

LanguageTool

[uncategorized] ~120-~120: Possible missing comma found.
Context: ...ted timestamp. 8. headers Optional headers which are meant to mimic http headers. ...

(AI_HYDRA_LEO_MISSING_COMMA)

174-174: Fix noun/verb agreement.

There seems to be a noun/verb agreement error. Change "write" to "writes" to correct it.
- Conditional write semantics use extra information contained in the MCP `headers` field to possibly avoid writing new aspects
+ Conditional writes semantics use extra information contained in the MCP `headers` field to possibly avoid writing new aspects
Tools

LanguageTool

[grammar] ~174-~174: There seems to be a noun/verb agreement error. Did you mean “writes” or “wrote”?
Context: ...es ### Conditional Writes Conditional write semantics use extra information contain...

(SINGULAR_NOUN_VERB_AGREEMENT)

179-179: Add a comma.

Add a comma after 'updated' to improve readability.
- Each time an aspect is updated a `version` is incremented to represent the change to the aspect.
+ Each time an aspect is updated, a `version` is incremented to represent the change to the aspect.
Tools

LanguageTool

[uncategorized] ~179-~179: Possible missing comma found.
Context: ...f-Version-Match Each time an aspect is updated a version is incremented to represent...

(AI_HYDRA_LEO_MISSING_COMMA)

183-183: Fix noun usage.

The verb ‘write’ does not usually follow articles like ‘the’. Change "the write" to "writing" to correct it.
- match the actual `version` stored in the database, the write will fail.
+ match the actual `version` stored in the database, writing will fail.
Tools

LanguageTool

[grammar] ~183-~183: The verb ‘write’ does not usually follow articles like ‘the’. Check that ‘write’ is spelled correctly; using ‘write’ as a noun may be non-standard.
Context: ...ctual version stored in the database, the write will fail. This prevents overwriting an...

(A_INFINITIVE)

188-188: Add a hyphen.

Add a hyphen in "version-based" to correct the expression.
- Similar to version based conditional writes
+ Similar to version-based conditional writes
Tools

LanguageTool

[uncategorized] ~188-~188: This expression is usually spelled with a hyphen.
Context: ...using http header semantics. Similar to version based conditional writes this method can be u...

(BASED_HYPHEN)

189-189: Fix noun usage.

The verb ‘write’ does not usually follow articles like ‘the’. Change "the write" to "writing" to correct it.
- this method can be used to prevent the write if the target aspect was modified after
+ this method can be used to prevent writing if the target aspect was modified after
Tools

LanguageTool

[grammar] ~189-~189: The verb ‘write’ does not usually follow articles like ‘the’. Check that ‘write’ is spelled correctly; using ‘write’ as a noun may be non-standard.
Context: ...ites this method can be used to prevent the write if the target aspect was modified after...

(A_INFINITIVE)
docs/api/openapi/openapi-usage-guide.md (1)
122-122: Minor Improvement: Add a comma after "In this example".

Adding a comma will improve readability.
- In this example we've added a URL parameter `createEntityIfNotExists=true`
+ In this example, we've added a URL parameter `createEntityIfNotExists=true`
Tools

LanguageTool

[typographical] ~122-~122: It appears that a comma is missing.
Context: ...ead of overwriting the entity. In this example we've added a URL parameter `createEnti...

(DURING_THAT_TIME_COMMA)
docs/how/updating-datahub.md (1)
25-25: Reduce Wordiness

Consider replacing "with respect to" with a more concise phrase like "for" or "regarding" to improve clarity and readability.
-  behavior is required.
- #10868 - OpenAPI V3 - Creation of aspects will need to be wrapped within a `value` key and the API is now symmetric with respect to input and outputs.
+  behavior is required.
+ #10868 - OpenAPI V3 - Creation of aspects will need to be wrapped within a `value` key and the API is now symmetric for input and outputs.
Tools

LanguageTool

[style] ~25-~25: ‘with respect to’ might be wordy. Consider a shorter alternative.
Context: ...value` key and the API is now symmetric with respect to input and outputs. Example Global Tags...

(EN_WORDINESS_PREMIUM_WITH_RESPECT_TO)
metadata-service/openapi-servlet/src/main/java/io/datahubproject/openapi/v3/OpenAPIV3Generator.java (1)
44-44: Add Field Description

Consider adding a description for the NAME_VERSION constant to provide context.
private static final String NAME_VERSION = "version";  // Description: Version number for the aspect

Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

Commits

Files that changed from the base of the PR and between c7b4002 and a73f076.

Files selected for processing (57)

docs/advanced/mcp-mcl.md (4 hunks)
docs/api/openapi/openapi-usage-guide.md (2 hunks)
docs/how/updating-datahub.md (1 hunks)
entity-registry/src/main/java/com/linkedin/metadata/aspect/AspectRetriever.java (2 hunks)
entity-registry/src/main/java/com/linkedin/metadata/aspect/EnvelopedSystemAspect.java (1 hunks)
entity-registry/src/main/java/com/linkedin/metadata/aspect/SystemAspect.java (2 hunks)
entity-registry/src/main/java/com/linkedin/metadata/aspect/batch/MCPItem.java (1 hunks)
entity-registry/src/main/java/com/linkedin/metadata/aspect/plugins/validation/AspectValidationException.java (1 hunks)
entity-registry/src/main/java/com/linkedin/metadata/aspect/plugins/validation/ValidationExceptionCollection.java (1 hunks)
entity-registry/src/main/java/com/linkedin/metadata/aspect/validation/ConditionalWriteValidator.java (1 hunks)
entity-registry/src/test/java/com/linkedin/metadata/aspect/validators/ConditionalWriteValidatorTest.java (1 hunks)
entity-registry/src/testFixtures/java/com/linkedin/test/metadata/aspect/MockAspectRetriever.java (4 hunks)
entity-registry/src/testFixtures/java/com/linkedin/test/metadata/aspect/batch/TestMCP.java (2 hunks)
entity-registry/src/testFixtures/java/com/linkedin/test/metadata/aspect/batch/TestSystemAspect.java (1 hunks)
metadata-integration/java/datahub-client/src/main/resources/MetadataChangeProposal.avsc (1 hunks)
metadata-integration/java/datahub-event/src/main/resources/MetadataChangeProposal.avsc (1 hunks)
metadata-integration/java/openlineage-converter/build.gradle (1 hunks)
metadata-integration/java/openlineage-converter/src/test/java/io/datahubproject/openlineage/HdfsPathDatasetTest.java (8 hunks)
metadata-io/metadata-io-api/src/main/java/com/linkedin/metadata/entity/ebean/batch/ChangeItemImpl.java (7 hunks)
metadata-io/src/main/java/com/linkedin/metadata/aspect/utils/DefaultAspectsUtil.java (3 hunks)
metadata-io/src/main/java/com/linkedin/metadata/client/EntityClientAspectRetriever.java (2 hunks)
metadata-io/src/main/java/com/linkedin/metadata/entity/EntityServiceAspectRetriever.java (2 hunks)
metadata-io/src/main/java/com/linkedin/metadata/entity/EntityServiceImpl.java (8 hunks)
metadata-io/src/main/java/com/linkedin/metadata/entity/EntityUtils.java (4 hunks)
metadata-io/src/test/java/com/linkedin/metadata/AspectGenerationUtils.java (3 hunks)
metadata-io/src/test/java/com/linkedin/metadata/entity/EntityServiceTest.java (14 hunks)
metadata-io/src/test/java/com/linkedin/metadata/structuredproperties/validators/StructuredPropertiesValidatorTest.java (2 hunks)
metadata-models-custom/src/main/java/com/linkedin/metadata/aspect/plugins/spring/validation/CustomDataQualityRulesValidator.java (2 hunks)
metadata-models-custom/src/main/java/com/linkedin/metadata/aspect/plugins/validation/CustomDataQualityRulesValidator.java (2 hunks)
metadata-models/src/main/pegasus/com/linkedin/mxe/MetadataChangeProposal.pdl (1 hunks)
metadata-models/src/main/pegasus/com/linkedin/mxe/SystemMetadata.pdl (1 hunks)
metadata-models/src/main/resources/entity-registry.yml (1 hunks)
metadata-operation-context/src/main/java/io/datahubproject/test/metadata/context/TestOperationContexts.java (2 hunks)
metadata-service/openapi-servlet/models/src/main/java/io/datahubproject/openapi/client/OpenApiClient.java (4 hunks)
metadata-service/openapi-servlet/models/src/main/java/io/datahubproject/openapi/models/GenericAspect.java (1 hunks)
metadata-service/openapi-servlet/models/src/main/java/io/datahubproject/openapi/models/GenericEntity.java (1 hunks)
metadata-service/openapi-servlet/models/src/main/java/io/datahubproject/openapi/models/GenericEntityScrollResult.java (1 hunks)
metadata-service/openapi-servlet/models/src/main/java/io/datahubproject/openapi/v2/models/BatchGetUrnRequestV2.java (2 hunks)
metadata-service/openapi-servlet/models/src/main/java/io/datahubproject/openapi/v2/models/BatchGetUrnResponseV2.java (2 hunks)
metadata-service/openapi-servlet/models/src/main/java/io/datahubproject/openapi/v2/models/GenericAspectV2.java (1 hunks)
metadata-service/openapi-servlet/models/src/main/java/io/datahubproject/openapi/v2/models/GenericEntityScrollResultV2.java (1 hunks)
metadata-service/openapi-servlet/models/src/main/java/io/datahubproject/openapi/v2/models/GenericEntityV2.java (2 hunks)
metadata-service/openapi-servlet/models/src/main/java/io/datahubproject/openapi/v3/models/GenericAspectV3.java (1 hunks)
metadata-service/openapi-servlet/models/src/main/java/io/datahubproject/openapi/v3/models/GenericEntityScrollResultV3.java (1 hunks)
metadata-service/openapi-servlet/models/src/main/java/io/datahubproject/openapi/v3/models/GenericEntityV3.java (3 hunks)
metadata-service/openapi-servlet/src/main/java/io/datahubproject/openapi/controller/GenericEntitiesController.java (8 hunks)
metadata-service/openapi-servlet/src/main/java/io/datahubproject/openapi/v2/controller/EntityController.java (4 hunks)
metadata-service/openapi-servlet/src/main/java/io/datahubproject/openapi/v3/OpenAPIV3Generator.java (8 hunks)
metadata-service/openapi-servlet/src/main/java/io/datahubproject/openapi/v3/controller/EntityController.java (5 hunks)
metadata-service/restli-api/src/main/snapshot/com.linkedin.entity.aspects.snapshot.json (2 hunks)
metadata-service/restli-api/src/main/snapshot/com.linkedin.entity.entities.snapshot.json (1 hunks)
metadata-service/restli-api/src/main/snapshot/com.linkedin.entity.entitiesV2.snapshot.json (1 hunks)
metadata-service/restli-api/src/main/snapshot/com.linkedin.entity.entitiesVersionedV2.snapshot.json (1 hunks)
metadata-service/restli-client-api/src/main/java/com/linkedin/entity/client/EntityClient.java (3 hunks)
metadata-service/services/src/main/java/com/linkedin/metadata/entity/EntityService.java (1 hunks)
metadata-utils/src/main/java/com/linkedin/metadata/utils/GenericRecordUtils.java (2 hunks)
metadata-utils/src/main/java/com/linkedin/metadata/utils/metrics/ExceptionUtils.java (1 hunks)

Files skipped from review due to trivial changes (2)

metadata-integration/java/datahub-event/src/main/resources/MetadataChangeProposal.avsc
metadata-models/src/main/pegasus/com/linkedin/mxe/MetadataChangeProposal.pdl

Files skipped from review as they are similar to previous changes (41)

entity-registry/src/main/java/com/linkedin/metadata/aspect/AspectRetriever.java
entity-registry/src/main/java/com/linkedin/metadata/aspect/EnvelopedSystemAspect.java
entity-registry/src/main/java/com/linkedin/metadata/aspect/SystemAspect.java
entity-registry/src/main/java/com/linkedin/metadata/aspect/batch/MCPItem.java
entity-registry/src/main/java/com/linkedin/metadata/aspect/plugins/validation/AspectValidationException.java
entity-registry/src/main/java/com/linkedin/metadata/aspect/plugins/validation/ValidationExceptionCollection.java
entity-registry/src/main/java/com/linkedin/metadata/aspect/validation/ConditionalWriteValidator.java
entity-registry/src/test/java/com/linkedin/metadata/aspect/validators/ConditionalWriteValidatorTest.java
entity-registry/src/testFixtures/java/com/linkedin/test/metadata/aspect/MockAspectRetriever.java
entity-registry/src/testFixtures/java/com/linkedin/test/metadata/aspect/batch/TestMCP.java
metadata-integration/java/datahub-client/src/main/resources/MetadataChangeProposal.avsc
metadata-integration/java/openlineage-converter/build.gradle
metadata-integration/java/openlineage-converter/src/test/java/io/datahubproject/openlineage/HdfsPathDatasetTest.java
metadata-io/metadata-io-api/src/main/java/com/linkedin/metadata/entity/ebean/batch/ChangeItemImpl.java
metadata-io/src/main/java/com/linkedin/metadata/aspect/utils/DefaultAspectsUtil.java
metadata-io/src/main/java/com/linkedin/metadata/client/EntityClientAspectRetriever.java
metadata-io/src/main/java/com/linkedin/metadata/entity/EntityServiceAspectRetriever.java
metadata-io/src/main/java/com/linkedin/metadata/entity/EntityServiceImpl.java
metadata-io/src/main/java/com/linkedin/metadata/entity/EntityUtils.java
metadata-io/src/test/java/com/linkedin/metadata/entity/EntityServiceTest.java
metadata-models-custom/src/main/java/com/linkedin/metadata/aspect/plugins/spring/validation/CustomDataQualityRulesValidator.java
metadata-models-custom/src/main/java/com/linkedin/metadata/aspect/plugins/validation/CustomDataQualityRulesValidator.java
metadata-models/src/main/pegasus/com/linkedin/mxe/SystemMetadata.pdl
metadata-operation-context/src/main/java/io/datahubproject/test/metadata/context/TestOperationContexts.java
metadata-service/openapi-servlet/models/src/main/java/io/datahubproject/openapi/models/GenericAspect.java
metadata-service/openapi-servlet/models/src/main/java/io/datahubproject/openapi/models/GenericEntity.java
metadata-service/openapi-servlet/models/src/main/java/io/datahubproject/openapi/models/GenericEntityScrollResult.java
metadata-service/openapi-servlet/models/src/main/java/io/datahubproject/openapi/v2/models/GenericAspectV2.java
metadata-service/openapi-servlet/models/src/main/java/io/datahubproject/openapi/v2/models/GenericEntityV2.java
metadata-service/openapi-servlet/models/src/main/java/io/datahubproject/openapi/v3/models/GenericAspectV3.java
metadata-service/openapi-servlet/models/src/main/java/io/datahubproject/openapi/v3/models/GenericEntityScrollResultV3.java
metadata-service/openapi-servlet/src/main/java/io/datahubproject/openapi/controller/GenericEntitiesController.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/v3/controller/EntityController.java
metadata-service/restli-api/src/main/snapshot/com.linkedin.entity.aspects.snapshot.json
metadata-service/restli-api/src/main/snapshot/com.linkedin.entity.entities.snapshot.json
metadata-service/restli-api/src/main/snapshot/com.linkedin.entity.entitiesV2.snapshot.json
metadata-service/restli-api/src/main/snapshot/com.linkedin.entity.entitiesVersionedV2.snapshot.json
metadata-service/restli-client-api/src/main/java/com/linkedin/entity/client/EntityClient.java
metadata-utils/src/main/java/com/linkedin/metadata/utils/GenericRecordUtils.java
metadata-utils/src/main/java/com/linkedin/metadata/utils/metrics/ExceptionUtils.java

Additional context used

LanguageTool

docs/advanced/mcp-mcl.md

[uncategorized] ~120-~120: Possible missing comma found.
Context: ...ted timestamp. 8. headers Optional headers which are meant to mimic http headers. ...

(AI_HYDRA_LEO_MISSING_COMMA)

[grammar] ~174-~174: There seems to be a noun/verb agreement error. Did you mean “writes” or “wrote”?
Context: ...es ### Conditional Writes Conditional write semantics use extra information contain...

(SINGULAR_NOUN_VERB_AGREEMENT)

[uncategorized] ~179-~179: Possible missing comma found.
Context: ...f-Version-Match Each time an aspect is updated a version is incremented to represent...

(AI_HYDRA_LEO_MISSING_COMMA)

[grammar] ~183-~183: The verb ‘write’ does not usually follow articles like ‘the’. Check that ‘write’ is spelled correctly; using ‘write’ as a noun may be non-standard.
Context: ...ctual version stored in the database, the write will fail. This prevents overwriting an...

(A_INFINITIVE)

[uncategorized] ~188-~188: This expression is usually spelled with a hyphen.
Context: ...using http header semantics. Similar to version based conditional writes this method can be u...

(BASED_HYPHEN)

[grammar] ~189-~189: The verb ‘write’ does not usually follow articles like ‘the’. Check that ‘write’ is spelled correctly; using ‘write’ as a noun may be non-standard.
Context: ...ites this method can be used to prevent the write if the target aspect was modified after...

(A_INFINITIVE)

[uncategorized] ~192-~192: Loose punctuation mark.
Context: ...SO-8601 standard. If-Unmodified-Since: A writer can specify that the aspect mu...

(UNLIKELY_OPENING_PUNCTUATION)

docs/api/openapi/openapi-usage-guide.md

[typographical] ~122-~122: It appears that a comma is missing.
Context: ...ead of overwriting the entity. In this example we've added a URL parameter `createEnti...

(DURING_THAT_TIME_COMMA)

docs/how/updating-datahub.md

[style] ~25-~25: ‘with respect to’ might be wordy. Consider a shorter alternative.
Context: ...value` key and the API is now symmetric with respect to input and outputs. Example Global Tags...

(EN_WORDINESS_PREMIUM_WITH_RESPECT_TO)

Additional comments not posted (37)

metadata-service/openapi-servlet/models/src/main/java/io/datahubproject/openapi/v2/models/GenericEntityScrollResultV2.java (1)

10-13: LGTM! The class and its fields look good.

The usage of Lombok annotations reduces boilerplate code, making the class definition clean and concise.

metadata-service/openapi-servlet/models/src/main/java/io/datahubproject/openapi/v2/models/BatchGetUrnResponseV2.java (1)

17-19: LGTM! The class and its field look good.

The usage of Jackson and OpenAPI annotations ensures proper serialization and API documentation.

entity-registry/src/testFixtures/java/com/linkedin/test/metadata/aspect/batch/TestSystemAspect.java (1)

14-26: LGTM! The class and its fields look good.

The usage of Lombok annotations reduces boilerplate code, making the class definition clean and concise.

metadata-service/openapi-servlet/models/src/main/java/io/datahubproject/openapi/v2/models/BatchGetUrnRequestV2.java (1)

16-18: LGTM! The class and its fields look good.

The usage of Jackson and OpenAPI annotations ensures proper serialization and API documentation.

metadata-service/openapi-servlet/models/src/main/java/io/datahubproject/openapi/v3/models/GenericEntityV3.java (4)

5-5: Import statement approved.

The import statement com.fasterxml.jackson.core.type.TypeReference is necessary for JSON deserialization.

28-29: Class declaration approved.

The class GenericEntityV3 extends LinkedHashMap and implements GenericEntity<GenericAspectV3>, which is appropriate for its intended functionality.

36-39: Method implementation approved.

The getAspects method filters out the "urn" key and collects the remaining entries into a map of GenericAspectV3.

48-71: Builder method implementation approved.

The builder method correctly handles JSON deserialization, constructs GenericAspectV3 objects, and builds a GenericEntityV3.

metadata-io/src/test/java/com/linkedin/metadata/AspectGenerationUtils.java (3)

Line range hint 9-18:
Import statements approved.

The import statements for SetMode and Nullable annotations are necessary for handling null values and optional parameters.

34-38: Method overloads approved.

The new overloads for createSystemMetadata provide flexibility for creating SystemMetadata with different sets of parameters.

47-55: Method implementation approved.

The method createSystemMetadata correctly sets the version parameter using SetMode.IGNORE_NULL to ensure it is only set if not null.

metadata-service/openapi-servlet/models/src/main/java/io/datahubproject/openapi/client/OpenApiClient.java (4)

5-6: Import statements approved.

The import statements for BatchGetUrnRequestV2 and BatchGetUrnResponseV2 are necessary for handling V2 batch get requests and responses.

43-46: Method implementation approved.

The method getBatchUrnsSystemAuth correctly handles batch get requests with system authentication using V2 request and response models.

Line range hint 51-66:
Method implementation approved.

The method getBatchUrns correctly handles batch get requests with specified authentication credentials using V2 request and response models.

Line range hint 69-84:
Method implementation approved.

The method mapResponse correctly deserializes the response to BatchGetUrnResponseV2 and handles potential IOExceptions.

docs/advanced/mcp-mcl.md (6)

63-66: Field addition approved.

The headers field added to MetadataChangeProposal is intended to mimic HTTP headers for conditional write logic.

172-176: Section addition approved.

The new section correctly explains the use of the headers field for conditional write logic.

Tools

LanguageTool

[grammar] ~174-~174: There seems to be a noun/verb agreement error. Did you mean “writes” or “wrote”?
Context: ...es ### Conditional Writes Conditional write semantics use extra information contain...

(SINGULAR_NOUN_VERB_AGREEMENT)

177-185: Section addition approved.

The new section correctly explains version-based conditional writes using the version field in SystemMetadata.

Tools

LanguageTool

[uncategorized] ~179-~179: Possible missing comma found.
Context: ...f-Version-Match Each time an aspect is updated a version is incremented to represent...

(AI_HYDRA_LEO_MISSING_COMMA)

[grammar] ~183-~183: The verb ‘write’ does not usually follow articles like ‘the’. Check that ‘write’ is spelled correctly; using ‘write’ as a noun may be non-standard.
Context: ...ctual version stored in the database, the write will fail. This prevents overwriting an...

(A_INFINITIVE)

186-191: Section addition approved.

The new section correctly explains time-based conditional writes using HTTP header semantics.

Tools

LanguageTool

[uncategorized] ~188-~188: This expression is usually spelled with a hyphen.
Context: ...using http header semantics. Similar to version based conditional writes this method can be u...

(BASED_HYPHEN)

[grammar] ~189-~189: The verb ‘write’ does not usually follow articles like ‘the’. Check that ‘write’ is spelled correctly; using ‘write’ as a noun may be non-standard.
Context: ...ites this method can be used to prevent the write if the target aspect was modified after...

(A_INFINITIVE)

192-194: Paragraph addition approved.

The new paragraph correctly explains the use of If-Unmodified-Since header for conditional writes.

Tools

LanguageTool

[uncategorized] ~192-~192: Loose punctuation mark.
Context: ...SO-8601 standard. If-Unmodified-Since: A writer can specify that the aspect mu...

(UNLIKELY_OPENING_PUNCTUATION)

195-196: Paragraph addition approved.

The new paragraph correctly explains the use of If-Modified-Since header for conditional writes.

metadata-models/src/main/resources/entity-registry.yml (1)

622-633: LGTM! ConditionalWriteValidator added correctly.

The ConditionalWriteValidator has been properly added to the aspect payload validators.

metadata-service/services/src/main/java/com/linkedin/metadata/entity/EntityService.java (1)

259-273: LGTM! New method getEnvelopedVersionedAspects is well-defined.

The new method getEnvelopedVersionedAspects is added correctly and follows the method conventions in the file.

metadata-io/src/test/java/com/linkedin/metadata/structuredproperties/validators/StructuredPropertiesValidatorTest.java (2)

392-392: LGTM! New assertion in testValidateImmutableMutation is correct.

The new assertion correctly checks the aspect group key and the exception message.

487-487: LGTM! New assertion in testValidateImmutableDelete is correct.

The new assertion correctly checks the aspect group key and the exception message.

docs/api/openapi/openapi-usage-guide.md (1)

586-591: LGTM! New sections for OpenAPI v3 features and conditional writes are clear and informative.

The new sections provide accurate information about OpenAPI v3 features and conditional writes.

docs/how/updating-datahub.md (3)

25-26: Clarify Breaking Change

The description of the breaking change is clear and concise. Ensure that the users are aware of the impact and provide guidance on how to adapt to the new format.

Tools

LanguageTool

[style] ~25-~25: ‘with respect to’ might be wordy. Consider a shorter alternative.
Context: ...value` key and the API is now symmetric with respect to input and outputs. Example Global Tags...

(EN_WORDINESS_PREMIUM_WITH_RESPECT_TO)

27-39: Ensure Example Accuracy

The example of the previous format for Global Tags Aspect is clear and accurate. Ensure that this format is consistent with the previous implementation.

41-55: Ensure Example Accuracy

The example of the new format for Global Tags Aspect with optional fields systemMetadata and headers is clear and accurate. Ensure that this format is consistent with the new implementation.

metadata-service/openapi-servlet/src/main/java/io/datahubproject/openapi/v3/OpenAPIV3Generator.java (8)

112-113: Ensure Schema Consistency

Ensure that the new schema for "BatchGet" entity requests is consistent with the rest of the schema definitions.

135-135: Ensure Endpoint Consistency

Ensure that the new batchGet endpoint is consistent with the other endpoints in terms of structure and functionality.

381-388: Ensure Schema Accuracy

Ensure that the schema definition for the batchGet request is accurate and conforms to the expected structure.

390-421: Ensure Response Accuracy

Ensure that the response schema for the batchGet request accurately represents the expected response structure.

574-597: New Schema for Aspect Request

The addition of systemMetadata and headers to the aspect request schema is correct. Ensure that these new fields are optional and nullable as intended.

645-675: New Schema for Entity BatchGet Request

The addition of systemMetadata and headers to the entity batchGet request schema is correct. Ensure that these fields are accurately represented in the schema.

738-743: Add Version Parameter

The addition of the version parameter for fetching a specific aspect version is correct. Ensure that this parameter is optional and has a default value of 0.

762-762: Add Parameters for Get Operation

The new parameters for the get operation, including systemMetadata and version, are correct. Ensure that these parameters are accurately represented in the OpenAPI specification.

kevin1chun · 2024-07-10T21:52:37Z

docs/how/updating-datahub.md

+  "systemMetadata": {},
+  "headers": {}
+}
+```


symmetric input & output is easier to work with :)

kevin1chun · 2024-07-10T22:17:02Z

metadata-io/src/main/java/com/linkedin/metadata/entity/EntityUtils.java

+      Map<String, SystemAspect> aspectMap) {
+    return aspectMap.entrySet().stream()
+        .filter(entry -> entry.getValue().getVersion() == 0)
+        .map(entry -> Map.entry(entry.getKey(), entry.getValue().getSystemMetadataVersion()))


for the aspect at version == 0,
entry.getValue().getSystemMetadataVersion() will return the monotonically increasing version, but how is this the "next" version?
Isn't this the current version?

My current understanding is
version is the custom logic version # which will be == 0 for the latest aspect
nextAspectVersion is the monotonically increasing version which is exposed to clients as version in system metadata

Due to legacy reasons the next version is the version that will be used for the version 0 when it is replaced. I opted not to change that here. This means for the currently being written aspect, it's system metadata contains the version that it will be given when it is re-written with a non-zero versioned row. Definitely confusing, but I'd say that's generally the case with zero based versioning.

ok so the version in the system metadata is actually the next version that will be computed 👍

...o/metadata-io-api/src/main/java/com/linkedin/metadata/entity/ebean/batch/ChangeItemImpl.java

kevin1chun · 2024-07-10T22:29:06Z

...o/metadata-io-api/src/main/java/com/linkedin/metadata/entity/ebean/batch/ChangeItemImpl.java

+  private final Map<String, String> headers;
+
+  @Override
+  public void setNextAspectVersion(long nextAspectVersion) {


Can you help me understand what context is this used vs. getSystemAspect?
They both have version incrementing logic

getSystemAspect allows overriding the version for some reason, I'd have to track down where and why. However since it overrides the version I just want to be sure it keeps the system metadata version in sync with whatever overriding is happening there

RyanHolstien

Overall LGTM, a few non-blocker comments

docs/advanced/mcp-mcl.md

RyanHolstien · 2024-07-10T22:42:08Z

metadata-io/src/main/java/com/linkedin/metadata/client/EntityClientAspectRetriever.java

@@ -52,4 +55,25 @@ public Map<Urn, Map<String, Aspect>> getLatestAspectObjects(
      }
    }
  }
+


Can the duplicated method here be shared between the client & server retrievers?

maybe, i'll see

metadata-io/src/main/java/com/linkedin/metadata/entity/EntityServiceImpl.java

coderabbitai

Actionable comments posted: 9

Outside diff range, codebase verification and nitpick comments (10)

entity-registry/src/testFixtures/java/com/linkedin/test/metadata/aspect/batch/TestSystemAspect.java (1)
1-26: Ensure proper implementation of interface methods.

The TestSystemAspect class does not implement the required methods from the SystemAspect interface (getVersion(), getCreatedOn(), and getCreatedBy()). These methods need to be implemented to comply with the interface.

Implement the getVersion() method.

Implement the getCreatedOn() method.

Implement the getCreatedBy() method.

Analysis chain

Ensure proper implementation of interface methods.

The class TestSystemAspect implements SystemAspect. Ensure that all methods required by the interface are correctly implemented.
Scripts executed

The following scripts were executed for the analysis:

Script:
#!/bin/bash
# Description: Verify that all methods required by the interface are correctly implemented.

# Test: Search for the implemented methods in the class.
rg --type java 'public class TestSystemAspect' -A 10
Length of output: 1362

Script:
#!/bin/bash
# Description: Identify methods in the SystemAspect interface.

# Locate the SystemAspect interface definition and list its methods.
rg --type java 'interface SystemAspect' -A 20
Length of output: 2186

Script:
#!/bin/bash
# Description: Verify that the TestSystemAspect class implements required methods.

# Locate the getVersion, getCreatedOn, and getCreatedBy method implementations in the TestSystemAspect class.
rg --type java 'public class TestSystemAspect' -A 50 | rg 'public long getVersion|public Timestamp getCreatedOn|public String getCreatedBy'
Length of output: 153
metadata-models/src/main/pegasus/com/linkedin/mxe/MetadataChangeProposal.pdl (1)
50-57: Ensure comprehensive documentation for the new field.

The new headers field is intended to mimic HTTP headers. Ensure that the documentation is comprehensive and covers all intended use cases.

Consider adding more details about the expected usage and format of the headers.
-  * Headers - intended to mimic http headers
+  * Headers - intended to mimic HTTP headers. This field can be used to pass additional metadata or control information with the proposal. Each entry in the map should have a key representing the header name and a value representing the header value.
metadata-integration/java/openlineage-converter/src/test/java/io/datahubproject/openlineage/HdfsPathDatasetTest.java (6)
45-45: Use consistent collection initialization.

Consider using a consistent style for initializing collections. For example, use List.of(...) instead of new LinkedList<>(Arrays.asList(...)).
- new LinkedList<>(Arrays.asList(
+ List.of(
92-92: Use consistent collection initialization.

Consider using a consistent style for initializing collections. For example, use List.of(...) instead of new LinkedList<>(Collections.singletonList(...)).
- new LinkedList<>(Collections.singletonList(
+ List.of(
124-124: Use consistent collection initialization.

Consider using a consistent style for initializing collections. For example, use List.of(...) instead of new LinkedList<>(Arrays.asList(...)).
- new LinkedList<>(Arrays.asList(
+ List.of(
157-157: Use consistent collection initialization.

Consider using a consistent style for initializing collections. For example, use List.of(...) instead of new LinkedList<>(Arrays.asList(...)).
- new LinkedList<>(Arrays.asList(
+ List.of(
190-190: Use consistent collection initialization.

Consider using a consistent style for initializing collections. For example, use List.of(...) instead of new LinkedList<>(Collections.singletonList(...)).
- new LinkedList<>(Collections.singletonList(
+ List.of(
16-17: Ensure proper test framework usage.

The transition from JUnit to TestNG is incomplete. The following findings indicate that JUnit is still being used across the project:

JUnit Imports and Usage:

metadata-service/services/src/test/java/com/linkedin/metadata/service/SettingsServiceTest.java

metadata-io/src/test/java/com/linkedin/metadata/search/fixtures/SampleDataFixtureTestBase.java

metadata-io/src/test/java/com/linkedin/metadata/search/LineageServiceTestBase.java

metadata-io/src/test/java/com/linkedin/metadata/entity/EntityServiceTest.java

metadata-integration/java/spark-lineage/src/test/java/datahub/spark/TestSparkJobsLineage.java

metadata-integration/java/spark-lineage/src/test/java/datahub/spark/TestCoalesceJobLineage.java

metadata-integration/java/datahub-event/src/test/java/datahub/event/MetadataChangeProposalWrapperTest.java

metadata-integration/java/datahub-event/src/test/java/datahub/event/EventFormatterTest.java

metadata-integration/java/spark-lineage-beta/src/test/java/datahub/spark/OpenLineageEventToDatahubTest.java

metadata-integration/java/datahub-client/src/test/java/datahub/client/kafka/KafkaEmitterTest.java

metadata-integration/java/datahub-client/src/test/java/datahub/client/rest/RestEmitterTest.java

metadata-integration/java/datahub-client/src/test/java/datahub/client/kafka/AvroSerializerTest.java

metadata-integration/java/datahub-client/src/test/java/datahub/client/patch/PatchTest.java

metadata-integration/java/datahub-client/src/test/java/datahub/client/file/FileEmitterTest.java

datahub-frontend/test/app/ApplicationTest.java

datahub-frontend/test/oidc/OidcCallbackLogicTest.java

datahub-frontend/test/utils/SearchUtilTest.java

datahub-frontend/test/security/DummyLoginModuleTest.java

datahub-frontend/test/security/OidcConfigurationTest.java

JUnit Dependencies in Build Files:

metadata-service/services/build.gradle

metadata-service/restli-servlet-impl/build.gradle

metadata-service/plugin/src/test/sample-test-plugins/build.gradle

metadata-ingestion-modules/dagster-plugin/build.gradle

metadata-ingestion-modules/airflow-plugin/build.gradle

metadata-ingestion/build.gradle

Root build.gradle

Recommendation:

Complete the transition to TestNG by updating the remaining test files and build configurations to use TestNG. This will ensure consistency and avoid potential issues arising from mixed test frameworks.

Analysis chain

Ensure proper test framework usage.

Ensure that the test framework is correctly used. The past comment indicates a change from JUnit to TestNG.
Scripts executed

The following scripts were executed for the analysis:

Script:
#!/bin/bash
# Description: Verify the test framework used in the project.

# Test: Search for the usage of TestNG and JUnit frameworks.
rg --type java 'import org.testng' -A 5
rg --type java 'import org.junit' -A 5
Length of output: 375622

Script:
#!/bin/bash
# Check for JUnit test annotations and methods usage in files that import JUnit
rg --type java '@Test' -g '*.java' | grep -E 'import org\.junit|import org\.junit\.jupiter'

# Look for testing framework configuration in build files
rg --type xml 'junit' -g 'pom.xml'
rg --type groovy 'junit' -g 'build.gradle'
Length of output: 2975
docs/how/updating-datahub.md (1)
25-25: Consider rephrasing for conciseness.

The phrase "with respect to" is wordy. Consider using a shorter alternative.
- and the API is now symmetric with respect to input and outputs.
+ and the API is now symmetric for inputs and outputs.
Tools

LanguageTool

[style] ~25-~25: ‘with respect to’ might be wordy. Consider a shorter alternative.
Context: ...value` key and the API is now symmetric with respect to input and outputs. Example Global Tags...

(EN_WORDINESS_PREMIUM_WITH_RESPECT_TO)
metadata-service/restli-api/src/main/snapshot/com.linkedin.entity.entities.snapshot.json (1)
6472-6476: Issue Found: Missing version field in SystemMetadata record definitions

The version field added to the SystemMetadata record in the file metadata-service/restli-api/src/main/snapshot/com.linkedin.entity.entities.snapshot.json is not present in other definitions of SystemMetadata across the codebase. Please ensure the version field is consistently added to all SystemMetadata record definitions in the following files:

metadata-service/restli-api/src/main/snapshot/com.linkedin.entity.entitiesVersionedV2.snapshot.json

metadata-service/restli-api/src/main/snapshot/com.linkedin.entity.entitiesV2.snapshot.json

metadata-service/restli-api/src/main/snapshot/com.linkedin.entity.aspects.snapshot.json

Analysis chain

LGTM! Ensure the usage of the version field is consistent.

The addition of the version field in the SystemMetadata record aligns with the PR objectives. The field name and description are clear, and marking it as optional ensures backward compatibility.

However, ensure that the version field is used consistently across the codebase.
Scripts executed

The following scripts were executed for the analysis:

Script:
#!/bin/bash
# Description: Verify the usage of the `version` field.

# Test: Search for the `version` field usage. Expect: Consistent usage across the codebase.
rg --type json '"version"'
Length of output: 142372

Script:
#!/bin/bash
# Description: Verify the usage of the `version` field in the context of `SystemMetadata`.

# Test: Search for the `SystemMetadata` record and its usage of the `version` field.
rg --type json '"SystemMetadata"'
Length of output: 678

Script:
#!/bin/bash
# Description: Extract and review the sections where `SystemMetadata` is defined or used.

# Extract relevant sections from the identified files
rg -A 10 '"SystemMetadata"' metadata-service/restli-api/src/main/snapshot/com.linkedin.entity.entitiesVersionedV2.snapshot.json
rg -A 10 '"SystemMetadata"' metadata-service/restli-api/src/main/snapshot/com.linkedin.entity.entitiesV2.snapshot.json
rg -A 10 '"SystemMetadata"' metadata-service/restli-api/src/main/snapshot/com.linkedin.entity.aspects.snapshot.json
rg -A 10 '"SystemMetadata"' metadata-service/restli-api/src/main/snapshot/com.linkedin.entity.entities.snapshot.json
Length of output: 2643

Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

Commits

Files that changed from the base of the PR and between a73f076 and e6b9d5e.

Files selected for processing (57)

docs/advanced/mcp-mcl.md (4 hunks)
docs/api/openapi/openapi-usage-guide.md (2 hunks)
docs/how/updating-datahub.md (1 hunks)
entity-registry/src/main/java/com/linkedin/metadata/aspect/AspectRetriever.java (2 hunks)
entity-registry/src/main/java/com/linkedin/metadata/aspect/EnvelopedSystemAspect.java (1 hunks)
entity-registry/src/main/java/com/linkedin/metadata/aspect/SystemAspect.java (2 hunks)
entity-registry/src/main/java/com/linkedin/metadata/aspect/batch/MCPItem.java (1 hunks)
entity-registry/src/main/java/com/linkedin/metadata/aspect/plugins/validation/AspectValidationException.java (1 hunks)
entity-registry/src/main/java/com/linkedin/metadata/aspect/plugins/validation/ValidationExceptionCollection.java (1 hunks)
entity-registry/src/main/java/com/linkedin/metadata/aspect/validation/ConditionalWriteValidator.java (1 hunks)
entity-registry/src/test/java/com/linkedin/metadata/aspect/validators/ConditionalWriteValidatorTest.java (1 hunks)
entity-registry/src/testFixtures/java/com/linkedin/test/metadata/aspect/MockAspectRetriever.java (4 hunks)
entity-registry/src/testFixtures/java/com/linkedin/test/metadata/aspect/batch/TestMCP.java (2 hunks)
entity-registry/src/testFixtures/java/com/linkedin/test/metadata/aspect/batch/TestSystemAspect.java (1 hunks)
metadata-integration/java/datahub-client/src/main/resources/MetadataChangeProposal.avsc (1 hunks)
metadata-integration/java/datahub-event/src/main/resources/MetadataChangeProposal.avsc (1 hunks)
metadata-integration/java/openlineage-converter/build.gradle (1 hunks)
metadata-integration/java/openlineage-converter/src/test/java/io/datahubproject/openlineage/HdfsPathDatasetTest.java (8 hunks)
metadata-io/metadata-io-api/src/main/java/com/linkedin/metadata/entity/ebean/batch/ChangeItemImpl.java (7 hunks)
metadata-io/src/main/java/com/linkedin/metadata/aspect/utils/DefaultAspectsUtil.java (3 hunks)
metadata-io/src/main/java/com/linkedin/metadata/client/EntityClientAspectRetriever.java (2 hunks)
metadata-io/src/main/java/com/linkedin/metadata/entity/EntityServiceAspectRetriever.java (2 hunks)
metadata-io/src/main/java/com/linkedin/metadata/entity/EntityServiceImpl.java (8 hunks)
metadata-io/src/main/java/com/linkedin/metadata/entity/EntityUtils.java (4 hunks)
metadata-io/src/test/java/com/linkedin/metadata/AspectGenerationUtils.java (3 hunks)
metadata-io/src/test/java/com/linkedin/metadata/entity/EntityServiceTest.java (14 hunks)
metadata-io/src/test/java/com/linkedin/metadata/structuredproperties/validators/StructuredPropertiesValidatorTest.java (2 hunks)
metadata-models-custom/src/main/java/com/linkedin/metadata/aspect/plugins/spring/validation/CustomDataQualityRulesValidator.java (2 hunks)
metadata-models-custom/src/main/java/com/linkedin/metadata/aspect/plugins/validation/CustomDataQualityRulesValidator.java (2 hunks)
metadata-models/src/main/pegasus/com/linkedin/mxe/MetadataChangeProposal.pdl (1 hunks)
metadata-models/src/main/pegasus/com/linkedin/mxe/SystemMetadata.pdl (1 hunks)
metadata-models/src/main/resources/entity-registry.yml (1 hunks)
metadata-operation-context/src/main/java/io/datahubproject/test/metadata/context/TestOperationContexts.java (2 hunks)
metadata-service/openapi-entity-servlet/src/main/java/io/datahubproject/openapi/v2/delegates/EntityApiDelegateImpl.java (2 hunks)
metadata-service/openapi-servlet/models/src/main/java/io/datahubproject/openapi/client/OpenApiClient.java (4 hunks)
metadata-service/openapi-servlet/models/src/main/java/io/datahubproject/openapi/models/GenericAspect.java (1 hunks)
metadata-service/openapi-servlet/models/src/main/java/io/datahubproject/openapi/models/GenericEntity.java (1 hunks)
metadata-service/openapi-servlet/models/src/main/java/io/datahubproject/openapi/models/GenericEntityScrollResult.java (1 hunks)
metadata-service/openapi-servlet/models/src/main/java/io/datahubproject/openapi/v2/models/BatchGetUrnRequestV2.java (2 hunks)
metadata-service/openapi-servlet/models/src/main/java/io/datahubproject/openapi/v2/models/BatchGetUrnResponseV2.java (2 hunks)
metadata-service/openapi-servlet/models/src/main/java/io/datahubproject/openapi/v2/models/GenericAspectV2.java (1 hunks)
metadata-service/openapi-servlet/models/src/main/java/io/datahubproject/openapi/v2/models/GenericEntityScrollResultV2.java (1 hunks)
metadata-service/openapi-servlet/models/src/main/java/io/datahubproject/openapi/v2/models/GenericEntityV2.java (2 hunks)
metadata-service/openapi-servlet/models/src/main/java/io/datahubproject/openapi/v3/models/GenericAspectV3.java (1 hunks)
metadata-service/openapi-servlet/models/src/main/java/io/datahubproject/openapi/v3/models/GenericEntityScrollResultV3.java (1 hunks)
metadata-service/openapi-servlet/models/src/main/java/io/datahubproject/openapi/v3/models/GenericEntityV3.java (3 hunks)
metadata-service/openapi-servlet/src/main/java/io/datahubproject/openapi/controller/GenericEntitiesController.java (8 hunks)
metadata-service/openapi-servlet/src/main/java/io/datahubproject/openapi/v2/controller/EntityController.java (4 hunks)
metadata-service/openapi-servlet/src/main/java/io/datahubproject/openapi/v3/OpenAPIV3Generator.java (8 hunks)
metadata-service/openapi-servlet/src/main/java/io/datahubproject/openapi/v3/controller/EntityController.java (5 hunks)
metadata-service/restli-api/src/main/snapshot/com.linkedin.entity.aspects.snapshot.json (2 hunks)
metadata-service/restli-api/src/main/snapshot/com.linkedin.entity.entities.snapshot.json (1 hunks)
metadata-service/restli-api/src/main/snapshot/com.linkedin.entity.entitiesV2.snapshot.json (1 hunks)
metadata-service/restli-api/src/main/snapshot/com.linkedin.entity.entitiesVersionedV2.snapshot.json (1 hunks)
metadata-service/restli-client-api/src/main/java/com/linkedin/entity/client/EntityClient.java (3 hunks)
metadata-service/services/src/main/java/com/linkedin/metadata/entity/EntityService.java (1 hunks)
metadata-utils/src/main/java/com/linkedin/metadata/utils/GenericRecordUtils.java (2 hunks)

Files not processed due to max files limit (1)

metadata-utils/src/main/java/com/linkedin/metadata/utils/metrics/ExceptionUtils.java

Files skipped from review due to trivial changes (1)

metadata-io/src/test/java/com/linkedin/metadata/structuredproperties/validators/StructuredPropertiesValidatorTest.java

Files skipped from review as they are similar to previous changes (39)

entity-registry/src/main/java/com/linkedin/metadata/aspect/AspectRetriever.java
entity-registry/src/main/java/com/linkedin/metadata/aspect/EnvelopedSystemAspect.java
entity-registry/src/main/java/com/linkedin/metadata/aspect/SystemAspect.java
entity-registry/src/main/java/com/linkedin/metadata/aspect/batch/MCPItem.java
entity-registry/src/main/java/com/linkedin/metadata/aspect/plugins/validation/ValidationExceptionCollection.java
entity-registry/src/main/java/com/linkedin/metadata/aspect/validation/ConditionalWriteValidator.java
entity-registry/src/testFixtures/java/com/linkedin/test/metadata/aspect/MockAspectRetriever.java
entity-registry/src/testFixtures/java/com/linkedin/test/metadata/aspect/batch/TestMCP.java
metadata-integration/java/datahub-event/src/main/resources/MetadataChangeProposal.avsc
metadata-integration/java/openlineage-converter/build.gradle
metadata-io/metadata-io-api/src/main/java/com/linkedin/metadata/entity/ebean/batch/ChangeItemImpl.java
metadata-io/src/main/java/com/linkedin/metadata/aspect/utils/DefaultAspectsUtil.java
metadata-io/src/main/java/com/linkedin/metadata/client/EntityClientAspectRetriever.java
metadata-io/src/main/java/com/linkedin/metadata/entity/EntityServiceAspectRetriever.java
metadata-io/src/main/java/com/linkedin/metadata/entity/EntityServiceImpl.java
metadata-io/src/main/java/com/linkedin/metadata/entity/EntityUtils.java
metadata-io/src/test/java/com/linkedin/metadata/AspectGenerationUtils.java
metadata-io/src/test/java/com/linkedin/metadata/entity/EntityServiceTest.java
metadata-models/src/main/pegasus/com/linkedin/mxe/SystemMetadata.pdl
metadata-operation-context/src/main/java/io/datahubproject/test/metadata/context/TestOperationContexts.java
metadata-service/openapi-servlet/models/src/main/java/io/datahubproject/openapi/models/GenericAspect.java
metadata-service/openapi-servlet/models/src/main/java/io/datahubproject/openapi/models/GenericEntity.java
metadata-service/openapi-servlet/models/src/main/java/io/datahubproject/openapi/models/GenericEntityScrollResult.java
metadata-service/openapi-servlet/models/src/main/java/io/datahubproject/openapi/v2/models/BatchGetUrnRequestV2.java
metadata-service/openapi-servlet/models/src/main/java/io/datahubproject/openapi/v2/models/BatchGetUrnResponseV2.java
metadata-service/openapi-servlet/models/src/main/java/io/datahubproject/openapi/v2/models/GenericAspectV2.java
metadata-service/openapi-servlet/models/src/main/java/io/datahubproject/openapi/v2/models/GenericEntityV2.java
metadata-service/openapi-servlet/models/src/main/java/io/datahubproject/openapi/v3/models/GenericAspectV3.java
metadata-service/openapi-servlet/models/src/main/java/io/datahubproject/openapi/v3/models/GenericEntityScrollResultV3.java
metadata-service/openapi-servlet/models/src/main/java/io/datahubproject/openapi/v3/models/GenericEntityV3.java
metadata-service/openapi-servlet/src/main/java/io/datahubproject/openapi/controller/GenericEntitiesController.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/v3/OpenAPIV3Generator.java
metadata-service/openapi-servlet/src/main/java/io/datahubproject/openapi/v3/controller/EntityController.java
metadata-service/restli-api/src/main/snapshot/com.linkedin.entity.aspects.snapshot.json
metadata-service/restli-api/src/main/snapshot/com.linkedin.entity.entitiesV2.snapshot.json
metadata-service/restli-client-api/src/main/java/com/linkedin/entity/client/EntityClient.java
metadata-service/services/src/main/java/com/linkedin/metadata/entity/EntityService.java
metadata-utils/src/main/java/com/linkedin/metadata/utils/GenericRecordUtils.java

Additional context used

LanguageTool

docs/advanced/mcp-mcl.md

[grammar] ~174-~174: There seems to be a noun/verb agreement error. Did you mean “writes” or “wrote”?
Context: ...es ### Conditional Writes Conditional write semantics use extra information contain...

(SINGULAR_NOUN_VERB_AGREEMENT)

[grammar] ~183-~183: The verb ‘write’ does not usually follow articles like ‘the’. Check that ‘write’ is spelled correctly; using ‘write’ as a noun may be non-standard.
Context: ...ctual version stored in the database, the write will fail. This prevents overwriting an...

(A_INFINITIVE)

[uncategorized] ~188-~188: This expression is usually spelled with a hyphen.
Context: ...using http header semantics. Similar to version based conditional writes this method can be u...

(BASED_HYPHEN)

[grammar] ~189-~189: The verb ‘write’ does not usually follow articles like ‘the’. Check that ‘write’ is spelled correctly; using ‘write’ as a noun may be non-standard.
Context: ...ites this method can be used to prevent the write if the target aspect was modified after...

(A_INFINITIVE)

[uncategorized] ~189-~189: Possible missing preposition found.
Context: ...get aspect was modified after a reading the aspect. Per the http specification date...

(AI_HYDRA_LEO_MISSING_OF)

[uncategorized] ~190-~190: A comma might be missing here.
Context: ...fter a reading the aspect. Per the http specification dates must comply with ISO-8601 standar...

(AI_EN_LECTOR_MISSING_PUNCTUATION_COMMA)

[uncategorized] ~192-~192: Loose punctuation mark.
Context: ...SO-8601 standard. If-Unmodified-Since: A writer can specify that the aspect mu...

(UNLIKELY_OPENING_PUNCTUATION)

docs/api/openapi/openapi-usage-guide.md

[typographical] ~122-~122: It appears that a comma is missing.
Context: ...ead of overwriting the entity. In this example we've added a URL parameter `createEnti...

(DURING_THAT_TIME_COMMA)

docs/how/updating-datahub.md

[style] ~25-~25: ‘with respect to’ might be wordy. Consider a shorter alternative.
Context: ...value` key and the API is now symmetric with respect to input and outputs. Example Global Tags...

(EN_WORDINESS_PREMIUM_WITH_RESPECT_TO)

Additional comments not posted (39)

metadata-service/openapi-servlet/models/src/main/java/io/datahubproject/openapi/v2/models/GenericEntityScrollResultV2.java (1)
10-11: Ensure proper implementation of interface methods.

The class GenericEntityScrollResultV2 implements GenericEntityScrollResult<GenericAspectV2, GenericEntityV2>. Ensure that all methods required by the interface are correctly implemented.

Verification successful

Interface methods correctly implemented

The GenericEntityScrollResult interface does not declare any methods. Therefore, the GenericEntityScrollResultV2 class correctly implements this interface as it stands.

No methods are required to be implemented by the GenericEntityScrollResultV2 class.
Scripts executed

The following scripts were executed for the analysis:

Script:
#!/bin/bash
# Description: Verify that all methods required by the interface are correctly implemented.

# Test: Search for the implemented methods in the class.
rg --type java 'public class GenericEntityScrollResultV2' -A 10
Length of output: 868

Script:
#!/bin/bash
# Description: Verify that all methods required by the interface are correctly implemented.

# Search for method declarations in the GenericEntityScrollResult interface
rg --type java 'interface GenericEntityScrollResult' -A 20

# Search for method implementations in the GenericEntityScrollResultV2 class
rg --type java 'public class GenericEntityScrollResultV2' -A 50
Length of output: 1146
entity-registry/src/main/java/com/linkedin/metadata/aspect/plugins/validation/AspectValidationException.java (1)
25-32: Ensure proper usage of exception handling.

The new methods forPrecondition handle precondition validation exceptions. Ensure that the exception handling logic is correct and consistent with the rest of the codebase.

Verification successful

Verified: Proper usage of forPrecondition methods is consistent with exception handling logic in the codebase.

The new methods forPrecondition in AspectValidationException are correctly and consistently used in the ConditionalWriteValidator.java file to handle precondition failures by creating and returning AspectValidationException.

Locations:

entity-registry/src/main/java/com/linkedin/metadata/aspect/validation/ConditionalWriteValidator.java
Scripts executed

The following scripts were executed for the analysis:

Script:
#!/bin/bash
# Description: Verify that the new methods are correctly used in the codebase.

# Test: Search for the usage of the new methods.
rg --type java 'AspectValidationException.forPrecondition'
Length of output: 939

Script:
#!/bin/bash
# Description: Inspect the context around the usage of `forPrecondition` in `ConditionalWriteValidator.java`.

# Display lines of code around the usage of `forPrecondition` in `ConditionalWriteValidator.java`.
rg --type java -A 10 -B 10 'AspectValidationException.forPrecondition' entity-registry/src/main/java/com/linkedin/metadata/aspect/validation/ConditionalWriteValidator.java
Length of output: 1922
metadata-models-custom/src/main/java/com/linkedin/metadata/aspect/plugins/validation/CustomDataQualityRulesValidator.java (2)

29-29: LGTM!

The validation logic ensures that at least one rule exists in DataQualityRules.

58-59: LGTM!

The validation logic ensures that the field types of DataQualityRules remain consistent.

metadata-service/openapi-servlet/models/src/main/java/io/datahubproject/openapi/client/OpenApiClient.java (3)

43-44: LGTM!

The method signature has been correctly updated to use BatchGetUrnRequestV2 and BatchGetUrnResponseV2.

51-52: LGTM!

The method signature has been correctly updated to use BatchGetUrnRequestV2 and BatchGetUrnResponseV2.

69-70: LGTM!

The method has been correctly updated to deserialize the response to BatchGetUrnResponseV2.

metadata-models-custom/src/main/java/com/linkedin/metadata/aspect/plugins/spring/validation/CustomDataQualityRulesValidator.java (2)

52-52: LGTM!

The validation logic ensures that at least one rule exists in DataQualityRules.

81-82: LGTM!

The validation logic ensures that the field types of DataQualityRules remain consistent.

metadata-integration/java/datahub-client/src/main/resources/MetadataChangeProposal.avsc (2)

171-174: LGTM!

The addition of the version field in SystemMetadata aligns with the PR objective of supporting conditional write semantics.

180-185: LGTM!

The addition of the headers field aligns with the PR objective of supporting conditional write semantics.
docs/advanced/mcp-mcl.md (2)
179-179: Add a comma after 'updated'.

There is a missing comma after 'updated' in the explanation of If-Version-Match.
- Each time an aspect is updated a `version` is incremented to represent the change to the aspect.
+ Each time an aspect is updated, a `version` is incremented to represent the change to the aspect.
Likely invalid or redundant comment.

174-174: Correct noun/verb agreement error.

There seems to be a noun/verb agreement error. The correct sentence should use "writes" instead of "write".
- Conditional write semantics use extra information contained in the MCP `headers` field to possibly avoid writing new aspects
+ Conditional writes semantics use extra information contained in the MCP `headers` field to possibly avoid writing new aspects
Likely invalid or redundant comment.

Tools

LanguageTool

[grammar] ~174-~174: There seems to be a noun/verb agreement error. Did you mean “writes” or “wrote”?
Context: ...es ### Conditional Writes Conditional write semantics use extra information contain...

(SINGULAR_NOUN_VERB_AGREEMENT)
metadata-integration/java/openlineage-converter/src/test/java/io/datahubproject/openlineage/HdfsPathDatasetTest.java (1)
252-252: Correct platform name in config.

The platform field should be set to "gcs" instead of "s3".
- platform("s3")
+ platform("gcs")
Likely invalid or redundant comment.
metadata-models/src/main/resources/entity-registry.yml (1)

622-633: Ensure correct configuration of ConditionalWriteValidator.

The ConditionalWriteValidator is correctly added and configured for various operations.

docs/api/openapi/openapi-usage-guide.md (1)

585-591: Document new OpenAPI v3 features.

The added section documents the new OpenAPI v3 features, particularly the support for conditional writes. This is a necessary addition for clarity.

docs/how/updating-datahub.md (1)

27-56: Document breaking changes and new format for Global Tags Aspect.

The added section documents the breaking changes and the new format for Global Tags Aspect, including optional fields systemMetadata and headers. This is a necessary addition for clarity.

metadata-service/openapi-entity-servlet/src/main/java/io/datahubproject/openapi/v2/delegates/EntityApiDelegateImpl.java (3)

73-74: Approved: Addition of Lombok annotations.

The added Lombok annotations Getter, Setter, and Accessors simplify the code by removing boilerplate getter and setter methods.

Also applies to: 82-82

75-75: Approved: Addition of Slf4j annotation.

The Slf4j annotation generates a logger field, which is a standard practice for logging.

93-93: Approved: Addition of HttpServletRequest field with Lombok annotations.

The HttpServletRequest field with Lombok's Getter and Setter annotations simplifies access and mutation of the request object.

entity-registry/src/test/java/com/linkedin/metadata/aspect/validators/ConditionalWriteValidatorTest.java (19)

1-1: Imports look good.

The necessary classes for testing are correctly imported.

2-2: Ensure proper package declaration.

The package declaration is correct and follows the naming conventions.

3-7: Static imports and Mockito setup are correct.

The static imports and Mockito setup are correctly used for the test cases.

9-35: Class imports and TestNG setup are correct.

The necessary classes for testing, including TestNG annotations, are correctly imported.

37-68: Initialization of mock objects and constants.

The mock objects and constants for supported change types and validator configuration are correctly initialized.

132-174: Test case testNoSystemMetadataNextVersionNextVersionSuccess looks good.

The test case covers all supported change types and validates that no exceptions are thrown when the previous system metadata is missing.

176-237: Test case testNoPreviousVersionsLookupSchemaMetadataNextVersionSuccess looks good.

The test case covers all supported change types and validates that no exceptions are thrown when the previous versions lookup based on schema metadata is successful.

239-299: Test case testNoPreviousVersionsLookupVersionNextVersionSuccess looks good.

The test case covers all supported change types and validates that no exceptions are thrown when the previous versions lookup based on version is successful.

301-379: Test case testNextVersionFail looks good.

The test case covers all supported change types and validates that exceptions are thrown when the expected version does not match the actual version.

381-440: Test case testNoSystemMetadataNextVersionNextVersionFail looks good.

The test case covers all supported change types and validates that exceptions are thrown when the previous system metadata is missing and the expected version does not match the actual version.

442-520: Test case testNoPreviousVersionsLookupSchemaMetadataNextVersionFail looks good.

The test case covers all supported change types and validates that exceptions are thrown when the previous versions lookup based on schema metadata fails.

522-599: Test case testNoPreviousVersionsLookupVersionNextVersionFail looks good.

The test case covers all supported change types and validates that exceptions are thrown when the previous versions lookup based on version fails.

601-674: Test case testModifiedSinceSuccess looks good.

The test case covers all supported change types and validates that no exceptions are thrown when the If-Modified-Since and If-Unmodified-Since headers are correctly handled.

676-749: Test case testNoPreviousLookupAuditStampModifiedSinceSuccess looks good.

The test case covers all supported change types and validates that no exceptions are thrown when the previous lookup based on audit stamp is successful.

751-836: Test case testModifiedSinceBeforeRangeFail looks good.

The test case covers all supported change types and validates that exceptions are thrown when the If-Modified-Since header is before the modification range.

838-923: Test case testModifiedSinceAfterRangeFail looks good.

The test case covers all supported change types and validates that exceptions are thrown when the If-Modified-Since header is after the modification range.

925-1010: Test case testNoPreviousLookupAuditStampModifiedSinceBeforeRangeFail looks good.

The test case covers all supported change types and validates that exceptions are thrown when the previous lookup based on audit stamp is before the modification range.

1012-1097: Test case testNoPreviousLookupAuditStampModifiedSinceAfterRangeFail looks good.

The test case covers all supported change types and validates that exceptions are thrown when the previous lookup based on audit stamp is after the modification range.

70-130: Test case testNextVersionSuccess looks good.

The test case covers all supported change types and validates that no exceptions are thrown when the expected version matches the actual version.

However, ensure that the ConditionalWriteValidator handles all edge cases correctly.

docs/api/openapi/openapi-usage-guide.md

...et/models/src/main/java/io/datahubproject/openapi/v2/models/GenericEntityScrollResultV2.java

...a-service/restli-api/src/main/snapshot/com.linkedin.entity.entitiesVersionedV2.snapshot.json

docs/advanced/mcp-mcl.md

...a/openlineage-converter/src/test/java/io/datahubproject/openlineage/HdfsPathDatasetTest.java

ajoymajumdar · 2024-07-11T14:57:58Z

docs/advanced/mcp-mcl.md

+in `SystemMetadata`.
+
+A writer can provide a header with the expected `version` when initiating the request. If the expected `version` does not


How will batch or multiple aspect updates work?

The batch endpoint was adjusted so that the entity/aspect object contains a headers Map<String, String> which will carry the information for each aspect. If any one of the aspect's conditions is violated, it will cancel the entire batch.

* introduce If-Modified-Since, If-Unmodified-Since, If-Version-Match * ConditionalWriteValidator added * MCP headers field added * SystemMetadata version field added * batchGet with If-Version-Match support

ajoymajumdar · 2024-07-11T15:17:42Z

entity-registry/src/main/java/com/linkedin/metadata/aspect/EnvelopedSystemAspect.java

+  }
+
+  @Override
+  public Timestamp getCreatedOn() {


Any reason why the updatedBy and lastUpdatedOn are not included?

Mostly because createdon is an actual column in the datastore. The other concepts are not necessarily uniformly implemented for all aspects. This one is required for every single aspect because it is part of the aspect's row in the database.
The System Aspect is a closer representation to the database aspect's row, whereas the other timestamps are higher level abstractions in the non-System aspect which is defined inside the metadata and systemmetadata json strings.

ajoymajumdar · 2024-07-11T15:20:51Z

...egistry/src/main/java/com/linkedin/metadata/aspect/validation/ConditionalWriteValidator.java

+@Setter
+@Getter
+@Accessors(chain = true)
+public class ConditionalWriteValidator extends AspectPayloadValidator {


Is this validator first in the chain for validations? For my understanding, how does one order the aspect validators?

Not sure the validation order matters in this case, however the order is defined by the yaml file. All validations are processed before continuing. The sum of all validation exceptions are collected in the exception collection and returned to the user. So all validation errors are reported, there is no early exit function at the moment.

Shouldn't this be first in the order and fail fast?

coderabbitai

Actionable comments posted: 2

Outside diff range, codebase verification and nitpick comments (6)

docs/advanced/mcp-mcl.md (4)
179-179: Add a comma after 'updated'.

There is a missing comma after 'updated' in the explanation of If-Version-Match.
- Each time an aspect is updated a `version` is incremented to represent the change to the aspect.
+ Each time an aspect is updated, a `version` is incremented to represent the change to the aspect.
Tools

LanguageTool

[uncategorized] ~179-~179: Possible missing comma found.
Context: ...f-Version-Match Each time an aspect is updated a version is incremented to represent...

(AI_HYDRA_LEO_MISSING_COMMA)

188-188: Add a hyphen in 'version-based'.

This expression is usually spelled with a hyphen.
- Similar to version based conditional writes
+ Similar to version-based conditional writes
Tools

LanguageTool

[uncategorized] ~188-~188: This expression is usually spelled with a hyphen.
Context: ...using http header semantics. Similar to version based conditional writes this method can be u...

(BASED_HYPHEN)

189-189: Add 'of' after 'reading'.

There is a missing preposition 'of' after 'reading'.
- the target aspect was modified after a reading the aspect.
+ the target aspect was modified after a reading of the aspect.
Tools

LanguageTool

[grammar] ~189-~189: The verb ‘write’ does not usually follow articles like ‘the’. Check that ‘write’ is spelled correctly; using ‘write’ as a noun may be non-standard.
Context: ...ites this method can be used to prevent the write if the target aspect was modified after...

(A_INFINITIVE)

[uncategorized] ~189-~189: Possible missing preposition found.
Context: ...get aspect was modified after a reading the aspect. Per the http specification date...

(AI_HYDRA_LEO_MISSING_OF)

190-190: Add a comma after 'specification'.

There is a missing comma after 'specification' in the explanation of If-Unmodified-Since.
- Per the http specification dates must comply with ISO-8601 standard.
+ Per the http specification, dates must comply with ISO-8601 standard.
Tools

LanguageTool

[uncategorized] ~190-~190: Possible missing comma found.
Context: ...fter a reading the aspect. Per the http specification dates must comply with ISO-8601 standar...

(AI_HYDRA_LEO_MISSING_COMMA)
docs/api/openapi/openapi-usage-guide.md (1)
122-122: Add a comma for clarity.

A comma is missing in the sentence for better readability.
- In this example we've added a URL parameter `createEntityIfNotExists=true`
+ In this example, we've added a URL parameter `createEntityIfNotExists=true`
Tools

LanguageTool

[typographical] ~122-~122: It appears that a comma is missing.
Context: ...ead of overwriting the entity. In this example we've added a URL parameter `createEnti...

(DURING_THAT_TIME_COMMA)
docs/how/updating-datahub.md (1)
25-25: Consider a more concise alternative to "with respect to".

The phrase "with respect to" might be wordy. Consider using "for" or "regarding" instead.
- and the API is now symmetric with respect to input and outputs.
+ and the API is now symmetric for input and outputs.
Tools

LanguageTool

[style] ~25-~25: ‘with respect to’ might be wordy. Consider a shorter alternative.
Context: ...value` key and the API is now symmetric with respect to input and outputs. Example Global Tags...

(EN_WORDINESS_PREMIUM_WITH_RESPECT_TO)

Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

Commits

Files that changed from the base of the PR and between e6b9d5e and 5d9a518.

Files selected for processing (57)

docs/advanced/mcp-mcl.md (4 hunks)
docs/api/openapi/openapi-usage-guide.md (2 hunks)
docs/how/updating-datahub.md (1 hunks)
entity-registry/src/main/java/com/linkedin/metadata/aspect/AspectRetriever.java (2 hunks)
entity-registry/src/main/java/com/linkedin/metadata/aspect/EnvelopedSystemAspect.java (1 hunks)
entity-registry/src/main/java/com/linkedin/metadata/aspect/SystemAspect.java (2 hunks)
entity-registry/src/main/java/com/linkedin/metadata/aspect/batch/MCPItem.java (1 hunks)
entity-registry/src/main/java/com/linkedin/metadata/aspect/plugins/validation/AspectValidationException.java (1 hunks)
entity-registry/src/main/java/com/linkedin/metadata/aspect/plugins/validation/ValidationExceptionCollection.java (1 hunks)
entity-registry/src/main/java/com/linkedin/metadata/aspect/validation/ConditionalWriteValidator.java (1 hunks)
entity-registry/src/test/java/com/linkedin/metadata/aspect/validators/ConditionalWriteValidatorTest.java (1 hunks)
entity-registry/src/testFixtures/java/com/linkedin/test/metadata/aspect/MockAspectRetriever.java (4 hunks)
entity-registry/src/testFixtures/java/com/linkedin/test/metadata/aspect/batch/TestMCP.java (2 hunks)
entity-registry/src/testFixtures/java/com/linkedin/test/metadata/aspect/batch/TestSystemAspect.java (1 hunks)
metadata-integration/java/datahub-client/src/main/resources/MetadataChangeProposal.avsc (1 hunks)
metadata-integration/java/datahub-event/src/main/resources/MetadataChangeProposal.avsc (1 hunks)
metadata-integration/java/openlineage-converter/build.gradle (1 hunks)
metadata-integration/java/openlineage-converter/src/test/java/io/datahubproject/openlineage/HdfsPathDatasetTest.java (8 hunks)
metadata-io/metadata-io-api/src/main/java/com/linkedin/metadata/entity/ebean/batch/ChangeItemImpl.java (7 hunks)
metadata-io/src/main/java/com/linkedin/metadata/aspect/utils/DefaultAspectsUtil.java (3 hunks)
metadata-io/src/main/java/com/linkedin/metadata/client/EntityClientAspectRetriever.java (2 hunks)
metadata-io/src/main/java/com/linkedin/metadata/entity/EntityServiceAspectRetriever.java (2 hunks)
metadata-io/src/main/java/com/linkedin/metadata/entity/EntityServiceImpl.java (8 hunks)
metadata-io/src/main/java/com/linkedin/metadata/entity/EntityUtils.java (4 hunks)
metadata-io/src/test/java/com/linkedin/metadata/AspectGenerationUtils.java (3 hunks)
metadata-io/src/test/java/com/linkedin/metadata/entity/EntityServiceTest.java (14 hunks)
metadata-io/src/test/java/com/linkedin/metadata/structuredproperties/validators/StructuredPropertiesValidatorTest.java (2 hunks)
metadata-models-custom/src/main/java/com/linkedin/metadata/aspect/plugins/spring/validation/CustomDataQualityRulesValidator.java (2 hunks)
metadata-models-custom/src/main/java/com/linkedin/metadata/aspect/plugins/validation/CustomDataQualityRulesValidator.java (2 hunks)
metadata-models/src/main/pegasus/com/linkedin/mxe/MetadataChangeProposal.pdl (1 hunks)
metadata-models/src/main/pegasus/com/linkedin/mxe/SystemMetadata.pdl (1 hunks)
metadata-models/src/main/resources/entity-registry.yml (1 hunks)
metadata-operation-context/src/main/java/io/datahubproject/test/metadata/context/TestOperationContexts.java (2 hunks)
metadata-service/openapi-entity-servlet/src/main/java/io/datahubproject/openapi/v2/delegates/EntityApiDelegateImpl.java (2 hunks)
metadata-service/openapi-servlet/models/src/main/java/io/datahubproject/openapi/client/OpenApiClient.java (4 hunks)
metadata-service/openapi-servlet/models/src/main/java/io/datahubproject/openapi/models/GenericAspect.java (1 hunks)
metadata-service/openapi-servlet/models/src/main/java/io/datahubproject/openapi/models/GenericEntity.java (1 hunks)
metadata-service/openapi-servlet/models/src/main/java/io/datahubproject/openapi/models/GenericEntityScrollResult.java (1 hunks)
metadata-service/openapi-servlet/models/src/main/java/io/datahubproject/openapi/v2/models/BatchGetUrnRequestV2.java (2 hunks)
metadata-service/openapi-servlet/models/src/main/java/io/datahubproject/openapi/v2/models/BatchGetUrnResponseV2.java (2 hunks)
metadata-service/openapi-servlet/models/src/main/java/io/datahubproject/openapi/v2/models/GenericAspectV2.java (1 hunks)
metadata-service/openapi-servlet/models/src/main/java/io/datahubproject/openapi/v2/models/GenericEntityScrollResultV2.java (1 hunks)
metadata-service/openapi-servlet/models/src/main/java/io/datahubproject/openapi/v2/models/GenericEntityV2.java (2 hunks)
metadata-service/openapi-servlet/models/src/main/java/io/datahubproject/openapi/v3/models/GenericAspectV3.java (1 hunks)
metadata-service/openapi-servlet/models/src/main/java/io/datahubproject/openapi/v3/models/GenericEntityScrollResultV3.java (1 hunks)
metadata-service/openapi-servlet/models/src/main/java/io/datahubproject/openapi/v3/models/GenericEntityV3.java (3 hunks)
metadata-service/openapi-servlet/src/main/java/io/datahubproject/openapi/controller/GenericEntitiesController.java (8 hunks)
metadata-service/openapi-servlet/src/main/java/io/datahubproject/openapi/v2/controller/EntityController.java (4 hunks)
metadata-service/openapi-servlet/src/main/java/io/datahubproject/openapi/v3/OpenAPIV3Generator.java (8 hunks)
metadata-service/openapi-servlet/src/main/java/io/datahubproject/openapi/v3/controller/EntityController.java (5 hunks)
metadata-service/restli-api/src/main/snapshot/com.linkedin.entity.aspects.snapshot.json (2 hunks)
metadata-service/restli-api/src/main/snapshot/com.linkedin.entity.entities.snapshot.json (1 hunks)
metadata-service/restli-api/src/main/snapshot/com.linkedin.entity.entitiesV2.snapshot.json (1 hunks)
metadata-service/restli-api/src/main/snapshot/com.linkedin.entity.entitiesVersionedV2.snapshot.json (1 hunks)
metadata-service/restli-client-api/src/main/java/com/linkedin/entity/client/EntityClient.java (3 hunks)
metadata-service/services/src/main/java/com/linkedin/metadata/entity/EntityService.java (1 hunks)
metadata-utils/src/main/java/com/linkedin/metadata/utils/GenericRecordUtils.java (2 hunks)

Files not processed due to max files limit (1)

metadata-utils/src/main/java/com/linkedin/metadata/utils/metrics/ExceptionUtils.java

Files skipped from review due to trivial changes (2)

metadata-io/src/test/java/com/linkedin/metadata/structuredproperties/validators/StructuredPropertiesValidatorTest.java
metadata-models/src/main/resources/entity-registry.yml

Files skipped from review as they are similar to previous changes (45)

entity-registry/src/main/java/com/linkedin/metadata/aspect/AspectRetriever.java
entity-registry/src/main/java/com/linkedin/metadata/aspect/EnvelopedSystemAspect.java
entity-registry/src/main/java/com/linkedin/metadata/aspect/SystemAspect.java
entity-registry/src/main/java/com/linkedin/metadata/aspect/batch/MCPItem.java
entity-registry/src/main/java/com/linkedin/metadata/aspect/plugins/validation/AspectValidationException.java
entity-registry/src/main/java/com/linkedin/metadata/aspect/plugins/validation/ValidationExceptionCollection.java
entity-registry/src/main/java/com/linkedin/metadata/aspect/validation/ConditionalWriteValidator.java
entity-registry/src/test/java/com/linkedin/metadata/aspect/validators/ConditionalWriteValidatorTest.java
entity-registry/src/testFixtures/java/com/linkedin/test/metadata/aspect/MockAspectRetriever.java
entity-registry/src/testFixtures/java/com/linkedin/test/metadata/aspect/batch/TestMCP.java
metadata-integration/java/datahub-event/src/main/resources/MetadataChangeProposal.avsc
metadata-integration/java/openlineage-converter/build.gradle
metadata-io/metadata-io-api/src/main/java/com/linkedin/metadata/entity/ebean/batch/ChangeItemImpl.java
metadata-io/src/main/java/com/linkedin/metadata/aspect/utils/DefaultAspectsUtil.java
metadata-io/src/main/java/com/linkedin/metadata/client/EntityClientAspectRetriever.java
metadata-io/src/main/java/com/linkedin/metadata/entity/EntityServiceAspectRetriever.java
metadata-io/src/main/java/com/linkedin/metadata/entity/EntityServiceImpl.java
metadata-io/src/main/java/com/linkedin/metadata/entity/EntityUtils.java
metadata-io/src/test/java/com/linkedin/metadata/AspectGenerationUtils.java
metadata-io/src/test/java/com/linkedin/metadata/entity/EntityServiceTest.java
metadata-models-custom/src/main/java/com/linkedin/metadata/aspect/plugins/spring/validation/CustomDataQualityRulesValidator.java
metadata-models/src/main/pegasus/com/linkedin/mxe/MetadataChangeProposal.pdl
metadata-models/src/main/pegasus/com/linkedin/mxe/SystemMetadata.pdl
metadata-operation-context/src/main/java/io/datahubproject/test/metadata/context/TestOperationContexts.java
metadata-service/openapi-servlet/models/src/main/java/io/datahubproject/openapi/client/OpenApiClient.java
metadata-service/openapi-servlet/models/src/main/java/io/datahubproject/openapi/models/GenericAspect.java
metadata-service/openapi-servlet/models/src/main/java/io/datahubproject/openapi/models/GenericEntity.java
metadata-service/openapi-servlet/models/src/main/java/io/datahubproject/openapi/models/GenericEntityScrollResult.java
metadata-service/openapi-servlet/models/src/main/java/io/datahubproject/openapi/v2/models/BatchGetUrnRequestV2.java
metadata-service/openapi-servlet/models/src/main/java/io/datahubproject/openapi/v2/models/GenericAspectV2.java
metadata-service/openapi-servlet/models/src/main/java/io/datahubproject/openapi/v2/models/GenericEntityScrollResultV2.java
metadata-service/openapi-servlet/models/src/main/java/io/datahubproject/openapi/v2/models/GenericEntityV2.java
metadata-service/openapi-servlet/models/src/main/java/io/datahubproject/openapi/v3/models/GenericAspectV3.java
metadata-service/openapi-servlet/models/src/main/java/io/datahubproject/openapi/v3/models/GenericEntityScrollResultV3.java
metadata-service/openapi-servlet/models/src/main/java/io/datahubproject/openapi/v3/models/GenericEntityV3.java
metadata-service/openapi-servlet/src/main/java/io/datahubproject/openapi/controller/GenericEntitiesController.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/v3/OpenAPIV3Generator.java
metadata-service/restli-api/src/main/snapshot/com.linkedin.entity.aspects.snapshot.json
metadata-service/restli-api/src/main/snapshot/com.linkedin.entity.entities.snapshot.json
metadata-service/restli-api/src/main/snapshot/com.linkedin.entity.entitiesV2.snapshot.json
metadata-service/restli-api/src/main/snapshot/com.linkedin.entity.entitiesVersionedV2.snapshot.json
metadata-service/restli-client-api/src/main/java/com/linkedin/entity/client/EntityClient.java
metadata-service/services/src/main/java/com/linkedin/metadata/entity/EntityService.java
metadata-utils/src/main/java/com/linkedin/metadata/utils/GenericRecordUtils.java

Additional context used

LanguageTool

docs/advanced/mcp-mcl.md

[grammar] ~174-~174: There seems to be a noun/verb agreement error. Did you mean “writes” or “wrote”?
Context: ...es ### Conditional Writes Conditional write semantics use extra information contain...

(SINGULAR_NOUN_VERB_AGREEMENT)

[uncategorized] ~179-~179: Possible missing comma found.
Context: ...f-Version-Match Each time an aspect is updated a version is incremented to represent...

(AI_HYDRA_LEO_MISSING_COMMA)

[grammar] ~183-~183: The verb ‘write’ does not usually follow articles like ‘the’. Check that ‘write’ is spelled correctly; using ‘write’ as a noun may be non-standard.
Context: ...ctual version stored in the database, the write will fail. This prevents overwriting an...

(A_INFINITIVE)

[uncategorized] ~188-~188: This expression is usually spelled with a hyphen.
Context: ...using http header semantics. Similar to version based conditional writes this method can be u...

(BASED_HYPHEN)

[grammar] ~189-~189: The verb ‘write’ does not usually follow articles like ‘the’. Check that ‘write’ is spelled correctly; using ‘write’ as a noun may be non-standard.
Context: ...ites this method can be used to prevent the write if the target aspect was modified after...

(A_INFINITIVE)

[uncategorized] ~189-~189: Possible missing preposition found.
Context: ...get aspect was modified after a reading the aspect. Per the http specification date...

(AI_HYDRA_LEO_MISSING_OF)

[uncategorized] ~190-~190: Possible missing comma found.
Context: ...fter a reading the aspect. Per the http specification dates must comply with ISO-8601 standar...

(AI_HYDRA_LEO_MISSING_COMMA)

[uncategorized] ~192-~192: Loose punctuation mark.
Context: ...SO-8601 standard. If-Unmodified-Since: A writer can specify that the aspect mu...

(UNLIKELY_OPENING_PUNCTUATION)

docs/api/openapi/openapi-usage-guide.md

[typographical] ~122-~122: It appears that a comma is missing.
Context: ...ead of overwriting the entity. In this example we've added a URL parameter `createEnti...

(DURING_THAT_TIME_COMMA)

docs/how/updating-datahub.md

[style] ~25-~25: ‘with respect to’ might be wordy. Consider a shorter alternative.
Context: ...value` key and the API is now symmetric with respect to input and outputs. Example Global Tags...

(EN_WORDINESS_PREMIUM_WITH_RESPECT_TO)

Additional comments not posted (22)

metadata-service/openapi-servlet/models/src/main/java/io/datahubproject/openapi/v2/models/BatchGetUrnResponseV2.java (1)

Line range hint 1-19:
LGTM! The class structure and annotations are well-defined.

The BatchGetUrnResponseV2 class uses generics and appropriate annotations for JSON serialization and API documentation.

entity-registry/src/testFixtures/java/com/linkedin/test/metadata/aspect/batch/TestSystemAspect.java (1)

1-26: LGTM! The class structure and Lombok annotations are well-defined.

The TestSystemAspect class correctly implements SystemAspect and uses Lombok annotations to reduce boilerplate code.

metadata-models-custom/src/main/java/com/linkedin/metadata/aspect/plugins/validation/CustomDataQualityRulesValidator.java (2)

28-29: LGTM! The validation logic ensures at least one rule is present.

The validateProposedAspects method is well-structured and correctly enforces at least one rule in DataQualityRules.

58-59: LGTM! The validation logic ensures field type consistency.

The validatePreCommitAspects method is well-structured and correctly enforces field type consistency in DataQualityRules.

metadata-integration/java/datahub-client/src/main/resources/MetadataChangeProposal.avsc (2)

170-174: LGTM! The addition of the version field is well-structured.

The version field in SystemMetadata is correctly added with a detailed description and default value.

180-185: LGTM! The addition of the headers field is well-structured.

The headers field in MetadataChangeProposal is correctly added with a detailed description and default value.

docs/advanced/mcp-mcl.md (3)

63-66: Addition of headers field is correct.

The addition of the headers field in the MetadataChangeProposal PDL model aligns with the PR objectives and is well-documented.

70-70: Inclusion of headers field in proposal components is correct.

The headers field has been correctly added to the list of proposal components.

118-121: Explanation of headers field is clear and correct.

The explanation for the headers field in the documentation is clear and aligns with the PR objectives for supporting conditional write semantics.

metadata-integration/java/openlineage-converter/src/test/java/io/datahubproject/openlineage/HdfsPathDatasetTest.java (4)

16-17: Addition of TestNG imports is correct.

The imports for Assert and Test from TestNG have been correctly added.

45-45: Addition of env field is correct.

The env field has been correctly added to the PathSpec builder in the testPathSpecList test case.

92-92: Addition of env field is correct.

The env field has been correctly added to the PathSpec builder in the testNoMatchPathSpecList test case.

124-124: Addition of env field is correct.

The env field has been correctly added to the PathSpec builder in the testPathSpecListPlatformInstance test case.

metadata-service/openapi-servlet/src/main/java/io/datahubproject/openapi/v3/controller/EntityController.java (4)

3-5: Addition of imports is correct.

The imports for ConditionalWriteValidator and ApiOperation.READ have been correctly added.

33-36: Addition of imports is correct.

The imports for OperationContext, RequestContext, and InvalidUrnException have been correctly added.

71-109: Addition of getEntityBatch method is correct.

The getEntityBatch method has been correctly added to support batch retrieval of entities. The method includes necessary checks and logic for handling batch requests.

201-255: Addition of toEntityVersionRequest method is correct.

The toEntityVersionRequest method has been correctly added to parse entity version requests. The method includes necessary logic for parsing and validating entity version requests.

docs/api/openapi/openapi-usage-guide.md (1)

585-591: Addition of OpenAPI v3 Features section is correct.

The new section provides a comprehensive explanation of the OpenAPI v3 features and their usage, aligning with the PR objectives.

docs/how/updating-datahub.md (1)

26-56: LGTM!

The example clearly illustrates the changes in the Global Tags Aspect format.

metadata-service/openapi-entity-servlet/src/main/java/io/datahubproject/openapi/v2/delegates/EntityApiDelegateImpl.java (3)

73-75: LGTM!

The Lombok annotations @Getter, @Setter, and @Accessors(chain = true) improve code readability and maintainability.

Line range hint 82-93:
LGTM!

The class EntityApiDelegateImpl is well-structured, and the constructor initializes the fields appropriately.

93-93: LGTM!

The @Setter and @Getter annotations on the request field are consistent with the rest of the class.

docs/advanced/mcp-mcl.md

ajoymajumdar · 2024-07-11T15:34:18Z

...egistry/src/main/java/com/linkedin/metadata/aspect/validation/ConditionalWriteValidator.java

+  }
+
+  @Override
+  protected Stream<AspectValidationException> validatePreCommitAspects(


What is the transaction isolation? Since we are validating by doing a read-before-write and comparing the state, will there be a race-condition where 2 requests come in with the same nextVersion and passes this validation?

At least for ebean supported datastores (mysql, postgres, etc) the isolation level is repeatable-read. However I believe if both transactions write to version 0, the database will throw an exception because of the write isolation which is standard. In the code, this will trigger a re-execution in a new transaction for one of the two and it would then violate the precondition and fail during the retry.

)

* feat(forms) Handle deleting forms references when hard deleting forms (datahub-project#10820) * refactor(ui): Misc improvements to the setup ingestion flow (ingest uplift 1/2) (datahub-project#10764) Co-authored-by: John Joyce <[email protected]> Co-authored-by: John Joyce <[email protected]> * fix(ingestion/airflow-plugin): pipeline tasks discoverable in search (datahub-project#10819) * feat(ingest/transformer): tags to terms transformer (datahub-project#10758) Co-authored-by: Aseem Bansal <[email protected]> * fix(ingestion/unity-catalog): fixed issue with profiling with GE turned on (datahub-project#10752) Co-authored-by: Aseem Bansal <[email protected]> * feat(forms) Add java SDK for form entity PATCH + CRUD examples (datahub-project#10822) * feat(SDK) Add java SDK for structuredProperty entity PATCH + CRUD examples (datahub-project#10823) * feat(SDK) Add StructuredPropertyPatchBuilder in python sdk and provide sample CRUD files (datahub-project#10824) * feat(forms) Add CRUD endpoints to GraphQL for Form entities (datahub-project#10825) * add flag for includeSoftDeleted in scroll entities API (datahub-project#10831) * feat(deprecation) Return actor entity with deprecation aspect (datahub-project#10832) * feat(structuredProperties) Add CRUD graphql APIs for structured property entities (datahub-project#10826) * add scroll parameters to openapi v3 spec (datahub-project#10833) * fix(ingest): correct profile_day_of_week implementation (datahub-project#10818) * feat(ingest/glue): allow ingestion of empty databases from Glue (datahub-project#10666) Co-authored-by: Harshal Sheth <[email protected]> * feat(cli): add more details to get cli (datahub-project#10815) * fix(ingestion/glue): ensure date formatting works on all platforms for aws glue (datahub-project#10836) * fix(ingestion): fix datajob patcher (datahub-project#10827) * fix(smoke-test): add suffix in temp file creation (datahub-project#10841) * feat(ingest/glue): add helper method to permit user or group ownership (datahub-project#10784) * feat(): Show data platform instances in policy modal if they are set on the policy (datahub-project#10645) Co-authored-by: Hendrik Richert <[email protected]> * docs(patch): add patch documentation for how implementation works (datahub-project#10010) Co-authored-by: John Joyce <[email protected]> * fix(jar): add missing custom-plugin-jar task (datahub-project#10847) * fix(): also check exceptions/stack trace when filtering log messages (datahub-project#10391) Co-authored-by: John Joyce <[email protected]> * docs(): Update posts.md (datahub-project#9893) Co-authored-by: Hyejin Yoon <[email protected]> Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com> * chore(ingest): update acryl-datahub-classify version (datahub-project#10844) * refactor(ingest): Refactor structured logging to support infos, warnings, and failures structured reporting to UI (datahub-project#10828) Co-authored-by: John Joyce <[email protected]> Co-authored-by: Harshal Sheth <[email protected]> * fix(restli): log aspect-not-found as a warning rather than as an error (datahub-project#10834) * fix(ingest/nifi): remove duplicate upstream jobs (datahub-project#10849) * fix(smoke-test): test access to create/revoke personal access tokens (datahub-project#10848) * fix(smoke-test): missing test for move domain (datahub-project#10837) * ci: update usernames to not considered for community (datahub-project#10851) * env: change defaults for data contract visibility (datahub-project#10854) * fix(ingest/tableau): quote special characters in external URL (datahub-project#10842) * fix(smoke-test): fix flakiness of auto complete test * ci(ingest): pin dask dependency for feast (datahub-project#10865) * fix(ingestion/lookml): liquid template resolution and view-to-view cll (datahub-project#10542) * feat(ingest/audit): add client id and version in system metadata props (datahub-project#10829) * chore(ingest): Mypy 1.10.1 pin (datahub-project#10867) * docs: use acryl-datahub-actions as expected python package to install (datahub-project#10852) * docs: add new js snippet (datahub-project#10846) * refactor(ingestion): remove company domain for security reason (datahub-project#10839) * fix(ingestion/spark): Platform instance and column level lineage fix (datahub-project#10843) Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com> * feat(ingestion/tableau): optionally ingest multiple sites and create site containers (datahub-project#10498) Co-authored-by: Yanik Häni <[email protected]> * fix(ingestion/looker): Add sqlglot dependency and remove unused sqlparser (datahub-project#10874) * fix(manage-tokens): fix manage access token policy (datahub-project#10853) * Batch get entity endpoints (datahub-project#10880) * feat(system): support conditional write semantics (datahub-project#10868) * fix(build): upgrade vercel builds to Node 20.x (datahub-project#10890) * feat(ingest/lookml): shallow clone repos (datahub-project#10888) * fix(ingest/looker): add missing dependency (datahub-project#10876) * fix(ingest): only populate audit stamps where accurate (datahub-project#10604) * fix(ingest/dbt): always encode tag urns (datahub-project#10799) * fix(ingest/redshift): handle multiline alter table commands (datahub-project#10727) * fix(ingestion/looker): column name missing in explore (datahub-project#10892) * fix(lineage) Fix lineage source/dest filtering with explored per hop limit (datahub-project#10879) * feat(conditional-writes): misc updates and fixes (datahub-project#10901) * feat(ci): update outdated action (datahub-project#10899) * feat(rest-emitter): adding async flag to rest emitter (datahub-project#10902) Co-authored-by: Gabe Lyons <[email protected]> * feat(ingest): add snowflake-queries source (datahub-project#10835) * fix(ingest): improve `auto_materialize_referenced_tags_terms` error handling (datahub-project#10906) * docs: add new company to adoption list (datahub-project#10909) * refactor(redshift): Improve redshift error handling with new structured reporting system (datahub-project#10870) Co-authored-by: John Joyce <[email protected]> Co-authored-by: Harshal Sheth <[email protected]> * feat(ui) Finalize support for all entity types on forms (datahub-project#10915) * Index ExecutionRequestResults status field (datahub-project#10811) * feat(ingest): grafana connector (datahub-project#10891) Co-authored-by: Shirshanka Das <[email protected]> Co-authored-by: Harshal Sheth <[email protected]> * fix(gms) Add Form entity type to EntityTypeMapper (datahub-project#10916) * feat(dataset): add support for external url in Dataset (datahub-project#10877) * docs(saas-overview) added missing features to observe section (datahub-project#10913) Co-authored-by: John Joyce <[email protected]> * fix(ingest/spark): Fixing Micrometer warning (datahub-project#10882) * fix(structured properties): allow application of structured properties without schema file (datahub-project#10918) * fix(data-contracts-web) handle other schedule types (datahub-project#10919) * fix(ingestion/tableau): human-readable message for PERMISSIONS_MODE_SWITCHED error (datahub-project#10866) Co-authored-by: Harshal Sheth <[email protected]> * Add feature flag for view defintions (datahub-project#10914) Co-authored-by: Ethan Cartwright <[email protected]> * feat(ingest/BigQuery): refactor+parallelize dataset metadata extraction (datahub-project#10884) * fix(airflow): add error handling around render_template() (datahub-project#10907) * feat(ingestion/sqlglot): add optional `default_dialect` parameter to sqlglot lineage (datahub-project#10830) * feat(mcp-mutator): new mcp mutator plugin (datahub-project#10904) * fix(ingest/bigquery): changes helper function to decode unicode scape sequences (datahub-project#10845) * feat(ingest/postgres): fetch table sizes for profile (datahub-project#10864) * feat(ingest/abs): Adding azure blob storage ingestion source (datahub-project#10813) * fix(ingest/redshift): reduce severity of SQL parsing issues (datahub-project#10924) * fix(build): fix lint fix web react (datahub-project#10896) * fix(ingest/bigquery): handle quota exceeded for project.list requests (datahub-project#10912) * feat(ingest): report extractor failures more loudly (datahub-project#10908) * feat(ingest/snowflake): integrate snowflake-queries into main source (datahub-project#10905) * fix(ingest): fix docs build (datahub-project#10926) * fix(ingest/snowflake): fix test connection (datahub-project#10927) * fix(ingest/lookml): add view load failures to cache (datahub-project#10923) * docs(slack) overhauled setup instructions and screenshots (datahub-project#10922) Co-authored-by: John Joyce <[email protected]> * fix(airflow): Add comma parsing of owners to DataJobs (datahub-project#10903) * fix(entityservice): fix merging sideeffects (datahub-project#10937) * feat(ingest): Support System Ingestion Sources, Show and hide system ingestion sources with Command-S (datahub-project#10938) Co-authored-by: John Joyce <[email protected]> * chore() Set a default lineage filtering end time on backend when a start time is present (datahub-project#10925) Co-authored-by: John Joyce <[email protected]> Co-authored-by: John Joyce <[email protected]> * Added relationships APIs to V3. Added these generic APIs to V3 swagger doc. (datahub-project#10939) * docs: add learning center to docs (datahub-project#10921) * doc: Update hubspot form id (datahub-project#10943) * chore(airflow): add python 3.11 w/ Airflow 2.9 to CI (datahub-project#10941) * fix(ingest/Glue): column upstream lineage between S3 and Glue (datahub-project#10895) * fix(ingest/abs): split abs utils into multiple files (datahub-project#10945) * doc(ingest/looker): fix doc for sql parsing documentation (datahub-project#10883) Co-authored-by: Harshal Sheth <[email protected]> * fix(ingest/bigquery): Adding missing BigQuery types (datahub-project#10950) * fix(ingest/setup): feast and abs source setup (datahub-project#10951) * fix(connections) Harden adding /gms to connections in backend (datahub-project#10942) * feat(siblings) Add flag to prevent combining siblings in the UI (datahub-project#10952) * fix(docs): make graphql doc gen more automated (datahub-project#10953) * feat(ingest/athena): Add option for Athena partitioned profiling (datahub-project#10723) * fix(spark-lineage): default timeout for future responses (datahub-project#10947) * feat(datajob/flow): add environment filter using info aspects (datahub-project#10814) * fix(ui/ingest): correct privilege used to show tab (datahub-project#10483) Co-authored-by: Kunal-kankriya <[email protected]> * feat(ingest/looker): include dashboard urns in browse v2 (datahub-project#10955) * add a structured type to batchGet in OpenAPI V3 spec (datahub-project#10956) * fix(ui): scroll on the domain sidebar to show all domains (datahub-project#10966) * fix(ingest/sagemaker): resolve incorrect variable assignment for SageMaker API call (datahub-project#10965) * fix(airflow/build): Pinning mypy (datahub-project#10972) * Fixed a bug where the OpenAPI V3 spec was incorrect. The bug was introduced in datahub-project#10939. (datahub-project#10974) * fix(ingest/test): Fix for mssql integration tests (datahub-project#10978) * fix(entity-service) exist check correctly extracts status (datahub-project#10973) * fix(structuredProps) casing bug in StructuredPropertiesValidator (datahub-project#10982) * bugfix: use anyOf instead of allOf when creating references in openapi v3 spec (datahub-project#10986) * fix(ui): Remove ant less imports (datahub-project#10988) * feat(ingest/graph): Add get_results_by_filter to DataHubGraph (datahub-project#10987) * feat(ingest/cli): init does not actually support environment variables (datahub-project#10989) * fix(ingest/graph): Update get_results_by_filter graphql query (datahub-project#10991) * feat(ingest/spark): Promote beta plugin (datahub-project#10881) Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com> * feat(ingest): support domains in meta -> "datahub" section (datahub-project#10967) * feat(ingest): add `check server-config` command (datahub-project#10990) * feat(cli): Make consistent use of DataHubGraphClientConfig (datahub-project#10466) Deprecates get_url_and_token() in favor of a more complete option: load_graph_config() that returns a full DatahubClientConfig. This change was then propagated across previous usages of get_url_and_token so that connections to DataHub server from the client respect the full breadth of configuration specified by DatahubClientConfig. I.e: You can now specify disable_ssl_verification: true in your ~/.datahubenv file so that all cli functions to the server work when ssl certification is disabled. Fixes datahub-project#9705 * fix(ingest/s3): Fixing container creation when there is no folder in path (datahub-project#10993) * fix(ingest/looker): support platform instance for dashboards & charts (datahub-project#10771) * feat(ingest/bigquery): improve handling of information schema in sql parser (datahub-project#10985) * feat(ingest): improve `ingest deploy` command (datahub-project#10944) * fix(backend): allow excluding soft-deleted entities in relationship-queries; exclude soft-deleted members of groups (datahub-project#10920) - allow excluding soft-deleted entities in relationship-queries - exclude soft-deleted members of groups * fix(ingest/looker): downgrade missing chart type log level (datahub-project#10996) * doc(acryl-cloud): release docs for 0.3.4.x (datahub-project#10984) Co-authored-by: John Joyce <[email protected]> Co-authored-by: RyanHolstien <[email protected]> Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com> Co-authored-by: Pedro Silva <[email protected]> * fix(protobuf/build): Fix protobuf check jar script (datahub-project#11006) * fix(ui/ingest): Support invalid cron jobs (datahub-project#10998) * fix(ingest): fix graph config loading (datahub-project#11002) Co-authored-by: Pedro Silva <[email protected]> * feat(docs): Document __DATAHUB_TO_FILE_ directive (datahub-project#10968) Co-authored-by: Harshal Sheth <[email protected]> * fix(graphql/upsertIngestionSource): Validate cron schedule; parse error in CLI (datahub-project#11011) * feat(ece): support custom ownership type urns in ECE generation (datahub-project#10999) * feat(assertion-v2): changed Validation tab to Quality and created new Governance tab (datahub-project#10935) * fix(ingestion/glue): Add support for missing config options for profiling in Glue (datahub-project#10858) * feat(propagation): Add models for schema field docs, tags, terms (datahub-project#2959) (datahub-project#11016) Co-authored-by: Chris Collins <[email protected]> * docs: standardize terminology to DataHub Cloud (datahub-project#11003) * fix(ingestion/transformer): replace the externalUrl container (datahub-project#11013) * docs(slack) troubleshoot docs (datahub-project#11014) * feat(propagation): Add graphql API (datahub-project#11030) Co-authored-by: Chris Collins <[email protected]> * feat(propagation): Add models for Action feature settings (datahub-project#11029) * docs(custom properties): Remove duplicate from sidebar (datahub-project#11033) * feat(models): Introducing Dataset Partitions Aspect (datahub-project#10997) Co-authored-by: John Joyce <[email protected]> Co-authored-by: John Joyce <[email protected]> * feat(propagation): Add Documentation Propagation Settings (datahub-project#11038) * fix(models): chart schema fields mapping, add dataHubAction entity, t… (datahub-project#11040) * fix(ci): smoke test lint failures (datahub-project#11044) * docs: fix learning center color scheme & typo (datahub-project#11043) * feat: add cloud main page (datahub-project#11017) Co-authored-by: Jay <[email protected]> * feat(restore-indices): add additional step to also clear system metadata service (datahub-project#10662) Co-authored-by: John Joyce <[email protected]> * docs: fix typo (datahub-project#11046) * fix(lint): apply spotless (datahub-project#11050) * docs(airflow): example query to get datajobs for a dataflow (datahub-project#11034) * feat(cli): Add run-id option to put sub-command (datahub-project#11023) Adds an option to assign run-id to a given put command execution. This is useful when transformers do not exist for a given ingestion payload, we can follow up with custom metadata and assign it to an ingestion pipeline. * fix(ingest): improve sql error reporting calls (datahub-project#11025) * fix(airflow): fix CI setup (datahub-project#11031) * feat(ingest/dbt): add experimental `prefer_sql_parser_lineage` flag (datahub-project#11039) * fix(ingestion/lookml): enable stack-trace in lookml logs (datahub-project#10971) * (chore): Linting fix (datahub-project#11015) * chore(ci): update deprecated github actions (datahub-project#10977) * Fix ALB configuration example (datahub-project#10981) * chore(ingestion-base): bump base image packages (datahub-project#11053) * feat(cli): Trim report of dataHubExecutionRequestResult to max GMS size (datahub-project#11051) * fix(ingestion/lookml): emit dummy sql condition for lookml custom condition tag (datahub-project#11008) Co-authored-by: Harshal Sheth <[email protected]> * fix(ingestion/powerbi): fix issue with broken report lineage (datahub-project#10910) * feat(ingest/tableau): add retry on timeout (datahub-project#10995) * change generate kafka connect properties from env (datahub-project#10545) Co-authored-by: david-leifker <[email protected]> * fix(ingest): fix oracle cronjob ingestion (datahub-project#11001) Co-authored-by: david-leifker <[email protected]> * chore(ci): revert update deprecated github actions (datahub-project#10977) (datahub-project#11062) * feat(ingest/dbt-cloud): update metadata_endpoint inference (datahub-project#11041) * build: Reduce size of datahub-frontend-react image by 50-ish% (datahub-project#10878) Co-authored-by: david-leifker <[email protected]> * fix(ci): Fix lint issue in datahub_ingestion_run_summary_provider.py (datahub-project#11063) * docs(ingest): update developing-a-transformer.md (datahub-project#11019) * feat(search-test): update search tests from datahub-project#10408 (datahub-project#11056) * feat(cli): add aspects parameter to DataHubGraph.get_entity_semityped (datahub-project#11009) Co-authored-by: Harshal Sheth <[email protected]> * docs(airflow): update min version for plugin v2 (datahub-project#11065) * doc(ingestion/tableau): doc update for derived permission (datahub-project#11054) Co-authored-by: Pedro Silva <[email protected]> Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com> Co-authored-by: Harshal Sheth <[email protected]> * fix(py): remove dep on types-pkg_resources (datahub-project#11076) * feat(ingest/mode): add option to exclude restricted (datahub-project#11081) * fix(ingest): set lastObserved in sdk when unset (datahub-project#11071) * doc(ingest): Update capabilities (datahub-project#11072) * chore(vulnerability): Log Injection (datahub-project#11090) * chore(vulnerability): Information exposure through a stack trace (datahub-project#11091) * chore(vulnerability): Comparison of narrow type with wide type in loop condition (datahub-project#11089) * chore(vulnerability): Insertion of sensitive information into log files (datahub-project#11088) * chore(vulnerability): Risky Cryptographic Algorithm (datahub-project#11059) * chore(vulnerability): Overly permissive regex range (datahub-project#11061) Co-authored-by: Harshal Sheth <[email protected]> * fix: update customer data (datahub-project#11075) * fix(models): fixing the datasetPartition models (datahub-project#11085) Co-authored-by: John Joyce <[email protected]> * fix(ui): Adding view, forms GraphQL query, remove showing a fallback error message on unhandled GraphQL error (datahub-project#11084) Co-authored-by: John Joyce <[email protected]> * feat(docs-site): hiding learn more from cloud page (datahub-project#11097) * fix(docs): Add correct usage of orFilters in search API docs (datahub-project#11082) Co-authored-by: Jay <[email protected]> * fix(ingest/mode): Regexp in mode name matcher didn't allow underscore (datahub-project#11098) * docs: Refactor customer stories section (datahub-project#10869) Co-authored-by: Jeff Merrick <[email protected]> * fix(release): fix full/slim suffix on tag (datahub-project#11087) * feat(config): support alternate hashing algorithm for doc id (datahub-project#10423) Co-authored-by: david-leifker <[email protected]> Co-authored-by: John Joyce <[email protected]> * fix(emitter): fix typo in get method of java kafka emitter (datahub-project#11007) * fix(ingest): use correct native data type in all SQLAlchemy sources by compiling data type using dialect (datahub-project#10898) Co-authored-by: Harshal Sheth <[email protected]> * chore: Update contributors list in PR labeler (datahub-project#11105) * feat(ingest): tweak stale entity removal messaging (datahub-project#11064) * fix(ingestion): enforce lastObserved timestamps in SystemMetadata (datahub-project#11104) * fix(ingest/powerbi): fix broken lineage between chart and dataset (datahub-project#11080) * feat(ingest/lookml): CLL support for sql set in sql_table_name attribute of lookml view (datahub-project#11069) * docs: update graphql docs on forms & structured properties (datahub-project#11100) * test(search): search openAPI v3 test (datahub-project#11049) * fix(ingest/tableau): prevent empty site content urls (datahub-project#11057) Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com> * feat(entity-client): implement client batch interface (datahub-project#11106) * fix(snowflake): avoid reporting warnings/info for sys tables (datahub-project#11114) * fix(ingest): downgrade column type mapping warning to info (datahub-project#11115) * feat(api): add AuditStamp to the V3 API entity/aspect response (datahub-project#11118) * fix(ingest/redshift): replace r'\n' with '\n' to avoid token error redshift serverless… (datahub-project#11111) * fix(entiy-client): handle null entityUrn case for restli (datahub-project#11122) * fix(sql-parser): prevent bad urns from alter table lineage (datahub-project#11092) * fix(ingest/bigquery): use small batch size if use_tables_list_query_v2 is set (datahub-project#11121) * fix(graphql): add missing entities to EntityTypeMapper and EntityTypeUrnMapper (datahub-project#10366) * feat(ui): Changes to allow editable dataset name (datahub-project#10608) Co-authored-by: Jay Kadambi <[email protected]> * fix: remove saxo (datahub-project#11127) * feat(mcl-processor): Update mcl processor hooks (datahub-project#11134) * fix(openapi): fix openapi v2 endpoints & v3 documentation update * Revert "fix(openapi): fix openapi v2 endpoints & v3 documentation update" This reverts commit 573c1cb. * docs(policies): updates to policies documentation (datahub-project#11073) * fix(openapi): fix openapi v2 and v3 docs update (datahub-project#11139) * feat(auth): grant type and acr values custom oidc parameters support (datahub-project#11116) * fix(mutator): mutator hook fixes (datahub-project#11140) * feat(search): support sorting on multiple fields (datahub-project#10775) * feat(ingest): various logging improvements (datahub-project#11126) * fix(ingestion/lookml): fix for sql parsing error (datahub-project#11079) Co-authored-by: Harshal Sheth <[email protected]> * feat(docs-site) cloud page spacing and content polishes (datahub-project#11141) * feat(ui) Enable editing structured props on fields (datahub-project#11042) * feat(tests): add md5 and last computed to testResult model (datahub-project#11117) * test(openapi): openapi regression smoke tests (datahub-project#11143) * fix(airflow): fix tox tests + update docs (datahub-project#11125) * docs: add chime to adoption stories (datahub-project#11142) * fix(ingest/databricks): Updating code to work with Databricks sdk 0.30 (datahub-project#11158) * fix(kafka-setup): add missing script to image (datahub-project#11190) * fix(config): fix hash algo config (datahub-project#11191) * test(smoke-test): updates to smoke-tests (datahub-project#11152) * fix(elasticsearch): refactor idHashAlgo setting (datahub-project#11193) * chore(kafka): kafka version bump (datahub-project#11211) * readd UsageStatsWorkUnit * fix merge problems * change logo --------- Co-authored-by: Chris Collins <[email protected]> Co-authored-by: John Joyce <[email protected]> Co-authored-by: John Joyce <[email protected]> Co-authored-by: John Joyce <[email protected]> Co-authored-by: dushayntAW <[email protected]> Co-authored-by: sagar-salvi-apptware <[email protected]> Co-authored-by: Aseem Bansal <[email protected]> Co-authored-by: Kevin Chun <[email protected]> Co-authored-by: jordanjeremy <[email protected]> Co-authored-by: skrydal <[email protected]> Co-authored-by: Harshal Sheth <[email protected]> Co-authored-by: david-leifker <[email protected]> Co-authored-by: sid-acryl <[email protected]> Co-authored-by: Julien Jehannet <[email protected]> Co-authored-by: Hendrik Richert <[email protected]> Co-authored-by: Hendrik Richert <[email protected]> Co-authored-by: RyanHolstien <[email protected]> Co-authored-by: Felix Lüdin <[email protected]> Co-authored-by: Pirry <[email protected]> Co-authored-by: Hyejin Yoon <[email protected]> Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com> Co-authored-by: cburroughs <[email protected]> Co-authored-by: ksrinath <[email protected]> Co-authored-by: Mayuri Nehate <[email protected]> Co-authored-by: Kunal-kankriya <[email protected]> Co-authored-by: Shirshanka Das <[email protected]> Co-authored-by: ipolding-cais <[email protected]> Co-authored-by: Tamas Nemeth <[email protected]> Co-authored-by: Shubham Jagtap <[email protected]> Co-authored-by: haeniya <[email protected]> Co-authored-by: Yanik Häni <[email protected]> Co-authored-by: Gabe Lyons <[email protected]> Co-authored-by: Gabe Lyons <[email protected]> Co-authored-by: 808OVADOZE <[email protected]> Co-authored-by: noggi <[email protected]> Co-authored-by: Nicholas Pena <[email protected]> Co-authored-by: Jay <[email protected]> Co-authored-by: ethan-cartwright <[email protected]> Co-authored-by: Ethan Cartwright <[email protected]> Co-authored-by: Nadav Gross <[email protected]> Co-authored-by: Patrick Franco Braz <[email protected]> Co-authored-by: pie1nthesky <[email protected]> Co-authored-by: Joel Pinto Mata (KPN-DSH-DEX team) <[email protected]> Co-authored-by: Ellie O'Neil <[email protected]> Co-authored-by: Ajoy Majumdar <[email protected]> Co-authored-by: deepgarg-visa <[email protected]> Co-authored-by: Tristan Heisler <[email protected]> Co-authored-by: Andrew Sikowitz <[email protected]> Co-authored-by: Davi Arnaut <[email protected]> Co-authored-by: Pedro Silva <[email protected]> Co-authored-by: amit-apptware <[email protected]> Co-authored-by: Sam Black <[email protected]> Co-authored-by: Raj Tekal <[email protected]> Co-authored-by: Steffen Grohsschmiedt <[email protected]> Co-authored-by: jaegwon.seo <[email protected]> Co-authored-by: Renan F. Lima <[email protected]> Co-authored-by: Matt Exchange <[email protected]> Co-authored-by: Jonny Dixon <[email protected]> Co-authored-by: Pedro Silva <[email protected]> Co-authored-by: Pinaki Bhattacharjee <[email protected]> Co-authored-by: Jeff Merrick <[email protected]> Co-authored-by: skrydal <[email protected]> Co-authored-by: AndreasHegerNuritas <[email protected]> Co-authored-by: jayasimhankv <[email protected]> Co-authored-by: Jay Kadambi <[email protected]> Co-authored-by: David Leifker <[email protected]>

github-actions bot added ingestion PR or Issue related to the ingestion of metadata product PR or Issue related to the DataHub UI/UX devops PR or Issue related to DataHub backend & deployment labels Jul 8, 2024

vercel bot had a problem deploying to Preview July 8, 2024 23:17 Failure

kevin1chun reviewed Jul 8, 2024

View reviewed changes

...egistry/src/main/java/com/linkedin/metadata/aspect/validation/ConditionalWriteValidator.java Show resolved Hide resolved

kevin1chun reviewed Jul 8, 2024

View reviewed changes

david-leifker force-pushed the conditional-write branch 2 times, most recently from 5bc178b to 53ea504 Compare July 10, 2024 01:04

vercel bot deployed to Preview July 10, 2024 01:31 View deployment

david-leifker force-pushed the conditional-write branch from 53ea504 to c7b4002 Compare July 10, 2024 02:30

david-leifker marked this pull request as ready for review July 10, 2024 02:30

david-leifker commented Jul 10, 2024

View reviewed changes

...a/openlineage-converter/src/test/java/io/datahubproject/openlineage/HdfsPathDatasetTest.java Show resolved Hide resolved

coderabbitai bot reviewed Jul 10, 2024

View reviewed changes

vercel bot deployed to Preview July 10, 2024 02:45 View deployment

kevin1chun requested a review from ajoymajumdar July 10, 2024 20:07

david-leifker force-pushed the conditional-write branch from c7b4002 to a73f076 Compare July 10, 2024 21:12

david-leifker requested a review from kevin1chun July 10, 2024 21:13

vercel bot had a problem deploying to Preview July 10, 2024 21:23 Failure

coderabbitai bot reviewed Jul 10, 2024

View reviewed changes

kevin1chun reviewed Jul 10, 2024

View reviewed changes

docs/how/updating-datahub.md

"systemMetadata": {},

"headers": {}

}

```

Copy link

Contributor

kevin1chun Jul 10, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

symmetric input & output is easier to work with :)

kevin1chun reviewed Jul 10, 2024

View reviewed changes

...o/metadata-io-api/src/main/java/com/linkedin/metadata/entity/ebean/batch/ChangeItemImpl.java Show resolved Hide resolved

kevin1chun reviewed Jul 10, 2024

View reviewed changes

RyanHolstien approved these changes Jul 10, 2024

View reviewed changes

kevin1chun approved these changes Jul 10, 2024

View reviewed changes

david-leifker force-pushed the conditional-write branch from a73f076 to e6b9d5e Compare July 11, 2024 04:04

vercel bot had a problem deploying to Preview July 11, 2024 04:04 Failure

coderabbitai bot reviewed Jul 11, 2024

View reviewed changes

ajoymajumdar reviewed Jul 11, 2024

View reviewed changes

feat(system): support conditional write semantics

5d9a518

* introduce If-Modified-Since, If-Unmodified-Since, If-Version-Match * ConditionalWriteValidator added * MCP headers field added * SystemMetadata version field added * batchGet with If-Version-Match support

david-leifker force-pushed the conditional-write branch from e6b9d5e to 5d9a518 Compare July 11, 2024 14:58

vercel bot had a problem deploying to Preview July 11, 2024 14:58 Failure

ajoymajumdar reviewed Jul 11, 2024

View reviewed changes

coderabbitai bot reviewed Jul 11, 2024

View reviewed changes

docs/advanced/mcp-mcl.md Show resolved Hide resolved

docs/advanced/mcp-mcl.md Show resolved Hide resolved

ajoymajumdar reviewed Jul 11, 2024

View reviewed changes

david-leifker merged commit 5327f80 into datahub-project:master Jul 11, 2024
65 of 66 checks passed

yoonhyejin pushed a commit that referenced this pull request Jul 16, 2024

feat(system): support conditional write semantics (#10868)

bb7f96a

coderabbitai bot mentioned this pull request Jul 19, 2024

feat(datajob/flow): add environment filter using info aspects #10814

Merged

5 tasks

aviv-julienjehannet pushed a commit to aviv-julienjehannet/datahub that referenced this pull request Jul 25, 2024

feat(system): support conditional write semantics (datahub-project#10868

cdd0ab2

)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(system): support conditional write semantics #10868

feat(system): support conditional write semantics #10868

david-leifker commented Jul 8, 2024 •

edited by coderabbitai bot

Loading

coderabbitai bot commented Jul 8, 2024 •

edited

Loading

Chat

CodeRabbit Commands (invoked as PR comments)

CodeRabbit Configration File (`.coderabbit.yaml`)

Documentation and Community

kevin1chun Jul 8, 2024

david-leifker Jul 8, 2024

david-leifker Jul 8, 2024 •

edited

Loading

kevin1chun Jul 10, 2024

david-leifker Jul 10, 2024

kevin1chun Jul 10, 2024 •

edited

Loading

coderabbitai bot left a comment

coderabbitai bot left a comment

kevin1chun Jul 10, 2024

kevin1chun Jul 10, 2024

kevin1chun Jul 10, 2024

david-leifker Jul 10, 2024

kevin1chun Jul 10, 2024

kevin1chun Jul 10, 2024

david-leifker Jul 10, 2024

RyanHolstien left a comment

RyanHolstien Jul 10, 2024

david-leifker Jul 10, 2024

coderabbitai bot left a comment

ajoymajumdar Jul 11, 2024

david-leifker Jul 11, 2024

ajoymajumdar Jul 11, 2024

david-leifker Jul 11, 2024 •

edited

Loading

ajoymajumdar Jul 11, 2024 •

edited

Loading

david-leifker Jul 11, 2024

ajoymajumdar Jul 11, 2024

coderabbitai bot left a comment

ajoymajumdar Jul 11, 2024

david-leifker Jul 11, 2024

@@ @@ -52,4 +55,25 @@ public Map<Urn, Map<String, Aspect>> getLatestAspectObjects( @@
                     }
                   }
                 }

		in `SystemMetadata`.

		A writer can provide a header with the expected `version` when initiating the request. If the expected `version` does not

feat(system): support conditional write semantics #10868

feat(system): support conditional write semantics #10868

Conversation

david-leifker commented Jul 8, 2024 • edited by coderabbitai bot Loading

Checklist

Summary by CodeRabbit

coderabbitai bot commented Jul 8, 2024 • edited Loading

Walkthrough

Changes

Poem

Chat

CodeRabbit Commands (invoked as PR comments)

CodeRabbit Configration File (.coderabbit.yaml)

Documentation and Community

Choose a reason for hiding this comment

Choose a reason for hiding this comment

david-leifker Jul 8, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kevin1chun Jul 10, 2024 • edited Loading

Choose a reason for hiding this comment

coderabbitai bot left a comment

Choose a reason for hiding this comment

coderabbitai bot left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RyanHolstien left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coderabbitai bot left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

david-leifker Jul 11, 2024 • edited Loading

Choose a reason for hiding this comment

ajoymajumdar Jul 11, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coderabbitai bot left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

david-leifker commented Jul 8, 2024 •

edited by coderabbitai bot

Loading

coderabbitai bot commented Jul 8, 2024 •

edited

Loading

CodeRabbit Configration File (`.coderabbit.yaml`)

david-leifker Jul 8, 2024 •

edited

Loading

kevin1chun Jul 10, 2024 •

edited

Loading

david-leifker Jul 11, 2024 •

edited

Loading

ajoymajumdar Jul 11, 2024 •

edited

Loading