wistia / nsq-ruby

NSQ Ruby client
MIT License
68 stars 27 forks source link

Add test for ephemeral topic discovery #30

Closed bschwartz closed 8 years ago

bschwartz commented 8 years ago

Fixes #27. Well, not exactly.

I went to write a failing test for ephemeral topic discovery but couldn't do it. It worked! The reason for this is that when we add unescaped characters to uri.query it will ultimately escape them when making the HTTP call:

uri1 = URI.parse 'https://1.2.3.4/blah'
uri1.query = 'this=that#thing'
uri2 = URI.parse 'https://1.2.3.4/blah'
uri2.query = 'this=that%23thing'
uri1 == uri2 #=> true

@mschneider82, maybe there was another reason it wasn't working for you? I'm all confused now :)

ghost commented 8 years ago

@bschwartz, thanks for your PR! By analyzing the annotation information on this pull request, we identified @freerobby, @alieander and @mmoutenot to be potential reviewers

mschneider82 commented 8 years ago

it fails for me without escaping. I am in the logstash environment that is using jruby:

URI::InvalidComponentError: bad component(expected query component): ts=1476776543&topic=testtest#ephemeral
                     check_query at /opt/logstash/vendor/jruby/lib/ruby/1.9/uri/generic.rb:895
                          query= at /opt/logstash/vendor/jruby/lib/ruby/1.9/uri/generic.rb:935
                       get_nsqds at /opt/logstash/vendor/bundle/jruby/1.9/gems/nsq-ruby-2.0.0/lib/nsq/discovery.rb:73
                 nsqds_for_topic at /opt/logstash/vendor/bundle/jruby/1.9/gems/nsq-ruby-2.0.0/lib/nsq/discovery.rb:44
  gather_nsqds_from_all_lookupds at /opt/logstash/vendor/bundle/jruby/1.9/gems/nsq-ruby-2.0.0/lib/nsq/discovery.rb:53
                             map at org/jruby/RubyArray.java:2414
  gather_nsqds_from_all_lookupds at /opt/logstash/vendor/bundle/jruby/1.9/gems/nsq-ruby-2.0.0/lib/nsq/discovery.rb:52
                 nsqds_for_topic at /opt/logstash/vendor/bundle/jruby/1.9/gems/nsq-ruby-2.0.0/lib/nsq/discovery.rb:43
              nsqds_from_lookupd at /opt/logstash/vendor/bundle/jruby/1.9/gems/nsq-ruby-2.0.0/lib/nsq/client_base.rb:58
             discover_repeatedly at /opt/logstash/vendor/bundle/jruby/1.9/gems/nsq-ruby-2.0.0/lib/nsq/client_base.rb:40
                            loop at org/jruby/RubyKernel.java:1479
             discover_repeatedly at /opt/logstash/vendor/bundle/jruby/1.9/gems/nsq-ruby-2.0.0/lib/nsq/client_base.rb:38

when i add URI.escape(topic) it works fine.

It seems a problem using the jruby uri library, generic.rb:

 882     def check_query(v)
 883       return v unless v
 884 
 885       # raise if both hier and opaque are not nil, because:
 886       # absoluteURI   = scheme ":" ( hier_part | opaque_part )
 887       # hier_part     = ( net_path | abs_path ) [ "?" query ]
 888       if @opaque
 889         raise InvalidURIError,
 890           "query conflicts with opaque"
 891       end
 892 
 893       if v && v != '' && parser.regexp[:QUERY] !~ v
 894           raise InvalidComponentError,
 895             "bad component(expected query component): #{v}"
 896       end
 897 
 898       return true
 899     end
 900     private :check_query

I really appreciate if you could do the escaping in nsq-ruby to be compatible with jruby. I know this is a special usecase but for logstash its every useful to use ephemeral topics if you just want to publish some logfiles to nsqd where you may dont have a consumer yet and doing the consumer also with logstash.

bschwartz commented 8 years ago

@mschneider82 Thanks for the detailed info. Just added the URI.escape back in. Pushing this up now.

bschwartz commented 8 years ago

@mschneider82 New gem pushed: https://rubygems.org/gems/nsq-ruby Thanks again for your help on this!