added optional 'prune' param to file:sync(...) #3084

sten1ee · 2019-11-01T14:48:52Z

Description:

file:sync(collection, targetPath, dateTime?) is now extended to accept another optional arg:
file:sync(collection, targetPath, dateTime?, prune?)
When not specified the function behaves as usual,
When specified and true, sync will take care to delete obsolete files and directories, i.e. it will delete any file/dir that does not correspond to a doc/collection currently in the DB.

Reference:

There is no GitHub issue corresponding to this PR

Type of tests:

There are no tests for the new feature.
I could not find the tests for file:sync anyway, so point me to those, pls. I will build on top of them.

sten1ee · 2019-11-01T17:19:18Z

@adamretter Could you please 'assign' a person from the eXist team, who is going to answer my questions and help me get this PR accepted!

As a beginning I see these issues:

Appveyor has failed the Win build (due to timeout), someone has to restart it or ... remove the Win build altogether
On the invocation site the new functionality will look like:

file:sync(
             concat($common:data-path, '/', $collection), 
             concat('/', $repo-path, '/', $collection), 
             (),
             true
         )

which is far from informative. Are there 'keyword arguments' in xquery, so that the last arg may be supplied in the form:

         prune: true

adamretter · 2019-11-04T17:25:15Z

extensions/modules/file/src/main/java/org/exist/xquery/modules/file/Sync.java

-                		"Optional: only resources modified after the given date/time will be synchronized.")
+                        "Optional: only resources modified after the given date/time will be synchronized."),
+                new FunctionParameterSequenceType("prune", Type.BOOLEAN,
+                        Cardinality.ZERO_OR_ONE,


The Cardinality for this should be EXACTLY_ONE. It doesn't really make sense for this to be optional as it is a boolean value.

I've made it "optional" with the single purpose that the API remains backward compatible.
Old style invocations of file:sync(...) will still work.
If the prune param is made non-optional, then:

Old-style invocations of that function will be rendered broken (i.e. will have to be revised)

The previous (3rd) param of the function will also need to become non-optional, because (to the best of my understanding) you can't have optional param, followed by non-optional, right?

That's not backwards compatible. There is no polymorphism in XQuery. You have to keep the original function signature, and then define additional function signatures with additional arities.

Wait, wait, wait.
Then (in order not to break existing code) this extended 'prune' functionality has to come together with a new function, e.g. file:sync_ex(...) or file:sync_prune(...) or something.
Is it not so?

You don't need a new named function as they are on different arities. But you have to design two signatures, the original signature, and the new signature which has an additional argument. The new signature should then be added to the FileModule.java

extensions/modules/file/src/main/java/org/exist/xquery/modules/file/Sync.java

adamretter · 2019-11-04T17:27:10Z

extensions/modules/file/src/main/java/org/exist/xquery/modules/file/Sync.java

+			}
 			for (final Iterator<DocumentImpl> i = collection.iterator(context.getBroker()); i.hasNext(); ) {
 				DocumentImpl doc = i.next();
 				if (startDate == null || doc.getMetadata().getLastModified() > startDate.getTime()) {


You should READ_LOCK the document before reading from it

(I am assuming you are referring to the collection when you say the document):
As far as I see on line 6 lines up (on line 152) the collection is locked and opened:
try(final Collection collection = context.getBroker().openCollection(collectionPath, LockMode.READ_LOCK))

You are calling doc.getMetadata().getLastModified() which is on the document, so you should read-lock the document.

No, I am not.
Once again -- this is old code, I have not modified it.
So you see - I am trying to get a feature PR through and then it turns out the code as it was before my PR was not exactly conformant.
I don't know what is the proper way to LOCK a doc in this case (and in general).
Could you suggest the relevant change to the code and I will take it from there.
(You know I've seen GitLab gives the reviewer a way to suggest changes in the code, there should be something equivalent here)

try (final ManagedLock lock = broker.getBrokerPool().getLockManager().acquireDocumentLock(docUri, LockMode.READ_LOCK) { ... }

Thanks.
You've missed one missing final - line 162: DocumentImpl doc = i.next(); ;-)

Alternatively -- is it not a better idea (for the sake of 'Atomicity')
to read_lock the whole collection tree that is being sync-ed?

adamretter · 2019-11-04T17:28:19Z

@sten1ee You can find tests in exist/extensions/modules/file/src/test. You can implement your sync tests in either Java or XQuery.

adamretter

small changes needed.

sten1ee · 2019-11-04T18:27:44Z

@adamretter : you did not answer my question:
Are there 'keyword arguments' in xquery, so that the last arg may be supplied in the form:
prune: true

And I also have this one:
So, an optional param to a xQuery function means that that param may be 'null', but this does not exempt callers from having to explicitly specify 'null' as an argument for this param?

adamretter · 2019-11-04T18:59:47Z

Are there 'keyword arguments' in xquery, so that the last arg may be supplied in the form:
prune: true

No.

So, an optional param to a xQuery function means that that param may be 'null', but this not exempt callers from having to explicitly specify 'null' as an argument for this param?

There is no null in XQuery; The closest thing is likely an "empty sequence". So even if the cardinality of the parameter is specified as ZERO_OR_ONE, you still have to specify a value when calling even if it is the empty sequence.

sten1ee · 2019-11-05T06:56:39Z

@adamretter in the Sync signature:

    new FunctionParameterSequenceType("collection", Type.STRING,
    Cardinality.EXACTLY_ONE, "The collection to sync."),
    new FunctionParameterSequenceType("targetPath", Type.ITEM, 
    Cardinality.EXACTLY_ONE, "The full path or URI to the directory"),

targetPath is declared as Type.ITEM (as opposed to Type.STRING) but is later treated as if it is Type.STRING:

	final String collectionPath = args[0].getStringValue();

	final String target = args[1].getStringValue();
	Path targetDir = FileModuleHelper.getFile(target);

Why is that?

sten1ee · 2019-11-05T08:38:00Z

I need a (xquery) hand here, in order to craft the test for this sync/4 (sync with prune)
The only sensible XQuery test I can see (so far) is:

declare
    %test:pending("need to mechanism to setup a temporary file to work with")
    %test:assertEquals("data", "true", "true", "true")
function serialization:overwrite() {

    let $node-set := text {"data"}
    let $path := system:get-exist-home() || "/test.txt"
    let $parameters := ()
    let $append := true()
    let $remove := file:delete($path)
    let $ser1 := 	file:serialize($node-set, $path, (), false())
    let $ser2 := 	file:serialize($node-set, $path, (), false())
    let $read := file:read($path)
    let $remove := file:delete($path)
    return ($read, $ser1, $ser2, $remove)
};

I am extremely short in XQuery skills, that's why I need help with the following test scenario:

create a collection C containing two docs: A and B
sync(C)
check both files are in target dir with expected content
update contents of doc A
sync(C ... mode='prune')
check both files are in target dir with expected content
remove/delete doc B from collection C
sync(C ... mode='no_prune')
check both files are in target dir with expected content
sync(C ... mode='prune')
check only file A is in target dir with expected content

Help needed!

adamretter · 2019-11-05T12:41:14Z

targetPath is declared as Type.ITEM (as opposed to Type.STRING) but is later treated as if it is Type.STRING:

It should likely have been declared as Type.String, however we can't change it easily now, and the getStringValue will likely give the expected result anyway.

adamretter · 2019-11-05T12:42:37Z

@duncdrum @joewiz any chance either of you could help @sten1ee with some XQuery for his test above?

duncdrum · 2019-11-11T16:16:37Z

@joewiz can i pass creating an XQsuite test on to you? I'm swamped with more pressing issues at the moment. If so can you assign yourself the ticket.

duncdrum

Code looks good to me. thank you @sten1ee
We are traditionally hesitant with on-disc delete operations, is there anything preventing me to prune C:\>

sten1ee · 2019-11-13T08:13:43Z

We are traditionally hesitant with on-disc delete operations
Is there anything preventing me to prune C:\>

You have a point here.
Security-wise I haven't changed anything in the existing code (the one that implemented file:sync but without option to prune).
I think exploit-wise overwriting is even more dangerous than deleting - imagine if file:sync gives you a chance to replace a system file.
OTOH disaster-wise deleting seems to be more dangerous.

Now back to the code:

on the eXist side: You can do that (ask to delete/overwrite files in C:>)
on the OS side: hopefully, eXist is not running as a user, who is granted the right to do that?

^^^^^
@duncdrum (that's to draw your attention)

line-o · 2019-11-16T10:37:03Z

related issue #2627

sten1ee · 2019-11-18T09:20:36Z

@line-o, are you willing to help this PR get through by authoring the required TC?
Please, see my post from Nov 5th (the ones starting with 'I need a (xquery) hand here')
Perhaps the TC may be simplified -- in general it has to demonstrate that the functionality is absent before that PR and present with it.

sten1ee · 2019-11-18T09:24:21Z

@duncdrum, perhaps you haven't been notified about my reply to your question about wiping out C:> with 'prune'
Please, check my comment from Nov 13th and tell me what do we do with it - is the option with using OS user rights enough of a 'protection' against this disaster or if it is not -- what else do you suggest ?

line-o · 2019-11-18T10:54:52Z

@sten1ee First of all, for me, just adding a prune argument is not enough. I need the ability to exclude files so that .git and other files are left untouched.

line-o · 2019-11-18T11:03:42Z

@duncdrum file:sync is able to modify a folder on the file system based on OS permissions. For production systems this should always be a user with limited rights. Write access includes the ability to remove files. Therefore, I see no issue in removing files when syncing to the filesystem.

joewiz · 2019-11-18T14:42:22Z

@line-o's suggestion about a mechanism for excluding file and folder paths and patterns makes a lot of sense, particularly given that we don't always store .git folders or other build artifacts in the database.

@sten1ee Would you be interested in adding this feature to this PR, or in a follow-on PR? I imagine this would take the form of a new $excludes as xs:string* parameter that would take 0-or-more strings containing the filenames or patterns. For a function that implements filename/patterns, see http://exist-db.org/exist/apps/fundocs/index.html?q=xmldb:store-files-from-pattern.

I would be happy to help craft an XQSuite test.

sten1ee · 2019-11-20T08:24:15Z

@joewiz I think I can do that, it shouldn't be that much of a work. I only need to coordinate this with (my) management as after all it is 84000 that's paying me.

Apart from that, it'd be very helpful for me if you go ahead and craft some XQSuite test for the already submitted code. Why is that? I am almost sure there will be things to fix in it, problems that I missed to spot without a test.
If you craft the test keeping in mind the 'mechanism for excluding file and folder paths and patterns' that is pending -- it will be quite easy to extend the test to cover this later.

I prefer we get all this done in a single PR, but it's not that important in the end.

sten1ee · 2019-12-12T15:30:36Z

@joewiz
I am more or less done (this includes the exclusion patterns functionality.
When will you be able to craft some TCs ?

sten1ee · 2020-01-15T11:46:05Z

@joewiz, are you still willing to contribute the XQSite test, so that we can move fwd with that feature?

joewiz · 2020-01-15T15:38:01Z

@sten1ee Sorry for the delay, I haven't forgotten. Just working on some hard deadlines and will hope to be able to supply some tests soon. In the meantime, Duncan smartly added a skeleton XQSuite test to the issue template, so if that provides the scaffolding you needed to add a test, by all means. But if that's not sufficient, I will still follow through on creating tests as soon as I can.

line-o · 2020-09-21T18:18:42Z

Just a heads up: @sten1ee your PR was discussed in todays Community Meeting. We are still committed to get it merged, but none of us has the time to prepare a test suite at the moment. As a result, this feature will not make it into the next release, which is around the corner.

adamretter · 2020-09-21T20:47:07Z

@sten1ee Of course, if you want to send a test-suite yourself, then we can get it merged ASAP.

joewiz · 2021-01-12T16:51:38Z

With #3704, I now have a technique for creating and accessing temporary directories on the filesystem, which is needed for writing tests of file:sync(). We could use the same technique for creating the xqsuite test for this PR. However, I see two challenges:

Some conflicts are present in the PR now and need to be resolved.
In light of [BUG] file:sync doesn't respect conf.xml serializer; hard codes serialization parameters #3704, it's clear to me that file:sync() needs a $parameters argument, mirroring file:serialize(), in order to allow developers to specify serialization parameters when syncing a collection to disk. See http://exist-db.org/exist/apps/fundocs/view.html?uri=http://exist-db.org/xquery/file#serialize.3. Should this $parameters argument come before or after the $prune and $excludes arguments? Or, alternatively, should we consolidate $parameters and $prune, etc. into an $options as map(*)* map, which has become quite standard in XQuery 3.1? It would look like this:

file:sync(
    $source-collection, 
    $destination-directory, 
    map { 
        "dateTime": xs:dateTime("2021-01-12T11:47:08.12-05:00"),
        "prune": true(),
        "excludes": array { "*.png", "*.jpg" },
        "indent": false(),
        "expand-xincludes": false(),
        "process-xsl-pi": false(),
        "encoding": "UTF-8",
        "omit-xml-declaration": true()
    }
)

Before we merge this PR, we'd ideally discuss the direction of parameter handling and make any necessary adjustments. Thoughts?

joewiz · 2021-01-12T17:16:02Z

Also, just to clarify for the purposes of writing tests, I seem to recall that file:sync is one-way - from database to disk - and will not pull changes from disk back to database. Could someone confirm if I'm right?

joewiz · 2021-01-12T18:45:49Z

(Also on the topic of how file:sync serializes, see #2394.)

line-o · 2021-11-09T15:08:08Z

I am in favour of an options parameter instead of separate $prune and $excludes for file:sync.

line-o · 2021-11-11T09:13:45Z

@joewiz Yes, you are right file:sync only allows to sync to disk not from it.
Re: your proposed change to the function signature:

file:sync(
    $source-collection, 
    $destination-directory, 
    map { "dateTime": xs:dateTime("2021-01-12T11:47:08.12-05:00"), ... }
)

I would love to change file:sync#3 to have the options map as the third parameter. But this then means that we have an almost breaking change to the API.
We could allow a xs:dateTime or a map(*) as the third parameter to file:sync#3, but this ODD.
While I really like to do that, maybe creating a separate file:sync#4 with explicit optional xs:dateTime as the third parameter and the options map as the fourth is the safer option.

file:sync(
    $source-collection, 
    $destination-directory,
    xs:dateTime("2021-01-12T11:47:08.12-05:00"), 
    map { "prune": true() , ... }
)

joewiz · 2021-11-11T23:36:08Z

@line-o Great. And I actually like your idea of the hybrid 3rd parameter. We could allow (xs:dateTime | map(*)) at first and immediately deprecate xs:dateTime, flagging it as to-be-deleted in the next major version of eXist.

line-o · 2021-11-13T21:37:09Z

@joewiz The first working version with the 3rd parameter accepting either an xs:dateTime or an options map is now in the follow-up PR #4081
I invite @joewiz and @sten1ee to discuss progress there and would propose to close this PR as development has taken a different route.

added optional 'prune' param to file:sync(...)

aafe3f7

dizzzz requested review from adamretter and wolfgangmm November 1, 2019 20:24

adamretter suggested changes Nov 4, 2019

View reviewed changes

addressed some of Adam's requests

c8f7943

sten1ee added 2 commits November 4, 2019 22:03

added .acquireDocumentReadLock(doc.getURI())

99ebbce

tabs to spaces

92daf66

added sync/4

5bba21a

small fix

154fd86

duncdrum added the needs XQSuite test XQSuite test required to reproduce label Nov 11, 2019

duncdrum reviewed Nov 11, 2019

View reviewed changes

support for prune-exclusion patterns

8259f45

adamretter added the awaiting-response requires additional information from submitter label Mar 11, 2020

line-o added the in progress Issue is actively being worked upon label Apr 15, 2020

joewiz self-assigned this Jul 7, 2020

joewiz mentioned this pull request Jun 2, 2021

[Master issue] Prepare eXist 5.3.0 stable release #3914

Closed

38 tasks

line-o added this to the eXist-5.4.0 milestone Jul 1, 2021

line-o mentioned this pull request Nov 10, 2021

Improve file:sync #4081

Merged

dizzzz closed this in #4081 Jan 24, 2022

Uh oh!

added optional 'prune' param to file:sync(...) #3084

added optional 'prune' param to file:sync(...) #3084

Uh oh!

Conversation

sten1ee commented Nov 1, 2019

Description:

Reference:

Type of tests:

Uh oh!

sten1ee commented Nov 1, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sten1ee Nov 4, 2019 • 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!

adamretter Nov 4, 2019 • 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!

Uh oh!

Uh oh!

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!

adamretter commented Nov 4, 2019

Uh oh!

adamretter left a comment

Choose a reason for hiding this comment

Uh oh!

sten1ee commented Nov 4, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

adamretter commented Nov 4, 2019

Uh oh!

sten1ee commented Nov 5, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sten1ee commented Nov 5, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

adamretter commented Nov 5, 2019

Uh oh!

adamretter commented Nov 5, 2019

Uh oh!

duncdrum commented Nov 11, 2019

Uh oh!

duncdrum left a comment

Choose a reason for hiding this comment

Uh oh!

sten1ee commented Nov 13, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

line-o commented Nov 16, 2019

Uh oh!

sten1ee commented Nov 18, 2019

Uh oh!

sten1ee commented Nov 18, 2019

Uh oh!

line-o commented Nov 18, 2019

Uh oh!

sten1ee commented Nov 1, 2019 •

edited

Loading

sten1ee Nov 4, 2019 •

edited

Loading

adamretter Nov 4, 2019 •

edited

Loading

sten1ee commented Nov 4, 2019 •

edited

Loading

sten1ee commented Nov 5, 2019 •

edited

Loading

sten1ee commented Nov 5, 2019 •

edited

Loading

sten1ee commented Nov 13, 2019 •

edited

Loading

sten1ee commented Nov 20, 2019 •

edited

Loading

joewiz commented Jan 15, 2020 •

edited

Loading

joewiz commented Jan 12, 2021 •

edited

Loading

joewiz commented Jan 12, 2021 •

edited

Loading

line-o commented Nov 11, 2021 •

edited

Loading