bedeoverend / feathers-datastore

MIT License
9 stars 4 forks source link

Use upstream @google-cloud/datastore #8

Open bedeoverend opened 7 years ago

bedeoverend commented 7 years ago

Currently we're using a forked tarball of @google-cloud/datastore, ick. This was a workaround to get a forked version of the git repository working under a scoped package name.

This isn't viable going forward so will need to be reverted to using upstream datastore package once this issue is resolved.

quentin-sommer commented 7 years ago

Hi,

just letting you know that we're wanting to use datastore with feathers just like you and ran into the same issue.

Thanks for the library by the way!

bedeoverend commented 7 years ago

@quentin-sommer no worries 😄 let me know how you go with it!

And the issue you've got is the storing properties > 1500 bytes? If you get a chance, be great if you could comment on this issue saying you've got the same problem / what you think of potential solutions. Hopefully will help bump the issue a bit 😃

quentin-sommer commented 7 years ago

Yep, the issue is regarding > 1500 bytes unindexed sub-properties in JSON fields, watching the other issue as well!

quentin-sommer commented 7 years ago

Hi @bedeoverend, I'm having trouble marking nested entities with array properties as excludeFromIndexes.

{
  nested: {
    dataOk: 1,
    dataFail: [{
        field: '',
    }]
   }
}
dontIndex: ['nested']

here the library will fail because of the array field. (I think) it will do recursion and try to set the excludeFromIndexes on dataFail property and make google's library yield Error: Exclude from indexes cannot be set on a list value

I don't know why because the code you wrote here https://github.com/GoogleCloudPlatform/google-cloud-node/issues/1916 seemed to address the array case. Any ideas ?

bedeoverend commented 7 years ago

@quentin-sommer I'm not quite sure what's going on here. Looks like Array values can't be excluded, but I thought I'd tested that when I forked / patched datastore...but perhaps that's not the case - I'll have to have a bit more of a dig into it and let you know how I go.

quentin-sommer commented 7 years ago

Yep I thought that too when I read your patch! I just double checked the test case to reproduce the error

const data = {
  nested: {
    dataOk: 1,
    dataFail: [{
      field: '',
    }]
  }
}

const params = {
  query: {
    dontIndex: ['nested']
  }
}

datastoreService.create(data, params)

yields :

{ Error: Exclude from indexes cannot be set on a list value
    at /node_modules/grpc/src/node/src/client.js:434:17 code: 400, metadata: Metadata { _internal_repr: {} } }

If I understood correctly the official documentation in order to mark an array as excludeFromIndexes we need to write each of its items as excludeFromIndexes but not the array field.

bedeoverend commented 7 years ago

Sorry I didn't get back to you sooner. Yeah so I've got that replicating locally too. I should be able to fix with a simple patch in the forked version - basically it will still recursively exclude items, but will skip array entities themselves; still excluding the array items however.

I'll try get that patch up in the next day or so

quentin-sommer commented 7 years ago

Awesome ! If you could @mention me then I'd appreciate :)

Le jeu. 4 mai 2017 à 10:06, Bede Overend notifications@github.com a écrit :

Sorry I didn't get back to you sooner. Yeah so I've got that replicating locally too. I should be able to fix with a simple patch in the forked version - basically it will still recursively exclude items, but will skip array entities themselves; still excluding the array items however.

I'll try get that patch up in the next day or so

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/bedeoverend/feathers-datastore/issues/8#issuecomment-299121073, or mute the thread https://github.com/notifications/unsubscribe-auth/AItOGC5_ZlyDlnbA1JKGR9F_tSeSfZnPks5r2YbxgaJpZM4LiSS0 .

--

Quentin Sommer

bedeoverend commented 7 years ago

@quentin-sommer I've updated to use the latest fork - could you pull that down and check it resolves your issue?

quentin-sommer commented 7 years ago

It resolved it! :+1: