On 2010年06月01日 23:42:01, David Masover wrote:
> I'll upload another patch tonight or tomorrow. I assume I should append it to
> this issue.
> 
> http://codereview.appspot.com/1422041/diff/1/2
> File dm-appengine/lib/appengine_adapter.rb (right):
> 
> http://codereview.appspot.com/1422041/diff/1/2#newcode71
> dm-appengine/lib/appengine_adapter.rb:71: if key.nil? && keys.first.serial?
> On 2010年06月01日 22:06:41, ribrdb wrote:
> > 0 is special cased here because it's a special case in the datastore (where
it
> > means to auto-assign an id).
> 
> That's what I thought, I'm just not clear when this would be used. But it may
as
> well be kept, maybe something like:
> 
> if keys.first.serial? && (key.nil? || key == 0)
I think what I was trying to do was let you specify an Integer key instead of
serial and still get auto-assigned ids. That's probably not a good idea though.
 
 
> I realize it's not safe to specify arbitrary numbers, but that is the behavior
I
> get on sqlite, where it's also not really safe. If we really want to prevent
> people from doing this, we should raise an exception -- it seems like the old
> behavior was to arbitrarily ignore any user-specified integer in a serial
field.
Yeah, raising an exception is probably the right thing to do. 
> That's also why I was confused about the nil and the 0 check -- if the key is
a
> Serial, it doesn't matter what you assign it to, it's still going to be
> automatic.
> 
> > It is now possible to pre-allocate ids, but
> > this was written before that was available. I'm not sure what the correct
way
> > to expose that functionality would be though.
> 
> I'm not sure if it's quite the same, but I think it's possible to create a key
> (without specifying an ID) and pull the ID out of that?
No, it won't get an id until an entity is written to the datastore. You need to
use the allocateIds function:
http://code.google.com/appengine/docs/java/javadoc/com/google/appengine/api/d...,
java.lang.String, long)
> http://codereview.appspot.com/1422041/diff/1/2#newcode105
> dm-appengine/lib/appengine_adapter.rb:105: # I guess it's just not valid?
> On 2010年06月01日 22:06:41, ribrdb wrote:
> > I'm not sure what's going on here. Is DM caching the key before the object
is
> > saved?
> 
> Basically, yes.
> 
> DM is special-casing on serial and nil, when we want serial-like behavior in
our
> custom type, and we want it to support initial values other than nil. From
> dm-core/resource/state/transient.rb, line 71:
> 
> property.serial? && value.nil? || property.valid?(value)
> 
> That's part of a test for "validity" which is used to determine whether or not
> the save can proceed -- in other words, if I have property.valid? return false
> here, it won't save.
> 
> But from resource.rb, line 156:
> 
> # only memoize a valid key
> @_key = key if model_key.valid?(key)
> 
> Basically, what happens is that a serial key, if left null, will _not_ be
cached
> to @_key, but _will_ be considered "valid enough" to persist the object. I
can't
> give our Key property similar semantics unless I define it to be 'serial', and
> then, it only works for the nil case. It doesn't work for a Hash, say.
> 
> So instead, what I do is I say a key is "valid enough" if it's anything that's
> passed through to the model layer, which has to include a hash. If I do that,
> it'll be memoized to @_key at the first opportunity, and this breaks lots of
> stuff later on. If I don't do that, the record won't be saved at all.
> 
> One possible solution might be to pretend we're serial and support nil, and
use
> the partial keys (no-id-yet) for children.
> 
> http://codereview.appspot.com/1422041/diff/1/5
> File dm-appengine/lib/dm-appengine/properties/key.rb (right):
> 
> http://codereview.appspot.com/1422041/diff/1/5#newcode43
> dm-appengine/lib/dm-appengine/properties/key.rb:43: # TODO: is it sane to not
> typecast this?
> On 2010年06月01日 22:06:41, ribrdb wrote:
> > parent.getChild(kind(property), 0) should work here.
> 
> Cool. Last time I tried something similar, I got an autoassigned ID --
no-id-yet
> is new.
> 
> This could resolve the @_key issue.
I don't know that it will. I'm not sure, but I would guess that the entity gets
a different key object when it's written and this one might keep saying the id
is 0. Also there's a bug in the dev_appserver datastore using keys with id 0 so
this may not work locally until 1.3.5 is released.
> http://codereview.appspot.com/1422041/diff/1/6
> File dm-appengine/lib/dm-appengine/properties/list.rb (right):
> 
> http://codereview.appspot.com/1422041/diff/1/6#newcode11
> dm-appengine/lib/dm-appengine/properties/list.rb:11: value.to_a
> On 2010年06月01日 22:06:41, ribrdb wrote:
> > Can value be nil?
> 
> I assume it would be if the list property wasn't set, or was set to NULL.
> 
> > Just using the java collection might be a bit faster, but I think I used
.to_a
> > becuase it can behave in unexpected ways sometimes. 
> 
> The main reason I'd want to not do this here (it should at least be an option)
> is that it's not difficult for the user to override accessors, but this way, I
> actually can't get that extra speed.
Ok sounds good. Go ahead and just use value.
> http://codereview.appspot.com/1422041/diff/1/9
> File dm-appengine/lib/dm-appengine/properties/string.rb (right):
> 
> http://codereview.appspot.com/1422041/diff/1/9#newcode37
> dm-appengine/lib/dm-appengine/properties/string.rb:37: length = 1024 * 1024
> On 2010年06月01日 22:06:41, ribrdb wrote:
> > doesn't this need to be self.length = ?
> 
> Probably. I'll do that.
> 
> http://codereview.appspot.com/1422041/diff/1/11
> File dm-appengine/spec/dm-appengine_spec.rb (right):
> 
> http://codereview.appspot.com/1422041/diff/1/11#newcode72
> dm-appengine/spec/dm-appengine_spec.rb:72: class Person
> On 2010年06月01日 22:06:41, ribrdb wrote:
> > On 2010年05月31日 07:34:02, David Masover wrote:
> > > This chunk belongs in a separate patch.
> > 
> > What do you mean?
> 
> It's fine, it just doesn't have anything to do with the DM 1.0 port. It's all
> about the discriminators and single-table inheritance.
> 
> http://codereview.appspot.com/1422041/diff/1/11#newcode465
> dm-appengine/spec/dm-appengine_spec.rb:465: end
> On 2010年06月01日 22:06:41, ribrdb wrote:
> > Maybe we should add tests for the string lengths?
> 
> Maybe. I think we're OK for now, though -- the scope of this is supposed to be
a
> slight refactoring and upgrade to DM 1.0.
Right, I'm just worried that the refactoring might have broken the string
lengths.