Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(custom-plugins): improve plugin factory merge #10796

Merged

Conversation

david-leifker
Copy link
Collaborator

@david-leifker david-leifker commented Jun 27, 2024

  • added additional tests
  • added the ability for Spring Plugin class to produce multiple plugin beans

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 new methods for plugin handling, including isEmpty() and hasLoadedPlugins(), enhancing the plugin management experience.
  • Bug Fixes

    • Improved merging logic for plugins to handle loading and merging more efficiently.
  • Tests

    • Added new test cases to ensure the proper functioning of empty and unloaded plugin merges.
    • Introduced comprehensive tests for Spring plugin factory configurations and entity registry with specific aspect configurations.
  • Chores

    • Updated test dependencies for better testing coverage and compatibility.

@github-actions github-actions bot added the devops PR or Issue related to DataHub backend & deployment label Jun 27, 2024
Copy link
Contributor

coderabbitai bot commented Jun 27, 2024

Walkthrough

The update enhances plugin management in the metadata service, with the primary changes including improved merging logic for PluginFactory instances, additional methods for plugin state checking, and new configurations for aspect handling in Spring. This involves updating plugin loading mechanisms, adding configurations for testing, and improving test dependencies and coverage.

Changes

Files/Groups of Files Change Summary
PluginFactory.java Added new methods for checking plugin states and enhanced merging logic.
PluginConfiguration.java Added isEmpty() method to check list emptiness in configurations.
PluginsTest.java Introduced new test methods to validate plugin merging logic.
metadata-service/plugin/build.gradle Updated test dependencies including Spring Boot Test and Lombok annotations.
SpringPluginFactory.java Modified to manage multiple plugins and updated logic for plugin creation and configuration.
SpringPluginFactoryTest.java, TestSpringPluginConfiguration.java Added new classes and methods for testing Spring-based plugin handling and configurations.
test-entity-registry-plugins-1.yml, test-entity-registry-spring-plugins-1.yml Introduced configurations for aspect payload validators, mutation hooks, and side effects.

Sequence Diagram(s)

sequenceDiagram
    actor Developer as Developer
    participant PluginFactory
    participant PluginConfiguration
    participant SpringPluginFactory
    participant Test

    Developer->>PluginFactory: Construct PluginFactory with plugins
    PluginFactory->>PluginConfiguration: Check isEmpty()
    PluginConfiguration-->>PluginFactory: Response

    Developer->>PluginFactory: Merge Plugins
    PluginFactory->>PluginFactory: Merge Logic

    Developer->>SpringPluginFactory: Load Plugins with Spring Context
    SpringPluginFactory->>SpringContext: Fetch Plugin Beans
    SpringContext-->>SpringPluginFactory: Return Beans

    Developer->>Test: Execute Tests
    Test->>PluginFactory: Validate Merged Plugins
    Test->>SpringPluginFactory: Validate Loaded Plugins
Loading

Poem

In the land of code and bytes,
Plugins merge with newfound might.
Spring and merge, they dance anew,
With tests and configs, many tasks to do.
Enhanced and lively, the system sings,
As integration’s magic brings.
🐇✨ Let's hop in joy, for all these things!


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.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 7

Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

Commits

Files that changed from the base of the PR and between d63f25f and 62fb66e.

Files selected for processing (9)
  • entity-registry/src/main/java/com/linkedin/metadata/aspect/plugins/PluginFactory.java (3 hunks)
  • entity-registry/src/main/java/com/linkedin/metadata/aspect/plugins/config/PluginConfiguration.java (1 hunks)
  • entity-registry/src/test/java/com/linkedin/metadata/aspect/plugins/PluginsTest.java (1 hunks)
  • metadata-service/plugin/build.gradle (1 hunks)
  • metadata-service/plugin/src/main/java/com/datahub/plugins/metadata/aspect/SpringPluginFactory.java (1 hunks)
  • metadata-service/plugin/src/test/java/com/datahub/plugins/metadata/aspect/SpringPluginFactoryTest.java (1 hunks)
  • metadata-service/plugin/src/test/java/com/datahub/plugins/metadata/aspect/test/TestSpringPluginConfiguration.java (1 hunks)
  • metadata-service/plugin/src/test/resources/test-entity-registry-plugins-1.yml (1 hunks)
  • metadata-service/plugin/src/test/resources/test-entity-registry-spring-plugins-1.yml (1 hunks)
Additional comments not posted (15)
metadata-service/plugin/build.gradle (2)

24-24: Approval of springBootTest dependency.

The addition of springBootTest aligns with the enhancements to Spring-related testing functionalities.


22-23: Approve addition of test-models dependencies.

The inclusion of test-models with a specific configuration testDataTemplate is appropriate for providing tailored models for testing. Ensure these dependencies are correctly integrated into the testing setup.

metadata-service/plugin/src/test/resources/test-entity-registry-spring-plugins-1.yml (3)

4-16: Well-defined configuration for aspectPayloadValidators.

The detailed configuration for various operations and entity aspects ensures comprehensive testing of the plugin's capabilities.


17-28: Appropriate configuration for mutationHooks.

This configuration provides a robust setup for testing mutation hooks across different operations and entity aspects.


29-52: Configuration for mclSideEffects and mcpSideEffects approved.

These configurations are crucial for testing the new side effect functionalities in various scenarios. Verify their correct application in the respective tests.

metadata-service/plugin/src/test/resources/test-entity-registry-plugins-1.yml (3)

18-43: Well-defined configuration for aspectPayloadValidators.

The detailed configuration for various operations and entity aspects ensures comprehensive testing of the plugin's capabilities.


44-51: Appropriate configuration for mutationHooks.

This configuration provides a robust setup for testing mutation hooks across different operations and entity aspects.


52-67: Configuration for mclSideEffects and mcpSideEffects approved.

These configurations are crucial for testing the new side effect functionalities in various scenarios. Verify their correct application in the respective tests.

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

30-34: Correct implementation of isEmpty method.

The method efficiently checks if all plugin configurations are empty using short-circuiting logic. This is a useful enhancement for easily verifying the presence of configured plugins.

metadata-service/plugin/src/test/java/com/datahub/plugins/metadata/aspect/test/TestSpringPluginConfiguration.java (4)

14-29: Well-configured springValidator1 bean.

The method correctly sets up a TestValidator with detailed configurations for operations and entity aspects, ensuring thorough testing.


32-48: Appropriately broad configuration in springValidator2.

This method extends the testing scope by configuring a TestValidator for all entities, which is beneficial for broader validation scenarios.


52-67: Correct configuration for springMutator.

This method sets up a TestMutator with configurations for multiple operations, ensuring comprehensive testing of mutation functionalities.


70-106: Well-configured side effect beans springMCPSideEffect and springMCLSideEffect.

These methods correctly set up side effects for specific entities and operations, facilitating comprehensive testing of new functionalities.

metadata-service/plugin/src/test/java/com/datahub/plugins/metadata/aspect/SpringPluginFactoryTest.java (1)

51-123: Comprehensive Test Coverage for Merged Entity Registry

This test method testMergedEntityRegistryWithSpringPluginFactory provides comprehensive coverage for the functionality of merging entity registries with Spring plugin factory configurations. It checks for the correct merging of entity and event specs, as well as the application of aspect payload validators and mutation hooks.

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

175-181: Validate isEmpty Method Logic

The isEmpty method checks if the plugin factory has no configured plugins. The use of Optional and stream operations is appropriate here, but ensure that this method accurately reflects the state of the factory, considering all types of plugins (validators, hooks, side effects).

Verification successful

Validate isEmpty Method Logic

The isEmpty method in the PluginFactory class accurately checks if the factory has no configured plugins by verifying that pluginConfiguration, aspectPayloadValidators, mutationHooks, mclSideEffects, and mcpSideEffects are empty. The use of Optional and List::isEmpty is appropriate and correctly implemented.

  • pluginConfiguration is checked directly for emptiness.
  • aspectPayloadValidators, mutationHooks, mclSideEffects, and mcpSideEffects are checked using Optional.ofNullable(...).map(List::isEmpty).orElse(true).

The method accurately reflects the state of the factory.

Scripts executed

The following scripts were executed for the analysis:


Script:

#!/bin/bash
# Description: Verify the isEmpty method logic is accurate.

# Test: Search for the function usage. Expect: Only occurances of the new signature.
rg --type java $'isEmpty'

Length of output: 66746



Script:

#!/bin/bash
# Extract the definition of the PluginFactory class and its attributes to verify the isEmpty method's logic.
ast-grep --lang java --pattern $'class PluginFactory {
  $_
}' --multi

Length of output: 268



Script:

#!/bin/bash
# Extract the definition of the PluginFactory class and its attributes to verify the isEmpty method's logic.
ast-grep --lang java --pattern $'class PluginFactory {
  $_
}'

Length of output: 63



Script:

#!/bin/bash
# Search for the definition of the PluginFactory class and its attributes to verify the isEmpty method's logic.
rg 'class PluginFactory' -A 20

Length of output: 2604



Script:

#!/bin/bash
# Search for the definition of the PluginFactory class and its attributes to verify the isEmpty method's logic.
rg 'class PluginFactory' -A 100

Length of output: 13033

Comment on lines +109 to +129
final List<T> plugins;
if (config.getSpring().getName() == null) {
plugin = (T) springApplicationContext.getBean(clazz);
plugins =
springApplicationContext.getBeansOfType(clazz).values().stream()
.map(plugin -> (T) plugin)
.collect(Collectors.toList());
} else {
plugin = (T) springApplicationContext.getBean(config.getSpring().getName(), clazz);
plugins =
List.of((T) springApplicationContext.getBean(config.getSpring().getName(), clazz));
}

if (plugin.enabled()) {
result.add((T) plugin.setConfig(config));
}
plugins.stream()
.filter(plugin -> plugin.enabled())
.forEach(
plugin -> {
if (plugin.getConfig() != null) {
result.add(plugin);
} else {
result.add((T) plugin.setConfig(config));
}
});
Copy link
Contributor

Choose a reason for hiding this comment

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

Refactor Suggestion: Streamline Plugin Collection Logic

The logic for collecting plugins from the Spring context is sound, but it can be streamlined for better readability and performance. Consider using a single stream operation for both cases (with and without specific names) to reduce redundancy and improve maintainability.

-          final List<T> plugins;
-          if (config.getSpring().getName() == null) {
-            plugins =
-                springApplicationContext.getBeansOfType(clazz).values().stream()
-                    .map(plugin -> (T) plugin)
-                    .collect(Collectors.toList());
-          } else {
-            plugins =
-                List.of((T) springApplicationContext.getBean(config.getSpring().getName(), clazz));
-          }
-
-          plugins.stream()
-              .filter(plugin -> plugin.enabled())
-              .forEach(
-                  plugin -> {
-                    if (plugin.getConfig() != null) {
-                      result.add(plugin);
-                    } else {
-                      result.add((T) plugin.setConfig(config));
-                    }
-                  });
+          List<T> plugins = config.getSpring().getName() == null
+              ? springApplicationContext.getBeansOfType(clazz).values().stream()
+                  .map(plugin -> (T) plugin)
+                  .collect(Collectors.toList())
+              : List.of((T) springApplicationContext.getBean(config.getSpring().getName(), clazz));
+
+          result.addAll(plugins.stream()
+              .filter(plugin -> plugin.enabled())
+              .map(plugin -> plugin.getConfig() != null ? plugin : (T) plugin.setConfig(config))
+              .collect(Collectors.toList()));
Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
final List<T> plugins;
if (config.getSpring().getName() == null) {
plugin = (T) springApplicationContext.getBean(clazz);
plugins =
springApplicationContext.getBeansOfType(clazz).values().stream()
.map(plugin -> (T) plugin)
.collect(Collectors.toList());
} else {
plugin = (T) springApplicationContext.getBean(config.getSpring().getName(), clazz);
plugins =
List.of((T) springApplicationContext.getBean(config.getSpring().getName(), clazz));
}
if (plugin.enabled()) {
result.add((T) plugin.setConfig(config));
}
plugins.stream()
.filter(plugin -> plugin.enabled())
.forEach(
plugin -> {
if (plugin.getConfig() != null) {
result.add(plugin);
} else {
result.add((T) plugin.setConfig(config));
}
});
List<T> plugins = config.getSpring().getName() == null
? springApplicationContext.getBeansOfType(clazz).values().stream()
.map(plugin -> (T) plugin)
.collect(Collectors.toList())
: List.of((T) springApplicationContext.getBean(config.getSpring().getName(), clazz));
result.addAll(plugins.stream()
.filter(plugin -> plugin.enabled())
.map(plugin -> plugin.getConfig() != null ? plugin : (T) plugin.setConfig(config))
.collect(Collectors.toList()));

Comment on lines +44 to +48
@BeforeTest
public void disableAssert() {
PathSpecBasedSchemaAnnotationVisitor.class
.getClassLoader()
.setClassAssertionStatus(PathSpecBasedSchemaAnnotationVisitor.class.getName(), false);
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Consider Improving Test Setup Method

The method disableAssert is used to disable assertions for a specific class loader. While this might be necessary for certain tests, disabling assertions globally can lead to missing out on potential issues during testing. Consider limiting the scope of this change or documenting why it is necessary.

Comment on lines +126 to +187
/*
* Various test plugins to be injected with Spring
*/
@Getter
@Setter
@Accessors(chain = true)
public static class TestValidator extends AspectPayloadValidator {

public AspectPluginConfig config;

@Override
protected Stream<AspectValidationException> validateProposedAspects(
@Nonnull Collection<? extends BatchItem> mcpItems,
@Nonnull RetrieverContext retrieverContext) {
return mcpItems.stream().map(i -> AspectValidationException.forItem(i, "test error"));
}

@Override
protected Stream<AspectValidationException> validatePreCommitAspects(
@Nonnull Collection<ChangeMCP> changeMCPs, @Nonnull RetrieverContext retrieverContext) {
return Stream.empty();
}
}

@Getter
@Setter
@Accessors(chain = true)
public static class TestMutator extends MutationHook {
public AspectPluginConfig config;
}

@Getter
@Setter
@Accessors(chain = true)
public static class TestMCPSideEffect extends MCPSideEffect {

public AspectPluginConfig config;

@Override
protected Stream<ChangeMCP> applyMCPSideEffect(
Collection<ChangeMCP> changeMCPS, @Nonnull RetrieverContext retrieverContext) {
return changeMCPS.stream();
}

@Override
protected Stream<MCPItem> postMCPSideEffect(
Collection<MCLItem> mclItems, @Nonnull RetrieverContext retrieverContext) {
return Stream.of();
}
}

@Getter
@Setter
@Accessors(chain = true)
public static class TestMCLSideEffect extends MCLSideEffect {
public AspectPluginConfig config;

@Override
protected Stream<MCLItem> applyMCLSideEffect(
@Nonnull Collection<MCLItem> batchItems, @Nonnull RetrieverContext retrieverContext) {
return null;
}
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Add More Specific Tests for Plugin Classes

While the test classes for various plugins (validators, mutators, side effects) are defined here, consider adding more specific unit tests to verify the behavior of these plugins under different conditions. This will ensure that each plugin behaves as expected in isolation and when integrated into the system.

Would you like assistance in creating additional unit tests for these plugin classes?

Comment on lines +250 to +284
@Test
public void testUnloadedMerge() throws EntityRegistryException {
ConfigEntityRegistry configEntityRegistry1 =
new ConfigEntityRegistry(
TestEntityProfile.class.getClassLoader().getResourceAsStream(REGISTRY_FILE_1),
(config, classLoaders) -> new PluginFactory(config, classLoaders));
ConfigEntityRegistry configEntityRegistry2 =
new ConfigEntityRegistry(
TestEntityProfile.class.getClassLoader().getResourceAsStream(REGISTRY_FILE_2),
(config, classLoaders) -> new PluginFactory(config, classLoaders));

MergedEntityRegistry mergedEntityRegistry = new MergedEntityRegistry(configEntityRegistry1);
mergedEntityRegistry.apply(configEntityRegistry2);

assertEquals(
mergedEntityRegistry.getAllAspectPayloadValidators().stream()
.filter(p -> p.getConfig().getSupportedOperations().contains("DELETE"))
.count(),
1);
assertEquals(
mergedEntityRegistry.getAllMutationHooks().stream()
.filter(p -> p.getConfig().getSupportedOperations().contains("DELETE"))
.count(),
1);
assertEquals(
mergedEntityRegistry.getAllMCLSideEffects().stream()
.filter(p -> p.getConfig().getSupportedOperations().contains("DELETE"))
.count(),
1);
assertEquals(
mergedEntityRegistry.getAllMCPSideEffects().stream()
.filter(p -> p.getConfig().getSupportedOperations().contains("DELETE"))
.count(),
1);
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Review Logic for Unloaded Merges Test

The testUnloadedMerge method tests the behavior of merging registries when plugins are not pre-loaded. This is a critical functionality to ensure robustness in dynamic environments. The implementation seems correct, but consider adding more detailed assertions to verify the exact state of the registry after the merge, especially focusing on the plugins' states.

Comment on lines +230 to +248
@Test
public void testEmptyMerges() throws EntityRegistryException {
ConfigEntityRegistry configEntityRegistry1 =
new ConfigEntityRegistry(
TestEntityProfile.class.getClassLoader().getResourceAsStream(REGISTRY_FILE_1));
ConfigEntityRegistry emptyEntityRegistry =
new ConfigEntityRegistry(
TestEntityProfile.class.getClassLoader().getResourceAsStream(REGISTRY_FILE_2),
(config, classLoaders) -> PluginFactory.empty());

MergedEntityRegistry mergedEntityRegistry = new MergedEntityRegistry(configEntityRegistry1);
mergedEntityRegistry.apply(emptyEntityRegistry);
assertEquals(mergedEntityRegistry.getPluginFactory(), configEntityRegistry1.getPluginFactory());

MergedEntityRegistry mergedEntityRegistry2 = new MergedEntityRegistry(emptyEntityRegistry);
mergedEntityRegistry2.apply(configEntityRegistry1);
assertEquals(
mergedEntityRegistry2.getPluginFactory(), configEntityRegistry1.getPluginFactory());
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Tip

Codebase Verification

Ensure Correctness of Test for Empty Merges

The testEmptyMerges method is crucial for ensuring that merging an empty registry does not alter the state of an existing registry. While some null handling is present in the constructors of ConfigEntityRegistry and PluginFactory, the MergedEntityRegistry lacks explicit null checks and error handling. To enhance robustness, consider adding these safeguards, particularly in MergedEntityRegistry.

  • ConfigEntityRegistry: Constructors handle nullable parameters.
  • MergedEntityRegistry: Lacks explicit null checks and error handling.
  • PluginFactory: Handles null PluginConfiguration by defaulting to PluginConfiguration.EMPTY.

Suggested Enhancements:

  • Add null checks and error handling in MergedEntityRegistry to ensure robustness against edge cases.
Analysis chain

Ensure Correctness of Test for Empty Merges

The method testEmptyMerges is crucial for ensuring that merging an empty registry does not alter the state of an existing registry. The logic appears correct, but it's important to ensure that all edge cases are covered, such as potential null pointers or unexpected behavior when merging configurations.

Scripts executed

The following scripts were executed for the analysis:


Script:

#!/bin/bash
# Description: Verify all edge cases for empty merges are covered.

# Test: Search for the function usage. Expect: Only occurances of the new signature.
rg --type java $'testEmptyMerges'

Length of output: 181



Script:

#!/bin/bash
# Description: Verify edge cases for empty merges, including null pointers and unexpected behavior handling.

# Search for null checks and error handling in PluginsTest.java
rg --type java 'null' entity-registry/src/test/java/com/linkedin/metadata/aspect/plugins/PluginsTest.java

# Search for error handling (e.g., try-catch blocks) in PluginsTest.java
rg --type java 'try\s*\{' entity-registry/src/test/java/com/linkedin/metadata/aspect/plugins/PluginsTest.java

Length of output: 217



Script:

#!/bin/bash
# Description: Verify constructors and relevant methods of involved classes for null checks and error handling.

# Search for constructors in ConfigEntityRegistry
rg --type java 'ConfigEntityRegistry\s*\('

# Search for constructors in MergedEntityRegistry
rg --type java 'MergedEntityRegistry\s*\('

# Search for constructors and methods in PluginFactory
rg --type java 'PluginFactory\s*\('

Length of output: 14188



Script:

#!/bin/bash
# Description: Extract and review constructors and methods of involved classes for null checks and error handling.

# Extract constructors of ConfigEntityRegistry
rg --type java --context 10 'public ConfigEntityRegistry\s*\(' entity-registry/src/main/java/com/linkedin/metadata/models/registry/ConfigEntityRegistry.java

# Extract constructors of MergedEntityRegistry
rg --type java --context 10 'public MergedEntityRegistry\s*\(' entity-registry/src/main/java/com/linkedin/metadata/models/registry/MergedEntityRegistry.java

# Extract constructors and methods of PluginFactory
rg --type java --context 10 'public PluginFactory\s*\(' entity-registry/src/main/java/com/linkedin/metadata/aspect/plugins/PluginFactory.java

Length of output: 6134

Comment on lines +183 to +190
public boolean hasLoadedPlugins() {
return Stream.of(
this.aspectPayloadValidators,
this.mutationHooks,
this.mcpSideEffects,
this.mcpSideEffects)
.anyMatch(Objects::nonNull);
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Check for Loaded Plugins Consistency

The hasLoadedPlugins method is crucial for determining if the factory has loaded any plugins. The logic here uses a stream to check for non-null lists of different plugin types. Consider adding more detailed logging to trace the loading process and help diagnose issues in environments where plugins might fail to load.

testCompileOnly externalDependency.lombok
testAnnotationProcessor externalDependency.lombok
Copy link
Contributor

Choose a reason for hiding this comment

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

Clarify the dual inclusion of Lombok in test dependencies.

Lombok is included as both testImplementation and testCompileOnly. Typically, Lombok should only be needed as testCompileOnly unless there's a specific reason for this configuration.

@david-leifker
Copy link
Collaborator Author

Odd test failure needs investigation

Caused by: java.security.AccessControlException: access denied ("java.util.PropertyPermission" "line.separator" "read")
	at java.base/java.security.AccessControlContext.checkPermission(AccessControlContext.java:485)
	at java.base/java.security.AccessController.checkPermission(AccessController.java:1068)
	at java.base/java.lang.SecurityManager.checkPermission(SecurityManager.java:416)
	at java.base/java.lang.SecurityManager.checkPropertyAccess(SecurityManager.java:1160)
	at java.base/java.lang.System.getProperty(System.java:915)
	at ch.qos.logback.core.CoreConstants.<clinit>(CoreConstants.java:44)
	... 61 more

* added additional tests
* added the ability for Spring Plugin class to produce multiple plugin beans
Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 0

Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

Commits

Files that changed from the base of the PR and between 62fb66e and 79df379.

Files selected for processing (9)
  • entity-registry/src/main/java/com/linkedin/metadata/aspect/plugins/PluginFactory.java (3 hunks)
  • entity-registry/src/main/java/com/linkedin/metadata/aspect/plugins/config/PluginConfiguration.java (1 hunks)
  • entity-registry/src/test/java/com/linkedin/metadata/aspect/plugins/PluginsTest.java (1 hunks)
  • metadata-service/plugin/build.gradle (1 hunks)
  • metadata-service/plugin/src/main/java/com/datahub/plugins/metadata/aspect/SpringPluginFactory.java (1 hunks)
  • metadata-service/plugin/src/test/java/com/datahub/plugins/metadata/aspect/SpringPluginFactoryTest.java (1 hunks)
  • metadata-service/plugin/src/test/java/com/datahub/plugins/metadata/aspect/test/TestSpringPluginConfiguration.java (1 hunks)
  • metadata-service/plugin/src/test/resources/test-entity-registry-plugins-1.yml (1 hunks)
  • metadata-service/plugin/src/test/resources/test-entity-registry-spring-plugins-1.yml (1 hunks)
Files skipped from review as they are similar to previous changes (9)
  • entity-registry/src/main/java/com/linkedin/metadata/aspect/plugins/PluginFactory.java
  • entity-registry/src/main/java/com/linkedin/metadata/aspect/plugins/config/PluginConfiguration.java
  • entity-registry/src/test/java/com/linkedin/metadata/aspect/plugins/PluginsTest.java
  • metadata-service/plugin/build.gradle
  • metadata-service/plugin/src/main/java/com/datahub/plugins/metadata/aspect/SpringPluginFactory.java
  • metadata-service/plugin/src/test/java/com/datahub/plugins/metadata/aspect/SpringPluginFactoryTest.java
  • metadata-service/plugin/src/test/java/com/datahub/plugins/metadata/aspect/test/TestSpringPluginConfiguration.java
  • metadata-service/plugin/src/test/resources/test-entity-registry-plugins-1.yml
  • metadata-service/plugin/src/test/resources/test-entity-registry-spring-plugins-1.yml

@david-leifker david-leifker merged commit 5f33bf3 into datahub-project:master Jun 28, 2024
38 of 39 checks passed
aviv-julienjehannet pushed a commit to aviv-julienjehannet/datahub that referenced this pull request Jul 17, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
devops PR or Issue related to DataHub backend & deployment publish-docker
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants