Key IDs Are Coming Back with String Values

[husafan]

Environment details

• OS: OS X
• Node.js version: 6.6.0
• npm version: 3.10.3
• google-cloud-node version:
  "@google-cloud/datastore": "^0.7.0",
  "@google-cloud/storage": "^0.6.0",

Steps to reproduce

1. require google-cloud
2. Create an entity in the datastore with an auto-generated long ID.

      let key = this._datastore.key(['SomeEntity']);
      let dsEntity = {
        key: key,
        data: {...},
      };
      this._datastore.upsert(dsEntity).then...

3. Run a query that would fetch the entity:

      this._datastore.runQuery(query).then((response) => {
      let entities = response[0];
      let results = [];
      entities.forEach((entity) => {
        const resolved = {
          key: entity[Datastore.KEY], // <-- THIS KEY.ID IS NOW A STRING
          data: entity,
        };

4 Attempt to fetch the entity identified by the key returned in step 3.

       this._datastore.get(resolved.key) // <-- THIS WILL FAIL BECAUSE ID IS A STRING

We believe this is related to #1413, specifically, https://www.google.com/url?q=https%3A%2F%2Fgithub.com%2FGoogleCloudPlatform%2Fgoogle-cloud-node%2Fcommit%2Fbab982faba1c6ef3c393da816535be3704113787&sa=D&sntz=1&usg=AFQjCNGTKOrx2uKSEd7TrDMwbFqat44fVw

[stephenplusplus]

On it.

[stephenplusplus]

Here's a test I put together for this scenario. It's currently passing-- can you modify it so that it reproduces what you're running into?

var kind = 'Post-' + Date.now();

var postKey = datastore.key(kind);
var data = {
  test: true
};

datastore.save({
  key: postKey,
  data: data
}, function(err) {
  assert.ifError(err);

  var query = datastore.createQuery(kind)
    .filter('test', true);

  query.run(function(err, entities) {
    assert.ifError(err);

    var match = entities[0];

    if (!match) {
      done(new Error('Query did not return a match.'));
      return;
    }

    var key = match[datastore.KEY];

    datastore.get(key, function(err, entity) {
      assert.ifError(err);

      assert.deepEqual(entity, data);

      datastore.delete(postKey, done);
    });
  });
});

[husafan]

var kind = 'Post-' + Date.now();

var postKey = datastore.key(kind);
var data = {
  test: true
};

datastore.save({
  key: postKey,
  data: data
}, function(err) {
  assert.ifError(err);

  var query = datastore.createQuery(kind)
    .filter('test', true);

  query.run(function(err, entities) {
    assert.ifError(err);

    var match = entities[0];

    if (!match) {
      done(new Error('Query did not return a match.'));
      return;
    }

    var keyId = match[datastore.KEY].id;
    var key = datastore.key([kind, keyId]);

    datastore.get(key, function(err, entity) {
      assert.ifError(err);

      assert.deepEqual(entity, data);

      datastore.delete(postKey, done);
    });
  });
});

[stephenplusplus]

Gotcha. Instead of isolating the ID, just caching the Key object that was returned will work for future API usage. In scenarios where IDs and names need to be passed around, and the user can't guarantee the type, previously relying on type checks, they should somehow associate the type to it as well, e.g.

var key = match[datastore.KEY]
var identifier

if (key.name) {
  identifier += 'name:' + key.name
} else {
  identifier += 'id:' + key.id
}

sendToUI(keyIdentifier)

Do you have any thoughts on how we should handle this?

[husafan]

Understood; we often pass around the IDs as they are easy to use in HTML and it seemed unnecessary to keep a reference to the original Key entity when we could just recreate it at access time.

I am a bit unclear as to why the Id attribute of a Key is ever of type string in JS land given that JS Numbers can have arbitrary length. Is it not possible to make sure that Key.Id is always a number by the time the entities are returned from the datastore layer?

[EDIT: We do also, for the most part, associate the type and convert when creating a key. We hit this path on our update method, which was trying to do a quick get and set, and was assuming the types would be correct between the two calls.]

[stephenplusplus]

Unfortunately, the string is necessary because Numbers can only go so high in JS-- not as high as IDs generated by Datastore in all environments: #1412

[husafan]

I see. I did not realize JS had issues with large numbers as such.

Assuming datastore.key() creates the correct key based on the type of it's arguments, i.e.:
datastore.key(['Kind', 1234]) => {kind: 'Kind', id: 1234}
datastore.key(['Kind', 'Identifier']) => {kind: 'Kind', name: 'Identifier'}

Then your fix uses a gcloud.datastore.int, which is a third argument type, to represent a long int that needs to be an Id. In that case, why does Key.Id not return a gcloud.datastore.int instance instead of a String?

Although I suppose this still requires passing type info to the UI along with the stringified Id. I feel like the real problem here is the ambiguity in the datastore.key method. I think it should be more explicit. For instance:

datastore.key([{
    kind: 'AncestorType',
    name: 'Identifier'
  }, {
    kind: 'ChildType',
    id: '123456789101123'
}]

[stephenplusplus]

I don't mind that solution either (having a more explicit key method), but I wouldn't deprecate the way it works currently, since the way it works now is going to work for the average use case.

Given that you already think .key() can behave in too many ways, do you think adding the new, detailed way alongside it is still a good idea?

[husafan]

If datastore started generating 15 digit or larger key IDs, the average user would start seeing odd errors when calling parseInt on the string returned from Key.Id, correct? They would need to know to use datastore.int instead?

If that is the case, what about enforcing that datastore.key takes in either a string or a datastore.int and deprecate the use of Number types?

[stephenplusplus]

Sorry, re-reading #1412, it says the user is generating long IDs themselves. So since they know they're doing that, they know to use the int method. Datastore shouldn't be generating these, sorry for misspeaking.

But I'd agree, if that were the case, we would likely consider a different solution, possibly exactly what you proposed. But for now, the average use case gets along fine with datastore.key(['Group', 10, 'User', 'stephenplusplus']), so I don't think we should force a more verbose or unnatural way of saying that.

[husafan]

Ok. I understand. But if that's the case, then when the datastore is NOT generating a long, it should continue to return Id as a JS number when it is 14 digits or less. To be clear, I don't think:

var id = entity[datstore.KEY].id;
var key = datastore.key(['Kind', id]);
datastore.get(key)

Is an infrequent use-case - especially when doing an update or using the Id to construct a separate query. And without any changes to our code, it started breaking for us because Id went from being a Number to a String, which has no significance for us, but DOES matter to datastore API. So I don't think it's entirely fair to say that with this change, things continue to work for the average use case.

[stephenplusplus]

It's hard to say what's an average use case, but the API as we provide it does work as intended. When we say you can get the Key using the datastore.KEY symbol, the thing you get back will work in subsequent calls to the API. And we do properly associate an ID with key.id and a name with key.name. That information is enough for any use case.

The problem with returning an ID as different types depending on its value is the user won't know what to expect.

I'm sorry that this change threw a wrench in your app and genuinely appreciate your taking the effort to investigate the problem and have a thorough dialog on how we can improve the library for all users. I'm open to hearing more feedback on the matter and potential solutions. For now, I will close this issue as we've found the core problem and I believe you will know the path to address it in your application. If there is more we come up with that we should be doing, we'll re-open and take those steps.

[husafan]

Fair enough. Thank you for taking the time to respond promptly.