fix (webapi): sipi integration

[subotic]

• fixed: sending post parameters to sipi as FormData and not Json
• fixed: error message handling from sipi responses
• fixed: multipart upload

resolves #291
resolves #339

[subotic]

@tobiasschweizer The file upload should work now. Could you please check?

You can now also use the brand new integration tests:

• inside sbt simply run it:test. It will use GraphDB and SIPI.

There is currently only one test written (create page with binary).

[tobiasschweizer]

I will look at it, thx.

I adapted the error handling when parsing the Sipi error messages on the branch StandoffUtil. Maybe it would have been better to do that directly on this branch.

[tobiasschweizer]

This is very nice!

Could you tell me how to set up the Sipi info route?

[tobiasschweizer]

We should adapt sipi.knora-test-config.lua so we can use it in the integration tests.

Attention: sipi.knora-test-config.lua is already used in the browser tests, always serving the same file. Probably we should have two test config files.

[subotic]

Could you tell me how to set up the Sipi info route?

Ah, yes, sorry. Forgot about that. It's a copy of scripts/test1.lua I named info.lua and added to routes in config/sipi.knora-config.lua:

routes = {
   ...
   {
        method = 'GET',
        route = '/info',
        script = 'info.lua'
   }
}

[tobiasschweizer]

Lukas thinks that we should put the Sipi config files and Lua files in the Knora repository.

[subotic]

We can create a sipi folder in the root directory and then move everything to this folder.

[tobiasschweizer]

I think that would make sense. In that directory we should put:

• the config files to start Sipi with for 1) productive use, 2) browser tests and 3) integration tests (to be defined) (subfolder config):

  • sipi.knora-config.lua (requires sipi.init-knora.lua)
  • sipi.init-knora.lua (contains function pre_flight making the Knora roundtrip)
  • sipi.knora-test-config.lua (requires sipi.init-knora-test.lua)
  • sipi.init-knora-test.lua (contains function pre_flight that only serves a static file for browser tests)

• Lua routes and scripts (subfolder scripts):

  • convert_from_binaries.lua (non GUI-case file upload)
  • make_thumbnail.lua (GUI case file upload)
  • convert_from_file.lua (GUI case file upload)
  • get_media_type.lua (determine media type of a given file from magic number)
  • send_response.lua (handle successful responses and error messages to be sent to the client)
  • Knora_login.lua (called on login to Knora)
  • Knora_logout.lua (called in logout from Knora)

[benjamingeer]

If an InternalServerException is thrown during the processing of an integration test, the exception's stack trace should be logged to the console. I added a primitive test for this: ErrorV1ITSpec. It deliberately causes an InternalServerException. Can you please have a look at the logging configuration?

[subotic]

I have added the same test to our normal tests. The behaviour, i.e. output is the same.

With ErrorV1E2ESpec:

ERROR ActorSystemImpl - E2ETest-akka.actor.default-dispatcher-5 - akka.actor.ActorSystemImpl(E2ETest) - Error during processing of request: 'null'. Completing with 500 Internal Server Error response.
HttpResponse(500 Internal Server Error,List(Server: akka-http/10.0.0, Date: Wed, 21 Dec 2016 16:38:09 GMT),HttpEntity.Strict(text/plain; charset=UTF-8,There was an internal server error.),HttpProtocol(HTTP/1.1))

With ErrorV1ITSpec:

ERROR ActorSystemImpl - IntegrationTests-akka.actor.default-dispatcher-13 - akka.actor.ActorSystemImpl(IntegrationTests) - Error during processing of request: 'null'. Completing with 500 Internal Server Error response.
HttpResponse(500 Internal Server Error,List(Server: akka-http/10.0.0, Date: Wed, 21 Dec 2016 16:37:45 GMT),HttpEntity.Strict(text/plain; charset=UTF-8,There was an internal server error.),HttpProtocol(HTTP/1.1))

As far as I can tell, this error is thrown by Akka when we try to send null in here: https://github.com/dhlab-basel/Knora/blob/develop/webapi/src/main/scala/org/knora/webapi/util/ActorUtil.scala#L79

What would you like to see as an error message?

[benjamingeer]

I'm confused now. I don't understand why there would be a null. This line returns (), an instance of Unit, as a message:

3cc9d28#diff-13eb43a5343d8a8f59b7bd0a75fa4350R70

So I think we should get an UnexpectedMessgeException, thrown here:

https://github.com/dhlab-basel/Knora/blob/3cc9d28d68482de65db763a6500ca3f9e5c4df6d/webapi/src/main/scala/org/knora/webapi/routing/RouteUtilV1.scala#L75

And I think we should get the stack trace of that exception on the console.

[subotic]

It is not really null, but it is apparently also not something that akka will accept as a message. This error is thrown by Akka before the message arrives to RouteUtilV1, so that is why the UnexpectedMessgeException is never thrown.

[benjamingeer]

OK, then I guess future2Message should check what's in the Future before sending it to sender. It could just check for KnoraResponseV1, like RouteUtilV1 does.

[benjamingeer]

On second thought, I guess it can't check for KnoraResponseV1, because the triplestore messages don't extend that trait. I suppose we could make an even more abstract trait, KnoraMessageV1.

[subotic]

What about all those cases, where a native type like Option, Seq or whatever is sent between actors? This would mean, that we always need to package it inside a message, and then unpack it on the other side, just so that we can catch a very unlikely user error? Besides, there is already an error thrown. Maybe it is easier to catch it and throw a more visible error?

[benjamingeer]

OK. But accidentally writing a responder method that returns Future[Unit] is an easy mistake to make. Can we just handle this case by having future2Message send a Future.failed() containing an AssertionException?

[benjamingeer]

And would it help to put a try...catch around sender ! result?

[subotic]

OK. But accidentally writing a responder method that returns Future[Unit] is an easy mistake to make.

Only if it is directly put into the future2Message method. If only adding method calls to future2Message (like we always do), then there isn't a large chance to return a Future[Unit] from a method that has not Future[Unit] as the return type. The compile will catch this.

And would it help to put a try...catch around sender ! result?

I just did, but it didn't catch the exception. I thing that there must be a way to check result. My hope is that akka is doing it and thus trowing the error.

[subotic]

I will continue tomorrow. Enough for tonight.

[benjamingeer]

What happened was that we temporarily gave a method a return type of Future[Unit] just to get it to compile, and then forgot to change it.

I will continue tomorrow. Enough for tonight.

Sleep well and don't dream about exceptions! :)

[subotic]

This "fix" is catching this kind of error now. Not much inside the stack trace, but at least it is more visible.

[benjamingeer]

OK great! No need for new when constructing the case class MessageEmptyException.

If a responder returns a failed future containing an InternalServerException during an e2e test, do we also get a stack trace on the console?

[benjamingeer]

Also, since we have the type ReplyT at compile time, can we just check that type, rather than using getClass.getName?

[subotic]

I've refactored it a bit. This way, we can add additional message types that should not be used.

[benjamingeer]

That looks good to me, but I'd like to see if it's possible to do this at compile time. I'm on the train, let me get back to you in a couple of hours.

[subotic]

If a responder returns a failed future containing an InternalServerException during an e2e test, do we also get a stack trace on the console?

yep, we have (see latest commit)

I don't like that we have test code inside the live responders. We should move this out of there. I have created an issue for this: #359. But I will leave this for next year.

[benjamingeer]

I figured out where the null was coming from: when you run ErrorV1E2ESpec, the LoggingAdapter passed to KnoraExceptionHandler.apply() is null:

dfa030c#diff-b8a7a8584b59cc92476dc6235a5f7bb4R28

I don't know why, but I've run out of time to work on it. We can look at it in January.

So what was happening is that KnoraExceptionHandler got the UnexpectedMessageException and tried to log it, but this threw a NullPointerException, so the UnexpectedMessageException didn't get logged.

So it turns out that Akka is happy to accept a message of type Unit, so there's no need to check for it in future2Message.

I refactored future2Message a little (getting rid of try2Message which was never used directly), changed it so it doesn't try to recover from errors that a reasonable application should not try to catch, and rewrote its scaladoc comment.

I changed all the responders so if they get an unexpected message, they call a function that handles it in a consistent way.

[benjamingeer]

OK I think I've fixed this now. I don't understand why, but it was this line in E2ESpec and in ITSpec that caused KnoraService to get a null logger, thus causing the NullPointerException in KnoraExceptionHandler:

override protected val log: LoggingAdapter = akka.event.Logging(system, this.getClass)

Removing this line seems to make everything work as it should:

• If an InternalServerException is thrown in RouteUtilV1 (e.g. because it got an unexpected message from a responder), KnoraExceptionHandler logs it to the console.
• If an InternalServerException is thrown in a responder, it gets thrown to the responder's supervisor, and this causes it to be logged to the console. Then RouteUtilV1 gets it, and KnoraExceptionHandler logs it to the console again. Maybe we should look at the supervisor error-handling policy so we don't log the same exception twice.

[benjamingeer]

Instead of requiring routes to wrap all request messages in futures, I added a method RouteUtilV1.runJsonRouteWithFuture, which is only needed by two routes.

Please just remove the commented-out code in UsersResponderV1Spec, then I think you can merge this.

[benjamingeer]

Please remove code rather than commenting it out.

[subotic]

Didn't I remove this already?

[subotic]

Jup, it's red :-)

[subotic]

Cool, thanks. I thought that there was something amiss with the whole thing, since I tried to replicate the error by sending a () to a mock actor, which didn't work.

Why the logger is not instantiating, I have no idea. It's one of those things that will hopefully become apparent one day.