SAK-43201 - Better Acceptance Tests #14310
Conversation
This is not working, because the kernel has a bad dependency on impls, by way of the MessagingService.
The only places that need to know about "Shared" are those creating the context to begin with.
We need to add an initializer to be able to register the context once it's created. We don't really have any other customization to do, but the API doesn't quite let us catch it directly... so we have to approximate the DelegatingSmartContextLoader. We lose Groovy support, but we retain XML and Annotations, with a trivial implementation. This still doesn't quite work, though, because the placeholder properties are lost by switching to a GenericApplicationContext, which is unfortunately required by the loaders with the real implementation. But, registering those two beans up front should take care of it.
This is a test that the kernel boots once per file/suite and that you can get reliable user journey testing if you want. We can add some reset capability, or more likely: do unrelated stuff, clean up, or use different files to isolate.
It turns out, things DO call the CM during refresh. Just set it first.
Getting the property overrides and placeholders loading in the normal startup phase was the last piece to using a standard Spring context. At this point, the SharedApplicationContext is unused, and probably not necessary. I'm leaving it in place for the moment, because it extends AbstractRefreshableApplicationContext, which could theoretically open the chance for reloadability. However, if we can fix up SakaiProperties and use regular beans and environment inheritance/resolution, we could load the kernel in a generic context and let each component build its own child context. That would require an entrypoint that delivers the parent context and initiates component load, but that seems like the logical progression. There doesn't seem to be much of a use case for kernel reloading if components could be redeployed, especially if there is parallel or lazy loading of tool webapps. The BDD/TDD cycle for tools can now be dramatically faster and easier. Reloadable components (and probably some proxy/reload trigger for the tools) would shorten the cycle time for interactive testing and changes.
The default SmartContextLoader behavior examines the configuration and annotations on your test class. It selects one of four loaders, based on whether or not you have @WebAppConfiguration and whether you have classes or locations defined in the @ContextConfiguration. These are the regular/web variations of annotation and XML loaders, respectively. We want to preserve this behavior and not reimplement the details of creating the context, but we have to plug in at exactly the right time: we need to register context as global after it created, but before refresh happens. Because loadContext is a template method, and it delegates to the appropriate subclass for the load-refresh, we have to extend all four variations to implement prepareContext, which is an extension point for exactly this purpose -- preparing the context to be refreshed. The reason we need this timing is that the singletons are "pre-instantiated" during refresh. Basically, any bean that gets an id is a singleton, and all of our traditional services fall in this category. Some of these call the ComponentManager cover during their init method, so we need to have the global context registered already. When it is, a ComponentManager.get translates to a context.getBean, which is no problem; the container carries on creating any beans that are requested, transitively. All put together, this means that a test can inherit from the regular test or the web test base class, depending on whether it needs a web application context (and MockMvc / WebClient) facilities, and it will have a full kernel and context set up, with all services injectable.
This adds some jars to the test helper and upgrades reactor-core (because WebTestClient uses Sinks, which is new in 3.4). With the web test working (though not passing or testing anything, it's time to move the test helpers/base classes to the kernel.
This isn't necessarily the right strategy, but it allows the portal to continue to function the same and let us instantiate the ToolPortalServlet in a test context. Combined with the RequestFilter, which is already in kernel-api, we can drive very realistic requests to a tool easily and see what else we need.
- Stop scanning announcements packages from the kernel - Add a component scan to announcement-impl for the bullhorn handler (and making way for anything else) - Set up context hierarchy so test components can be loaded alongside the kernel or in a child context - Add a portal level to the hierarchy that scans for the ToolPortalServlet, making it available to inject easily - Expand the test helper to include edu-services and portal-util - Build out the WebContextTest into a real proof of concept, making requests through the request filter, tool portal, and announcements
Because a different kind of Spring application context is now mentioned in the ComponentManager cover, PowerMock has to deal with that. In these few cases, it could not during tests, because spring-context wasn't loaded. It's always provided in shared, anyway, so we just add it. The other issue was that portal-tool now needs to know about the @nAmed annotation, added so the ToolPortalServlet can be detected automatically as a component/bean. A simple addition of javax.inject fixes that.
The recently added kernel-test and test-helper modules will probably merge eventually, but for now we put all of the test support code in kernel-test and pack it up in the helper jar like everything else. The former gets a few new provided dependencies identified for compile. This also adds test-helper to the kernel pom, so it is now within the normal compile tree/cycle. For now, anyway, the classes all have a Modi prefix. That's probably unnecessary now that they're in a reasonable package. This also turns down the log noise. If you want to turn logs up, put a sakai.properties file in target/sakai-home of the module you are testing with log.config entries.
- json-simple - javax-mail - ehcache-core We add json-simple because HtmlPageFilter gained this dependency (which is widely used outside of the kernel) in support of configuring MathJax. These should probably be converted across the board to Jackson or an alternative implementation like jolokia-json. We add com.sun.mail:jakarta-mail to support changes in the Message Service implementation. Whether we stay with that jar or switch to something like Angus, the references should all be updated to jakarta. The ehcache-core dependency is dubious, with both Ignite and Hazelcast in place.
The tests aren't to what I consider a fully running state because they cannot start and stop cleanly when running a group of test classes. Getting the startup and shutdown isolation working properly will be quite a bit of work. - Tidy up the test hierarchy slightly - Add the LTI API for test scope - Stub out the LTIService since all it does for announcement is adjust links embedded in messages
This large commit adjusts a number of problems with dependencies and initialization. They are mostly related to either caching references or sequencing and a bit of circular dependency, especially around Hibernate, Ignite, the ServerConfigurationService, and the MemoryService. Now, it's possible to run multiple tests that each get their own kernel and clean application context. The main problem around Ignite/Hibernate: There has been a long-standing dependency cycle between the caching provider and Hibernate. The issue is that Ignite needs startup properties, which our setup step was reading from ServerConfigurationService, which can read properties from the database, so it needs Hibernate, which needs Ignite for its caching. The IgniteSpringBean works out of the box to start the grid at the right time, provided you do not have a dependency cycle. The Fix: - Add a non-dynamic ConfiguredContext to stand in for these fundamental properties that cannot be sourced dynamically (at least not through the global Hibernate). - Inject this into the IgniteConfiguration instead of SCS. - Eliminate the caching of the IgniteConfiguration that was in place because of reentrant setup -- AbstractFactoryBean already manages this and will create one configuration per context. - Adjust the lifecycle interaction so that our EntityManagerFactory is injected with the configured Ignite grid instance, rather than the configuration; ensure that it is started in time for Hibernate, but only once. - Adjust the SakaiPersistenceUnitManager and AssignableUUIDGenerator so they also only depend on this ConfiguredContext at startup. - Allow for multiple Ignite instances to be started by appending an index to the name -- this is primarily to avoid any timing issues with test runners. The problem and fix around Ehcache: - The CacheManager class is rather strange at our version. The clearAll and removalAll methods do not behave consistently and throw exceptions on what would be normal shutdown, so we simply call shutdown. General reference caching: - The GlobalApplicationContext now ensures that the ComponentManager cover does not hold a stale reference to a closed context. - The SiteService uses BaseDbFlatStorage, which was calling the caching MemoryServiceLocator. MemoryAction was also using this locator. - It's unlikely that outside code was using this locator, but it is now marked deprecated for removal in 27. It also has its caching removed, so it will reliably retrieve from the correct context. - The ResourceLoader was caching components statically, meaning that almost any tool would throw NPEs on a reloaded kernel. - This ResourceLoader caching is completely removed, but could be added back at the instance level if we find performance impact. Miscellaneous items: - This is the first time that the modi SysProp is used outside the package, so it needed some methods marked public. - Add a few NumberUtil methods to better handle Optionals. - Adjust the mocking in one assignment test that relies on advising the Hibernate ID generation (switch it to use ConfiguredContext)
WalkthroughThis pull request introduces a configuration abstraction layer (ConfiguredContext) to replace ServerConfigurationService, refactors the component manager to use Spring's ConfigurableApplicationContext instead of SharedApplicationContext, establishes a new Modi test framework, and updates memory service usage to leverage ComponentManager dependency injection. Changes
Suggested reviewers
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
Note
Due to the large number of review comments, Critical, Major severity comments were prioritized as inline comments.
🟡 Minor comments (13)
announcement/announcement-tool/tool/src/test/org/sakaiproject/announcement/tool/AnncTestBeanConfig.java-10-12 (1)
10-12: Missing@Configurationannotation.For Spring to properly recognize this as a configuration class and process the
@ComponentScan, you typically need to add@Configuration. Without it, the component scanning may not be activated when this class is used as a configuration source.🔎 Proposed fix
+import org.springframework.context.annotation.Configuration; + /** * This is here only to set up component scanning... */ +@Configuration @ComponentScan(basePackages = "org.sakaiproject.announcement.tool") public class AnncTestBeanConfig { }kernel/kernel-test/src/main/java/org/sakaiproject/test/ModiXmlWebLoader.java-5-5 (1)
5-5: Remove unused import.The import
org.springframework.context.support.GenericApplicationContextis not used in this class.🔎 Proposed fix
import org.sakaiproject.modi.GlobalApplicationContext; import org.springframework.context.ConfigurableApplicationContext; -import org.springframework.context.support.GenericApplicationContext; import org.springframework.test.context.MergedContextConfiguration; import org.springframework.test.context.web.GenericXmlWebContextLoader; import org.springframework.web.context.WebApplicationContext;kernel/kernel-util/src/main/java/org/sakaiproject/util/NumberUtil.java-153-159 (1)
153-159: Handle null input to avoidNullPointerException.
Integer.valueOf(value)throwsNullPointerExceptionwhenvalueis null, which isn't caught by theNumberFormatExceptionhandler. Consider returningOptional.empty()for null input to match the expected behavior of a safe parsing method.🔎 Proposed fix
public static Optional<Integer> toInteger(final String value) { + if (value == null) { + return Optional.empty(); + } try { return Optional.of(Integer.valueOf(value)); } catch (final NumberFormatException nfe) { return Optional.empty(); } }kernel/kernel-util/src/main/java/org/sakaiproject/util/NumberUtil.java-201-207 (1)
201-207: Handle null input fortoLongdefault-value variant.Same null-safety issue as the other methods.
🔎 Proposed fix
public static Long toLong(final String value, final Long defaultValue) { + if (value == null) { + return defaultValue; + } try { return Long.valueOf(value); } catch (final NumberFormatException e) { return defaultValue; } }kernel/kernel-util/src/main/java/org/sakaiproject/util/NumberUtil.java-185-191 (1)
185-191: Handle null input fortoLongOptional variant.Same null-safety issue as
toInteger.🔎 Proposed fix
public static Optional<Long> toLong(final String value) { + if (value == null) { + return Optional.empty(); + } try { return Optional.of(Long.valueOf(value)); } catch (final NumberFormatException nfe) { return Optional.empty(); } }kernel/kernel-util/src/main/java/org/sakaiproject/util/NumberUtil.java-169-175 (1)
169-175: Handle null input for the default-value variant.Similar to the
Optionalvariant, null input causesNullPointerException. ReturndefaultValuewhen input is null.🔎 Proposed fix
public static Integer toInteger(final String value, final Integer defaultValue) { + if (value == null) { + return defaultValue; + } try { return Integer.valueOf(value); } catch (final NumberFormatException nfe) { return defaultValue; } }kernel/kernel-impl/src/main/webapp/WEB-INF/components.xml-13-14 (1)
13-14: Removeorg.sakaiproject.messaging.api.testhandlerfrom production component scan.The
testhandlerpackage is located insrc/testand contains test-only components (e.g.,TestUserNotificationHandler). These should not be scanned in the productioncomponents.xml. Either move this configuration to a test-specific Spring context or remove the package from the scan list.Note: The removal of
org.sakaiproject.announcement.apiis not problematic—AnnouncementsUserNotificationHandleris properly discovered through the announcement module's own component-scan configuration inannouncement/announcement-impl/impl/src/webapp/WEB-INF/components.xml.kernel/kernel-test/src/main/java/org/sakaiproject/test/ModiWebTest.java-12-16 (1)
12-16: Includeinitializers = ModiInitializer.classin the kernel context configuration.Unlike
locations(which subclasses are designed to provide), theinitializersattribute should be carried forward. The parent classModiTestspecifiesinitializers = ModiInitializer.class, which sets critical system properties (sakai.homeandsakai.modi.enabled). SinceModiWebTestuses@ContextHierarchyto replace the parent's configuration rather than extend it, these initializers are lost. Addinitializers = ModiInitializer.classto the kernel context inModiWebTestto match the parent's behavior.kernel/component-manager/src/main/java/org/sakaiproject/modi/ModiSakaiProperties.java-55-64 (1)
55-64: Fix Javadoc @link syntax error.The
@linkreference on line 57 is missing the closing brace, which will cause Javadoc generation warnings.🔎 Proposed fix
/** * The base class yields its override and placeholder processors for registration. We simply delegate to them in - * {@link #postProcessBeanFactory(ConfigurableListableBeanFactory). + * {@link #postProcessBeanFactory(ConfigurableListableBeanFactory)}. * * @return an empty list, always */kernel/api/src/main/java/org/sakaiproject/hibernate/AssignableUUIDGenerator.java-47-54 (1)
47-54: Replacevarwith explicit type declaration.Line 48 uses local variable type inference (
var), which violates the project's coding guidelines. As per the guidelines, Java code should always declare explicit types.🔎 Proposed fix
@Override public Serializable generate(SharedSessionContractImplementor session, Object object) throws HibernateException { if (configuredContext != null) { - var classes = configuredContext.getStrings(HIBERNATE_ASSIGNABLE_ID_CLASSES); + Collection<String> classes = configuredContext.getStrings(HIBERNATE_ASSIGNABLE_ID_CLASSES); String entityName = object.getClass().getName(); if (classes.contains(entityName)) { final Serializable id = session.getEntityPersister(entityName, object).getIdentifier(object, session); if (id != null) return id; } } return super.generate(session, object); }You'll also need to add the import:
import java.util.Collection;kernel/kernel-impl/src/main/java/org/sakaiproject/ignite/IgniteConfigurationAdapter.java-286-297 (1)
286-297: Replacevarwith explicit types per coding guidelines.Lines 287-288 use local variable type inference (
var), which violates the project's coding guidelines. The guidelines explicitly state: "Never use local variable type inference (var). Always declare explicit types."🔎 Proposed fix
private void configureHome() { - var base = Path.of(home); - var igniteHome = StringUtils.isBlank(node) ? base : base.resolve(node); + Path base = Path.of(home); + Path igniteHome = StringUtils.isBlank(node) ? base : base.resolve(node); try { Files.createDirectories(igniteHome); if (!Files.isWritable(igniteHome)) throw cannotWriteToIgniteDirectory(igniteHome); home = igniteHome.toAbsolutePath().toString(); } catch (IOException e) { throw cannotCreateIgniteDirectory(igniteHome); } }As per coding guidelines,
varusage is non-compliant and should be replaced with explicit types before merge.kernel/kernel-impl/src/main/java/org/sakaiproject/component/impl/ConfiguredContextImpl.java-101-107 (1)
101-107: Replacevarwith explicit types per coding guidelines.Lines 102 and 105 use local variable type inference (
var), which violates the project's coding guidelines.🔎 Proposed fix
Optional<List<String>> enumeratedStrings(String name) { - var count = getInt(name + ".count").filter(n -> n > 0); + Optional<Integer> count = getInt(name + ".count").filter(n -> n > 0); if (count.isEmpty()) return Optional.empty(); - var keys = IntStream.rangeClosed(1, count.get()).mapToObj(i -> name + "." + i); + Stream<String> keys = IntStream.rangeClosed(1, count.get()).mapToObj(i -> name + "." + i); return Optional.of(keys.map(key -> getString(key, "")).toList()); }Note: You'll need to add
import java.util.stream.Stream;As per coding guidelines,
varusage is non-compliant and should be replaced with explicit types before merge.Committable suggestion skipped: line range outside the PR's diff.
kernel/test-helper/pom.xml-112-122 (1)
112-122: Hardcoded versions diverging from parent BOM; missing scope on reactor-core.Both
reactive-streamsandreactor-corehave hardcoded versions that differ from the parent BOM (master/pom.xml). Additionally,reactor-coreis missing an explicit<scope>declaration. Removing these versions will inherit the managed versions from the parent (reactive-streams 1.0.4 and reactor-core 3.5.19 with scope provided).<dependency> <groupId>org.reactivestreams</groupId> <artifactId>reactive-streams</artifactId> - <version>1.0.3</version> <scope>compile</scope> </dependency> <dependency> <groupId>io.projectreactor</groupId> <artifactId>reactor-core</artifactId> - <version>3.4.18</version> + <scope>compile</scope> </dependency>
🧹 Nitpick comments (19)
kernel/kernel-impl/src/main/java/org/sakaiproject/memory/impl/EhcacheMemoryService.java (1)
92-96: Consider adding a null-guard before shutdown.If
destroy()is called beforeinit()completes (or if called twice),cacheManagercould be null, causing an NPE. The previous implementation likely had defensive checks.🔎 Proposed fix
public void destroy() { + if (cacheManager == null) { + log.warn("CacheManager already null during destroy"); + return; + } cacheManager.shutdown(); cacheManager = null; // release log.info("SHUTDOWN"); }kernel/component-manager/src/main/java/org/sakaiproject/modi/Launcher.java (2)
70-71: Update outdated Javadoc reference.The Javadoc mentions
SharedApplicationContextbut the implementation now usesGenericApplicationContext. Consider updating for consistency.🔎 Proposed fix
- * The {@link SharedApplicationContext} is both refreshed (loaded) and started (beans created/wired). We make no + * The {@link GenericApplicationContext} is both refreshed (loaded) and started (beans created/wired). We make no
98-105: Consider null-guard for context instop().If
stop()is called beforestart()completes (e.g., during startup failure),contextcould be null, causing an NPE. A null check similar to the one forcomponentswould improve robustness.🔎 Proposed fix
public void stop() { log.info("Stopping Sakai components"); if (components != null) { components.stopping(); } - context.stop(); - context.close(); + if (context != null) { + context.stop(); + context.close(); + } }portal/portal-tool/tool/src/java/org/sakaiproject/portal/tool/ToolPortal.java (1)
31-31: Consider qualifying the static import for clarity.The static import of
getmakes call sites likeget(ActiveToolManager.class)less immediately readable. While this works, fully qualifying asComponentManager.get(...)would make the source clearer.That said, given this is draft PR with transitional wiring and the description notes removing static covers as remaining work, this is acceptable for now.
portal/portal-util/util/src/java/org/sakaiproject/portal/util/ToolPortalServlet.java (1)
150-160: Consider enhancing error handling for permission exceptions.When a
PermissionExceptionoccurs during site visit, the method returnsfalse(likely resulting in a 404), but the TODO comment suggests a login redirect might be more appropriate for user experience. This would guide unauthorized users to authenticate rather than showing a not-found error.announcement/announcement-tool/tool/src/test/org/sakaiproject/announcement/tool/MockFactory.java (1)
7-9: Make the factory method public for clarity.Since this method is invoked via Spring's factory-method mechanism (as seen in mocks.xml), it should be explicitly declared
publicto clearly express its intended usage, even though package-private access works when the Spring context is in the same package.🔎 Proposed change
- public LTIService ltiService() { + public LTIService ltiService() {Wait, it's already public. Let me re-check the annotated code...
Actually, looking at the code again:
public class MockFactory { public LTIService ltiService() {The method IS already public. My mistake. Let me approve this instead.
announcement/announcement-tool/tool/pom.xml (1)
79-84: Consider using dependency management for assertj-core.The assertj-core dependency has a hardcoded version (3.23.1) while other test dependencies rely on the parent POM's dependency management. For consistency and easier maintenance, consider managing this version centrally in the parent POM's
<dependencyManagement>section.🔎 Proposed refactor
<dependency> <groupId>org.assertj</groupId> <artifactId>assertj-core</artifactId> - <version>3.23.1</version> <scope>test</scope> </dependency>Then add to the parent POM's dependencyManagement section:
<dependency> <groupId>org.assertj</groupId> <artifactId>assertj-core</artifactId> <version>3.23.1</version> </dependency>kernel/kernel-test/src/main/java/org/sakaiproject/test/ModiInitializer.java (1)
12-12: Use proper path separator handling.The manual string concatenation
+ "/"for adding a trailing slash is platform-dependent and fragile. Consider usingFile.separatoror Path's built-in methods for better cross-platform compatibility.🔎 Proposed fix using Path resolution
- System.setProperty("sakai.home", Path.of("target/sakai-home").toAbsolutePath().toString() + "/"); + System.setProperty("sakai.home", Path.of("target", "sakai-home", "").toAbsolutePath().toString());Or alternatively, ensure trailing separator explicitly:
- System.setProperty("sakai.home", Path.of("target/sakai-home").toAbsolutePath().toString() + "/"); + System.setProperty("sakai.home", Path.of("target/sakai-home").toAbsolutePath().toString() + File.separator);admin-tools/src/java/org/sakaiproject/memory/tool/MemoryAction.java (1)
49-131: LGTM! Improved dependency management.The refactoring from
MemoryServiceLocator.getInstance()to injectedmemoryServicefield improves testability and aligns with the PR's goal of reducing problematic static/cached resource usage. All four call sites have been updated correctly.As a future enhancement, consider replacing
ComponentManager.get()calls in the constructor with proper Spring constructor injection (though this would require broader refactoring of the Velocity tool infrastructure).kernel/kernel-test/src/main/java/org/sakaiproject/test/ModiContextLoader.java (2)
14-21: Minor naming inconsistency for clarity.The field
annotationLoadercould be renamed toannotationConfigLoaderfor consistency with the method namegetAnnotationConfigLoader()and the naming in the parent class.🔎 Proposed fix
public class ModiContextLoader extends DelegatingSmartContextLoader { private final SmartContextLoader xmlLoader; - private final SmartContextLoader annotationLoader; + private final SmartContextLoader annotationConfigLoader; public ModiContextLoader() { xmlLoader = new ModiXmlLoader(); - annotationLoader = new ModiAnnotationLoader(); + annotationConfigLoader = new ModiAnnotationLoader(); }
35-38: Update the return expression to match the renamed field.If you apply the naming consistency fix above, update this method accordingly.
🔎 Proposed fix (if field is renamed)
@Override protected SmartContextLoader getAnnotationConfigLoader() { - return annotationLoader; + return annotationConfigLoader; }kernel/component-manager/src/main/bundle/org/sakaiproject/config/modi-configuration.xml (1)
2-5: Add util schema location for XML validation.The
utilnamespace is declared but its schema location is missing. This may cause XML validation warnings in IDEs and build tools.🔎 Proposed fix
<beans xmlns="http://www.springframework.org/schema/beans" xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance" xmlns:util="http://www.springframework.org/schema/util" - xsi:schemaLocation="http://www.springframework.org/schema/beans http://www.springframework.org/schema/beans/spring-beans.xsd"> + xsi:schemaLocation="http://www.springframework.org/schema/beans http://www.springframework.org/schema/beans/spring-beans.xsd + http://www.springframework.org/schema/util http://www.springframework.org/schema/util/spring-util.xsd">Alternatively, if the
utilnamespace is not actually used in this file, consider removing the declaration from line 4.kernel/kernel-test/src/main/java/org/sakaiproject/test/ModiWebContextLoader.java (1)
11-18: Same naming inconsistency as ModiContextLoader.For consistency with the method name
getAnnotationConfigLoader(), consider renamingannotationLoadertoannotationConfigLoader.🔎 Proposed fix
public class ModiWebContextLoader extends WebDelegatingSmartContextLoader { private final SmartContextLoader xmlLoader; - private final SmartContextLoader annotationLoader; + private final SmartContextLoader annotationConfigLoader; public ModiWebContextLoader() { xmlLoader = new ModiXmlWebLoader(); - annotationLoader = new ModiAnnotationWebLoader(); + annotationConfigLoader = new ModiAnnotationWebLoader(); }announcement/announcement-tool/tool/src/test/org/sakaiproject/announcement/tool/KernelIntegrityTest.java (1)
40-99: Consider extracting the repeated SecurityAdvisor pattern.The same anonymous
SecurityAdvisorimplementation is duplicated in all four test methods. This could be extracted to a field or helper method for better maintainability.I notice that tests
_3_and_4_are intentionally interdependent (demonstrating that state persists across tests). If this is meant to illustrate the problem with shared state (as the test name suggests), consider adding a class-level comment documenting this intent for future maintainers.🔎 Proposed refactor to reduce duplication
public class KernelIntegrityTest extends ModiTest { + private final SecurityAdvisor allowAllAdvisor = (userId, function, reference) -> SecurityAdvice.ALLOWED; + @Inject public SiteService siteService; @Inject public SecurityService securityService; @Inject public ApplicationContext context; @Test public void _1_givenAModiKernel_thenWeCanRunRealIntegrationTests() throws IdUnusedException { - SecurityAdvisor advisor = new SecurityAdvisor() { - public SecurityAdvice isAllowed(String userId, String function, String reference) { - return SecurityAdvice.ALLOWED; - } - }; - - securityService.pushAdvisor(advisor); + securityService.pushAdvisor(allowAllAdvisor); String siteTitle = siteService.getSite("!admin").getTitle(); - securityService.popAdvisor(advisor); + securityService.popAdvisor(allowAllAdvisor); assertThat(siteTitle).isEqualTo("Administration Workspace"); }assignment/impl/src/test/org/sakaiproject/assignment/impl/AssignmentConversionTest.java (1)
32-32: Wildcard import detected.Using
java.util.*is generally discouraged in favor of explicit imports for better readability and to avoid potential naming conflicts. Consider expanding to explicit imports.announcement/announcement-tool/tool/src/test/org/sakaiproject/announcement/tool/WebContextSetupTest.java (1)
87-103: Consider extracting session setup to reduce duplication.The session setup pattern (start session, set user, set current session, create cookie) is repeated identically in all three test methods. Consider extracting this to a helper method or a
@Beforesetup to improve maintainability.🔎 Example helper method
private Cookie setupAdminSession() { Session session = sessionManager.startSession(); session.setUserEid("admin"); session.setUserId("admin"); sessionManager.setCurrentSession(session); String cookieValue = session.getId() + "." + System.getProperty(RequestFilter.SAKAI_SERVERID, "sakai"); return new Cookie("JSESSIONID", cookieValue); }kernel/test-helper/pom.xml (3)
30-35: Inconsistent version specification across Sakai dependencies.
sakai-kernel-implandscoring-service-apiexplicitly specify${project.version}while other Sakai dependencies inherit versions from the parent BOM. For consistency, consider removing the explicit version declarations if the parent BOM already manages these versions.🔎 Suggested fix
<dependency> <groupId>org.sakaiproject.kernel</groupId> <artifactId>sakai-kernel-impl</artifactId> - <version>${project.version}</version> <scope>compile</scope> </dependency><dependency> <groupId>org.sakaiproject.edu-services.scoringservice</groupId> <artifactId>scoring-service-api</artifactId> - <version>${project.version}</version> <scope>compile</scope> </dependency>Also applies to: 56-61
181-185: Consider upgrade path for deprecated ehcache-core.
net.sf.ehcache:ehcache-coreis in maintenance mode and superseded byorg.ehcache:ehcache(3.x). If this dependency is required for Hibernate 2nd-level cache compatibility with the existing Sakai stack, this is acceptable. Otherwise, consider migrating to Ehcache 3.x or the Ignite-based caching that appears to be used elsewhere.
134-143: Remove deprecatedcommons-langdependency.
commons-lang(Apache Commons Lang 2.x) is deprecated and superseded bycommons-lang3. The deprecated package is not used anywhere in the kernel codebase, making it safe to remove.🔎 Suggested fix
- <dependency> - <groupId>commons-lang</groupId> - <artifactId>commons-lang</artifactId> - <scope>compile</scope> - </dependency> <dependency> <groupId>org.apache.commons</groupId> <artifactId>commons-lang3</artifactId> <scope>compile</scope> </dependency>
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (62)
admin-tools/src/java/org/sakaiproject/memory/tool/MemoryAction.javaannouncement/announcement-impl/impl/pom.xmlannouncement/announcement-impl/impl/src/webapp/WEB-INF/components.xmlannouncement/announcement-tool/tool/pom.xmlannouncement/announcement-tool/tool/src/test/org/sakaiproject/announcement/tool/AnncTestBeanConfig.javaannouncement/announcement-tool/tool/src/test/org/sakaiproject/announcement/tool/ContextSetupTest.javaannouncement/announcement-tool/tool/src/test/org/sakaiproject/announcement/tool/KernelIntegrityTest.javaannouncement/announcement-tool/tool/src/test/org/sakaiproject/announcement/tool/MockFactory.javaannouncement/announcement-tool/tool/src/test/org/sakaiproject/announcement/tool/WebContextSetupTest.javaannouncement/announcement-tool/tool/src/test/resources/mocks.xmlassignment/impl/src/test/org/sakaiproject/assignment/impl/AssignmentConversionTest.javaassignment/impl/src/test/org/sakaiproject/assignment/impl/AssignmentTestConfiguration.javakernel/api/src/main/java/org/sakaiproject/hibernate/AssignableUUIDGenerator.javakernel/api/src/main/java/org/sakaiproject/memory/cover/MemoryServiceLocator.javakernel/api/src/main/java/org/sakaiproject/springframework/orm/hibernate/SakaiPersistenceUnitManager.javakernel/component-manager/src/main/bundle/org/sakaiproject/config/modi-configuration.xmlkernel/component-manager/src/main/java/org/sakaiproject/component/api/ConfiguredContext.javakernel/component-manager/src/main/java/org/sakaiproject/component/impl/ComponentManagerShim.javakernel/component-manager/src/main/java/org/sakaiproject/modi/ComponentsDirectory.javakernel/component-manager/src/main/java/org/sakaiproject/modi/Configuration.javakernel/component-manager/src/main/java/org/sakaiproject/modi/GlobalApplicationContext.javakernel/component-manager/src/main/java/org/sakaiproject/modi/Launcher.javakernel/component-manager/src/main/java/org/sakaiproject/modi/ModiSakaiProperties.javakernel/component-manager/src/main/java/org/sakaiproject/modi/SysProp.javakernel/component-manager/src/main/java/org/sakaiproject/util/SakaiProperties.javakernel/component-manager/src/test/java/org/sakaiproject/modi/EnvironmentTest.javakernel/component-manager/src/test/java/org/sakaiproject/modi/LauncherTest.javakernel/component-manager/src/test/java/org/sakaiproject/modi/SakaiStartListenerTest.javakernel/kernel-impl/src/main/java/org/sakaiproject/component/impl/ConfiguredContextImpl.javakernel/kernel-impl/src/main/java/org/sakaiproject/ignite/EagerIgniteSpringBean.javakernel/kernel-impl/src/main/java/org/sakaiproject/ignite/IgniteConfigurationAdapter.javakernel/kernel-impl/src/main/java/org/sakaiproject/ignite/IgniteEntityManagerFactoryBean.javakernel/kernel-impl/src/main/java/org/sakaiproject/memory/impl/EhcacheMemoryService.javakernel/kernel-impl/src/main/webapp/WEB-INF/component-components.xmlkernel/kernel-impl/src/main/webapp/WEB-INF/components.xmlkernel/kernel-impl/src/main/webapp/WEB-INF/db-components.xmlkernel/kernel-impl/src/main/webapp/WEB-INF/ignite-components.xmlkernel/kernel-impl/src/test/java/org/sakaiproject/messaging/impl/RuntimeRegistrationTest.javakernel/kernel-storage-util/src/main/java/org/sakaiproject/util/BaseDbFlatStorage.javakernel/kernel-test/pom.xmlkernel/kernel-test/src/main/java/org/sakaiproject/test/ModiAnnotationLoader.javakernel/kernel-test/src/main/java/org/sakaiproject/test/ModiAnnotationWebLoader.javakernel/kernel-test/src/main/java/org/sakaiproject/test/ModiContextLoader.javakernel/kernel-test/src/main/java/org/sakaiproject/test/ModiInitializer.javakernel/kernel-test/src/main/java/org/sakaiproject/test/ModiPortalConfig.javakernel/kernel-test/src/main/java/org/sakaiproject/test/ModiTest.javakernel/kernel-test/src/main/java/org/sakaiproject/test/ModiWebContextLoader.javakernel/kernel-test/src/main/java/org/sakaiproject/test/ModiWebTest.javakernel/kernel-test/src/main/java/org/sakaiproject/test/ModiXmlLoader.javakernel/kernel-test/src/main/java/org/sakaiproject/test/ModiXmlWebLoader.javakernel/kernel-util/src/main/java/org/sakaiproject/util/NumberUtil.javakernel/kernel-util/src/main/java/org/sakaiproject/util/ResourceLoader.javakernel/pom.xmlkernel/test-helper/pom.xmlkernel/test-helper/src/assembly/helper-jar.xmlkernel/test-helper/src/main/resources/log4j.propertieskernel/test-helper/src/main/resources/org/sakaiproject/config/test-configuration.xmlkernel/test-helper/src/main/resources/org/sakaiproject/config/test.propertiesportal/portal-tool/tool/pom.xmlportal/portal-tool/tool/src/java/org/sakaiproject/portal/tool/ToolPortal.javaportal/portal-util/util/src/java/org/sakaiproject/portal/util/ToolPortalServlet.javasamigo/samigo-qti/pom.xml
🧰 Additional context used
📓 Path-based instructions (5)
**/*.java
📄 CodeRabbit inference engine (.cursor/rules/logging-rule.mdc)
**/*.java: Use SLF4J parameterized logging (logger.info("Value is: {}", value)) instead of string concatenation (logger.info("Value is: " + value))
Log messages and code comments should be in English. Log messages should never be translated.
**/*.java: Java: Never use local variable type inference (var). Always declare explicit types. Yes:Map<String, Integer> counts = new HashMap<>();No:var counts = new HashMap<String, Integer>();
When proposing Java code, spell out full types in local variable declarations,forloops, and try-with-resources
When editing Java, prefer clarity over brevity; avoid introducing language features that aren't widely used in the repo
Treat any PR or suggestion containing Javavaras non-compliant. Recommend replacing with explicit types before merge
**/*.java: Use Java 17 for trunk development (Java 11 was used for Sakai 22 and Sakai 23)
Do not use local variable type inference (var) in Java code. Always declare explicit types (e.g.,List<String> names = new ArrayList<>();notvar names = new ArrayList<String>();). Enforced by Checkstyle rule duringmvn validate
Files:
announcement/announcement-tool/tool/src/test/org/sakaiproject/announcement/tool/MockFactory.javakernel/kernel-test/src/main/java/org/sakaiproject/test/ModiWebTest.javakernel/component-manager/src/main/java/org/sakaiproject/util/SakaiProperties.javakernel/kernel-test/src/main/java/org/sakaiproject/test/ModiInitializer.javakernel/kernel-test/src/main/java/org/sakaiproject/test/ModiWebContextLoader.javakernel/component-manager/src/test/java/org/sakaiproject/modi/EnvironmentTest.javakernel/kernel-test/src/main/java/org/sakaiproject/test/ModiContextLoader.javaannouncement/announcement-tool/tool/src/test/org/sakaiproject/announcement/tool/WebContextSetupTest.javakernel/kernel-util/src/main/java/org/sakaiproject/util/NumberUtil.javaannouncement/announcement-tool/tool/src/test/org/sakaiproject/announcement/tool/AnncTestBeanConfig.javakernel/component-manager/src/main/java/org/sakaiproject/modi/Launcher.javakernel/component-manager/src/main/java/org/sakaiproject/modi/ModiSakaiProperties.javakernel/kernel-test/src/main/java/org/sakaiproject/test/ModiAnnotationLoader.javakernel/kernel-test/src/main/java/org/sakaiproject/test/ModiPortalConfig.javaannouncement/announcement-tool/tool/src/test/org/sakaiproject/announcement/tool/KernelIntegrityTest.javakernel/component-manager/src/main/java/org/sakaiproject/component/api/ConfiguredContext.javakernel/kernel-test/src/main/java/org/sakaiproject/test/ModiXmlWebLoader.javakernel/kernel-impl/src/test/java/org/sakaiproject/messaging/impl/RuntimeRegistrationTest.javakernel/component-manager/src/main/java/org/sakaiproject/modi/ComponentsDirectory.javaadmin-tools/src/java/org/sakaiproject/memory/tool/MemoryAction.javakernel/kernel-storage-util/src/main/java/org/sakaiproject/util/BaseDbFlatStorage.javakernel/api/src/main/java/org/sakaiproject/hibernate/AssignableUUIDGenerator.javakernel/component-manager/src/main/java/org/sakaiproject/component/impl/ComponentManagerShim.javakernel/kernel-test/src/main/java/org/sakaiproject/test/ModiAnnotationWebLoader.javakernel/api/src/main/java/org/sakaiproject/springframework/orm/hibernate/SakaiPersistenceUnitManager.javakernel/kernel-test/src/main/java/org/sakaiproject/test/ModiTest.javakernel/component-manager/src/main/java/org/sakaiproject/modi/SysProp.javaassignment/impl/src/test/org/sakaiproject/assignment/impl/AssignmentConversionTest.javakernel/kernel-impl/src/main/java/org/sakaiproject/memory/impl/EhcacheMemoryService.javakernel/component-manager/src/test/java/org/sakaiproject/modi/LauncherTest.javakernel/kernel-impl/src/main/java/org/sakaiproject/ignite/IgniteConfigurationAdapter.javakernel/component-manager/src/main/java/org/sakaiproject/modi/GlobalApplicationContext.javakernel/component-manager/src/test/java/org/sakaiproject/modi/SakaiStartListenerTest.javaportal/portal-util/util/src/java/org/sakaiproject/portal/util/ToolPortalServlet.javakernel/kernel-impl/src/main/java/org/sakaiproject/ignite/IgniteEntityManagerFactoryBean.javaportal/portal-tool/tool/src/java/org/sakaiproject/portal/tool/ToolPortal.javakernel/kernel-test/src/main/java/org/sakaiproject/test/ModiXmlLoader.javaannouncement/announcement-tool/tool/src/test/org/sakaiproject/announcement/tool/ContextSetupTest.javakernel/component-manager/src/main/java/org/sakaiproject/modi/Configuration.javakernel/kernel-impl/src/main/java/org/sakaiproject/component/impl/ConfiguredContextImpl.javakernel/kernel-util/src/main/java/org/sakaiproject/util/ResourceLoader.javakernel/api/src/main/java/org/sakaiproject/memory/cover/MemoryServiceLocator.javakernel/kernel-impl/src/main/java/org/sakaiproject/ignite/EagerIgniteSpringBean.javaassignment/impl/src/test/org/sakaiproject/assignment/impl/AssignmentTestConfiguration.java
**/*.{html,jsp,vm,xhtml,xml}
📄 CodeRabbit inference engine (.cursor/rules/frontend.mdc)
**/*.{html,jsp,vm,xhtml,xml}: Use Bootstrap 5.2 as the preferred UI framework for styling in Sakai frontends
Leverage Bootstrap 5 components for consistent UI/UX
Files:
kernel/kernel-impl/src/main/webapp/WEB-INF/component-components.xmlkernel/kernel-impl/src/main/webapp/WEB-INF/ignite-components.xmlkernel/pom.xmlportal/portal-tool/tool/pom.xmlkernel/kernel-impl/src/main/webapp/WEB-INF/db-components.xmlannouncement/announcement-tool/tool/src/test/resources/mocks.xmlkernel/kernel-impl/src/main/webapp/WEB-INF/components.xmlsamigo/samigo-qti/pom.xmlannouncement/announcement-impl/impl/src/webapp/WEB-INF/components.xmlkernel/kernel-test/pom.xmlkernel/test-helper/src/main/resources/org/sakaiproject/config/test-configuration.xmlkernel/component-manager/src/main/bundle/org/sakaiproject/config/modi-configuration.xmlannouncement/announcement-impl/impl/pom.xmlkernel/test-helper/src/assembly/helper-jar.xmlannouncement/announcement-tool/tool/pom.xmlkernel/test-helper/pom.xml
**/*.{html,jsp,vm,xhtml,xml,css,scss}
📄 CodeRabbit inference engine (.cursor/rules/frontend.mdc)
Ensure all UI components work across different screen sizes (Responsive Design)
Files:
kernel/kernel-impl/src/main/webapp/WEB-INF/component-components.xmlkernel/kernel-impl/src/main/webapp/WEB-INF/ignite-components.xmlkernel/pom.xmlportal/portal-tool/tool/pom.xmlkernel/kernel-impl/src/main/webapp/WEB-INF/db-components.xmlannouncement/announcement-tool/tool/src/test/resources/mocks.xmlkernel/kernel-impl/src/main/webapp/WEB-INF/components.xmlsamigo/samigo-qti/pom.xmlannouncement/announcement-impl/impl/src/webapp/WEB-INF/components.xmlkernel/kernel-test/pom.xmlkernel/test-helper/src/main/resources/org/sakaiproject/config/test-configuration.xmlkernel/component-manager/src/main/bundle/org/sakaiproject/config/modi-configuration.xmlannouncement/announcement-impl/impl/pom.xmlkernel/test-helper/src/assembly/helper-jar.xmlannouncement/announcement-tool/tool/pom.xmlkernel/test-helper/pom.xml
**/*.{js,html,jsp,vm,xhtml,xml}
📄 CodeRabbit inference engine (.cursor/rules/frontend.mdc)
Use the web components in the webcomponents/ directory when possible in Sakai frontends
Files:
kernel/kernel-impl/src/main/webapp/WEB-INF/component-components.xmlkernel/kernel-impl/src/main/webapp/WEB-INF/ignite-components.xmlkernel/pom.xmlportal/portal-tool/tool/pom.xmlkernel/kernel-impl/src/main/webapp/WEB-INF/db-components.xmlannouncement/announcement-tool/tool/src/test/resources/mocks.xmlkernel/kernel-impl/src/main/webapp/WEB-INF/components.xmlsamigo/samigo-qti/pom.xmlannouncement/announcement-impl/impl/src/webapp/WEB-INF/components.xmlkernel/kernel-test/pom.xmlkernel/test-helper/src/main/resources/org/sakaiproject/config/test-configuration.xmlkernel/component-manager/src/main/bundle/org/sakaiproject/config/modi-configuration.xmlannouncement/announcement-impl/impl/pom.xmlkernel/test-helper/src/assembly/helper-jar.xmlannouncement/announcement-tool/tool/pom.xmlkernel/test-helper/pom.xml
**/*.{html,jsp,jspx,xml,ts,tsx,js,jsx}
📄 CodeRabbit inference engine (AGENTS.md)
Prefer kebab-case for values of HTML class and id attributes
Files:
kernel/kernel-impl/src/main/webapp/WEB-INF/component-components.xmlkernel/kernel-impl/src/main/webapp/WEB-INF/ignite-components.xmlkernel/pom.xmlportal/portal-tool/tool/pom.xmlkernel/kernel-impl/src/main/webapp/WEB-INF/db-components.xmlannouncement/announcement-tool/tool/src/test/resources/mocks.xmlkernel/kernel-impl/src/main/webapp/WEB-INF/components.xmlsamigo/samigo-qti/pom.xmlannouncement/announcement-impl/impl/src/webapp/WEB-INF/components.xmlkernel/kernel-test/pom.xmlkernel/test-helper/src/main/resources/org/sakaiproject/config/test-configuration.xmlkernel/component-manager/src/main/bundle/org/sakaiproject/config/modi-configuration.xmlannouncement/announcement-impl/impl/pom.xmlkernel/test-helper/src/assembly/helper-jar.xmlannouncement/announcement-tool/tool/pom.xmlkernel/test-helper/pom.xml
🧠 Learnings (11)
📚 Learning: 2025-10-10T15:13:08.361Z
Learnt from: ern
Repo: sakaiproject/sakai PR: 14152
File: reset-pass/account-validator-impl/src/test/java/org/sakaiproject/accountvalidator/test/AccountValidationTestConfiguration.java:69-72
Timestamp: 2025-10-10T15:13:08.361Z
Learning: In the Sakai project, test-specific mock configurations should remain in the test class itself as part of the test logic. Only mock configurations that are common across multiple test classes should be centralized in test configuration classes.
Applied to files:
announcement/announcement-tool/tool/src/test/org/sakaiproject/announcement/tool/MockFactory.javakernel/kernel-test/src/main/java/org/sakaiproject/test/ModiWebTest.javaannouncement/announcement-tool/tool/src/test/resources/mocks.xmlkernel/kernel-test/src/main/java/org/sakaiproject/test/ModiPortalConfig.javakernel/kernel-test/src/main/java/org/sakaiproject/test/ModiTest.javakernel/test-helper/src/main/resources/org/sakaiproject/config/test-configuration.xmlkernel/component-manager/src/main/bundle/org/sakaiproject/config/modi-configuration.xmlassignment/impl/src/test/org/sakaiproject/assignment/impl/AssignmentConversionTest.javaannouncement/announcement-tool/tool/src/test/org/sakaiproject/announcement/tool/ContextSetupTest.javaannouncement/announcement-tool/tool/pom.xmlassignment/impl/src/test/org/sakaiproject/assignment/impl/AssignmentTestConfiguration.java
📚 Learning: 2025-11-24T19:11:31.821Z
Learnt from: CR
Repo: sakaiproject/sakai PR: 0
File: .cursor/rules/frontend.mdc:0-0
Timestamp: 2025-11-24T19:11:31.821Z
Learning: Applies to **/*.{js,html,jsp,vm,xhtml,xml} : Use the web components in the webcomponents/ directory when possible in Sakai frontends
Applied to files:
kernel/pom.xmlannouncement/announcement-impl/impl/pom.xml
📚 Learning: 2025-11-24T19:12:20.537Z
Learnt from: CR
Repo: sakaiproject/sakai PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-11-24T19:12:20.537Z
Learning: Applies to **/*.java : Use Spring for dependency injection, MVC, and services in Java development
Applied to files:
portal/portal-tool/tool/pom.xmlkernel/kernel-impl/src/main/webapp/WEB-INF/components.xmlsamigo/samigo-qti/pom.xmlkernel/kernel-test/pom.xmlannouncement/announcement-tool/tool/pom.xml
📚 Learning: 2025-10-06T22:15:40.541Z
Learnt from: ottenhoff
Repo: sakaiproject/sakai PR: 14152
File: reset-pass/account-validator-api/src/main/java/org/sakaiproject/accountvalidator/model/ValidationAccount.java:24-33
Timestamp: 2025-10-06T22:15:40.541Z
Learning: The Sakai project (as of this PR) uses Hibernate 5.6.15.Final and Spring 5.3.39, which requires javax.persistence imports, not jakarta.persistence. The jakarta namespace migration will happen when the project upgrades to JDK 21, Spring 6, and Hibernate 6+.
Applied to files:
kernel/kernel-impl/src/main/webapp/WEB-INF/db-components.xmlsamigo/samigo-qti/pom.xmlkernel/kernel-test/pom.xmlkernel/api/src/main/java/org/sakaiproject/hibernate/AssignableUUIDGenerator.javakernel/api/src/main/java/org/sakaiproject/springframework/orm/hibernate/SakaiPersistenceUnitManager.javaannouncement/announcement-tool/tool/pom.xml
📚 Learning: 2025-12-10T10:41:09.339Z
Learnt from: st-manu
Repo: sakaiproject/sakai PR: 14256
File: kernel/kernel-impl/src/main/sql/mysql/sakai_realm.sql:734-734
Timestamp: 2025-12-10T10:41:09.339Z
Learning: In Sakai, DB upgrade/backfill for new realm functions and grants belongs in the sakai-reference repo under docs/conversion (e.g., sakai_25_1-25_2_*_conversion.sql). For SAK-51713, assessment.all.groups is granted to maintain and Instructor and backfilled across existing realms via conversion scripts.
Applied to files:
kernel/kernel-impl/src/main/webapp/WEB-INF/db-components.xml
📚 Learning: 2025-11-24T19:12:20.537Z
Learnt from: CR
Repo: sakaiproject/sakai PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-11-24T19:12:20.537Z
Learning: Applies to **/*.java : Use Hibernate as the ORM framework for database interactions in Java
Applied to files:
kernel/test-helper/src/main/resources/org/sakaiproject/config/test.properties
📚 Learning: 2025-11-24T19:12:20.537Z
Learnt from: CR
Repo: sakaiproject/sakai PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-11-24T19:12:20.537Z
Learning: Applies to **/*.{java,properties} : Reconcile push notification subscriptions proactively and keep localization strings in `sakai-notifications.properties`
Applied to files:
kernel/kernel-impl/src/main/webapp/WEB-INF/components.xmlannouncement/announcement-impl/impl/pom.xml
📚 Learning: 2025-11-24T19:12:27.663Z
Learnt from: CR
Repo: sakaiproject/sakai PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-11-24T19:12:27.663Z
Learning: Spring is crucial for dependency injection, MVC, and services; use Spring for new development
Applied to files:
samigo/samigo-qti/pom.xmlkernel/kernel-test/pom.xml
📚 Learning: 2025-10-07T16:35:33.991Z
Learnt from: ern
Repo: sakaiproject/sakai PR: 14152
File: reset-pass/account-validator-impl/pom.xml:37-42
Timestamp: 2025-10-07T16:35:33.991Z
Learning: The Sakai project cannot upgrade to Spring Framework 6 yet and must remain on Spring 5.x for the time being.
Applied to files:
samigo/samigo-qti/pom.xml
📚 Learning: 2025-09-25T13:34:06.742Z
Learnt from: susowrc
Repo: sakaiproject/sakai PR: 14105
File: assignment/tool/src/java/org/sakaiproject/assignment/entityproviders/AssignmentEntityProvider.java:1736-1745
Timestamp: 2025-09-25T13:34:06.742Z
Learning: In Sakai, each site can only have one tool placement for a given common tool ID (e.g., "sakai.assignment.grades" for the Assignment tool). The method getToolForCommonId() returns a single ToolConfiguration object, not a collection, confirming this one-to-one relationship between tools and sites.
Applied to files:
portal/portal-util/util/src/java/org/sakaiproject/portal/util/ToolPortalServlet.javaportal/portal-tool/tool/src/java/org/sakaiproject/portal/tool/ToolPortal.java
📚 Learning: 2025-11-24T19:12:27.663Z
Learnt from: CR
Repo: sakaiproject/sakai PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-11-24T19:12:27.663Z
Learning: Build the project using `mvn install`
Applied to files:
announcement/announcement-impl/impl/pom.xml
🧬 Code graph analysis (15)
kernel/kernel-test/src/main/java/org/sakaiproject/test/ModiWebTest.java (1)
kernel/kernel-test/src/main/java/org/sakaiproject/test/ModiTest.java (1)
ContextHierarchy(15-39)
announcement/announcement-tool/tool/src/test/org/sakaiproject/announcement/tool/WebContextSetupTest.java (3)
announcement/announcement-tool/tool/src/test/org/sakaiproject/announcement/tool/ContextSetupTest.java (1)
RunWith(22-53)kernel/kernel-test/src/main/java/org/sakaiproject/test/ModiTest.java (1)
ContextHierarchy(15-39)kernel/kernel-test/src/main/java/org/sakaiproject/test/ModiWebTest.java (1)
WebAppConfiguration(11-17)
kernel/component-manager/src/main/java/org/sakaiproject/modi/Launcher.java (1)
kernel/component-manager/src/main/java/org/sakaiproject/modi/GlobalApplicationContext.java (1)
GlobalApplicationContext(30-82)
kernel/component-manager/src/main/java/org/sakaiproject/modi/ModiSakaiProperties.java (1)
kernel/component-manager/src/main/java/org/sakaiproject/util/SakaiProperties.java (1)
Slf4j(59-409)
kernel/kernel-test/src/main/java/org/sakaiproject/test/ModiAnnotationLoader.java (1)
kernel/component-manager/src/main/java/org/sakaiproject/modi/GlobalApplicationContext.java (1)
GlobalApplicationContext(30-82)
announcement/announcement-tool/tool/src/test/org/sakaiproject/announcement/tool/KernelIntegrityTest.java (1)
announcement/announcement-tool/tool/src/test/org/sakaiproject/announcement/tool/ContextSetupTest.java (1)
RunWith(22-53)
kernel/kernel-test/src/main/java/org/sakaiproject/test/ModiXmlWebLoader.java (1)
kernel/component-manager/src/main/java/org/sakaiproject/modi/GlobalApplicationContext.java (1)
GlobalApplicationContext(30-82)
kernel/kernel-test/src/main/java/org/sakaiproject/test/ModiAnnotationWebLoader.java (1)
kernel/component-manager/src/main/java/org/sakaiproject/modi/GlobalApplicationContext.java (1)
GlobalApplicationContext(30-82)
kernel/api/src/main/java/org/sakaiproject/springframework/orm/hibernate/SakaiPersistenceUnitManager.java (1)
kernel/api/src/main/java/org/sakaiproject/hibernate/AssignableUUIDGenerator.java (1)
AssignableUUIDGenerator(33-57)
assignment/impl/src/test/org/sakaiproject/assignment/impl/AssignmentConversionTest.java (1)
kernel/api/src/main/java/org/sakaiproject/hibernate/AssignableUUIDGenerator.java (1)
AssignableUUIDGenerator(33-57)
kernel/component-manager/src/test/java/org/sakaiproject/modi/SakaiStartListenerTest.java (1)
kernel/component-manager/src/main/java/org/sakaiproject/modi/GlobalApplicationContext.java (1)
GlobalApplicationContext(30-82)
portal/portal-tool/tool/src/java/org/sakaiproject/portal/tool/ToolPortal.java (1)
portal/portal-util/util/src/java/org/sakaiproject/portal/util/ToolPortalServlet.java (1)
Slf4j(22-217)
kernel/kernel-test/src/main/java/org/sakaiproject/test/ModiXmlLoader.java (1)
kernel/component-manager/src/main/java/org/sakaiproject/modi/GlobalApplicationContext.java (1)
GlobalApplicationContext(30-82)
announcement/announcement-tool/tool/src/test/org/sakaiproject/announcement/tool/ContextSetupTest.java (3)
announcement/announcement-tool/tool/src/test/org/sakaiproject/announcement/tool/KernelIntegrityTest.java (1)
RunWith(24-100)announcement/announcement-tool/tool/src/test/org/sakaiproject/announcement/tool/WebContextSetupTest.java (1)
RunWith(45-184)samigo/samigo-app/src/webapp/js/finInputValidator.js (1)
context(30-30)
kernel/kernel-impl/src/main/java/org/sakaiproject/component/impl/ConfiguredContextImpl.java (1)
kernel/kernel-util/src/main/java/org/sakaiproject/util/NumberUtil.java (1)
NumberUtil(36-210)
🪛 GitHub Actions: Windows Build
kernel/component-manager/src/test/java/org/sakaiproject/modi/EnvironmentTest.java
[error] 116-116: IO Critical error -- could not set test directory read-only: C:\Users\RUNNER~1\AppData\Local\Temp\junit12627441752225610901\readonly
[error] 132-132: IO Critical error -- could not set test directory read-only: C:\Users\RUNNER~1\AppData\Local\Temp\junit13493837206627215482\readonly\sakai
🪛 OSV Scanner (2.3.1)
kernel/test-helper/pom.xml
[HIGH] 1-1: org.springframework:spring-core 5.3.39: Spring Framework annotation detection mechanism may result in improper authorization
[CRITICAL] 1-1: org.springframework:spring-web 5.3.39: Spring Framework DataBinder Case Sensitive Match Exception
[CRITICAL] 1-1: org.springframework:spring-web 5.3.39: Pivotal Spring Framework contains unsafe Java deserialization methods
[HIGH] 1-1: org.springframework:spring-webmvc 5.3.39: Path traversal vulnerability in functional web frameworks
[HIGH] 1-1: org.springframework:spring-webmvc 5.3.39: Spring Framework Path Traversal vulnerability
[HIGH] 1-1: org.springframework:spring-webmvc 5.3.39: Spring Framework MVC Applications Path Traversal Vulnerability
[HIGH] 1-1: org.springframework:spring-webmvc 5.3.39: Spring MVC controller vulnerable to a DoS attack
⏰ Context from checks skipped due to timeout of 900000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: sakai-deploy
- GitHub Check: maven-build
| <plugin> | ||
| <groupId>org.codehaus.mojo</groupId> | ||
| <artifactId>build-helper-maven-plugin</artifactId> | ||
| <version>3.0.0</version> | ||
| <executions> | ||
| <execution> | ||
| <phase>package</phase> | ||
| <goals> | ||
| <goal>attach-artifact</goal> | ||
| </goals> | ||
| <configuration> | ||
| <artifacts> | ||
| <artifact> | ||
| <file> | ||
| ${project.build.directory}/${project.build.finalName}/WEB-INF/lib/${project.build.finalName}.jar | ||
| </file> | ||
| <type>jar</type> | ||
| </artifact> | ||
| </artifacts> | ||
| </configuration> | ||
| </execution> | ||
| </executions> | ||
| </plugin> |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, find and read the actual pom.xml file
find . -path "*announcement/announcement-impl/impl/pom.xml" -type f | head -5Repository: sakaiproject/sakai
Length of output: 107
🏁 Script executed:
# List the directory structure to understand context
ls -la announcement/announcement-impl/impl/ 2>/dev/null | head -20Repository: sakaiproject/sakai
Length of output: 282
🏁 Script executed:
# Check if parent pom.xml exists and read relevant sections
fd "pom.xml" | grep -E "(pom.xml$|parent|announcement)" | head -10Repository: sakaiproject/sakai
Length of output: 301
🏁 Script executed:
# Read the pom.xml file with line numbers
cat -n ./announcement/announcement-impl/impl/pom.xml | head -120Repository: sakaiproject/sakai
Length of output: 5391
🏁 Script executed:
# Search for parent POM reference
rg "parent\|pluginManagement" ./announcement/announcement-impl/impl/pom.xml -A 3Repository: sakaiproject/sakai
Length of output: 44
🏁 Script executed:
# Check the parent POM to see if there's pluginManagement
cat -n ./announcement/pom.xml | head -80Repository: sakaiproject/sakai
Length of output: 1888
🏁 Script executed:
# Search for build-helper-maven-plugin in other parts of the codebase
rg "build-helper-maven-plugin" -A 2 | head -40Repository: sakaiproject/sakai
Length of output: 944
🏁 Script executed:
# Check the master pom.xml for pluginManagement section
cat -n ./master/pom.xml | grep -A 5 -B 5 "build-helper-maven-plugin"Repository: sakaiproject/sakai
Length of output: 565
🏁 Script executed:
# Get more context around the pluginManagement in master/pom.xml
cat -n ./master/pom.xml | grep -A 15 "pluginManagement"Repository: sakaiproject/sakai
Length of output: 1478
🏁 Script executed:
# Check how kernel/kernel-impl/pom.xml handles build-helper-maven-plugin
rg -A 10 "build-helper-maven-plugin" kernel/kernel-impl/pom.xmlRepository: sakaiproject/sakai
Length of output: 524
🏁 Script executed:
# Verify current version of build-helper-maven-plugin in the wild
web_search "build-helper-maven-plugin latest version 2024 2025"Repository: sakaiproject/sakai
Length of output: 112
🌐 Web query:
What is the latest version of build-helper-maven-plugin from org.codehaus.mojo?
💡 Result:
Latest version: 3.6.1. [1][2]
Sources:
[1] mojohaus plugin info (build-helper-maven-plugin) — shows version 3.6.1.
[2] Maven Central listing for org.codehaus.mojo:build-helper-maven-plugin:3.6.1.
🏁 Script executed:
# Check if kernel/kernel-impl/pom.xml specifies version for build-helper-maven-plugin
cat -n kernel/kernel-impl/pom.xml | grep -A 15 "build-helper-maven-plugin"Repository: sakaiproject/sakai
Length of output: 955
Remove the hardcoded version and inherit from parent POM's pluginManagement.
The build-helper-maven-plugin version 3.0.0 is hardcoded here, but the master POM already defines version 3.4.0 in its <pluginManagement> section. Remove the <version> tag from this plugin configuration to inherit the centrally managed version, consistent with how kernel/kernel-impl/pom.xml handles the same plugin.
🤖 Prompt for AI Agents
In announcement/announcement-impl/impl/pom.xml around lines 68 to 90, the
build-helper-maven-plugin has a hardcoded <version>3.0.0</version>; remove that
<version> element so the plugin inherits the version defined in the parent POM's
<pluginManagement> (3.4.0). Keep the rest of the plugin configuration intact
(groupId, artifactId, executions, configuration) and ensure
formatting/indentation remains valid XML after removing the version tag.
| public String getRaw() { | ||
| return System.getProperty(key); | ||
| } | ||
|
|
||
| /** Get the unchecked value of this property, with a default value; currently implemented over System properties. */ | ||
| String getRaw(String def) { | ||
| public String getRaw(String def) { | ||
| return System.getProperty(key, def); | ||
| } |
There was a problem hiding this comment.
Document null-returning behavior for public API.
Now that getRaw() is public, the JavaDoc should explicitly document that it returns null when the property is not set. This is inconsistent with the Optional-based accessors above and could lead to NPEs if callers aren't aware.
Consider adding @return JavaDoc: "the property value, or null if not set".
💡 Suggested JavaDoc improvement
- /** Get the unchecked value of this property; currently implemented over System properties. */
+ /**
+ * Get the unchecked value of this property; currently implemented over System properties.
+ * @return the property value, or null if not set
+ */
public String getRaw() {
return System.getProperty(key);
}Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In kernel/component-manager/src/main/java/org/sakaiproject/modi/SysProp.java
around lines 83 to 90, the public getRaw() method does not document that it can
return null when the system property is not set; update its JavaDoc to include
an @return tag reading: "the property value, or null if not set". Also
add/update JavaDoc for getRaw(String def) to include an @return tag like: "the
property value, or the provided default if not set" and, optionally, a short
note directing callers to the Optional-based accessors for null-safe usage.
| public Path getRawPath() { | ||
| return get().map(Path::of).orElse(null); | ||
| } |
There was a problem hiding this comment.
Document or refactor null-returning Path method.
getRawPath() returns null when the property is not set (line 94), which is risky for a public API returning Path. Callers may assume a non-null return and encounter NPEs. This is inconsistent with the safer getPath() method that returns Optional<Path>.
Recommendations:
- Add explicit JavaDoc documenting the null return
- Consider deprecating this in favor of
getPath() - Or refactor to throw an exception instead of returning null
💡 Suggested JavaDoc improvement
- /** Get the unchecked value of this property as a Path; currently implemented over System properties. */
+ /**
+ * Get the unchecked value of this property as a Path; currently implemented over System properties.
+ * @return the property value as a Path, or null if not set
+ */
public Path getRawPath() {
return get().map(Path::of).orElse(null);
}Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In kernel/component-manager/src/main/java/org/sakaiproject/modi/SysProp.java
around lines 93-95, the public getRawPath() returns null when the property is
absent which risks NPEs; mark this method as @Deprecated, add clear JavaDoc
stating it may return null and instruct callers to use getPath() which returns
Optional<Path>, update any internal call sites to prefer getPath() (or handle
null) and add a short migration note in the Javadoc pointing to getPath(); if
you prefer a stricter change instead, replace the body to throw an unchecked
exception (IllegalStateException) rather than returning null and document that
behavior in the Javadoc.
| public Path getRawPathPlus(String other) { | ||
| return getPath().map(p -> p.resolve(other)).orElse(null); | ||
| } |
There was a problem hiding this comment.
Document or refactor null-returning Path method.
Similar to getRawPath(), this method returns null when the property is not set (line 103), which creates NPE risk in public API. The presence of the other parameter may mislead callers into expecting a non-null result.
Recommendations:
- Add explicit JavaDoc documenting the null return
- Consider deprecating this in favor of
getPathPlus(String other) - Or refactor to throw an exception instead of returning null
💡 Suggested JavaDoc improvement
/**
* Get this property as a Path, with a resolved extension; currently implemented over System properties.
* <p>
* The extension is applied if the property is set. This returns the combined path if so, null otherwise.
+ * @param other the path segment to resolve against the property value
+ * @return the resolved path, or null if the property is not set
*/
public Path getRawPathPlus(String other) {
return getPath().map(p -> p.resolve(other)).orElse(null);
}Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In kernel/component-manager/src/main/java/org/sakaiproject/modi/SysProp.java
around lines 102 to 104, the public method getRawPathPlus(String other) returns
null when the property is not set, which risks NPEs and is misleading to
callers; update this API by either (A) adding clear JavaDoc on the method
stating that it may return null when the property is unset (and document callers
must null-check), or (B) mark this method as @Deprecated and create a safer
replacement getPathPlus(String other) that returns Optional<Path>, or (C) change
behavior to throw a well-documented unchecked exception (e.g.,
IllegalStateException) when the property is absent; implement one of these fixes
consistently (JavaDoc + deprecation is preferred), update callers/tests
accordingly, and run project build to ensure no regressions.
| public void givenUnwritableParentOfSakaiHome_whenInitialized_thenInitializationFails() throws IOException { | ||
| tmpDir.newFolder("components"); | ||
| File file = tmpDir.newFolder("readonly"); | ||
| if (file.setReadOnly()) { | ||
| System.setProperty("catalina.base", tmp.toString()); | ||
| System.setProperty("sakai.home", tmp.resolve("readonly/sakai").toString()); | ||
|
|
||
| assertThatExceptionOfType(InitializationException.class) | ||
| .isThrownBy(Environment::initialize) | ||
| .withMessageContaining("could not create sakai.home"); | ||
| if (!file.setReadOnly()) { | ||
| throw new IOException("Critical error -- could not set test directory read-only: " + file.getAbsolutePath()); | ||
| } | ||
|
|
||
| System.setProperty("catalina.base", tmp.toString()); | ||
| System.setProperty("sakai.home", tmp.resolve("readonly/sakai").toString()); | ||
|
|
||
| assertThatExceptionOfType(InitializationException.class) | ||
| .isThrownBy(Environment::initialize) | ||
| .withMessageContaining("could not create sakai.home"); | ||
| } |
There was a problem hiding this comment.
Critical: Test fails on Windows due to unreliable File.setReadOnly() behavior on directories.
The pipeline failures confirm that File.setReadOnly() returns false on Windows when called on directories, causing these tests to throw IOException and fail. File.setReadOnly() has known platform-specific limitations—it doesn't work reliably on directories in Windows.
Consider one of these approaches:
🔎 Solution 1: Skip test on Windows using JUnit Assumptions
@Test
public void givenUnwritableParentOfSakaiHome_whenInitialized_thenInitializationFails() throws IOException {
+ assumeFalse(System.getProperty("os.name").toLowerCase().contains("win"),
+ "setReadOnly() is unreliable on Windows directories");
tmpDir.newFolder("components");
File file = tmpDir.newFolder("readonly");
if (!file.setReadOnly()) {
throw new IOException("Critical error -- could not set test directory read-only: " + file.getAbsolutePath());
}
System.setProperty("catalina.base", tmp.toString());
System.setProperty("sakai.home", tmp.resolve("readonly/sakai").toString());
assertThatExceptionOfType(InitializationException.class)
.isThrownBy(Environment::initialize)
.withMessageContaining("could not create sakai.home");
}Add import: import static org.junit.Assume.assumeFalse;
🔎 Solution 2: Use Java NIO with platform detection
@Test
public void givenUnwritableParentOfSakaiHome_whenInitialized_thenInitializationFails() throws IOException {
tmpDir.newFolder("components");
File file = tmpDir.newFolder("readonly");
- if (!file.setReadOnly()) {
- throw new IOException("Critical error -- could not set test directory read-only: " + file.getAbsolutePath());
- }
+
+ try {
+ Set<PosixFilePermission> perms = PosixFilePermissions.fromString("r-xr-xr-x");
+ Files.setPosixFilePermissions(file.toPath(), perms);
+ } catch (UnsupportedOperationException e) {
+ assumeTrue(file.setReadOnly(), "Cannot make directory read-only on this platform");
+ }
System.setProperty("catalina.base", tmp.toString());
System.setProperty("sakai.home", tmp.resolve("readonly/sakai").toString());
assertThatExceptionOfType(InitializationException.class)
.isThrownBy(Environment::initialize)
.withMessageContaining("could not create sakai.home");
}Add imports: import java.nio.file.*;, import java.nio.file.attribute.*;, import static org.junit.Assume.assumeTrue;
🧰 Tools
🪛 GitHub Actions: Windows Build
[error] 116-116: IO Critical error -- could not set test directory read-only: C:\Users\RUNNER~1\AppData\Local\Temp\junit12627441752225610901\readonly
🤖 Prompt for AI Agents
In
kernel/component-manager/src/test/java/org/sakaiproject/modi/EnvironmentTest.java
around lines 112 to 125, the test relies on File.setReadOnly() which is
unreliable on Windows and causes CI failures; modify the test to skip on Windows
using a JUnit assumption: add import static org.junit.Assume.assumeFalse; detect
Windows (e.g. String os = System.getProperty("os.name").toLowerCase(); boolean
isWin = os.contains("win");) and call assumeFalse("Skip test on Windows where
setReadOnly on directories is unreliable", isWin) before attempting to set the
directory read-only, so the rest of the test can proceed unchanged on supported
platforms.
| @Test | ||
| public void givenUnwritableSakaiHome_whenInitialized_thenInitializationFails() throws IOException { | ||
| tmpDir.newFolder("components"); | ||
| File file = tmpDir.newFolder("readonly/sakai"); | ||
| if (file.setReadOnly()) { | ||
| System.setProperty("catalina.base", tmp.toString()); | ||
| System.setProperty("sakai.home", tmp.resolve("readonly/sakai").toString()); | ||
|
|
||
| assertThatExceptionOfType(InitializationException.class) | ||
| .isThrownBy(Environment::initialize) | ||
| .withMessageContaining("sakai.home is missing or unreadable"); | ||
| if (!file.setReadOnly()) { | ||
| throw new IOException("Critical error -- could not set test directory read-only: " + file.getAbsolutePath()); | ||
| } | ||
|
|
||
| System.setProperty("catalina.base", tmp.toString()); | ||
| System.setProperty("sakai.home", tmp.resolve("readonly/sakai").toString()); | ||
|
|
||
| assertThatExceptionOfType(InitializationException.class) | ||
| .isThrownBy(Environment::initialize) | ||
| .withMessageContaining("sakai.home is missing or unreadable"); | ||
| } |
There was a problem hiding this comment.
Critical: Test fails on Windows due to unreliable File.setReadOnly() behavior on directories.
This test has the same issue as givenUnwritableParentOfSakaiHome_whenInitialized_thenInitializationFails. Apply the same fix (either skip on Windows or use Java NIO with platform detection).
🔎 Solution 1: Skip test on Windows using JUnit Assumptions
@Test
public void givenUnwritableSakaiHome_whenInitialized_thenInitializationFails() throws IOException {
+ assumeFalse(System.getProperty("os.name").toLowerCase().contains("win"),
+ "setReadOnly() is unreliable on Windows directories");
tmpDir.newFolder("components");
File file = tmpDir.newFolder("readonly/sakai");
if (!file.setReadOnly()) {
throw new IOException("Critical error -- could not set test directory read-only: " + file.getAbsolutePath());
}
System.setProperty("catalina.base", tmp.toString());
System.setProperty("sakai.home", tmp.resolve("readonly/sakai").toString());
assertThatExceptionOfType(InitializationException.class)
.isThrownBy(Environment::initialize)
.withMessageContaining("sakai.home is missing or unreadable");
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| @Test | |
| public void givenUnwritableSakaiHome_whenInitialized_thenInitializationFails() throws IOException { | |
| tmpDir.newFolder("components"); | |
| File file = tmpDir.newFolder("readonly/sakai"); | |
| if (file.setReadOnly()) { | |
| System.setProperty("catalina.base", tmp.toString()); | |
| System.setProperty("sakai.home", tmp.resolve("readonly/sakai").toString()); | |
| assertThatExceptionOfType(InitializationException.class) | |
| .isThrownBy(Environment::initialize) | |
| .withMessageContaining("sakai.home is missing or unreadable"); | |
| if (!file.setReadOnly()) { | |
| throw new IOException("Critical error -- could not set test directory read-only: " + file.getAbsolutePath()); | |
| } | |
| System.setProperty("catalina.base", tmp.toString()); | |
| System.setProperty("sakai.home", tmp.resolve("readonly/sakai").toString()); | |
| assertThatExceptionOfType(InitializationException.class) | |
| .isThrownBy(Environment::initialize) | |
| .withMessageContaining("sakai.home is missing or unreadable"); | |
| } | |
| @Test | |
| public void givenUnwritableSakaiHome_whenInitialized_thenInitializationFails() throws IOException { | |
| assumeFalse(System.getProperty("os.name").toLowerCase().contains("win"), | |
| "setReadOnly() is unreliable on Windows directories"); | |
| tmpDir.newFolder("components"); | |
| File file = tmpDir.newFolder("readonly/sakai"); | |
| if (!file.setReadOnly()) { | |
| throw new IOException("Critical error -- could not set test directory read-only: " + file.getAbsolutePath()); | |
| } | |
| System.setProperty("catalina.base", tmp.toString()); | |
| System.setProperty("sakai.home", tmp.resolve("readonly/sakai").toString()); | |
| assertThatExceptionOfType(InitializationException.class) | |
| .isThrownBy(Environment::initialize) | |
| .withMessageContaining("sakai.home is missing or unreadable"); | |
| } |
🧰 Tools
🪛 GitHub Actions: Windows Build
[error] 132-132: IO Critical error -- could not set test directory read-only: C:\Users\RUNNER~1\AppData\Local\Temp\junit13493837206627215482\readonly\sakai
🤖 Prompt for AI Agents
In
kernel/component-manager/src/test/java/org/sakaiproject/modi/EnvironmentTest.java
around lines 127-141, the test attempts to make a directory read-only using
File.setReadOnly(), which is unreliable on Windows and causes intermittent
failures; update the test to match the fix used in the similar test: guard the
read-only logic with a platform check and skip the test on Windows (use JUnit
Assumptions.assumeFalse(System.getProperty("os.name").toLowerCase().contains("win"))),
or alternatively use Java NIO to set POSIX permissions only when the OS supports
POSIX (check Files.getFileAttributeView and apply Files.setPosixFilePermissions
on non-Windows); ensure the test cleanly aborts/skips on Windows instead of
attempting File.setReadOnly().
| private boolean started = false; | ||
|
|
||
| /** | ||
| * Ensure that the Ignite grid instance has been started. | ||
| * <p> | ||
| * IgniteSpringBean conducts an unsafe (once-only) startup of the instance in its normal | ||
| * afterSingletonsInstantiated lifecycle hook, but it does so after some components like | ||
| * the Hibernate entity manager need the grid started. Here, we safely bridge our needed | ||
| * lifecycle with the base bean behavior by checking and conditionally starting. | ||
| * {@link IgniteEntityManagerFactoryBean} | ||
| */ | ||
| public void ensureStarted() { | ||
| if (started) return; | ||
|
|
||
| super.afterSingletonsInstantiated(); | ||
| started = true; | ||
| } |
There was a problem hiding this comment.
Race condition: ensureStarted() is not thread-safe.
The started flag check-and-set pattern is not atomic. Multiple threads could see started == false simultaneously and both call super.afterSingletonsInstantiated(), potentially starting the Ignite grid twice. The Javadoc mentions "safely bridge" but the current implementation doesn't provide thread safety.
🔎 Proposed fix using synchronized
- private boolean started = false;
+ private volatile boolean started = false;
/**
* Ensure that the Ignite grid instance has been started.
* <p>
* IgniteSpringBean conducts an unsafe (once-only) startup of the instance in its normal
* afterSingletonsInstantiated lifecycle hook, but it does so after some components like
* the Hibernate entity manager need the grid started. Here, we safely bridge our needed
* lifecycle with the base bean behavior by checking and conditionally starting.
* {@link IgniteEntityManagerFactoryBean}
*/
- public void ensureStarted() {
+ public synchronized void ensureStarted() {
if (started) return;
super.afterSingletonsInstantiated();
started = true;
}Alternatively, for better performance with the double-checked locking pattern:
🔎 Alternative: Double-checked locking
- private boolean started = false;
+ private volatile boolean started = false;
+ private final Object startLock = new Object();
public void ensureStarted() {
if (started) return;
-
- super.afterSingletonsInstantiated();
- started = true;
+ synchronized (startLock) {
+ if (started) return;
+ super.afterSingletonsInstantiated();
+ started = true;
+ }
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| private boolean started = false; | |
| /** | |
| * Ensure that the Ignite grid instance has been started. | |
| * <p> | |
| * IgniteSpringBean conducts an unsafe (once-only) startup of the instance in its normal | |
| * afterSingletonsInstantiated lifecycle hook, but it does so after some components like | |
| * the Hibernate entity manager need the grid started. Here, we safely bridge our needed | |
| * lifecycle with the base bean behavior by checking and conditionally starting. | |
| * {@link IgniteEntityManagerFactoryBean} | |
| */ | |
| public void ensureStarted() { | |
| if (started) return; | |
| super.afterSingletonsInstantiated(); | |
| started = true; | |
| } | |
| private volatile boolean started = false; | |
| /** | |
| * Ensure that the Ignite grid instance has been started. | |
| * <p> | |
| * IgniteSpringBean conducts an unsafe (once-only) startup of the instance in its normal | |
| * afterSingletonsInstantiated lifecycle hook, but it does so after some components like | |
| * the Hibernate entity manager need the grid started. Here, we safely bridge our needed | |
| * lifecycle with the base bean behavior by checking and conditionally starting. | |
| * {@link IgniteEntityManagerFactoryBean} | |
| */ | |
| public synchronized void ensureStarted() { | |
| if (started) return; | |
| super.afterSingletonsInstantiated(); | |
| started = true; | |
| } |
| private boolean started = false; | |
| /** | |
| * Ensure that the Ignite grid instance has been started. | |
| * <p> | |
| * IgniteSpringBean conducts an unsafe (once-only) startup of the instance in its normal | |
| * afterSingletonsInstantiated lifecycle hook, but it does so after some components like | |
| * the Hibernate entity manager need the grid started. Here, we safely bridge our needed | |
| * lifecycle with the base bean behavior by checking and conditionally starting. | |
| * {@link IgniteEntityManagerFactoryBean} | |
| */ | |
| public void ensureStarted() { | |
| if (started) return; | |
| super.afterSingletonsInstantiated(); | |
| started = true; | |
| } | |
| private volatile boolean started = false; | |
| private final Object startLock = new Object(); | |
| /** | |
| * Ensure that the Ignite grid instance has been started. | |
| * <p> | |
| * IgniteSpringBean conducts an unsafe (once-only) startup of the instance in its normal | |
| * afterSingletonsInstantiated lifecycle hook, but it does so after some components like | |
| * the Hibernate entity manager need the grid started. Here, we safely bridge our needed | |
| * lifecycle with the base bean behavior by checking and conditionally starting. | |
| * {@link IgniteEntityManagerFactoryBean} | |
| */ | |
| public void ensureStarted() { | |
| if (started) return; | |
| synchronized (startLock) { | |
| if (started) return; | |
| super.afterSingletonsInstantiated(); | |
| started = true; | |
| } | |
| } |
🤖 Prompt for AI Agents
In
kernel/kernel-impl/src/main/java/org/sakaiproject/ignite/EagerIgniteSpringBean.java
around lines 22 to 38, the ensureStarted() method performs a non-atomic
check-then-set on the started flag which can lead to multiple threads invoking
super.afterSingletonsInstantiated() concurrently; make this method thread-safe
by using either a synchronized block or double-checked locking: declare started
as volatile, first check started and return if true, then synchronize on this
and re-check started inside the synchronized block before calling
super.afterSingletonsInstantiated() and setting started = true so the base
startup runs exactly once.
| <dependency> | ||
| <groupId>org.springframework</groupId> | ||
| <artifactId>spring-core</artifactId> | ||
| <scope>compile</scope> | ||
| </dependency> | ||
| <dependency> | ||
| <groupId>org.springframework</groupId> | ||
| <artifactId>spring-context</artifactId> | ||
| <scope>compile</scope> | ||
| </dependency> | ||
| <dependency> | ||
| <groupId>org.springframework</groupId> | ||
| <artifactId>spring-context-support</artifactId> | ||
| <scope>compile</scope> | ||
| </dependency> | ||
| <dependency> | ||
| <groupId>org.springframework</groupId> | ||
| <artifactId>spring-orm</artifactId> | ||
| <scope>compile</scope> | ||
| </dependency> | ||
| <dependency> | ||
| <groupId>org.springframework.data</groupId> | ||
| <artifactId>spring-data-jpa</artifactId> | ||
| <scope>compile</scope> | ||
| </dependency> | ||
| <dependency> | ||
| <groupId>org.springframework</groupId> | ||
| <artifactId>spring-test</artifactId> | ||
| <scope>compile</scope> | ||
| </dependency> | ||
| <dependency> | ||
| <groupId>org.springframework</groupId> | ||
| <artifactId>spring-web</artifactId> | ||
| <scope>compile</scope> | ||
| </dependency> | ||
| <dependency> | ||
| <groupId>org.springframework</groupId> | ||
| <artifactId>spring-webmvc</artifactId> | ||
| <scope>compile</scope> | ||
| </dependency> |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's find and read the pom.xml file in question
find . -path "*/kernel/test-helper/pom.xml" -type f | head -1Repository: sakaiproject/sakai
Length of output: 90
🏁 Script executed:
# Read the pom.xml to check the Spring version
cat -n kernel/test-helper/pom.xml | head -100Repository: sakaiproject/sakai
Length of output: 4097
🌐 Web query:
CVE GHSA-4gc7-5j7h-4qph Spring Framework DataBinder
💡 Result:
Summary — CVE-2024-38820 / GHSA-4gc7-5j7h-4qph
- What: A DataBinder "disallowedFields" case-insensitivity issue: the patch for CVE-2022-22968 used String.toLowerCase(), which has locale-dependent exceptions and can let some protected fields bypass DataBinder protection. [1][2]
- Severity/IDs: Moderate; CVE-2024-38820, GitHub GHSA-4gc7-5j7h-4qph. [2][3]
- Affected Spring Framework versions:
- 5.3.0 — 5.3.40
- 6.0.0 — 6.0.24
- 6.1.0 — 6.1.13
- (older unsupported versions may also be affected). [1][2]
- Fixed versions / recommended action:
- Upgrade to at least 5.3.41, 6.0.25, or 6.1.14 (depending on your line). Spring recommends upgrading; no other mitigations are listed. [1][2]
- References:
- Spring security advisory (CVE-2024-38820). [1]
- GitHub Advisory / GHSA-4gc7-5j7h-4qph. [2]
- OSV / NVD summary (CVE-2024-38820). [3]
🌐 Web query:
CVE GHSA-4wrc-f8pq-fpqp Spring Framework unsafe deserialization
💡 Result:
Summary: GHSA-4wrc-f8pq-fpqp (alias CVE-2016-1000027) is a critical insecure-deserialization / potential RCE issue in the Spring Framework: org.springframework:spring-web versions before 6.0.0 may perform unsafe Java deserialization of untrusted data, allowing remote code execution depending on how Spring is used. [1][2][3]
Affected versions:
- spring-web / Spring Framework < 6.0.0 (older 5.x, 4.x, 3.x lines as noted). [1][2]
Mitigation / recommended actions:
- Upgrade to Spring Framework 6.0.0 or later (6.0.0 removes the impacted classes). [1][2]
- If you cannot upgrade immediately:
- Avoid deserializing untrusted data; ensure only trusted sources can provide serialized Java payloads.
- Use allowlists / blocklists for deserialization (or switch to safer serializers like JSON) where applicable.
- Note: Spring 5.3.x deprecated the affected classes; 4.2.6 and 3.2.17 include enhanced documentation recommending precautions. [1][2]
References:
- GitHub Advisory GHSA-4wrc-f8pq-fpqp (Spring Framework unsafe Java deserialization). [1]
- OSV / vulnerability record for GHSA-4wrc-f8pq-fpqp (alias CVE-2016-1000027). [2]
- Vulnerability database summary (CVE-2016-1000027). [3]
🌐 Web query:
Spring Framework 5.3.40 release date security fixes
💡 Result:
-
Spring Framework 5.3.40 was released as a commercial (support-customer) hotfix on September 12, 2024 to address a path‑traversal issue (CVE‑2024‑38816). [1][2]
-
That CVE (CVE‑2024‑38816) affects 5.3.0–5.3.39 and was fixed in 5.3.40 (commercial). Spring later published a related path‑traversal report (CVE‑2024‑38819) which still affected 5.3.0–5.3.40 and was fixed in 5.3.41. If you run 5.3.40 you should upgrade to 5.3.41 (or a supported OSS 6.1.x) to address the later report. [1][3]
Sources:
- Spring blog — "Spring Framework 5.3.40 and 6.0.24 available now" (Sept 12, 2024). [4]
- Spring Security advisory — CVE‑2024‑38816 (Sept 12, 2024). [1]
- Spring Security advisory / blog — CVE‑2024‑38819 and guidance to upgrade to 5.3.41 (Oct 2024). [3]
(If you want, I can fetch the exact release notes/changelog for 5.3.40 and 5.3.41.)
🏁 Script executed:
# Read the parent pom.xml
cat -n kernel/pom.xml | head -150Repository: sakaiproject/sakai
Length of output: 6076
🌐 Web query:
spring-webmvc path traversal vulnerabilities 5.3
💡 Result:
Short summary — Spring WebMVC (5.3) path-traversal issues and what to do:
-
CVE-2024-38816: path traversal in functional web frameworks (WebMvc.fn/WebFlux.fn). Affects Spring Framework 5.3.0 — 5.3.39; fixed in 5.3.40 (commercial release). Mitigations: upgrade to fixed release, or (if you cannot upgrade) enable Spring Security HTTP Firewall or run on Tomcat/Jetty which reject the malicious requests. [1][2]
-
CVE-2025-41242 (reported Aug 2025): path traversal affecting spring-webmvc when deployed on non‑compliant servlet containers. Affects 5.3.0 — 5.3.43 (and certain 6.x ranges); advisories recommend upgrading to a patched release (upstream advisories list 6.2.10 as a patched baseline and instruct upgrading 5.3.x to a patched branch). If you run on a servlet container that properly canonicalizes/rejects suspicious paths, your exposure is reduced, but upgrade is recommended. [3][4]
Action items (urgent):
- Identify projects using org.springframework:spring-webmvc 5.3.x and exactly which sub-version(s).
- Upgrade to a patched Spring release (for CVE-2024-38816: 5.3.40+ where available; for the 2025 advisory follow the patched versions in your advisory feed — see links). If you rely on open-source 5.3.x and a patch isn’t available in OSS for your exact micro version, plan migration to a supported branch or apply vendor/commercial fixes. [1][2][3][4]
- As short-term mitigations: enable Spring Security HTTP Firewall and/or run on Tomcat or Jetty (if acceptable), and review any use of RouterFunctions serving static resources or FileSystemResource locations. [1][2]
Sources:
[1] Spring Security advisory — CVE-2024-38816 (path traversal in functional web frameworks). (spring.io)
[2] Spring blog post: fixes and release notes for CVE-2024-38816. (spring.io)
[3] GitHub Advisory (GHSA / CVE-2025-41242) — spring-webmvc path traversal advisory and affected/patched ranges. (github.com)
[4] GitLab advisory / package database entry for CVE-2025-41242. (advisories.gitlab.com)
If you want, tell me the exact spring-webmvc versions you have and I’ll say whether they’re vulnerable and which specific upgrade target to use.
Citations:
- 1: https://spring.io/security/cve-2024-38816?utm_source=openai
- 2: https://spring.io/blog/2024/09/12/spring-framework-releases-fixes-for-cve-2024-38816/?utm_source=openai
- 3: https://github.com/advisories/GHSA-r936-gwx5-v52f?utm_source=openai
- 4: https://advisories.gitlab.com/pkg/maven/org.springframework/spring-webmvc/CVE-2025-41242/?utm_source=openai
🌐 Web query:
Spring Framework 5.3.41 release available
💡 Result:
Yes. Spring Framework 5.3.41 was released as an enterprise (commercial) hotfix on October 17, 2024 (contains fixes for CVE‑2024‑38819 and CVE‑2024‑38820) and is available to Spring Enterprise / Tanzu customers — it was not released as an open‑source public 5.3.x update. [1][2][3]
Sources: Spring blog announcement (Oct 17, 2024). [1] Spring Enterprise project page for 5.3.41. [2] 5.3.41 docs (updated Oct 17, 2024). [3]
🏁 Script executed:
# Find and read the master pom.xml to determine actual Spring version
find . -path "*/master/pom.xml" -type fRepository: sakaiproject/sakai
Length of output: 78
🏁 Script executed:
# Look for Spring version declaration in master pom
cat -n master/pom.xml | grep -A 5 -B 5 "spring" | head -100Repository: sakaiproject/sakai
Length of output: 4929
Spring Framework 5.3.39 contains multiple known vulnerabilities requiring mitigation strategy.
Spring Framework 5.3.39 is affected by several CVEs:
| CVE | Severity | Issue | Mitigation |
|---|---|---|---|
| CVE-2024-38820 | Moderate | DataBinder case-insensitive field bypass | Requires 5.3.40+ (commercial-only release) |
| CVE-2024-38816 | High | Path traversal in functional web frameworks | Requires 5.3.40+ (commercial-only release) |
| CVE-2025-41242 | High | Path traversal in spring-webmvc | Ongoing patch releases; 5.3.43+ affected |
| CVE-2016-1000027 | Critical | Unsafe Java deserialization | Only fixed in Spring 6.0.0+ |
Given that the Sakai project cannot upgrade to Spring 6 at this time, CVE-2016-1000027 cannot be fully resolved on the 5.3.x line. For the path traversal issues (CVE-2024-38816, CVE-2025-41242), consider enabling Spring Security HTTP Firewall as a compensating control and ensure this test-helper module's runtime exposure is isolated. For commercial customers, upgrading to Spring 5.3.40 or later resolves some vulnerabilities, but plan migration to Spring 6.x when feasible to address the deserialization issue.
| <context:annotation-config /> | ||
| <!-- We scan to pick up the bullhorn handler; it will be created and self-register with the MessagingService --> | ||
| <context:component-scan base-package="org.sakaiproject.announcement.api" /> |
There was a problem hiding this comment.
This may or may not be relevant now with everything using the base class that auto-registers.
| <dependency> | ||
| <groupId>javax.inject</groupId> | ||
| <artifactId>javax.inject</artifactId> | ||
| </dependency> |
There was a problem hiding this comment.
Switch to jakarta.inject -- and consider whether the CDI API really helps anything, considering we are quite Springy.
| * specific and clear, not as a pattern to follow. | ||
| */ | ||
| @ComponentScan(basePackages = "org.sakaiproject.announcement.tool") | ||
| public class AnncTestBeanConfig { |
There was a problem hiding this comment.
This is effectively an example. Either use it for real announcement tests or extract to example area.
| import static org.assertj.core.api.Assertions.*; | ||
|
|
||
| @RunWith(SpringJUnit4ClassRunner.class) | ||
| public class ContextSetupTest extends ModiTest { |
There was a problem hiding this comment.
This whole test is an example now -- relocate.
| "file:../../announcement-impl/impl/src/webapp/WEB-INF/components.xml" | ||
| }) | ||
| @FixMethodOrder(MethodSorters.NAME_ASCENDING) | ||
| public class KernelIntegrityTest extends ModiTest { |
There was a problem hiding this comment.
This is not an example, but should be moved into the kernel (without the announcement components).
| * The default Ignite base directory is ${sakai.home}/ignite/ | ||
| */ | ||
| protected String fallbackBaseDirectory() { | ||
| return sakai_home.getPathPlus("ignite") |
There was a problem hiding this comment.
Note: we use the SysProp here because of the super special status of sakai.home and the tricky dependency graph around SakaiProperties. This is definitely better than System.getProperty, though.
| <context:annotation-config /> | ||
| <!-- We need to scan these to pick up the bullhorn handlers for MessagingServiceImpl --> | ||
| <context:component-scan base-package="org.sakaiproject.messaging.api.testhandler,org.sakaiproject.announcement.api, org.sakaiproject.profile2.model, org.sakaiproject.commons.api, org.sakaiproject.lessonbuildertool.api" /> | ||
| <context:component-scan base-package="org.sakaiproject.messaging.api.testhandler, org.sakaiproject.profile2.model, org.sakaiproject.commons.api, org.sakaiproject.lessonbuildertool.api" /> |
There was a problem hiding this comment.
With the registration now incumbent upon the handlers themselves, I think this component-scan is unnecessary (and inappropriate). If we get to a point where tool-owned components are in their own tier (context), this kind of scan would always come up empty. It only kind-of works now because everything goes into one context.
| <modelVersion>4.0.0</modelVersion> | ||
| <groupId>org.sakaiproject.kernel</groupId> | ||
| <artifactId>sakai-test-helper</artifactId> | ||
| <packaging>jar</packaging> | ||
| <name>Sakai Kernel Test Helper</name> |
There was a problem hiding this comment.
There may be a way to make this jar without replicating all of the stuff from kernel or deploy that makes it all work, but there are a bunch of places where we change scope from provided to compile so they get unrolled and flattened. This is the painstakingly created minimal set needed to boot the kernel.
| ### Spring and Hibernate ### | ||
| #log4j.logger.org.springframework=debug | ||
| #log4j.logger.net.sf.hibernate=debug | ||
| #log4j.logger.org.hibernate=debug | ||
|
|
||
| #log4j.logger.org.hibernate.SQL=debug | ||
| #log4j.logger.org.hibernate.type=debug | ||
| #log4j.logger.org.hibernate.tool.hbm2ddl=info | ||
| #log4j.logger.org.hibernate.cache=debug |
There was a problem hiding this comment.
Definitely want to these down by default for tests. They are extremely noisy with Ignite and Hibernate, in addition to all of the standard Spring stuff. It's easy enough to turn them up within a test context if you need.
| <dependency> | ||
| <groupId>org.springframework</groupId> | ||
| <artifactId>spring-context</artifactId> | ||
| </dependency> |
There was a problem hiding this comment.
This may not be necessary -- seems spurious.
1 participant
TL;DR -- the kernel can be booted standalone and deliver a standard application context to components and tools. This enables context hierarchy, annotation-based configuration, and acceptance testing that can use the entire real kernel and request filter stack without needing to mock anything.
This is an overhaul of the long-stale PR #10677, brought up to date with Sakai 25 and extended some. I won't repeat the lengthy description from that PR, but will mention the additions from 28dc436 and f2e2444 and a couple of notes:
I'm marking this PR as draft while I do that last couple of steps and consider what documentation, discussion, or roadmapping is appropriate.
Summary by CodeRabbit
Release Notes
✏️ Tip: You can customize this high-level summary in your review settings.