Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

datastore: Support excludeFromIndexes for primitives in arrays. #2773

Closed

Conversation

beaulac
Copy link
Contributor

@beaulac beaulac commented Dec 12, 2017

Resolves #2615 .

This slightly modifies the index exclusion syntax discussed in #1916, which was merged in #2497.

Primitives in an array can now be excluded from indexes like this:

{
  key: key,
  data: {
      array: [
          'a string', // Excluded
          {name: 'string in entity'} // Not excluded
      ]
  },
  excludeFromIndexes: ['array[]']
}

Previously, excludeFromIndexes: ['array[]'] would have thrown a server-side error because it was interpreted as excluding the arrayValue itself from indexes, which is disallowed by the Datastore API.

This PR changes this behaviour: now, an excludeFromIndexes path ending with [] means that non-entity values in the array should be excluded.

To exclude properties of entities in arrays, the existing nested-property syntax (e.g. array[].name) is still used.

For example, to exclude both from indexes:

{
  key: key,
  data: {
      array: [
          'a string', // Excluded
          {name: 'string in entity'} // 'name' property is also excluded
      ]
  },
  excludeFromIndexes: ['array[]', 'array[].name']
}

Nested arrays remain unsupported. The following will still break, as it currently does:

{
  key: key,
  data: {
      array: [
          [1] // 'excludeFromEntities' will be set on the arrayValue, which breaks. 
      ]
  },
  excludeFromIndexes: ['array[]']
}

I wanted to change as little as possible of how entity properties are excluded, since there is no clear conclusion on how #2510 ("catch-all" index-exclusion) will be addressed. As such, I didn't want to redo the core implementation, but I realize that excludePathFromEntity is becoming rather convoluted. If you'd prefer, I'd be happy to extract it into something that's more manageable.

@googlebot googlebot added the cla: yes This human has signed the Contributor License Agreement. label Dec 12, 2017
@coveralls
Copy link

Coverage Status

Coverage remained the same at 100.0% when pulling b29308b on beaulac:datastore-exclude-array-primitives into 9b562af on GoogleCloudPlatform:master.

@googlebot googlebot added cla: no This human has *not* signed the Contributor License Agreement. and removed cla: yes This human has signed the Contributor License Agreement. labels Dec 14, 2017
@stephenplusplus
Copy link
Contributor

This looks good to me. I added the scenario into the system tests (the tests that run against the actual API).

Only problem now is that we're blocked for a short time while we are extracting the Datastore module into its own repo (soon to be https://github.com/googleapis/nodejs-datastore). After that, I'll port the PR over, and get it released ASAP.

Thank you for helping out!

@googleapis googleapis deleted a comment from googlebot Dec 14, 2017
@stephenplusplus stephenplusplus added api: datastore Issues related to the Datastore API. status: blocked Resolving the issue is dependent on other work. labels Dec 14, 2017
@coveralls
Copy link

Coverage Status

Coverage remained the same at 100.0% when pulling edb71c7 on beaulac:datastore-exclude-array-primitives into 9b562af on GoogleCloudPlatform:master.

@lostpebble
Copy link

Ah thank you @beaulac ! This has been a much wanted resolution.

@stephenplusplus
Copy link
Contributor

@beaulac The other repo is now open: https://github.com/googleapis/nodejs-datastore -- I plan to port your work this week, but if you're willing, feel free to beat me to it! :)

@stephenplusplus stephenplusplus added status: do not merge and removed status: blocked Resolving the issue is dependent on other work. labels Dec 18, 2017
@beaulac
Copy link
Contributor Author

beaulac commented Dec 18, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api: datastore Issues related to the Datastore API. cla: no This human has *not* signed the Contributor License Agreement.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants