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

adding hash to the subscription object to avoid duplicated subscripti… #23

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions HISTORY.MD
Original file line number Diff line number Diff line change
@@ -1,3 +1,7 @@
0.4.2 /2020-03-15
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
0.4.2 /2020-03-15
0.4.3 /2020-03-15

And please bump version in package.json

=================
* Updated code to avoid duplicated subscriptions by adding a hash to subscription object

0.4.2 / 2020-02-16
==================

Expand Down
18 changes: 15 additions & 3 deletions logic/observer.js
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
const errors = require('../util/errors'),
const crypto = require ('crypto')
const errors = require('../util/errors'),
TransactionWatcher = require('./transaction-watcher'),
Notifier = require('./notifier'),
storage = require('./storage'),
Expand Down Expand Up @@ -38,10 +39,21 @@ class Observer {
}

subscribe(subscriptionParams, user) {
//TODO: prevent duplicate subscriptions by checking subscription hash (fields "account", "asset_type" etc.)
//https://www.npmjs.com/package/farmhash
return this.loadSubscriptions()
.then(() => {
// Create hash in the subscription to avoid duplication

let hashData = `${subscriptionParams.account} ${subscriptionParams.asset_type} ${subscriptionParams.asset_code} ${subscriptionParams.asset_issuer}`
Copy link
Collaborator

@orbitlens orbitlens Mar 17, 2020

Choose a reason for hiding this comment

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

Please check all meaningful subscription params here:

createSubscription(subscriptionParams, user) {

We can use account, memo, operation_types, asset_code, asset_issuer, expires, and reaction_url. Maybe it makes sense to do something like JSON.stringify(subscriptionParams) to transparently handle all future parameters changes. What do you think?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You are welcome!

  • Adding the relevant parameters for the hash --> will do

  • having the hash as the generic_id --> I am in favor of it. However, I think that we need to be able to deal with older versions that will GET and DELETE based on the old id's. How do we deal with that? Finally, the DELETE endpoint does not DELETE the subscription from mongo, I could not find the code for it. Is this correct?

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 did a bit more of research and found that Mongo uses 24 character hex string for its _id. The hash generated is a 32 character hex string. No idea how to use the has as the generic id

Copy link
Contributor Author

Choose a reason for hiding this comment

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

And about the delete endpoint I saw that the deleted subscriptions have a status of 1 👍

let hash = crypto.createHash('md5').update(hashData).digest("hex");
subscriptionParams.hash = hash

let subscription = this.subscriptions.find(s => s.hash == hash)

if(subscription){

return subscription
}

if (this.getActiveSubscriptionsCount() >= config.maxActiveSubscriptions) {
return Promise.reject(errors.forbidden('Max active subscriptions exceeded.'))
}
Expand Down
15 changes: 15 additions & 0 deletions logic/storage.js
Original file line number Diff line number Diff line change
Expand Up @@ -58,6 +58,19 @@ class Storage {
})
}

/**
*
* @param {*} hash - the hash of the subscription
*/

async fetchSubscriptioHash(hash){
this.provider.fetchSubscriptioHash(hash)
.then(subscription =>{
return subscription
})
}


/**
* Load next notification from db
* @param {*} subscriptionId - subscription id
Expand Down Expand Up @@ -208,6 +221,8 @@ class Storage {
subscription.expires = expirationDate
}

subscription.hash = subscriptionParams.hash

return this.provider.saveSubscription(subscription)
}

Expand Down
6 changes: 6 additions & 0 deletions models/subscription-model.js
Original file line number Diff line number Diff line change
Expand Up @@ -67,6 +67,12 @@ class SubscriptionModel extends Model {
* Cached notifications, associated with the subscription
*/
notifications

/**
* Subscription hash to avoid duplicated subscriptions
*/
hash

}

module.exports = SubscriptionModel
4 changes: 4 additions & 0 deletions persistence-layer/mongodb-storage-provider/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,10 @@ class MongoDBStorageProvider extends StorageProvider {
return Subscription.findById(id)
}

fetchSubscriptioHash(hash){
return Subscription.findOne({hash})
}

fetchNextNotification(subscriptionId) {
return Notification.findOne({subscriptions: toObjectId(subscriptionId)})
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,8 @@ const subscriptionSchema = new Schema({
reaction_url: {type: String},
delivery_failures: {type: Number, default: 0},
sent: {type: Number, default: 0},
expires: {type: Date}
expires: {type: Date},
hash:{type:String}
},
{
timestamps: {createdAt: 'created', updatedAt: 'updated'}
Expand Down