tmm1 / http_parser.rb

simple callback-based HTTP request/response parser
MIT License
200 stars 43 forks source link

Check value passed to set_header_value_type #63

Closed jcoglan closed 3 years ago

jcoglan commented 5 years ago

The details of the change are given in the commit message.

I'm requesting this change because a user of Faye is having trouble on JRuby 9.2. Faye depends on em-http-request, which uses http_parser.rb. On this line, it calls into set_header_value_type in the Java implementation:

https://github.com/igrigorik/em-http-request/blob/v1.1.5/lib/em-http/http_connection.rb#L115

This worked on JRuby 1.7 but not on 9.2, so this method always fails to accept the given argument, even if it should be valid, and this means we cannot make HTTP requests.

Here's the relevant Faye issue: https://github.com/faye/faye/issues/502

jcoglan commented 5 years ago

I'd also note that I cannot get the gem to compile; running rake gives me this error:

ext/ruby_http_parser/org/ruby_http_parser/RubyHttpParser.java:113: error: cannot find symbol
    this.settings.on_status = new HTTPDataCallback() {
                 ^
  symbol:   variable on_status
  location: variable settings of type ParserSettings
Note: ext/ruby_http_parser/org/ruby_http_parser/RubyHttpParser.java uses or overrides a deprecated API.

It looks like other builds on Travis have the same problem. If there's a way we can compile this gem ourselves, we can work around the bug affecting em-http-request until an official release of this gem arrives.

jcoglan commented 5 years ago

@headius forgive my Java ignorance but is there a way of creating a static final List from an array literal? Like, can I say private static final List<String> = ...? Or do I need to create the array from a literal and then create the List in another statement? I'm currently doing the following to statically declare a List:

    private static final String[] accepted = {"mixed", "arrays", "strings"};
    private static final List<String> valid = Arrays.asList(accepted);
jcoglan commented 5 years ago

I've ended up doing this -- if this is the idiomatic way to do it, I'll update my PR:

  private static final List<String> VALUE_TYPES = new ArrayList<String>(
    Arrays.asList("mixed", "arrays", "strings")
  );
ashie commented 3 years ago

I confirmed that it fixes broken tests like this:

    Fix broken tests like the following:

      1) HTTP::Parser should allow us to set the header value type
         Failure/Error: @parser.header_value_type = type

         ArgumentError:
           Invalid header value type
         # org/ruby_http_parser/RubyHttpParser.java:478:in `header_value_type='
         # ./spec/parser_spec.rb:36:in `block in <main>'
         # ./spec/parser_spec.rb:35:in `block in <main>'

Thanks for you contribution!

ashie commented 3 years ago

BTW original code should use String.equals() instead of !=, different String instances are never matched by == on Java.