[MongoMapper] Why does MM uses public object API for unmarshalling?

[alexey.petrushin]

Hello, it's more theoretical question, in my case it causes some
problems, I can workaround it but it not seems to be the right way.

Here's the problem:

There's an object with :owner (username, String) and :viewers (array
of usernames) keys. It's supposed that viewers always should include
the object owner.
To implement it we'll override :owner= method, it'll check and add
object owner to list of viewers.
And it works fine, until you try to load previously saved object,
you'll then get two owner names in viewers instead just one.

Theoretically, persistence should return the same object as it took.
It shouldn't build object using public API.

Sure, we can easily workaround this, rename method, or use internal
state check in method body. But this workaround looks like a kind of
hack.
But maybe I'm missing something and there's a good reason to do it
this way?

Thanks, Alex.

PS here's the code responsible for this in MM (keys.rb)

if respond_to?(writer_method)
self.send(writer_method, value)
else
self[name.to_s] = value
end

to alter it behavior we just need to comment some lines.

--
You received this message because you are subscribed to the Google
Groups "MongoMapper" group.
For more options, visit this group at
http://groups.google.com/group/mongomapper?hl=en?hl=en

[John Nunemaker]

I don't understand what your point is. Can you paste some code/tests with something failing and how you think it should work? Thanks.

[alexey.petrushin]

require 'mongo_mapper'

describe "MM Spec" do
before :all do
MongoMapper.database = 'test'

class ::AnObject
include MongoMapper::Document

key :owner_name, String
key :viewers, Array, :default => lambda{[]}

def owner_name= owner_name
viewers.delete self.owner_name
viewers << owner_name
write_attribute :owner_name, owner_name
owner_name
end
end
end

after :all do
Object.send :remove_const, :AnObject if
Object.const_defined? :AnObject
end

it do
o = AnObject.new
o.owner_name = 'user'
o.save!

o = AnObject.first # don't use reload, it willn't catch this error
o.viewers.should == %w{user}
end
end

(the same on github http://github.com/alexeypetrushin/sm_commons/blob/master/spec/models/mm_spec.rb)

[John Nunemaker]

Ok, so you are wondering why MM calls the public writers when loading from the database? Why wouldn't it? 

What is the goal of owner_name= in your case? It seems like you are removing the current owner_name and then adding the new value. What are viewers? What is the end goal you are trying to accomplish? That will help me suggest the best way. 

A simple fix for what you are doing is to just uniq! viewers in that method. Also, you should use super instead of write_attribute if you are overriding a key accessor. write_attribute will set the stuff but doesn't play nicely with other key things like dirty tracking. super will play nice with everything. Only use write_attribute if you are in another method or something. 

[John Nunemaker]

Forgot to email code:

def owner_name=(value)

  viewers.delete self.owner_name

  viewers << value

  viewers.uniq!

  super

end

[John Nunemaker]

One more thing, Array keys always default to [] so there is no need for that. Hashes default to {} as well.

[alexey.petrushin]

Thanks, John,

list of viewers - list of user roles that are allowed to view this
object (in reality it looks like ['manager', 'member', 'user:alex']).
And then I can leverage db indexes for the authorization check.
In my app every object has custom access. There are millions of
objects, and user should view only that are allowed.

Yes uniq will solve the problem, my first thought was that it will be
called always, on every object load and maybe there's way to escape it.

[Stephen Eley]

On Thu, Apr 22, 2010 at 4:07 PM, alexey.petrushin
<alexey.p...@gmail.com> wrote:
>

> Theoretically, persistence should return the same object as it took.
> It shouldn't build object using public API.

"Should" according to what set of principles? The way it's
implemented in MongoMapper is in line with object-oriented practice:
if you're extending an object's existing behavior, it's not always
safe to assume that no one _else_ has extended it. So you make a
habit of sending messages and calling 'super' so that anything else
that's supposed to happen, happens without you needing to know about
all of it.

In the case of your requirement, I'd probably do the add-to-viewers
logic in MM with a 'before_save' callback instead of overriding the
assignment. It's the sort of thing callbacks are for in the Rails
idiom, and it keeps the assignment methods a little less cluttered
with side effects.



--
Have Fun,
Steve Eley (sfe...@gmail.com)
ESCAPE POD - The Science Fiction Podcast Magazine
http://www.escapepod.org

[Brandon Keepers]

I've actually gotten tripped up by this a lot too. In fact, I was just going to post to the list to start a conversation about this.

When I override setters, I'm usually doing so to make it easier for my controller to interact with the object, or to take params directly from a user. I don't usually do it thinking that Mongo will use it when loading the object from the database.

I would actually prefer to see MM handle this similar to how Active Record does, just setting the instance variables for the accessors to use.

Brandon

[Chris Hanks]

I'm in agreement with Brandon Keepers - I've been bitten by this too.

A similar issue that's gotten me is that code I place in the
"initialize" method of my models is run when returning records from
the db. I only expect that code to run when I instantiate an object
myself, but MongoMapper instantiates found objects using the "new"
class method so it runs there too.

ActiveRecord uses the "allocate" class method to get around this, see
more information here: http://paydrotalks.com/posts/89-rubys-classallocate-and-activerecordbasefind

It's certainly not a big problem, just throwing it out there for
discussion. Does anyone expect this behavior?

> For more options, visit this group athttp://groups.google.com/group/mongomapper?hl=en?hl=en

[alexey.petrushin]

Encounter the same problem in another situation. Here it is:

I'm using the markdown markup in a lots of places and I'm doing the
processing instantly when object is changed and save markup in the
cache attribute.

Here is pseudo code:

class Site
include MongoMapper::Document
plugin TextProcessor

markup_key :text
end

It creates two attributes: 'text', 'original_text', and every time you
do assign 'original_text' it processes it with markdown and also
assigns 'text'.

site.original_text = "**ruby**"

site.original_text ==> "**ruby**"
site.text ==> "<strong>ruby</strong>"

The interesting point - it almost always works (with performance lost,
but works), I noticed this problem only by accident.

I tried to override MongoMapper behavior (by overriding the
"attributes=" method) but it results in a lots of broken tests.

def attributes=(attrs)
return if attrs.blank?

attrs.each_pair do |name, value|
writer_method = "#{name}="

# if respond_to?(writer_method)
# self.send(writer_method, value)
# else
self[name.to_s] = value
# end
end
end

So I guess it is a wrong way :), but maybe there's another way to
alter it behaviour?
By owerriding I mean place the hack in the rails/initializers folder.

Thanks.

P.S.
- sample code with failed spec is here http://gist.github.com/387147
- the real-life code is here
http://github.com/alexeypetrushin/sm_commons/blob/master/lib/mongo_mapper/plugins/text_processor.rb
- and the app itself is here http://www.bom4.com (i.e. it's not my
theoretical fantasies, I'm really using it :) )

[Dhruva Sagar]

In your plugin, you could instead create keys for your original_test / text. In my understanding (which is limited) of what you are trying to achieve, I think your plugin should be creating keys, that way you won't have to override mongomapper's default behavior the way you have done.

--

Thanks & Regards,
Dhruva Sagar.

[alexey.petrushin]

> "Should" according to what set of principles? The way it's
> implemented in MongoMapper is in line with object-oriented practice:
> if you're extending an object's existing behavior, it's not always
> safe to assume that no one _else_ has extended it. So you make a
> habit of sending messages and calling 'super' so that anything else
> that's supposed to happen, happens without you needing to know about
> all of it.

As far as I understood, you mean that I called 'write_attribute'
instead of calling the 'super' method. Yes it's my bug (thanks to John
for pointing it out). But it's a side issue, it's not the point of
this discussion.
The point here is that MongoMapper is unmarshalling object such a way
that you can save an object in one state and later got it in another.

My point is that the persistence shouldn't be dealing with the
object's business logic.
For me it seems not very natural - if I save my pdf file, I suppose
that when later I load it it will be in the same state, no matter what
are written there. The success of the loading pdf file shouldn't
depend on it's content.

> In the case of your requirement, I'd probably do the add-to-viewers
> logic in MM with a 'before_save' callback instead of overriding the
> assignment. It's the sort of thing callbacks are for in the Rails
> idiom, and it keeps the assignment methods a little less cluttered
> with side effects.

Yes, it can be done with before save callback. But there is such thing
as invariant (object always should be in 'consistent' state). In case
of callback the object is in 'wrong' , 'inconsistent state' till you
save it.

[John Nunemaker]

I've updated master to just use write_key when loading from the database if the key exists.

github.com/jnunemaker/mongomapper/commit/f8969b2cb891cd0d4e9719b74973d8c107334aaa