Skip to content

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

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
sten1ee wants to merge 7 commits into from
Closed

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

Changes from 2 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
aafe3f7
added optional 'prune' param to file:sync(...)
sten1ee Nov 1, 2019
c8f7943
addressed some of Adam's requests
sten1ee Nov 4, 2019
99ebbce
added .acquireDocumentReadLock(doc.getURI())
sten1ee Nov 4, 2019
92daf66
tabs to spaces
sten1ee Nov 4, 2019
5bba21a
added sync/4
sten1ee Nov 5, 2019
154fd86
small fix
sten1ee Nov 5, 2019
8259f45
support for prune-exclusion patterns
sten1ee Dec 12, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
52 changes: 43 additions & 9 deletions extensions/modules/file/src/main/java/org/exist/xquery/modules/file/Sync.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@
import java.nio.file.Path;
import java.nio.file.StandardCopyOption;
import java.nio.file.attribute.FileTime;
import java.net.URISyntaxException;
import java.util.*;

import javax.xml.XMLConstants;
Expand Down Expand Up @@ -40,6 +41,7 @@
import org.exist.xquery.XPathException;
import org.exist.xquery.XQueryContext;
import org.exist.xquery.value.DateTimeValue;
import org.exist.xquery.value.BooleanValue;
import org.exist.xquery.value.FunctionParameterSequenceType;
import org.exist.xquery.value.FunctionReturnSequenceType;
import org.exist.xquery.value.Sequence;
Expand All @@ -64,7 +66,10 @@ public class Sync extends BasicFunction {
Cardinality.EXACTLY_ONE, "The full path or URI to the directory"),
new FunctionParameterSequenceType("dateTime", Type.DATE_TIME,
Cardinality.ZERO_OR_ONE,
"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,
Copy link
Contributor

@adamretter adamretter Nov 4, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

@sten1ee sten1ee Nov 4, 2019 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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:

  1. Old-style invocations of that function will be rendered broken (i.e. will have to be revised)
  2. 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?

Copy link
Contributor

@adamretter adamretter Nov 4, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

@sten1ee sten1ee Nov 4, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor

@adamretter adamretter Nov 4, 2019 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

"Optional: delete any file/dir that does not correspond to a doc/collection currently in the DB.")
},
new FunctionReturnSequenceType(Type.BOOLEAN, Cardinality.EXACTLY_ONE, "true if successful, false otherwise")
);
Expand All @@ -88,15 +93,21 @@ public Sequence eval(final Sequence[] args, final Sequence contextSequence) thro
}

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

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

Date startDate = null;
if (args[2].hasOne()) {
DateTimeValue dtv = (DateTimeValue) args[2].itemAt(0);
final DateTimeValue dtv = (DateTimeValue) args[2].itemAt(0);
startDate = dtv.getDate();
}

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


boolean prune = false;
if (args[3].hasOne()) {
final BooleanValue bv = (BooleanValue) args[3].itemAt(0);
prune = bv.getValue();
}

context.pushDocumentContext();
final MemTreeBuilder output = context.getDocumentBuilder();
Expand All @@ -111,7 +122,7 @@ public Sequence eval(final Sequence[] args, final Sequence contextSequence) thro
output.addAttribute(new QName("collection", FileModule.NAMESPACE_URI), collectionPath);
output.addAttribute(new QName("dir", FileModule.NAMESPACE_URI), targetDir.toAbsolutePath().toString());

saveCollection(XmldbURI.create(collectionPath), targetDir, startDate, output);
saveCollection(XmldbURI.create(collectionPath), targetDir, startDate, prune, output);

output.endElement();
output.endDocument();
Expand All @@ -123,7 +134,7 @@ public Sequence eval(final Sequence[] args, final Sequence contextSequence) thro
return output.getDocument();
}

private void saveCollection(final XmldbURI collectionPath, Path targetDir, final Date startDate, final MemTreeBuilder output) throws PermissionDeniedException, LockException {
private void saveCollection(final XmldbURI collectionPath, Path targetDir, final Date startDate, final boolean prune, final MemTreeBuilder output) throws PermissionDeniedException, LockException {
try {
targetDir = Files.createDirectories(targetDir);
} catch(final IOException ioe) {
Expand All @@ -143,6 +154,9 @@ private void saveCollection(final XmldbURI collectionPath, Path targetDir, final
reportError(output, "Collection not found: " + collectionPath);
return;
}
if (prune) {
pruneCollectionEntries(collection, targetDir, output);
}
for (final Iterator<DocumentImpl> i = collection.iterator(context.getBroker()); i.hasNext(); ) {
DocumentImpl doc = i.next();
if (startDate == null || doc.getMetadata().getLastModified() > startDate.getTime()) {
Copy link
Contributor

@adamretter adamretter Nov 4, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You should READ_LOCK the document before reading from it

Copy link
Contributor Author

@sten1ee sten1ee Nov 4, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(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))

Copy link
Contributor

@adamretter adamretter Nov 4, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

@sten1ee sten1ee Nov 4, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

Copy link
Contributor

@adamretter adamretter Nov 4, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

@sten1ee sten1ee Nov 4, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

@sten1ee sten1ee Nov 4, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Expand All @@ -162,7 +176,7 @@ private void saveCollection(final XmldbURI collectionPath, Path targetDir, final

for (final XmldbURI childURI : subcollections) {
final Path childDir = targetDir.resolve(childURI.lastSegment().toString());
saveCollection(collectionPath.append(childURI), childDir, startDate, output);
saveCollection(collectionPath.append(childURI), childDir, startDate, prune, output);
}
}

Expand All @@ -172,6 +186,26 @@ private void reportError(final MemTreeBuilder output,final String msg) {
output.endElement();
}

private void pruneCollectionEntries(final Collection collection, final Path targetDir, final MemTreeBuilder output) {
try {
Files.walk(targetDir, 1).forEach(path -> {
try {
final String fname = path.getFileName().toString();
final XmldbURI dbname = XmldbURI.xmldbUriFor(fname);
if (!collection.hasDocument(context.getBroker(), dbname)
&& !collection.hasChildCollection(context.getBroker(), dbname)) {
Files.deleteIfExists(path);
}
} catch (IOException | URISyntaxException
| PermissionDeniedException | LockException e) {
reportError(output, e.getMessage());
}
});
} catch (IOException e) {
reportError(output, e.getMessage());
}
}

private void saveXML(final Path targetDir, final DocumentImpl doc, final MemTreeBuilder output) {
Path targetFile = targetDir.resolve(doc.getFileURI().toASCIIString());
final SAXSerializer sax = (SAXSerializer)SerializerPool.getInstance().borrowObject( SAXSerializer.class );
Expand Down