json-rules-engine icon indicating copy to clipboard operation
json-rules-engine copied to clipboard

PATH is not resolving correctly

Open mdebarros opened this issue 4 years ago • 3 comments
trafficstars

I have seen some strange happenings with regard to how the JSON-PATH is resolving in the rules with "json-rules-engine": "6.1.2".

If you consider the following rule:

{
    "ruleId": 1,
    "description": "Returns an NDC exceeded error response (ML error 4001) from the simulator when transfer value is 123 in any currency",
    "conditions": {
        "all": [
            {
                "fact": "path",
                "operator": "equal",
                "value": "/transfers"
            },
            {
                "fact": "method",
                "operator": "equal",
                "value": "POST"
            },
            {
                "fact": "body",
                "operator": "equal",
                "value": "123",
                "path": "$.amount"
            }
        ]
    },
    "event": {
        "type": "simulateError",
        "params": {
            "statusCode": 500,
            "body": {
                "statusCode": "4001",
                "message": "Payer FSP insufficient liquidity"
            }
        }
    }
}

With the following payload:

{
  path: '/transfers',
  body: '123', // <-- Non-matching PATH
  method: 'POST',
}

The rule will still resolve and fire off the event which I believe is incorrect.

My expectation is that it should only resolve when the payload is as follows:

{
  path: '/transfers',
  body: {      // <-- correct body 
      amount: '123',  // <-- Matching PATH
  },
  method: 'POST',
}

Link for reference: https://github.com/mojaloop/mojaloop-simulator/pull/120/files#r747311386

mdebarros avatar Nov 11 '21 09:11 mdebarros

It's explicitly defined behaviour (as you can see by this test), but I'd argue it's poor design.

You can see at this line: https://github.com/CacheControl/json-rules-engine/blob/master/src/almanac.js#L158 if the fact value isn't an object, then the path is essentially ignored. I'd open up a PR that this should return undefined instead, but I'm questioning whether this library is still maintained. There's been 0 activity from the maintainer of this repo since June?

akmjenkins avatar Nov 14 '21 01:11 akmjenkins

It's explicitly defined behaviour (as you can see by this test), but I'd argue it's poor design.

You can see at this line: https://github.com/CacheControl/json-rules-engine/blob/master/src/almanac.js#L158 if the fact value isn't an object, then the path is essentially ignored. I'd open up a PR that this should return undefined instead, but I'm questioning whether this library is still maintained. There's been 0 activity from the maintainer of this repo since June?

It still looks like it's being somewhat maintained.

I do see that the dependency version used by json-rules-engine is "jsonpath-plus": "^5.0.7" with the latest being v6.0.1. Perhaps it's worth first upgrading to see if jsonpath-plus functions any differently?

However, I do think your proposal for returning undefined makes sense regardless.

mdebarros avatar Nov 16 '21 11:11 mdebarros

It still looks like it's being somewhat maintained.

@CacheControl do you have an update on the status on the project? There are several issues/requests and I'm sure there are a few people who are able and willing to help contribute.

aaron-harvey avatar Dec 17 '21 14:12 aaron-harvey