accodeing / fortnox-api

Gem that abstracts Fortnox's F3 API
GNU Lesser General Public License v3.0
9 stars 8 forks source link

Tiny confusing behavior when creating a new instance from another instance #234

Open marcus-allabrf opened 1 year ago

marcus-allabrf commented 1 year ago

This is a little bit confusing:

invoice1 = Fortnox::API::Model::Invoice.new(customer_number: 1)
invoice2 = invoice1.update(customer_number: '1')

puts invoice1 == invoice2 # true
puts invoice1.object_id == invoice2.object_id # true

invoice1 = Fortnox::API::Model::Invoice.new(customer_number: 1)
invoice2 = invoice1.update(customer_number: 1)

puts invoice1 == invoice2 # true
puts invoice1.object_id == invoice2.object_id # false (!)

I guess this is because how the update method works:

  def update(hash)
    old_attributes = to_hash
    new_attributes = old_attributes.merge(hash)

Here, customer_number has been converted to a String in old_attributes via it's type (Types::Sized::String[1024]) but customer_number in hash has the raw value (Integer).

d-Pixie commented 1 year ago

The object equality in invoice1 == invoice2 is on content (see lib/fortnox/api/models/base.rb:77-81) while comparing on object_id is akin to === in javascript. So you can have two different instances of an invoice in your code, loaded at different times for example, that are considered equal since they contain the same values.

You could even change a property on one of them, then change it back to the same value and they would be considered equal again. This is by design, so that part is ok 🙂

The second case you point out, where one has been coerced into a string while the other is still an integer, is also expected behavior. The raw input is whatever you give update and should reasonably match the type of the model (since that is the "truth"). If you choose to give it an integer the type of the value will differ, the test for equality will "fail" and you will get a new instance with the same value - leading to the case above.

We could fix this by checking the final new instance that we create (lib/fortnox/api/models/base.rb:73) for equality and discard it in favor of the original instance - as we do on line 68. But before we do, is this an actual problem for you, or merely an observation? 😄

marcus-allabrf commented 1 year ago

@d-Pixie Thanks for the clarification! 👍 Yes, it was more of an observation. 😄

My main point was just that the behavior is not obvious if you don't read the code carefully. And if you don't, that might give you unexpected results when you later use the returned instance from Base#update.

For example, this might surprise someone (in the first example, we compare Date with String on lib/fortnox/api/models/base.rb:68):

invoice1 = Fortnox::API::Model::Invoice.new(customer_number: 1, due_date: '2023-01-01')
invoice2 = Fortnox::API::Repository::Invoice.new.save(invoice1)
invoice3 = invoice2.update(due_date: '2023-01-01')
puts invoice3.saved? # false

invoice1 = Fortnox::API::Model::Invoice.new(customer_number: 1, city: 'Stockholm')
invoice2 = Fortnox::API::Repository::Invoice.new.save(invoice1)
invoice3 = invoice2.update(city: 'Stockholm')
puts invoice3.saved? # true