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

Inconsistent field serialization #249

Open
liveh2o opened this issue Feb 24, 2015 · 1 comment
Open

Inconsistent field serialization #249

liveh2o opened this issue Feb 24, 2015 · 1 comment
Labels

Comments

@liveh2o
Copy link
Contributor

liveh2o commented Feb 24, 2015

I'm seeing some inconsistencies in how we handle field serialization for different field types, which seems wrong.

Given the following definition:

message Account {
  optional double balance = 1
  optional int64 balance_updated_at = 2  
  optional int64 transactions_count = 3
}

Protobuf gladly accepts a string for double fields, presumably because strings respond to to_f:

2.2.0 > a = Account.new
=> #<Account balance=0.0 transactions_count=0 balance_updated_at=0
2.2.0 > a.balance = "123"
=> 123.0 

However, it does not accept a string for integer fields, even though strings respond to to_i:

2.2.0 > a = Account.new
=> #<Account balance=0.0 transactions_count=0 balance_updated_at=0
2.2.0 > a.transactions_count = "1"
TypeError: Unacceptable value 1 for field transactions_count of type Protobuf::Field::Int64Field

Curiously, it will accept a time for integer fields, but does not convert it to an integer, even though it does so with strings on double fields:

2.2.0 > a = Account.new
=> #<Account balance=0.0 transactions_count=0 balance_updated_at=0
2.2.0 > a.balance_updated_at = Time.current
=> Tue, 24 Feb 2015 20:09:08 UTC +00:00

This, of course, becomes a problem once it attempts to encode the message:

2.2.0 > a = Account.new(:balance_updated_at => Time.current)
=> #<Account balance=0.0 transactions_count=0 balance_updated_at=Tue, 24 Feb 2015 20:09:08 UTC +00:00
2.2.0 > a.encode
NoMethodError: undefined method `&' for Tue, 24 Feb 2015 21:17:58 UTC +00:00:Time
  ...

There may actually be two issues here, but I wanted to document the inconsistent behavior so we can start discussion a possible solution. If we need to split this into two, I'm happy to do so.

@liveh2o liveh2o added the Bug label Feb 24, 2015
@localshred
Copy link
Contributor

Wow, that is shocking. Definitely should get some test cases in place for these.

abrandoned added a commit that referenced this issue Mar 1, 2015
  - should standardize how we coerced Integer/Float values in pb
  - need to coerce value in Varint to make sure it is in bounds
  - bounds should be inclusive
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