Multiselect_not_equals is identical to Multiselect_equals inside "not" group in jsonlogic

[Tupsu-jy]

When making these choices as shown in the picture it results in exported jsonlogic where these two are identical: // Rule: { "and": [ { "!": { "all": [ { "var": "multicolor" }, { "in": [ { "var": "" }, [ "green", "orange" ] ] } ] } }, { "!": { "all": [ { "var": "multicolor" }, { "in": [ { "var": "" }, [ "orange" ] ] } ] } }, But when exportPreserveGroup is set to true there is however a difference between the 2: // Rule: { "and": [ { "!": { "and": [ { "all": [ { "var": "multicolor" }, { "in": [ { "var": "" }, [ "green", "orange" ] ] } ] } ] } }, { "!": { "all": [ { "var": "multicolor" }, { "in": [ { "var": "" }, [ "orange" ] ] } ] } }, Now in a way this is fine as these choices are functionally identical when it comes to the logic that they represent. How ever there is the issue that should this be first exported as jsonlogic and then later imported, then this roundtrip would see the multi_select_not_equals transform during import into multi_select_equals that is inside a "not" group. This would be confusing for user, especially if react-awesome-query-builder used in an app where this might happen rapidly and often. Also if there is a desire to not use the "not" thing (ie. showNot: false) then this is especially confusing as the user does not even see that multi_select_equals is negated. Everything said here applies to like(Contains) and not_like(Not contains) in exact same way and for exact same reason.

So, in order to fix this issue I have been considering a solution like this.

Export: When it comes time to remove single rule groups we check to see if the single rule is like, not_like, multi_select_equals or multi_select_not_equals in which case we dont remove the group. Thus we have difference between the 2 scenarios same as when exportPreserveGroup is set to true. Alternatively we could just no longer remove single rule groups at all. That would be simpler and make code bit less convoluted.

Import: Here then if we see a ! ->all or !->in we can just assume these refer to multi_select_not_equals and not_like and we can parse them accordingly. I think I have a pretty clear idea on how to do this if the concept is ok.

Again I can do this myself if there is nothing in the plan that would be problematic

[ukrbublik]

Thanks for opening this issue!

After a brief look I think this can do the trick: https://github.com/ukrbublik/react-awesome-query-builder/blob/51f6d13d7127cb142f55e85ff266cb2d590eddf1/packages/core/modules/import/jsonLogic.js#L703-L705

           || !!config.settings.exportPreserveGroups // add this

[Tupsu-jy]

Thanks for quick response again!

I dont that would be ideal solution even though it would actually work for my purposes. I have tested setting reverseOperatorsForNot=true and it did fix the issue and adding exportPreserveGroups like you suggest would solve the issue if exportPreserveGroups=true. However doing what you suggest would:

1. Cause exportPreserveGroups to also have the same effect as reverseOperatorsForNot during jsonlogic import which would be unexpected behavior
2. Would only solve the issue if exportPreserveGroups is true (i assume we want this issue not exist at all rather than just when correct settings are present)
3. Would cause similar issues with other operators if "not" group is being used (haven't actually tested this but it should be true)

Also i realize i have question. I have approached this with the assumption that export/import roundtrip for queries should not cause any meaningful or even visible changes. Is this actually a goal? It just makes sense to me so I assumed achieving that is a goal.

[ukrbublik]

I know this is a tricky issue with reversing operators. Similar issue I tried to solve with reverseOperatorsForNot = false by default: https://github.com/ukrbublik/react-awesome-query-builder/issues/1059

I think the only way to have same result for import-export roundtrip is to have options exportPreserveGroups: true, reverseOperatorsForNot: false (probably should be default behavior).

Then we can distinguish ! -> and -> all and ! -> all during import (as in your example in first post with exportPreserveGroups = true). Otherwise (without exportPreserveGroups) the export results for both cases in your example would be identical as you noted, so we can't distinguish such cases during import.

Is this actually a goal? It just makes sense to me so I assumed achieving that is a goal

Right.

[ukrbublik]

Import: Here then if we see a ! ->all or !->in we can just assume these refer to multi_select_not_equals and not_like and we can parse them accordingly

Variable canRev in line 703 decides weather we can make this assumption.

[Tupsu-jy]

Otherwise (without exportPreserveGroups) the export results for both cases in your example would be identical as you noted, so we can't distinguish such cases during import.

Right but in my suggestion I proposed changes to this so that there would always be a way to distinguish. Either removing the check that removes single rule groups completely because it does not seem very important to have that and its going to create problems or adding some extra checks so that it never removes these groups in cases where it makes it impossible to distiguish between these 2 scenarios. In essence i want to make it so that during export and for these problematic cases it always behaves as if exportPreserveGroups=true.

I don't think some way to distinguish them existing should be dependant on anything. It is inevitably going to cause problems if with any setting these 2 operators are allowed to be identical.

Also by making it so that them being exported as identical is always, always completely impossible means that we can create a solution for importing that also always works regardless of settings.

[Tupsu-jy]

Import: Here then if we see a ! ->all or !->in we can just assume these refer to multi_select_not_equals and not_like and we can parse them accordingly

Variable canRev in line 703 decides weather we can make this assumption.

And im saying it should not and that im making a solution where its not necessary for it to determine it.

[Tupsu-jy]

Actually what I just wanted to ask was if you would be okay with me creating a changes to code to make it so that from now on:

1. When exporting then from this change onwards !->all->in is always going to refer to multiselect_not_equals, no matter the settings. (same for not_like)
2. Some one rule groups are going to be mandatory no matter the settings. Because that way can always distinguish between !->all->in (=multiselect_not_equals) and !->and/or->all->in (=multiselect_equals inside one rule negation group)
3. During import we will then be able to and will always treat !->all->in as multiselect_not_equals no matter the settings. (same for not_like)

This was my basic plan and i just wanted to know if there is some issue you have with it. I just wanted to check if the pull request following this plan would be accepted before working/trying too much :D

Entirely possible there is some problem with the plan but, i dont yet see but at the moment only downside i see is that there has to be these logically meaningless one rule groups.

[ukrbublik]

I think your plan makes sense. I can't think of possible issues with it right now. Do I understand correctly, that your implementation would involve changing logic for assigning shouldPreserveGroups value during export? Like if group has 1 rule and group has "not" flag then group should be preserved

It can be done for any operator or only ops like multiselect_equals that can't be safely reversed:

let operatorDefinition = getOperatorConfig(config, operator, field);
let reversedOp = operatorDefinition?.reversedOp;
  let revOperatorDefinition = getOperatorConfig(config, reversedOp, field);
const opCanReverse = !!revOperatorDefinition?.jsonLogic;
// If !opCanReverse and it's only rule in group with "not" then always preserve

[Tupsu-jy]

Hi again. Hope you had a nice weekend :D

I have this in working condition now, but export works because i just made it so that single rule groups are not removed at all. I almost feel like this should be fine as is because removing these single rule groups does not feel that beneficial and doing it like this would reduce complexity in the code. I'll look into solving the export properly next.

The general idea is that these single rule groups would be left only in cases where they are necessary to maintain the ability to distinguish operators, so not_like, multiselect_not_contains and select_not_any_in. I'm not completely sure how I will change export exactly but that is the general idea.

[Tupsu-jy]

Hello again.

I have been working on this and I have a working solution. I actually had a working solution some time ago already. However I got kind of carried away while working on this.

The basic problem here is that for some operators "!" is part of the operators jsonlogic definition, but code does not really account for that. One simpler solution to this is just adding a bunch of special checks for all of these cases that then deal with them accordingly. I feel like this solution would have been fragile, prone to bugs and would have added complexity to already complex code (especially import). This was my initial solution but i was not happy with it.

The second imo better solution, that i have done now, is to use jsonlogic definitions found in config (ie jsonlogic:s that are functions). I added a bunch of functions as "jsonlogic" in default operators, for the more complex operators, config and then i use these during import to match against jsonlogic that is being imported. This allows for avoiding of a lot of complex special logic, both when it comes to recognizing these operators and getting the fields and values from them and into correct place. Both of these things (matching operator, and getting values) are done by a function i created simultaniusly. What i have done so far could be expanded to make jsonlogic import/export a lot simpler. Basically we could replace a lot of hardcoded solutions for each situation with a single dynamic solution that works for all. All the stuff about arity, vals being taken from and put to spesific indexes, reverseArgs, could be replaced. This could also theoretically enable complex custom jsonlogic configs.

Currently I have only applied this partially for some operators but it could be rather easily expanded to cover all operators. How do you feel about expanding this solution to cover all operators for jsonlogic import/export?

Additional issues that came up: when going thru the tests after making my solution I think i found out that spell import has identical issues as jsonlogic. Meaning that when operator includes negation it is not handled properly.

I cant really work on this until monday again, but there isnt really much of anything left to do before i can make a pull request.

[ukrbublik]

How do you feel about expanding this solution to cover all operators for jsonlogic import/export?

The second solution sounds great! Thanks for thinking of this issue thoroughly.