jsonapi-suite / jsonapi_compliable

MIT License
20 stars 35 forks source link

Allow escaped string filters #102

Closed richmolj closed 6 years ago

richmolj commented 6 years ago

Let's say you have a keywords filter where users may put commas in the input. Prior to this commit, we would interpret that comma as a delimiter and treat this incorrectly as an array of multiple values.

You can now escape values to ensure the desired behavior:

?filter[keywords]={{foo,bar}}

Would end up as .where(keywords: "foo,bar")

This works with arrays:

?filter[keywords]={{foo,bar}},baz,{{another,thing}}

Would end up as .where(keywords: ["foo,bar", "baz", "another,thing"])

Originally, quoting the string was considered. However, for this use case it would be common for users to type quotes, and then you'd have to escape the quotes. Using double-curlies keeps things simple and without conflict.

richmolj commented 6 years ago

@wadetandy

wadetandy commented 6 years ago

While the need for the feature is 100% there, I'm not convinced this is the right solution. I feel like there's always going to be exceptions to any special quoting character we put in place, and clients shouldn't have to handle that escaping logic.

I think it's much more straightforward to support "simple" and "complex" versions of the field. The "simple" field would be what we have now: split on commas and that's your array of items. The complex version would be a JSON.stringifyed array of the items, URL encoded. This would allow for all clients to use built-in functionality for escaping more advanced values. On the server side, we could either use some heuristics to detect the correct starting characters to decide whether to parse as json, or naively parse and rescue back to the simple version.

I think the version you have here solved most of the problems that you and I encounter in our everyday use of this feature, but thinking about a set of generally applicable recommendations for -suite, I think relying on a standard format like json is the correct way to go.

richmolj commented 6 years ago

I'm not sure why they are mutually exclusive? We agree there's a simple and a complex way to do things. Allowing commas in the simple syntax seems like low-hanging fruit.

always going to be exceptions to any special quoting character we put in place

Then you can use the complex version. This seems to accommodate 99.99% of scenarios at very little cost though.

IOW, we shouldn't support comma-arrays at all if we follow the path you propose. If we're going to support simple comma-arrays, it seems correct to allow escaping commas.

wadetandy commented 6 years ago

Fine with this as long as this isn't our only eventual escaping solution.