Datamapper DataMapper → 0.10.0

Comments and changes to this ticket

• 

  Martin Gamsjaeger (snusnu) June 3rd, 2009 @ 04:38 PM

  I added a patch to dm-core to my fork at

  http://is.gd/NvrG

  and a patch to dm-more to my fork at

  http://is.gd/Nvvv

  As mentioned in the commit messages I haven't yet found out the best way to spec that

• 

  Martin Gamsjaeger (snusnu) June 3rd, 2009 @ 04:49 PM

  I was too fast, thx namelessjon for pointing this out!

  http://is.gd/NvYd

• 

  Dan Kubb (dkubb) June 7th, 2009 @ 04:06 AM

  • State changed from “unconfirmed” to “accepted”
  • Assigned user set to “Dan Kubb (dkubb)”
• 

  Dan Kubb (dkubb) June 16th, 2009 @ 03:07 AM

  • Milestone set to “0.10.0”
• 

  Dan Kubb (dkubb) July 2nd, 2009 @ 01:53 PM

  • Assigned user changed from “Dan Kubb (dkubb)” to “Martin Kihlgren”

  Is it necessary to wrap update! in a chainable block? I'm not entirely sure it is.

  If you can provide some specs for this on the dm-validations side I will commit it.

• 

  Martin Kihlgren July 2nd, 2009 @ 03:58 PM

  This is very very old news - I thought it was closed ages ago. So you
  can just close it :)

• 

  Dan Kubb (dkubb) July 3rd, 2009 @ 12:24 PM

  • State changed from “accepted” to “resolved”
• 

  Dan Kubb (dkubb) July 3rd, 2009 @ 01:20 PM

  • Assigned user changed from “Martin Kihlgren” to “Martin Gamsjaeger (snusnu)”
  • State changed from “resolved” to “confirmed”
• 

  Martin Gamsjaeger (snusnu) July 3rd, 2009 @ 01:23 PM

  • State changed from “confirmed” to “accepted”

• You flagged this item as spam.
  

  mrship July 7th, 2009 @ 09:01 AM

  chainable do
    def update(attributes, context = :default)
      return false unless context.nil? || valid?(context)
      super(attributes)
    end  
  end
  

  Surely, the patch suggested actually performs the validations against the previous attributes, not the ones passed to the update call? As such, even for an invalid update on a previously valid object the update will get applied. Plus, from then on, it is impossible to update that object as it is in an invalid state!

  In the previous 0.9 release, update_attributes in dm-core (now deprecated for update in 0.10) set the attributes of the object and then called save, thus using the dm-validations save to return false for an invalid object.

  Upshot is, and per the spec in #952, how can you call get update to return false (and not persist any changes) when updating a valid resource with an invalid set of attributes?

• 

  Martin Kihlgren July 7th, 2009 @ 10:05 AM

  Oh. Sorry about my earlier statement. I must have gotten this ticket mixed up with some old shite I was working on ages ago. Just keep on disregarding my earlier comment... :)

• 

  Martin Gamsjaeger (snusnu) July 7th, 2009 @ 10:14 AM

  You are totally right that the above patch would validate the previous resource and thus even compromise the remainder of the operation. It's a really silly (and obvious) bug that would have been immediately caught, were there any specs for that already. I'm still wondering where to place such a spec though, given that the whole dm-validations specsuite calls CRUD related methods only 2 times in the uniqueness_validator_spec. I was thinking of maybe adding some spec file dedicated especially at the CRUD methods save(!) and update(!). What do you guys think?

  And more to the point, what would you think of doing this?

  chainable do
    def update(attributes, context = :default)
      self.attributes = attributes
      return false unless context.nil? || valid?(context)
      super(attributes)
    end  
  end
  

  That should make sure that the current resource gets validated with the newly given attributes, no?

  I was talking to Dan on IRC, and iirc he said that Resource#update won't go through Resource#save in 0.10 because of the way Resource#save is written (or more precisely Resource#save_self), which basically simply delegates to either #create or #update

• 

  Martin Gamsjaeger (snusnu) July 7th, 2009 @ 10:48 AM

  Looking at my patch again, changes from my above comment incorporated, I now do doubt that the source code comment I added in there, really holds.

  chainable do
    def update(attributes, context = :default)
      self.attributes = attributes
      # no context stack needed, update will only operate on one resource
      return false unless context.nil? || valid?(context)
      super(attributes)
    end  
  end
  

  Resource#attributes= explicitly supports assigning to relationships in addition to properties of the resource. This means that it's not safe to assume that #update will only operate on one resource. While I tend to think that at least a validation_context_stack like used in the overwritten Resource#save is needed to correctly pass down the validation contexts to all members of the object graph, I'm not really sure how the current #update implementation would handle updating the object graph in the correct order.

  What do you think?

• You flagged this item as spam.
  

  mrship July 8th, 2009 @ 06:00 AM

  I've resorted to setting the attributes directly and then calling save (the old behaviour of update_attributes) until this is resolved. I'm not sufficiently familiar with the DM code to comment on your changes above. One for dkubb methinks!

• 

  Martin Gamsjaeger (snusnu) July 9th, 2009 @ 01:12 PM

  For reference, #952 was duplicating this ticket but it contains a spec that might be useful nevertheless.

• 

  Dan Kubb (dkubb) July 9th, 2009 @ 01:15 PM

  • State changed from “accepted” to “duplicate”
• 

  Dan Kubb (dkubb) July 9th, 2009 @ 01:16 PM

  • State changed from “duplicate” to “accepted”

  Whoops. Didn't mean to flag this as a dupe.

• 

  Martin Gamsjaeger (snusnu) July 11th, 2009 @ 12:00 PM

  • Assigned user changed from “Martin Gamsjaeger (snusnu)” to “Dan Kubb (dkubb)”

  Dan, I assigned this back to you because of the possible changes to #update's implementation that you mentioned on IRC.

• 

  Dan Kubb (dkubb) July 11th, 2009 @ 01:26 PM

  @Ashley: That is a good question. At one time Resource#update just did the equivalent of resource.attributes = attributes; resource.save. When I was speccing it for 0.10 I didn't feel that was 100% correct, so I changed it so it still set the attributes, but uses Repository#update to update in the data store.

  Now that I've thought about this more I don't think either approach is correct. Consider the following example:

  user = User.get(1)
  user.name = 'Ashley Moran'
  
  # ...
  
  user.update(:admin => true)
  

  In the above example, should it save only the :admin => true flag, and not persist the name change, or should it update all the changed data in the resource. I'm leaning towards explicitness.. you asked for the admin flag to be updated, but nothing else, so I'll do that -- I figure if you wanted to persist the whole object you would use user.save.

  However, this presents another problem. What if we have some validation logic that says the name must be "Ashley Moran" for the admin to be true? So we persisted the admin change, but not the name change, so when we pull the resource back out user.valid? will be false.

  One thing I've always maintained with dm-core and dm-validations is that anything persisted can be assumed to be valid.. otherwise you have to load every association to check it's validity, and pretty soon you'd load every object in the graph into memory, which is totally unreasonable. This sort of breaks that rule and makes some the concept of association validity a bit ambiguous.

  To get around this problem inside Resource#update I could "stash" the dirty values somewhere resetting the object to it's persisted state, set the :admin => true, then validate, if valid save it, and then restore the dirty values. Doing this should be relatively trivial, but I'm still not 100% sure what side effects it will have. I think we need more discussion and talk out some of the issues that could arise.

  Another behavior I would like to add to Resource#update is for the following:

  user = User.new
  user.update(:name => 'Ashley Moran')
  

  So It would work on a new and saved resource, and what happens behind the scenes is an implementation detail we handle inside DM.

  Finally, the last behavior I would like to see Resource#update get is:

  user = User.get(1)
  user.update(:partner => Partner.get(1))
  

  So we'd have the ability to persist associations in the update call.

  None of these is particularly difficult to add. I think I could add them all in less than a couple of hours since most of the framework supports it, it's mostly just speccing the behavior and wiring it up -- the specs are critical though because once we define a behavior we need to have a spec in place to ensure it does not regress. I don't even like closing tickets unless there's a spec somewhere that verifies the behavior directly (I don't count indirect coverage as being proper verification).

• 

  Dan Kubb (dkubb) July 13th, 2009 @ 08:18 PM

  But, that doesn't change the fact we've found an extraordinary number of examples of undesirable behaviour - far, far more than you would expect in a release labelled 'RC'.

  I will be the first to admit that I did not expect most of the tickets you've submitted. However all of them are for unspecified behavior, and once the new behavior was demonstrated and fit with the API, DM was modified to match.

  There are literally tens of thousands of possible use cases for a library like DataMapper, and it would take several man-years of development to specify every possible combination. That's just for the core behavior -- factor in one or more of 30+ adapters and twice again as many plugins, multiple versions of each database/datastore, compiled drivers that need to work across Linux/OSX/Solaris/FreeBSD/Windows, compatibility with MRI 1.8.6, 1.8.7, 1.9.1, and JRuby 1.3.1, and support for all major web frameworks and you'll begin to scratch the surface of the different permutations we need to keep in mind when developing DM.

  There are some days I wish DataMapper was just and ORM that worked with only a set of relational databases, but then I quickly realize it wouldn't be half as interesting.I believe that it's a pretty commonly held view that normal ORMs are not even close to being a solved problem, so I wouldn't expect a more abstract mapper like DM to be either. We are still evolving, and as the project progresses we are finding better and better abstractions for problems, which is why we've held off going 1.0 for so long.

  I will offer as examples both FreeBSD and - even more so - PostgreSQL, as projects with model release processes. My current experience of DataMapper is way below these.

  While I can surely appreciate your passion, but I do not think it's fair to compare DataMapper against those projects. Given equivalent resources and time I have no doubt that we could deliver a significantly better experience.

  It costs my clients a significant amount every day I am with them to isolate and work around library bugs. At some point I have to question if the savings from the library outweigh the cost of finding and compensating for these bugs.

  Right now, I would be very hard pushed to sell DataMapper on the grounds that it saves more time than it costs in working around bugs.

  I've heard you mention this a few times on twitter, the mailing list and I've held back any response because I didn't think it would add anything constructive, and might come across the wrong way. However, I have to question why you ever thought using pre-release software, that had just undergone a half-year long rewrite, and has not had any real world testing by the community as being something that would be a time saver?

  Neither I, nor anyone who is even remotely involved with DM has ever claimed 0.10 is more stable than it is. In fact if you'll see in the RC statement that I asked for people to run DM against their existing specs and report issues There is a presupposition that there will be issues and I asked people to report them.

  It's true that 0.10 solves a number of long standing issues, but more than half of DM had to be rewritten to resolve the underlying design that cause those issues. In fact there are more than twice as many commits in the rewrite as there were since the beginning of the DM project up to the start of the rewrite.

  Code was only rewritten after being specced and shown to have 100% coverage, but specs don't catch everything, and certainly cannot exercise every conciveable state the library can exist in. Even 100% coverage is mostly meaningless, it only tells you what lines of code have 0 coverage, not when you've covered every possible state. That doesn't mean I don't try -- I've written thousands of specs to cover many common cases and am adding more as time goes on.

  @Ashley: I really appreciate you taking the time to post this, and I think you've outlined some valid concerns, but I am curious what you think the solutions are?

• 

  Dan Kubb (dkubb) July 23rd, 2009 @ 11:56 AM

  • Tag changed from “dm-core, dm-validations” to “dm-core, dm-validations, regression”
• 

  Dan Kubb (dkubb) August 18th, 2009 @ 02:28 AM

  • State changed from “accepted” to “resolved”

  (from [e3bb9f1ff0726e512e1f5476b9bfeecaa219de9e]) [dm-validations] Added specs for Resource#update w/validation context

  • Added Resource#update that handled the validation context
  • Removed unecessary Model#create! method
  • Updated the default_validation_context to check the context stack before returning the default label to use, in case we are nested inside another context.

  [#882 state:resolved] http://github.com/datamapper/dm-more/commit/e3bb9f1ff0726e512e1f547...