Release 2024.3 #2940
Release 2024.3 #2940
Conversation
WalkthroughThe pull request introduces multiple modifications across various files, primarily focusing on the Changes
Possibly related PRs
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: 1
🧹 Outside diff range and nitpick comments (5)
plugin/core/src/main/java/com/perl5/lang/perl/psi/stubs/PerlPolyNamedElementType.java (1)
122-126: LGTM: Good type safety improvementThe explicit type casting and proper warning suppressions improve code clarity while maintaining type safety. However, consider using the same approach as the previous segment for consistency.
Consider this minor adjustment for consistency with the previous segment:
- stub.getLightNamedElementsStubs().forEach(childStub -> { - @SuppressWarnings("rawtypes") var typedStub = (StubElement)childStub; - //noinspection unchecked - typedStub.getStubType().indexStub(typedStub, sink); - }); + //noinspection rawtypes + for (StubElement childStub : stub.getLightNamedElementsStubs()) { + //noinspection unchecked + childStub.getStubType().indexStub(childStub, sink); + }This maintains the same functionality while being more consistent with the style used in the serialization method above.
plugin/core/src/main/java/com/perl5/lang/perl/idea/project/PerlNamesCache.java (1)
179-182: Document threading requirements in method Javadoc.The method has specific threading requirements that should be documented for maintainability.
Add Javadoc to clarify the threading requirements:
+ /** + * Forces an immediate update of the names cache. + * Must be called from a non-dispatch thread to prevent UI freezes. + * @throws IllegalStateException if called from the dispatch thread + */ public void forceCacheUpdate() { var application = ApplicationManager.getApplication(); application.assertIsNonDispatchThread(); doUpdateCache(); }build.gradle.kts (1)
348-350: Consider the implications of dynamic version cachingCaching dynamic versions for 7 days will improve build performance by reducing network requests, but it might delay getting the latest dependency updates. This is generally a good trade-off, but ensure your CI/CD pipeline has a mechanism to clear caches when needed.
Consider adding a CI job that periodically clears dependency caches to ensure timely updates of critical dependencies.
plugin/core/src/main/java/com/perl5/errorHandler/YoutrackErrorHandler.java (1)
78-78: Consider improving token security managementWhile updating the YouTrack access token is necessary, storing authentication tokens directly in source code poses several security risks:
- Token exposure through version control history
- Difficulty in token rotation
- Potential misuse if the repository becomes public
Consider these security improvements:
- Move the token to a secure configuration management system
- Use environment variables or encrypted configuration files
- Implement token rotation mechanisms
- Add token expiration monitoring
Would you like me to propose a more secure implementation approach?
plugin/src/testFixtures/java/base/PerlLightTestCaseBase.java (1)
472-482: Well-structured thread-safe cache update implementationThe new method properly handles cache updates in both EDT and non-EDT contexts:
- Uses
invokeAndWaitto ensure thread safety- Implements proper timeout handling with
waitWithEventsDispatching- Falls back to direct update in non-EDT context
Consider adding a timeout parameter to make the wait time configurable for different test scenarios.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (14)
build.gradle.kts(6 hunks)gradle.properties(1 hunks)mojo/src/test/java/completion/MojoCompletionTest.java(1 hunks)mojo/src/test/java/completion/MojoPerlCompletionTest.java(1 hunks)plugin/build.gradle.kts(1 hunks)plugin/core/src/main/java/com/perl5/errorHandler/YoutrackErrorHandler.java(1 hunks)plugin/core/src/main/java/com/perl5/lang/perl/idea/project/PerlNamesCache.java(1 hunks)plugin/core/src/main/java/com/perl5/lang/perl/psi/impl/PerlPolyNamedElement.java(0 hunks)plugin/core/src/main/java/com/perl5/lang/perl/psi/stubs/PerlPolyNamedElementType.java(2 hunks)plugin/parts/pluginChanges.html(1 hunks)plugin/src/test/java/completion/PerlCompletionTestCase.java(1 hunks)plugin/src/test/java/completion/PerlPMCompletionTest.java(1 hunks)plugin/src/test/java/completion/PodCompletionEmbeddedTest.java(1 hunks)plugin/src/testFixtures/java/base/PerlLightTestCaseBase.java(4 hunks)
💤 Files with no reviewable changes (1)
- plugin/core/src/main/java/com/perl5/lang/perl/psi/impl/PerlPolyNamedElement.java
✅ Files skipped from review due to trivial changes (2)
- gradle.properties
- plugin/parts/pluginChanges.html
🔇 Additional comments (19)
plugin/src/test/java/completion/PerlPMCompletionTest.java (1)
29-29: LGTM! Good improvement to test reliability.
The change from forceCacheUpdate() to updateNamesCacheSynchronously() ensures that the names cache is properly updated before test execution, preventing potential race conditions.
Let's verify that this change has been consistently applied across all test classes:
✅ Verification successful
All test classes consistently use the new synchronous cache update method
The verification shows that:
- The new
updateNamesCacheSynchronously()method is properly defined inPerlLightTestCaseBaseand used consistently across all test classes - The only remaining usages of
forceCacheUpdate()are in the base implementation itself (PerlLightTestCaseBaseandPerlNamesCache), which is expected as it's the underlying implementation
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify consistent cache update method usage across test classes
# Expected: All test classes should use updateNamesCacheSynchronously() instead of forceCacheUpdate()
echo "Checking for any remaining usage of forceCacheUpdate()..."
rg "forceCacheUpdate\(\)" --type java
echo "Verifying consistent implementation across test classes..."
rg "updateNamesCacheSynchronously\(\)" --type java
Length of output: 1641
mojo/src/test/java/completion/MojoCompletionTest.java (1)
29-29: LGTM! Verify the new cache update method.
The change aligns with the standardization of cache updates across test classes, potentially improving thread safety and test reliability.
Let's verify the new method exists and its implementation:
✅ Verification successful
Method implementation and usage verified successfully
The new updateNamesCacheSynchronously() method is properly implemented in PerlLightTestCaseBase with thread-safe cache updates:
- Ensures indexes are up-to-date using
invokeAndWait - Handles both dispatch and non-dispatch thread scenarios
- Uses pooled thread execution with timeout for dispatch thread case
- Used consistently across multiple completion test classes
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the existence and implementation of updateNamesCacheSynchronously
# Expected: Find the method definition in PerlLightTestCaseBase and its usage in other test classes
# Check for the method definition
echo "Searching for method definition:"
ast-grep --pattern 'protected void updateNamesCacheSynchronously() {
$$$
}'
# Check for similar changes in other test classes
echo -e "\nChecking usage in other test classes:"
rg -l "updateNamesCacheSynchronously\(\)" --type java
Length of output: 2009
plugin/src/test/java/completion/PerlCompletionTestCase.java (1)
30-30: LGTM! Good improvement to cache management.
The change from direct cache force update to using updateNamesCacheSynchronously() is a good improvement that aligns with better thread safety practices and maintains consistency across test classes.
Let's verify this change is consistently applied across other test classes:
✅ Verification successful
Let me analyze the results:
-
forceCacheUpdate()appears in:PerlNamesCache.java(core implementation)PerlLightTestCaseBase.java(internal implementation ofupdateNamesCacheSynchronously)
-
updateNamesCacheSynchronously()is consistently used in:- All test classes (
PodCompletionEmbeddedTest,PerlPMCompletionTest,PerlCompletionTestCase,MojoPerlCompletionTest,MojoCompletionTest) - Defined in
PerlLightTestCaseBase
- All test classes (
Let me verify the implementation in the base class to ensure it's properly synchronized:
The cache update refactoring is correctly and consistently implemented
The verification confirms that:
updateNamesCacheSynchronously()is properly implemented inPerlLightTestCaseBasewith thread-safe handling:- Uses
invokeAndWaitfor index updates - Handles both dispatch and non-dispatch thread scenarios
- Includes timeout protection with event dispatching
- Uses
- All test classes consistently use this new synchronized method
- The only remaining
forceCacheUpdate()calls are internal to the implementation
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify consistent cache update approach across test classes
# Expected: All test classes should use updateNamesCacheSynchronously() instead of forceCacheUpdate()
echo "Checking for any remaining uses of forceCacheUpdate():"
rg "forceCacheUpdate\(\)" --type java
echo -e "\nVerifying consistent usage of updateNamesCacheSynchronously():"
rg "updateNamesCacheSynchronously\(\)" --type java
echo -e "\nChecking test classes extending PerlLightTestCase:"
ast-grep --pattern 'class $_ extends PerlLightTestCase {
$$$
}'
Length of output: 1834
Script:
#!/bin/bash
# Check the implementation of updateNamesCacheSynchronously
rg -A 10 "protected final void updateNamesCacheSynchronously" plugin/src/testFixtures/java/base/PerlLightTestCaseBase.java
Length of output: 694
plugin/src/test/java/completion/PodCompletionEmbeddedTest.java (1)
28-28: LGTM! Thread-safe cache update implementation.
The change to use updateNamesCacheSynchronously() aligns with the broader refactoring across test classes and improves thread safety in test setup.
mojo/src/test/java/completion/MojoPerlCompletionTest.java (1)
32-32: LGTM! Consistent with the codebase-wide cache management improvements.
The change aligns with similar updates in other test classes, replacing the direct cache update with the new synchronized approach.
Let's verify the implementation of the new method:
✅ Verification successful
Method is properly implemented in the base class and consistently used across test classes
The verification confirms:
updateNamesCacheSynchronously()is implemented inPerlLightTestCaseBasewith proper thread safety handling- The method is consistently used across multiple test classes in both plugin and mojo modules
- The implementation properly handles both dispatch thread and non-dispatch thread scenarios
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the implementation of updateNamesCacheSynchronously method
# Expected: Find the method definition in PerlLightTestCaseBase or related base classes
# Search for the method definition
ast-grep --pattern 'protected void updateNamesCacheSynchronously() {
$$$
}'
# Search for any references to this method to confirm consistent usage
rg -l 'updateNamesCacheSynchronously'
Length of output: 1825
plugin/build.gradle.kts (1)
69-71: Consider migrating to JUnit 5
While adding JUnit 4.13.2 as a test fixture dependency is valid, JUnit 4 is in maintenance mode. Consider migrating to JUnit 5 (Jupiter) for new test implementations to leverage modern testing features and ensure long-term support.
Let's verify the current JUnit usage in the codebase:
plugin/core/src/main/java/com/perl5/lang/perl/psi/stubs/PerlPolyNamedElementType.java (1)
91-92: LGTM: Appropriate use of raw type suppression
The suppression annotation is correctly scoped to the specific loop where raw type usage is unavoidable due to the heterogeneous nature of child stubs.
plugin/core/src/main/java/com/perl5/lang/perl/idea/project/PerlNamesCache.java (1)
180-180: Verify the thread assertion change impact on test classes.
The change from LOG.assertTrue(!application.isDispatchThread() || application.isUnitTestMode()); to application.assertIsNonDispatchThread(); enforces stricter threading requirements. This is a good practice as it prevents blocking the UI thread, but we should verify that all test classes have been properly updated.
✅ Verification successful
Let me gather more information about how these test classes interact with the cache update mechanism.
Thread assertion change is properly handled in test classes
The verification shows that the codebase has been properly updated to handle the stricter thread assertion:
- All test classes use
updateNamesCacheSynchronously()fromPerlLightTestCaseBase, which properly handles thread safety by:- Running
forceCacheUpdateon a pooled thread when called from dispatch thread - Directly calling
forceCacheUpdatewhen already on a non-dispatch thread
- Running
The only direct cache manipulation outside this pattern is in PerlFormatterSpacingTest which uses cleanCache(), but this is a different method not affected by the thread assertion change.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Find test classes that might be affected by the thread assertion change
# Search for test classes that use PerlNamesCache
rg -l "PerlNamesCache" --type java | rg "Test\.java$" | while read -r file; do
echo "=== $file ==="
# Show the context around PerlNamesCache usage
rg -B 2 -A 2 "PerlNamesCache" "$file"
done
# Search for classes that might still use forceCacheUpdate on dispatch thread
ast-grep --pattern 'class $_ {
$$$
@Test
$$$
forceCacheUpdate()
$$$
}'
Length of output: 2529
Script:
#!/bin/bash
# Search for updateNamesCacheSynchronously usage in test files
echo "=== Searching for updateNamesCacheSynchronously usage ==="
rg -B 2 -A 2 "updateNamesCacheSynchronously" --type java
# Search for cleanCache usage in test files
echo -e "\n=== Searching for cleanCache usage ==="
rg -B 2 -A 2 "cleanCache" --type java
# Search for direct forceCacheUpdate calls
echo -e "\n=== Searching for forceCacheUpdate calls ==="
rg -B 2 -A 2 "forceCacheUpdate" --type java
Length of output: 6793
build.gradle.kts (8)
69-69: LGTM! Improved plugin project identification
The use of the in operator is more idiomatic in Kotlin and provides better readability while maintaining the same functionality.
83-83: Verify the inclusion of nightly repository in release
Adding the nightly repository could potentially introduce instability. Since this PR is for Release 2024.3, please confirm if including the nightly repository is intentional for this release.
Consider adding a conditional flag to enable/disable the nightly repository based on the build type (release vs. development).
92-93: LGTM! Improved version string readability
The multi-line formatting improves readability of the version string construction.
108-108: LGTM! Added stable JUnit dependency
Adding JUnit 4.13.2 as a test dependency is appropriate and uses a stable version.
119-121: LGTM! Consistent Kotlin compilation target
The Kotlin compilation target is properly aligned with the Java target version, ensuring consistency across the codebase.
178-182: LGTM! Improved test logging configuration
The test logging events configuration has been reformatted for better readability while maintaining comprehensive test output in CI environments.
Line range hint 190-211: LGTM! Improved project configuration structure
The separation of root and plugin project configurations is clear and well-organized, with appropriate task enabling/disabling logic.
97-105: Verify platformToolsVersion property configuration
The conditional logic depends on platformToolsVersion property. Please ensure this property is properly defined in gradle.properties and its empty state is handled correctly.
Run the following script to check the property definition:
✅ Verification successful
The property exists but is empty. Let's check its usage across the codebase to understand if this is the intended behavior and how it's handled.
Empty platformToolsVersion property is handled correctly
The empty value in gradle.properties is intentionally handled by the conditional logic in build.gradle.kts. When empty, it falls back to using instrumentationTools() and the platform test framework without version specification. When set, it uses the specified version for both Java compiler and test framework. This is a valid configuration pattern.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if platformToolsVersion is defined in gradle.properties
rg "platformToolsVersion" -A 1 "gradle.properties"
Length of output: 89
Script:
#!/bin/bash
# Let's check how platformToolsVersion is used across the codebase
rg "platformToolsVersion" -g '*.{kt,kts,gradle}' -B 2 -A 2
Length of output: 680
plugin/src/testFixtures/java/base/PerlLightTestCaseBase.java (3)
233-233: LGTM: Import added for Future
The import is required for handling asynchronous operations in the new cache update functionality.
423-423: Consistent usage of synchronous cache updates
The cache update calls have been standardized to use the new thread-safe method.
Also applies to: 469-469
863-863: LGTM: Consistent cache update in resolve test
The resolve test now uses the synchronized cache update method for consistency.
| @@ -75,7 +75,7 @@ public class YoutrackErrorHandler extends ErrorReportSubmitter { | |||
| public static final String YOUTRACK_PROPERTY_KEY = "youtrack.token"; | |||
| public static final String YOUTRACK_PROPERTY_VALUE = System.getProperty(YOUTRACK_PROPERTY_KEY); | |||
| private static final String ADMIN_TOKEN = "Bearer " + YOUTRACK_PROPERTY_VALUE; | |||
| private static final String ACCESS_TOKEN = "Bearer perm:YXV0b3JlcG9ydGVy.NjEtMjE=.7ylXO3QulwaVCA2ZEGLIdc0wPAJQ13"; | |||
| private static final String ACCESS_TOKEN = "Bearer perm:YXV0b3JlcG9ydGVy.NjEtMjI=.8vBQNSaZTUCojZZYUn4uayG1BKDfTk"; | |||
There was a problem hiding this comment.
💡 Codebase verification
Token usage appears secure but requires system property configuration
The token is properly scoped and securely used:
- Only used for HTTP Authorization headers in
createIssueand file attachment operations - Follows proper Bearer token format
- Has a system property override mechanism similar to
ADMIN_TOKEN
However, the code should prioritize using the system property configuration (youtrack.token) instead of having a hardcoded token. Consider:
- Moving the hardcoded token to a fallback position
- Adding documentation about configuring the
youtrack.tokensystem property - Adding a warning log when falling back to the hardcoded token
🔗 Analysis chain
Verify token usage and permissions scope
Let's ensure the new token maintains the same permission scope as the previous one and is only used for intended operations.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify token usage across the codebase
# Check if the token is used in other files
rg -l "perm:YXV0b3JlcG9ydGVy.NjEtMjI=" --type java
# Check for any other Bearer token usage patterns
rg -l "Bearer perm:" --type java
# Look for any direct token usage without Bearer prefix
rg -l "YXV0b3JlcG9ydGVy" --type java
Length of output: 347
Script:
#!/bin/bash
# Let's examine how this token is used within YoutrackErrorHandler.java
rg -A 5 -B 5 "perm:YXV0b3JlcG9ydGVy" plugin/core/src/main/java/com/perl5/errorHandler/YoutrackErrorHandler.java
# Check for any HTTP client usage or API calls
ast-grep --pattern 'new HttpClient($$$)'
# Look for methods that might use this token
ast-grep --pattern $'class YoutrackErrorHandler {
$$$
createIssue($$$) {
$$$
}
$$$
}'
ast-grep --pattern $'class YoutrackErrorHandler {
$$$
attachFiles($$$) {
$$$
}
$$$
}'
Length of output: 950
Script:
#!/bin/bash
# Let's check how the token is actually used in HTTP requests
rg -A 10 "ACCESS_TOKEN" plugin/core/src/main/java/com/perl5/errorHandler/YoutrackErrorHandler.java
# Check if there's a system property override similar to ADMIN_TOKEN
rg -A 2 "System.getProperty" plugin/core/src/main/java/com/perl5/errorHandler/YoutrackErrorHandler.java
# Look for any HTTP headers being set
rg -A 3 "setRequestProperty|addHeader" plugin/core/src/main/java/com/perl5/errorHandler/YoutrackErrorHandler.java
Length of output: 2087
Qodana for JVM62 new problems were found
💡 Qodana analysis was run in the pull request mode: only the changed files were checked Detected 11 dependenciesThird-party software listThis page lists the third-party software dependencies used in perl5
Contact Qodana teamContact us at [email protected]
|
|
Gradle unable to solve the version of ant compiler tasks and test framework
243 does not have it in IDE distribution
Not sure how this did work previously. But we requested read action in smart mode from edt, and if we were in the dumb mode, this should have locked, because we are blocking edt and it is necessary to leave the dumb mode.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
build.gradle.kts (2)
97-105: LGTM! Consider adding documentation.The conditional logic for platform tools dependency is well-structured. However, it would be helpful to add a comment explaining when each path should be used.
Add a comment like this above the condition:
+ // Use instrumentationTools for standard builds and javaCompiler for custom platform tools version val platformToolsVersion = properties("platformToolsVersion")
108-108: Consider migrating to JUnit 5.While JUnit 4.13.2 is stable, consider planning a migration to JUnit 5 (Jupiter) for access to newer testing features and continued support.
Example migration path:
- testImplementation("junit:junit:4.13.2") + testImplementation("org.junit.jupiter:junit-jupiter-api:5.10.1") + testRuntimeOnly("org.junit.jupiter:junit-jupiter-engine:5.10.1") + testImplementation("org.junit.jupiter:junit-jupiter-params:5.10.1")
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (14)
build.gradle.kts(3 hunks)gradle.properties(1 hunks)mojo/src/test/java/completion/MojoCompletionTest.java(1 hunks)mojo/src/test/java/completion/MojoPerlCompletionTest.java(1 hunks)plugin/build.gradle.kts(1 hunks)plugin/core/src/main/java/com/perl5/errorHandler/YoutrackErrorHandler.java(1 hunks)plugin/core/src/main/java/com/perl5/lang/perl/idea/project/PerlNamesCache.java(1 hunks)plugin/core/src/main/java/com/perl5/lang/perl/psi/impl/PerlPolyNamedElement.java(0 hunks)plugin/core/src/main/java/com/perl5/lang/perl/psi/stubs/PerlPolyNamedElementType.java(2 hunks)plugin/parts/pluginChanges.html(1 hunks)plugin/src/test/java/completion/PerlCompletionTestCase.java(1 hunks)plugin/src/test/java/completion/PerlPMCompletionTest.java(1 hunks)plugin/src/test/java/completion/PodCompletionEmbeddedTest.java(1 hunks)plugin/src/testFixtures/java/base/PerlLightTestCaseBase.java(4 hunks)
💤 Files with no reviewable changes (1)
- plugin/core/src/main/java/com/perl5/lang/perl/psi/impl/PerlPolyNamedElement.java
🚧 Files skipped from review as they are similar to previous changes (11)
- gradle.properties
- mojo/src/test/java/completion/MojoCompletionTest.java
- mojo/src/test/java/completion/MojoPerlCompletionTest.java
- plugin/build.gradle.kts
- plugin/core/src/main/java/com/perl5/errorHandler/YoutrackErrorHandler.java
- plugin/core/src/main/java/com/perl5/lang/perl/idea/project/PerlNamesCache.java
- plugin/core/src/main/java/com/perl5/lang/perl/psi/stubs/PerlPolyNamedElementType.java
- plugin/parts/pluginChanges.html
- plugin/src/test/java/completion/PerlCompletionTestCase.java
- plugin/src/test/java/completion/PerlPMCompletionTest.java
- plugin/src/test/java/completion/PodCompletionEmbeddedTest.java
🔇 Additional comments (4)
build.gradle.kts (2)
83-83: Verify the need for nightly repository.
Adding the nightly repository could introduce instability. Ensure this is intentionally added for testing against the latest IntelliJ platform builds.
Consider adding a conditional flag to enable/disable nightly repositories based on the build environment to maintain stability in production builds.
348-350: LGTM! Good performance optimization.
Setting a 7-day cache for dynamic versions is a reasonable optimization that will help reduce unnecessary network requests during builds.
plugin/src/testFixtures/java/base/PerlLightTestCaseBase.java (2)
472-482: LGTM: Well-implemented thread-safe cache update method.
The new updateNamesCacheSynchronously() method provides a robust and thread-safe way to update the names cache, properly handling both EDT and non-EDT execution contexts.
423-423: LGTM: Consistent cache update calls.
The cache update calls have been standardized to use the new synchronous method, improving thread safety and maintainability.
Also applies to: 469-469
Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Refactor