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

[#172885142] delete user data cli command #53

Merged
merged 9 commits into from
May 21, 2020

Conversation

balanza
Copy link
Contributor

@balanza balanza commented May 18, 2020

Includes:

  • cli command
  • redis session purge
  • block/unblock user during the execution

How to use

yarn build && REDIS_URL=redis://url-to-redis node dist/UserDataDeleteOrchestrator/cli.js <FISCAL_CODE>

To implement yet

  • change user data processing status
  • bundle user data and save it to a dedicated storage
  • delete all user data

@balanza balanza requested a review from gunzip May 18, 2020 18:12
@pagopa-github-bot
Copy link
Contributor

pagopa-github-bot commented May 18, 2020

Affected stories

  • 🌟 #172885142: Come CIT, voglio che quando la procedura di eliminazione dei miei dati è avviata, mi venga impedito l'accesso all'app, in modo da non ritrovarmi dati inconsistenti

New dependencies added: redis and redis-clustr.

redis

Author: Matt Ranney

Description: A high performance Redis client.

Homepage: https://github.com/NodeRedis/node-redis

Createdover 9 years ago
Last Updated3 months ago
LicenseMIT
Maintainers3
Releases103
Direct Dependenciesdenque, redis-commands, redis-errors and redis-parser
Keywordsdatabase, redis, transaction, pipelining, performance, queue, nodejs, pubsub and backpressure
This README is too long to show.

redis-clustr

Author: Unknown

Description: Redis cluster client

Homepage: https://github.com/gosquared/redis-clustr#readme

Createdover 6 years ago
Last Updatedabout 1 year ago
LicenseMIT
Maintainers4
Releases23
Direct Dependenciescluster-key-slot, denque and redis
Keywordsredis, cluster and database
README

redis-clustr

Dependencies
Join the chat at https://gitter.im/gosquared/redis-clustr

NPM

This module is a relatively thin wrapper around the node redis client to enable use of Redis Cluster. It tries to be as unobtrusive as possible - mimicing the behaviour of the node_redis client.

Usage

var RedisClustr = require('redis-clustr');

var redis = new RedisClustr({
  servers: [
    {
      host: '127.0.0.1',
      port: 7000
    }
  ]
});

redis.set('key', 'value');

Servers

Servers in the cluster will be automatically connected to (via the response of cluster slots). Of course, to allow discovery there must be at least one server specified in the configuration.

Client creation

By default, clients will be created using Redis.createClient(port, host). This can be overridden by providing a function which must return a node_redis client. Clients are cached so only one connection will be made to each server.

var RedisClustr = require('redis-clustr');
var RedisClient = require('redis');
var redis = new RedisClustr({
  servers: [...],
  createClient: function(port, host) {
    // this is the default behaviour
    return RedisClient.createClient(port, host);
  }
});

Options

var RedisClustr = require('redis-clustr');
var redis = new RedisClustr({
  servers: [...],
  slotInterval: 1000, // default: none. Interval to repeatedly re-fetch cluster slot configuration
  maxQueueLength: 100, // default: no limit. Maximum length of the getSlots queue (basically number of commands that can be queued whilst connecting to the cluster)
  queueShift: false, // default: true. Whether to shift the getSlots callback queue when it's at max length (error oldest callback), or to error on the new callback
  wait: 5000, // default: no timeout. Max time to wait to connect to cluster before sending an error to all getSlots callbacks
  slaves: 'share', // default: 'never'. How to direct readOnly commands: 'never' to use masters only, 'share' to distribute between masters and slaves or 'always' to  only use slaves (if available)
  createClient: function(port, host, options) {
    return require('redis').createClient(port, host, options);
  }, // default: redis.createClient. Function used to connect to redis, called with arguments above
  redisOptions: {
    // options passed to the node_redis client https://github.com/NodeRedis/node_redis#options-is-an-object-with-the-following-possible-properties
    retry_max_delay: 500
    // etc
  }
});

Supported functionality

Slot reallocation

Supported - when a response is given with a MOVED error, we will immediately re-issue the command on the other server and run another cluster slots to get the new slot allocations. ASK redirection is also supported - we wil re-issue the command without updating the slots. TRYAGAIN responses will be retried automatically.

Multi / Exec (Batch)

Multi commands are supported but treated as a batch of commands (not an actual multi) and the response is recreated in the original order. Commands are grouped by node and sent as node_redis batches

Multi-key commands (del, mget and mset)

Multi-key commands are also supported and will be split into individual commands (using a batch) then have the response recreated. Only del, mget and mset are supported at the moment.

Pub/Sub

Pub/Sub is fully supported. When subscribe is used, a new client will be created (connected to a random node). This client is shared for all subscriptions.

var RedisClustr = require('redis-clustr');
var redis = new RedisClustr({...});

redis.on('message', function(channel, message) { /* ... */ });

redis.subscribe('my-channel', function(err) {
  redis.publish('my-channel', 'have a lovely day!');
});

Errors

Just like node_redis, listen to the error event to stop your application from crashing due to errors. Redis Clustr automatically intercepts connection errors and will try to reconnect to the server.

Generated by 🚫 dangerJS

const deleteSessionTokens = new Promise<Either<Error, true>>(resolve => {
// Remove the specified key. A key is ignored if it does not exist.
// @see https://redis.io/commands/del
this.redisClient.del(
Copy link
Contributor

Choose a reason for hiding this comment

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

can you put some logging for every ops ? ie:

getting user sessions... <session1> ... <session2> ...
deleting SESSIONIFO-< token> ì ...
deleting USERMETA-<CF>
...

): TaskEither<Error, true> => notImplementedTask("setUserDataProcessingStatus");

// after the operation, unblock the user to allow another login
const post = (fiscalCode: FiscalCode): TaskEither<Error, true> =>
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd rather call these blockUser / unblockUser

@codecov-commenter
Copy link

codecov-commenter commented May 19, 2020

Codecov Report

Merging #53 into master will decrease coverage by 2.52%.
The diff coverage is 79.24%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #53      +/-   ##
==========================================
- Coverage   88.81%   86.29%   -2.53%     
==========================================
  Files          21       26       +5     
  Lines         608      868     +260     
  Branches       37       55      +18     
==========================================
+ Hits          540      749     +209     
- Misses         67      118      +51     
  Partials        1        1              
Impacted Files Coverage Δ
CreateService/handler.ts 85.18% <0.00%> (ø)
UpdateService/handler.ts 90.47% <0.00%> (ø)
GetService/handler.ts 86.95% <50.00%> (+3.62%) ⬆️
utils/zip.ts 53.33% <53.33%> (ø)
ExtractUserDataActivity/handler.ts 77.46% <77.46%> (ø)
utils/random.ts 77.77% <77.77%> (ø)
SetUserDataProcessingStatusActivity/handler.ts 88.37% <88.37%> (ø)
...eleteOrchestrator/GetUserDataProcessing/handler.ts 88.46% <88.46%> (ø)
... and 3 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 2ec3768...2f5a344. Read the comment docs.

*
* @returns either an Error or the new created record
*/
const fetchRecordFromDb = ({
Copy link
Contributor

Choose a reason for hiding this comment

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

can you extract this method outside the body of createGetUserDataProcessingActivityHandler ?

Copy link
Contributor

Choose a reason for hiding this comment

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

naming: fetchRecordFromDb ? -> getUserDataProcessingRequest

@@ -0,0 +1,204 @@
/**
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we need this Activity ? I'm asking because the orchestrator will be triggered by an upcoming record inserted into user-data-requests collection so the input comes from the orchestrator client function bindings

Copy link
Contributor

Choose a reason for hiding this comment

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

(it's ok to use a query in the client.ts)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I basically did it for not have to construct here models and require env keys (as we do already with other activities). I guess we can eventually remove it if we find it unhelpful in future refactoring

Copy link
Contributor

Choose a reason for hiding this comment

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

yes we should convert this into an utility function only used by the cli (not by the orchestrator)

@@ -140,32 +140,34 @@ const logFailure = (context: Context) => (
}
};

/**
Copy link
Contributor

@gunzip gunzip May 19, 2020

Choose a reason for hiding this comment

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

can you put these files inside the orchestrator / cli folder? (all top level folders are suposed to be functions and/or activities)

@gunzip gunzip merged commit 139e8b1 into master May 21, 2020
@gunzip gunzip deleted the 172885142-manual-user-delete branch May 21, 2020 11:43
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.

4 participants