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

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

Open
irishshagua opened this issue Nov 16, 2016 · 7 comments
Open

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

irishshagua opened this issue Nov 16, 2016 · 7 comments
Labels

Comments

@irishshagua
Copy link

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

I added a PR here for this. It's a very small change:#58

@mkiedys
Copy link
Contributor

mkiedys commented Nov 18, 2016

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

@irishshagua
Copy link
Author

@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
Copy link
Author

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

@mkiedys
Copy link
Contributor

mkiedys commented Dec 6, 2016

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

@mkiedys mkiedys added the bug label Dec 6, 2016
@irishshagua
Copy link
Author

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

@mkiedys
Copy link
Contributor

mkiedys commented Feb 13, 2017

Merged.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

2 participants