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

Add core types to be used to replace the use of jcr.Session in the kernel API #1107

Merged
merged 13 commits into from Oct 13, 2016
@@ -0,0 +1,41 @@
/*
* Licensed to DuraSpace under one or more contributor license agreements.
* See the NOTICE file distributed with this work for additional information
* regarding copyright ownership.
*
* DuraSpace licenses this file to you under the Apache License,
* Version 2.0 (the "License"); you may not use this file except in
* compliance with the License. You may obtain a copy of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing, software
* distributed under the License is distributed on an "AS IS" BASIS,
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
* See the License for the specific language governing permissions and
* limitations under the License.
*/
package org.fcrepo.kernel.api;

/**
* The basic abstraction for a Fedora repository
* @author acoburn
*/
public interface FedoraRepository {

/**
* Login to the repository
*
* @return a FedoraSession
*/
FedoraSession login();


/**
* Login to the repository with credentials
*
* @param credentials the credentials
* @return a FedoraSession
*/
FedoraSession login(final Object credentials);
}
@@ -0,0 +1,111 @@
/*
* Licensed to DuraSpace under one or more contributor license agreements.
* See the NOTICE file distributed with this work for additional information
* regarding copyright ownership.
*
* DuraSpace licenses this file to you under the Apache License,
* Version 2.0 (the "License"); you may not use this file except in
* compliance with the License. You may obtain a copy of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing, software
* distributed under the License is distributed on an "AS IS" BASIS,
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
* See the License for the specific language governing permissions and
* limitations under the License.
*/
package org.fcrepo.kernel.api;

import java.time.Duration;
import java.time.Instant;
import java.util.Collection;
import java.util.Map;
import java.util.Optional;

/**
* The Fedora Session abstraction
*
* @author acoburn
*/
public interface FedoraSession {

/**
* Expire the session
*/
void expire();

/**
* Commit any batch operations
*/
void commit();

/**
* Update the expiry by the provided amount
* @param amountToAdd the amount of time to add
* @return the new expiration date
*/
Instant updateExpiry(Duration amountToAdd);

/**
* Get the date this session was created
* @return creation date
*/
Instant getCreated();

/**
* Get the date this session expires
Copy link
Contributor

Choose a reason for hiding this comment

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

We've talked about immortal sessions. Either there should be a note explaining what to use for that "value" or this should be Optional<Instant> (I like the latter).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I updated the FedoraSession interface to return an Optional<Instant>

* @return expiration date, if one exists
*/
Optional<Instant> getExpires();

/**
* Get the session identifier
* @return the session id
*/
String getId();

/**
* Get the user identifier associated with this session
* @return the user id
*/
String getUserId();

/**
* Get a mapping of registered namespaces
* @return the namespace mapping
*/
Map<String, String> getNamespaces();

/**
* Add session-specific data
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we have a note here that it is up to the impl whether multiple values per key are afforded, and a note on the immediate impl that they aren't?

* @param key the key
* @param value the value
*
* Note: it is up to the particular implementation as to whether multi-valued session data
* is allowed.
*/
void addSessionData(String key, String value);

/**
* Retrieve the session data for a given key
* @param key the key
* @return the value
*/
Collection<String> getSessionData(String key);

/**
* Remove a particular session key-value pair
* @param key the data key
* @param value the data value
*/
void removeSessionData(String key, String value);

/**
* Remove all session data for a particular key
* @param key the data key
*/
default void removeSessionData(String key) {
getSessionData(key).forEach(v -> removeSessionData(key, v));
}
}
@@ -0,0 +1,138 @@
/*
* Licensed to DuraSpace under one or more contributor license agreements.
* See the NOTICE file distributed with this work for additional information
* regarding copyright ownership.
*
* DuraSpace licenses this file to you under the Apache License,
* Version 2.0 (the "License"); you may not use this file except in
* compliance with the License. You may obtain a copy of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing, software
* distributed under the License is distributed on an "AS IS" BASIS,
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
* See the License for the specific language governing permissions and
* limitations under the License.
*/
package org.fcrepo.kernel.api.services;

import org.fcrepo.kernel.api.FedoraSession;

/**
* @author acoburn
* @since Sept 30, 2016
*/
public interface BatchService {
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this a kind of "manage the connections between users and open transactions" thing?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There is currently an interface called TransactionService. This interface is a slight modification of that, mostly to remove some of the baggage of using the word "transaction"

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah. I wonder if we want this stuff in the kernel at all.

Partly, this is about the fact that I don't think we have any clear sense of why there are all these *Service guys.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The reason they are in the kernel is so that the HTTP layers can access these "services" via spring injection. Moving those out would be a structural change that goes way beyond the scope of this effort.

Copy link
Contributor

Choose a reason for hiding this comment

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

No, I'm not asking you to do that. I just don't want to make any effort to preserve them if we can avoid it.

Copy link
Contributor

Choose a reason for hiding this comment

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

Okay, leave it be for now. I'm certainly not trying to block this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@ajs6f: related to the org.fcrepo.kernel.api.services.*Service types, I wonder whether they might not be better situated in a different package -- say: org.fcrepo.spi or org.fcrepo.kernel.spi (which could be independent of the fcrepo-kernel-api module). Once we have a full specification and remove all of the JCR concepts from the API, I think there's a certain amount of re-organization that can happen w/r/t the modules and package namespaces, but that's a conversation for later.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, let's get through this. I have yet to hear anyone give a purpose for those types. (I know you have OSGi in mind, but I don't think could have been our original purpose. If we decide to have these kinds of types for OSGi, they should be impl'd in a common OSGi-related module, not in the kernel-api forcing everyone to impl them directly.)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'd argue that the purpose of these types is to make it possible for the HTTP layer (fcrepo-http-api and fcrepo-http-commons) to access those implementation-specific services that are injected via Spring (but that could be substituted with Blueprint for OSGi). Either way, I agree that they are not a necessary part of the kernel-api.

Copy link
Contributor

Choose a reason for hiding this comment

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

They do that, but they are only needed to do that on the assumption that model types (like FedoraResource) aren't managed by the bean container or other outer system. @awoods and I have talked in the past about whether that's a step we ought to contemplate taking. My point here is that whatever we do in the JCR impl, other people may want to play it out differently elsewhere and we shouldn't make their lives any harder.


/**
* Check for expired batch operations and remove them
*/
void removeExpired();

/**
* Create a new batch operation with a FedoraSession and add it to the currently open ones
*
* @param session The session to use for this batch operation
* @param username the name of the {@link java.security.Principal}
*/
void begin(FedoraSession session, String username);
Copy link

Choose a reason for hiding this comment

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

From the FedoraSession interface, it would appear that a userId is already associated with sessions. How is this arg username reconciled with the session userId?

Copy link
Contributor Author

@acoburn acoburn Oct 1, 2016

Choose a reason for hiding this comment

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

You are correct: the jcr.Session interface has a getUserID method, and the new FedoraSession interface has a getUserId method -- which in the impl just proxies to the jcr.Session::getUserID. The issue is that in the case of an anonymous user, the result of the jcr.Session::getUserID is not null, but rather <anonymous>. But we can't actually rely on that string, because that, too, is configurable in the repository.json file. This way, we are relying on the value of the java.security.Principal::getName value for looking up FedoraSession values.

Believe me -- my first stab at this excluded the username parameter on all of these methods, but relying on that conflicts with how transaction values are looked up.


/**
* Create a new FedoraSession for the anonymous user and add it to the currently open ones
Copy link

Choose a reason for hiding this comment

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

I am confused by: "Create a new FedoraSession..." since a FedoraSession is being passed into this method.

*
* @param session The session to use for this batch operation
*/
default void begin(FedoraSession session) {
begin(session, null);
}

/**
* Retrieve an open {@link FedoraSession} for a given user
*
* @param sessionId the Id of the {@link FedoraSession}
* @param username the name of the {@link java.security.Principal}
* @return the {@link FedoraSession} with this user
*/
FedoraSession getSession(String sessionId, String username);
Copy link

Choose a reason for hiding this comment

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

What happens if no such session for the provided username exists?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

A SessionMissingException is thrown.


/**
* Retrieve an open {@link FedoraSession} for an anonymous user
*
* @param sessionId the Id of the {@link FedoraSession}
* @return the {@link FedoraSession}
*/
default FedoraSession getSession(String sessionId) {
return getSession(sessionId, null);
}

/**
* Check if a FedoraSession exists for a particular user
*
* @param sessionId the Id of the {@link FedoraSession}
* @param username the name of the {@link java.security.Principal}
* @return the {@link FedoraSession} object for the defined user
*/
boolean exists(String sessionId, String username);

/**
* Check if a FedoraSession exists for the anonymous user
*
* @param sessionId the Id of the {@link FedoraSession}
* @return the {@link FedoraSession} object
*/
default boolean exists(String sessionId) {
return exists(sessionId, null);
}

/**
* Refresh an existing session using an implementation-defined default
Copy link

Choose a reason for hiding this comment

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

What is the difference between "Refresh an existing session" here and FedoraSession.updateExpiry? https://github.com/fcrepo4/fcrepo4/pull/1107/files#diff-2a974e96c1ebe0bcd554be7d704b6955R48

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The BatchService implementation will typically involve an in-memory (possibly distributed) map of FedoraSessions. BatchService::refresh will update the expiry of one of those batch sessions. I.e., BatchService::refresh will probably call FedoraSession::updateExpiry, but as for how that is done is up to the implementation.

*
* @param sessionId the Id of the {@link FedoraSession}
* @param username the name of the {@link java.security.Principal}
*/
void refresh(String sessionId, String username);

/**
* Refresh an existing anonymous session using an implementation-defined default
*
* @param sessionId the Id of the {@link FedoraSession}
*/
default void refresh(String sessionId) {
refresh(sessionId, null);
}

/**
* Commit any changes during a {@link FedoraSession} with the given id and username
*
* @param sessionId the id of the {@link FedoraSession}
* @param username the name of the {@link java.security.Principal}
*/
void commit(String sessionId, String username);
Copy link

Choose a reason for hiding this comment

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

There seems to be some overlap between this interface and FedoraSession. How are we reconciling this commit with FedoraSession.commit?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The BatchService::commit will call FedoraSession::commit, but it may also (depending on the implementation) remove the persistent session from the (distributed) session map.


/**
* Commit any changes during a {@link FedoraSession} with the given id for the anonymous user
*
* @param sessionId the id of the {@link FedoraSession}
*/
default void commit(String sessionId) {
commit(sessionId, null);
}

/**
* Roll back any uncommited changes during a {@link FedoraSession}
*
* @param sessionId the id of the {@link FedoraSession}
* @param username the name of the {@link java.security.Principal}
*/
void abort(String sessionId, String username);

/**
* Roll back any uncommited changes during a {@link FedoraSession} for the anonymous user
*
* @param sessionId the id of the {@link FedoraSession}
*/
default void abort(String sessionId) {
abort(sessionId, null);
}
}
@@ -0,0 +1,87 @@
/*
* Licensed to DuraSpace under one or more contributor license agreements.
* See the NOTICE file distributed with this work for additional information
* regarding copyright ownership.
*
* DuraSpace licenses this file to you under the Apache License,
* Version 2.0 (the "License"); you may not use this file except in
* compliance with the License. You may obtain a copy of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing, software
* distributed under the License is distributed on an "AS IS" BASIS,
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
* See the License for the specific language governing permissions and
* limitations under the License.
*/
package org.fcrepo.kernel.modeshape;

import javax.jcr.Credentials;
import javax.jcr.Repository;
import javax.jcr.RepositoryException;

import org.fcrepo.kernel.api.FedoraRepository;
import org.fcrepo.kernel.api.FedoraSession;
import org.fcrepo.kernel.api.exception.RepositoryRuntimeException;

/**
* The basic abstraction for a Fedora repository
* @author acoburn
*/
public class FedoraRepositoryImpl implements FedoraRepository {

private final Repository repository;

/**
* Create a FedoraRepositoryImpl with a JCR-based Repository
*
* @param repository the JCR repository
*/
public FedoraRepositoryImpl(final Repository repository) {
this.repository = repository;
}

@Override
public FedoraSession login() {
try {
return new FedoraSessionImpl(repository.login());
} catch (final RepositoryException ex) {
throw new RepositoryRuntimeException(ex);
}
}

@Override
public FedoraSession login(final Object credentials) {
if (credentials instanceof Credentials) {
try {
return new FedoraSessionImpl(repository.login((Credentials) credentials));
} catch (final RepositoryException ex) {
throw new RepositoryRuntimeException(ex);
}
}
throw new IllegalArgumentException("Credentials are of the wrong type");
Copy link
Contributor

Choose a reason for hiding this comment

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

Can this be a more specific subtype?

}

/**
* Retrieve the internal JCR Repository object
*
* @return the JCR Repository
*/
public Repository getJcrRepository() {
return repository;
}

/**
* Retrieve the internal JCR Repository from a FedoraRepository object
*
* @param repository the FedoraRepository
* @return the JCR Repository
*/
public static Repository getJcrRepository(final FedoraRepository repository) {
if (repository instanceof FedoraRepositoryImpl) {
return ((FedoraRepositoryImpl)repository).getJcrRepository();
}
throw new ClassCastException("FedoraRepository is not a " + FedoraRepositoryImpl.class.getCanonicalName());
}
}