Repository navigation
I can't fetch by entity ID #2065
Description
Activity
For an ID, make sure you don't send it as a string, so:
datastore.key(['Task', 123]) // good datastore.key(['Task', '123']) // bad
OK, I'm sure I found a bug, but I have to explain. The method is save()
here: https://github.com/topseed/topseed-bp/blob/master/dsrv/ds/ContentDS.js
(it's called by Smoke.js https://github.com/topseed/topseed-bp/blob/master/dsrv/smoke.js.
When I give it a valid ID, it works. But when the ID is wrong - as a part of testing, it does a ' Unhandled Rejection at: Promise'.
The reason that is a problem is that it is called by Express, a 'unhandled rejection' is global, I never return an error to the html user!What the method does is give an ID, it does a fetch, merges the fields (so I don't lose columns) and then updates. It works when ID is found. But again, when no ID, I expect it to reject 'inline' so I can handle the rejection. Insert and select methods there work fine.
So if rejection is global, it creates a problem, I need to trap the rejection in flow. Worst case if I have to do call back, but I don't know how to get around this - and I need to put in production.
I have put a catch block there and no joy.
(My code starts at 'smoke.js')I think this would be a good example to have:
update kind by ID - but don't delete any columns. ( I do that by fetch and merge before update there ). But make sure if the ID is wrong or something wrong to notify inline. I think this is common operation and I don't know how to do that. Can you please just do a sample hello world update(lossless) by id that also rejects?@callmehiphop can you take a look through that code?
I'm not very well versed in promises, so he'll likely be more help. We do have a number of issues on backlog though, so please also ask on StackOverflow and link here, so others can follow along.
If you can boil it down to a simple test case without any external dependencies, maybe we can clear up what's going on.
So in ContentDS.js#64 I think your save method should handle failure cases as @stephenplusplus points out. Then you should also handle failure cases in smoke.js#54.
I have put catch in both places and it did not catch or handle.
I pushed the code, even if it does nothing - as per your suggestions. Errors get handled outside of the loop creating leaks of sorts each time there is an error. An easy exploit on www.
I think this is a common need, to update via key and 'merge' fields - it takes me 50 lines of code and... it does not handle errors. Like I said insert and load work fine on errors, just update does not handle errors.Your code does promises and if there is no fix coming soon, then I'd like to know if there is a way to update using callbacks?
@puppetmaster3 I think the error handling is just in the wrong spot, your save method should look more like this
save: function(dom, id, jDObj) { return _save(dom, id, jDObj).then(function(res) { return res[0].mutationResults[0]; }); }
I fixed it. I was not catch, but the other syntax used instead of catch - you can see code in git.
I suspect something is wrong deep in your code and at some point when you fix it, it will break my working code. Proof: insert and load test out bad path w/ catch. But update (w/ merge) works w/ }, function (err).
I can use it, but I'll be very worried when you guys do new releases. You should have example of update by key and merge fields as a test case.
@puppetmaster3 I'm looking at your code a little closer and I think there might be an issue with your
_savefunction as well. If you were to refactor it to resemble something like thisfunction _save(dom, id, JDObj) { const key = _makeKey(id); return DS.get(key).then(function(data) { const row = data[0]; Object.assign(row, jDObj); const entity = { key: key, data: row }; return DS.update(entity); }).catch(function(err) { console.log('_save CDS err ' + err); return Promise.reject(err); }); }
I think you might see some different results! (The provided code is untested though.).
I have committed that code and I still have to use , function(er){
in 2 places (not needed for load or insert).
If I don't have it, I get the unhandled error out of the path.
If the test is positive, all works no matter what, but when I test a fail, I get the unhandled w/o ,function(er) {@puppetmaster3 can you condense your code to the smallest possible use case without any external dependencies? Ideally a single file that serially runs commands up until they fail. That will help us figure out the root cause in the quickest way possible.
@puppetmaster3 I'm sorry, but we'll need to work from a smaller reproduction case. It's understandable that it will take time for you to come up with one, so whenever you get to it, please let me know and we will re-open this issue.
3 remaining items
Your previous replies, now deleted, were suggesting we look through the repo. Sorry for misunderstanding, I'm happy to re-open so we can keep working on it.
Tried running, currently stuck at:
const auThoPro = A.getAuThoB('aa' ) //return role promise ^ ReferenceError: A is not definedMy note also said I'll be working on an example but you can't just give me an hour.
Updated:
https://github.com/topseed/topseed-bp/blob/master/ssrv/routes/ex.jsAgain, this code works as I need it to, and using it is clean. Just the implementation promises looks weird relative to other code that does not use your update(). For example, I should only have the final catch, and I need one in middle and I need a 'err' handle in 3rd function - to handle a fail inline.
You guys can also provide example or docs of merging fields that works w/ a 'fail', I should not have to figure this out by self.
$ node . aa Smoke err ReferenceError: U is not definedFrom this line:
jD.lastDate = U.getDt()//store date of changeI commented that line out, now I get:
$ node . aa _save er TypeError: Cannot convert undefined or null to object save er Error: TypeError: Cannot convert undefined or null to object at index.js:86:11
Just to confirm, is that the error we're trying to fix?
The example is good and works well and gets used well.
The result you have is a good one: that id does not exists and the smoke test is able to catch the error.The issue is that the implementation code to get it to work is 'strange'. For example catch and er handlers are not needed when I do checks on insert and inserts work as I expect promise to work. But when I update I have to add a ', err' handler and a catch in middle for it to say inline on a fail. You nsert follows promise proper use. But using update requires jumps. What I am doing is quite simple, a merge. Again, I have no issues, and using it works. Also I have base class that handles it, so when you guys do refactor, my code will not be affected.
I'm glad to hear it's working well, so there's at least no immediate need to come to a resolution.
So I believe what you're asking for is a convenience method in our API that can replace these steps you have to go through with one single method call. Can you show how you'd like that to look?
Correct, there is no immediate need.
But I am not needing a convenience method either, I should implement update like I implement insert, observe the insert implementation line, 97:
https://github.com/topseed/topseed-bp/blob/master/ssrv/routes/ex.jsNotice that I don't have a catch() or , errror. As per promise spec, I just have a catch when I use it, in my route. The insert first checks to see if a record exists.
But to do a realistic update, I require both! Line 89 and 53. Both are neede and neither should be needed. That is not per promise spec! I should only need line 36 when I use it! Do you see the diff btwn how insert works and how update works?
But, I don't need line 89 or 53 if I test using the happy path. But when I test a 'failed' update/ID, both are needed, else I get a unhandledRejection resource leak in node on any browser side error passed to node - outside of my handlers.But the implementation issue is abstracted in my code so that when you fix it it won't affect my code. Those 2 lines fix the resource leak (unhandledRejection). It would help you to have docs, examples or tests that touch on common/realistic uses cases. Realistically, it's never update, it is update and merge if ...; and it's never insert, it is insert if; almost always combined w/ a fetch first. And always report the success or fail all the way up.
Also, the issue may be w/ 'get' line 76 and not the update.
I believe this is a matter of how you are writing the promises. I condensed your code further, keeping the same amount of functions and the execution path. An error from any promise is returned in the singular
catchhandler.It's possible. Do you have an example of similar?
Sorry, forgot to include the link: https://gist.github.com/stephenplusplus/2f25ad7272d45612a9e8c1d920469be4
Sorry, that is not a userful examples
The tst code you have does not have a catch - and it must: In my router I have to know did it work or not. That is using the implementation, and no catch there to handle. Compare to my working exmaple that does sow if it worked or not in tst(). I just think given that requirement, the implementation should be cleaner, that is all. To see this, just add the 3 lines of code to route an express JSON, that posts ID and some field to change. Yes, you can say its the way I wrote it, but there are no docs, examples or tst code that I found of how. So I had to write my own that works, but code is funny looking.- added 2 commits that reference this issue
on Feb 26, 2026
Using v0.7 on node 7.
Looking here:
https://cloud.google.com/datastore/docs/concepts/queries
it says
What I need is, given an id for an entity, ex id = '5709436928655360' and entity 'bla'
My code :
https://github.com/topseed/topseed-bp/blob/master/dserver/ds/ContentDS.js
to make a key is ~ line 37.
Is that right? It's not working.
The code to fetch is ~ line 15. Also not working but could be due to key not correct.
There are no examples to fetch a row by key id, I looked and googled.
(Separate part 2, I want to update new columns, but not delete old ones, so first I fetch so I can merge).
Help/hints please.