-
Notifications
You must be signed in to change notification settings - Fork 12
adding hash to the subscription object to avoid duplicated subscripti… #23
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
base: master
Are you sure you want to change the base?
adding hash to the subscription object to avoid duplicated subscripti… #23
Conversation
orbitlens
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
First of all, good job! Thanks for contributing!
What do you think about replacing a generic subscription id with the hash you generate? It makes sense from the perspective of subscription uniqueness and a hash itself is more meaningful than auto-generated ids.
Most dbs do support binary primary keys, and for those rare cases when it is not supported directly, we can use a stringified representation in a storage plugin.
HISTORY.MD
Outdated
| @@ -1,3 +1,7 @@ | |||
| 0.4.2 /2020-03-15 | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| 0.4.2 /2020-03-15 | |
| 0.4.3 /2020-03-15 |
And please bump version in package.json
logic/observer.js
Outdated
| .then(() => { | ||
| // Create hash in the subscription to avoid duplication | ||
|
|
||
| let hashData = `${subscriptionParams.account} ${subscriptionParams.asset_type} ${subscriptionParams.asset_code} ${subscriptionParams.asset_issuer}` |
There was a problem hiding this comment.
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:
operations-notifier/logic/storage.js
Line 138 in 93aa567
| 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?
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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 👍
…bscription object
adding hash to the subscription object to avoid duplications in mongo