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

fix(mojaloop/#2574): sdk-scheme-adapter is not returning party sub-id #120

Conversation

mdebarros
Copy link
Member

@mdebarros mdebarros commented Nov 10, 2021

fix(mojaloop/#2574): sdk-scheme-adapter is not returning party sub-id:
- fix for mojaloop/project#2574
- re-factored party model to break up the DB logic into discrete functions
- re-worked to correctly handle accounts
- added correct index'ing for the accounts table
- subIdValue has been replaced with idSubValue to be consistent with the DFSP API, note the Database will continue to use subIdValue to be backward compatible
- updated eslint to ignore some un-pragmatic rules

fix(mojaloop/#2575): mojaloop-simulator rules are not executing as expected
- updated unit tests to cover issues for mojaloop/project#2575
- issue is caused by an update to the json-rules-engine. The path defined in rules must now start with "$" (e.g. $.<attribute>), previously .<attribute> would work but no longer functional due to the dependency being updated
- updated example rule paths with the missing $ prefix

chore: updating dependencies
- updated dependencies
- added node-fetch & eslint to ncurc ignore list due to a breaking change due to node version incomptability
- fixed audit-resolve issues
- fixed some typos/spacing/lines in readme

- fix for missing "party.idSubValue" value expected by the SDK-Scheme-Adapter for GET /parties/{type}/{id}/{subId}
- fix for mojaloop/project#2574
- re-factored party model to break up the DB logic into discrete functions
- re-worked to correctly handle accounts
- added correct index'ing for the accounts table
- `subIdValue` has been replaced with `idSubValue` to be consistent with the DFSP API, note the Database will continue to use `subIdValue` to be backward compatible
- updated eslint to ignore some un-pragmatic rules
- updated dependencies
- added node-fetch to ncurc ignore list due to a breaking change with regard to node version
- fixed audit-resolve issues
- fixed some typos/spacing/lines in readme
…pected

- updated unit tests to cover issues for mojaloop/project#2575
- issue is caused by an update to the [json-rules-engine](https://github.com/CacheControl/json-rules-engine). The path defined in rules must now start with "$" (e.g. `$.<attribute>`), previously `.<attribute>` would work but no longer functional due to the dependency being updated
- rolled back eslint dependency update to the latest 7.x due to node version incompatibility issues.
- added eslint to ignore ncurc list
- lint fixes
@mdebarros mdebarros changed the title fix(mojaloop/#2574): sdk-scheme-adapter is not returning party sub-id fix(mojaloop/#2574): sdk-scheme-adapter is not returning party sub-id Nov 10, 2021
@mdebarros mdebarros marked this pull request as draft November 10, 2021 16:17
@mdebarros mdebarros marked this pull request as ready for review November 10, 2021 16:29
vijayg10
vijayg10 previously approved these changes Nov 10, 2021
vijayg10
vijayg10 previously approved these changes Nov 10, 2021
Copy link
Contributor

@lewisdaly lewisdaly left a comment

Choose a reason for hiding this comment

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

Just a minor note on the rules-engine test

@@ -72,7 +72,10 @@ test('Evaluates a rule based on demo data', async (t) => {

const input = {
path: '/transfers',
body: '123',
// body: '123', // <-- This still passes and seems to be a bug with the json-rules-engine: https://github.com/CacheControl/json-rules-engine/releases/tag/v2.3.6. This comment is kept here until this can be addressed in future with the library dependency.
Copy link
Contributor

Choose a reason for hiding this comment

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

We're using the latest version: https://github.com/mojaloop/mojaloop-simulator/blob/master/src/lib/rules-engine/package.json#L13

I think this is because we need to update the syntax of this expression to be:

path: '$.transfers',
body: '123',
method: 'POST'

See this example of the update json-rules-engine syntax

Copy link
Member Author

@mdebarros mdebarros Nov 11, 2021

Choose a reason for hiding this comment

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

Please see notes above:

fix(mojaloop/#2575): mojaloop-simulator rules are not executing as expected
- updated unit tests to cover issues for mojaloop/project#2575
- issue is caused by an update to the json-rules-engine. The path defined in rules must now start with "$" (e.g. $.<attribute>), previously .<attribute> would work but no longer functional due to the dependency being updated
- updated example rule paths with the missing $ prefix

Let me know when you are online so we can discuss this.

The above comment is due to a bug (or at least I think it is) in json-rules-engine where the JSON Path does not resolve correctly as per the above example.

I.e.

If the body is:

  body: '123',

And the Path is:

  "path": "$.amount"

It should not resolve based on the JSON-PATH resolution, but yet it does (i.e. the test passes). I believe this is a bug in the dependency library (which is probably a bug in the JSON-PATH library that it depends on).

The above path SHOULD only resolve if the body is as follows:

  body: {      // <-- correct body 
      amount: '123',  // <-- Matching PATH
  }

Issue reference: CacheControl/json-rules-engine#285

@mdebarros mdebarros merged commit b0e9504 into mojaloop:master Nov 11, 2021
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.

None yet

3 participants