datamapper / dm-types

DataMapper plugin providing extra data types
http://datamapper.org/
MIT License
55 stars 80 forks source link

dm-types Flag fails to persist values assigned with the left shift operator. #26

Open solnic opened 13 years ago

solnic commented 13 years ago

DataMapper::Types::Flag caches values assigned/appended with the left shift << operator, but fails to persist them on save().

I've created a gist to illustrate the problem.

I might also add that finders like Model.all(:flagprop => [:one, :two]) do not behave as (I) expected, but I really have no idea how they are supposed to work.

See also: #1128


Created by Austin Bales (at 417east) - 2009-12-21 05:26:02 UTC

Original Lighthouse ticket: http://datamapper.lighthouseapp.com/projects/20609/tickets/1161

solnic commented 13 years ago

<< fails to invoke dirty tracking so calling save fails to see any changes.

This raises a larger issue about how to handle methods on attributes which modify the receiver therefore bypassing dirty tracking.

3 approaches to handle this:

  1. Document this issue as an implementation detail for the user
  2. Override receiver modifying methods to invoke dirty tracking
  3. Freeze the attribute so it cannot be modified; the user has to reassign changes

I like approach 3. A simple fix that works across all kinds of properties and works with dirty tracking. It also makes it hard to accidentally change an attribute without reassigning it.

by Carl Porth

solnic commented 13 years ago

For example, you could do:

by Carl Porth

solnic commented 13 years ago

I see your reason for supporting approach 3, but it seems an unusual restriction to set (perhaps I’m uninformed). However, if the behavior is atypical, then it sort of presents a usability issue. Is it that approach two is too complex or costly? I’m by no means a Ruby expert, but bear with me...

Because the Flag type contains multiple values, and is represented as an array, it seems logical that I can push things into it. One of the reasons I appreciate DataMapper is that it does feel like "All Ruby All The Time", and things work naturally. I think this is especially important if #1128’s conclusion was that Flag should yield arrays.

by Austin Bales (at 417east)

solnic commented 13 years ago

I don’t think your example is atypical at all; consider the following case with calling capitalize! on a string attribute: http://gist.github.com/276969

Receiver modifying methods seem to be a weakness with the current implementation of dirty tracking. ActiveRecord addresses this by requiring you to call #{attribute}_will_change! before modifying the receiver.

Maybe there is a better pattern or ruby idiom to solve this?

by Carl Porth

solnic commented 13 years ago

I don’t love #attribute_will_change!, but what if you could set values to explicitly save.

p = Person.get(3)
p.first_name.capitalize!
p.visits << Visit.new()
p.save :first_name, :visits

by Austin Bales (at 417east)

solnic commented 13 years ago

This is going to be resolved in 1.x.x series as it requires changes in dm-core and we won’t make it for 1.0.0

by Piotr Solnica (solnic)

solnic commented 13 years ago

[bulk edit]

by Dan Kubb (dkubb)

jpr5 commented 13 years ago

Note that pull request 33 (https://github.com/datamapper/dm-types/pull/33) + adding the DirtyMinder concept to Flag would conceptually fix this.

dkubb commented 13 years ago

@jpr5's pull request was merged which solves this problem. Closing this ticket.

If the problem still persists please add a comment to this ticket, otherwise enjoy!

dkubb commented 13 years ago

Ok, my bad. That commit does not solve this issue yet. However the same lib can be used with this. @jpr5 has said he would look into it.

lsiden commented 12 years ago

Aaron Pfeiffer offered this temporary sollution in his post https://groups.google.com/d/msg/datamapper/JBkeBWxZUmI/H6nPD0kCWwcJ

I had to update it just a little but it's working for me now. For example:

  account.status << :email_confirmed
  account.taint! :status
  account.save
STDERR.puts account.saved? ? 'account is saved' : 'account was not saved'
account.reload
p account.status # ==> [ :email_confirmed ]

Maybe it will help someone else until they find a better way. I'm just getting my sea-legs with Ruby. This looks like a pretty challenging problem.