Issue 1422041: Support DataMapper 1.0 RC

David Masover

Ported dm-appengine to the DataMapper 1.0 RC. There's still work that could be done, but ...

15 years, 7 months ago (2010-05-31 07:34:01 UTC) #1

ribrdb

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? 0 is special cased here ...

15 years, 7 months ago (2010-06-01 22:06:41 UTC) #2

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?
0 is special cased here because it's a special case in the datastore (where it
means to auto-assign an id).

It's not safe to just specify arbitrary numbers as keys since they can collide
with automatically assigned ids.   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.
On 2010/05/31 07:34:02, David Masover wrote:
> I'm not sure why the old behavior special-cased a 0 key. Also, even with a
> Serial key, it should be possible for the user to override it with a specific
> integer value -- unless DataMapper was doing its own autoincrement logic?

http://codereview.appspot.com/1422041/diff/1/2#newcode105
dm-appengine/lib/appengine_adapter.rb:105: # I guess it's just not valid?
I'm not sure what's going on here.  Is DM caching the key before the object is
saved?

http://codereview.appspot.com/1422041/diff/1/4
File dm-appengine/lib/dm-appengine/properties.rb (right):

http://codereview.appspot.com/1422041/diff/1/4#newcode8
dm-appengine/lib/dm-appengine/properties.rb:8: ['string', [:AppEngineStringType,
:Blob, :ByteString, :Link, :Email, :Category, :PhoneNumber, :PostalAddress]],
Please wrap at 80 columns.

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?
parent.getChild(kind(property), 0) should work here.

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
Can value be nil?

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.  
On 2010/05/31 07:34:02, David Masover wrote:
> Is to_a correct here? I think Java lists can duck type as arrays, to some
> extent. I remember there being some discussion about which is faster, but I
> don't remember the conclusion, but this is how it was before.

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
doesn't this need to be self.length = ?

http://codereview.appspot.com/1422041/diff/1/11
File dm-appengine/spec/dm-appengine_spec.rb (left):

http://codereview.appspot.com/1422041/diff/1/11#oldcode74
dm-appengine/spec/dm-appengine_spec.rb:74: AppEngine::Testing.install_test_env
On 2010/05/31 07:34:02, David Masover wrote:
> This no longer works, and I don't know what to replace it with.

It should be fine without it.

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/05/31 07:34:02, David Masover wrote:
> This chunk belongs in a separate patch.

What do you mean?

http://codereview.appspot.com/1422041/diff/1/11#newcode465
dm-appengine/spec/dm-appengine_spec.rb:465: end
Maybe we should add tests for the string lengths?

Sign in to reply to this message.

David Masover

I'll upload another patch tonight or tomorrow. I assume I should append it to this ...

15 years, 7 months ago (2010-06-01 23:42:01 UTC) #3

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 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.

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?

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.

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.

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.

Sign in to reply to this message.

ribrdb

On 2010/06/01 23:42:01, David Masover wrote: > I'll upload another patch tonight or tomorrow. I ...

15 years, 7 months ago (2010-06-03 17:38:36 UTC) #4

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.

Sign in to reply to this message.

David Masover

15 years, 7 months ago (2010-06-04 03:35:53 UTC) #5

On 2010/06/03 17:38:36, ribrdb wrote:
> On 2010/06/01 23:42:01, David Masover wrote:
> > 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.  

Actually, I'd vote for letting people specify arbitrary integers. It's not a
good idea, but it's also not much worse than manually specifying a value in an
autoincrement column on a normal SQL database, is it?

An example of a good reason for doing this might be migrating a bunch of data
from another app, with the current app disabled (or at least, disabled from
writes)

> 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)

Huh, alright.

> > 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.

I can't seem to create a new root object with an ID of 0 (without creating a new
throwaway entity first, which seems wasteful), but I can do this:

k = AppEngine::Datastore::Key.from_path 'Foo', 1
child = k.getChild 'Bar', 0
# Child currently has no-id-yet.
e = AppEngine::Datastore::Entity.new child
AppEngine::Datastore.put [e]
# Child now has an id, without even looking at the entity.

I don't know to what extent that can be relied on, but it is convenient.

> > > 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.

Oh, good point. We should.

Sign in to reply to this message.

Issue 1422041: Support DataMapper 1.0 RC

Patch Set 1 #

Patch Set 2 : Bumped DM dependency #

Messages