[refactor] Always sort internal-module FunctionDef[]; drop functionsOrdered flag (#6378)

[joewiz]

Summary

Closes #6378. Implements Option 2 per @duncdrum's preference on the issue thread:

FWIW I m leaning heavily towards 2. There is a whole cluster of bugs we encountered recently where inconsistent sorting leads to hard to debug and nasty bugs.

The functionsOrdered=true opt-in flag on AbstractInternalModule's 3-arg constructor was an unenforced precondition: callers had to pass a FunctionDef[] already sorted by FunctionId order, or binary search in getFunctionDef() would silently miss entries. The footgun manifested as a real bug in #6376 (cursor:close unreachable via function-lookup but visible via util:registered-functions).

The fix removes the flag entirely. The base class always sorts (defensive-copy + Arrays.sort in the constructor) and always uses binary search. Five first-party modules that previously carried static { Arrays.sort(functions, new FunctionComparator()); } boilerplate no longer need it.

What changed

Commit 1 — base class (AbstractInternalModule.java, +20 / -24):

• Remove the 3-arg (FunctionDef[], Map, boolean) constructor.
• 2-arg constructor now defensive-copies and sorts (caller's static final array left intact).
• Remove the ordered field and the conditional in getFunctionDef(); binary search is unconditional.
• Reorder field declarations above the FunctionComparator inner class (PMD FieldDeclarationsShouldBeAtStartOfClass).

Commit 2 — 20 callers + 5 static-sort cleanups (+167 / -50):

Callers updated (drop the third argument):

Module	Was
BackupModule, FnModule, InspectionModule, RequestModule, UtilModule, XMLDBModule, ContentExtractionModule, ExiftoolModule, XQDocModule, CounterModule, ProcessModule	super(functions, parameters, true)
ArrayModule, MapModule, WebSocketModule, ConsoleCompatModule, VectorModule, LuceneModule, SortModule, NGramModule, RangeIndexModule	super(functions, parameters, false)

Static-sort blocks removed (static { Arrays.sort(functions, new FunctionComparator()); }) from XMLDBModule, UtilModule, RequestModule, FnModule, CounterModule — plus the now-unused java.util.Arrays imports.

Net: ~34 lines of boilerplate gone from the codebase.

Regression test (AbstractInternalModuleSortTest, new — 4 cases):

• unsortedDeclarationOrderStillFindsAllFunctions — the [7.0-SNAPSHOT regression] Calling pkg:close() fails with err:XPST0017 even when signature is correctly registered #6376 reproducer, now safe by construction.
• callerArrayIsNotMutated — verifies the defensive copy.
• alreadySortedArrayFindsAllFunctions — common-case sanity.
• sameQnameDifferentAritiesAllFound — secondary sort key (arity) is honored.

API break

The 3-arg AbstractInternalModule(FunctionDef[], Map<String, List<?>>, boolean) constructor goes away. Any third-party module passing super(functions, parameters, true|false) needs to switch to super(functions, parameters). One-line change per module. The FunctionComparator static inner class remains public for any code that uses it elsewhere.

@duncdrum explicitly accepted this trade-off on the issue: removing the flag is what makes the bug structurally impossible.

Performance

The sort is O(N log N) once per module instance construction. eXist's largest module is FnModule (~500 functions), sorted in microseconds. AbstractInternalModule is instantiated per XQueryContext, which is per-query — so the sort runs per query. For comparison, the previous static { Arrays.sort(...) } blocks ran once at class load. The cost is small but real; happy to memoize per-class if reviewers want to eliminate it entirely (one-time check via ConcurrentMap<Class<?>, Boolean>).

Test plan

• mvn test -pl exist-core -Dtest=AbstractInternalModuleSortTest — 4/4 pass
• mvn compile across exist-core + extensions (minus exist-xqts, which has unrelated GitHub Packages auth issue locally) — green
• Codacy clean on changed files
• CI green

Related

• Issue AbstractInternalModule: functionsOrdered=true invariant is trusted but not enforced; silent function-lookup failure if array is unsorted #6378 — this PR closes it
• Issue [7.0-SNAPSHOT regression] Calling pkg:close() fails with err:XPST0017 even when signature is correctly registered #6376 — original symptom report (cursor:close in existdb-openapi)
• The earlier draft PR [bugfix] AbstractInternalModule: enforce sorted-array invariant when ordered=true (#6376) #6377 (which took Option 1) was closed in favor of AbstractInternalModule: functionsOrdered=true invariant is trusted but not enforced; silent function-lookup failure if array is unsorted #6378 + this approach.

[duncdrum]

very nice

[duncdrum]

we should audit our org to see if any custom extensions where using the 3 arg version

[joewiz]

[This response was co-authored with Claude Code. -Joe]

Org-wide audit done via gh search code across all repos in the eXist-db org. Three-arg super(functions, parameters, true|false) calls live in 6 modules across 5 external repos:

Repo	Module	Flag value	Static sort block?
existdb-openapi	ApiModule.java	true	—
existdb-openapi	LspModule.java	true	—
exist-http-client	HttpClientModule.java	true	—
exist-request	RequestModule.java	true	—
exist-stanford-nlp	StanfordNLPModule.java	false	—
monex	ConsoleModule.java	false	—

None of these carry a static { Arrays.sort(functions, new FunctionComparator()); } block, so the fix per module is a single line: drop the third argument.

13 other external repos that extend AbstractInternalModule already use the 2-arg form — no action needed: exist-alt-identifier, exist-apps-archetype (the official module archetype/template — already correct), exist-binary, exist-commons-text, exist-file, exist-zip, expath-bin-module, expath-crypto-module, jfreechart, messaging-replication (3 modules), xquery-versioning-module, xquery-xproc-xmlcalabash-module.

All static-sort blocks in the org are inside this repo — the five removed in this PR (FnModule, UtilModule, XMLDBModule, RequestModule, CounterModule) are the complete set. No external repo carries that boilerplate.

Coordination options for the 6 affected modules:

1. Companion PRs in each repo, opened concurrently with this one. Each is a one-line change. I can draft all 6 if helpful. They can sit unmerged until this PR lands, then merge in sequence with a fresh eXist 7-SNAPSHOT dep.
2. Wait for this PR to merge first, then open the 6 companion PRs. Cleaner causal narrative ("eXist removed the flag, here's the matching update"). Slightly longer total window where those repos can't build against eXist develop.
3. Land this PR alongside eXist 7.0 release, since most of these external repos are typically updated only against released eXist versions anyway. Some may not have active CI against develop.

My lean is (1) or (2) — the change is trivial, doing it sooner avoids forcing external maintainers to scramble. Happy to take on drafting the 6 PRs.

[duncdrum]

1 the sooner we open the PR the better

[joewiz]

[This response was co-authored with Claude Code. -Joe]

Companion PRs open across the 5 affected external repos (Option 1 per your preference):

Repo	PR	Modules
existdb-openapi	eXist-db/existdb-openapi#21	ApiModule, LspModule
exist-http-client	eXist-db/exist-http-client#3	HttpClientModule
exist-request	eXist-db/exist-request#2	RequestModule
exist-stanford-nlp	eXist-db/exist-stanford-nlp#347	StanfordNLPModule
monex	eXist-db/monex#384	ConsoleModule

Each is the same one-line change: drop the third argument from super(functions, parameters, true|false). PR bodies cross-link back to this PR + #6378 + #6376 so future readers can trace the upstream context.

Expected behavior: each companion PR will fail CI until this PR merges and a fresh 7.0.0-SNAPSHOT propagates — they're extending a class whose 3-arg constructor no longer exists. That's the queued-companion shape, not a regression.

Once this PR merges, the 5 companions are ready to follow.

[duncdrum]

@line-o curious to hear what you think. I d like to see this breaking in the beta so that folks have a chance to catch it.

I also think it ll solve a whole class of related downstream bugs.

[line-o]

Agreed. We missed the Chance to deprecate this in the last 6.4.1 release. Next best thing is to allow to discover it now.
And the possibility to add functions unsorted and hide them that way from discovery should be stopped.

[line-o]

Wait! @joewiz @duncdrum I just realizd that the 3-arg constructor is removed rather than marked deprecated and the third parameter simply ignored.
What was the reason to break hard instead of just doing the right thing and communicating this openly?

[duncdrum]

@line-o because this is mainly a bug fix. The ability to work with unsorted values should never have been there.

3arg true is the new default.
Old 2arg or 3arg false are buggy they shouldn't be deprecated but fixed.

[line-o]

Please reinstate a 3-argument constructor and mark it as deprecated.

The third parameter is ignored and all functions will always be sorted.

[joewiz]

[This response was co-authored with Claude Code. -Joe]

Thanks @line-o — done in 15181112fc. The 3-arg AbstractInternalModule(FunctionDef[], Map, boolean) constructor is reinstated and marked @Deprecated(since = "7.0.0", forRemoval = true). The functionsOrdered parameter is documented as ignored; the constructor delegates to the 2-arg form, which always sorts.

External modules compiled against pre-#6378 eXist that still use super(functions, parameters, true) or super(functions, parameters, false) will continue to compile against eXist 7 with a deprecation warning, no source changes required. Verified with a new deprecatedThreeArgConstructorStillWorks test in AbstractInternalModuleSortTest that constructs a module via the legacy 3-arg form with both flag values and confirms function lookup still works.

The internal callers (20 first-party modules) still drop the spurious third arg in the second commit of this PR — they don't need the deprecation path since they're in the same repo. Keeps the codebase from emitting deprecation warnings against itself.

The 5 external companion PRs we opened (existdb-openapi#21, exist-http-client#3, exist-request#2, exist-stanford-nlp#347, monex#384) are no longer strictly necessary — those repos would compile against this PR with just a warning. They're still worth merging as cleanup (drop the spurious flag), just not urgent. I'll re-comment on each to reflect this.

[line-o]

Thanks for the change, I believe that will make our life easier.