While I was examining the openmrs-module-datafilter repository, I noticed the blinding amount of setters and getters in the EntityBasisMap class and couldn’t help but wonder if Lombok wouldn’t be a well-suited addition to the Maven configuration.
Also, I noticed the static final class for Constants that could make use of an Enum to help trim down some lines and improve type safety.
Furthermore, I had some issues with running tests locally due to the use of legacy PowerMock (found in jackson-databind dependencies and others). I suggest a migration to Mockito 5 to ensure the module is buildable on more modern environments (like jdk 17).
Finally, some tests have slight checking issues that may cause unexpected exceptions as .contains() isn’t always checked before data extraction (e.g., in DataFilterBeanFactoryPostProcessorIntegrationTest). I’d be happy to help with these cleanups!
Modules should always remain in sync with the version of Mockito provided by the lowest version of core they support. It keeps everyone happy.
Lombok is fine for new modules and the like, but I don’t think refactoring to use Lombok is useful. There’s just more chances of accidentally introducing errors.
Do you mean ImplConstants? What there do you think can usefully be changed to an Enum?
I mean, there are improvements that could be made there and I think that would be welcome, but it’s unclear exactly what you mean by “may cause unexpected exceptions”.
I also ran into some of the same issues while exploring the module.
Regarding Lombok, I agree with the concern about introducing it into an existing module purely for refactoring getters/setters. Since the module is relatively stable, minimizing large refactors probably reduces the risk of unintended regressions.
On the test side, I also noticed that some of the current tests rely on older tooling (e.g., PowerMock). Improving the reliability and maintainability of the test suite can be helpful.
For example, having stronger tests around the filtering behavior (especially the session interception and full-text filtering logic) could make it much safer to refactor the parts that interact with Hibernate Search.
Regarding the .contains() observation (made in DataFilterBeanFactoryPostProcessorIntegrationTest.java), my explanation could have used more clarity. I was on the right track but used the wrong terminology.
The actual issue is that .contains() is called in the test, but the return value isn’t used or asserted. At first, I thought it might be a smart try/catch to catch the error in its tracks. However, after checking, I confirmed that .contains() does not throw exceptions here (except for throwing a NullPointerException when being fed null in TreeSets which implement SortedSet and NavigableSet or a ClassCastException, which is not the case here).
Because the result of .contains() is not asserted, the test will always pass in its current version. I propose a snippet that would actually check the intended feature. I could create a quick ticket and fix it if you think it would be helpful.
if (!registeredFilters.contains(filterName)) {
throw new AssertionError("Expected filter '" + filterName + "' to be registered");
}
Regarding the Enum use in ImplConstants, the current code is quite repetitive and harsh on the eyes. The snippet I provided leads to a shorter and more coherent solution. Also, the static {} block used to populate the HashSet could be replaced with a lambda expresion. The proposed change would imply changing all calls.
If the result of .contains() isn’t asserted, the test wouldn’t actually verify whether the filter registration succeeded, so adding an explicit assertion would definitely improve the reliability of that test.
Strengthening the test suite could also be quite useful if the module ends up being updated, since parts of the filtering and search integration may need adjustments. Having clearer tests around filter registration would make those changes safer to implement.
Regarding the Enum suggestion for ImplConstants, I agree that the current constants can be a bit verbose. At the same time, since this module is fairly stable and used in production implementations, it might make sense to keep structural refactors minimal unless they clearly simplify the code without affecting existing usage patterns.
As you’ve identified, we don’t assert anything about the contains. Your example, though can be written much more cleanly and concisely using my least favorite thing, Assert.assertTrue().
The equally important issue is that we do nothing to assert that all of the filters in the testXMlFilters object actually exist.
Yeah, that’s what I was worried about. Are you sure that that Category is a good use-case for an enum? What does that actually save us? Are those really permanent constants that make sense?
Regarding the enum discussion in ImplConstants, I am wondering about the intended role of those categories. From looking through the module, it seems like filters are often identified primarily by their name/prefix rather than by a strongly typed category. If that’s the case @ibacher, @veryfriendlysolver, would introducing an enum for something like Category actually improve type safety or readability, or would it risk coupling the implementation too tightly to a fixed set of filter types?
For example, if implementations later introduce additional filter types (beyond things like location/program/encounter-type privilege filters), would an enum make that harder to extend compared to keeping the current constant-based approach?
The new snippet for the DataFilterBeanFactoryPostProcessorIntegrationTest class now checks for all the 3 filters respectively to make sure they are contained, and also checks if the filters exist in the registered filters. The tests are inspired by UtilTest.java. The 20 is the number of successfully loaded filters in the config files, and the 19 is the number of successfully injected filters in the SessionFactory. The difference is made by the Full-Text filter, which belongs to the Hibernate Search Engine (serving as a sort of file offset/indicator or like the page table itself). The other 19 are used directly for the SQL queries.
assertEquals(FILTER_REGISTRATION_COUNT, Util.getHibernateFilterRegistrations().size());
Set<String> registeredFilters = sessionFactory.getDefinedFilterNames();
assertEquals(FILTER_REGISTERED_COUNT, registeredFilters.size());
for (String filterName : testXMlFilters) {
assertTrue("Expected filter '" + filterName + "' to be registered in SessionFactory",
registeredFilters.contains(filterName));
}
Regarding the Enum use, I think I will backpedal. It might be over the top for just 3 strings. This does not actually save us much except some lines of code. It might be a better programming principle, but it may be harder to follow. I think the use would have been more warranted for a higher amount of strings.