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

mikedurbin
Copy link
Contributor

No description provided.

@ajs6f
Copy link
Contributor

ajs6f 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
Copy link
Contributor Author

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


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

Choose a reason for hiding this comment

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

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

  - 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 };
Copy link

Choose a reason for hiding this comment

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

So much TEXT_PLAIN!

...I will remove it for you.

@awoods
Copy link

awoods commented May 8, 2014

Resolved with: 2afdcf0

Addresses: https://www.pivotaltracker.com/story/show/70597666

@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
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants