JNG-6064 Configuration of Guice added #225
Conversation
robertcsakany
commented
Dec 15, 2024
•
robertcsakany
commented
 JNG-6064 Configure Guice with System.properties, Environment variables and config files
|
robertcsakany
requested review from
noherczeg,
bencegelei,
gaborflorian and
Madmat8
WalkthroughThe pull request introduces a comprehensive enhancement to the Judo runtime core's configuration management by implementing a new Changes
Poem
Finishing Touches
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 9
🧹 Outside diff range and nitpick comments (31)
judo-runtime-core-guice-jetty/pom.xml (1)
Line range hint
29-31: Remove duplicate Guice dependency declarationThe
com.google.inject:guicedependency is declared twice in the POM file. This can lead to confusion and potential version conflicts.Remove one of the duplicate declarations:
<dependency> <groupId>com.google.inject</groupId> <artifactId>guice</artifactId> </dependency> <!-- ... other dependencies ... --> - <dependency> - <groupId>com.google.inject</groupId> - <artifactId>guice</artifactId> - </dependency>Also applies to: 89-91
judo-runtime-core-guice/src/main/java/hu/blackbelt/judo/runtime/core/guice/JudoDefaultModuleConfiguration.java (1)
53-53: LGTM! Well-structured addition of RuntimeVariableResolver configuration.The new runtimeVariableResolver field follows the established pattern of optional components with null defaults, maintaining backward compatibility while enabling flexible configuration resolution across different modules.
This addition enhances the framework's configurability by:
- Supporting dynamic resolution of configuration values
- Maintaining loose coupling with the resolution mechanism
- Allowing gradual adoption in different modules
judo-runtime-core-guice/src/main/java/hu/blackbelt/judo/runtime/core/guice/ComponentScanModule.java (2)
11-11: LGTM! Consider adding documentation for inheritance.The removal of the
finalmodifier aligns with the PR's goal of enabling flexible configuration through inheritance. However, since this is a core configuration class, it would be beneficial to document the intended use cases for subclassing.Consider adding a class-level Javadoc comment explaining:
- Why the class is now inheritable
- How it relates to RuntimeVariableResolver
- Expected use cases for subclassing
+/** + * A Guice module that performs component scanning for annotated classes. + * This class can be extended to customize the scanning behavior or to integrate + * with RuntimeVariableResolver for dynamic configuration resolution. + */ public class ComponentScanModule extends AbstractModule {
Line range hint
20-26: Consider protecting the configure method from unintended overrides.Since this is a core configuration class, it's important to ensure that the component scanning behavior remains consistent even when the class is subclassed.
Consider marking the
configure()method asfinalto prevent accidental overrides:@Override -public void configure() { +public final void configure() { Reflections packageReflections = new Reflections(packageName); bindingAnnotations.stream() .map(packageReflections::getTypesAnnotatedWith) .flatMap(Set::stream) .forEach(this::bind); }If extensibility is needed, consider introducing a protected template method that subclasses can override to customize specific aspects of the configuration.
judo-runtime-core-guice-keycloak/src/main/java/hu/blackbelt/judo/runtime/core/security/keycloak/guice/JudoKeycloakModuleConfiguration.java (2)
17-19: Consider providing a default implementation of RuntimeVariableResolverThe
runtimeVariableResolveris initialized tonullby default. While this allows for optional resolution behavior, consider providing a default implementation (like a no-op resolver or environment-based resolver) to maintain consistent behavior when not explicitly configured.This would:
- Prevent potential NPEs in code that assumes a resolver exists
- Provide a clear default behavior
- Follow the pattern of other fields in this class that have meaningful defaults
Consider this approach:
- @Builder.Default - RuntimeVariableResolver runtimeVariableResolver = null; + @Builder.Default + RuntimeVariableResolver runtimeVariableResolver = new RuntimeVariableResolver() { + @Override + public String resolve(String key) { + return System.getProperty(key, System.getenv(key)); + } + };
Line range hint
45-45: Fix double semicolon typoThere's a double semicolon at the end of the
keycloakRealmSynchronizerRetryExponentialBackofffield declaration.- Boolean keycloakRealmSynchronizerRetryExponentialBackoff = true;; + Boolean keycloakRealmSynchronizerRetryExponentialBackoff = true;judo-runtime-core-guice-jetty/src/main/java/hu/blackbelt/judo/runtime/core/jetty/guice/JudoJettyModule.java (5)
44-44: InitializeruntimeVariableResolverwith a default instanceInstead of defaulting
runtimeVariableResolvertonull, consider initializing it with a defaultRuntimeVariableResolverinstance. This can simplify the code and avoid potential null checks elsewhere.Apply this change:
@Builder.Default -RuntimeVariableResolver runtimeVariableResolver = null; +RuntimeVariableResolver runtimeVariableResolver = RuntimeVariableResolver.builder().prefix("judo").build();
54-54: Ensure consistency in default initializationSimilarly, in the
JudoJettyModuleBuilderclass, consider initializingruntimeVariableResolverwith a default instance to maintain consistency and reduce null checks.Apply this change:
RuntimeVariableResolver runtimeVariableResolver = null; +RuntimeVariableResolver runtimeVariableResolver = RuntimeVariableResolver.builder().prefix("judo").build();
Line range hint
68-74: Simplify constructor by using the configuration objectConsider simplifying the constructor by relying solely on the
JudoJettyModuleConfigurationobject. Accepting both a configuration object and individual parameters may lead to confusion and potential errors. Streamlining the constructor can enhance readability and maintainability.
79-80: Avoid redundant initialization ofruntimeVariableResolverIf you initialize
runtimeVariableResolverby default in the configuration, the use ofObjects.requireNonNullElseGetbecomes unnecessary. This can simplify the constructor logic.Apply this change:
.runtimeVariableResolver( - Objects.requireNonNullElseGet(runtimeVariableResolver, - () -> RuntimeVariableResolver.builder().prefix("judo").build())) + runtimeVariableResolver)
96-120: Define variable names as constants to prevent typosTo enhance maintainability and reduce the risk of typos, consider defining the variable names used in
getVariableAsIntegerandgetVariableAsStringas constants.Apply this refactor:
+public static final String JETTY_SERVER_PORT = "jettyServerPort"; +public static final String JETTY_SERVER_CONTEXT_PATH = "jettyServerContextPath"; +public static final String JETTY_SERVER_MAX_THREADS = "jettyServerMaxThreads"; +public static final String JETTY_SERVER_MIN_THREADS = "jettyServerMinThreads"; +public static final String JETTY_SERVER_IDLE_TIMEOUT = "jettyServerIdleTimeout"; bind(Integer.class).annotatedWith(JettyConfigurations.JettyServerPort.class) .toInstance(configuration.getRuntimeVariableResolver() - .getVariableAsInteger("jettyServerPort", + .getVariableAsInteger(JETTY_SERVER_PORT, configuration.getJettyServerPort()));Repeat similar changes for the other variable names.
judo-runtime-core/pom.xml (1)
121-128: Remove commented dependency blockThe commented-out junit-pioneer dependency should be removed if it's not needed. If it might be needed in the future, consider tracking it in a separate ticket or documentation instead.
- <!-- - <dependency> - <groupId>org.junit-pioneer</groupId> - <artifactId>junit-pioneer</artifactId> - <version>2.3.0</version> - <scope>test</scope> - </dependency> - -->judo-runtime-core/src/main/java/hu/blackbelt/judo/runtime/core/utils/VariableResolver.java (3)
8-10: Add interface and method documentation.The interface and its methods lack documentation. Please add Javadoc comments explaining:
- The purpose and responsibility of the
VariableResolverinterface- The contract and expected behavior of
getName()andprocess()- Any constraints or assumptions about the returned values
21-32: Fix typo and optimize regex pattern matching.
- Variable name "isUpperSnakeCae" has a typo (should be "isUpperSnakeCase")
- The regex patterns could be pre-compiled for better performance
Consider this optimization:
+ private static final Pattern SNAKE_CASE_PATTERN = Pattern.compile("([A-Z_0-9]*)"); + private static final Pattern SPECIAL_CHARS_PATTERN = Pattern.compile(".*[.#@_,;:-].*"); + static String generalizeName(String str) { if (str == null || str.length() == 0) { return str; } - boolean isUpperSnakeCae = !str.matches(negateRegex("([A-Z_0-9]*)")); - if (isUpperSnakeCae) { + boolean isUpperSnakeCase = !SNAKE_CASE_PATTERN.matcher(str).matches(); + if (isUpperSnakeCase) { return toCamelCase(Arrays.stream(str.split("_")).map(s -> capitalize(s)).collect(Collectors.joining())); - } else if (str.matches(".*[.#@_,;:-].*")) { + } else if (SPECIAL_CHARS_PATTERN.matcher(str).matches()) {
52-71: Reduce code duplication in string utility methods.The string manipulation methods (
firstToUpper,firstToLower,capitalize) contain duplicate validation logic and similar transformations.Consider consolidating the common logic:
+ private static String transformFirstChar(String str, Function<String, String> charTransform) { + if (str == null || str.length() <= 1) { + return str; + } + return charTransform.apply(str.substring(0, 1)) + str.substring(1); + } + static String firstToUpper(String str) { - if (str != null && str.length() > 1) { - return str.substring(0, 1).toUpperCase() + str.substring(1); - } - return str; + return transformFirstChar(str, String::toUpperCase); } static String firstToLower(String str) { - if (str != null && str.length() > 1) { - return str.substring(0, 1).toLowerCase() + str.substring(1); - } - return str; + return transformFirstChar(str, String::toLowerCase); } static String capitalize(String str) { - if (str != null && str.length() > 1) { - return str.substring(0,1).toUpperCase() + str.substring(1).toLowerCase(); - } - return str; + return str == null || str.length() <= 1 ? str + : str.substring(0, 1).toUpperCase() + str.substring(1).toLowerCase(); }judo-runtime-core/src/main/java/hu/blackbelt/judo/runtime/core/utils/EnvironmentVariableResolver.java (2)
7-14: Add class-level documentation to explain the resolver's purpose and behavior.Please add Javadoc to describe:
- The purpose of this resolver in the context of RuntimeVariableResolver
- The environment variable naming conventions
- How variable name generalization works
+/** + * Resolves configuration values from environment variables. + * <p> + * This resolver implements the VariableResolver interface to provide access to system environment + * variables. Variable names are generalized according to the template variable naming convention. + */ public class EnvironmentVariableResolver implements VariableResolver {
16-24: Consider adding null safety checks and logging.The implementation looks correct, but consider adding:
- Null safety check for System.getenv()
- Debug logging for resolved variables (excluding sensitive values)
@Override public Map<String, Object> process() { + Map<String, String> env = System.getenv(); + if (env == null) { + return new HashMap<>(); + } - return VariableResolver.generalizeTemplateVariableNames(System.getenv().entrySet().stream().collect( + return VariableResolver.generalizeTemplateVariableNames(env.entrySet().stream().collect(judo-runtime-core/src/main/java/hu/blackbelt/judo/runtime/core/utils/SystemPropertiesVariableResolver.java (2)
7-14: Add consistent documentation matching EnvironmentVariableResolver.Please add similar Javadoc to maintain consistency across resolver implementations.
+/** + * Resolves configuration values from system properties. + * <p> + * This resolver implements the VariableResolver interface to provide access to Java system + * properties. Both keys and values are converted to strings, and variable names are + * generalized according to the template variable naming convention. + */ public class SystemPropertiesVariableResolver implements VariableResolver {
16-24: Consider optimizing the property resolution process.The current implementation has a few potential improvements:
- Properties could be cached to avoid repeated stream operations
- Null checks could be added for safety
- Consider using Properties.stringPropertyNames() for better performance
@Override public Map<String, Object> process() { + Properties props = System.getProperties(); + if (props == null) { + return new HashMap<>(); + } + // Use Properties.stringPropertyNames() for better performance + return VariableResolver.generalizeTemplateVariableNames( + props.stringPropertyNames().stream() + .collect(Collectors.toMap( + key -> key, + key -> props.getProperty(key), + (prev, next) -> next, + HashMap::new + )) + ); - return VariableResolver.generalizeTemplateVariableNames(System.getProperties().entrySet().stream().collect( - Collectors.toMap( - e -> String.valueOf(e.getKey()), - e -> String.valueOf(e.getValue()), - (prev, next) -> next, HashMap::new - ))); }judo-runtime-core-guice-postgresql/src/main/java/hu/blackbelt/judo/runtime/core/guice/dao/rdbms/postgresql/JudoPostgresqlModuleConfiguration.java (1)
21-21: Document the RuntimeVariableResolver integration.Please add documentation explaining:
- The purpose of RuntimeVariableResolver in this configuration
- How it affects the resolution of other configuration properties
- Expected behavior when resolver is null
+ /** + * Variable resolver used for dynamic resolution of configuration values. + * When null, default values will be used for all configuration properties. + */ @Builder.Default RuntimeVariableResolver runtimeVariableResolver = null;judo-runtime-core-guice-hsqldb/src/main/java/hu/blackbelt/judo/runtime/core/guice/dao/rdbms/hsqldb/JudoHsqldbModuleConfiguration.java (1)
22-22: Document the RuntimeVariableResolver usage and consider non-null defaultThe field is initialized to null which could lead to NullPointerExceptions. Consider:
- Adding documentation on how this resolver should be used
- Providing a default non-null implementation
- @Builder.Default - RuntimeVariableResolver runtimeVariableResolver = null; + /** + * Resolver for runtime configuration variables. When provided, enables dynamic resolution + * of configuration values from environment variables, system properties, and property files. + */ + @Builder.Default + RuntimeVariableResolver runtimeVariableResolver = RuntimeVariableResolver.builder().build();judo-runtime-core-guice-cxf/src/main/java/hu/blackbelt/judo/runtime/core/jaxrs/cxf/server/guice/JudoCxfModuleConfiguration.java (1)
14-15: Document the RuntimeVariableResolver usage and consider non-null defaultSimilar to JudoHsqldbModuleConfiguration, consider:
- Adding documentation explaining how the resolver affects configuration properties like cxfJaxRsServerUrl, corsAllowOrigin, etc.
- Providing a default non-null implementation
- @Builder.Default - RuntimeVariableResolver runtimeVariableResolver = null; + /** + * Resolver for runtime configuration variables. When provided, enables dynamic resolution + * of configuration values like server URL, CORS settings, etc. from environment variables, + * system properties, and property files. + */ + @Builder.Default + RuntimeVariableResolver runtimeVariableResolver = RuntimeVariableResolver.builder().build();judo-runtime-core/src/test/java/hu/blackbelt/judo/runtime/core/util/RuntimeVariableResolverTest.java (3)
19-23: Consider using an enum for test constantsThe string constants could be better organized using an enum to group related values and prevent typos.
- public static final String CAMEL_CASE = "camelCase"; - public static final String DOT_SEPARATED = "dot separated"; - public static final String ENV = "env"; - public static final String WRONG = "wrong"; - public static final String PROPERTYFILE = "judoPidNamePropertyFile"; + private enum TestValue { + CAMEL_CASE("camelCase"), + DOT_SEPARATED("dot separated"), + ENV("env"), + WRONG("wrong"), + PROPERTYFILE("judoPidNamePropertyFile"); + + private final String value; + TestValue(String value) { this.value = value; } + public String getValue() { return value; } + }
121-162: Improve test organization and readabilityThe precedence test contains two similar test blocks that could be refactored using a parameterized test.
+ @ParameterizedTest + @MethodSource("precedenceTestCases") + void testVariablePrecedence(List<String> precedence, String expectedValue) throws Exception { + restoreSystemProperties(() -> { + withEnvironmentVariable("JUDO_PID_NAME", ENV) + .execute(() -> { + System.setProperty("judoPidName", CAMEL_CASE); + RuntimeVariableResolver resolver = createResolver(precedence); + assertEquals(expectedValue, resolver.getVariableAsString("judoPidName", WRONG)); + }); + }); + } + + private static Stream<Arguments> precedenceTestCases() { + return Stream.of( + Arguments.of( + List.of(SystemPropertiesVariableResolver.SYSTEM_PROPERTIES, + EnvironmentVariableResolver.ENVIRONMENT_VARIABLES), + ENV + ), + Arguments.of( + List.of(SystemPropertiesVariableResolver.SYSTEM_PROPERTIES, + EnvironmentVariableResolver.ENVIRONMENT_VARIABLES, + PropertyFileVariableResolver.PROPERTIES_FILES), + PROPERTYFILE + ) + ); + }
1-164: Add missing test scenariosThe test suite could be more comprehensive. Consider adding tests for:
- Error cases (invalid file paths, malformed values)
- Edge cases (empty values, whitespace)
- Concurrent access scenarios
- Memory usage with large property files
judo-runtime-core/src/main/java/hu/blackbelt/judo/runtime/core/utils/PropertyFileVariableResolver.java (3)
3-3: Replace deprecated GuavaCharsetswithStandardCharsetsThe Guava
Charsetsclass is deprecated. It's better to usejava.nio.charset.StandardCharsetsfor better compatibility and to reduce external dependencies.Apply this diff:
- import com.google.common.base.Charsets; + import java.nio.charset.StandardCharsets;And ensure that you're using
StandardCharsets.UTF_8in the code:- prop.load(new FileReader(parameterFile, Charsets.UTF_8)); + try (Reader reader = Files.newBufferedReader(parameterFile.toPath(), StandardCharsets.UTF_8)) { + prop.load(reader); + }Also applies to: 44-44
35-35: Simplify null and emptiness check forparameterFilesSince
parameterFilesis annotated with@NonNull, the null check is unnecessary. Also, useisEmpty()for clarity.Apply this diff:
- if (parameterFiles != null && parameterFiles.size() > 0) { + if (!parameterFiles.isEmpty()) {
55-55: Use parameterized logging instead of string concatenationTo improve performance and avoid unnecessary string concatenation when logging is disabled, use parameterized logging.
Apply this diff:
- log.warn("File is missing:" + parameterFile.getAbsolutePath()); + log.warn("File is missing: {}", parameterFile.getAbsolutePath());judo-runtime-core/src/main/java/hu/blackbelt/judo/runtime/core/utils/RuntimeVariableResolver.java (1)
20-21: Remove unusedcoercerFactoryfieldThe
coercerFactoryfield is only used to initialize thecoercerand is not used elsewhere. You can remove it to simplify the code.Apply this diff:
- final DefaultCoercerFactory coercerFactory = new DefaultCoercerFactory(); final Coercer coercer;And update the constructor:
- this.coercer = new DefaultCoercerFactory().getCoercerInstance(); + this.coercer = new DefaultCoercerFactory().getCoercerInstance();Since you're creating a new
DefaultCoercerFactoryinstance just to get thecoercer, you can inline it directly.Also applies to: 47-47
judo-runtime-core/src/main/java/hu/blackbelt/judo/runtime/core/utils/JudoProcessHandler.java (1)
48-50: UseProcessHandle.current().pid()for obtaining the PIDStarting from Java 9, you can use
ProcessHandle.current().pid()to get the current process ID more reliably and without parsing.Apply this diff:
public long getCurrentProcessPid() { - int pid = Integer.parseInt(ManagementFactory.getRuntimeMXBean().getName().split("@")[0]); - return pid; + return ProcessHandle.current().pid(); }This approach is cleaner and less error-prone.
judo-runtime-core-guice-hsqldb/src/main/java/hu/blackbelt/judo/runtime/core/guice/dao/rdbms/hsqldb/JudoHsqldbModule.java (1)
82-83: SimplifyRuntimeVariableResolverinitializationThe use of
Objects.requireNonNullElseGetcan be simplified since you already have a default value in the builder. Alternatively, ensure consistency in the way defaults are applied.Consider updating the code to streamline the initialization:
this.configuration = JudoHsqldbModuleConfiguration.builder() - .runtimeVariableResolver(Objects.requireNonNullElseGet(runtimeVariableResolver, - () -> RuntimeVariableResolver.builder().prefix("judo").build())) + .runtimeVariableResolver(runtimeVariableResolver != null ? runtimeVariableResolver : + RuntimeVariableResolver.builder().prefix("judo").build()) // other configurations .build();This makes the code more readable and consistent.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (26)
judo-runtime-core-guice-cxf/src/main/java/hu/blackbelt/judo/runtime/core/jaxrs/cxf/server/guice/JudoCxfModule.java(4 hunks)judo-runtime-core-guice-cxf/src/main/java/hu/blackbelt/judo/runtime/core/jaxrs/cxf/server/guice/JudoCxfModuleConfiguration.java(2 hunks)judo-runtime-core-guice-hsqldb/src/main/java/hu/blackbelt/judo/runtime/core/guice/dao/rdbms/hsqldb/JudoHsqldbModule.java(5 hunks)judo-runtime-core-guice-hsqldb/src/main/java/hu/blackbelt/judo/runtime/core/guice/dao/rdbms/hsqldb/JudoHsqldbModuleConfiguration.java(2 hunks)judo-runtime-core-guice-jetty/pom.xml(1 hunks)judo-runtime-core-guice-jetty/src/main/java/hu/blackbelt/judo/runtime/core/jetty/guice/JudoJettyModule.java(6 hunks)judo-runtime-core-guice-keycloak/src/main/java/hu/blackbelt/judo/runtime/core/security/keycloak/guice/JudoKeycloakModule.java(5 hunks)judo-runtime-core-guice-keycloak/src/main/java/hu/blackbelt/judo/runtime/core/security/keycloak/guice/JudoKeycloakModuleConfiguration.java(2 hunks)judo-runtime-core-guice-keycloak/src/test/java/hu/blackbelt/judo/runtime/core/security/keycloak/guice/JudoKeycloakModuleTest.java(1 hunks)judo-runtime-core-guice-postgresql/src/main/java/hu/blackbelt/judo/runtime/core/guice/dao/rdbms/postgresql/JudoPostgresqlModule.java(4 hunks)judo-runtime-core-guice-postgresql/src/main/java/hu/blackbelt/judo/runtime/core/guice/dao/rdbms/postgresql/JudoPostgresqlModuleConfiguration.java(2 hunks)judo-runtime-core-guice/src/main/java/hu/blackbelt/judo/runtime/core/guice/ComponentScanModule.java(1 hunks)judo-runtime-core-guice/src/main/java/hu/blackbelt/judo/runtime/core/guice/JudoDefaultModule.java(6 hunks)judo-runtime-core-guice/src/main/java/hu/blackbelt/judo/runtime/core/guice/JudoDefaultModuleConfiguration.java(2 hunks)judo-runtime-core-security-keycloak/src/main/java/hu/blackbelt/judo/runtime/core/security/keycloak/KeycloakRealmSynchronizer.java(0 hunks)judo-runtime-core/pom.xml(2 hunks)judo-runtime-core/src/main/java/hu/blackbelt/judo/runtime/core/utils/EnvironmentVariableResolver.java(1 hunks)judo-runtime-core/src/main/java/hu/blackbelt/judo/runtime/core/utils/JudoProcessHandler.java(1 hunks)judo-runtime-core/src/main/java/hu/blackbelt/judo/runtime/core/utils/PropertyFileVariableResolver.java(1 hunks)judo-runtime-core/src/main/java/hu/blackbelt/judo/runtime/core/utils/RuntimeVariableResolver.java(1 hunks)judo-runtime-core/src/main/java/hu/blackbelt/judo/runtime/core/utils/SystemPropertiesVariableResolver.java(1 hunks)judo-runtime-core/src/main/java/hu/blackbelt/judo/runtime/core/utils/VariableResolver.java(1 hunks)judo-runtime-core/src/test/java/hu/blackbelt/judo/runtime/core/util/JudoProcessHandlerTest.java(1 hunks)judo-runtime-core/src/test/java/hu/blackbelt/judo/runtime/core/util/RuntimeVariableResolverTest.java(1 hunks)judo-runtime-core/src/test/resources/testVariableNaming.properties(1 hunks)judo-runtime-core/src/test/resources/testVariablePrecedence.properties(1 hunks)
💤 Files with no reviewable changes (1)
- judo-runtime-core-security-keycloak/src/main/java/hu/blackbelt/judo/runtime/core/security/keycloak/KeycloakRealmSynchronizer.java
✅ Files skipped from review due to trivial changes (2)
- judo-runtime-core/src/test/resources/testVariableNaming.properties
- judo-runtime-core/src/test/resources/testVariablePrecedence.properties
🔇 Additional comments (35)
judo-runtime-core-guice-jetty/pom.xml (1)
106-109: LGTM: New dependency aligns with RuntimeVariableResolver implementation
The addition of judo-runtime-core dependency is appropriate for implementing the new configuration management functionality. The version is correctly inherited from the parent POM through dependency management.
judo-runtime-core-guice/src/main/java/hu/blackbelt/judo/runtime/core/guice/JudoDefaultModuleConfiguration.java (1)
26-26: LGTM!
Clean addition of the RuntimeVariableResolver import.
judo-runtime-core-guice-keycloak/src/main/java/hu/blackbelt/judo/runtime/core/security/keycloak/guice/JudoKeycloakModuleConfiguration.java (2)
4-4: LGTM!
The import for RuntimeVariableResolver is appropriately added to support the new configuration capability.
17-19: Verify consistent RuntimeVariableResolver usage pattern across modules
The AI summary indicates this change is part of a broader integration of RuntimeVariableResolver across multiple modules. Let's verify the consistency of the implementation pattern.
✅ Verification successful
Let's try a different search strategy to understand the usage pattern better.
Based on the search results, I can now provide a conclusive response about the RuntimeVariableResolver usage pattern:
RuntimeVariableResolver implementation follows consistent pattern across modules
The codebase shows a consistent implementation pattern where RuntimeVariableResolver:
- Is properly declared as an optional (@Builder.Default) configuration field across all modules
- Has a standardized null-safe initialization with "judo" prefix
- Is used consistently for resolving configuration values with fallbacks
The implementation in JudoKeycloakModuleConfiguration aligns with this established pattern:
- Optional field with @Builder.Default annotation
- Null value is acceptable as modules handle the default initialization
- Used consistently for resolving Keycloak-specific configuration values
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check the implementation pattern of RuntimeVariableResolver across modules
# Search for RuntimeVariableResolver field declarations
echo "Checking RuntimeVariableResolver field declarations:"
ast-grep --pattern 'class $_ {
$$$
RuntimeVariableResolver $_ = $_;
$$$
}'
# Search for RuntimeVariableResolver constructor parameters
echo -e "\nChecking constructor parameters:"
ast-grep --pattern 'class $_ {
$$$
$_($_: RuntimeVariableResolver, $$$) {
$$$
}
$$$
}'
Length of output: 379
Script:
#!/bin/bash
# Search for RuntimeVariableResolver usage across the codebase
echo "Searching for RuntimeVariableResolver references:"
rg "RuntimeVariableResolver" -l
# Get detailed context of RuntimeVariableResolver usage
echo -e "\nDetailed context of RuntimeVariableResolver usage:"
rg "RuntimeVariableResolver" -B 2 -A 2
# Check if there are any other similar configuration classes
echo -e "\nSearching for similar configuration classes:"
fd ".*Configuration.java$" --exec grep -l "RuntimeVariableResolver" {} \;
Length of output: 102621
judo-runtime-core-guice-keycloak/src/main/java/hu/blackbelt/judo/runtime/core/security/keycloak/guice/JudoKeycloakModule.java (6)
37-37: Imports added for new functionality
The addition of the RuntimeVariableResolver import is appropriate and necessary for the new configuration management functionality.
41-41: Import statement for Objects is appropriate
Importing java.util.Objects enables the use of Objects.requireNonNullElseGet, which is used effectively in the constructor.
50-50: Initialization of runtimeVariableResolver in the builder
The runtimeVariableResolver field in the JudoKeycloakModuleBuilder is initialized to null. Ensure that it is properly set when building the module to avoid potential NullPointerException.
If not set explicitly, the constructor handles null by initializing runtimeVariableResolver with a default value. Confirm that this meets the desired configuration requirements.
Line range hint 77-105: Constructor updated to include RuntimeVariableResolver
The addition of RuntimeVariableResolver to the constructor enhances the module's configurability. Ensure that all usages of this constructor are updated accordingly to pass the appropriate runtimeVariableResolver instance.
If existing code relies on the previous constructor signature, this change may cause compilation errors. Review and adjust any instantiations of JudoKeycloakModule to include the new parameter.
106-107: Proper handling of runtimeVariableResolver initialization
Using Objects.requireNonNullElseGet to initialize runtimeVariableResolver ensures that a default resolver is provided if none is supplied. This approach appropriately avoids potential NullPointerException.
150-256: Dynamic configuration binding using RuntimeVariableResolver
The use of configuration.getRuntimeVariableResolver() to retrieve configuration values enhances flexibility by allowing overrides at runtime. The bindings for Keycloak settings are correctly updated to utilize the resolver.
judo-runtime-core-guice-keycloak/src/test/java/hu/blackbelt/judo/runtime/core/security/keycloak/guice/JudoKeycloakModuleTest.java (1)
48-48: Corrected class name from JudeKeycloakModuleTest to JudoKeycloakModuleTest
Renaming the class to JudoKeycloakModuleTest fixes a typographical error, improving code clarity and consistency.
judo-runtime-core/pom.xml (3)
27-36: LGTM: maven-dependency-plugin configuration
Standard configuration for accessing dependency properties during the build process.
116-119: LGTM: mapper-impl dependency addition
The addition of mapper-impl complements the existing mapper-api dependency and aligns with the implementation of RuntimeVariableResolver.
38-48: Verify test execution with the new surefire configuration
The configuration looks correct with necessary JVM arguments for:
- UTF-8 encoding
- Module access for system stubs testing
- System exit handling via junit5-system-exit agent
Let's verify the test dependencies referenced in the configuration:
✅ Verification successful
Test configuration is properly aligned with codebase usage
The verification confirms that:
- The junit5-system-exit dependency is properly declared in pom.xml with version 2.0.2
- System.exit handling is used in RuntimeVariableResolverTest.java, justifying the junit5-system-exit agent
- The --add-opens configuration for java.base modules is justified as:
- java.lang: Required for system exit handling
- java.util and java.time: Common requirements for runtime testing, especially with the RuntimeVariableResolver
The surefire plugin configuration correctly supports all the test requirements in the codebase.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Verify that all required test dependencies are present
# and the versions are consistent
# Check for junit5-system-exit dependency referenced in argLine
rg -l "junit5-system-exit" --type xml
# Check for modules that need the --add-opens configuration
rg -l "java\.base\/(java\.lang|java\.util|java\.time)" --type java
Length of output: 255
Script:
#!/bin/bash
# Check for junit5-system-exit dependency declaration in pom.xml
rg -A 5 "junit5-system-exit" judo-runtime-core/pom.xml
# Check for test classes that might use system stubs or exit handling
rg -l "SystemStubs|System\.exit" --type java
# Check for test classes using java.time, java.util mocking/stubbing
rg -l "@Mock.*\b(Time|Date|Calendar|Collection|List|Map|Set)\b" --type java
Length of output: 562
judo-runtime-core/src/main/java/hu/blackbelt/judo/runtime/core/utils/VariableResolver.java (1)
8-10: Verify interface method completeness.
Let's ensure the interface methods satisfy all requirements of the RuntimeVariableResolver implementation.
✅ Verification successful
Based on the search results, I can now generate the final response:
Interface methods are complete and correctly defined
The VariableResolver interface has all the necessary methods required by its implementations:
getName()- identifies the resolver typeprocess()- returns resolved variables as key-value pairs- Static utility methods for name processing
The interface is used correctly by multiple implementations (PropertyFile, Environment, SystemProperties) and the RuntimeVariableResolver orchestrator class, which composes multiple resolvers.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for RuntimeVariableResolver implementation to verify interface usage
ast-grep --pattern 'class RuntimeVariableResolver implements VariableResolver {
$$$
}'
Length of output: 90
Script:
#!/bin/bash
# Let's try a broader search for RuntimeVariableResolver implementations
rg "RuntimeVariableResolver" -l
# Also search for any class implementing VariableResolver
rg "implements VariableResolver" -A 5
# And search for usages of VariableResolver interface
rg "VariableResolver" -A 2
Length of output: 79102
Script:
#!/bin/bash
# Let's check the implementation of RuntimeVariableResolver to understand its relationship with VariableResolver
rg -A 10 "class RuntimeVariableResolver" judo-runtime-core/src/main/java/hu/blackbelt/judo/runtime/core/utils/RuntimeVariableResolver.java
# Also search for any static utility methods in VariableResolver interface
rg -A 5 "static.*firstToUpper" judo-runtime-core/src/main/java/hu/blackbelt/judo/runtime/core/utils/VariableResolver.java
# And check for any other methods in the interface
rg -A 5 "interface VariableResolver" judo-runtime-core/src/main/java/hu/blackbelt/judo/runtime/core/utils/VariableResolver.java
Length of output: 1814
judo-runtime-core-guice-postgresql/src/main/java/hu/blackbelt/judo/runtime/core/guice/dao/rdbms/postgresql/JudoPostgresqlModule.java (6)
36-36: Import RuntimeVariableResolver for dynamic configuration
The import of RuntimeVariableResolver allows for dynamic resolution of configuration variables.
40-40: Import java.util.Objects for null handling
The import of java.util.Objects is necessary for using Objects.requireNonNullElseGet to handle null values gracefully.
49-49: Add runtimeVariableResolver field to the builder
Including RuntimeVariableResolver in the builder enables the module to utilize runtime variables during configuration.
65-66: Update constructor to include RuntimeVariableResolver
The constructor now accepts RuntimeVariableResolver, enhancing the flexibility of configuration management.
83-84: Ensure non-null runtimeVariableResolver with default initialization
Using Objects.requireNonNullElseGet ensures that runtimeVariableResolver is initialized, defaulting to a new instance with the prefix "judo" if none is provided.
117-140: Configuration parameters correctly utilize RuntimeVariableResolver
The binding of configuration parameters now leverages RuntimeVariableResolver to dynamically retrieve values, enhancing adaptability.
judo-runtime-core-guice-cxf/src/main/java/hu/blackbelt/judo/runtime/core/jaxrs/cxf/server/guice/JudoCxfModule.java (6)
29-29: Import RuntimeVariableResolver for dynamic configuration
The import of RuntimeVariableResolver enables dynamic resolution of configuration variables within the module.
33-34: Import necessary utility classes
The imports for java.util.Objects and java.util.function.Consumer are required for null handling and consumer functionalities.
43-43: Add runtimeVariableResolver field to the builder
Including RuntimeVariableResolver in the builder allows for dynamic configurations during module setup.
69-69: Update constructor to include RuntimeVariableResolver
The constructor now accepts RuntimeVariableResolver, providing flexibility in configuration management.
94-95: Ensure non-null runtimeVariableResolver with default initialization
Utilizing Objects.requireNonNullElseGet ensures runtimeVariableResolver is initialized, defaulting to a new instance with the prefix "judo" if not provided.
150-238: Configuration parameters correctly utilize RuntimeVariableResolver
The bindings now retrieve configuration values through RuntimeVariableResolver, enhancing the module's adaptability to runtime environments.
judo-runtime-core-guice/src/main/java/hu/blackbelt/judo/runtime/core/guice/JudoDefaultModule.java (6)
62-62: Import RuntimeVariableResolver for dynamic configuration
The import of RuntimeVariableResolver allows for dynamic resolution of runtime variables within the module.
75-75: Import java.util.Objects for null handling
The import of java.util.Objects is necessary for handling null values using Objects.requireNonNullElseGet.
85-85: Add runtimeVariableResolver field to the builder
Including RuntimeVariableResolver in the builder facilitates dynamic variable resolution during module configuration.
138-141: Update constructor to include RuntimeVariableResolver
The constructor now accepts RuntimeVariableResolver, enhancing flexibility in configuration management.
191-192: Ensure non-null runtimeVariableResolver with default initialization
Using Objects.requireNonNullElseGet ensures that runtimeVariableResolver is initialized, defaulting to a new instance with the prefix "judo" if none is provided.
272-308: Configuration parameters correctly utilize RuntimeVariableResolver
The module's configuration parameters now dynamically retrieve values via RuntimeVariableResolver, enhancing adaptability.
judo-runtime-core/src/main/java/hu/blackbelt/judo/runtime/core/utils/EnvironmentVariableResolver.java (1)
1-25: Verify test coverage for environment variable resolution.
Please ensure there are unit tests covering:
- Resolution of regular environment variables
- Handling of missing environment variables
- Variable name generalization
- Duplicate key handling
✅ Verification successful
Let me check the content of the test file to verify the coverage.
Test coverage for environment variable resolution is adequate
The test file RuntimeVariableResolverTest.java provides comprehensive coverage for the EnvironmentVariableResolver class:
-
Resolution of regular environment variables is tested in multiple test methods:
testVariableNaming()verifies basic environment variable resolutiontestPrefix()tests environment variables with prefixestestBoolean()andtestInteger()verify type conversion
-
Handling of missing environment variables is covered in:
testVariableNaming()with the "judoPidDoesNotExists" case returning default value
-
Variable name generalization is tested in:
testVariableNaming()with environment variable "JUDO_PID_NAME_ENV" being accessed as "judoPidNameEnv"testPrefix()with "JUDO_PREFIX_VARIABLE" being accessed as "variable"
-
Duplicate key handling is implicitly tested through:
testVariablePrecedence()which verifies resolution order when same key exists in multiple sources
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for test files covering EnvironmentVariableResolver
echo "Searching for test files..."
fd -e java -e groovy -e scala test
rg -l "EnvironmentVariableResolver"
Length of output: 2803
Script:
#!/bin/bash
# Check the content of RuntimeVariableResolverTest.java which seems to be the relevant test file
cat "judo-runtime-core/src/test/java/hu/blackbelt/judo/runtime/core/util/RuntimeVariableResolverTest.java"
Length of output: 9607
judo-runtime-core-guice-postgresql/src/main/java/hu/blackbelt/judo/runtime/core/guice/dao/rdbms/postgresql/JudoPostgresqlModuleConfiguration.java (1)
Line range hint 21-40: Verify the integration with existing configuration properties.
The RuntimeVariableResolver addition might affect how other configuration properties are resolved. Please ensure:
- Proper fallback to default values when resolver is null
- Correct precedence order between resolver and explicitly set values
- Proper handling of sensitive properties like password
✅ Verification successful
Let me generate another script to check how the configuration properties are resolved in the PostgreSQL module:
Based on the search results, I can now provide the final response since I have enough information about how the configuration properties are resolved:
RuntimeVariableResolver integration is properly implemented with fallback values
The code shows correct implementation of configuration property resolution:
- All properties have proper fallback to default values through
getVariableAs*methods - Clear precedence order: environment/system variables first (through resolver), then fallback to explicitly set configuration values
- Sensitive properties like password are handled consistently with other properties
- Default values are defined using
@Builder.Defaultannotations in the configuration class
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for usage of RuntimeVariableResolver in the module
echo "Searching for RuntimeVariableResolver usage..."
rg -A 5 "runtimeVariableResolver"
# Search for configuration property resolution
echo "Searching for property resolution..."
ast-grep --pattern 'class JudoPostgresqlModule {
$$$
configure() {
$$$
}
$$$
}'
Length of output: 44382
Script:
#!/bin/bash
# Search for PostgreSQL module configuration and property resolution
echo "Searching for PostgreSQL module configuration..."
rg -A 10 "class JudoPostgresqlModule"
# Search for property resolution in the module
echo "Searching for property resolution patterns..."
rg -A 5 "getVariableAsString|getVariableAsInteger|getVariableAsBoolean" judo-runtime-core-guice-postgresql/
Length of output: 10358
judo-runtime-core/src/main/java/hu/blackbelt/judo/runtime/core/utils/VariableResolver.java
Outdated
Show resolved
Hide resolved
...runtime-core-guice/src/main/java/hu/blackbelt/judo/runtime/core/guice/JudoDefaultModule.java
Show resolved
Hide resolved
judo-runtime-core/src/test/java/hu/blackbelt/judo/runtime/core/util/JudoProcessHandlerTest.java
Outdated
Show resolved
Hide resolved
...time-core/src/test/java/hu/blackbelt/judo/runtime/core/util/RuntimeVariableResolverTest.java
Outdated
Show resolved
Hide resolved
...time-core/src/test/java/hu/blackbelt/judo/runtime/core/util/RuntimeVariableResolverTest.java
Outdated
Show resolved
Hide resolved
...me-core/src/main/java/hu/blackbelt/judo/runtime/core/utils/PropertyFileVariableResolver.java
Outdated
Show resolved
Hide resolved
...runtime-core/src/main/java/hu/blackbelt/judo/runtime/core/utils/RuntimeVariableResolver.java
Outdated
Show resolved
Hide resolved
...db/src/main/java/hu/blackbelt/judo/runtime/core/guice/dao/rdbms/hsqldb/JudoHsqldbModule.java
Outdated
Show resolved
Hide resolved
...test/java/hu/blackbelt/judo/runtime/core/security/keycloak/guice/JudoKeycloakModuleTest.java
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 4
🔭 Outside diff range comments (5)
judo-runtime-core-guice-jetty/src/test/java/hu/blackbelt/judo/runtime/core/jetty/guice/JudoJettyModuleTest.java (1)
Line range hint
35-53: Test coverage needs expansion for new configuration featuresThe current test only verifies the basic context path configuration. Given that this PR introduces Guice configuration from system properties, environment variables, and configuration files, this test class should be expanded to verify these new features.
Consider adding test cases for:
- Loading configuration from system properties
- Loading configuration from environment variables
- Loading configuration from configuration files
- Verifying configuration precedence order
Example test structure:
@Test void testSystemPropertyConfiguration() { System.setProperty("judo.jetty.contextPath", "/custom"); // Create new injector and verify the property was applied } @Test void testConfigurationFileLoading() { // Create test config file // Verify the configuration is properly loaded } @Test void testConfigurationPrecedence() { // Set up conflicting configurations in different sources // Verify the correct precedence order is followed }Would you like me to help create these test cases or open a GitHub issue to track this task?
judo-runtime-core-security-keycloak-cxf/pom.xml (2)
Line range hint
85-103: Consider using property-based version management for Keycloak dependenciesMultiple Keycloak dependencies are using a fixed version (17.0.1). This approach could make version updates more difficult and error-prone.
Consider moving the version to a property in the parent POM:
+ <properties> + <keycloak.version>17.0.1</keycloak.version> + </properties> <dependency> <groupId>org.keycloak</groupId> <artifactId>keycloak-adapter-core</artifactId> - <version>17.0.1</version> + <version>${keycloak.version}</version> <scope>provided</scope> </dependency>Apply similar changes to other Keycloak dependencies. This will:
- Make version updates easier and less error-prone
- Ensure version consistency across Keycloak components
- Follow Maven best practices for version management
Keycloak 17.0.1 has known security vulnerabilities - upgrade recommended
Based on the security advisory data:
- Version 17.0.1 is affected by a HIGH severity vulnerability (Denial of Service) that was patched in version 24.0.0
- Version 17.0.1 is also affected by a HIGH severity mTLS Authentication Bypass vulnerability
- The version is consistently used across all Keycloak dependencies in the project
Recommended actions:
- Upgrade all Keycloak dependencies to at least version 24.0.9 to address these security issues
- Consider upgrading to the latest stable version (26.0.6) for additional security improvements
🔗 Analysis chain
Line range hint
85-103: Verify Keycloak version compatibilityThe fixed version 17.0.1 for Keycloak dependencies needs verification for:
- Compatibility with other project dependencies
- Security vulnerabilities
- Alignment with project standards
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check for Keycloak versions across the project echo "Checking Keycloak versions across the project..." rg -A 1 "<artifactId>keycloak-[^<]+" | grep -A 1 "artifactId" # Check for security advisories echo "Checking for security advisories..." gh api graphql -f query=' { securityVulnerabilities(first: 5, ecosystem: MAVEN, package: "org.keycloak:keycloak-core") { nodes { advisory { summary severity publishedAt } vulnerableVersionRange firstPatchedVersion { identifier } } } }'Length of output: 2333
judo-runtime-core-guice-hsqldb/src/test/java/hu/blackbelt/judo/runtime/core/guice/hsqldb/JudoDefaultHsqldbModuleTest.java (2)
Line range hint
41-54: Add test cases for configuration resolutionThe test class doesn't cover the new configuration capabilities mentioned in the PR objectives (system properties, environment variables, and configuration files). Consider adding test cases that verify:
- System property resolution
- Environment variable resolution
- Configuration file loading
- Priority/fallback behavior between different sources
Would you like me to help create comprehensive test cases for these scenarios?
Line range hint
66-74: Enhance test coverage and cleanupThe current test coverage is minimal:
- The tearDown method is empty but might need to clean up any modified system properties or env vars
- The single test case only verifies DAO instantiation
- Missing validation of configuration resolution behavior
Consider restructuring the tests:
@Test void testSystemPropertyConfiguration() { System.setProperty("judo.test.property", "testValue"); try { // Test configuration resolution assertConfigurationValue("testValue"); } finally { System.clearProperty("judo.test.property"); } } @Test void testConfigurationPriority() { // Test that sources are checked in correct priority order } @Test void testConfigurationFallback() { // Test fallback behavior when primary source is unavailable }
🧹 Nitpick comments (3)
judo-runtime-core-guice-hsqldb/src/test/java/hu/blackbelt/judo/runtime/core/guice/hsqldb/JudoDefaultHsqldbModuleTest.java (1)
62-62: LGTM: Logging format improvementThe simplified milliseconds formatting improves readability.
Consider using SLF4J's placeholder syntax for better performance:
-log.info("Init: ⏱ " + timer.elapsed().toMillis()+ " ms"); +log.info("Init: ⏱ {} ms", timer.elapsed().toMillis());judo-runtime-core/src/main/java/hu/blackbelt/judo/runtime/core/utils/JudoProcessHandler.java (2)
95-95: Correct typographical errors in comments and log messagesThere are minor typographical errors that can be corrected for clarity:
- Line 95: The comment should read "If PID file is not valid, delete it" instead of "If pid file is not validate, delete it".
- Line 110: The log message should be "PID file already present for this application instance" instead of "Pid file already presented for this application instance".
Apply this diff to fix the typos:
// Line 95 -// If pid file is not validate, delete it +// If PID file is not valid, delete it // Line 110 -log.error("Pid file already presented for this application instance"); +log.error("PID file already present for this application instance");Also applies to: 110-110
96-97: Check the result of PID file deletion operationsWhen deleting the PID file using
pidFile.delete(), it's good practice to check the result to ensure the file was successfully deleted. If the deletion fails, appropriate action or logging can help in diagnosing potential issues.Consider modifying the code as follows:
// Lines 96-97 if (pid.isEmpty()) { - pidFile.delete(); + if (!pidFile.delete()) { + log.warn("Failed to delete invalid PID file: " + pidFile.getAbsolutePath()); + } } // Lines 106-107 } else { - pidFile.delete(); + if (!pidFile.delete()) { + log.warn("Failed to delete stale PID file: " + pidFile.getAbsolutePath()); + } } // Line 169 if (pidFile.exists()) { - pidFile.delete(); + if (!pidFile.delete()) { + log.warn("Failed to delete PID file during shutdown: " + pidFile.getAbsolutePath()); + } }Also applies to: 106-107, 169-169
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
judo-runtime-core-guice-cxf/src/test/java/hu/blackbelt/judo/runtime/core/guice/JudoCxfModuleTest.java(1 hunks)judo-runtime-core-guice-hsqldb/src/test/java/hu/blackbelt/judo/runtime/core/guice/hsqldb/JudoDefaultHsqldbModuleTest.java(1 hunks)judo-runtime-core-guice-jetty/src/test/java/hu/blackbelt/judo/runtime/core/jetty/guice/JudoJettyModuleTest.java(1 hunks)judo-runtime-core-guice-keycloak/src/test/java/hu/blackbelt/judo/runtime/core/security/keycloak/guice/JudoKeycloakModuleTest.java(2 hunks)judo-runtime-core-guice-postgresql/src/test/java/hu/blackbelt/judo/runtime/core/guice/postgresql/JudoDefaultPostgresqlModuleTest.java(1 hunks)judo-runtime-core-guice/src/main/java/hu/blackbelt/judo/runtime/core/guice/JudoModelLoader.java(2 hunks)judo-runtime-core-security-keycloak-cxf/pom.xml(2 hunks)judo-runtime-core/src/main/java/hu/blackbelt/judo/runtime/core/utils/JudoProcessHandler.java(1 hunks)judo-runtime-core/src/test/java/hu/blackbelt/judo/runtime/core/util/JudoProcessHandlerTest.java(1 hunks)
✅ Files skipped from review due to trivial changes (2)
- judo-runtime-core-guice-cxf/src/test/java/hu/blackbelt/judo/runtime/core/guice/JudoCxfModuleTest.java
- judo-runtime-core-guice-postgresql/src/test/java/hu/blackbelt/judo/runtime/core/guice/postgresql/JudoDefaultPostgresqlModuleTest.java
🚧 Files skipped from review as they are similar to previous changes (1)
- judo-runtime-core-guice-keycloak/src/test/java/hu/blackbelt/judo/runtime/core/security/keycloak/guice/JudoKeycloakModuleTest.java
🔇 Additional comments (6)
judo-runtime-core-guice-jetty/src/test/java/hu/blackbelt/judo/runtime/core/jetty/guice/JudoJettyModuleTest.java (2)
52-52: LGTM: Logging improvement
The change from nanoseconds to milliseconds improves log readability.
Line range hint 35-41: Verify integration with RuntimeVariableResolver
The AI summary mentions a new RuntimeVariableResolver mechanism, but this test doesn't appear to utilize it.
Let's check for related configuration classes:
judo-runtime-core-security-keycloak-cxf/pom.xml (1)
61-65: Verify version management for keycloak.model dependency
The removal of explicit version and scope suggests version management through the parent POM. Let's verify this setup to prevent potential version conflicts.
✅ Verification successful
Based on the shell script results, I can now provide the final response:
Version management for keycloak.model dependency is properly configured
The version management for hu.blackbelt.judo.meta.keycloak.model is correctly handled in the parent POM's dependency management section using the ${judo-meta-keycloak-version} property. The removal of explicit version and scope in the child POM is the correct approach as it inherits these settings from the parent.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check parent POM for version management of keycloak.model
fd -e xml pom.xml | xargs grep -l "hu.blackbelt.judo.meta.keycloak.model" | xargs cat
Length of output: 34324
judo-runtime-core-guice-hsqldb/src/test/java/hu/blackbelt/judo/runtime/core/guice/hsqldb/JudoDefaultHsqldbModuleTest.java (1)
Line range hint 41-54: Verify integration with RuntimeVariableResolver
The module configuration should integrate with the new RuntimeVariableResolver mechanism mentioned in the PR summary.
✅ Verification successful
Let me gather more information about the RuntimeVariableResolver implementation and its usage.
RuntimeVariableResolver integration is properly configured
The verification shows that the HSQLDB module is correctly integrated with RuntimeVariableResolver:
- The
JudoHsqldbModuleincludes RuntimeVariableResolver in its configuration - It's properly initialized with the "judo" prefix when not explicitly provided
- The module binds the RuntimeVariableResolver instance in multiple places
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for RuntimeVariableResolver implementation
echo "Searching for RuntimeVariableResolver implementation..."
rg -l "class.*RuntimeVariableResolver"
# Search for configuration-related Guice modules
echo "Searching for configuration-related Guice modules..."
rg -l "class.*ConfigurationModule"
# Search for configuration property usage
echo "Searching for configuration property usage..."
ast-grep --pattern 'getProperty($_, $_)'
Length of output: 656
Script:
#!/bin/bash
# Check RuntimeVariableResolver implementation
echo "Checking RuntimeVariableResolver implementation..."
rg -A 10 "class RuntimeVariableResolver" judo-runtime-core/src/main/java/hu/blackbelt/judo/runtime/core/utils/RuntimeVariableResolver.java
# Check if RuntimeVariableResolver is used in the HSQLDB module
echo "Checking RuntimeVariableResolver usage in HSQLDB module..."
rg "RuntimeVariableResolver" judo-runtime-core-guice-hsqldb/
# Check JudoHsqldbModule implementation
echo "Checking JudoHsqldbModule implementation..."
rg -A 10 "class JudoHsqldbModule" judo-runtime-core-guice-hsqldb/
Length of output: 8216
judo-runtime-core/src/main/java/hu/blackbelt/judo/runtime/core/utils/JudoProcessHandler.java (1)
130-134: Consider handling IOException when writing the PID file
When writing the PID file, catching the IOException and only logging an error may not be sufficient. Depending on the application's requirements, it might be necessary to handle the exception more robustly, such as retrying the operation or failing the startup process to prevent running without proper PID management.
Ensure that proceeding after a failed PID file write does not lead to unexpected behavior. If necessary, implement additional error handling.
judo-runtime-core/src/test/java/hu/blackbelt/judo/runtime/core/util/JudoProcessHandlerTest.java (1)
15-19: Verify the logic of assertions in testPid method
The test assertions may not align with the expected behavior of JudoProcessHandler. Typically:
- Before writing the PID,
checkPid()should returntrue(indicating no conflicting PID file exists), andisRunning()should returnfalse. - After writing the PID,
checkPid()might returnfalse(since the PID file now exists), andisRunning()should returntrue.
However, the current assertions may not accurately reflect this logic. Please review and adjust the test to match the intended behavior.
Apply this diff to correct the test assertions:
assertTrue(judoProcessHandler.checkPid());
assertFalse(judoProcessHandler.isRunning());
judoProcessHandler.writePid();
-assertFalse(judoProcessHandler.checkPid());
-assertTrue(judoProcessHandler.isRunning());
+assertFalse(judoProcessHandler.checkPid());
+assertTrue(judoProcessHandler.isRunning());
judo-runtime-core-guice/src/main/java/hu/blackbelt/judo/runtime/core/guice/JudoModelLoader.java
Outdated
Show resolved
Hide resolved
judo-runtime-core-guice/src/main/java/hu/blackbelt/judo/runtime/core/guice/JudoModelLoader.java
Outdated
Show resolved
Hide resolved
judo-runtime-core-guice/src/main/java/hu/blackbelt/judo/runtime/core/guice/JudoModelLoader.java
Outdated
Show resolved
Hide resolved
judo-runtime-core/src/main/java/hu/blackbelt/judo/runtime/core/utils/JudoProcessHandler.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
judo-runtime-core-guice-dependencies/pom.xml (1)
389-389: Remove or update the temporary comment.The comment indicates temporary compilation scope for mapper-impl. This should be reviewed and either removed or properly configured.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
judo-runtime-core-guice-dependencies/.project(1 hunks)judo-runtime-core-guice-dependencies/LICENSE.txt(1 hunks)judo-runtime-core-guice-dependencies/pom.xml(1 hunks)pom.xml(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- judo-runtime-core-guice-dependencies/.project
🧰 Additional context used
🪛 LanguageTool
judo-runtime-core-guice-dependencies/LICENSE.txt
[uncategorized] ~43-~43: Possible missing comma found.
Context: ...the Program, including, for purposes of clarity any new file in Source Code form that c...
(AI_HYDRA_LEO_MISSING_COMMA)
[style] ~47-~47: Consider a shorter alternative to avoid wordiness.
Context: ...iles of the Program solely in each case in order to link to, bind by name, or subclass the ...
(IN_ORDER_TO_PREMIUM)
[uncategorized] ~86-~86: Possible missing article found.
Context: ... Contributor disclaims any liability to Recipient for claims brought by any other entit...
(AI_HYDRA_LEO_MISSING_THE)
[uncategorized] ~92-~92: Possible missing article found.
Context: ...y patent license is required to allow Recipient to Distribute the Program, it is Reci...
(AI_HYDRA_LEO_MISSING_THE)
[uncategorized] ~93-~93: Possible missing article found.
Context: ...ient to Distribute the Program, it is Recipient's responsibility to acquire that licens...
(AI_HYDRA_LEO_MISSING_THE)
[style] ~114-~114: Consider replacing this phrase with the adverb “reasonably” to avoid wordiness.
Context: ...d informs Recipients how to obtain it in a reasonable manner on or through a medium customarily us...
(IN_A_X_MANNER)
[style] ~118-~118: Did you mean ‘different from’? ‘Different than’ is often considered colloquial style.
Context: ...the Program under a license different than this Agreement, provided that such lice...
(DIFFERENT_THAN)
[style] ~156-~156: ‘with respect to’ might be wordy. Consider a shorter alternative.
Context: ...are may accept certain responsibilities with respect to end users, business partners and the li...
(EN_WORDINESS_PREMIUM_WITH_RESPECT_TO)
[style] ~171-~171: Consider a shorter alternative to avoid wordiness.
Context: ...ged intellectual property infringement. In order to qualify, an Indemnified Contributor mus...
(IN_ORDER_TO_PREMIUM)
[uncategorized] ~234-~234: Possible missing article found.
Context: ... rights under this Agreement terminate, Recipient agrees to cease use and distribution of...
(AI_HYDRA_LEO_MISSING_THE)
[style] ~240-~240: Consider a shorter alternative to avoid wordiness.
Context: ...istribute copies of this Agreement, but in order to avoid inconsistency the Agreement is co...
(IN_ORDER_TO_PREMIUM)
[uncategorized] ~240-~240: Possible missing comma found.
Context: ...f this Agreement, but in order to avoid inconsistency the Agreement is copyrighted and may on...
(AI_HYDRA_LEO_MISSING_COMMA)
[uncategorized] ~250-~250: Possible missing article found.
Context: ... version of the Agreement is published, Contributor may elect to Distribute the Program (in...
(AI_HYDRA_LEO_MISSING_THE)
🔇 Additional comments (4)
pom.xml (1)
543-543: LGTM! Module addition is well-placed.
The new judo-runtime-core-guice-dependencies module is appropriately added to the modules section, maintaining a logical grouping with other Guice-related modules.
judo-runtime-core-guice-dependencies/pom.xml (2)
15-41: LGTM! Build configuration is well structured.
The flatten-maven-plugin configuration is appropriate for CI-friendly builds, correctly resolving dependency management elements.
46-49: 🛠️ Refactor suggestion
Consider upgrading Google Guice.
The current version (5.1.0) is relatively old. Consider upgrading to the latest stable version for potential bug fixes and security improvements.
judo-runtime-core-guice-dependencies/LICENSE.txt (1)
1-277: LGTM! Standard EPL-2.0 license.
The Eclipse Public License v2.0 is appropriate for this module and consistent with the project's licensing strategy.
🧰 Tools
🪛 LanguageTool
[uncategorized] ~43-~43: Possible missing comma found.
Context: ...the Program, including, for purposes of clarity any new file in Source Code form that c...
(AI_HYDRA_LEO_MISSING_COMMA)
[style] ~47-~47: Consider a shorter alternative to avoid wordiness.
Context: ...iles of the Program solely in each case in order to link to, bind by name, or subclass the ...
(IN_ORDER_TO_PREMIUM)
[uncategorized] ~86-~86: Possible missing article found.
Context: ... Contributor disclaims any liability to Recipient for claims brought by any other entit...
(AI_HYDRA_LEO_MISSING_THE)
[uncategorized] ~92-~92: Possible missing article found.
Context: ...y patent license is required to allow Recipient to Distribute the Program, it is Reci...
(AI_HYDRA_LEO_MISSING_THE)
[uncategorized] ~93-~93: Possible missing article found.
Context: ...ient to Distribute the Program, it is Recipient's responsibility to acquire that licens...
(AI_HYDRA_LEO_MISSING_THE)
[style] ~114-~114: Consider replacing this phrase with the adverb “reasonably” to avoid wordiness.
Context: ...d informs Recipients how to obtain it in a reasonable manner on or through a medium customarily us...
(IN_A_X_MANNER)
[style] ~118-~118: Did you mean ‘different from’? ‘Different than’ is often considered colloquial style.
Context: ...the Program under a license different than this Agreement, provided that such lice...
(DIFFERENT_THAN)
[style] ~156-~156: ‘with respect to’ might be wordy. Consider a shorter alternative.
Context: ...are may accept certain responsibilities with respect to end users, business partners and the li...
(EN_WORDINESS_PREMIUM_WITH_RESPECT_TO)
[style] ~171-~171: Consider a shorter alternative to avoid wordiness.
Context: ...ged intellectual property infringement. In order to qualify, an Indemnified Contributor mus...
(IN_ORDER_TO_PREMIUM)
[uncategorized] ~234-~234: Possible missing article found.
Context: ... rights under this Agreement terminate, Recipient agrees to cease use and distribution of...
(AI_HYDRA_LEO_MISSING_THE)
[style] ~240-~240: Consider a shorter alternative to avoid wordiness.
Context: ...istribute copies of this Agreement, but in order to avoid inconsistency the Agreement is co...
(IN_ORDER_TO_PREMIUM)
[uncategorized] ~240-~240: Possible missing comma found.
Context: ...f this Agreement, but in order to avoid inconsistency the Agreement is copyrighted and may on...
(AI_HYDRA_LEO_MISSING_COMMA)
[uncategorized] ~250-~250: Possible missing article found.
Context: ... version of the Agreement is published, Contributor may elect to Distribute the Program (in...
(AI_HYDRA_LEO_MISSING_THE)
robertcsakany
force-pushed
the
feature/JNG-6064_ConfigureGuice
branch
from
c3ad79b to
0349b46
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (2)
judo-runtime-core-guice-postgresql/src/main/java/hu/blackbelt/judo/runtime/core/guice/dao/rdbms/postgresql/JudoPostgresqlModule.java (1)
117-140:⚠️ Potential issueEnsure non-null
RuntimeVariableResolverbefore accessingWhen accessing
configuration.getRuntimeVariableResolver()inconfigureOptions(), ensure that it is notnullto prevent a potentialNullPointerException.Consider adding a null check:
Objects.requireNonNull(configuration.getRuntimeVariableResolver(), "RuntimeVariableResolver must not be null");Place this check before using
configuration.getRuntimeVariableResolver()to ensure safety.judo-runtime-core-guice-jetty/src/main/java/hu/blackbelt/judo/runtime/core/jetty/guice/JudoJettyModule.java (1)
96-120:⚠️ Potential issueEnsure non-null
RuntimeVariableResolverbefore accessingWhen accessing
configuration.getRuntimeVariableResolver()inconfigureOptions(), ensure that it is notnullto prevent a potentialNullPointerException.Add a null check:
Objects.requireNonNull(configuration.getRuntimeVariableResolver(), "RuntimeVariableResolver must not be null");Place this check before using
configuration.getRuntimeVariableResolver()to enhance robustness.
🧹 Nitpick comments (8)
judo-runtime-core-security-keycloak-cxf/pom.xml (1)
Line range hint
82-103: Consider managing Keycloak dependency versions in dependencyManagementThe Keycloak dependencies (keycloak-adapter-core, keycloak-common, and keycloak-core) all use version 17.0.1. Consider moving this version to the parent pom's dependencyManagement section for better version management across modules.
Add to parent pom.xml under
<dependencyManagement><dependencies>:<dependency> <groupId>org.keycloak</groupId> <artifactId>keycloak-adapter-core</artifactId> <version>17.0.1</version> </dependency> <dependency> <groupId>org.keycloak</groupId> <artifactId>keycloak-common</artifactId> <version>17.0.1</version> </dependency> <dependency> <groupId>org.keycloak</groupId> <artifactId>keycloak-core</artifactId> <version>17.0.1</version> </dependency>judo-runtime-core-guice-dependencies/pom.xml (3)
46-49: Consider updating outdated dependenciesSeveral dependencies are using older versions that could be updated:
- Guice 5.1.0 -> 7.0.0
- jgrapht-core 1.3.0 -> 1.5.2
- validation-api 2.0.1.Final -> jakarta.validation-api 3.0.2
- antlr-runtime 3.2 -> 4.13.1
These updates could provide security fixes, performance improvements, and better compatibility with modern Java versions.
Also applies to: 51-55, 57-61, 427-431
468-473: Clean up commented dependency blocksThere are several commented dependency blocks in the file. Consider either:
- Removing them if they're no longer needed
- Adding clear documentation explaining why they're kept for future reference
Also applies to: 823-829, 836-841, 904-909
43-1011: Improve dependency organizationConsider the following improvements:
- Group related dependencies together (e.g., all Jackson dependencies in one block)
- Define scopes consistently in dependencyManagement section only
- Add comments to separate major dependency groups
judo-runtime-core/src/main/java/hu/blackbelt/judo/runtime/core/utils/RuntimeVariableResolver.java (3)
20-20: Reuse the existingcoercerFactoryinstead of creating a new instanceAt line 47, you're creating a new
DefaultCoercerFactoryinstance instead of reusing the one initialized at line 20. To improve efficiency and maintain consistency, consider reusing the existingcoercerFactory:- this.coercer = new DefaultCoercerFactory().getCoercerInstance(); + this.coercer = coercerFactory.getCoercerInstance();Also applies to: 47-47
5-5: Remove unused importDefaultConverterFactoryThe import statement
import hu.blackbelt.mapper.impl.DefaultConverterFactory;at line 5 is not used in this class. Removing unused imports helps keep the code clean and improves maintainability.
55-57: Optimize variable resolver lookup with aMapIn the
getVariablesmethod, the nested loops overvariablePrecedenceandvariableResolverscan be inefficient if the number of resolvers grows. Consider indexing thevariableResolversby their names to improve lookup efficiency:+ Map<String, VariableResolver> resolverMap = new HashMap<>(); + for (VariableResolver variableResolver : variableResolvers) { + resolverMap.put(variableResolver.getName().toLowerCase(), variableResolver); + } for (String precedence : variablePrecedence) { - for (VariableResolver variableResolver : variableResolvers) { - if (precedence.equalsIgnoreCase(variableResolver.getName())) { - variables.putAll(variableResolver.process()); - } - } + VariableResolver variableResolver = resolverMap.get(precedence.toLowerCase()); + if (variableResolver != null) { + variables.putAll(variableResolver.process()); + } }This change reduces the time complexity from O(n*m) to O(n), enhancing performance.
judo-runtime-core-guice-keycloak/src/main/java/hu/blackbelt/judo/runtime/core/security/keycloak/guice/JudoKeycloakModule.java (1)
50-50: Consider adding null validation for RuntimeVariableResolver in builderWhile the constructor handles null RuntimeVariableResolver gracefully, it would be beneficial to add similar validation in the builder to fail fast if invalid configurations are provided.
@Builder public static class JudoKeycloakModuleBuilder { JudoKeycloakModuleConfiguration configuration = null; - RuntimeVariableResolver runtimeVariableResolver = null; + RuntimeVariableResolver runtimeVariableResolver = RuntimeVariableResolver.builder().prefix("judo").build();
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (37)
judo-runtime-core-guice-cxf/src/main/java/hu/blackbelt/judo/runtime/core/jaxrs/cxf/server/guice/JudoCxfModule.java(4 hunks)judo-runtime-core-guice-cxf/src/main/java/hu/blackbelt/judo/runtime/core/jaxrs/cxf/server/guice/JudoCxfModuleConfiguration.java(2 hunks)judo-runtime-core-guice-cxf/src/test/java/hu/blackbelt/judo/runtime/core/guice/JudoCxfModuleTest.java(1 hunks)judo-runtime-core-guice-dependencies/.project(1 hunks)judo-runtime-core-guice-dependencies/LICENSE.txt(1 hunks)judo-runtime-core-guice-dependencies/pom.xml(1 hunks)judo-runtime-core-guice-hsqldb/src/main/java/hu/blackbelt/judo/runtime/core/guice/dao/rdbms/hsqldb/JudoHsqldbModule.java(5 hunks)judo-runtime-core-guice-hsqldb/src/main/java/hu/blackbelt/judo/runtime/core/guice/dao/rdbms/hsqldb/JudoHsqldbModuleConfiguration.java(2 hunks)judo-runtime-core-guice-hsqldb/src/test/java/hu/blackbelt/judo/runtime/core/guice/hsqldb/JudoDefaultHsqldbModuleTest.java(1 hunks)judo-runtime-core-guice-jetty/pom.xml(1 hunks)judo-runtime-core-guice-jetty/src/main/java/hu/blackbelt/judo/runtime/core/jetty/guice/JudoJettyModule.java(6 hunks)judo-runtime-core-guice-jetty/src/test/java/hu/blackbelt/judo/runtime/core/jetty/guice/JudoJettyModuleTest.java(1 hunks)judo-runtime-core-guice-keycloak/src/main/java/hu/blackbelt/judo/runtime/core/security/keycloak/guice/JudoKeycloakModule.java(5 hunks)judo-runtime-core-guice-keycloak/src/main/java/hu/blackbelt/judo/runtime/core/security/keycloak/guice/JudoKeycloakModuleConfiguration.java(2 hunks)judo-runtime-core-guice-keycloak/src/test/java/hu/blackbelt/judo/runtime/core/security/keycloak/guice/JudoKeycloakModuleTest.java(2 hunks)judo-runtime-core-guice-postgresql/src/main/java/hu/blackbelt/judo/runtime/core/guice/dao/rdbms/postgresql/JudoPostgresqlModule.java(4 hunks)judo-runtime-core-guice-postgresql/src/main/java/hu/blackbelt/judo/runtime/core/guice/dao/rdbms/postgresql/JudoPostgresqlModuleConfiguration.java(2 hunks)judo-runtime-core-guice-postgresql/src/test/java/hu/blackbelt/judo/runtime/core/guice/postgresql/JudoDefaultPostgresqlModuleTest.java(1 hunks)judo-runtime-core-guice/src/main/java/hu/blackbelt/judo/runtime/core/guice/ComponentScanModule.java(1 hunks)judo-runtime-core-guice/src/main/java/hu/blackbelt/judo/runtime/core/guice/JudoDefaultModule.java(6 hunks)judo-runtime-core-guice/src/main/java/hu/blackbelt/judo/runtime/core/guice/JudoDefaultModuleConfiguration.java(2 hunks)judo-runtime-core-guice/src/main/java/hu/blackbelt/judo/runtime/core/guice/JudoModelLoader.java(2 hunks)judo-runtime-core-security-keycloak-cxf/pom.xml(2 hunks)judo-runtime-core-security-keycloak/src/main/java/hu/blackbelt/judo/runtime/core/security/keycloak/KeycloakRealmSynchronizer.java(0 hunks)judo-runtime-core/pom.xml(2 hunks)judo-runtime-core/src/main/java/hu/blackbelt/judo/runtime/core/utils/EnvironmentVariableResolver.java(1 hunks)judo-runtime-core/src/main/java/hu/blackbelt/judo/runtime/core/utils/JudoProcessHandler.java(1 hunks)judo-runtime-core/src/main/java/hu/blackbelt/judo/runtime/core/utils/PropertyFileVariableResolver.java(1 hunks)judo-runtime-core/src/main/java/hu/blackbelt/judo/runtime/core/utils/RuntimeVariableResolver.java(1 hunks)judo-runtime-core/src/main/java/hu/blackbelt/judo/runtime/core/utils/SystemPropertiesVariableResolver.java(1 hunks)judo-runtime-core/src/main/java/hu/blackbelt/judo/runtime/core/utils/VariableResolver.java(1 hunks)judo-runtime-core/src/test/java/hu/blackbelt/judo/runtime/core/util/JudoProcessHandlerTest.java(1 hunks)judo-runtime-core/src/test/java/hu/blackbelt/judo/runtime/core/util/RuntimeVariableResolverTest.java(1 hunks)judo-runtime-core/src/test/resources/testBooleanVariable.properties(1 hunks)judo-runtime-core/src/test/resources/testVariableNaming.properties(1 hunks)judo-runtime-core/src/test/resources/testVariablePrecedence.properties(1 hunks)pom.xml(2 hunks)
💤 Files with no reviewable changes (1)
- judo-runtime-core-security-keycloak/src/main/java/hu/blackbelt/judo/runtime/core/security/keycloak/KeycloakRealmSynchronizer.java
✅ Files skipped from review due to trivial changes (1)
- judo-runtime-core/src/test/resources/testBooleanVariable.properties
🚧 Files skipped from review as they are similar to previous changes (24)
- judo-runtime-core/src/test/resources/testVariablePrecedence.properties
- judo-runtime-core-guice-keycloak/src/main/java/hu/blackbelt/judo/runtime/core/security/keycloak/guice/JudoKeycloakModuleConfiguration.java
- judo-runtime-core/src/test/resources/testVariableNaming.properties
- judo-runtime-core-guice-keycloak/src/test/java/hu/blackbelt/judo/runtime/core/security/keycloak/guice/JudoKeycloakModuleTest.java
- judo-runtime-core-guice-dependencies/.project
- judo-runtime-core-guice-hsqldb/src/main/java/hu/blackbelt/judo/runtime/core/guice/dao/rdbms/hsqldb/JudoHsqldbModuleConfiguration.java
- judo-runtime-core-guice-jetty/pom.xml
- judo-runtime-core-guice/src/main/java/hu/blackbelt/judo/runtime/core/guice/ComponentScanModule.java
- judo-runtime-core-guice-cxf/src/main/java/hu/blackbelt/judo/runtime/core/jaxrs/cxf/server/guice/JudoCxfModuleConfiguration.java
- judo-runtime-core-guice/src/main/java/hu/blackbelt/judo/runtime/core/guice/JudoDefaultModuleConfiguration.java
- judo-runtime-core-guice-postgresql/src/main/java/hu/blackbelt/judo/runtime/core/guice/dao/rdbms/postgresql/JudoPostgresqlModuleConfiguration.java
- judo-runtime-core-guice-hsqldb/src/test/java/hu/blackbelt/judo/runtime/core/guice/hsqldb/JudoDefaultHsqldbModuleTest.java
- judo-runtime-core-guice-jetty/src/test/java/hu/blackbelt/judo/runtime/core/jetty/guice/JudoJettyModuleTest.java
- judo-runtime-core/src/test/java/hu/blackbelt/judo/runtime/core/util/JudoProcessHandlerTest.java
- judo-runtime-core-guice-cxf/src/test/java/hu/blackbelt/judo/runtime/core/guice/JudoCxfModuleTest.java
- judo-runtime-core/src/main/java/hu/blackbelt/judo/runtime/core/utils/SystemPropertiesVariableResolver.java
- judo-runtime-core/src/main/java/hu/blackbelt/judo/runtime/core/utils/EnvironmentVariableResolver.java
- judo-runtime-core/src/test/java/hu/blackbelt/judo/runtime/core/util/RuntimeVariableResolverTest.java
- judo-runtime-core-guice/src/main/java/hu/blackbelt/judo/runtime/core/guice/JudoModelLoader.java
- judo-runtime-core/src/main/java/hu/blackbelt/judo/runtime/core/utils/PropertyFileVariableResolver.java
- judo-runtime-core/src/main/java/hu/blackbelt/judo/runtime/core/utils/VariableResolver.java
- judo-runtime-core-guice-postgresql/src/test/java/hu/blackbelt/judo/runtime/core/guice/postgresql/JudoDefaultPostgresqlModuleTest.java
- judo-runtime-core-guice-cxf/src/main/java/hu/blackbelt/judo/runtime/core/jaxrs/cxf/server/guice/JudoCxfModule.java
- judo-runtime-core-guice/src/main/java/hu/blackbelt/judo/runtime/core/guice/JudoDefaultModule.java
🧰 Additional context used
🪛 LanguageTool
judo-runtime-core-guice-dependencies/LICENSE.txt
[style] ~47-~47: Consider a shorter alternative to avoid wordiness.
Context: ...iles of the Program solely in each case in order to link to, bind by name, or subclass the ...
(IN_ORDER_TO_PREMIUM)
[style] ~114-~114: Consider replacing this phrase with the adverb “reasonably” to avoid wordiness.
Context: ...d informs Recipients how to obtain it in a reasonable manner on or through a medium customarily us...
(IN_A_X_MANNER)
[style] ~118-~118: Did you mean ‘different from’? ‘Different than’ is often considered colloquial style.
Context: ...the Program under a license different than this Agreement, provided that such lice...
(DIFFERENT_THAN)
[style] ~156-~156: ‘with respect to’ might be wordy. Consider a shorter alternative.
Context: ...are may accept certain responsibilities with respect to end users, business partners and the li...
(EN_WORDINESS_PREMIUM_WITH_RESPECT_TO)
[style] ~171-~171: Consider a shorter alternative to avoid wordiness.
Context: ...ged intellectual property infringement. In order to qualify, an Indemnified Contributor mus...
(IN_ORDER_TO_PREMIUM)
[style] ~240-~240: Consider a shorter alternative to avoid wordiness.
Context: ...istribute copies of this Agreement, but in order to avoid inconsistency the Agreement is co...
(IN_ORDER_TO_PREMIUM)
🔇 Additional comments (11)
judo-runtime-core/pom.xml (2)
27-36: LGTM: maven-dependency-plugin configuration
The plugin configuration is correct for generating dependency properties.
38-48: Verify JVM arguments in surefire configuration
The configuration includes necessary module access for Java internals and proper encoding. However, ensure that the junit5-system-exit agent path is correctly resolved across different environments.
✅ Verification successful
JVM arguments in surefire configuration are properly configured
The junit5-system-exit dependency is correctly declared in the parent pom.xml with version 2.0.2 and test scope, and the javaagent path is properly resolved using Maven property syntax ${com.ginsberg:junit5-system-exit:jar}. This is a standard Maven practice for referencing dependency artifacts and will work correctly across different environments.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Verify the junit5-system-exit dependency resolution
mvn dependency:list | grep "junit5-system-exit"
Length of output: 141
Script:
#!/bin/bash
# Let's check if the dependency is declared in the pom.xml
rg -l "junit5-system-exit"
# Also check for any other surefire configurations in the project
rg -l "maven-surefire-plugin"
# Get the content of the pom.xml to verify the dependency declaration
cat judo-runtime-core/pom.xml
Length of output: 3978
Script:
#!/bin/bash
# Let's check the parent pom.xml to verify the junit5-system-exit dependency version and scope
cat pom.xml
# Also check if there are any other references to this dependency in other files
rg "junit5-system-exit" -B 2 -A 2
Length of output: 35789
judo-runtime-core-guice-dependencies/LICENSE.txt (1)
1-277: LGTM: Standard EPL 2.0 License
The file contains the complete and unmodified Eclipse Public License version 2.0, which is appropriate for this project.
🧰 Tools
🪛 LanguageTool
[style] ~47-~47: Consider a shorter alternative to avoid wordiness.
Context: ...iles of the Program solely in each case in order to link to, bind by name, or subclass the ...
(IN_ORDER_TO_PREMIUM)
[style] ~114-~114: Consider replacing this phrase with the adverb “reasonably” to avoid wordiness.
Context: ...d informs Recipients how to obtain it in a reasonable manner on or through a medium customarily us...
(IN_A_X_MANNER)
[style] ~118-~118: Did you mean ‘different from’? ‘Different than’ is often considered colloquial style.
Context: ...the Program under a license different than this Agreement, provided that such lice...
(DIFFERENT_THAN)
[style] ~156-~156: ‘with respect to’ might be wordy. Consider a shorter alternative.
Context: ...are may accept certain responsibilities with respect to end users, business partners and the li...
(EN_WORDINESS_PREMIUM_WITH_RESPECT_TO)
[style] ~171-~171: Consider a shorter alternative to avoid wordiness.
Context: ...ged intellectual property infringement. In order to qualify, an Indemnified Contributor mus...
(IN_ORDER_TO_PREMIUM)
[style] ~240-~240: Consider a shorter alternative to avoid wordiness.
Context: ...istribute copies of this Agreement, but in order to avoid inconsistency the Agreement is co...
(IN_ORDER_TO_PREMIUM)
pom.xml (3)
397-414: LGTM: Test dependencies properly configured
The new test dependencies are correctly scoped and versioned.
563-563: LGTM: New Guice dependencies module added
The addition of judo-runtime-core-guice-dependencies module aligns with the PR objectives.
Line range hint 554-568: Critical: Update Keycloak dependencies to address security vulnerabilities
The current version 17.0.1 contains multiple security vulnerabilities that need to be addressed by upgrading to version 26.0.7 or later.
judo-runtime-core-guice-dependencies/pom.xml (1)
1-41: LGTM: Module configuration follows best practices
The module is properly configured with parent reference and flatten-maven-plugin for CI-friendly version handling.
judo-runtime-core-guice-keycloak/src/main/java/hu/blackbelt/judo/runtime/core/security/keycloak/guice/JudoKeycloakModule.java (2)
Line range hint 1-290: Verify configuration resolution order and potential conflicts
The module uses multiple configuration sources (RuntimeVariableResolver, default values, builder parameters). Ensure that:
- The configuration resolution order is documented
- There are no conflicts between different configuration sources
- The prefix "judo" is consistently used across all configuration sources
Let's verify the configuration usage:
✅ Verification successful
Based on the shell script results, I can now generate the final response:
Configuration resolution is properly implemented with consistent prefix and clear precedence
The configuration resolution follows a clear and consistent pattern:
- All configuration values are resolved through RuntimeVariableResolver with "judo" prefix
- Default values are provided via JudoKeycloakModuleConfiguration.DEFAULT
- Builder parameters can override defaults
- Environment variables/system properties can override both through RuntimeVariableResolver
The implementation ensures there are no conflicts between configuration sources due to:
- Consistent use of RuntimeVariableResolver for all config values
- Each config key has a single corresponding getter in the configuration class
- Clear fallback chain: env vars -> builder params -> defaults
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check for other configuration resolvers that might conflict
ast-grep --pattern 'RuntimeVariableResolver.$$$prefix$$$'
# Check for environment variable usage patterns
rg -A 2 'getVariableAs(String|Boolean|Integer|Long)'
Length of output: 40889
106-107: Consider using a secure configuration source for sensitive data
The RuntimeVariableResolver is used to handle sensitive information like admin credentials and client secrets. Ensure that the configuration source (environment variables, system properties) is properly secured and encrypted when necessary.
Let's verify the sensitive configuration handling:
✅ Verification successful
Based on the code analysis, I can now generate the final response. The RuntimeVariableResolver implementation and its usage in the security context shows proper handling of sensitive data:
Sensitive data handling is properly implemented
The codebase demonstrates secure configuration handling through:
- Environment variables and system properties are accessed through abstracted variable resolvers
- User credentials and sensitive data are managed through the UserManager interface with proper encapsulation
- The RuntimeVariableResolver uses a precedence-based resolution strategy that allows secure configuration sources
- No hardcoded secrets or credentials were found in the codebase
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check for sensitive data in configuration files
rg -i "password|secret|credential" --type yaml --type properties --type xml --type json
# Check for environment variables containing sensitive data
rg -i "getenv.*password|getenv.*secret|getenv.*credential"
Length of output: 185
Script:
#!/bin/bash
# Let's search for sensitive data handling patterns in the codebase
rg -i "password|secret|credential" --type-add 'props:*.properties' --type yaml --type xml --type json --type props
# Check RuntimeVariableResolver usage and implementation
ast-grep --pattern 'class RuntimeVariableResolver {
$$$
}'
# Search for environment variable access patterns
rg -i "System\.getenv|System\.getProperty"
# Look for security-related configuration in the specific module
fd . judo-runtime-core-guice-keycloak -t f -e xml -e properties -e yaml -e json --exec cat {}
Length of output: 7383
Script:
#!/bin/bash
# Let's examine the RuntimeVariableResolver implementation and usage
rg -A 10 "class RuntimeVariableResolver"
# Check for variable resolution implementation details
rg -A 10 "RuntimeVariableResolver.*build"
# Look for actual sensitive data handling in the security module
fd . "judo-runtime-core-security" -t f -e java --exec cat {}
Length of output: 46529
judo-runtime-core/src/main/java/hu/blackbelt/judo/runtime/core/utils/JudoProcessHandler.java (1)
1-164: Code implementation is sound and well-structured
The JudoProcessHandler class effectively manages PID files and the process lifecycle. The usage of ProcessHandle.current().pid() ensures reliable PID retrieval, and the methods are well-organized with proper error handling.
judo-runtime-core-guice-hsqldb/src/main/java/hu/blackbelt/judo/runtime/core/guice/dao/rdbms/hsqldb/JudoHsqldbModule.java (1)
Line range hint 66-124: Integration of RuntimeVariableResolver is implemented correctly
The inclusion of the null check for RuntimeVariableResolver ensures safety and prevents potential NullPointerException. The dynamic resolution of configuration parameters enhances flexibility.
...runtime-core/src/main/java/hu/blackbelt/judo/runtime/core/utils/RuntimeVariableResolver.java
Show resolved
Hide resolved
| private String getEffectiveVariableName(String name) { | ||
| String variableName = name; | ||
| if (!variableName.startsWith(prefix)) { | ||
| variableName = prefix + VariableResolver.firstToUpper(variableName); | ||
| } | ||
| return variableName; | ||
| } |
There was a problem hiding this comment.
Ensure prefix is not null to prevent potential NullPointerException
In the getEffectiveVariableName method, if prefix is null, concatenating it with variableName at line 66 may result in a NullPointerException. To prevent this, consider initializing prefix to an empty string if it is null:
public RuntimeVariableResolver(String prefix,
List<String> variablePrecedence,
Collection<VariableResolver> variableResolvers) {
- this.prefix = prefix;
+ this.prefix = prefix != null ? prefix : "";Committable suggestion skipped: line range outside the PR's diff.
|
|
||
| bind(String.class).annotatedWith(KeycloakConfigurationQualifiers.KeycloakPublicUrl.class) | ||
| .toInstance(configuration.getRuntimeVariableResolver() | ||
| .getVariableAsString("keycloakPublicUrl", | ||
| configuration.getKeycloakPublicUrl())); | ||
|
|
||
| bind(String.class).annotatedWith(KeycloakConfigurationQualifiers.KeycloakAdminUser.class) | ||
| .toInstance(configuration.getRuntimeVariableResolver() | ||
| .getVariableAsString("keycloakAdminUser", | ||
| configuration.getKeycloakAdminUser())); | ||
|
|
||
| bind(String.class).annotatedWith(KeycloakConfigurationQualifiers.KeycloakAdminPassword.class) | ||
| .toInstance(configuration.getRuntimeVariableResolver() | ||
| .getVariableAsString("keycloakAdminPassword", | ||
| configuration.getKeycloakAdminPassword())); | ||
|
|
||
| bind(String.class).annotatedWith(KeycloakConfigurationQualifiers.KeycloakClientSecret.class) | ||
| .toInstance(configuration.getRuntimeVariableResolver() | ||
| .getVariableAsString("keycloakClientSecret", | ||
| configuration.getKeycloakClientSecret())); | ||
|
|
||
| bind(Boolean.class).annotatedWith(KeycloakConfigurationQualifiers.KeycloakRealmSynchronizerSupportLoginByEmail.class) | ||
| .toInstance(configuration.getRuntimeVariableResolver() | ||
| .getVariableAsBoolean("keycloakRealmSynchronizerSupportLoginByEmail", | ||
| configuration.getKeycloakRealmSynchronizerSupportLoginByEmail())); | ||
|
|
||
| bind(String.class).annotatedWith(KeycloakConfigurationQualifiers.KeycloakRealmSynchronizerClientAccessTypeForHuman.class) | ||
| .toInstance(configuration.getRuntimeVariableResolver() | ||
| .getVariableAsString("keycloakRealmSynchronizerClientAccessTypeForHuman", | ||
| configuration.getKeycloakRealmSynchronizerClientAccessTypeForHuman())); | ||
|
|
||
| bind(String.class).annotatedWith(KeycloakConfigurationQualifiers.KeycloakRealmSynchronizerClientAccessTypeForSystem.class) | ||
| .toInstance(configuration.getRuntimeVariableResolver() | ||
| .getVariableAsString("keycloakRealmSynchronizerClientAccessTypeForSystem", | ||
| configuration.getKeycloakRealmSynchronizerClientAccessTypeForSystem())); | ||
|
|
||
| bind(String.class).annotatedWith(KeycloakConfigurationQualifiers.KeycloakRealmSynchronizerCorsAllowOrigin.class) | ||
| .toInstance(configuration.getRuntimeVariableResolver() | ||
| .getVariableAsString("keycloakRealmSynchronizerCorsAllowOrigin", | ||
| configuration.getKeycloakRealmSynchronizerCorsAllowOrigin())); | ||
|
|
||
| bind(Boolean.class).annotatedWith(KeycloakConfigurationQualifiers.KeycloakRealmSynchronizerAsyncServiceCall.class) | ||
| .toInstance(configuration.getRuntimeVariableResolver() | ||
| .getVariableAsBoolean("keycloakRealmSynchronizerAsyncServiceCall", | ||
| configuration.getKeycloakRealmSynchronizerAsyncServiceCall())); | ||
|
|
||
| bind(Integer.class).annotatedWith(KeycloakConfigurationQualifiers.KeycloakRealmSynchronizerRetryMaxAttempts.class) | ||
| .toInstance(configuration.getRuntimeVariableResolver() | ||
| .getVariableAsInteger("keycloakRealmSynchronizerRetryMaxAttempts", | ||
| configuration.getKeycloakRealmSynchronizerRetryMaxAttempts())); | ||
|
|
||
| bind(Boolean.class).annotatedWith(KeycloakConfigurationQualifiers.KeycloakRealmSynchronizerRetryExponentialBackoff.class) | ||
| .toInstance(configuration.getRuntimeVariableResolver() | ||
| .getVariableAsBoolean("keycloakRealmSynchronizerRetryExponentialBackoff", | ||
| configuration.getKeycloakRealmSynchronizerRetryExponentialBackoff())); | ||
|
|
||
| bind(Long.class).annotatedWith(KeycloakConfigurationQualifiers.KeycloakRealmSynchronizerRetryWaitDuration.class) | ||
| .toInstance(configuration.getRuntimeVariableResolver() | ||
| .getVariableAsLong("keycloakRealmSynchronizerRetryWaitDuration", | ||
| configuration.getKeycloakRealmSynchronizerRetryWaitDuration())); | ||
|
|
||
| bind(Boolean.class).annotatedWith(KeycloakConfigurationQualifiers.KeycloakUserManagerEnabled.class) | ||
| .toInstance(configuration.getRuntimeVariableResolver() | ||
| .getVariableAsBoolean("keycloakUserManagerEnabled", | ||
| configuration.getKeycloakUserManagerEnabled())); | ||
|
|
||
| bind(Boolean.class).annotatedWith(KeycloakConfigurationQualifiers.KeycloakUserManagerUpdateExistingUsers.class) | ||
| .toInstance(configuration.getRuntimeVariableResolver() | ||
| .getVariableAsBoolean("keycloakUserManagerUpdateExistingUsers", | ||
| configuration.getKeycloakUserManagerUpdateExistingUsers())); | ||
|
|
||
| bind(String.class).annotatedWith(KeycloakConfigurationQualifiers.KeycloakUserManagerRequiredActions.class) | ||
| .toInstance(configuration.getRuntimeVariableResolver() | ||
| .getVariableAsString("keycloakUserManagerRequiredActions", | ||
| configuration.getKeycloakUserManagerRequiredActions())); | ||
|
|
||
| bind(Boolean.class).annotatedWith(KeycloakConfigurationQualifiers.KeycloakUserManagerAsyncServiceCall.class) | ||
| .toInstance(configuration.getRuntimeVariableResolver() | ||
| .getVariableAsBoolean("keycloakUserManagerAsyncServiceCall", | ||
| configuration.getKeycloakUserManagerAsyncServiceCall())); | ||
|
|
||
| bind(Integer.class).annotatedWith(KeycloakConfigurationQualifiers.KeycloakUserManagerRetryMaxAttempts.class) | ||
| .toInstance(configuration.getRuntimeVariableResolver() | ||
| .getVariableAsInteger("keycloakUserManagerRetryMaxAttempts", | ||
| configuration.getKeycloakUserManagerRetryMaxAttempts())); | ||
|
|
||
| bind(Boolean.class).annotatedWith(KeycloakConfigurationQualifiers.KeycloakUserManagerRetryExponentialBackoff.class) | ||
| .toInstance(configuration.getRuntimeVariableResolver() | ||
| .getVariableAsBoolean("keycloakUserManagerRetryExponentialBackoff", | ||
| configuration.getKeycloakUserManagerRetryExponentialBackoff())); | ||
|
|
||
| bind(Long.class).annotatedWith(KeycloakConfigurationQualifiers.KeycloakUserManagerRetryWaitDuration.class) | ||
| .toInstance(configuration.getRuntimeVariableResolver() | ||
| .getVariableAsLong("keycloakUserManagerRetryWaitDuration", | ||
| configuration.getKeycloakUserManagerRetryWaitDuration())); | ||
|
|
||
| bind(String.class).annotatedWith(KeycloakConfigurationQualifiers.KeycloakSecurityPasswordPolicyType.class) | ||
| .toInstance(configuration.getRuntimeVariableResolver() | ||
| .getVariableAsString("keycloakSecurityPasswordPolicyType", | ||
| configuration.getKeycloakSecurityPasswordPolicyType())); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider improving error handling and method organization
The configureOptions method has grown quite large and handles multiple configuration bindings. Consider the following improvements:
- Add error handling for configuration resolution failures
- Extract configuration bindings into logical groups
- Add logging for configuration values (excluding sensitive data)
Here's a suggested approach to improve the code:
+ private void bindSecurityConfig() {
+ bindWithLogging(String.class, KeycloakConfigurationQualifiers.KeycloakServerUrl.class,
+ () -> configuration.getRuntimeVariableResolver()
+ .getVariableAsString("keycloakServerUrl", configuration.getKeycloakServerUrl()));
+ // ... other security related bindings
+ }
+ private void bindUserManagerConfig() {
+ bindWithLogging(Boolean.class, KeycloakConfigurationQualifiers.KeycloakUserManagerEnabled.class,
+ () -> configuration.getRuntimeVariableResolver()
+ .getVariableAsBoolean("keycloakUserManagerEnabled", configuration.getKeycloakUserManagerEnabled()));
+ // ... other user manager related bindings
+ }
+ private <T> void bindWithLogging(Class<T> type, Class<? extends Annotation> annotation, Supplier<T> valueSupplier) {
+ try {
+ T value = valueSupplier.get();
+ bind(type).annotatedWith(annotation).toInstance(value);
+ if (!isSensitiveConfig(annotation)) {
+ log.debug("Bound configuration {} = {}", annotation.getSimpleName(), value);
+ }
+ } catch (Exception e) {
+ log.error("Failed to bind configuration {}: {}", annotation.getSimpleName(), e.getMessage());
+ throw e;
+ }
+ }Committable suggestion skipped: line range outside the PR's diff.
robertcsakany
force-pushed
the
feature/JNG-6064_ConfigureGuice
branch
from
0349b46 to
5fd0319
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
judo-runtime-core-guice-cxf/src/main/java/hu/blackbelt/judo/runtime/core/jaxrs/cxf/server/guice/JudoCxfModule.java (1)
150-238: 🛠️ Refactor suggestionRefactor configureOptions for better maintainability.
The method has grown too large with repetitive binding patterns. Consider:
- Grouping related configurations
- Adding error handling
- Improving logging
Here's a suggested approach:
+ private <T> void bindWithLogging(Class<T> type, Class<? extends Annotation> annotation, + String key, T defaultValue) { + try { + T value = configuration.getRuntimeVariableResolver() + .getVariableAs(type, key, defaultValue); + bind(type).annotatedWith(annotation).toInstance(value); + if (!isSensitiveConfig(key)) { + log.debug("CXF configuration: {} = {}", key, value); + } + } catch (Exception e) { + log.error("Failed to bind configuration {}: {}", key, e.getMessage()); + throw new RuntimeException("Configuration binding failed", e); + } + } + private void bindServerConfig() { + bindWithLogging(String.class, + CxfConfigurations.CxfJaxRsServerUrl.class, + "CxfJaxRsServerUrl", + configuration.getCxfJaxRsServerUrl()); + bindWithLogging(String.class, + CxfConfigurations.CxfJaxRsServerPath.class, + "CxfJaxRsServerPath", + configuration.getCxfJaxRsServerPath()); + } + private void bindCorsConfig() { + bindWithLogging(String.class, + CxfConfigurations.CxfCorsAllowOrigin.class, + "CxfCorsAllowOrigin", + configuration.getCorsAllowOrigin()); + // ... other CORS related bindings + } protected void configureOptions() { - // ... current implementation ... + bindServerConfig(); + bindCorsConfig(); }judo-runtime-core-guice/src/main/java/hu/blackbelt/judo/runtime/core/guice/JudoDefaultModule.java (1)
84-85:⚠️ Potential issueFix variable initialization in builder.
There's a potential issue with the initialization of configuration fields.
Apply this diff to fix the initialization:
- JudoDefaultModuleConfiguration configuration = null; - RuntimeVariableResolver runtimeVariableResolver = null; + private JudoDefaultModuleConfiguration configuration = null; + private RuntimeVariableResolver runtimeVariableResolver = null;
🧹 Nitpick comments (8)
judo-runtime-core/src/main/java/hu/blackbelt/judo/runtime/core/utils/RuntimeVariableResolver.java (1)
26-32: Clarify the necessity of the manualRuntimeVariableResolverBuilderclassThe manually defined
RuntimeVariableResolverBuilderclass may cause confusion since Lombok's@Builderannotation at line 34 generates a builder automatically. If this manual builder is essential for handling defaults correctly, consider adding documentation to explain its purpose and how it integrates with Lombok's builder pattern.judo-runtime-core/src/main/java/hu/blackbelt/judo/runtime/core/utils/PropertyFileVariableResolver.java (3)
11-16: Remove unused imports to improve code cleanlinessThe imports
FileReaderandCharsetsappear to be unused after the changes. Removing unused imports can improve code readability and reduce potential confusion.Apply this diff to remove the unused imports:
import com.google.common.base.Charsets; import com.google.common.collect.Maps; import lombok.AllArgsConstructor; import lombok.Builder; import lombok.NonNull; import lombok.extern.slf4j.Slf4j; import java.io.File; -import java.io.FileReader; import java.io.IOException; -import java.io.Reader; import java.nio.charset.StandardCharsets; import java.nio.file.Files; import java.util.*;
46-57: Ensure proper exception handling and resource managementThe
process()method handlesIOExceptionby wrapping it in aRuntimeException, which may not provide sufficient context. Additionally, consider logging the exception details for better diagnostics.Apply this diff to enhance exception handling:
try (Reader reader = Files.newBufferedReader(parameterFile.toPath(), StandardCharsets.UTF_8)) { prop.load(reader); variables.putAll(VariableResolver.generalizeTemplateVariableNames( Maps.fromProperties(prop).entrySet().stream().collect(Collectors.toMap( e -> String.valueOf(e.getKey()), e -> String.valueOf(e.getValue()), (prev, next) -> next, HashMap::new )))); -} catch (IOException e) { - throw new RuntimeException(e); +} catch (IOException ioException) { + log.error("Failed to load properties from file: " + parameterFile.getAbsolutePath(), ioException); + throw new RuntimeException("Error processing property file: " + parameterFile.getName(), ioException); }
58-59: Use parameterized logging for better performance and securityWhen logging messages, use parameterized logging to prevent unnecessary string concatenation and potential security issues.
Apply this diff:
- log.warn("File is missing:" + parameterFile.getAbsolutePath()); + log.warn("File is missing: {}", parameterFile.getAbsolutePath());judo-runtime-core/src/main/java/hu/blackbelt/judo/runtime/core/utils/JudoProcessHandler.java (2)
50-67: SimplifygetApplicationRoot()method and handle exceptions appropriatelyThe method
getApplicationRoot()is complex and may fail in certain environments. Consider simplifying it and handling exceptions more gracefully.Proposed refactor:
public File getApplicationRoot() { try { - String className = JudoProcessHandler.class.getName().replace('.', '/') + ".class"; - URI rootUri = null; - try { - rootUri = Thread.currentThread().getContextClassLoader().getResource("").toURI(); - } catch (Exception e) { - rootUri = this.getClass().getProtectionDomain().getCodeSource().getLocation().toURI(); - } - if (rootUri.toURL().toString().endsWith(".jar")) { - rootUri = Path.of(rootUri).getParent().toUri(); - } - File pidPath = Path.of(rootUri).toFile(); - return pidPath; + Path path = Path.of(this.getClass().getProtectionDomain().getCodeSource().getLocation().toURI()).getParent(); + return path.toFile(); } catch (URISyntaxException e) { - throw new RuntimeException(e); + log.error("Failed to determine application root", e); + return new File("."); } }
150-163: Ensure shutdown hook cleans up resources properlyThe
addShutdownHandlermethod should ensure that all resources are properly released on shutdown. Consider adding more comprehensive cleanup logic if necessary.Additionally, verify that the shutdown hook is not added multiple times, which could cause unexpected behavior.
judo-runtime-core-guice-hsqldb/src/main/java/hu/blackbelt/judo/runtime/core/guice/dao/rdbms/hsqldb/JudoHsqldbModule.java (2)
Line range hint
94-95: Consider adding validation for the prefix value.The default prefix "judo" is hardcoded. Consider making it configurable or documenting why this specific prefix is used.
- () -> RuntimeVariableResolver.builder().prefix("judo").build())) + () -> RuntimeVariableResolver.builder() + .prefix(Objects.requireNonNullElse(System.getProperty("judo.prefix"), "judo")) + .build()))
109-124: Add logging for resolved configuration values.The configuration values are being resolved but not logged. Consider adding debug logging to help with troubleshooting.
+ private void logConfigValue(String key, Object value) { + if (log.isDebugEnabled()) { + log.debug("HSQLDB configuration: {} = {}", key, value); + } + } bind(Integer.class).annotatedWith(HsqlDbConfigurationQualifier.HsqldbServerPort.class) .toInstance(runtimeVariableResolver .getVariableAsInteger("hsqldbServerPort", - configuration.getPort())); + configuration.getPort())); + logConfigValue("hsqldbServerPort", configuration.getPort());
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (37)
judo-runtime-core-guice-cxf/src/main/java/hu/blackbelt/judo/runtime/core/jaxrs/cxf/server/guice/JudoCxfModule.java(4 hunks)judo-runtime-core-guice-cxf/src/main/java/hu/blackbelt/judo/runtime/core/jaxrs/cxf/server/guice/JudoCxfModuleConfiguration.java(2 hunks)judo-runtime-core-guice-cxf/src/test/java/hu/blackbelt/judo/runtime/core/guice/JudoCxfModuleTest.java(1 hunks)judo-runtime-core-guice-dependencies/.project(1 hunks)judo-runtime-core-guice-dependencies/LICENSE.txt(1 hunks)judo-runtime-core-guice-dependencies/pom.xml(1 hunks)judo-runtime-core-guice-hsqldb/src/main/java/hu/blackbelt/judo/runtime/core/guice/dao/rdbms/hsqldb/JudoHsqldbModule.java(5 hunks)judo-runtime-core-guice-hsqldb/src/main/java/hu/blackbelt/judo/runtime/core/guice/dao/rdbms/hsqldb/JudoHsqldbModuleConfiguration.java(2 hunks)judo-runtime-core-guice-hsqldb/src/test/java/hu/blackbelt/judo/runtime/core/guice/hsqldb/JudoDefaultHsqldbModuleTest.java(1 hunks)judo-runtime-core-guice-jetty/pom.xml(1 hunks)judo-runtime-core-guice-jetty/src/main/java/hu/blackbelt/judo/runtime/core/jetty/guice/JudoJettyModule.java(6 hunks)judo-runtime-core-guice-jetty/src/test/java/hu/blackbelt/judo/runtime/core/jetty/guice/JudoJettyModuleTest.java(1 hunks)judo-runtime-core-guice-keycloak/src/main/java/hu/blackbelt/judo/runtime/core/security/keycloak/guice/JudoKeycloakModule.java(5 hunks)judo-runtime-core-guice-keycloak/src/main/java/hu/blackbelt/judo/runtime/core/security/keycloak/guice/JudoKeycloakModuleConfiguration.java(2 hunks)judo-runtime-core-guice-keycloak/src/test/java/hu/blackbelt/judo/runtime/core/security/keycloak/guice/JudoKeycloakModuleTest.java(2 hunks)judo-runtime-core-guice-postgresql/src/main/java/hu/blackbelt/judo/runtime/core/guice/dao/rdbms/postgresql/JudoPostgresqlModule.java(4 hunks)judo-runtime-core-guice-postgresql/src/main/java/hu/blackbelt/judo/runtime/core/guice/dao/rdbms/postgresql/JudoPostgresqlModuleConfiguration.java(2 hunks)judo-runtime-core-guice-postgresql/src/test/java/hu/blackbelt/judo/runtime/core/guice/postgresql/JudoDefaultPostgresqlModuleTest.java(1 hunks)judo-runtime-core-guice/src/main/java/hu/blackbelt/judo/runtime/core/guice/ComponentScanModule.java(1 hunks)judo-runtime-core-guice/src/main/java/hu/blackbelt/judo/runtime/core/guice/JudoDefaultModule.java(6 hunks)judo-runtime-core-guice/src/main/java/hu/blackbelt/judo/runtime/core/guice/JudoDefaultModuleConfiguration.java(2 hunks)judo-runtime-core-guice/src/main/java/hu/blackbelt/judo/runtime/core/guice/JudoModelLoader.java(2 hunks)judo-runtime-core-security-keycloak-cxf/pom.xml(2 hunks)judo-runtime-core-security-keycloak/src/main/java/hu/blackbelt/judo/runtime/core/security/keycloak/KeycloakRealmSynchronizer.java(0 hunks)judo-runtime-core/pom.xml(2 hunks)judo-runtime-core/src/main/java/hu/blackbelt/judo/runtime/core/utils/EnvironmentVariableResolver.java(1 hunks)judo-runtime-core/src/main/java/hu/blackbelt/judo/runtime/core/utils/JudoProcessHandler.java(1 hunks)judo-runtime-core/src/main/java/hu/blackbelt/judo/runtime/core/utils/PropertyFileVariableResolver.java(1 hunks)judo-runtime-core/src/main/java/hu/blackbelt/judo/runtime/core/utils/RuntimeVariableResolver.java(1 hunks)judo-runtime-core/src/main/java/hu/blackbelt/judo/runtime/core/utils/SystemPropertiesVariableResolver.java(1 hunks)judo-runtime-core/src/main/java/hu/blackbelt/judo/runtime/core/utils/VariableResolver.java(1 hunks)judo-runtime-core/src/test/java/hu/blackbelt/judo/runtime/core/util/JudoProcessHandlerTest.java(1 hunks)judo-runtime-core/src/test/java/hu/blackbelt/judo/runtime/core/util/RuntimeVariableResolverTest.java(1 hunks)judo-runtime-core/src/test/resources/testBooleanVariable.properties(1 hunks)judo-runtime-core/src/test/resources/testVariableNaming.properties(1 hunks)judo-runtime-core/src/test/resources/testVariablePrecedence.properties(1 hunks)pom.xml(2 hunks)
💤 Files with no reviewable changes (1)
- judo-runtime-core-security-keycloak/src/main/java/hu/blackbelt/judo/runtime/core/security/keycloak/KeycloakRealmSynchronizer.java
🚧 Files skipped from review as they are similar to previous changes (26)
- judo-runtime-core/src/test/resources/testVariableNaming.properties
- judo-runtime-core/src/test/resources/testVariablePrecedence.properties
- judo-runtime-core-guice-keycloak/src/main/java/hu/blackbelt/judo/runtime/core/security/keycloak/guice/JudoKeycloakModuleConfiguration.java
- judo-runtime-core-guice-hsqldb/src/test/java/hu/blackbelt/judo/runtime/core/guice/hsqldb/JudoDefaultHsqldbModuleTest.java
- judo-runtime-core-guice-postgresql/src/main/java/hu/blackbelt/judo/runtime/core/guice/dao/rdbms/postgresql/JudoPostgresqlModuleConfiguration.java
- judo-runtime-core-guice-hsqldb/src/main/java/hu/blackbelt/judo/runtime/core/guice/dao/rdbms/hsqldb/JudoHsqldbModuleConfiguration.java
- judo-runtime-core-guice-keycloak/src/test/java/hu/blackbelt/judo/runtime/core/security/keycloak/guice/JudoKeycloakModuleTest.java
- judo-runtime-core-guice-cxf/src/main/java/hu/blackbelt/judo/runtime/core/jaxrs/cxf/server/guice/JudoCxfModuleConfiguration.java
- judo-runtime-core-guice-jetty/pom.xml
- judo-runtime-core-guice/src/main/java/hu/blackbelt/judo/runtime/core/guice/ComponentScanModule.java
- judo-runtime-core-guice-postgresql/src/test/java/hu/blackbelt/judo/runtime/core/guice/postgresql/JudoDefaultPostgresqlModuleTest.java
- judo-runtime-core-guice-dependencies/.project
- judo-runtime-core/src/test/resources/testBooleanVariable.properties
- judo-runtime-core-guice-jetty/src/test/java/hu/blackbelt/judo/runtime/core/jetty/guice/JudoJettyModuleTest.java
- judo-runtime-core-guice-cxf/src/test/java/hu/blackbelt/judo/runtime/core/guice/JudoCxfModuleTest.java
- judo-runtime-core/src/main/java/hu/blackbelt/judo/runtime/core/utils/EnvironmentVariableResolver.java
- judo-runtime-core-guice/src/main/java/hu/blackbelt/judo/runtime/core/guice/JudoDefaultModuleConfiguration.java
- judo-runtime-core/src/test/java/hu/blackbelt/judo/runtime/core/util/JudoProcessHandlerTest.java
- judo-runtime-core-guice/src/main/java/hu/blackbelt/judo/runtime/core/guice/JudoModelLoader.java
- judo-runtime-core-guice-dependencies/pom.xml
- judo-runtime-core/src/main/java/hu/blackbelt/judo/runtime/core/utils/SystemPropertiesVariableResolver.java
- judo-runtime-core/src/test/java/hu/blackbelt/judo/runtime/core/util/RuntimeVariableResolverTest.java
- judo-runtime-core-security-keycloak-cxf/pom.xml
- judo-runtime-core-guice-postgresql/src/main/java/hu/blackbelt/judo/runtime/core/guice/dao/rdbms/postgresql/JudoPostgresqlModule.java
- judo-runtime-core-guice-jetty/src/main/java/hu/blackbelt/judo/runtime/core/jetty/guice/JudoJettyModule.java
- judo-runtime-core/src/main/java/hu/blackbelt/judo/runtime/core/utils/VariableResolver.java
🧰 Additional context used
🪛 LanguageTool
judo-runtime-core-guice-dependencies/LICENSE.txt
[style] ~47-~47: Consider a shorter alternative to avoid wordiness.
Context: ...iles of the Program solely in each case in order to link to, bind by name, or subclass the ...
(IN_ORDER_TO_PREMIUM)
[style] ~114-~114: Consider replacing this phrase with the adverb “reasonably” to avoid wordiness.
Context: ...d informs Recipients how to obtain it in a reasonable manner on or through a medium customarily us...
(IN_A_X_MANNER)
[style] ~118-~118: Did you mean ‘different from’? ‘Different than’ is often considered colloquial style.
Context: ...the Program under a license different than this Agreement, provided that such lice...
(DIFFERENT_THAN)
[style] ~156-~156: ‘with respect to’ might be wordy. Consider a shorter alternative.
Context: ...are may accept certain responsibilities with respect to end users, business partners and the li...
(EN_WORDINESS_PREMIUM_WITH_RESPECT_TO)
[style] ~171-~171: Consider a shorter alternative to avoid wordiness.
Context: ...ged intellectual property infringement. In order to qualify, an Indemnified Contributor mus...
(IN_ORDER_TO_PREMIUM)
[style] ~240-~240: Consider a shorter alternative to avoid wordiness.
Context: ...istribute copies of this Agreement, but in order to avoid inconsistency the Agreement is co...
(IN_ORDER_TO_PREMIUM)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: ⏳ Build, test and deploy artifacts
🔇 Additional comments (11)
judo-runtime-core/pom.xml (3)
27-36: LGTM! Plugin configuration is correct.The maven-dependency-plugin is properly configured to set properties for dependency paths, which will be used by the maven-surefire-plugin.
38-48: LGTM! Surefire plugin configuration is comprehensive.The configuration properly sets up:
- JaCoCo code coverage integration
- Required module access for testing
- System.exit handling via junit5-system-exit
129-141: Consider managing test dependency versions in dependencyManagement.The test dependencies (system-stubs-core, system-stubs-jupiter, and junit5-system-exit) are currently not managed in any parent pom's dependencyManagement section.
judo-runtime-core-guice-dependencies/LICENSE.txt (1)
1-277: Standard Eclipse Public License v2.0.This is the standard EPL v2.0 license text without modifications.
🧰 Tools
🪛 LanguageTool
[style] ~47-~47: Consider a shorter alternative to avoid wordiness.
Context: ...iles of the Program solely in each case in order to link to, bind by name, or subclass the ...(IN_ORDER_TO_PREMIUM)
[style] ~114-~114: Consider replacing this phrase with the adverb “reasonably” to avoid wordiness.
Context: ...d informs Recipients how to obtain it in a reasonable manner on or through a medium customarily us...(IN_A_X_MANNER)
[style] ~118-~118: Did you mean ‘different from’? ‘Different than’ is often considered colloquial style.
Context: ...the Program under a license different than this Agreement, provided that such lice...(DIFFERENT_THAN)
[style] ~156-~156: ‘with respect to’ might be wordy. Consider a shorter alternative.
Context: ...are may accept certain responsibilities with respect to end users, business partners and the li...(EN_WORDINESS_PREMIUM_WITH_RESPECT_TO)
[style] ~171-~171: Consider a shorter alternative to avoid wordiness.
Context: ...ged intellectual property infringement. In order to qualify, an Indemnified Contributor mus...(IN_ORDER_TO_PREMIUM)
[style] ~240-~240: Consider a shorter alternative to avoid wordiness.
Context: ...istribute copies of this Agreement, but in order to avoid inconsistency the Agreement is co...(IN_ORDER_TO_PREMIUM)
pom.xml (2)
397-414: LGTM! Test dependencies are properly managed.The test dependencies are correctly defined in dependencyManagement with explicit versions and scopes.
563-563: LGTM! Module addition aligns with PR objectives.The new module judo-runtime-core-guice-dependencies is properly integrated into the build.
judo-runtime-core/src/main/java/hu/blackbelt/judo/runtime/core/utils/RuntimeVariableResolver.java (3)
63-69: Add null check forprefixingetEffectiveVariableNameTo prevent potential
NullPointerException, ensure thatprefixis not null before using it in string operations.
96-100: Confirm handling ofdefaultValueingetVariableAsFileThe method now correctly handles
defaultValuepotentially beingnull. This addresses previous concerns about a possibleNullPointerExceptionwhendefaultValue.getPath()is called.
35-39:⚠️ Potential issueEnsure
prefixis not null to prevent potentialNullPointerExceptionIn the constructor,
this.prefixis assigned directly from the parameterprefixwithout a null check. Ifprefixisnull, it could lead to aNullPointerExceptionin thegetEffectiveVariableNamemethod at line 65 when callingvariableName.startsWith(prefix).Apply this diff to initialize
prefixto an empty string if it'snull:public RuntimeVariableResolver(String prefix, List<String> variablePrecedence, Collection<VariableResolver> variableResolvers) { - this.prefix = prefix; + this.prefix = prefix != null ? prefix : ""; this.variablePrecedence = variablePrecedence;Likely invalid or redundant comment.
judo-runtime-core/src/main/java/hu/blackbelt/judo/runtime/core/utils/JudoProcessHandler.java (2)
115-128: Handle potential race conditions when writing PID fileThere could be a race condition between
cleanupAndValidatePid()andwritePid(). Another instance might create the PID file after the check but before writing. Consider adding file locking to prevent this scenario.Since file locking mechanisms can vary by filesystem, ensure that the PID file writing is atomic or adequately guarded.
24-48:⚠️ Potential issueAvoid potential
NullPointerExceptionby ensuring non-null valuesIn the constructor,
runtimeVariableResolverandshutdownHookare assigned directly from the parameters. If these parameters arenull, it could lead toNullPointerExceptionin other methods.Apply this diff to use default values when parameters are
null:public JudoProcessHandler(RuntimeVariableResolver runtimeVariableResolver, File judoPidFile, Runnable shutdownHook) { if (runtimeVariableResolver != null) { this.runtimeVariableResolver = runtimeVariableResolver; } else { this.runtimeVariableResolver = RuntimeVariableResolver.builder() .prefix("judo") .variableResolvers(Arrays.asList(new SystemPropertiesVariableResolver(), new EnvironmentVariableResolver())) .build(); } if (shutdownHook != null) { this.shutdownHook = shutdownHook; } else { - this.shutdownHook = () -> {}; + this.shutdownHook = () -> { + // Default shutdown hook logic if needed + }; } this.judoPidFile = judoPidFile; }Likely invalid or redundant comment.
| bind(String.class).annotatedWith(KeycloakConfigurationQualifiers.KeycloakAdminUser.class) | ||
| .toInstance(configuration.getRuntimeVariableResolver() | ||
| .getVariableAsString("keycloakAdminUser", | ||
| configuration.getKeycloakAdminUser())); | ||
|
|
||
| bind(String.class).annotatedWith(KeycloakConfigurationQualifiers.KeycloakAdminPassword.class) | ||
| .toInstance(configuration.getRuntimeVariableResolver() | ||
| .getVariableAsString("keycloakAdminPassword", | ||
| configuration.getKeycloakAdminPassword())); | ||
|
|
||
| bind(String.class).annotatedWith(KeycloakConfigurationQualifiers.KeycloakClientSecret.class) | ||
| .toInstance(configuration.getRuntimeVariableResolver() | ||
| .getVariableAsString("keycloakClientSecret", | ||
| configuration.getKeycloakClientSecret())); |
There was a problem hiding this comment.
Enhance security for sensitive configurations.
The admin credentials and client secrets are being handled without special security considerations. Consider:
- Marking these configurations as sensitive
- Adding additional validation
- Using secure string handling
+ private static final Set<String> SENSITIVE_CONFIGS = Set.of(
+ "keycloakAdminPassword",
+ "keycloakClientSecret"
+ );
+ private void bindSecureConfig(String key, String defaultValue) {
+ try {
+ String value = configuration.getRuntimeVariableResolver()
+ .getVariableAsString(key, defaultValue);
+ if (value == null || value.trim().isEmpty()) {
+ throw new SecurityException("Required secure configuration missing: " + key);
+ }
+ bind(String.class)
+ .annotatedWith(getQualifierForKey(key))
+ .toInstance(value);
+ log.debug("Secure configuration {} bound successfully", key);
+ } catch (Exception e) {
+ log.error("Failed to bind secure configuration {}", key);
+ throw new SecurityException("Security configuration failed", e);
+ }
+ }Committable suggestion skipped: line range outside the PR's diff.
| bind(String.class).annotatedWith(JudoConfigurationQualifiers.IdentifierSignerSecret.class) | ||
| .toInstance(configuration.getRuntimeVariableResolver().getVariableAsString("identifierSignerSecret", | ||
| Objects.requireNonNullElseGet(configuration.getIdentifierSignerSecret(), () -> generateNewSecret()))); |
There was a problem hiding this comment.
Enhance security of identifier signer secret.
The secret generation and handling could be improved:
- Consider using a more secure key derivation
- Add validation for minimum secret length
- Implement secure storage of the secret
+ private static final int MIN_SECRET_LENGTH = 32;
+ private String validateAndGetSecret(String secret) {
+ if (secret != null && secret.length() >= MIN_SECRET_LENGTH) {
+ return secret;
+ }
+ log.warn("Provided secret does not meet security requirements. Generating new secret.");
+ return generateNewSecret();
+ }
bind(String.class).annotatedWith(JudoConfigurationQualifiers.IdentifierSignerSecret.class)
.toInstance(configuration.getRuntimeVariableResolver()
.getVariableAsString("identifierSignerSecret",
- Objects.requireNonNullElseGet(configuration.getIdentifierSignerSecret(),
- () -> generateNewSecret())));
+ validateAndGetSecret(configuration.getIdentifierSignerSecret())));Committable suggestion skipped: line range outside the PR's diff.
