Exception thrown when deserializing from JSON into FilterRule when value is an array

[cbrianball]

This was talked about in issue #7 , but the solution provided there was to change the JSON before it is sent to the server. This is less than ideal for a couple of reasons:

1. I view the rules as a "document" and I should never need to inspect or modify. I rely on the query builder control to create/edit the rules and your package to execute them.
2. Converting from an array to a comma delimited list can cause trouble when converting back if the source values have commas in them (and you have to make the assumption that the string is meant to be converted back to an array).

I'd be willing to look into submitting a PR to fix this issue, but I'm not sure I can do it without a breaking change to the API. Also, I am unsure of the other expected uses of this library and if that could have a negative impact to those users.

Thoughts?

[tghamm]

@cbrianball I don't really disagree with your assessment of the limitations of a client-side fix to the solution, however, I'd be weary of breaking changes to the API at this point. There are a decent number of people using this solution as-is, and I'm not sure they'll all want to make the necessary changes to comply. If there were a way that they could live side-by-side I'd be happy to accept a PR that handles both cases though. In theory I think it's possible, it's really just a matter of taking the Value property in as an object instead of a string (on FilterRule), which shouldn't break anything outright in the serialization (I don't think at least), and then parsing the object appropriately as either a comma delimited list or an array. Most of that is handled by the GetConstants function, so other than Value being an object instead of a string, you wouldn't have to change all that many places to get it to play nice.

Tl;dr; - If we can make both solutions work, I'm happy to accept a PR. If it requires breaking changes that will require developers to adjust, then I'd say I'm not sure the positives of the change outweigh the negatives in the eyes of the current users. If you want to take a stab at it, I'm happy to be helpful in any way I can.

Let me know your thoughts, or if I can be helpful in any way. Thanks for your input, it's appreciated!

[cbrianball]

@tghamm I agree that the only change needed in the contract is to change the string to an object and ensure the conversion to the lambda still works, but in my mind, that is a breaking change (if someone had written code to call Substring on Value, then when they updated the package, their code would no longer compile.

I have a couple ideas that may work -- if i get time to look into it, I'll let you know what I come up with.

[tghamm]

@cbrianball yeah that's true. My sense it there's a pretty low number of people doing that kind of server-side manipulation of the filter, but to be honest, I don't actually know, so you're right, that is technically a breaking change.

Do let me know if you come up with anything, I'd certainly be interested.

[cbrianball]

@tghamm I've started work in my fork to resolve this issue. At a high level these are the steps I'm taking:

• Creating an IFilterRule interface that closely resembles the existing FilterRule class (with the main exception being the interface expects an object for the Value property)
• Have the FilterRule class implement the new interface
• Creating a new class called QueryBuilderFilterRule that also implements the new interface
• Updating the various methods in QueryBuilder to take an IFilterRule instead
• Updating the logic that builds the expression to work with an object instead of a string.
  • The logic only differs if Value is an IEnumerable and is NOT a string

For the unit tests, I've essentially copied all the tests you had related to FilterRule and am changing the copied tests over to use QueryBuilderFilterRule, and updating/adding tests as necessary.

Doing that last part will take some time (no easy find/replace), but I wanted you to look at what I've done so far and see if you like the direction I'm taking.

Net out, consumers of your library should be able to update to the latest code and not have to change anything and it should work the same (they will still be using the FilterRule class), but if they need the ability to handle multiple values for the Value property without needing to split/join strings, then they can update their code to use the new QueryBuilderFilterRule class instead and they've essentially 'opted-in' to the new functionality.

Here's the fork:
https://github.com/cbrianball/dynamic-linq-query-builder

[tghamm]

@cbrianball Took a look at the changes, I like the approach. Using an explicit interface implementation likely would not have occurred to me, very clever :)

In addition to the unit tests, it'd be cool to update the sample project to use the new implementation as this is the de-facto superior approach. If you don't have time for that just let me know when you submit the PR and I can manage that update myself, up to you.

This seems like a pretty viable approach and definitely adds value to the project. When you're ready for a PR, we'll test and accept it.

Your help is much appreciated.

[cbrianball]

@tghamm I tried to update the sample application, but I ran into an issue. Through all of my testing, I was using the Newtonsoft.Json library to serialize/deserialize. The sample application, I believe, uses the JavaScriptSerializer (which is the default for that version of ASP.NET MVC). That serializer works differently and does not put any data into the Value field for the newer IFilterRule type, it simply news up an object and sets it on the property.

If I manually do the deserialization myself using the Newtonsoft.Json library in the controller method, everything works as expected.

I'm not thrilled with the prospect of having the newer solution work with only a specific library. On one hand, Newtonsoft.Json is pretty standard, but on the other hand, I believe MSFT is planning to move away from that library in ASP.NET Core MVC 3.

Thoughts/ideas?

[tghamm]

@cbrianball Yeah, yikes. I didn't consider that. I just spent the last hour or so playing around with some options. It's pretty restrictive. Not only does the JavaScriptSerializer not support the object type, but other serialization libraries fall short as well. I tried Jil, which I use in several projects, and while it does more than the JavaScriptSerializer, it still ultimately fails because of the way it boxes the types. Looks like JSON.Net is the only viable deserializer. I'm honestly not sure where to go from here, as I'd consider this dependency more intrusive than the original problem trying to be solved. Getting opinionated about how things get serialized is not ideal at all. I'm open to ideas and will continue to mess around with it, but like I said, not real sure where to go from here.

[tghamm]

@cbrianball one more thought. This idea only solves one of your original problems, but maybe it's still a step forward.

Instead of trying to stuff an array into an object and depending on it being deserialized properly, what if we made Value a string[] in the new type and used client side js to modify non-array entries to be arrays, so they are consistently arrays. This violates the tenant of the jQuery Querybuilder rules being a document you shouldn't have to modify, but it does correct the potential edge case of using a comma in one of your rules and that causing a break.

Thoughts?

[cbrianball]

@tghamm Yeah, your suggestion to always make it an array is better than the comma delimited list. I was thinking on it some more and maybe this library should be more in control of HOW the JSON is serialized/deserialized. Instead of trying to make it work with all the various libraries, maybe users shoudl take the JSON string and pass it into the a method and it would do the deserializing for them.

I'm not suggesting that we write code to the do the parsing -- that's too error prone and the problem has been solved before.

I like the fact that this library currently doesn't have any dependencies. If we want to keep it that way, we can pick a library and look into using IL merge. I haven't used that tool much, but ideally I'd like to see if there's a way to use it such that the other library is no longer visible to consumers of this library (i.e. if we went with Newtonsoft.Json, consumers of this library wouldn't see the Newtonsoft namespace nor any of its exported types.

If that doesn't pan out, we could take a dependency on System.Json (which is available for 4.5 and .NETStandard 2.0, the two current libraries you support). It'd be a little more code, but we wouldn't be doing the actual parsing.

Those who are currently already using the library could continue to deserialize directly into FilterRule like they currently are, so we wouldn't be breaking backwards compatibility.

Thoughts?

[tghamm]

@cbrianball I'm struggling with this one a little, admittedly. I'm pretty reluctant for the library to take on serialization/deserialization duties. It just seems pretty far out of scope for the library, especially when the underlying reason is that the generated rule structure has no business being modified (I only sort of agree with this tenant). To me the direction we were taking made more sense in the original context where the simple comma-delimiting could legitimately cause bugged behavior if an errant comma were to be introduced into a value. That said, if we're pitted between a complicated relationship with serialization/deserialization and simply modifying value client side to always be an array, I feel like the latter is something users would be more comfortable with overall, as exhibited by them already modifying the rules client-side, just in an inferior way.

In terms of a path forward, here's what I'm thinking, let me know your thoughts:

• The interface and using explicit implementations have opened the door to all proposed solutions essentially. Using the interface you have 3 options for running your query:
  • Use the old FilterRule and client side manipulation of the arrays to a comma delimited list, as many are doing presently. This obviously works, but technically is capable of producing a bug.
  • Use the QueryBuilderFilterRule coupled with a serialization framework that can gracefully handle the object type, which, after testing, appears to only be Newtonsoft.Json. This works so long as you're using that serializer, and avoids the bug, and avoids any manipulation of the rules, if that's important to you.
  • Use a new type based on IFilterRule that implements Value as a string[] and requires a new version of client-side manipulation to force value to always be an array. This modifies the client-side value, but works without any modification or dependencies at the serialization/deserialization layer, and is bug-free.
• If we implement the 3 classes, I'd propose making the string[] solution the sample solution simply because it's the simplest, with the least amount of dependencies and exhibits bug-free behavior. At the same time, we can create an issue in the wiki that offers the other implementation strategy if you're using a compatible serialization library.
• This approach essentially let's people do what they want without us having to get prescriptive about their implementation, so it sort of feels like all use cases win.

If you agree, then the string[] implementation will require some additional code in GetConstants, basically just checking for an unboxing the array in cases not currently being checked for in your branch.
Then the sample project would need to be updated to use this method, along with the client-side helper:

var convertToArray = function (obj) {
            if (obj != null) {
                if (obj.hasOwnProperty("value")) {
                    if (Object.prototype.toString.call(obj.value) !== '[object Array]') {
                        obj.value = [obj.value];
                    }
                }
                if (obj.hasOwnProperty("rules") && obj.rules != null) {
                    for (var i = 0; i < obj.rules.length; i++) {
                        convertToArray(obj.rules[i]);
                    }
                }
            }
        }

Let me know if you're interested in rounding out the implementation this way, if not, I'm happy to take a PR from you as the starting point and finish it myself, as your preferred implementation essentially already works.

[cbrianball]

@tghamm Sounds good to me. I'll try to find time this week to work on it. Do you have preferences for the class names that more accurately describe their intended use (the two implementations of IFilterRule other than FilterRule)?

[tghamm]

@cbrianball awesome, thanks for working through this with me. Good question on the class names, maybe the string[] implementation can become the QueryBuilderFilterRule, and the object implementation can become something else. I don't have a strong preference, we could either do something like ObjectFilterRule, or JSONNetFilterRule or something else entirely, I'm pretty amenable. My only request would be that it's not so similar to QueryBuilderFilterRule that intellisense makes it easy to confuse the two.

I'd also suggest we do the following:

• Mark FilterRule as Obsolete with an explanation for the preference of using the newer string[] variant or the object variant.
• Annotate both of the new classes with either a description of their respective purposes, or maybe just a see wiki for more kind of thing.

Thanks again for the help, and let me know if I can be helpful any other way!

[cbrianball]

@tghamm I created a PR (#42), Please let me know if you see any issues.

[tghamm]

@cbrianball thanks! I'm out of town for a couple of days but will review and aim to release a new version late next week. I really appreciate your contribution, and from what I've seen so far, looks great!

[oguzhankahyaoglu]

Hey there, we are facing the same problem. Do you have any plan date about releasing the fix?

[tghamm]

@oguzhankahyaoglu aiming to release a new version in the next couple of days, I've been out of town.

[tghamm]

@cbrianball the new release is out. Thanks for all your help!

@oguzhankahyaoglu see above, new release is out.

[oguzhankahyaoglu]

@cbrianball the new release is out. Thanks for all your help!

@oguzhankahyaoglu see above, new release is out.

Appreciate your efforts, but I quit my job so it does not help for the moment :) Anyway, this is one of the libraries that I kept in my mind and hope you spread it much more...