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

State of "excludeFromIndexes" (datastore) #14

Closed
stephenplusplus opened this issue Dec 16, 2017 · 12 comments
Closed

State of "excludeFromIndexes" (datastore) #14

stephenplusplus opened this issue Dec 16, 2017 · 12 comments
Assignees
Labels
api: datastore Issues related to the googleapis/nodejs-datastore API. priority: p2 Moderately-important priority. Fix may not be included in next release. 🚨 This issue needs some love. type: bug Error or flaw in code with unintended results or allowing sub-optimal usage patterns.

Comments

@stephenplusplus
Copy link
Contributor

Copied from original issue: googleapis/google-cloud-node#2770

@­sebelga
December 12, 2017 6:35 AM

Hello,

I have seen that since version 1.1.0 of the datastore api there is a new syntax for the "excludeFromIndexes" allowing embedded entities to have their properties excluded from indexes.

I am wondering if this new syntax is currently supported as it is not yet in the "master" documentation (it only appears under the "1.1.0" version).

I am sending the following payload to save an entity but all of the properties are marked as "indexed" in the google cloud console. Thus the "excludeFromIndexes" is not working as expected.

{
    "key": {
        "namespace": "my.namespace",
        "kind": "MyEntity",
        "path": [
            "MyEntity",
            null
        ]
    },
    "data": [
        {
            "name": "systems",
            "value": {
                "a": "123c",
                "b": "456c"
            }
        },
        {
            "name": "phoneNumber",
            "value": "0123456789"
        },
        {
            "name": "createdAt",
            "value": "2017-12-12T06:16:35.145Z"
        }
    ],
    "excludeFromIndexes": [
        "phoneNumber",
        "systems",
        "createdAt",
        "systems.a",
        "systems.b"
    ]
}

Thank for any help on this!

@stephenplusplus
Copy link
Contributor Author

December 12, 2017 8:58 PM

Hey @sebelga! I don't believe excludeFromIndexes is compatible with the array-notation format of data.

We originally only supported the old excludeFromIndexes markup if the user provided an array. That was so we could separate the excludeFromIndexes property from the properties within a user's entity. This style was a problem when nested properties needed to declare excludeFromIndexes. To handle that situation, we created the more robust, top-level excludeFromIndexes array, like you've defined. To opt-in, you just need to convert the data array to an object:

{
    "data": {
        "systems": {
            "a": "123c",
            "b": "456c"
        },
        "phoneNumber": "0123456789"
        "createdAt": "2017-12-12T06:16:35.145Z"
    }
}

Let me know if that doesn't fix the issue, or if that style for data creates any problems.

@stephenplusplus stephenplusplus added api: datastore priority: p2 Moderately-important priority. Fix may not be included in next release. type: question Request for information or clarification. Not an issue. labels Dec 16, 2017
@stephenplusplus
Copy link
Contributor Author

@­sebelga
December 13, 2017 10:40 AM

Hi @stephenplusplus ! Ok I will give it a try and come back with my results.

I've found inconsistencies (see my comment here googleapis/google-cloud-node#2510 (comment)) and haven't been able to find the proper way to both declare "excludeFromIndexes" for a simple properties, for Arrays (exclude all their properties) and for a properties on an embedded entity.

I will give a try with what you comment and let you know. 👍

@stephenplusplus
Copy link
Contributor Author

@­beaulac
December 14, 2017 5:06 PM

If #2773 is merged, it would allow this syntax to also support (non-nested) arrays.

@sebelga
Copy link

sebelga commented Dec 18, 2017

I have been doing some test and now I remember why I mixed the old "array" style for the data with the new more robust excludeFromIndexes array declaration.

If we declare an "array" property as being "excludeFromIndexes" an error is returned:

"Exclude from indexes cannot be set on a list value"

So this entity will throw an error

{
    "key": {
        "namespace": "my.namespace",
        "kind": "MyEntity",
        "path": [
            "MyEntity",
            null
        ]
    },
    "data": {
        "phoneNumber": "123-456",
        "systems": {
            "abc": 123
        },
        "tags": [
            "abc",
            "def"
        ],
        "createdAt": "2017-12-18T18: 40: 50.287Z"
    },
    "excludeFromIndexes": [
        "phoneNumber",
        "systems",
        "createdAt",
        "systems.abc",
        "tags" // tags is an Array. It throws an error
    ]
}

Will #2773 fix this issue?
thanks! 👍

@stephenplusplus
Copy link
Contributor Author

@sebelga I believe this is resolved with the latest release. Could you give it a shot and let me know? Thanks!

@sebelga
Copy link

sebelga commented Dec 30, 2017

@stephenplusplus ok thanks! I am out on holidays, I'll give it a try next week and let you know.
Happy new year!

@stephenplusplus
Copy link
Contributor Author

Sounds great and happy holidays! I'll close the issue for now, but we can re-open if needed :)

@sebelga
Copy link

sebelga commented Jan 3, 2018

@stephenplusplus I confirm that it does work, thanks! And thank you @beaulac for the #18 👍

@trollkotze
Copy link
Contributor

trollkotze commented Jun 8, 2018

This is not fixed!

I am using @google-cloud/datastore v1.4.0, which includes the changes from #2773 (old repo) merged in #18.

@sebelga

Will #2773 fix this issue?
thanks! 👍

@stephenplusplus

I believe this is resolved with the latest release. Could you give it a shot and let me know? Thanks!

@sebelga

I confirm that it does work, thanks! And thank you @beaulac for the #18 👍

I tested it with the very same example entity as @sebelga posted above:

{
    "key": {
        "namespace": "my.namespace",
        "kind": "MyEntity",
        "path": [
            "MyEntity",
            null
        ]
    },
    "data": {
        "phoneNumber": "123-456",
        "systems": {
            "abc": 123
        },
        "tags": [
            "abc",
            "def"
        ],
        "createdAt": "2017-12-18T18: 40: 50.287Z"
    },
    "excludeFromIndexes": [
        "phoneNumber",
        "systems",
        "createdAt",
        "systems.abc",
        "tags" // tags is an Array. It throws an error
    ]
}

I get the same old error: "INVALID_ARGUMENT: Exclude from indexes cannot be set on a list value".

@JustinBeckwith JustinBeckwith reopened this Jun 8, 2018
@JustinBeckwith JustinBeckwith added type: bug Error or flaw in code with unintended results or allowing sub-optimal usage patterns. and removed type: question Request for information or clarification. Not an issue. labels Jun 8, 2018
@trollkotze
Copy link
Contributor

Oh! I'm sorry!

I forgot to include in the example the modified syntax as described in #2773 (old repo):

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.

So changing the test example syntax to

    "excludeFromIndexes": [
        // ...
        "tags[]"
    ]

makes it work.

Thanks for the quick reopening. Can be closed again already. :)

@trollkotze
Copy link
Contributor

trollkotze commented Jun 8, 2018

However, this here, as described in #2773 (old repo) by @beaulac, does not quite work as expected by me:

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']
}

In this case, both array elements (the primitive string value and the object containing "name" property) are excluded from indexes. However, the array itself (without any of its elements?) is still included in the index.

I can exclude the array completely from indexes only if I rewrite it in the property-array syntax like this:

 {
   key: key,
   data: [{
       name: 'array',
       value: [
           'a string',
           {name: 'string in entity'}
       ],
       excludeFromIndexes: true,
   }]
 }

Maybe this should have been clear already, but it was not for me. So I'm just dropping that here for anyone else who might encounter that problem.

I think a detailed explanation of all the different modes of excluding from indexes and what works together with what would really be necessary for the docs, if it does not yet exist. (I have not come across it.)

@sqram
Copy link

sqram commented Oct 18, 2018

Agreed that the different ways of doing this should be more clear in the docs.
Right now we're using bug reports as the doc, which works but isn't the best.

@google-cloud-label-sync google-cloud-label-sync bot added the api: datastore Issues related to the googleapis/nodejs-datastore API. label Jan 31, 2020
@yoshi-automation yoshi-automation added the 🚨 This issue needs some love. label Apr 6, 2020
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 googleapis/nodejs-datastore API. priority: p2 Moderately-important priority. Fix may not be included in next release. 🚨 This issue needs some love. type: bug Error or flaw in code with unintended results or allowing sub-optimal usage patterns.
Projects
None yet
Development

No branches or pull requests

6 participants