Use a userspace lastmodified property instead of jcr:lastModified #1033

acoburn · 2016-05-03T14:36:03Z

Resolves: https://jira.duraspace.org/browse/FCREPO-1742
Resolves: https://jira.duraspace.org/browse/FCREPO-1498

Supersedes #917

whikloj · 2016-05-03T14:44:19Z

I don't understand all of this, but what I do understand, I like 👍

acoburn · 2016-05-03T14:45:21Z

Most of the heavy lifting was from @escowles 👏

whikloj · 2016-05-03T14:46:19Z

well congrats to both of you

whikloj · 2016-05-03T14:53:18Z

This is just an observation and is probably a question of timing and PR order, but I noticed that you guys have a couple getNode()'s in here and PR #1032 aims to remove that (I think).

I'm going to try this with my pcdm:Collections and see if the membershipResource gets reindexed when a new proxy is added.

Again, nice job. @escowles++ & @acoburn++

acoburn · 2016-05-03T15:12:02Z

@whikloj related to timing, yes, some rebasing will be required between this PR and #1032
I've been waiting for the 4.5.1 release for these two PRs. I have one more on deck, but that one depends more explicitly on this PR.

whikloj · 2016-05-12T19:01:46Z

Soooo, maybe I'm missing something but was this supposed to resolve the issue of not emitting events when ldp:membershipResources are updated by an indirectContainer? I think I made a mistake and this is not what I thought it was.

acoburn · 2016-05-12T19:07:17Z

@whikloj This PR addresses a number of related issues. The first is that of ETags. The current Fedora release uses the JCR-based lastModified property, which changes every time something changes with a resource (good) and every time any other resource links to that resource (bad). As a byproduct of using a user-space last-modified property, we have more control over when the last-modified property changes -- including when a resource changes due to a change of LDP-based membership triples (i.e. direct and indirect container). And with this PR, when LDP membership triples change, the user-space last-modified property changes, causing an event to be emitted.

ajs6f · 2016-05-12T19:11:15Z

fcrepo-http-api/src/test/java/org/fcrepo/integration/http/api/FedoraLdpIT.java

+        patch.addHeader("Content-Type", "application/sparql-update");
+        patch.setEntity(new StringEntity(
+                "INSERT { <> <http://purl.org/dc/elements/1.1/relation> <" + serverAddress + id1 + "> } WHERE {}"));
+        try (final CloseableHttpResponse response = execute(patch)) {


Isn't this just this?

yes, thanks for catching that

whikloj · 2016-05-12T19:19:33Z

@acoburn ok, that's what I thought but in creating an indirect container and adding an object to it, the membershipResource resource in Fedora gets the updated property but it does not seem to be "re-indexed" by the camel-toolbox. Perhaps it is my setup, I'll try and log the actual JMS messages coming out.

ajs6f · 2016-05-12T19:20:09Z

...ernel-modeshape/src/main/java/org/fcrepo/kernel/modeshape/services/ContainerServiceImpl.java

@@ -61,10 +64,29 @@ public Container findOrCreate(final Session session, final String path) {

            if (node.isNew()) {
                initializeNewObjectProperties(node);
+
+                final Node parent = node.getDepth() > 0 ? node.getParent() : null;


Am I nuts or is this very much like what's new in BinaryServiceImpl above?

I put this and several other instances of similar code into a single utility function.

If the dsNode was created without a slug and therefore the parent is a PairTree, do we want to be touching the PairTree or the logical parent container? I suspect the latter.

ajs6f · 2016-05-12T19:46:57Z

Aside from sniping comments about code style and the like, this looks basically good to me. I mean, it's definitely the right direction. Just as a note to myself, the integration test for FedoraLdp is getting crazy long. I think we may need to break it up at some point.

Resolves: https://jira.duraspace.org/browse/FCREPO-1742 Resolves: https://jira.duraspace.org/browse/FCREPO-1498

acoburn · 2016-05-13T12:20:07Z

@whikloj you are right -- the eventing machinery is missing some changes related to indirect containers. Stay tuned for another commit related to that.

whikloj · 2016-05-13T13:16:45Z

@acoburn cool thanks.

…roperty

ajs6f · 2016-05-17T13:15:04Z

Is this ready?

acoburn · 2016-05-17T13:15:40Z

yes, please review/test

ajs6f · 2016-05-17T13:19:37Z

fcrepo-kernel-modeshape/src/main/java/org/fcrepo/kernel/modeshape/utils/FedoraTypesUtils.java

+        return empty();
+    }
+
+    public static Function<Session, Function<Resource, Optional<String>>> resourceToProperty = session -> resource -> {


This might be clearer as a method:

public static Function<Resource, Optional<String>> resourceToProperty(Session session) {

The only time you use it that I see, you explicitly call apply. That's the very semantic of a plain method, which is more concise.

The Session is actually part of the state of the Resource -> Optional<String> function.

Yeah, I previously changed it to a plain method and then changed it back for some reason (I don't recall exactly why). I'll see about changing to a plain method.

acoburn · 2016-05-17T17:48:00Z

@ajs6f I have added another commit to address your most recent comments, along with a unit test for the PropertyChangedListener class.

ajs6f · 2016-05-17T17:53:04Z

Looks good to me. I'll wait a bit for other's comments.

awoods · 2016-05-17T18:09:10Z

/me reviewing now

awoods · 2016-05-17T21:38:02Z

fcrepo-kernel-modeshape/src/main/java/org/fcrepo/kernel/modeshape/utils/FedoraTypesUtils.java

+     * Check if a property is intentionally suppressed.
+     */
+    final static List<String> suppressed = Arrays.asList("jcr:mimeType", "jcr:lastModified");
+    private static Predicate<Property> isSuppressedProperty = uncheck(p -> suppressed.contains(p.getName()));


This Predicate does not appear to be used. If so, please remove.

@awoods this Predicate has been removed.

awoods · 2016-05-17T21:52:32Z

fcrepo-kernel-modeshape/src/main/java/org/fcrepo/kernel/modeshape/utils/FedoraTypesUtils.java

+     */
+    public static void touchLdpMembershipResource(final Node node) throws RepositoryException {
+        if (node.getDepth() > 0) {
+            final Node parent = node.getParent();


This logic only works as expected if the parent is the Direct/Indirect container... which is true if the node was created with a slug. However, in the common case where no slug is used and the UUIDPathMinter creates the hierarchy, the true Direct/Indirect container does not get updated.
This can be demonstrated in an integration test.

@awoods that is an excellent point. I wonder if reusing some of the code in FedoraResource::getContainer would be appropriate.

FedoraResource::getContainer should resolve the issue.

acoburn · 2016-05-18T02:55:58Z

@awoods I have addressed your comments in a new commit to this PR.

awoods · 2016-05-18T13:28:24Z

fcrepo-kernel-modeshape/src/main/java/org/fcrepo/kernel/modeshape/FedoraResourceImpl.java

        if (createdDate != null) {
            LOGGER.trace("Using created date for last modified date for node {}", node);
            return createdDate;
        }

        return null;
    }
+    private long getTimestamp(final String property, final long created) throws RepositoryException {


Minor: add a newline before this private method.

acoburn · 2016-05-18T19:26:54Z

I have addressed @awoods comments in a new commit.

awoods · 2016-05-18T19:31:54Z

👍 Looks good. Any final words before we commit?

acoburn mentioned this pull request May 3, 2016

Using userspace last-modified property #917

Closed

ajs6f reviewed May 12, 2016
View reviewed changes

escowles and others added 3 commits May 12, 2016 17:38

Using a userspace lastmodified property instead of jcr:lastModified

5ddc85b

Resolves: https://jira.duraspace.org/browse/FCREPO-1742 Resolves: https://jira.duraspace.org/browse/FCREPO-1498

Support touch on direct/indirect containers

a7182a6

code review

3b7aa7a

Make indirect containers actually work with the fedora:lastModified p…

c786dff

…roperty

ajs6f reviewed May 17, 2016
View reviewed changes

code review (2)

87824ff

awoods reviewed May 17, 2016
View reviewed changes

code review (3)

9f68918

awoods reviewed May 17, 2016
View reviewed changes

code review (4)

b846dec

awoods reviewed May 18, 2016
View reviewed changes

code review (5)

8c1dc83

awoods merged commit 04d534d into fcrepo:master May 19, 2016

acoburn deleted the fcrepo-1742 branch May 19, 2016 18:34

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use a userspace lastmodified property instead of jcr:lastModified #1033

Use a userspace lastmodified property instead of jcr:lastModified #1033

acoburn commented May 3, 2016

whikloj commented May 3, 2016

acoburn commented May 3, 2016

whikloj commented May 3, 2016

whikloj commented May 3, 2016

acoburn commented May 3, 2016

whikloj commented May 12, 2016

acoburn commented May 12, 2016

ajs6f May 12, 2016

acoburn May 12, 2016

whikloj commented May 12, 2016

ajs6f May 12, 2016

acoburn May 12, 2016

awoods May 17, 2016

ajs6f commented May 12, 2016

acoburn commented May 13, 2016

whikloj commented May 13, 2016

ajs6f commented May 17, 2016

acoburn commented May 17, 2016

ajs6f May 17, 2016

acoburn May 17, 2016

acoburn commented May 17, 2016

ajs6f commented May 17, 2016

awoods commented May 17, 2016

awoods May 17, 2016

acoburn May 17, 2016

awoods May 17, 2016

acoburn May 17, 2016

awoods May 17, 2016

acoburn commented May 18, 2016

awoods May 18, 2016

acoburn commented May 18, 2016

awoods commented May 18, 2016

Use a userspace lastmodified property instead of jcr:lastModified #1033

Use a userspace lastmodified property instead of jcr:lastModified #1033

Conversation

acoburn commented May 3, 2016

whikloj commented May 3, 2016

acoburn commented May 3, 2016

whikloj commented May 3, 2016

whikloj commented May 3, 2016

acoburn commented May 3, 2016

whikloj commented May 12, 2016

acoburn commented May 12, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

whikloj commented May 12, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ajs6f commented May 12, 2016

acoburn commented May 13, 2016

whikloj commented May 13, 2016

ajs6f commented May 17, 2016

acoburn commented May 17, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

acoburn commented May 17, 2016

ajs6f commented May 17, 2016

awoods commented May 17, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

acoburn commented May 18, 2016

Choose a reason for hiding this comment

acoburn commented May 18, 2016

awoods commented May 18, 2016