search

RusticiSoftware / TinCanJava

Tin Can Java Library
http://rusticisoftware.github.io/TinCanJava/
Apache License 2.0
44 stars 46 forks source link

Add support for GZipped contents. #17

Closed zachlowry closed 8 years ago

zachlowry commented 10 years ago

Moved this to a new PR because I messed up my upstream while creating the other PR.

zachlowry commented 10 years ago

I think all of your notes are addressed, can you review @brianjmiller? Thanks!

brianjmiller commented 10 years ago

Test doesn't seem to be passing against SCORM Cloud. I get:

Tests run: 22, Failures: 0, Errors: 1, Skipped: 0, Time elapsed: 8.59 sec <<< FAILURE! testSaveStatementGZIP(com.rusticisoftware.tincan.RemoteLRSTest) Time elapsed: 0.077 sec <<< ERROR! com.rusticisoftware.tincan.exceptions.UnexpectedHTTPResponse: Unexpected HTTP Response at com.rusticisoftware.tincan.RemoteLRS.saveStatement(RemoteLRS.java:268) at com.rusticisoftware.tincan.RemoteLRSTest.testSaveStatementGZIP(RemoteLRSTest.java:151)

brianjmiller commented 10 years ago

Returning a 400 error reading:

Object with name 'statement' could not be created from the provided JSON. Error message: Illegal character ((CTRL-CHAR, code 31)): only regular white space (\r, \n, \t) is allowed between tokens at line 1, column 2, near: ... ...

Guessing no likey the compression. I'm wondering whether there needs to be some sort of LRS detection of this.

brianjmiller commented 10 years ago

Test has been confirmed to pass against Watershed LRS. I'm, :thumbsup: over to you @bscSCORM.

bscSCORM commented 10 years ago

@zachlowry back to you with some comments

zachlowry commented 10 years ago

@bscSCORM @brianjmiller We ended up not needing this, so if it's not of any use feel free to close the PR. I won't likely have a chance to address anything with it in the near term.

bscSCORM commented 10 years ago

@brianjmiller to you then, to fix up or close -- might not be a high priority, but seems like it would be a good feature to have.

tseabrooks commented 10 years ago

Let's get this moving along @brianjmiller

brianjmiller commented 10 years ago

@tseabrooks do we have a push from someone specific on it? Otherwise I'd likely let it land after the RemoteLRS rewrite changes which I'm trying to get merged in.

brianjmiller commented 10 years ago

See PR #27, once that merges then we'll need to rework this to match changes there + add the ability to provide the thread pool.

bscSCORM commented 10 years ago

actually, I didn't think about this being a problem the first time around, but that PR puts gzip compression on the request whereas HTTP defines how to negotiate it on the response only AFAIK there is no requirement for a server to support a gzip request , so it seems like the only safe way to do it is to try a small gzipp reqquest (mabye to about) , and then carry on with gzip only if that works regarding responses, cloud not supporting gzip shouldn't cause an error, as the client can say it supports gzip, but responding with gzip is optional @brian.miller: does that make sense to you?

brianjmiller commented 8 years ago

This has been left open pending looking at the thread handling, etc. I have new commits building on the Jetty 9 upgrade (which I believe gets rid of the thread pooling anyways) that will land that will allow a user to set their own HTTP client object so closing this.