OF-3116 & OF-3117: Iqhandler error and Testing on Windows

[annovanvliet]

The openfire logs are filled with this Error.

It is not serious, but annoying. It is triggered by a server plugin asking the last activity to a server.

2024.11.06 09:55:49.573 �[1;31mERROR�[m [socket_s2s-thread-3]: org.jivesoftware.openfire.handler.IQHandler - Internal server error
java.lang.IllegalArgumentException: IQ must be of type 'set' or 'get'. Original IQ: <iq from="jchat.mlbl.example" to="jchat.oftst.example" type="result" id="544-830"><query xmlns="jabber:iq:last" seconds="12690"></query></iq>
    at org.xmpp.packet.IQ.createResultIQ(IQ.java:378) ~[tinder-2.0.0.jar:?]
    at org.jivesoftware.openfire.handler.IQLastActivityHandler.handleIQ(IQLastActivityHandler.java:56) ~[xmppserver-4.8.3.jar:4.8.3]

Second fix is for testing DB unit test on a windows environment.

[dwd]

Hi Anno! Looks good to me, but I'm not sure whether we want this against 5.0 or main at this point. I'll leave that choice to @guusdk who knows better than I.

[guusdk]

The Windows build issue surprises me. Is that a consistent build failure for anyone on Windows? I am trying to get Windows added to our continuous integration build to catch bugs like this, but so far I haven't been able to reproduce the problem that you appear to be fixing in this PR.

I've raised https://igniterealtime.atlassian.net/browse/OF-3116 for the IQLastActivityHandler fix.

[guusdk]

After quite a bit of confusion, I believe I've figured it out: it is not the directory name separator character (\ vs /) that is causing the issue, as I initially assumed. The problem is caused by using a space character in the directory name that contains Openfire's source code (e.g. c:\Program Files\Openfire\).

This causes the database unit tests to fail.

That problem is not specific to Windows, as it also occurs on Linux. I have created https://igniterealtime.atlassian.net/browse/OF-3117 to track this issue.

[guusdk]

I've tried to add WIndows and a whitespace-in-the-working-dir to expose this issue in our CI, in this PR: #2986

Turns out that I'm not smart enough to include that whitespace in a GitHub workflow working directory in a way that is maintainable. I've given up.

[guusdk]

When running all unit tests from a directory with a space in it, I'm seeing another failure (on Linux) in the LauncherTest class. That test is new, but the construct in which it passes along a File (and it's path), is very old, and it's being used in the Launcher itself.

I wonder if this means that the Launcher is broken, when running from a directory that contains a space.

Could this be a Linux-only problem? The launcher is only used on Windows (as I believe it's only bundled with the Windows distribution). I had imagined people would have complained earlier if that fails when there's a space in the path.

[guusdk]

Turns out it was a problem in the unit test after all. I guess it's new enough to have escaped your attention.

I added a commit that fixes that unit test.

[annovanvliet]

You are correct. When i set up my Windows machine i used in my eternal wisdom my full name including spaces. An absolute showstopper for building typical Linux projects which use getResource.

[guusdk]

I'm still a bit annoyed that I cannot get this reproduced easily in our CI, which would prevent us from running into issues like this in the future. For now though, your improvements are highly appreciated.

We'll merge this in main, and backport it to the 5.0 branch.

[github-actions]

Backport failed for 5.0, because it was unable to cherry-pick the commit(s).

Please cherry-pick the changes locally and resolve any conflicts.

git fetch origin 5.0
git worktree add -d .worktree/backport-2982-to-5.0 origin/5.0
cd .worktree/backport-2982-to-5.0
git switch --create backport-2982-to-5.0
git cherry-pick -x 90b016cb4caafb9c95f256b6f6d725c24a97821d 90c703a9825dc35c772f7cec777a8c8c37c7d74a 07dee858b8f14538f57d5998e2b914974bd96459 ab3e5433c7ae40e215cf7cb248a0aed3500055d8

[guusdk]

Ah, I only now notice that this PR was made against the 5.0 branch. That's not a big deal, but breaks our normal way of working. We typically create PRs against Main, then backport to release branches (like 5.0). We've got some automation set up to help with that (which is based on and triggered by GitHub labels).

I've manually created a port from 5.0 to Main for the changes in this PR in #2987

For future PRs, it would be easier if they're created against the Main branch.