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

Added session.logout() where missing from REST call implementations. #341

Closed
wants to merge 1 commit into from

Conversation

Projects
None yet
3 participants
@mikedurbin
Copy link
Contributor

commented May 6, 2014

No description provided.

@ajs6f

This comment has been minimized.

Copy link
Contributor

commented May 6, 2014

There's a lot of:

finally {

  •        if (rdfStream == null) {
    
  •            session.logout();
    
  •        } else {
    
  •            addCallback(rdfStream, new LogoutCallback(session));
    
  •        }
    
  •    }
    

Maybe factor that out into a method in AbstractResource or somewhere else?

@mikedurbin

This comment has been minimized.

Copy link
Contributor Author

commented May 6, 2014

Good idea... I'll centralize this bit of logic.


@Override
public void addListener(Runnable listener, Executor executor) {
executionList.add(listener, executor);

This comment has been minimized.

Copy link
@awoods

awoods May 6, 2014

Member

Please add a new unit test for this, including that listeners are indeed invoked.

* unimplemented and throws an UnsupportedOperationException.
*/
@Override
public Void get() throws InterruptedException, ExecutionException {

This comment has been minimized.

Copy link
@awoods

awoods May 6, 2014

Member

When I actually test this manually, I get errors. Some additional implementation will likely be needed:

  • RdfStream implements ListenableFuture
  • When the RdfStream has the LogoutCallback added, Uninterruptibles.getUniterruptibly(RdfStream) is called...
    which throws an exception because RdfStream.get() throws an exception.

We probably need a more robust implementation of get() here.

Added integration test for FedoraLocks.
  - demonstrating that all mime types can be retrurned
  - demonstrating the need to read the whole response
    in order to have the session close.
@Test
public void testResponseContentTypes() throws Exception {
String POSSIBLE_RDF_RESPONSE_VARIANTS_STRING[] = {
TURTLE, N3, N3_ALT2, RDF_XML, NTRIPLES, TEXT_PLAIN, APPLICATION_XML, TEXT_PLAIN, TURTLE_X };

This comment has been minimized.

Copy link
@awoods

awoods May 8, 2014

Member

So much TEXT_PLAIN!

...I will remove it for you.

@awoods

This comment has been minimized.

Copy link
Member

commented May 8, 2014

@awoods awoods closed this May 8, 2014

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.