solnic / virtus

[DISCONTINUED ] Attributes on Steroids for Plain Old Ruby Objects
MIT License
3.77k stars 229 forks source link

String type does not coerce Array of Strings #151

Closed postmodern closed 11 years ago

postmodern commented 11 years ago

I noticed that the String type fails to coerce Arrays of Strings into a single String.

class Command
  include Virtus
  attribute :name, String
end

command.name = ['foo', '--evil', '--bad']
command.name
# => ["foo", "--evil", "--bad"]
dkubb commented 11 years ago

@postmodern do you think it should? In general virtus is conservative in what it coerces. Unless the conversion is unambiguous, like coercing an Integer to a String, it passes through the value untouched. The idea is eventually to pair it with a validation lib that can see the value's primitive does not match the attribute so it can be flagged as invalid.

postmodern commented 11 years ago

I kind of think it should try to coerce the Array into a String, or raise an exception. Otherwise you're allowing the attributes to be polluted with unexpected Objects, until validations catch them.

mbj commented 11 years ago

If validations catch them. @solnic I thought an exception will be raised in 1.0 if input is uncoercable?

dkubb commented 11 years ago

Ahh right, good point. I had forgotten that we decided to change to raise exceptions when coercion hasn't occured.

@solnic do you think this stricter form of coercion will be ready before virtus 1.0? It seems like a really big change to make after 1.0 is released.

postmodern commented 11 years ago

Off topic, can I define a coercion rule for Array -> String using join(' ')?

dkubb commented 11 years ago

@postmodern in your local project, or as part of virtus? I think the answer to the second question would probably be no based on conversations @solnic and I have had (even as recently as this morning about this specific issue).

My feeling is that coercions should not result in a loss of information, and when possible they should be bijective. The #to_array coercion is already implemented as Array(value), so it would be odd to introduce something that splits on spaces. Plus if one of the strings included spaces it would also require escaping to handle, etc.

In your own app, it should be possible to extend it the coercions though.

postmodern commented 11 years ago

@dkubb local project of course. I'm writing a "Command Mapper" that uses Virtus to prevent option injection (a non-Collection option should always map to a single String).

dkubb commented 11 years ago

Somewhat OT:

@postmodern is the input structured so that you can define constraints stronger than "must be a string"? Like for example, if an option can be one of 3 separate values, you can have a type represent that, and then the coercions can throw away anything invalid.

FWIW the idea behind axiom-types and refactoring virtus to use it underneath is that these kinds of constraints can be expressed in the type itself. I don't yet know how much will be exposed in virtus, but I would love to see us move towards a system where user defined types are not only easy to define, but the norm. I would love it if we could move away from having simple primitives describe things within the system, and towards richer types that can constraint the values. I would much rather see this kind of thing:

class User
  include Virtus

  attribute :name, Name
end

In this example Name is a type that inherits from a String type, and then applies further constraints like "length must be greater than or equal to 3 characters", "length must be less than or equal to 50 characters" "must only include characters from set X", etc.

postmodern commented 11 years ago

The Command Mapper would use virtus to coerce arbitrary values into the desired types for the options (Float, etc). The Command Mapper would also check whether the option type was a Collection, and whether to combine the option value into one argument, or repeat the option for each element of the value.

["--option", "value1 value2"]

vs.

["--option", "value1", "--option", "value2"]

I have also been toying with the idea of using Embedded Values for complex options:

option :plugins, PluginFlags do |flags|
  flags.map { |name,value| "#{name}:#{value}" }
end
solnic commented 11 years ago

Coercion is no longer part of Virtus. I will add raising exceptions on unhandled exceptions to Coercible. I'm only not sure how to approach exceptions in Virtus. Maybe it should be configurable.

solnic commented 11 years ago

Closing since it's coercion-related. No longer belongs here. To solve this you can inject your own coercer.