ScalaConsultants / reactive-rabbit

Reactive Streams driver for AMQP protocol. Powered by RabbitMQ library.
Apache License 2.0
184 stars 40 forks source link

Parsing Messages fails if content-type is not recognised #57

Open irishshagua opened 7 years ago

irishshagua commented 7 years ago

If there is a content-type header in a Rabbit message which is not successfully parsed by Guavas MediaType class then an exception is thrown. Although this is an invalid content type I would think that maybe this should be logged as a warning and the contentType field in the Message class just be set to a None.

Am happy to try and put together a PR if people agree that this is an issue...

Sample Exception: com.rabbitmq.client.impl.DefaultExceptionHandler: Consumer QueueSubscription(channel=AMQChannel(amqp://xxx@x.x.x.x:5671/,4), queue=xxx, subscriber=akka.stream.impl.fusing.ActorGraphInterpreter$BoundarySubscriber@4fef0dc0, demand=16, buffer.size=0) (amq.ctag-zIlVYhaNIQ8bmDQCbe-ZUA) method handleDelivery for channel AMQChannel(amqp://xxx@x.x.x.x:5671/,4) threw an exception for channel AMQChannel(amqp://xxxx@x.x.x.x:5671/,4): java.lang.IllegalArgumentException: Could not parse 'application-json' at com.google.common.net.MediaType.parse(MediaType.java:656) at io.scalac.amqp.impl.Conversions$$anonfun$toMessage$1.apply(Conversions.scala:63) at io.scalac.amqp.impl.Conversions$$anonfun$toMessage$1.apply(Conversions.scala:63) at scala.Option.map(Option.scala:146) at io.scalac.amqp.impl.Conversions$.toMessage(Conversions.scala:63) at io.scalac.amqp.impl.Conversions$.toDelivery(Conversions.scala:80) at io.scalac.amqp.impl.QueueSubscription.handleDelivery(QueueSubscription.scala:44) at com.rabbitmq.client.impl.ConsumerDispatcher$5.run(ConsumerDispatcher.java:144) at com.rabbitmq.client.impl.ConsumerWorkService$WorkPoolRunnable.run(ConsumerWorkService.java:99) at java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1142) at java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:617) at java.lang.Thread.run(Thread.java:745) Caused by: java.lang.IllegalStateException at com.google.common.base.Preconditions.checkState(Preconditions.java:159) at com.google.common.net.MediaType$Tokenizer.consumeCharacter(MediaType.java:691) at com.google.common.net.MediaType.parse(MediaType.java:627) ... 11 more

irishshagua commented 7 years ago

I added a PR here for this. It's a very small change:https://github.com/ScalaConsultants/reactive-rabbit/pull/58

mkiedys commented 7 years ago

What do you think about changing type of Message.contentType from MediaType to String and removing dependency to Google Guava?

irishshagua commented 7 years ago

@mkiedys It would certainly make sense from my point of view. The reason I didn't do that in the PR was because I wasn't sure how backward compatible you wanted to keep the library.

irishshagua commented 7 years ago

@mkiedys Do you think the PR should be changed to make the content-type a String or is this PR good to merge?

mkiedys commented 7 years ago

Let's change it to String and get rid of Guava. This will be a breaking change.

irishshagua commented 7 years ago

Ok @mkiedys , PR updated. Couldn't remove the Guava lib as it's still used for ImmutableMap for building up properties.

mkiedys commented 7 years ago

Merged.