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

"fixing" jsonLogic.is_logic method. #51

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

jabiinfante
Copy link

First of all, thanks for this great module. I have found it really usefull.

I have decided to submit this pull request, because we have had some strange behaviour handling objects:

Having:

jsonLogic.apply("hello"); // "hello" 🆗

and

jsonLogic.apply({"item1":2, "item2":2}); // {"item1":2, "item2":2} 🆗

I would expect:

jsonLogic.apply({"item": 22}); // {"item":22} ❎

and also:

jsonLogic.apply({"if": [true, {"item": 10}, "no"]}), // {"item": 10} ❎

Which is not happening because of jsonLogic.is_logic treats every single item object as a logic rule.

So I have checked the operations collection and have made a whitelsit ("asymetricOperationsNames") containing every valid operation.

I have had to change a few tests, since we are not getting as many "Unrecognized operation" as before.

(and also, my editor is responsible for the linting)

Maybe adding a runtime toggle (jsonLogic.smartOperationResolve = true) would be a more conservative approach (and backwards compatible).

any thoughts?

@germanamz
Copy link

@jabiinfante did you found a solution?

@jabiinfante we will love to make a merge of your fix with our changes.

@jabiinfante
Copy link
Author

sorry, @germanamz but i don't know what you mean....
My PR already fixes the issue described on the comment with jsonLogic.is_logic

But it may break a few use cases so it's not 100% backwards compatible... I have had to change a few tests.

Allthought i think those use cases might be wrong... so i have suggested that maybe a runtime toggle can be added it the user needs compatibility with objects when Object.keys(object).length === 1... but i think the library mantainers should make that decission.

@josephdpurcell
Copy link

I'm glad I found this PR! I think I ran into something similar.

The PR was hard to read due to all the code style changes. But, I think the key changes in the PR are these: https://github.com/jwadhams/json-logic-js/pull/51/files#diff-9f6a5a68bd112cdd9fc270b72abdf7addf40ce02b6e26a036012ec0b820d2a4bR192-R205

I presume the core issue here is that is_logic is a little ambiguous? I thought that is_logic meant "is a valid JSON Logic operation (object)". Which is... kind of true.

It's true in the sense that JSON Logic is very opinionated. If something is an object (and not null and not an array) and has only 1 key, then it is going to be interpreted as a JSON Logic. And, an error will be thrown if its invalid.

But, I think that's a little misleading, because it doesn't also check if the operation in the logic is valid.

The changes in this PR would be breaking, as noted. So, perhaps what could be done instead is adding a new method called is_operation that takes 1 parameter that is the operation name. It would return true if the operation is a known operation, else false. I recently had a use case for a is_operation method, which I documented #116 (comment).

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.

3 participants