Segfault error if options is an OpenStruct

[JohnAtFenestra]

Description

Confirmed on Windows.

Documenting a problem and the resolution. My code wrapped options into an OpenStruct as part of loading from a configuration file. A Segmentation fault is generated when the following is invoked:

Example code

opts = OpenStruct.new({
    username: 'MyTestUser',
    password: 'mypassword',
    dataserver: 'EXAMPLE\SQLEXPRESS',
    database: 'MyTestDatabase'
})

client = TinyTds.new opts

Workaround

The external workaround is to ensure that opts is a hash:

client = TinyTds.new opts.to_h

Possible long term resolution

A fix could be injected into the main code, which would follow the rules of Duck Typing such that any opts could be passed in so long as it supports to_h:

module TinyTds
  class Client
...
    def initialize(opts = {})
      opts = opts.to_h # Ensure that opts is a hash
      if opts[:dataserver].to_s.empty? && opts[:host].to_s.empty?
        raise ArgumentError, 'missing :host option if no :dataserver given'
      end
...

[aharpervc]

Only problem I can think of with that approach is that defaults will now be applied to a copy of the input parameter, rather than mutating it. That means any code that relied on reading the updated opts after initializing a client instance would fail.

Also, I don't think that that "this parameter must be a real hash" is that awful of a requirement. Your workaround seems fine, so I don't know if it's worth taking on that burden inside of initialize. Otherwise we get into kind of a ridiculous scenario where "what if whatever you pass as opts doesn't implement to_h?" or "what if whatever you pass to opts implements to_h but returns something that's not a hash" which is endless.

However, all that aside, I'm not really opposed to this change. If you were going to write a PR, maybe the right balance is to store opts as an instance variable (accessible via attr_reader to anyone who was trying to access it after initialization).

[JohnAtFenestra]

"Only problem I can think of with that approach is that defaults will now be applied to a copy of the input parameter, rather than mutating it."

We'll have to agree to disagree, as I would consider this a strong advantage. I prefer the functional approach: the result of the function should be idempotent - never mutate an object passed to a function (except, in Ruby by using the ! indicator). On the other hand, exposing the mutated opts as an attribute of the object is a good idea; it makes clear to the consumer that client.options may be different than opts.

"Otherwise we get into kind of a ridiculous scenario where "what if whatever you pass as opts doesn't implement to_h?" or "what if whatever you pass to opts implements to_h but returns something that's not a hash" "

Use assertions to validate the contract: whatever comes in must respond to :to_h and to_h must return an object of type Hash. If either fails, return an error that tells the consumer explicitly what they did wrong.

  raise TinyTdsException.new("opts must respond to .to_h") unless opts.respond_to?(:to_h)
  opts = opts.to_h
  raise TinyTdsException.new("opts.to_h must return a Hash") unless opts.instance_of?(Hash)

Alternatively, we can enforce the contract that opts either must be a type Hash or we throw an appropriate exception notifying the consumer.

  raise TinyTdsException.new("opts must be an instance of Hash because it is passed to a C function") unless opts.instance_of?(Hash)

The problem I'm addressing is the seg fault; it took a while to track down. Either the method should obey Duck Typing or it should explicitly enforce the contract. My main point is that a seg fault isn't a useful exception message and all exceptions must be useful.

At least in my opinion. What do you think?