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

fix BB-672: rewriting promises using async await in .routes/entity folder #1058

Open
wants to merge 9 commits into
base: master
Choose a base branch
from

Conversation

Tarunmeena0901
Copy link
Contributor

Problem

BB-672

Solution

  • rewrite the promises using aync await syntax in entity routes folder
  • used try catch block rather than .catch() to handle error

This pull request finishes the remaining tasks from PR-980. I began everything again to better understand the solution for myself.

Areas of Impact

src/server/routes/entity

Copy link
Contributor

@MonkeyDo MonkeyDo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the work !

There's a few tricky cases in there, and by god I know they are hard to read ad reason through!

I left a bunch of comments, some of which will need to be applied to all the files (variable naming, utility function, etc.)

Do let me know if something is unclear or if you have questions.

src/server/routes/entity/edition.ts Outdated Show resolved Hide resolved
src/server/routes/entity/edition.ts Outdated Show resolved Hide resolved
src/server/routes/entity/entity.tsx Outdated Show resolved Hide resolved
src/server/routes/entity/entity.tsx Outdated Show resolved Hide resolved
src/server/routes/entity/entity.tsx Outdated Show resolved Hide resolved
src/server/routes/entity/entity.tsx Outdated Show resolved Hide resolved
src/server/routes/entity/publisher.ts Show resolved Hide resolved
src/server/routes/entity/publisher.ts Show resolved Hide resolved
src/server/routes/entity/publisher.ts Outdated Show resolved Hide resolved
src/server/routes/entity/work.ts Outdated Show resolved Hide resolved
@Tarunmeena0901
Copy link
Contributor Author

Oh my, there are some rookie mistakes on my side. thank you for pointing them out

@MonkeyDo
Copy link
Contributor

Oh my, there are some rookie mistakes on my side. thank you for pointing them out

Like I said, reading these promise chains and figure out what they do and when is a real pain in the ass !
It's easy to get something wrong in the process (I should know, I've made my share of "promisetakes"!)

@Tarunmeena0901
Copy link
Contributor Author

Tarunmeena0901 commented Feb 22, 2024

I Renamed some variables , and I also took another look at the entity.tsx file. I found some more potential improvements in terms of rewriting the logic to remove promise chains. Although I attempted to rewrite the promises in a new syntax without altering the actual logic of the code, I still have my doubts about entity.tsx specially in -> L745 to L817.
please have a look @MonkeyDo 🤞🏻 🤞🏻

Copy link
Contributor

@MonkeyDo MonkeyDo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I had these pending comments on the PR. I will need to do another review next week, I'm finishing up for this week as we speak.

In the meantime the comments below should apply

src/server/routes/entity/entity.tsx Outdated Show resolved Hide resolved
src/server/routes/entity/entity.tsx Outdated Show resolved Hide resolved
src/server/routes/entity/entity.tsx Outdated Show resolved Hide resolved
src/server/routes/entity/entity.tsx Outdated Show resolved Hide resolved
src/server/routes/entity/entity.tsx Outdated Show resolved Hide resolved
@Tarunmeena0901
Copy link
Contributor Author

Tarunmeena0901 commented Jun 21, 2024

yup I thought so , working on this PR was little confusing for me back then and surely I made some rookie mistakes here ✅✅
I will fix them this weekend

PS: I will be manually checking this PR, one more time after latest changes tonight

Copy link
Contributor

@MonkeyDo MonkeyDo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking much better, thanks for having a look.

Just one remaining subtlety below, after that I think it's ready to merge.

Comment on lines 486 to 494
try {
res.send(entityDelete);
search.deleteEntity(entityDelete);
return entityDelete;
}
catch (err) {
log.error(err);
return error.sendErrorAsJSON(res, err);
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here there is a small subtlety which is that previously search.deleteEntity was called after the main entityDeletePromise succeeded, in such a way that an error thrown in search.deleteEntity was not sent back to the user.
Since we are now awaiting the promise further up, we don't need this try-catch block for returning the response (res.send) anymore, but we do need one for separately catching search indexing errors like so:

Suggested change
try {
res.send(entityDelete);
search.deleteEntity(entityDelete);
return entityDelete;
}
catch (err) {
log.error(err);
return error.sendErrorAsJSON(res, err);
}
res.send(entityDelete);
try {
search.deleteEntity(entityDelete);
}
catch (err) {
log.error(err);
}
return entityDelete;

However, we might need to wrap all the code from const entityDelete = await ... onwards into another top-level try-catch block, and in the catch block use that return error.sendErrorAsJSON(res, err); .

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done 👍

(〃 ̄︶ ̄)人( ̄︶ ̄〃)

@Tarunmeena0901
Copy link
Contributor Author

I have tested this PR manually everything seems to work right as far as I checked 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants