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

After upgrading to 5.1.0 retrieving balance using custom properties as keys returns 0 #58

Closed
cycleguy opened this issue Jan 25, 2022 · 11 comments

Comments

@cycleguy
Copy link

@koresar
Thank you for this module.

We have created a test program that demonstrates this. We have introduced two custom properties: _businessId and _assetId.

  • Without using a custom schema retrieving a book balance using these custom properties as keys returns the correct result.
  • When using a custom schema (line 15 in app.js is uncommented), retrieving book balance without using any of the custom properties works correctly (see _retrieveBalance() in app.js), but retrieving balance using either one or two of the custom properties as keys returns 0 (see _retrieveBalanceWithCustomProperties1() and _retrieveBalanceWithCustomProperties2() in app.js)

For the 3 test cases the correct balance should be

  • -2000 for _retrieveBalance()
  • 2000 for both _retrieveBalanceWithCustomProperties1() and _retrieveBalanceWithCustomProperties2()

To run without custom schema defined (ensure line 15 in app.js is commented out)
Tun run with custom schema defined (ensure line 15 in app.js is uncommented)

Possibly we are doing something wrong in our schema definition. Thanks in advance for your help.

medici-v5.zip

@Uzlopak
Copy link
Collaborator

Uzlopak commented Jan 25, 2022

The implicit registration of Schemas is obsolete with medici v5. you have to write a mongoose schema and use setTransactionSchema use as first parameter the Schema. Based on the Schema, medici v5 determines if it is a meta key or root key. As you did not assigned it, medici tries to search for _businessId and _assetId in the meta field and not on the root field

Also be aware: In medici v4 there was an implicit transformation of all vaues to ObjectIds if the key was starting with an _. Now based on the Schema, we determine if it is an ObjectId or not.

@cycleguy
Copy link
Author

We have written a mongoose schema and we are using setTransactionSchema within the mediciTransaction.js file. You mentioned that we did not assign it, what exactly are you referring to? Thanks.

@Uzlopak
Copy link
Collaborator

Uzlopak commented Jan 25, 2022

In your zipfile you don't call setTransactionSchema.

@cycleguy
Copy link
Author

I double checked the zip file that I sent you....

  • On line 15 of app.js (if uncommented out) does a require on ./models/mediciTransaction

  • On line 41 of mediciTransaction.js it calls setTransactionSchema()

  • src/app.js

  • src/index.js

  • src/models/mediciTransaction.js

@dolcalmi
Copy link
Contributor

We have the same issue, I think is a problem in parseBalanceQuery https://github.com/flash-oss/medici/blob/master/src/helper/parse/parseBalanceQuery.ts#L38 should use the same logic used in parseFilterQuery https://github.com/flash-oss/medici/blob/master/src/helper/parse/parseFilterQuery.ts#L52

@koresar
Copy link
Collaborator

koresar commented Jan 26, 2022

@dolcalmi @cycleguy any chance you can fix the parseBalanceQuery for us in a PR? That would be awesome and the fastest solution.

@cycleguy
Copy link
Author

Thank you in advance @dolcalmi for taking the time to fix this issue.

@koresar
Copy link
Collaborator

koresar commented Jan 27, 2022

The difference exists because the balance query (not the balance, but the query JSON) is being serialised to a single string as a Quick Balance cache key:

export function constructKey(book: string, account?: string, meta?: IAnyObject): string {

The key is being constructed wrongly when schema is not native and you query by our own keys.

Quick workarounds would be to disable Quick Balance feature. Pass balanceSnapshotSec as zero:
new Book(name, { balanceSnapshotSec: 0 });

@dolcalmi
Copy link
Contributor

this is not the only issue, the problem is how the query is parsed. I will work on it today

@dolcalmi
Copy link
Contributor

dolcalmi commented Jul 6, 2022

@koresar this can be closed once the new version is released

@koresar
Copy link
Collaborator

koresar commented Jul 8, 2022

Published in v5.2

@koresar koresar closed this as completed Jul 8, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

No branches or pull requests

4 participants