Exist: Dashboard and problem upgrading to 4.0.0 after removal of empty() sequence type

[joewiz]

What is the problem

After https://github.com/eXist-db/exist/pull/1680 was merged, the following code began raising an error in 3.7.0-SNAPSHOT (current develop):

() instance of empty()

The error:

error found while executing expression: org.exist.xquery.XPathException: err:XPST0003 unexpected token: ( [at line 1, column 21]

Note that "line 1, column 21" corresponds to the open parens of empty().

(In contrast, () instance of empty-sequence() returns true().)

The problem is that when users install eXist 4.0.0 over 3.6.1, they will not be able to install the new version of Dashboard will not install over the old version. It errors out because of this empty() error. If we can't overcome this error, it will make upgrading to eXist 4.0.0 from 3.6.1 quite painful, and we might need to revert this PR -- but doesn't this PR still allow empty() for backward compatibility?

Context information

• eXist-db version + Git Revision hash: eXist 3.7.0-SNAPSHOT 912a29ef7
• Java version: Java 9.0.4+11
• Operating system: macOS 10.13.3
• 32 or 64 bit: 64 bit
• Any custom changes in e.g. conf.xml: no

[dizzzz]

this is valid according to saxon:

() instance of empty-sequence()

whilst

() instance of empty()

returns

Description: Expected type name in SequenceType, found <function>(

[adamretter]

@dizzzz yes but eXist used to support the later (I think)

[joewiz]

As discussed just now, the intent of #1680 is to support the old empty() syntax for backward compatibility, but after #1680, code that still uses the old empty() syntax now throws an error. Thus someone running 3.6.1 or earlier who upgrades to 3.7.0-or-later will encounter a broken Dashboard - because its usermanager.xqm module uses the old empty() sequence.

As you noted in HipChat, it's strange that https://github.com/eXist-db/exist/commit/61a2bf231268c1f19bde005c35e6892410e2bc43 (from June 2006) didn't start causing the old empty() sequence to be rejected:

sequence type changed: empty-sequence() instead of empty()

[dizzzz]

So the changes are in Type.java

so the type is declared with a name and an alias:

 defineBuiltInType(EMPTY, "empty-sequence()","empty()");

however... the alias is never used as the following function is marked gray in Intellij:

in the xquerytreeparser there is a direct reference to the 'Type.EMPTY' but not with the textual name....

the function

 public static String getTypeName(int type) {
        return typeNames.get(type)[0];
    }

is not used in the parser code :-/

[dizzzz]

I agree with @joewiz that we should keep the 'upgrade path' in mind. With this change, a simple upgrade by a backup-restore will fail. It would be: backup-restore followed either by "replace XAR files in the autodeploy directory" of when in sufficient "remove old apps from database, and start database -with autodeploy running".

A tricky one. Probably it is better to have the parser support empty() and empty-sequence() for the time being. Or -as 4.0 is a major release!- provide a clear and detailed description on how to get things working.

one option is: provide a small tool, that updates the backup ZIP file which removes the APPs that are present in the ZIP file and in the autodeploy directory

second option: have this 'remove procedure' in the restore step - optional

@wolfgangmm ?

[adamretter]

So for the two XQueries (legacy, and spec compliant, respectively):

1. () instance of empty()
2. () instance of empty-sequence()

eXist-db 3.6.1 returns

1. true
2. true

eXist-db 3.7.0-SNAPSHOT+201802131306 returns:

1. An exception occurred during query execution: exerr:ERROR org.exist.xquery.XPathException: err:XPST0003 unexpected token: ( [at line 1, column 21]
2. true

[dizzzz]

this make that the only improvement (...) in 3.7 is that the inspection module (hence the fundocs documentation! --> that was my main goal) is now showing "empty-sequence()" instead of "empty()"

[adamretter]

@dizzzz no problem I am just locating the regression now...

[adamretter]

@dizzzz @joewiz Okay so here is the deal:

This is not a regression. The change was made by @shabanovd and merged by me in https://github.com/eXist-db/exist/pull/1319... after we all agreed that it was OK to break backward compatibility (because major release 4.0)!

So why are we worried now? We all previously agreed to break backward compatibility!

p.s. Dannes, sorry for the attribution of breakage. To be clear, the problem was not https://github.com/eXist-db/exist/pull/1680 (as @dizzzz already knew ;-))

[dizzzz]

ah......... ok that clarifies :-)

the issue here is that a simple backup-restore will not work for users ; because the code in a backup will use empty() as the parser will only accept empty-sequence()

This is a serious impact...... for which we should provide a solution upfront ;

I would find it acceptable if..... we would support both for the time being, though it will not solve every (future) scenario

[adamretter]

@dizzzz I don't think users should be backing up or restoring (the eXist-db shipped) Apps in that way ever! It encourages you to keep using old broken app versions.

I don't think we should support the old empty() we all decided not to do that. This is a major release, there will be API changes!

[joewiz]

Right, we agreed on dropping the old syntax, so there is no regression. I had just thought based on #1680 that we were trying to support old syntax for backward compatibility, but since we're explicitly not supporting empty() from 4.0.0 onward, we should perhaps revise the files modified in #1680 to remove the dropped syntax and comments about maintaining backward compatibility. But that isn't particularly urgent.

The urgent issue is to prevent problems as users upgrade from 3.6.1-or-earlier to 3.7.0-or-later. Currently, because the version of Dashboard installed on systems running 3.6.1-or-earlier uses empty() as a sequence type, as soon as a user upgrades to 4.0.0 (be it through backup and restore or directly swapping data directories), their Dashboard will cease working: attempts to log in will fail because of usermanager.xqm's use of empty(), and they will be unable to use Dashboard's Package Manager to upgrade to a compatible version of Dashboard.

How's this for a solution:

I will release a new version of Dashboard targeting 3.6.1-or-earlier. This will delete the empty() sequence type from the usermanager.xqm function signatures that use it. This will make the app compatible with both 3.6.1-or-earlier and 4.0.0-or-later. I will release this to the public-repo, and we will put the following into release notes for 4.0.0: "Before upgrading to eXist 4.0.0, you MUST first upgrade Dashboard. We recommend the following methods of upgrading Dashboard: either (1) via the package manager or (2) via the following script: [insert repo:install command to trigger upgrade of Dashboard]...

[wolfgangmm]

@joewiz if you release a version of dashboard target 3.6.1, it will work but the usermanager it depends on will not. So in this version you would best remove the dependency on usermanager. If people then update to latest dashboard once they upgraded to eXistdb 4.0.0, they will get a working usermanager as well.

Apart from this, I think your proposal is good. We just have to make sure to prominently document the upgrade path in the notes.

[adamretter]

@joewiz How about this for the release notes:

NOTE: the old Dashboard (version X.Y) from eXist-db 3.6.1 and before will not run on eXist-db 4.0.0.

If you use the Dashboard and wish to continue doing so, you must either:

1. Before upgrading to eXist 4.0.0, with your eXist 3.6.1 (or before), you MUST first upgrade the Dashboard. We recommend the following methods of upgrading Dashboard:
  1.1 via the package manager,
  1.2 or, via the following script: [insert repo:install command to trigger upgrade of Dashboard]...

2. After upgrading to eXist 4.0.0, you must run the following XQuery script using the Java Admin Client or eXide to upgrade the Dashboard. [Insert script here]

[joewiz]

@wolfgangmm Ok, I think I've accounted for your concern. Here's what I've done:

1. I created a new branch, https://github.com/joewiz/dashboard/tree/fix/allow-upgrade-to-4.0.0, which as you'll see from https://github.com/joewiz/dashboard/commits/fix/allow-upgrade-to-4.0.0 I forked from the late-2017 state of Dashboard master. I removed empty() and incremented the version to 0.4.10. I've successfully installed this locally on a fresh installation of 3.6.1 - and confirmed that I'm able to authenticate and don't get any usermanager.xqm errors. Here's the .xar:
   dashboard-0.4.10.xar.zip

2. I created a second new branch, https://github.com/joewiz/dashboard/commits/prepare-for-4.0.0, which merely increments the version to be higher than 0.4.10. And rather than go to a new hotfix version (0.4.11), I thought this breaking change merited a new major version, so I went straight to 1.0.0. I've successfully installed this locally on a newly built version of 3.7.0-SNAPSHOT (develop), starting up using the database that had Dashboard 0.4.10 installed - and confirmed that I'm able to authenticate and don't get any usermanager.xqm errors. Here's the .xar:
   dashboard-1.0.0.xar.zip

In fact, since the 0.4.10 has semver-max=3.6.1, the 4.0.0 installer will install its own bundled version over 0.4.10. So if the 4.0.0 installer comes with Dashboard 1.0.0, users won't even have to take any action after upgrading to 4.0.0. I tested this scenario, putting 1.0.0 into my autodeploy directory, and observed as my 3.7.0-SNAPSHOT installed 1.0.0 over 0.4.10.

If this all sounds good, I'll publish both .xar files to the public-repo.

[joewiz]

@adamretter Based on my testing above, I edited your directions. Does this make sense?

NOTE: The version of Dashboard (0.4.7) installed with eXist-db 3.6.1 (and earlier) will 
not run on eXist-db 4.0.0. Therefore, before upgrading to eXist-db 4.0.0, you MUST 
first upgrade Dashboard to v0.4.10. We recommend the following methods of upgrading 
Dashboard:

1. Open Dashboard > Package Manager, and upgrade to 0.4.10.
2. Or, run this script: `repo:install-and-deploy("http://exist-db.org/apps/dashboard", "http://demo.exist-db.org/exist/apps/public-repo/modules/find.xql")`

After upgrading to Dashboard version 0.4.10 with eXist-db 3.6.1 (or earlier), you may 
safely install eXist-db 4.0.0. Upon first starting eXist-db 4.0.0, it will automatically 
upgrade your Dashboard to the new version of Dashboard, 1.0.0.

[wolfgangmm]

@joewiz @adamretter Maybe the notes should explain that the procedure only applies to users who want to migrate their entire database, e.g.:

... Therefore, if you plan to migrate your entire database to eXist-db 4.0.0, you MUST first upgrade
Dashboard to v0.4.10 in your previous eXist instance before you move your data directory or create 
a backup to be restored into eXist-db 4.0.0. ...

[adamretter]

@joewiz Erm not quite what I was suggesting. I was suggesting that you can fix the issue either 1. before upgrading, or 2. after upgrading

[wolfgangmm]

@joewiz your packages work well for me. I would maybe just remove the dependency on usermanager from the expath-pkg.xml of Dashboard 0.4.10 (this version still has the old usermanager and the new one won't work on an older eXist).

[joewiz]

@adamretter @wolfgangmm I've revised the directions to incorporate your points:

NOTE: The version of Dashboard that was installed with eXist-db 3.6.1 (or earlier) is 
incompatible with eXist-db 4.0.0. If you plan to migrate your entire database to 
eXist-db 4.0.0, you MUST take one of the following steps to allow Dashboard to
work after the upgrade:

1. If you have not yet installed eXist-db 4.0.0, open Dashboard > Package Manager, 
and upgrade Dashboard to 0.4.10, or run this script in eXide or the Java Admin Client: 
    `repo:install-and-deploy("http://exist-db.org/apps/dashboard", "http://demo.exist-db.org/exist/apps/public-repo/modules/find.xql")`
Then you may safely install eXist-db 4.0.0 and use Dashboard.
2. If you install eXist-db 4.0.0 before upgrading Dashboard, run this script in eXide 
or the Java Admin Client: 
    `repo:install-and-deploy("http://exist-db.org/apps/dashboard", "http://demo.exist-db.org/exist/apps/public-repo/modules/find.xql")`
Then you may use Dashboard.

[adamretter]

@joewiz cool! For step.2 I assume we don't have to do an uninstall of the existing Dashboard first?

[joewiz]

@wolfgangmm Thanks for catching that. I had done so in an earlier version. I will rebase and release 0.4.10 with that change.

[joewiz]

@adamretter Correct, repo:install-and-deploy() uninstalls any previously-installed version before installing/deploying the new version.

[joewiz]

Ok, I've now published the xars for Dashboard 0.4.10 (targeting 3.6.1 and earlier) and 1.0.0 (targeting 4.0.0 and later) to the public-repo.