Please audit for removed deprecations

[craigberry]

A lot of deprecated functions have recently been removed in the development branch. Sounds reasonable, except core eXist or extensions that ship with it are still using a number of the functions that core eXist just removed. For example, the dashboard was broken:

eXist-db/dashboard#83

I noticed map:new is gone, which breaks tei-simple, tei-publisher-lib, and the shared extension. I could find these pretty easily by doing this from the top level of the build directory:

$ find . -name '*.xql' | xargs -n1 grep -H 'map:new' | wc -l
      55
$ find . -name '*.xql' | xargs -n1 grep -H 'map:remove' | wc -l
       7
$ find . -name '*.xql' | xargs -n1 grep -H 'map:for-each' | wc -l
      14

but I can't think of a way to get a list of all the removed functions without reading through thousands of lines of commit diffs and trying to identify the XQuery names and namespaces from the removed Java code. Is there a handy list of all the removed functions?

[joewiz]

@craigberry Great question. Here's a list of functions deprecated in eXist 4.x that were removed in #2603 for the forthcoming release of eXist 5 (of course, the list may change before the release). @adamretter Please correct/amend if I've missed anything - see also some inline questions.

1. fn module
   1. fn:map -> use XQuery 3.1 fn:for-each
   2. fn:map-pairs -> use XQuery 3.1 fn:for-each-pair
2. map module
   1. map:for-each-entry -> use XQuery 3.1 map:for-each
   2. map:new -> use XQuery 3.1 map:merge
3. util extension module
   1. util:catch -> use XQuery 3.1 try-catch expression
   2. util:eval-async (long broken)
   3. util:serialize -> use XQuery 3.1 fn:serialize
4. validation extension module
   1. validation:validate -> use the other more specific functions in this module
   2. validation:validate-report -> see above
5. xmldb extension module
   1. xmldb:add-user-to-group -> sm:add-group-member
   2. xmldb:change-user -> various more specific functions in sm module
   3. xmldb:chmod-collection -> sm:chmod
   4. xmldb:chmod-resource -> sm:chmod
   5. xmldb:copy -> xmldb:copy-collection & xmldb:copy-resource
   6. xmldb:create-group -> sm:create-group
   7. xmldb:create-user -> sm:create-account
   8. xmldb:delete-user -> sm:remove-account
   9. xmldb:document -> fn:doc
   10. xmldb:exists-user -> sm:user-exists
   11. xmldb:get-current-user -> sm:id()//sm:real/sm:username/string()
   12. xmldb:get-current-user-attribute -> sm:get-account-metadata
   13. xmldb:get-current-user-attribute-names -> sm:get-account-metadata-keys
   14. xmldb:get-group -> sm:get-group*
   15. xmldb:get-owner -> sm:get-permissions
   16. xmldb:get-user-groups -> sm:get-user-groups
   17. xmldb:get-user-home -> No replacement! This function hasn’t actually returned anything meaningful for probably a decade.
   18. xmldb:get-user-primary-group -> sm:get-user-primary-group
   19. xmldb:get-users -> sm:list-users
   20. xmldb:group-exists -> sm:group-exists
   21. xmldb:is-admin-user -> sm:is-dba
   22. xmldb:is-authenticated -> sm:is-authenticated, sm:is-externally-authenticated
   23. xmldb:get-permissions -> sm:get-permissions
   24. xmldb:permissions-to-string -> sm:octal-to-mode
   25. xmldb:string-to-permissions -> sm:mode-to-octal
   26. xmldb:remove-user-from-group -> sm:remove-group-member
   27. xmldb:set-collection-permissions -> sm:chmod (see also sm:chown, sm:chgrp)
   28. xmldb:set-resource-permissions -> sm:chmod (see also sm:chown, sm:chgrp)

(On the topic of the xmldb module's functions related to users, accounts, and permissions removed here, it's worth reviewing the entire security manager (sm) module if you haven't used it before. See http://exist-db.org/exist/apps/fundocs/view.html?uri=http://exist-db.org/xquery/securitymanager.)

The following modules were removed:

1. context (obsolete; see PR for explanation)
2. datetime -> all functions except for datetime:parse-* are available in XQuery 3.1, FunctX, or https://gist.github.com/adamretter/fcd8c99cf977ff31ad83dc37c4e49783. (@adamretter thinks it may be possible to implement datetime:parse-* in XQuery; see also https://gist.github.com/joewiz/e5ae9e84fa16c34b20424ed8dd2f4aae.)
3. ftp -> use EXPath File Transfer Client (available via Package Manager)
4. httpclient -> use EXPath HTTP Client (included with eXist)
5. math-ext -> use XQuery 3.1 math module
6. memcached (obsolete; see PR for explanation)
7. svn -> use https://github.com/shabanovd/eXist-svn
8. xmpp (obsolete; see PR for explanation)

The PR also removed the WAR build facility.

[dizzzz]

@joewiz thank you for this splendid summary !

[craigberry]

Thanks, @joewiz for the very helpful documentation of what's being removed. I have used this list to throw together a quick and dirty Perl script that runs a shell command that scans .xql files in the current directory tree and reports what to do with removed functions only. I didn't attempt to do anything with the module removals as it might be harder, for example to parse "ftp" and make sure it wasn't just some legitimate string.

The script doesn't find all that much in a fresh git checkout of the develop branch:

$ perl ../removed_func_audit.pl

Replace the following instances of xmldb:is-admin-user with sm:is-dba.

./extensions/modules/persistentlogin/src/main/resources/org/exist/xquery/modules/persistentlogin/login.xql:    if (not($asDba) or xmldb:is-admin-user($user)) then (
./extensions/modules/persistentlogin/src/main/resources/org/exist/xquery/modules/persistentlogin/login.xql:        if ($isLoggedIn and (not($asDba) or xmldb:is-admin-user($user))) then (

After restoring a backup of my working database and running again, I see that the following modules all use many of the functions that have now been removed from 5.x

tei-pm-1.1.2
shared-0.8.2
tei-publisher-lib-2.6.0

So it looks like some common modules will need quite a bit of work to get up to snuff for 5.x, and a corollary is that I have no near-term prospects of testing my app with 5.x.

removed_func_audit.pl.zip

[joewiz]

@craigberry Neat. Two ideas to make your audit more realistic:

1. You look for *.xql but probably this should be *.xq* to cover .xq and .xqm - the other two common extensions used in the eXist community.
2. The fn: prefix isn't required and in most code I've seen in the eXist community is left off, so perhaps this should be stripped.

Would you consider making this change and reporting if you find more? Thanks.

[duncdrum]

@craigberry if you would like to scan for the deprecated modules I suggest scanning for the qualified namespace which needs to appear in the module import statement. E.g http://www.exist-db.org/exist/ftpclient instead of ftp (The namespace is documented in the exist-db function documentation)

[craigberry]

@joewiz good points both. I will add your number 1. For number 2, that means removing the fn: prefix from fn:map and fn:map-pairs. It seems like map is a common enough word that there would be a lot of false positives. I might come a little closer with egrep and something like /\bmap\s*(/ but that would miss cases where the opening paren is on the following line, which I think is legal? I will give this a bit more thought, but I'm not planning to write a full-fledged XQuery parser using regexes :-).

Thanks for the suggestion, @duncdrum. I will put that on my to-do list but if someone beats me to making a list of affected namespaces I won't object.

[duncdrum]

1. context: http://exist-db.org/xquery/context
2. datetime: http://exist-db.org/xquery/datetime
3. ftp: http://exist-db.org/xquery/ftpclient
4. httpclient: http://exist-db.org/xquery/httpclient
5. math-ext: http://exist-db.org/xquery/math
6. memcached: http://exist-db.org/xquery/memcached
7. svn: http://exist-db.org/xquery/versioning/svn
8. xmpp: http://exist-db.org/xquery/xmpp

[craigberry]

Thanks @duncdrum, I will add checks for those namespaces to my script soonish. For now I have a new version attached here that will find fn:map without the fn: prefix. It was trickier than I thought because I have to avoid variables named $map and parameter declarations as map(*), as map(?), etc. And I wanted it to work with the egrep on macOS, which does not have negative lookahead or lookbehind assertions. It has false positives for cases like as map(xs:integer), but I haven't found an easy way to avoid that. I may have to rewrite the whole thing to read through the files with Perl rather than spawning an egrep command.

Here's what I get in a fresh pull of the develop branch. If anyone sees additional false positives (or knows of false negatives) please comment:

$ perl ../removed_func_audit.pl

>>>  Replace the following instances of map with fn:apply.

./exist-testkit/src/main/resources/org/exist/test/runner/xml-test-runner.xq:11:declare variable $test-failure-function as (function(xs:string, map(xs:string, item()?), map(xs:string, item()?)?) as empty-sequence())? external;
./exist-testkit/src/main/resources/org/exist/test/runner/xml-test-runner.xq:12:declare variable $test-assumption-failed-function as (function(xs:string, map(xs:string, item()?)) as empty-sequence())? external;
./exist-testkit/src/main/resources/org/exist/test/runner/xml-test-runner.xq:13:declare variable $test-error-function as (function(xs:string, map(xs:string, item()?)?) as empty-sequence())? external;
./exist-testkit/src/main/resources/org/exist/test/runner/xquery-test-runner.xq:11:declare variable $test-failure-function as (function(xs:string, map(xs:string, item()?), map(xs:string, item()?)?) as empty-sequence())? external;
./exist-testkit/src/main/resources/org/exist/test/runner/xquery-test-runner.xq:12:declare variable $test-assumption-failed-function as (function(xs:string, map(xs:string, item()?)) as empty-sequence())? external;
./exist-testkit/src/main/resources/org/exist/test/runner/xquery-test-runner.xq:13:declare variable $test-error-function as (function(xs:string, map(xs:string, item()?)?) as empty-sequence())? external;
./exist-core/src/test/xquery/maps/maps.xql:303:declare %private function mt:mapTest($map as map(*)) as map(xs:integer, xs:string) {
./exist-core/src/main/resources/org/exist/xquery/lib/xqsuite/xqsuite.xql:67:        $test-failure-function as (function(xs:string, map(xs:string, item()?), map(xs:string, item()?)) as empty-sequence())?,
./exist-core/src/main/resources/org/exist/xquery/lib/xqsuite/xqsuite.xql:68:        $test-assumption-failed-function as (function(xs:string, map(xs:string, item()?)?) as empty-sequence())?,
./exist-core/src/main/resources/org/exist/xquery/lib/xqsuite/xqsuite.xql:69:        $test-error-function as (function(xs:string, map(xs:string, item()?)?) as empty-sequence())?,
./exist-core/src/main/resources/org/exist/xquery/lib/xqsuite/xqsuite.xql:137:        $test-error-function as (function(xs:string, map(xs:string, item()?)?) as empty-sequence())?) as element()? {
./exist-core/src/main/resources/org/exist/xquery/lib/xqsuite/xqsuite.xql:171:        $test-failure-function as (function(xs:string, map(xs:string, item()?), map(xs:string, item()?)) as empty-sequence())?,
./exist-core/src/main/resources/org/exist/xquery/lib/xqsuite/xqsuite.xql:172:        $test-assumption-failed-function as (function(xs:string, map(xs:string, item()?)?) as empty-sequence())?,
./exist-core/src/main/resources/org/exist/xquery/lib/xqsuite/xqsuite.xql:173:        $test-error-function as (function(xs:string, map(xs:string, item()?)?) as empty-sequence())?,
./exist-core/src/main/resources/org/exist/xquery/lib/xqsuite/xqsuite.xql:217:function test:test-assumptions($meta as element(function), $test-assumption-failed-function as (function(xs:string, map(xs:string, item()?)?) as empty-sequence())?) as element(annotation)* {
./exist-core/src/main/resources/org/exist/xquery/lib/xqsuite/xqsuite.xql:239:function test:test-assumption($assumption-annotation as element(annotation), $test-assumption-failed-function as (function(xs:string, map(xs:string, item()?)?) as empty-sequence())?) as element(annotation)? {
./exist-core/src/main/resources/org/exist/xquery/lib/xqsuite/xqsuite.xql:261:        $test-failure-function as (function(xs:string, map(xs:string, item()?), map(xs:string, item()?)) as empty-sequence())?,
./exist-core/src/main/resources/org/exist/xquery/lib/xqsuite/xqsuite.xql:262:        $test-error-function as (function(xs:string, map(xs:string, item()?)?) as empty-sequence())?,
./exist-core/src/main/resources/org/exist/xquery/lib/test.xq:95:        $test-failure-function as (function(xs:string, map(xs:string, item()?), map(xs:string, item()?)) as empty-sequence())?,
./exist-core/src/main/resources/org/exist/xquery/lib/test.xq:96:        $test-assumption-failed-function as (function(xs:string, map(xs:string, item()?)?) as empty-sequence())?,
./exist-core/src/main/resources/org/exist/xquery/lib/test.xq:97:        $test-error-function as (function(xs:string, map(xs:string, item()?)?) as empty-sequence())?,
./exist-core/src/main/resources/org/exist/xquery/lib/test.xq:266:        $test-failure-function as (function(xs:string, map(xs:string, item()?), map(xs:string, item()?)) as empty-sequence())?,
./exist-core/src/main/resources/org/exist/xquery/lib/test.xq:267:        $test-assumption-failed-function as (function(xs:string, map(xs:string, item()?)?) as empty-sequence())?,
./exist-core/src/main/resources/org/exist/xquery/lib/test.xq:268:        $test-error-function as (function(xs:string, map(xs:string, item()?)?) as empty-sequence())?,

>>>  Replace the following instances of xmldb:is-admin-user with sm:is-dba.

./extensions/modules/persistentlogin/src/main/resources/org/exist/xquery/modules/persistentlogin/login.xql:78:    if (not($asDba) or xmldb:is-admin-user($user)) then (
./extensions/modules/persistentlogin/src/main/resources/org/exist/xquery/modules/persistentlogin/login.xql:121:        if ($isLoggedIn and (not($asDba) or xmldb:is-admin-user($user))) then (

removed_func_audit.pl.zip

[joewiz]

@craigberry Thanks! The good news is that all of the cases of map are false positives - without exception, they're parameter or return type checks in function or global variable declarations. The 2 cases of xmldb:is-admin-user in login.xql line 78 and line 121 will definitely cause problems in modules that import it. This will require a PR here in the core eXist repo.

Did you say that you also observed cases of this in "tei-simple, tei-publisher-lib, and the shared extension"?

In other news, I've updated the list above to include a note from @adamretter that sm:id is the replacement for xmldb:get-current-user. I also discovered that Dmitriy already created an external extension for the SVN code, so I've added the link above. I'll update that note above as we find others.

[craigberry]

I think the only way to winnow out the false positives on map is going to be to make a list of all the data types in XQuery and consider cases of map(xs:<datatype> to be declarations and not function calls. I'm currently excluding map( *, map(?, map(+, and map(), but the list is a lot longer with the explicit data types.

Yes, tei-simple, tei-publisher-lib, and shared-resources all use some of the removed functions, usually map:new.

[craigberry]

Attempt the third. This one uses pure Perl so does not depend on having a shell or egrep and might even work on Windows. It works pretty hard to skip function names followed by quantifiers or data types that would be declarations rather than function calls. It also includes searching for deprecated modules and suggests alternatives where available.

It takes a single optional argument, which is the top-level directory of the tree you want scanned, or defaults to the current working directory. Here's what it shows in the current develop branch after running ./build.sh. As you can see, eXide, packageservice, and dashboard all have some work to do:

$ perl audit_removals.pl exist/

>>>  Replace the following instances of the function map:new with map:merge.

exist/webapp/WEB-INF/data/expathrepo/packageservice-1.3.3/modules/apputil.xql:24     map:new(

>>>  Replace the following instances of the function util:catch with XQuery 3.1 try-catch expression.

exist/webapp/WEB-INF/data/expathrepo/eXide-2.4.6/modules/upload.xql:103     util:catch("*",

>>>  Replace the following instances of the function util:serialize with fn:serialize.

exist/webapp/WEB-INF/data/expathrepo/eXide-2.4.6/modules/docs.xql:159         util:serialize($help, "method=xml omit-xml-declaration=yes")

>>>  Replace the following instances of the function validation:validate-report with more specific validation function.

exist/webapp/WEB-INF/data/expathrepo/eXide-2.4.6/modules/validate-xml.xql:30     let $report := validation:validate-report($doc, doc($schema))

>>>  Replace the following instances of the function xmldb:add-user-to-group with sm:add-group-member.

exist/webapp/WEB-INF/data/expathrepo/dashboard-2.0.0/bower_components/existdb-usermanager/modules/userManager.xqm:217                 let $null := xmldb:add-user-to-group($user, $group)  (: TODO need to be able to set manager flag! :) (: TODO implement secman version instead :)

>>>  Replace the following instances of the function xmldb:change-user with more specific sm function.

exist/webapp/WEB-INF/data/expathrepo/dashboard-2.0.0/bower_components/existdb-usermanager/modules/userManager.xqm:171                 xmldb:change-user($user, $password, $groups),           (: TODO replace with secman function :)

>>>  Replace the following instances of the function xmldb:create-group with sm:create-group.

exist/webapp/WEB-INF/data/expathrepo/dashboard-2.0.0/bower_components/existdb-usermanager/modules/userManager.xqm:210         if(xmldb:create-group($group))then (

>>>  Replace the following instances of the function xmldb:create-user with sm:create-account.

exist/webapp/WEB-INF/data/expathrepo/dashboard-2.0.0/bower_components/existdb-usermanager/modules/userManager.xqm:128         xmldb:create-user($user, $password, $groups, ()),

>>>  Replace the following instances of the function xmldb:delete-user with sm:remove-account.

exist/webapp/WEB-INF/data/expathrepo/dashboard-2.0.0/bower_components/existdb-usermanager/modules/userManager.xqm:70     xmldb:delete-user($user)

>>>  Replace the following instances of the function xmldb:exists-user with sm:user-exists.

exist/webapp/WEB-INF/data/expathrepo/eXide-2.4.6/modules/deployment.xql:185         if (xmldb:exists-user($user)) then

>>>  Replace the following instances of the function xmldb:get-group with sm:get-group*.

exist/webapp/WEB-INF/data/expathrepo/eXide-2.4.6/modules/collections.xql:128                             xmldb:get-group($path)

>>>  Replace the following instances of the function xmldb:get-owner with ???.

exist/webapp/WEB-INF/data/expathrepo/eXide-2.4.6/modules/collections.xql:123                             xmldb:get-owner($path)

>>>  Replace the following instances of the function xmldb:get-user-groups with sm:get-user-groups.

exist/webapp/WEB-INF/data/expathrepo/eXide-2.4.6/modules/deployment.xql:183     let $group := if ($perms/@group) then $perms/@group/string() else xmldb:get-user-groups($user)[1]

>>>  Replace the following instances of the function xmldb:group-exists with sm:group-exists.

exist/webapp/WEB-INF/data/expathrepo/eXide-2.4.6/modules/deployment.xql:174     if (xmldb:group-exists($group)) then

>>>  Replace the following instances of the function xmldb:is-admin-user with sm:is-dba.

exist/webapp/WEB-INF/data/expathrepo/packageservice-1.3.3/modules/install.xql:12     if (xmldb:is-admin-user(xmldb:get-current-user())) then

>>>  Replace the following instances of the function xmldb:permissions-to-string with ???.

exist/webapp/WEB-INF/data/expathrepo/eXide-2.4.6/modules/deployment.xql:205                 xmldb:permissions-to-string(util:base-to-integer(xs:int($permissions), 8))

>>>  Replace the following instances of the function xmldb:remove-user-from-group with sm:remove-group-member.

exist/webapp/WEB-INF/data/expathrepo/dashboard-2.0.0/bower_components/existdb-usermanager/modules/userManager.xqm:261                         let $null := xmldb:remove-user-from-group($existing-member, $group) return

audit_removals.pl.zip

[adamretter]

Additional security functions changes, as asked about by Joe:

1. xmldb:get-current-user-attribute -> sm:get-account-metadata
2. xmldb:get-current-user-attribute-names -> sm:get-account-metadata-keys
3. xmldb:get-owner -> sm:get-permissions
4. xmldb:get-user-home... No replacement! This function hasn’t actually returned anything meaningful for probably a decade.
5. xmldb:permissions-to-string -> sm:octal-to-mode
6. xmldb:string-to-permissions -> sm:mode-to-octal

[wolfgangmm]

@craigberry thanks for the Perl script. It was very useful to fix shared-resources and eXide.

[craigberry]

You're welcome, @wolfgangmm -- glad it helped. I see you've also been busy with tei-publisher-lib. I'm attaching a slightly revised version that updates the recommended replacements based on recent posts by @adamretter and @joewiz. What it finds won't be any different from the last version, but I think there are now no gaps in the recommended replacements.

audit_removals.pl.zip

[wolfgangmm]

I corrected two entries in the list above:

1. fn:map -> ´fn:for-each`
2. xmldb:get-current-user() -> sm:id()//sm:real/sm:username/string()

[adamretter]

@wolfgangmm I would argue that xmldb:get-current-user() should actually map to sm:id()//sm:effective/sm:username/string()

[craigberry]

There was a silly bug in my script that caused it to report only the first instance of each deprecated function; it was finding them all, but only reporting the first. If you ran it again after fixing something, you might very well find additional instances. Now fixed in the attached version.

Here's the score with the current develop branch after running build.sh:

$ git describe
eXist-5.0.0-RC1-733-gf042445fb
$ perl ../audit_removals.pl

>>>  Replace the following instances of the function xmldb:copy-collection with xmldb:copy.

./exist-core/src/test/xquery/xmldb/permission-tests.xql:111     xmldb:copy-collection($t:collection || "/inaccessible", $t:collection || "/copy")
./exist-core/src/test/xquery/xmldb/copy-tests.xql:71     let $copy := xmldb:copy-collection($t:collection, $t:target-collection)
./exist-core/src/test/xquery/xmldb/copy-tests.xql:83     let $copy := xmldb:copy-collection($t:collection, $t:target-collection, true())
./exist-core/src/test/xquery/xmldb/copy-tests.xql:95     xmldb:copy-collection($t:collection, $t:collection)

>>>  Replace the following instances of the function xmldb:copy-resource with xmldb:copy.

./exist-core/src/test/xquery/xmldb/copy-tests.xql:31     let $copy := xmldb:copy-resource($t:collection, "test.xml", $t:collection, "copy.xml")
./exist-core/src/test/xquery/xmldb/copy-tests.xql:41     let $copy := xmldb:copy-resource($t:collection, "test.xml", $t:target-collection, "copy.xml")
./exist-core/src/test/xquery/xmldb/copy-tests.xql:52     let $copy := xmldb:copy-resource($t:collection, "test.xml", $t:target-collection, "preserve.xml", true())
./exist-core/src/test/xquery/xmldb/copy-tests.xql:62     let $copy := xmldb:copy-resource($t:collection, "test.xml", $t:target-collection, "preserve.xml", true())

audit_removals.pl.zip

[craigberry]

Can someone please explain the situation with xmldb:copy? In the list @joewiz posted a couple of weeks ago, the deprecations were going in this direction:

5. xmldb:copy-collection -> xmldb:copy
6. xmldb:copy-resource -> xmldb:copy

But apparently that's backwards based on what @wolfgangmm recently fixed in eXide to make it work again with the devel-4.x.x branch. So if I finally have this straight it appears xmldb:copy is not present in eXist 5.x and its replacements are not present in eXist 4.x, and, since there is nothing in the docs for xmldb:copy indicating it's deprecated, we have a function being dropped and replaced with no notice and no deprecation cycle, so even if people do become aware that it's going away, they can't get their code ready for 5.x ahead of time without doing arcane tricks to locate which functions are available at run-time. I could not find an eXist deprecation policy, but this would not comply with any deprecation policy I've ever seen in any other project.

[wolfgangmm]

@craigberry indeed xmldb:copy in 4.x.x has been dropped and is now called xmldb:copy-resource and xmldb:copy-collection. It is true that xmldb:copy was not marked as deprecated in 4.x.x, so as you say, it appears it was removed without proper deprecation cycle. This is unfortunate indeed.

[joewiz]

Should we reverse this change? Or is there some compelling reason for the redesign?

[adamretter]

Previously you could not copy a collection or resource to a new named collection or resource which was a major short-coming of eXist-db. We are allowed to make breaking changes in major releases so I think this should be okay for 5.0 ;-)

[craigberry]

But adding the new capability does not require immediate removal of the old capability, does it? The very meaning of deprecate is to warn against. If there is no period during which people are warned against using the old API, then it is not a deprecation at all but just a breaking change, which seems capricious and user-hostile to me. It's bad enough that deprecations don't provide any real warnings, such as messages in the log that a function or namespace is being deprecated, and the only way to know something will break in the future is to read the docs.

If the maintainers of all the core extensions that ship with eXist only found out about the removals when those removals broke the code they maintain and rendered one or both of the develop and develop-4.x.x branches completely non-functional for most of the last few weeks, how is the average user in the field supposed to plan for the next upgrade?

[duncdrum]

I am leaning towards agreeing with adam on this one. There is a reason to introduce this change with 5.0.0, but xmldb:copy should not be removed from 4.x without proper deprecation warning

@craigberry given users time to test and adopt is the reason why we are doing this whole release candidate dance.

I'd also suggest to take a look at the integration-testing article in the docs. Among other things it explains how to set up a continuous integration service to test against upcoming releases for breaking changes. You'll find many examples of apps having this set up throughout our org's repo, e.g. eXide

[adamretter]

@craigberry Maybe of interest for you, there is an option in conf.xml where you can disable deprecated functions disable-deprecated-functions="no". So if you wanted to test on 4.x.x without deprecated functions, then you could flip that.

[adamretter]

@craigberry I am not intentionally trying to be hostile to users. My experience with eXist-db over the last 14 years, is that users will continue to use deprecated functions even when they know they are deprecated. When they are eventually removed, then some of them complain!

This tidy-up has happened, to try and give the developers a leaner code-base with less legacy to maintain. Hopefully this means we can move faster in future. This benefits both developers and users... So it's a trade-off ;-)

[craigberry]

I am not against deprecation and removal, especially when there is a clear case that it improves code quality, maintainability, standards compliance, or security. I am against removal without a deprecation cycle unless there is some really urgent reason to do so, such as a security hole. I have yet to hear what the urgency is for xmldb:copy. The disable-deprecated-functions configuration option is good to know about, but it won't help if a function is removed without being deprecated first. Skipping a deprecation cycle also forces either a risky all-or-nothing migration or requires special tricks to make code work on more than one version of eXist.

I am also against pushing removals before ensuring that everything that ships with eXist has been upgraded to the replacements. All the integration testing in the world does nothing but provide a false sense of security if you don't test what you ship. Running ./build.sh test, as far as I can tell, does not test the core extensions -- it just downloads the latest pre-built versions of them without doing anything to ensure they work with the current core build. So all the green lights meant nothing and what built after the removals was an eXist package with non-functional dashboard, eXide, and package manager, among other things.

Apologies for the back seat driving. I've been involved with Perl core development for close to 20 years and I'm well aware of the difficulties of managing deprecations and testing dependencies. But we do test what we ship -- there are 155 extensions that ship with Perl and their test suites get run with every run of the core test suite. Nothing gets pushed that introduces new test failures in any of them.

[duncdrum]

All the integration testing in the world does nothing but provide a false sense of security if you don't test what you ship. Running ./build.sh test, as far as I can tell, does not test the core extensions -- it just downloads the latest pre-built versions of them without doing anything to ensure they work with the current core build. So all the green lights meant nothing and what built after the removals was an eXist package with non-functional dashboard, eXide, and package manager, among other things.

Just in case anybody else is wondering ./build.sh test is running the unit-tests for the code in this repo. Integration tests for default apps are in the e2e-core repo, with the daily tests runs set up on travis.

[craigberry]

All the integration testing in the world does nothing but provide a false sense of security if you don't test what you ship. Running ./build.sh test, as far as I can tell, does not test the core extensions -- it just downloads the latest pre-built versions of them without doing anything to ensure they work with the current core build. So all the green lights meant nothing and what built after the removals was an eXist package with non-functional dashboard, eXide, and package manager, among other things.

Just in case anybody else is wondering ./build.sh test is running the unit-tests for the code in this repo. Integration tests for default apps are in the e2e-core repo, with the daily tests runs set up on travis.

That looks very promising, but if I'm understanding it right, a committer would have to run it locally before pushing to know whether a change to the core broke any of the core extensions. Is that something that committers always do? I assume that travis will test against whatever has already been committed to the core, rather than against what has yet to be committed to the core. In which case at best it can identify damage already done rather than preventing it from happening in the first place.

[duncdrum]

@craigberry I realise that the process might not be immediately obvious to folks, so let me lay it out. I m pretty optimistic that i speak for all core contributors that we are happy to hear suggestions for how to improve our set-up and do things differently, especially if these suggestions are grounded in experience with other open-source projects.

exist-db is not released continuously, to ensure the quality of release we use human code review, and automated tests. Upcoming changes only get merged into develop if the unit-tests of exist including its extensions (like the geo-index) are passing (dashboard is not an extension in that sense). In external repos, we use docker to test both against the latest stable release and HEAD of develop (and sometimes older releases).

If a PR here, looks like it might impact the default apps, reviewers will run our integration tests when submitting their review, other PRs require performance test above all (these can take quite some time to run, so reviews take time as well). We are in the process of automating these as well, and looking for contributors who would like to work on that.

In any case an automated run of the integration test suite against develop will happen within 24h max. All test failures are reported to our slack channel. A release only happens after a 48h lockout period to code changes in external apps, and after all automated tests are green.

Committers are of course free and encouraged to run our integration tests, when working on code that might affect the default apps. But in a conscious decision to try to set as low a bar for contributions as possible, this is what we came up with. Practically speaking the 24h limit works well enough, there are many contributions that might struggle with setting up the integration tests, and whose valuable contributions will never effect Dashboard n co.

In the end, develop is a development branch, and there is no guarantee that users that depend on immediate access to the latest commits, will not encounter side-effects. A calmer experience can be had via master or any of the other stable release channels that we maintain.

[adamretter]

@craigberry If you still feel it is important, I am willing to reinstate xmldb:copy as a deprecated function.

[craigberry]

@adamretter I don't have any personal stake in xmldb:copy now that the core extensions have been fixed to work with both 4.x and 5.x. But I do still feel it would be a better policy in general to require a release cycle between deprecation and removal, and to run the integration test suite before pushing removals.

[craigberry]

Apologies for the multiple script versions, but I have another one. @joewiz noticed a false negative recently in the dashboard as discussed here. In trying to avoid instances of the word "map" that are not functions, I had ended up skipping function calls without any arguments. Oops. I think I've fixed it, and have released a new version that looks like it does the right thing with the penultimate versions of eXist 4.x.x, eXide, and dashboard (current versions have had removals replaced or, in the case of eXide, handled with a multi-version dance).

Given the number of versions of the script (though I hope that's winding down), I've decided to put the latest copy of it under this new thing they've got now called version control. If you find problems, please report an issue, send me a PR, or, better yet, e-mail me a patch.

[joewiz]

@craigberry Thank you! I just used your revised script from https://github.com/craigberry/audit_exist_5x_removals to audit tei-simple-pm, and it didn't miss a single function in my testing of the resulting app. I submitted a PR and will publish a hotfix when it's merged: wolfgangmm/tei-simple-pm#29.

(Also, somehow I only just registered your point above that I had reversed the direction of the arrow noting the xmldb:copy* replacements. I fixed the original post now. Thanks for letting me know, and sorry it took me until now to realize it!)

[joewiz]

@craigberry I've published tei-simple-pm v1.1.3 to https://github.com/wolfgangmm/tei-simple-pm/releases and http://demo.exist-db.org/exist/apps/public-repo/packages/tei-pm.html?eXist-db-min-version=2.3.0.

Also, everyone, please see the latest version of Craig's https://github.com/craigberry/audit_exist_5x_removals, which now searches for instances of the xdb prefix variant of xmldb. I caught a couple of these when preparing my apps for eXist 5.

All of the apps included in stock distributions of eXist 5.x have now been audited:

• dashboard-2.0.1-prod.xar
• eXide-2.4.8.xar
• exist-documentation-4.1.1.xar
• functx-1.0.xar
• fundocs-1.1.0.xar
• markdown-0.6.xar
• monex-2.0.1.xar
• packageservice-1.3.4.xar
• shared-resources-0.8.3.xar