If this is the culprit, then why did you experience differences in localhost vs docker. This is a little strange that you got inconsistent results across the environments.
The running environments are way different for both cases: different CPU architecture, different JVM provider, different filesystem, etc. So I am assuming in one case the JVM took one jar and in the other JVM it took the other, resulting in this JAR-Hell based problem. Eliminating the old JAR from the soup made it work perfectly in both scenarios.
OK one of those subtle bugs. Nice work on tracking it!
Great work @taher, thank you for this great update contribution! I am testing the upgrade branch right now and so far the framework seems to work correctly, I was able to start and use the system and tools screens. However, I created a new component based on the start component and I get the following error when trying to build:
FAILURE: Build failed with an exception.
* Where:
Build file 'C:\Users\danut\git\dunca-scraper\runtime\component\dunca-scraper\build.gradle' line: 69
* What went wrong:
Execution failed for task ':runtime:component:dunca-scraper:copyDependencies'.
> Could not get unknown property 'archivePath' for task ':framework:jar' of type org.gradle.api.tasks.bundling.Jar.
Line 69 is the second line here:
tasks.register('copyDependencies') { doLast {
copy { from (configurations.runtimeClasspath - project(':framework').configurations.runtimeClasspath - project(':framework').jar.archivePath)
into file(projectDir.absolutePath + '/lib') }
} }
Replaced that line with this and it seems to work:
tasks.register('copyDependencies') { doLast {
copy { from (configurations.runtimeClasspath - project(':framework').configurations.runtimeClasspath - project(':framework').tasks.named('jar').get().archiveFile.get().asFile)
into file(projectDir.absolutePath + '/lib') }
} }
Can somebody else confirm this issue?
Sorry, my bad. I forgot to change the branch to ‘upgrade’ in the addons.xml file for the start component before generating the new component. It works properly.
2 Likes
We are now testing moqui 4 on our own projects. Things are looking good.
One interesting bug we caught was in view entities where we had IF as the alias for some members, it worked fine on Postgres but H2 blew up.
So lesson learned: test on different databases, you will catch bugs. Also, it might be good to enhance the entity facade to enforce validations that minimize bugs due to differences between databases.
Maybe after we launch V4 we can improve the entity engine to warn or fail on having aliases copy keywords like AS, IF, AND, OR etc …
Do all tests pass for you? When I run them I have 3 tests failing:
createBulk TestNoSqlEntity - EntityNoSqlCrud
ELI find TestNoSqlEntity - EntityNoSqlCrud
render DataView screens - ToolsScreenRenderTests
The first 2 fail only on the first run on a clean opensearch container and subsequent runs pass. The 3rd one fails consistently. Any idea why this happens?
The third one fails on subsequent runs because of data. I’ll check those two.
@grozadanut do these tests pass on master branch for you?
Yea, they all pass on master branch. And just as clarification, the third one also fails on the first run, so even with a clear state. Does that one pass for you?
I have somewhat inconsistent results. But I will dig deeper into those tests.
OK question, when you are testing in the older version, are you using elasticsearch or opensearch ?
I’m using opensearch running inside docker. As you can see here: moqui-framework/docker/opensearch-compose.yml at master · grozadanut/moqui-framework · GitHub, the older version I use opensearch 2.4.0, and in the new version(jdk21 temurin): moqui-framework/docker/opensearch-compose.yml at dunca-scraper · grozadanut/moqui-framework · GitHub, I use opensearch 3.4.0
OK I figured it out, a PR is coming soon, but essentially as I was upgrading the libraries I faced a bug with refresh semantics on opensearch. It turns out this change wasn’t necessary and is the reason for all that we faced. I made the fix as follows, you can test for yourself while I prepare my PR (I made a few other corrections.
modified framework/src/main/groovy/org/moqui/impl/context/ElasticFacadeImpl.groovy
@@ -357,8 +357,7 @@ class ElasticFacadeImpl implements ElasticFacade {
jacksonMapper.writeValue(bodyWriter, entry)
bodyWriter.append((char) '\n')
}
- Map params = isOpenSearch ? [:] : [refresh:(refresh ? "true" : "wait_for")]
- RestClient restClient = makeRestClient(Method.POST, index, "_bulk", params)
+ RestClient restClient = makeRestClient(Method.POST, index, "_bulk", [refresh:(refresh ? "true" : "wait_for")])
Render tests are failing because of stub changes as part of the upgrade to Jakarta. So working on a fix in there, another comprehensive PR for elastic, rendering and all the other issues in tests is coming.
Wow I just finally cracked the render DataView screens test bug. The bug has absolutely nothing to do with DbView, nothing to do with the test code, nothing to do with the refactor to Jakarta, nothing to do with upgrading any libraries, and nothing to do with Java 21!
So what is this bug? When you query any entity in the system, what you get back in your hands is an instance of org.moqui.entity.EntityValue. AND some entities are defined with a field called entityName. BUT the EntityValue class has a method called getEntityName() and as per the discussion earlier, groovy 4 / 5 has changed the way it maps properties to methods.
def someMember = ec.entity
.find('moqui.entity.view.DbViewEntityMember')
.condition(...)
.one()
someMember.entityName // BUG, .getEntityName() instead of .get('entityName')
This is nasty! We should be careful with every entity in the system that has a column called entityName and handle it with care. Alternatively, we can rename getEntityName() in EntityValue to something else, but this would break the signature of a core moqui API.
Anyway, I am researching ways to override this behavior to restore original behavior. We can accept the groovy property map in many places, but EntityValue is not one of them. This can cause many bugs.
This groovy spec is the source of our bug. So the pattern is specifically for getters and setters like getEntityName() and the other methods that start with “get” or “set”. It seems like a rename is necessary especially for this class EntityValue. The alternative is to access safely like value.get('entityName') or value.@entityName (need to check the proper syntax but you get the idea).
This is a set of PRs that makes ALL unit tests pass successfully for both the framework and all main components, it also solves multiple bugs and behavior issues. We had to introduce a breaking change by renaming two functions in EntityValue and it was properly documented in the release notes.
moqui-framework PR: fix all testing bugs, remove old comments, fix elasticsearch issues around refresh semantics, rename EntityValue APIs to solve groovy4+ bug, and remove redundant tasks.
mantle-usl PR: minor comment fix to follow API change above
moqui-camel PR: Fix to run tests successfully
SimpleScreens PR: Fix to run tests and follow API change in framework. Please create “upgrade” branch to stabilize.
1 Like
moqui-sso had an upgrade branch, that had the gradle fixes. I just sent one to fix the javax → jakarta update(1 line to build.gradle). Locally, I have moqui and keycloak(23) working together, no custom keycloak adapter code, so this has been tested.
1 Like
I just checked, actually all fixes are there, but seem to be in master instead of upgrade