-
Notifications
You must be signed in to change notification settings - Fork 620
Key IDs Are Coming Back with String Values #2093
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
Comments
On it. |
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);
});
});
}); |
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);
});
});
}); |
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? |
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.] |
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 |
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.: 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:
|
I don't mind that solution either (having a more explicit Given that you already think |
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? |
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 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 |
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:
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. |
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 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. |
Fair enough. Thank you for taking the time to respond promptly. |
Uh oh!
There was an error while loading. Please reload this page.
Environment details
"@google-cloud/datastore": "^0.7.0",
"@google-cloud/storage": "^0.6.0",
Steps to reproduce
google-cloud
4 Attempt to fetch the entity identified by the key returned in step 3.
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
The text was updated successfully, but these errors were encountered: