feat(datastore): support setting a property indexed value

[stephenplusplus]

By default, property index values are set to true, without allowing
the user to specify an override. Now, when a user passes in an array
to dataset.save, they will have the option of setting true or
false.

Example:

dataset.save({
  key: dataset.key('Company'),
  data: [
    name: 'propertyName',
    value: 'any value type',
    excludeFromIndexes: true
  ]
}, function(err, keys) {}).

Resolves: #208
Related: http://goo.gl/tKVvhP

[silvolu]

LGTM, other opinions on this change?

[pcostell]

Indexed is likely not the best way to describe this. In particular, its usage in many of the other APIs has caused confusion due to its interaction with composite indexes. What this flag really should indicate is that the specific value cannot be indexed (i.e. a blob or string that is too long). The Cloud Datastore API has changed this flag to "exclude_from_indexes" to make it more verbose. Something that is more readable could be something along the lines of "indexable".

[silvolu]

I think you're right.
Although more verbose, exclude_from_indexes makes the concept immediately clear, but I don't have any strong opinions.

[stephenplusplus]

To make sure I'm understanding, this PR would work the same way, we just want to change the term indexed to indexable?

If that's correct... is a user providing a 500+ char string and using indexable: false just a way of acknowledging they know the string can't be indexed, thus asking for the property to be committed with indexed: false? (I can't see why indexable is clearer than indexed in this case)

If that's not correct... would someone be able to write up a quick sample of what our api should look like and the effect that has on the resulting commit api call (if any)?

Please excuse my confusion!

Also, should we handle 500+ char strings that are sent in without indexable: false differently than we do currently? e.g. we wouldn't send the api request, instead throw an error telling the user if they want to save this, they must use the explicit indexed: false notation.

[silvolu]

The problem that emerges from the issue linked by @pcostell is that the name indexed is ambiguous.
I would think that if I set it to false for a certain property, the per-property index is not automatically created, but I can still manually define composite indexes that use the property; but this is not true, because setting it to false means that the property should not appear in any indexes.
So indexable is an attempt at making it more clear.

Does it make sense?

Regarding the throwing errors bit, I'd delegate that to the API backend and just make the call and report the error. Did you have a chance to check if the API error message is clear enough?

[stephenplusplus]

Thank you, I understand now. 💡 I'll update the PR later and get back with an answer on the API error message clarity.

[stephenplusplus]

PTAL.

[stephenplusplus]

The API error we pass lgtm:

{ [Error: The string property hi has a value that is too long. It cannot exceed 500 characters.]
     errors: [],
     code: 400,
     message: 'The string property hi has a value that is too long. It cannot exceed 500 characters.' } }

[pcostell]

Just to clarify a little when a user would want to use exclude_from_indexes:

1. As mentioned, if the value cannot be indexed i.e. the string is too long. Yes we could detect this case and just not index the data, but then when the user tries to query on it they wouldn't find that entity.
2. The data never needs to be queried. For example an encoded image (that's less than 500 bytes). Technically it's still indexable, but there isn't really a point to indexing it.

[stephenplusplus]

Thank you for helping me understand :)

I've updated the PR with your wording improvements and also renamed indexable to the more clear excludeFromIndexes. Please let me know if there's room for any more improvements.

[ryanseys]

Other than documentation notes, this LGTM.

[stephenplusplus]

PTAL