Differentiate between single- and multi- props.

Differentiate between single- and multi- props.

Fixes #58.

Change PropertyMap to accept either a single Property or a
PropertySlice, the former being a single Property and the latter
being a multi-property regardless of content size.

This will likely break API compatibility for packages that directly use
PropertyMap, which shouldn't be too many. Updating is not difficult:

If you want to have a single property, you can assign directly instead
of wrapping in a []Property:

  PropertyMap{"Foo": MkProperty("Bar")}

If you want to have a multi-valued Property, use a PropertySlice:

  PropertyMap{"Foo": PropertySlice{ds.MkProperty("Bar")}}

Note that datastore now differentiates between single-valued Properties
and multi-valued Properties. When dealing with structs, luci/gae doesn't
really care when loading, but will export them as multi- if they are backed
by a slice.

BUG=None
TEST=local

Committed: https://github.com/luci/gae/commit/c4fab9e7d5a1b17dc32257a21fde91696d6cb217

[dnj, 2016-09-16 03:51:05]

dnj@chromium.org changed reviewers:
+ iannucci@chromium.org

[dnj, 2016-09-16 03:51:05]

PTAL. I'm going to put some polish on, but it should be good enough for review.

[iannucci, 2016-09-16 06:50:39]

Description was changed from

==========
Differentiate between single- and multi- propes.

Fixes #58.

Change PropertyMap to accept either a single Property or a
PropertySlice, the former being a single Property and the latter
being a multi-property regardless of content size.

This will likely break API compatibility for packages that directly use
PropertyMap, which shouldn't be too many. Updating is not difficult:

If you want to have a single property, you can assign directly instead
of wrapping in a []Property:

  PropertyMap{"Foo": MkProperty("Bar")}

If you want to have a multi-valued Property, use a PropertySlice:

  PropertyMap{"Foo": PropertySlice{ds.MkProperty("Bar")}}

Note that datastore now differentiates between single-valued Properties
and multi-valued Properties. When dealing with structs, luci/gae doesn't
really care when loading, but will export them as multi- if they are backed
by a slice.

BUG=None
TEST=local
==========

to

==========
Differentiate between single- and multi- props.

Fixes #58.

Change PropertyMap to accept either a single Property or a
PropertySlice, the former being a single Property and the latter
being a multi-property regardless of content size.

This will likely break API compatibility for packages that directly use
PropertyMap, which shouldn't be too many. Updating is not difficult:

If you want to have a single property, you can assign directly instead
of wrapping in a []Property:

  PropertyMap{"Foo": MkProperty("Bar")}

If you want to have a multi-valued Property, use a PropertySlice:

  PropertyMap{"Foo": PropertySlice{ds.MkProperty("Bar")}}

Note that datastore now differentiates between single-valued Properties
and multi-valued Properties. When dealing with structs, luci/gae doesn't
really care when loading, but will export them as multi- if they are backed
by a slice.

BUG=None
TEST=local
==========

[iannucci, 2016-09-16 07:11:04]

lgtm, though I think we may need to update the in-memory model if 'multiple'
affects the sort order at all. That would be undocumented behavior, however.

https://chromiumcodereview.appspot.com/2342063003/diff/1/impl/memory/datastor...
File impl/memory/datastore_index_test.go (right):

https://chromiumcodereview.appspot.com/2342063003/diff/1/impl/memory/datastor...
impl/memory/datastore_index_test.go:41: {
question: now that we know that the datastore retains the 'multiple' data inside
the datastore itself, does that data affect the sort-order of the indexed
values? So if you have:

type Foo struct { Value int }
type Foo struct { Value []int }

And you store:

Foo{1}
Foo{[]int{1}}
Foo{2}
Foo{[]int{2}}

And then do

Query("Foo").Order("Value")

What order do they come back in?

https://chromiumcodereview.appspot.com/2342063003/diff/1/impl/prod/raw_datast...
File impl/prod/raw_datastore_type_converter.go (right):

https://chromiumcodereview.appspot.com/2342063003/diff/1/impl/prod/raw_datast...
impl/prod/raw_datastore_type_converter.go:172: return fmt.Errorf("mixed
Multiple/non-Multiple properties for %q", p.Name)
I don't think this is possible?

https://chromiumcodereview.appspot.com/2342063003/diff/1/service/datastore/pr...
File service/datastore/properties.go (right):

https://chromiumcodereview.appspot.com/2342063003/diff/1/service/datastore/pr...
service/datastore/properties.go:877: if err := ret.appendPropertyData(k, v); err
!= nil {
I think this just needs to be

  ret[k] = v.Clone()

now? The append was just a convenient way to copy the slice before. There's no
real 'append' happening.

https://chromiumcodereview.appspot.com/2342063003/diff/1/service/datastore/pr...
service/datastore/properties.go:927: func (pm PropertyMap) Slice(key string)
PropertySlice {
should we also have a `First(key string)`? Or Single?

[dnj, 2016-09-16 19:07:22]

https://chromiumcodereview.appspot.com/2342063003/diff/1/impl/memory/datastor...
File impl/memory/datastore_index_test.go (right):

https://chromiumcodereview.appspot.com/2342063003/diff/1/impl/memory/datastor...
impl/memory/datastore_index_test.go:41: {
On 2016/09/16 07:11:03, iannucci wrote:
> question: now that we know that the datastore retains the 'multiple' data
inside
> the datastore itself, does that data affect the sort-order of the indexed
> values? So if you have:
> 
> type Foo struct { Value int }
> type Foo struct { Value []int }
> 
> And you store:
> 
> Foo{1}
> Foo{[]int{1}}
> Foo{2}
> Foo{[]int{2}}
> 
> And then do
> 
> Query("Foo").Order("Value")
> 
> What order do they come back in?

Discussed, probably can't matter for queries.

https://chromiumcodereview.appspot.com/2342063003/diff/1/impl/prod/raw_datast...
File impl/prod/raw_datastore_type_converter.go (right):

https://chromiumcodereview.appspot.com/2342063003/diff/1/impl/prod/raw_datast...
impl/prod/raw_datastore_type_converter.go:172: return fmt.Errorf("mixed
Multiple/non-Multiple properties for %q", p.Name)
On 2016/09/16 07:11:03, iannucci wrote:
> I don't think this is possible?

It shouldn't be, but since we're sitting at an interface boundary and have to
type assert anyway, it seems reasonable to check.

https://chromiumcodereview.appspot.com/2342063003/diff/1/service/datastore/pr...
File service/datastore/properties.go (right):

https://chromiumcodereview.appspot.com/2342063003/diff/1/service/datastore/pr...
service/datastore/properties.go:863: pm[k] = v.Clone()
I'm going to write some code to clear out "pm" here so it doesn't have residual
properties. This seems like the right thing to do on Load.

https://chromiumcodereview.appspot.com/2342063003/diff/1/service/datastore/pr...
service/datastore/properties.go:877: if err := ret.appendPropertyData(k, v); err
!= nil {
On 2016/09/16 07:11:03, iannucci wrote:
> I think this just needs to be
> 
>   ret[k] = v.Clone()
> 
> now? The append was just a convenient way to copy the slice before. There's no
> real 'append' happening.

Oh good catch. Yes, correct.

https://chromiumcodereview.appspot.com/2342063003/diff/1/service/datastore/pr...
service/datastore/properties.go:927: func (pm PropertyMap) Slice(key string)
PropertySlice {
On 2016/09/16 07:11:03, iannucci wrote:
> should we also have a `First(key string)`? Or Single?

I thought about this. TBH I am of the opinion that "Slice" is for when you just
want values and you don't care. It doesn't lose any data.

Something like "Single" would actually potentially lose some data, unless it
panics if len != 1. Seems not worthwhile given that Slice is fairly easy to work
with.

[dnj, 2016-09-16 19:21:34]

https://chromiumcodereview.appspot.com/2342063003/diff/1/service/datastore/pr...
File service/datastore/properties.go (right):

https://chromiumcodereview.appspot.com/2342063003/diff/1/service/datastore/pr...
service/datastore/properties.go:863: pm[k] = v.Clone()
On 2016/09/16 19:07:22, dnj wrote:
> I'm going to write some code to clear out "pm" here so it doesn't have
residual
> properties. This seems like the right thing to do on Load.

Actually nm on this.

[dnj, 2016-09-16 19:44:25]

Description was changed from

==========
Differentiate between single- and multi- props.

Fixes #58.

Change PropertyMap to accept either a single Property or a
PropertySlice, the former being a single Property and the latter
being a multi-property regardless of content size.

This will likely break API compatibility for packages that directly use
PropertyMap, which shouldn't be too many. Updating is not difficult:

If you want to have a single property, you can assign directly instead
of wrapping in a []Property:

  PropertyMap{"Foo": MkProperty("Bar")}

If you want to have a multi-valued Property, use a PropertySlice:

  PropertyMap{"Foo": PropertySlice{ds.MkProperty("Bar")}}

Note that datastore now differentiates between single-valued Properties
and multi-valued Properties. When dealing with structs, luci/gae doesn't
really care when loading, but will export them as multi- if they are backed
by a slice.

BUG=None
TEST=local
==========

to

==========
Differentiate between single- and multi- props.

Fixes #58.

Change PropertyMap to accept either a single Property or a
PropertySlice, the former being a single Property and the latter
being a multi-property regardless of content size.

This will likely break API compatibility for packages that directly use
PropertyMap, which shouldn't be too many. Updating is not difficult:

If you want to have a single property, you can assign directly instead
of wrapping in a []Property:

  PropertyMap{"Foo": MkProperty("Bar")}

If you want to have a multi-valued Property, use a PropertySlice:

  PropertyMap{"Foo": PropertySlice{ds.MkProperty("Bar")}}

Note that datastore now differentiates between single-valued Properties
and multi-valued Properties. When dealing with structs, luci/gae doesn't
really care when loading, but will export them as multi- if they are backed
by a slice.

BUG=None
TEST=local
==========

[dnj, 2016-09-16 21:12:57]

The CQ bit was checked by dnj@chromium.org

[dnj, 2016-09-16 21:12:58]

The patchset sent to the CQ was uploaded after l-g-t-m from
iannucci@chromium.org
Link to the patchset: https://chromiumcodereview.appspot.com/2342063003/#ps40001
(title: "Slice is now always a clone. This is marginally worse performance, but
a much safer UI.")

[commit-bot: I haz the power, 2016-09-16 21:13:05]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-16 21:18:48]

Description was changed from

==========
Differentiate between single- and multi- props.

Fixes #58.

Change PropertyMap to accept either a single Property or a
PropertySlice, the former being a single Property and the latter
being a multi-property regardless of content size.

This will likely break API compatibility for packages that directly use
PropertyMap, which shouldn't be too many. Updating is not difficult:

If you want to have a single property, you can assign directly instead
of wrapping in a []Property:

  PropertyMap{"Foo": MkProperty("Bar")}

If you want to have a multi-valued Property, use a PropertySlice:

  PropertyMap{"Foo": PropertySlice{ds.MkProperty("Bar")}}

Note that datastore now differentiates between single-valued Properties
and multi-valued Properties. When dealing with structs, luci/gae doesn't
really care when loading, but will export them as multi- if they are backed
by a slice.

BUG=None
TEST=local
==========

to

==========
Differentiate between single- and multi- props.

Fixes #58.

Change PropertyMap to accept either a single Property or a
PropertySlice, the former being a single Property and the latter
being a multi-property regardless of content size.

This will likely break API compatibility for packages that directly use
PropertyMap, which shouldn't be too many. Updating is not difficult:

If you want to have a single property, you can assign directly instead
of wrapping in a []Property:

  PropertyMap{"Foo": MkProperty("Bar")}

If you want to have a multi-valued Property, use a PropertySlice:

  PropertyMap{"Foo": PropertySlice{ds.MkProperty("Bar")}}

Note that datastore now differentiates between single-valued Properties
and multi-valued Properties. When dealing with structs, luci/gae doesn't
really care when loading, but will export them as multi- if they are backed
by a slice.

BUG=None
TEST=local

Committed:
https://github.com/luci/gae/commit/c4fab9e7d5a1b17dc32257a21fde91696d6cb217
==========

[commit-bot: I haz the power, 2016-09-16 21:18:49]

Committed patchset #3 (id:40001) as
https://github.com/luci/gae/commit/c4fab9e7d5a1b17dc32257a21fde91696d6cb217