Syntax updates by duncdrum · Pull Request #6239 · eXist-db/exist

duncdrum · 2026-04-14T09:35:12Z

Large Changeset, supersedes #6226

use a number of open rewrite recipes for syntax updates, mainly

mod run . --recipe org.openrewrite.java.migrate.UpgradeToJava21

These are applied in the whole codebase, not just within files directly affected by the lucene update. I planned this as a separate PR, to facilitate review.

duncdrum · 2026-04-14T10:01:14Z

P.S.: the larger number of codacy issues, is to be expected. This PR liberally modifies legacy files, so all old issues resurface.

dizzzz

LGTM

- Added .moderne directory to .gitignore while excluding context and moderne.yml files to ensure proper handling of prethink output.

mod run . --recipe org.openrewrite.java.migrate.UpgradeToJava21

mostly unsorted imports, low hanging fruit to combat linter noise

- Introduced new instruction files for analyzing impact, creating organizations, creating recipes, and running recipes with OpenRewrite. - Each file includes prerequisites, workflows, and detailed steps to guide users through various tasks related to Moderne. - Enhanced documentation aims to improve user experience and streamline the process of utilizing Moderne tools effectively.

- Replaced traditional instanceof checks with pattern matching in several classes, including Backup, ExtArrayNodeSet, NewArrayNodeSet, and RemoteCollection, improving code readability and conciseness. - Updated BinaryValue to ensure proper stream closure and resource management. - Refactored ArrayType and FunHigherOrderFun for cleaner syntax and better handling of sequences. - Enhanced error handling in various classes by utilizing try-with-resources for automatic resource management.

- Adjusted property references in the exist-installer and other modules to use prefixes. - Cleaned up unused properties. - consistent order of POM elements

bump outdated actions

reinhapa

There are a couple of points that need some rework:

Still a lot of static String constants that could be converted to text blocks in order to be better readable.
We need to define how he handle imports in general, as there are some places, where single imports were replaced with start imports and vice-versa.
There are static code validation errors, that can be solved by moving the constructor in relation of the field definition or using local variables instead of fields...

reinhapa · 2026-04-15T14:43:39Z

+        if (aceTarget == ACLPermission.ACE_TARGET.USER) {
+            cmbGroupName.setEnabled(false);
+            cmbUsername.setEnabled(true);
+        } else if (aceTarget == ACLPermission.ACE_TARGET.GROUP) {
+            cmbUsername.setEnabled(false);
+            cmbGroupName.setEnabled(true);


All fine and good as long we have only two options. Using this way, we will silently miss a potential implementation space if an enum value is added later...

I don't see it. The poorly formed switch without a default doesn't anticipate anything other than group or user . The if-else-if is functionally identical, removes linter noise, and is shorter. If we ever get trustees other than group or users, this code section will have to be modified one way or the other.

reinhapa · 2026-04-15T14:50:06Z

+                if (e.getKeyCode() == KeyEvent.VK_BACK_SPACE) {
+                    hitBackspace = true;
+                    hitBackspaceOnSelection = editor.getSelectionStart() != editor.getSelectionEnd();
+                } else if (e.getKeyCode() == KeyEvent.VK_DELETE) {
+                    e.consume();
+                    comboBox.getToolkit().beep();


I know the the default "behavior" for only two options is to go with if-else-if constructs but for my taste the new switch expression seem easier to read...

s.b. we could tweak that setting, but it strikes me as a personal preference. Not sure what others think. One problem with j21 minimal switches (which I like) is that the codacy cli (on Mac) cannot parse them properly yet, so you loose analysis locally, and have to wait for CI to tell you about errors in the file.

If Codacy on desktop can parse them or not is much less relevant than the points @reinhapa raised.
I am in favour of switching to the construct that will give us static checks if all options were handled wherever possible.

my comment was about switch statements with less then two switches. @line-o happy to review your PR.

Less than two would be one or zero. I think when switching on enums the current number of options is less relevant.

@line-o you're missing the point. I'm evaluating recipes. If the output of a recipes, will lead into an infinite cycle because our manual or automated review undoes the output that is exactly the relevant information I'm looking for.

I seem to remember you arguing for switch statements with defaults in the past. But thanks for the hints about counting.

reinhapa · 2026-04-15T14:54:34Z

the change from octal to decimal makes the code less readable in this special case of umask

agreed, this is something we should try to filter from the recipe

reinhapa · 2026-04-15T14:55:34Z

+        boolean canModify = userSelected && !SecurityManager.SYSTEM.equals(selectedUsername);
+        boolean canDelete = userSelected && !(SecurityManager.SYSTEM.equals(selectedUsername) || SecurityManager.DBA_USER.equals(selectedUsername) || SecurityManager.GUEST_USER.equals(selectedUsername));


Maybe using static imports would make it even more readable

It would, but that applies to both before and after. So essentially something for a future refactor.

reinhapa · 2026-04-15T14:55:50Z

        final String selectedGroup = getSelectedGroup();

-        boolean canDelete = groupSelected && !(selectedGroup.equals(SecurityManager.DBA_GROUP) || selectedGroup.equals(SecurityManager.GUEST_GROUP));
+        boolean canDelete = groupSelected && !(SecurityManager.DBA_GROUP.equals(selectedGroup) || SecurityManager.GUEST_GROUP.equals(selectedGroup));


Maybe using static imports would make it even more readable

reinhapa · 2026-04-15T19:50:31Z

-            // async release of collection lock
-            collection.close();
-


Maybe this was removed unintentionally?

yup static analysis

duncdrum · 2026-04-16T10:07:50Z

@reinhapa thank you for looking into this. The idea was to get a sense of the capabilities of open rewrite and to see existing recipes in action against our code base.

The points you raise are valid. However we should clarify what our expectations are. From the time I spent with this tool I can say that the collaborative and iterative tooling for refactorings works well in principle, and is something I want to further explore and make use of (e.g. for migrating apps from exist v6 to v7).

When it comes to the output of any specific recipe run, there are a number of questions. (each commit above is a different recipe sometimes run repeatedly). The point is less that it promises to catch-em-all in a single run, but to quickly perform safe ones. It's not yet clear to me when the tool determines a textblock refactor to be safe. It clearly skips some. Generally, I don't mind that some are missed, as long as the ones which are found are actually implemented properly.

If the java21 recipe does not meet our expectations we have in principle three options.

we can try to figure out why and send a PR to update the recipe.
We can create private or public recipes that do what we want,
in some cases we can tweak the parameters when invoking a recipe.

The question for me really is about, is this recipe worth applying or not? To me it looks to do more good than bad overall.

as for a policy for imports, agreed. Let's discuss this in a community call, and depending on outcome we can see if we want a refractor at all, and the direction. I had to rebase this multiple times it's also possible that that introduces some regressions with star imports. The common static recipe was the least convincing and most error prone.

From a practical point of view rewrite recipes compete with other modes of enforcing code style. It's certainly not the only or best option for everything. But I see the value of having reproducible shareable and composable refactor recipes doing the heavy lifting. I'm more curious to see if I'm the only one feeling that way, or if this is something we want to explore as a group.

I m happy to drop commits from this PR, to make it a smaller change set. The gitignore and formatting commits however are important to actually use the tool without error. The pom changes seems also good. Would that be a way forward, any of the others you think are safe to pull in?

duncdrum · 2026-04-16T10:37:56Z

in case anybody is curious, fresh run of java21 recipe against develop:

Co-authored-by: Patrick Reinhart <patrick@reini.net>

duncdrum · 2026-04-17T13:45:43Z

ok I want through all the comments and suggestions. The main takeaway for me is that the java21 changes are fundamentally sound. Which is good, as they are part of the upcoming java25 changes we want to tackle in the not so distant future.

The static analysis recipe is not a good match for our code. Not even going to try to adjust it to do what we want.

the instanceOf recipe is similarly sound, but has room for improvement. I think sequence matters here. It should have run before the java21 updates. Good to know.

I m going to split the work from this PR into smaller chunks.

duncdrum requested a review from a team as a code owner April 14, 2026 09:35

dizzzz approved these changes Apr 14, 2026

View reviewed changes

duncdrum added 8 commits April 15, 2026 12:56

[ignore] update .gitignore to include moderne configuration

9b639b2

- Added .moderne directory to .gitignore while excluding context and moderne.yml files to ensure proper handling of prethink output.

[refactor] java 21 updates

f5859d8

mod run . --recipe org.openrewrite.java.migrate.UpgradeToJava21

[refactor] common static analysis error

5d24af5

mostly unsorted imports, low hanging fruit to combat linter noise

[ignore] formatting

b82b50a

[refactor] POM cleanups

09b8085

- Adjusted property references in the exist-installer and other modules to use prefixes. - Cleaned up unused properties. - consistent order of POM elements

[fix] switch to temurin

cb3109a

bump outdated actions

duncdrum force-pushed the dp-syntax-fixes branch from c89af24 to cb3109a Compare April 15, 2026 11:07

duncdrum requested a review from a team April 15, 2026 11:07

reinhapa requested changes Apr 15, 2026

View reviewed changes

Apply suggestions from code review

004897f

Co-authored-by: Patrick Reinhart <patrick@reini.net>

duncdrum marked this pull request as draft April 17, 2026 13:43

This was referenced Apr 17, 2026

Dp mod refactor1 #6246

Open

[refactor] mod refactor 2: J21 #6245

Draft

		boolean canModify = userSelected && !SecurityManager.SYSTEM.equals(selectedUsername);
		boolean canDelete = userSelected && !(SecurityManager.SYSTEM.equals(selectedUsername) \|\| SecurityManager.DBA_USER.equals(selectedUsername) \|\| SecurityManager.GUEST_USER.equals(selectedUsername));

Uh oh!

Conversation

duncdrum commented Apr 14, 2026

Uh oh!

duncdrum commented Apr 14, 2026

Uh oh!

dizzzz left a comment

Choose a reason for hiding this comment

Uh oh!

reinhapa left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

duncdrum Apr 17, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

duncdrum Apr 17, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

duncdrum commented Apr 16, 2026

Uh oh!

duncdrum commented Apr 16, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

duncdrum commented Apr 17, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

reinhapa left a comment •

edited

Loading

duncdrum Apr 17, 2026 •

edited

Loading

duncdrum Apr 17, 2026 •

edited

Loading

duncdrum commented Apr 16, 2026 •

edited

Loading