Skip to content
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.

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

remove dependency on fcrepo-mint #856

Closed
wants to merge 8 commits into from

Conversation

Projects
None yet
4 participants
@acoburn
Copy link
Contributor

commented Jul 27, 2015

@escowles

This comment has been minimized.

Copy link
Contributor

commented Jul 27, 2015

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

This comment has been minimized.

Copy link
Contributor

commented Jul 27, 2015

@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

This comment has been minimized.

Copy link
Contributor

commented Jul 27, 2015

@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

This comment has been minimized.

Copy link
Contributor

commented Jul 27, 2015

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

This comment has been minimized.

Copy link
Contributor

commented Jul 27, 2015

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

This comment has been minimized.

Copy link
Contributor

commented Jul 27, 2015

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

This comment has been minimized.

Copy link
Contributor

commented Jul 27, 2015

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

This comment has been minimized.

Copy link
Contributor

commented Jul 27, 2015

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

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

This comment has been minimized.

Copy link
@awoods

awoods Aug 3, 2015

Member

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.

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

This comment has been minimized.

Copy link
@ajs6f

ajs6f Aug 4, 2015

Contributor

This should be Java 8 Objects::requireNonNull.


import javax.jcr.RepositoryException;
import javax.jcr.observation.Event;

import org.fcrepo.kernel.api.exception.RepositoryRuntimeException;
import org.fcrepo.kernel.api.utils.EventType;
import org.fcrepo.mint.UUIDPathMinter;

import com.google.common.base.Function;
import com.google.common.base.Joiner;

This comment has been minimized.

Copy link
@ajs6f

ajs6f Aug 4, 2015

Contributor

These Guava types should be replaced with Java 8 types Function and StringJoiner (or potentially String::join or Collectors::joining).

This comment has been minimized.

Copy link
@acoburn

acoburn Aug 4, 2015

Author Contributor

I completely agree. This PR, however, was focused entirely on removing the dependency on fcrepo-mint, so doing all the java8 refactoring seemed out of scope.

This comment has been minimized.

Copy link
@awoods

awoods Aug 4, 2015

Member

Agreed, @acoburn... but thanks for the tips, @ajs6f!

@acoburn

This comment has been minimized.

Copy link
Contributor Author

commented Aug 5, 2015

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

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

This comment has been minimized.

Copy link
@ajs6f

ajs6f Aug 5, 2015

Contributor

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?

This comment has been minimized.

Copy link
@acoburn

acoburn Aug 5, 2015

Author Contributor

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

This comment has been minimized.

Copy link
@ajs6f

ajs6f Aug 5, 2015

Contributor

Right on.

*/
default public String get() {
final int defaultLength = 2;
final int defaultCount = 4;

This comment has been minimized.

Copy link
@ajs6f

ajs6f Aug 5, 2015

Contributor

Is there some reason these are local variables?

This comment has been minimized.

Copy link
@acoburn

acoburn Aug 5, 2015

Author Contributor

can you have private fields on an interface?

This comment has been minimized.

Copy link
@ajs6f

ajs6f Aug 5, 2015

Contributor

Why private? These are defaults the effects of which are entirely obvious-- why not (public) static?

This comment has been minimized.

Copy link
@acoburn

acoburn Aug 5, 2015

Author Contributor

That makes sense -- I had been trying not to leak implementation details, but in this case, that's probably a good thing to advertise.

This comment has been minimized.

Copy link
@ajs6f

ajs6f Aug 5, 2015

Contributor

Let it all hang out. There is no place for shame or modesty in Fedora.

@acoburn

This comment has been minimized.

Copy link
Contributor Author

commented Aug 5, 2015

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

import java.util.function.Supplier;

/**
* Unique value minter that creates unique IDs

This comment has been minimized.

Copy link
@ajs6f

ajs6f Aug 5, 2015

Contributor

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

This comment has been minimized.

Copy link
@acoburn

acoburn Aug 5, 2015

Author Contributor

The comment has been updated.

acoburn added some commits Aug 5, 2015

@awoods

This comment has been minimized.

Copy link
Member

commented Aug 6, 2015

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

This comment has been minimized.

Copy link
Contributor

commented Aug 6, 2015

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

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

This comment has been minimized.

Copy link
@acoburn

acoburn Aug 6, 2015

Author Contributor

shouldn't this be static final?

This comment has been minimized.

Copy link
@awoods

This comment has been minimized.

Copy link
@awoods

awoods Aug 6, 2015

Member

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

This comment has been minimized.

Copy link
@acoburn

acoburn Aug 6, 2015

Author Contributor

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

@@ -108,7 +108,7 @@

private static final Model m = createDefaultModel();

private static final Supplier<String> pidMinter = new UUIDPathMinter();
private final UniqueValueSupplier pidMinter = new DefaultPathMinter();

This comment has been minimized.

Copy link
@acoburn

acoburn Aug 6, 2015

Author Contributor

And shouldn't this be static?

This comment has been minimized.

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

This comment has been minimized.

Copy link
@awoods

awoods Aug 6, 2015

Member

Since this PR keeps staying open, I keep looking at it...
Why is this new class UniqueValueSupplier in the org.fcrepo.kernel.api.services.functions package?

Would org.fcrepo.kernel.api.identifiers make sense?

This comment has been minimized.

Copy link
@acoburn

acoburn Aug 6, 2015

Author Contributor

I put them in o.f.k.a.services.functions because they are different types of Supplier interfaces, which, itself provides a functional interface. That's the only reason.... If you'd prefer they go in o.f.k.a.identifiers, that's fine by me.

This comment has been minimized.

Copy link
@awoods

awoods Aug 6, 2015

Member

Tying it back to Supplier is rationale enough for me.

* @author awoods
* @author acoburn
*/
public interface HierarchicalIdentifierSupplier extends UniqueValueSupplier {

This comment has been minimized.

Copy link
@awoods

awoods Aug 6, 2015

Member

Why is this new class HierarchicalIdentifierSupplier in the org.fcrepo.kernel.api.services.functions package?

Would org.fcrepo.kernel.api.identifiers make sense?

acoburn added some commits Aug 6, 2015

@awoods

This comment has been minimized.

Copy link
Member

commented Aug 6, 2015

Resolved with: f7daf30

@awoods awoods closed this Aug 6, 2015

@acoburn acoburn deleted the acoburn:fcrepo-1642 branch Aug 6, 2015

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.