remove dependency on fcrepo-mint #856

acoburn · 2015-07-27T13:32:07Z

See: https://jira.duraspace.org/browse/FCREPO-1642

escowles · 2015-07-27T13:45:44Z

This replicates the current behavior, but there's also an outstanding ticket for making the event ids configurable: https://jira.duraspace.org/browse/FCREPO-1568

Can we inject a Supplier, allowing us to wire up whatever kind of ID minter was desired? This would also remove the dependency on fcrepo-mint.

ajs6f · 2015-07-27T13:51:16Z

@escowles How do you want to use a Supplier? To supply a minter or to supply ids? Keep in mind that the basic Supplier type doesn't guarantee that you get the same thing each time, so supplying ids makes sense, but not supplying minters.

escowles · 2015-07-27T13:58:11Z

@ajs6f I think it's stripping out the angle brackets: I meant to say Supplier<String> -- so a supplier of ids. I think it makes sense to use the same approach we're using for the main repository ids in https://github.com/fcrepo4/fcrepo4/blob/master/fcrepo-http-commons/src/main/java/org/fcrepo/http/commons/AbstractResource.java#L101

ajs6f · 2015-07-27T14:01:22Z

Holy crap! That's no good. Supplier doesn't guarantee that it won't give you the same thing over and over again. I hope I didn't do that in AbstractResource, but I think I might have absent-mindedly done that. If we want to use Supplier to produce a stream of unique things, we have to subtype it.

escowles · 2015-07-27T14:07:46Z

So maybe we should add a PidMinter interface to fcrepo-kernel-api, and then inject fcrepo-mint instances in the fcrepo-http-commons repository config?

ajs6f · 2015-07-27T14:33:49Z

I don't think we should be as specific as PidMinter because we don't need to be. UniqueValueSupplier < Supplier would do. Then we could toast fcrepo-mint, right? We could offer a sidecar extension with the HTTP minter in it.

escowles · 2015-07-27T14:37:11Z

UniqueValueSupplier<String> sounds good to me. Do you mean remove fcrepo-mint altogether? Do the UUID impls. get moved to fcrepo-kernel-modeshape or somewhere else?

ajs6f · 2015-07-27T14:38:24Z

Hey, man, this is Java 8! The UUID impl becomes default method bodies on the interface itself.

awoods · 2015-08-03T23:23:43Z

fcrepo-kernel-api/src/main/java/org/fcrepo/kernel/api/observer/FedoraEvent.java

@@ -64,7 +61,7 @@
     */
    public FedoraEvent(final Event e) {
        checkArgument(e != null, "null cannot support a FedoraEvent!");
-        eventID = pidMinter.get();
+        eventID = randomUUID().toString();


Instead of hard-coding randomUUID() here, is there a reason why an UniqueValueSupplier<String>, as suggested in the code review comments, was not injected?

As a note, randomUUID().toString() is not equivalent to the previous UUIDPathMinter.get(). UUIDPathMinter provides a pairtree path with a UUID leaf element, whereas randomUUID simply provides a UUID.
The reason a full pairtree path is important is because the InternalAuditor uses the FedoraEvent#eventID as the identifier name at which an audit record resource is created:
https://github.com/fcrepo4-labs/fcrepo-audit/blob/master/src/main/java/org/fcrepo/audit/InternalAuditor.java#L211

The pairtree path provided by the UUIDPathMinter gets around the performance limitation of too many children of a given resource.

acoburn · 2015-08-05T17:23:23Z

Updated PR with an injectable UniqueValueSupplier that subclasses Supplier<String>. Now neither of the fcrepo-kernel-* modules depend on fcrepo-mint.

ajs6f · 2015-08-05T17:27:41Z

...o-kernel-api/src/main/java/org/fcrepo/kernel/api/services/functions/UniqueValueSupplier.java

+ * @author awoods
+ * @author acoburn
+ */
+public interface UniqueValueSupplier extends Supplier<String> {


This is way more specific than the name UniqueValueSupplier would imply. How about HierarchicalIdentifierSupplier < UniqueValueSupplier, with UniqueValueSupplier simply guaranteeing the uniqueness of results (no default impl) and HierarchicalIdentifierSupplier bringing the "minter that creates hierarchical IDs from a UUID" semantic with the default impl?

That works for me. I can add the additional interface.

acoburn · 2015-08-05T18:32:03Z

added an interface for HierarchicalIdentifierSupplier < UniqueValueSupplier and moved the default impl to HierarchicalIdentifierSupplier, as @ajs6f suggested

ajs6f · 2015-08-05T18:39:16Z

...o-kernel-api/src/main/java/org/fcrepo/kernel/api/services/functions/UniqueValueSupplier.java

+import java.util.function.Supplier;
+
+/**
+ * Unique value minter that creates unique IDs


This is now the wrong comment. This type just guarantees the uniqueness of its supplied values. It has nothing to do with minting IDs.

The comment has been updated.

awoods · 2015-08-06T17:23:49Z

This looks good. @escowles, you will note that the issue of injecting a pidMinter implementation into FedoraEvents is still outstanding, for the same reasons mentioned in the ticket:
https://jira.duraspace.org/browse/FCREPO-1568

If @escowles or @ajs6f are comfortable with this current PR as well, I will go ahead and squash/merge.

escowles · 2015-08-06T17:28:58Z

👍 -- looks like a good improvement, and we can figure out the injection separately.

acoburn · 2015-08-06T18:05:55Z

fcrepo-kernel-api/src/main/java/org/fcrepo/kernel/api/observer/FedoraEvent.java

@@ -54,8 +54,7 @@

    private Set<Integer> eventTypes = new HashSet<>();
    private Set<String> eventProperties = new HashSet<>();
-
-    private static final Supplier<String> pidMinter = new UUIDPathMinter();
+    private UniqueValueSupplier pidMinter = new DefaultPathMinter();


shouldn't this be static final?

If you push one more commit with the addition of static we can close this PR.

Commit pushed (actually two). The nested classes had to be made static in order to use them on a static field.

awoods · 2015-08-06T21:20:33Z

Resolved with: f7daf30

awoods reviewed Aug 3, 2015
View reviewed changes

acoburn added 2 commits August 5, 2015 11:14

remove dependency on fcrepo-mint

457f9ec

added UniqueValueSupplier

f557766

ajs6f reviewed Aug 5, 2015
View reviewed changes

response to code comments

8871099

ajs6f reviewed Aug 5, 2015
View reviewed changes

acoburn added 3 commits August 5, 2015 14:57

documentation edit

dbeae7c

fix documentation errors

8f547e2

remove dependency injection for id minter

565edce

acoburn reviewed Aug 6, 2015
View reviewed changes

acoburn added 2 commits August 6, 2015 16:48

code review comments

735e934

reversed field labels

87ac128

awoods closed this Aug 6, 2015

acoburn deleted the fcrepo-1642 branch August 6, 2015 22:53

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

remove dependency on fcrepo-mint #856

remove dependency on fcrepo-mint #856

acoburn commented Jul 27, 2015

escowles commented Jul 27, 2015

ajs6f commented Jul 27, 2015

escowles commented Jul 27, 2015

ajs6f commented Jul 27, 2015

escowles commented Jul 27, 2015

ajs6f commented Jul 27, 2015

escowles commented Jul 27, 2015

ajs6f commented Jul 27, 2015

awoods Aug 3, 2015

acoburn commented Aug 5, 2015

ajs6f Aug 5, 2015

acoburn Aug 5, 2015

ajs6f Aug 5, 2015

acoburn commented Aug 5, 2015

ajs6f Aug 5, 2015

acoburn Aug 5, 2015

awoods commented Aug 6, 2015

escowles commented Aug 6, 2015

acoburn Aug 6, 2015

awoods Aug 6, 2015

awoods Aug 6, 2015

acoburn Aug 6, 2015

awoods commented Aug 6, 2015

remove dependency on fcrepo-mint #856

remove dependency on fcrepo-mint #856

Conversation

acoburn commented Jul 27, 2015

escowles commented Jul 27, 2015

ajs6f commented Jul 27, 2015

escowles commented Jul 27, 2015

ajs6f commented Jul 27, 2015

escowles commented Jul 27, 2015

ajs6f commented Jul 27, 2015

escowles commented Jul 27, 2015

ajs6f commented Jul 27, 2015

Choose a reason for hiding this comment

acoburn commented Aug 5, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

acoburn commented Aug 5, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

awoods commented Aug 6, 2015

escowles commented Aug 6, 2015

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

awoods commented Aug 6, 2015