Skip to content

Add voters in batches#157

Merged
PascalinDe merged 14 commits intomainfrom
feat_batch_voters
May 7, 2024
Merged

Add voters in batches#157
PascalinDe merged 14 commits intomainfrom
feat_batch_voters

Conversation

@PascalinDe
Copy link
Member

This PR adds voters in batches instead of one-by-one, allowing to add > 5000 voters at a time. Closes #155 .

@PascalinDe PascalinDe requested review from ineiti and multiscan April 24, 2024 17:03
@PascalinDe PascalinDe self-assigned this Apr 24, 2024
Copy link
Member

@ineiti ineiti left a comment

Choose a reason for hiding this comment

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

There is one part with the then where I'm not sure the code does what you want it to. Else it looks good to me.

.catch((e) => {
res.status(400).send(`Error while adding to roles: ${e}`);
});
if (Object.hasOwn(req.body, 'userId')) {
Copy link
Member

Choose a reason for hiding this comment

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

For next time: I think 'userId' in req.body would be clearer to me. Same below.

Comment on lines 38 to 45
addPolicy(req.body.userId, req.body.subject, req.body.permission)
.then(() => {
res.set(200).send();
next();
})
.catch((e) => {
res.status(400).send(`Error while adding to roles: ${e}`);
});
Copy link
Member

Choose a reason for hiding this comment

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

You have to be careful with the then construct here, as it will continue executing the code below, which might not be what you want. If you think this is a problem (I didn't look at it in detail), you should change it to the following. Same for the then just below.

  try {
    await addPolicy(req.body.userId, req.body.subject, req.body.permission);
    res.set(200).send();
    next();
  } catch(e) {
    res.status(400).send(`Error while adding to roles: ${e}`);
  };

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm not sure I understand what you mean with "ilt will continue executing the code below" - the three cases are mutually exclusive (else if and else) and the function returns after executing either of the three?

Copy link
Member

Choose a reason for hiding this comment

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

As discussed during coffee - it should be fine. But a good cleanup would be to do a

try {
  await ...
} catch (e) {
}

Instead of using then.

@PascalinDe PascalinDe requested a review from ineiti April 30, 2024 15:09
@PascalinDe PascalinDe marked this pull request as draft April 30, 2024 15:52
@PascalinDe PascalinDe marked this pull request as ready for review May 1, 2024 19:20
Copy link
Member

@ineiti ineiti left a comment

Choose a reason for hiding this comment

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

// empty then sciper unknown, otherwise add it in userDB
if ('userId' in req.body) {
try {
addPolicy(req.body.userId, req.body.subject, req.body.permission);
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
addPolicy(req.body.userId, req.body.subject, req.body.permission);
await addPolicy(req.body.userId, req.body.subject, req.body.permission);

And you'll also need to add an async somewhere in the definition of the callback, IIRC, like that:

usersRouter.post('/add_role', async (req, res, next) => {

Copy link
Member Author

Choose a reason for hiding this comment

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

used Promise.resolve because I wasn't sure about the async in the callback, does that make sense?

next();
} else if ('userIds' in req.body) {
try {
addListPolicy(req.body.userIds, req.body.subject, req.body.permission);
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
addListPolicy(req.body.userIds, req.body.subject, req.body.permission);
await addListPolicy(req.body.userIds, req.body.subject, req.body.permission);

@PascalinDe PascalinDe force-pushed the feat_batch_voters branch from 8747b69 to bc6e987 Compare May 6, 2024 15:49
@PascalinDe PascalinDe requested a review from ineiti May 6, 2024 15:49
Copy link
Member

@ineiti ineiti left a comment

Choose a reason for hiding this comment

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

I'm still surprised it actually works - according to my test in the typescript playground, the async should never be called.

if ('userId' in req.body) {
try {
readSCIPER(req.body.userId);
Promise.resolve(addPolicy(req.body.userId, req.body.subject, req.body.permission));
Copy link
Member

Choose a reason for hiding this comment

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

The Promise.resolve doesn't change anything - the promise will not be awaited for. Does the following PR break it?

#159

You can run Await Playground and see that goodbye is never called.

Copy link
Member Author

Choose a reason for hiding this comment

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

I wasn't sure about the async in the callback 😕 I'll try with async/await

Copy link
Member

@ineiti ineiti May 7, 2024

Choose a reason for hiding this comment

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

Here is a thread from stackoverflow: https://stackoverflow.com/questions/44813401/passing-in-async-functions-to-node-js-express-js-router

TLDR:

  • express >= 4 handles async callbacks
  • express >= 5 handles also errors w/o a try...catch

Edit: the post from 2021 in Stackoverflow made me think that express 5 is already a thing. But it's still in beta...

@PascalinDe PascalinDe force-pushed the feat_batch_voters branch from bc6e987 to 6f748b2 Compare May 7, 2024 07:28
@PascalinDe PascalinDe requested a review from ineiti May 7, 2024 07:28
@PascalinDe PascalinDe merged commit 19e40f2 into main May 7, 2024
@PascalinDe PascalinDe deleted the feat_batch_voters branch May 7, 2024 07:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: Done

Development

Successfully merging this pull request may close these issues.

Cannot add 5'000 scipers for an election

3 participants