Questions about a couple design decisions
2 views
Skip to first unread message
psada...@gmail.com
Nov 25, 2007, 3:06:16 PM11/25/07
to DataMapper
I'm interested in helping out with data_mapper, so many things about
it feel more 'sane' to me than they do in AR. I just had a couple
questions about the way things are implemented currently.
1. The ActiveRecordImpersonation Module. I agree that supporting
similar methods to AR is a good idea, and putting them in a separate
module is also a good idea. However, I'm curious as to why critical
methods to data_mapper, such as #save, live in this module rather
than ::Base. Is `session.save(self)` the preferred way to be saving
objects to the database? In most of the DM specs, `record.save` seems
to be the way its done. So why not just implement #save (and others)
in Base instead?
Also, the ARImpersonation Module implements methods like Person.all
and Person.first, which are not valid AR methods. It feels funny to
have them in a module that sounds like it is meant to impersonate AR.
Perhaps it would be better to implement these methods in ::Base, and
leave the Impersonation module for things like find_by_foo, and other
AR magic.
2. Using ruby exceptions rather than #save and #create returning true/
false. AR has methods #save! and #create! that raise an appropriate
exception, and the Rails 2.0 exception handling code is getting much
better. It also makes for simpler code, as in this merb controller
example:
# without exceptions
def update(id, person)
@person = Person[id]
raise NotFound unless @person
if @person.update_attributes(person)
redirect url(:person, @person)
else
render @person
end
end
# with exceptions
def update(id, person)
@person = Person[id]
@person.update_attributes(person)
redirect url(:person, @person)
end
# and in a nice DRY error-handling method, catch the RecordInvalid
# exception that was thrown by update_attributes, and render an
# XML-error document, or re-render the html form.
Rails 2.0 handles this exception handling automatically, and is easily
extensible, without having to do an if/else in every create & update
controller action.
I'm willing to write up some patches for this, but I wanted to ask the
list for their thoughts. #1 is a simple refactoring, and #2 could be
implemented the same way as AR, and have the normal methods return
true/false, and the same-named bang methods raise exceptions. I just
thought I would gather some opinions and ideas, and if the patches
would even be accepted if I went to the trouble of creating them. I do
hope this this will prompt some discussion on the design ideas as
DataMapper moves forward towards a stable release.
I also just wanted to say that what little I've used of DataMapper so
far is awesome. Thanks for the good work guys.
Paul Sadauskas
it feel more 'sane' to me than they do in AR. I just had a couple
questions about the way things are implemented currently.
1. The ActiveRecordImpersonation Module. I agree that supporting
similar methods to AR is a good idea, and putting them in a separate
module is also a good idea. However, I'm curious as to why critical
methods to data_mapper, such as #save, live in this module rather
than ::Base. Is `session.save(self)` the preferred way to be saving
objects to the database? In most of the DM specs, `record.save` seems
to be the way its done. So why not just implement #save (and others)
in Base instead?
Also, the ARImpersonation Module implements methods like Person.all
and Person.first, which are not valid AR methods. It feels funny to
have them in a module that sounds like it is meant to impersonate AR.
Perhaps it would be better to implement these methods in ::Base, and
leave the Impersonation module for things like find_by_foo, and other
AR magic.
2. Using ruby exceptions rather than #save and #create returning true/
false. AR has methods #save! and #create! that raise an appropriate
exception, and the Rails 2.0 exception handling code is getting much
better. It also makes for simpler code, as in this merb controller
example:
# without exceptions
def update(id, person)
@person = Person[id]
raise NotFound unless @person
if @person.update_attributes(person)
redirect url(:person, @person)
else
render @person
end
end
# with exceptions
def update(id, person)
@person = Person[id]
@person.update_attributes(person)
redirect url(:person, @person)
end
# and in a nice DRY error-handling method, catch the RecordInvalid
# exception that was thrown by update_attributes, and render an
# XML-error document, or re-render the html form.
Rails 2.0 handles this exception handling automatically, and is easily
extensible, without having to do an if/else in every create & update
controller action.
I'm willing to write up some patches for this, but I wanted to ask the
list for their thoughts. #1 is a simple refactoring, and #2 could be
implemented the same way as AR, and have the normal methods return
true/false, and the same-named bang methods raise exceptions. I just
thought I would gather some opinions and ideas, and if the patches
would even be accepted if I went to the trouble of creating them. I do
hope this this will prompt some discussion on the design ideas as
DataMapper moves forward towards a stable release.
I also just wanted to say that what little I've used of DataMapper so
far is awesome. Thanks for the good work guys.
Paul Sadauskas
Nick Plante
Nov 25, 2007, 3:14:57 PM11/25/07
to DataMapper
I agree with pretty much everything Paul says here.
I just created the AR impersonation specs last night while I was fixing
the find_or_create bug, and noticed the lack of specs, so I created them.
The fact that save() is in there weirded me out as well. Is there a
specific reason for this? Otherwise I'd also prefer to see that (and the
other non-AR-ish stuff) moved into Base.
Sam, I know you mentioned wanting to keep Base small, but save seems to
certainly belong there unless I'm missing something. The all and first and
such methods could possibly be moved to another include'd module?
I also agree with the exception-raising variants. I want to say we have
some tickets in lighthouse that mention that, but I'm much too lazy to
look at the moment :)
Best,
..nap
psada...@gmail.com
Nov 25, 2007, 3:32:26 PM11/25/07
to DataMapper
I searched, and didn't find any tickets related to raising exceptions.
The closest I saw was this one, from Sam:
http://wm.lighthouseapp.com/projects/4819/tickets/54-save-should-return-success
On Nov 25, 1:14 pm, Nick Plante <n...@zerosum.org> wrote:
> I agree with pretty much everything Paul says here.
>
> I just created the AR impersonation specs last night while I was fixing
> the find_or_create bug, and noticed the lack of specs, so I created them.
> The fact that save() is in there weirded me out as well. Is there a
> specific reason for this? Otherwise I'd also prefer to see that (and the
> other non-AR-ish stuff) moved into Base.
>
> Sam, I know you mentioned wanting to keep Base small, but save seems to
> certainly belong there unless I'm missing something. The all and first and
> such methods could possibly be moved to another include'd module?
>
> I also agree with the exception-raising variants. I want to say we have
> some tickets in lighthouse that mention that, but I'm much too lazy to
> look at the moment :)
>
> Best,
>
> ..nap
>
The closest I saw was this one, from Sam:
http://wm.lighthouseapp.com/projects/4819/tickets/54-save-should-return-success
On Nov 25, 1:14 pm, Nick Plante <n...@zerosum.org> wrote:
> I agree with pretty much everything Paul says here.
>
> I just created the AR impersonation specs last night while I was fixing
> the find_or_create bug, and noticed the lack of specs, so I created them.
> The fact that save() is in there weirded me out as well. Is there a
> specific reason for this? Otherwise I'd also prefer to see that (and the
> other non-AR-ish stuff) moved into Base.
>
> Sam, I know you mentioned wanting to keep Base small, but save seems to
> certainly belong there unless I'm missing something. The all and first and
> such methods could possibly be moved to another include'd module?
>
> I also agree with the exception-raising variants. I want to say we have
> some tickets in lighthouse that mention that, but I'm much too lazy to
> look at the moment :)
>
> Best,
>
> ..nap
>
Nick Plante
Nov 25, 2007, 7:28:04 PM11/25/07
to DataMapper
This is what I was thinking of. Not the same, but does acknowledge that
we're cognizant of the need for ! to raise. WIP.
we're cognizant of the need for ! to raise. WIP.
http://wm.lighthouseapp.com/projects/4819/tickets/29-dm-context-write
Any other comments on the original email? Any reason NOT to refactor AR
Impersonation?
..nap
Sam Smoot
Nov 26, 2007, 3:13:06 AM11/26/07
to DataMapper
This is a pretty interesting conversation. So I guess there's three
things to address:
1) ActiveRecordImpersonation: It's not really very clear, and not
really the original intent, but as it stands now this module is about
ActiveRecord the pattern, not the library. #save, #all, #first, #find
etc are all methods you'd expect to see in an ActiveRecord. Not a Data-
Mapper. But they're convenient. And since the module isn't really
serving the original intent anymore, I agree, they should probably
move into DM::Base.
2) Exceptions. There's been some discussion around this before. From a
pragmatic point-of-view, I think I'm about ready to side with you
guys. "!" methods should raise, because that's what we're most
familiar with. Sometimes convenient trumps "right".
I do think Rails 2.0's use of them is wrong though. Exceptions should
not be used for flow-control. It's a sure-fire way to cut your
application performance in half (at least). It's a terrible idea. The
whole Model.find; update_attributes; redirect_to pattern is a pattern.
It can be easily factored out into a CrudController or something. Or a
mixin. Then the 3 lines becomes one just to declare the type. Or even
inferred from the controller name. So the Rails style exceptions are
not only glacially slow, but they're actually a lot more verbose.
So I feel pretty strongly that the default assumption should be that
we return some sort of status instead of raising exceptions for
situations that aren't actually exceptional (like not finding an id,
or an attribute change that results in an invalid state). So #save!
RecordInvalidError yes. #find(nil) resulting in RecordNotFoundError
no.
3) Moving the meat of #save, etc into DM::Base. So the reason
Base#save is just a simple delegate to Context#save is that gives us
more flexibility. It'll make transactions easier to implement. Or auto-
flushing contexts. Or batch updates. Or non-SQL adapters. So keeping
the actual implementation of these out of DM::Base makes a lot of
things easier. It comes back down to the differences between a
DataMapper and an ActiveRecord I suppose.
things to address:
1) ActiveRecordImpersonation: It's not really very clear, and not
really the original intent, but as it stands now this module is about
ActiveRecord the pattern, not the library. #save, #all, #first, #find
etc are all methods you'd expect to see in an ActiveRecord. Not a Data-
Mapper. But they're convenient. And since the module isn't really
serving the original intent anymore, I agree, they should probably
move into DM::Base.
2) Exceptions. There's been some discussion around this before. From a
pragmatic point-of-view, I think I'm about ready to side with you
guys. "!" methods should raise, because that's what we're most
familiar with. Sometimes convenient trumps "right".
I do think Rails 2.0's use of them is wrong though. Exceptions should
not be used for flow-control. It's a sure-fire way to cut your
application performance in half (at least). It's a terrible idea. The
whole Model.find; update_attributes; redirect_to pattern is a pattern.
It can be easily factored out into a CrudController or something. Or a
mixin. Then the 3 lines becomes one just to declare the type. Or even
inferred from the controller name. So the Rails style exceptions are
not only glacially slow, but they're actually a lot more verbose.
So I feel pretty strongly that the default assumption should be that
we return some sort of status instead of raising exceptions for
situations that aren't actually exceptional (like not finding an id,
or an attribute change that results in an invalid state). So #save!
RecordInvalidError yes. #find(nil) resulting in RecordNotFoundError
no.
3) Moving the meat of #save, etc into DM::Base. So the reason
Base#save is just a simple delegate to Context#save is that gives us
more flexibility. It'll make transactions easier to implement. Or auto-
flushing contexts. Or batch updates. Or non-SQL adapters. So keeping
the actual implementation of these out of DM::Base makes a lot of
things easier. It comes back down to the differences between a
DataMapper and an ActiveRecord I suppose.
psada...@gmail.com
Nov 26, 2007, 5:47:09 PM11/26/07
to DataMapper
1 & 3) Ok, I'll write up a simple patch to move these to Base this
evening.
2) What does the speed of Rails exception handing have to do with the
behavior of DataMapper's interface? I think DataMapper should do the
"right thing", and let Rails/Merb/whatever worry about how to deal
with the exceptions. In this case, raising exceptions would be the
right thing to do. (I actually think #save should raise the exception,
and not even bother with #save!, but I understand the desire for AR
compatibility.) Checking return values to see if something succeeded
feels very old-school C/C++, and not the ruby way of doing things.
Also, I'm wondering how you got your 'half' number for app
performance. From here ( http://www.notsostupid.com/blog/2006/08/31/the-price-of-a-rescue/
) it looks like rescues are only a tiny bit slower than not having
them. And his 'plain' benchmark is even missing the conditional at the
beginning to see if 5 == 0 before doing the divide. I modified it a
tiny bit, and posted the results on my blog (its easier to read code
there: http://www.theamazingrando.com/blog/2007/11/26/ruby-rescues-are-not-slow/
). Maybe rails is doing it stupidly (which is entirely possible,
there's many other things they do stupidly), but it would seem its not
the fault of ruby exception handling.
My vote, if it counts, would be to use exceptions whenever possible.
#save & #update_attributes raises DataMapperError::RecordInvalid. #[]
should raise DME::RecordNotFound. #find(id) or #find(id1, id2) should
return nil or empty set, because that implies you are 'searching'
rather attempting to retrieve a known record.
Exceptions might not make as much sense for html errors, where you
need to re-render, and give the user a chance to fix it in the form,
but they make great sense for web services, where you want to just
render an error document and be done with it.
evening.
2) What does the speed of Rails exception handing have to do with the
behavior of DataMapper's interface? I think DataMapper should do the
"right thing", and let Rails/Merb/whatever worry about how to deal
with the exceptions. In this case, raising exceptions would be the
right thing to do. (I actually think #save should raise the exception,
and not even bother with #save!, but I understand the desire for AR
compatibility.) Checking return values to see if something succeeded
feels very old-school C/C++, and not the ruby way of doing things.
Also, I'm wondering how you got your 'half' number for app
performance. From here ( http://www.notsostupid.com/blog/2006/08/31/the-price-of-a-rescue/
) it looks like rescues are only a tiny bit slower than not having
them. And his 'plain' benchmark is even missing the conditional at the
beginning to see if 5 == 0 before doing the divide. I modified it a
tiny bit, and posted the results on my blog (its easier to read code
there: http://www.theamazingrando.com/blog/2007/11/26/ruby-rescues-are-not-slow/
). Maybe rails is doing it stupidly (which is entirely possible,
there's many other things they do stupidly), but it would seem its not
the fault of ruby exception handling.
My vote, if it counts, would be to use exceptions whenever possible.
#save & #update_attributes raises DataMapperError::RecordInvalid. #[]
should raise DME::RecordNotFound. #find(id) or #find(id1, id2) should
return nil or empty set, because that implies you are 'searching'
rather attempting to retrieve a known record.
Exceptions might not make as much sense for html errors, where you
need to re-render, and give the user a chance to fix it in the form,
but they make great sense for web services, where you want to just
render an error document and be done with it.
Peter Williams
Nov 26, 2007, 6:04:15 PM11/26/07
to DataMapper
On Nov 26, 3:47 pm, "psadaus...@gmail.com" <psadaus...@gmail.com>
wrote:
to perform the requested operation it should raise an exception. If
there is a feeling that status returns might be occasionally
convenient, a `#save_with_armor` method could be provided the save
functionality with boolean result. However, I think it is pretty rare
that a status return is actually more useful than a simple exception
raise.
Peter
wrote:
> My vote, if it counts, would be to use exceptions whenever possible.
> #save & #update_attributes raises DataMapperError::RecordInvalid. #[]
> should raise DME::RecordNotFound. #find(id) or #find(id1, id2) should
> return nil or empty set, because that implies you are 'searching'
> rather attempting to retrieve a known record.
I agree with Paul about raising exceptions. If the object is not able
> #save & #update_attributes raises DataMapperError::RecordInvalid. #[]
> should raise DME::RecordNotFound. #find(id) or #find(id1, id2) should
> return nil or empty set, because that implies you are 'searching'
> rather attempting to retrieve a known record.
to perform the requested operation it should raise an exception. If
there is a feeling that status returns might be occasionally
convenient, a `#save_with_armor` method could be provided the save
functionality with boolean result. However, I think it is pretty rare
that a status return is actually more useful than a simple exception
raise.
Peter
Sam Smoot
Nov 26, 2007, 6:18:36 PM11/26/07
to DataMapper
Paul,
The benchmark is flawed. It doesn't actually raise errors. I commented
on the original blog with a working example. The exception raising
code is well over 100 times slower.
Exceptions should be reserved for exceptional situations, and these
aren't. I agree we should have versions that do raise errors, but the
defaults should simply return statuses IMO.
Just to satisfy your curiosity, my "half" figure was completely made
up. :-) Just a guesstimate. In the context of a ~200/request-per-
second Merb app I'm guessing it stands up ok, but I'm to lazy to test
it right now. ;-)
Sorry to pull rank. I don't like to do that. I'll happily accept any
patches you'd like to submit regarding error-raising variants. But I
feel pretty strongly about this one.
If you really want to use some alternative form of flow-control, maybe
we could look into continuations? The same ability to "inject" code
when a certain state occurs, but without the performance penalty (at
least that's my assumption).
I'd also be happy to accept a Errorful plugin/patch that overwrites
the default methods with their error-raising brethren. So you can
still eat your cake if you're willing to require 'plugins/
error_happy'. That seems like a decent compromise?
On Nov 26, 4:47 pm, "psadaus...@gmail.com" <psadaus...@gmail.com>
wrote:
The benchmark is flawed. It doesn't actually raise errors. I commented
on the original blog with a working example. The exception raising
code is well over 100 times slower.
Exceptions should be reserved for exceptional situations, and these
aren't. I agree we should have versions that do raise errors, but the
defaults should simply return statuses IMO.
Just to satisfy your curiosity, my "half" figure was completely made
up. :-) Just a guesstimate. In the context of a ~200/request-per-
second Merb app I'm guessing it stands up ok, but I'm to lazy to test
it right now. ;-)
Sorry to pull rank. I don't like to do that. I'll happily accept any
patches you'd like to submit regarding error-raising variants. But I
feel pretty strongly about this one.
If you really want to use some alternative form of flow-control, maybe
we could look into continuations? The same ability to "inject" code
when a certain state occurs, but without the performance penalty (at
least that's my assumption).
I'd also be happy to accept a Errorful plugin/patch that overwrites
the default methods with their error-raising brethren. So you can
still eat your cake if you're willing to require 'plugins/
error_happy'. That seems like a decent compromise?
On Nov 26, 4:47 pm, "psadaus...@gmail.com" <psadaus...@gmail.com>
wrote:
> 1 & 3) Ok, I'll write up a simple patch to move these to Base this
> evening.
>
> 2) What does the speed of Rails exception handing have to do with the
> behavior of DataMapper's interface? I think DataMapper should do the
> "right thing", and let Rails/Merb/whatever worry about how to deal
> with the exceptions. In this case, raising exceptions would be the
> right thing to do. (I actually think #save should raise the exception,
> and not even bother with #save!, but I understand the desire for AR
> compatibility.) Checking return values to see if something succeeded
> feels very old-school C/C++, and not the ruby way of doing things.
>
> Also, I'm wondering how you got your 'half' number for app
> performance. From here (http://www.notsostupid.com/blog/2006/08/31/the-price-of-a-rescue/
> evening.
>
> 2) What does the speed of Rails exception handing have to do with the
> behavior of DataMapper's interface? I think DataMapper should do the
> "right thing", and let Rails/Merb/whatever worry about how to deal
> with the exceptions. In this case, raising exceptions would be the
> right thing to do. (I actually think #save should raise the exception,
> and not even bother with #save!, but I understand the desire for AR
> compatibility.) Checking return values to see if something succeeded
> feels very old-school C/C++, and not the ruby way of doing things.
>
> Also, I'm wondering how you got your 'half' number for app
> ) it looks like rescues are only a tiny bit slower than not having
> them. And his 'plain' benchmark is even missing the conditional at the
> beginning to see if 5 == 0 before doing the divide. I modified it a
> tiny bit, and posted the results on my blog (its easier to read code
> there:http://www.theamazingrando.com/blog/2007/11/26/ruby-rescues-are-not-s...
> them. And his 'plain' benchmark is even missing the conditional at the
> beginning to see if 5 == 0 before doing the divide. I modified it a
> tiny bit, and posted the results on my blog (its easier to read code
Peter Williams
Nov 26, 2007, 7:07:44 PM11/26/07
to DataMapper
On Nov 26, 4:18 pm, Sam Smoot <ssm...@gmail.com> wrote:
> Paul,
>
> The benchmark is flawed. It doesn't actually raise errors. I commented
> on the original blog with a working example. The exception raising
> code is well over 100 times slower.
Fair enough. On the other hand, the additional performance cost you
> Paul,
>
> The benchmark is flawed. It doesn't actually raise errors. I commented
> on the original blog with a working example. The exception raising
> code is well over 100 times slower.
noticed is only an issue if something does not work as advertised and
you need to raise an exception. If a saving a model fails you are
likely to be in a situation where performance is the least of your
worries. And you can remove a conditional from the happy path by
using exceptions (since we are talking about microsecond level
performance issues).
I suspect the overhead of an exception raise would be way down in the
noise of handling any real HTTP request. To be fair, though, it
would, as you pointed out, really need to be tested in-situ to be
sure one way or the other.
> Exceptions should be reserved for exceptional situations, and these
> aren't. I agree we should have versions that do raise errors, but the
> defaults should simply return statuses IMO.
situations". I, however, consider being unable to insert/update a
record in the database a exceptional situation. After it almost
always works. I suppose there might be a few situations where the
application might consider that unexceptional but the library does not
have the context to determine when that is exactly.
> Sorry to pull rank. I don't like to do that. I'll happily accept any
> patches you'd like to submit regarding error-raising variants. But I
> feel pretty strongly about this one.
> If you really want to use some alternative form of flow-control, maybe
> we could look into continuations? The same ability to "inject" code
> when a certain state occurs, but without the performance penalty (at
> least that's my assumption).
want to get into using a mechanism with such short life span. It is
an interesting idea, though.
> I'd also be happy to accept a Errorful plugin/patch that overwrites
> the default methods with their error-raising brethren. So you can
> still eat your cake if you're willing to require 'plugins/
> error_happy'. That seems like a decent compromise?
probably just resort to the `*!` versions (assuming such variants
exist). There is a lot to be said for not confusing matters which is
primarily what overwriting methods as you propose would achieve, I
think.
Peter
psada...@gmail.com
Nov 26, 2007, 8:05:33 PM11/26/07
to DataMapper
On Nov 26, 5:07 pm, Peter Williams <pe...@barelyenough.org> wrote:
> > I'd also be happy to accept a Errorful plugin/patch that overwrites
> > the default methods with their error-raising brethren. So you can
> > still eat your cake if you're willing to require 'plugins/
> > error_happy'. That seems like a decent compromise?
>
> Not really. If you decide to continue to return status codes I would
> probably just resort to the `*!` versions (assuming such variants
> exist). There is a lot to be said for not confusing matters which is
> primarily what overwriting methods as you propose would achieve, I
> think.
patch with the bang methods this week. I'll try to come up with a way
to benchmark it, too, in a merb app, and see where it stands when
compared to the http overhead.
Paul
Lance
Nov 27, 2007, 1:08:46 PM11/27/07
to DataMapper
Anyone remember Twitter (http://dev.rubyonrails.org/ticket/8159)? I
figure there are three take-away lessons:
1) Tests lie when they're not complex (deep) enough, and complexity
(depth) is proportional to speed.
2) Generating exceptions is slow because of the backtrace generation.
That is, they're slow because they're designed for debugging
situations that shouldn't occur.
3) Unless an exception is raised many times per request, it won't have
any significant impact on speed.
Looks like fodder for both sides of the debate, the purists and the
pragmatists. :)
On Nov 26, 5:05 pm, "psadaus...@gmail.com" <psadaus...@gmail.com>
wrote:
figure there are three take-away lessons:
1) Tests lie when they're not complex (deep) enough, and complexity
(depth) is proportional to speed.
2) Generating exceptions is slow because of the backtrace generation.
That is, they're slow because they're designed for debugging
situations that shouldn't occur.
3) Unless an exception is raised many times per request, it won't have
any significant impact on speed.
Looks like fodder for both sides of the debate, the purists and the
pragmatists. :)
On Nov 26, 5:05 pm, "psadaus...@gmail.com" <psadaus...@gmail.com>
wrote:
Reply all
Reply to author
Forward
0 new messages