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

Better approach (in my opinion) to AND/OR junctions #198

Closed
wants to merge 11 commits into from

Conversation

franklx
Copy link

@franklx franklx commented May 2, 2013

The junction mechanism doesn't solve the nested and/or problem.

Consider the query:

SELECT *
FROM table
WHERE fld1='val1' 
  AND (fld2='val2' OR fld3='val3' OR (val4='val4' AND val5='val5')) 
  AND fld6='val6';

The junction mechanism doesn't provide syntax to write it like a json filter.
My method looks like an hack (because it's not intuitive) but works very well.
It uses lists to group and apply the correct conjunction/disjunction.
My implementation allow the WHERE part of the query to be written as:

{
    "filters": [
        {"name":"fld1", "op":"eq", "val":"val1"},
        [
            {"name":"fld2", "op":"eq", "val":"val2"},
            {"name":"fld3", "op":"eq", "val":"val3"},
            [
                {"name":"fld4", "op":"eq", "val":"val4"},
                {"name":"fld5", "op":"eq", "val":"val5"}
            ]
        ],
        {"name":"fld6", "op":"eq", "val":"val6"}
    ]
}

As you can see the or / and junction is toggled on every new level.

If you are interested in this approach I will write documentation and tests and submit and more clean pull request.

@jfinkels
Copy link
Owner

jfinkels commented May 8, 2013

Thanks for the suggestions! I am not totally on board with this for the following reasons.

  1. It is not obvious from reading the query string that there are implicit alternating conjunctions and disjunctions.
  2. What if I want a disjunction at the top level? Would you have just a top-level list with a single child list as its only element? I don't think that's clear either.
  3. The purpose of this project is not (necessarily) to provide an all-powerful search interface; it is to provide simple API generation for basic use cases. I fear that allowing this may be a slippery slope; will the next pull request add more complicated search functionality?

However, I am open to persuasive arguments.

@franklx
Copy link
Author

franklx commented May 14, 2013

Fine. :-)

  1. Yes, I know. An option I thought about was integrating my implementation with yours (specifying the disjunction flag will obviously affect the outermost retaining inner filters).
  2. The base idea was to use a top-level with a single child; quite hackish and not really clear.
  3. I used your module for populating an ajax data-table and it's been very convenient but I immediately faced the problem to enable some inclusive and exclusive filters.
    I think that this feature completes your module - not being a simple "additional feature".
    Implementing the dual behavior (leaving your modification as-is and adding mine) should cover any filter requirement - unless the programmer needs to query an 12-dimensions OLAP cube stored in an MSAccess DB . :-)

Obviously if you approve this I will write code, documentation and tests needed to support it - and documentation can include a note regarding the "search feature being complete for the scope of the module".

Whatever you decide thank you for your reply!

@jfinkels
Copy link
Owner

I understand the value of a response that includes only the data that the client requires (fewer bits need to be transported), but I'm still not convinced that this belongs in Flask-Restless. I'm going to leave this pull request open for a while, and if many other people have the same issue, then I'll pull in the changes.

@franklx
Copy link
Author

franklx commented May 16, 2013

Allright.
Thank you for reviewing my pull request.

@joostdevries
Copy link
Contributor

I actually have a related issue: I need to filter based on two columns in a related object, which for as far as I can see is not possible using the current options. So either this solution or allowing something like below would work for me. (I can also open up a PR for the solution below if appreciated).

GET /object?q={"filters":[{"name":"related_object","op":"eq","val":[{"name":"related_col","op":"eq","val":"val1"},{"name":"related_col_2","op":"eq","val":"val2"}]}]} HTTP/1.1
Host: example.com

@synergetic
Copy link

I think nested and/or will help very much. Not sure about frankix' approach, though.

@christophervalles
Copy link

I'm facing this very same problem so having a way to specify an OR operator for just a few filters and use AND for the rest would be great. The alternation between AND/OR for each level feels weird to me but as long as it's well documented it's fine for me... I will apply the patch on my code until this is PR is merged.

I agree that this PR also completes the search functionality giving advance features for people who's building a pretty complex set of filters.

@christophervalles
Copy link

@franklx, @jfinkels, @synergetic: I've been thinking and maybe a syntax like the following would be easier:

GET /api/car?q={"filters":[
    {"name":"built","op":"ge","val":2004}, 
    {"op":"or","val":[
        [{"name":"brand","op":"eq","val":2},{"name":"model","op":"eq","val":21}],     
        [{"name":"brand","op":"eq","val":3},{"name":"model","op":"eq","val":11}]
    ]}
]}

The code above would generate a query like this:

WHERE built >= 2004 AND ((brand = 2 AND model = 21) OR (brand = 3 AND model = 11))

Things can get more standardised, flexible (and also complicated) like this:

GET /api/car?q={"filters":[
    {"op":"and","val":[
        {"name":"built","op":"ge","val":2004}, 
        {"op":"or","val":[
            [{"name":"brand","op":"eq","val":2},{"name":"model","op":"eq","val":21}],     
            [{"name":"brand","op":"eq","val":3},{"name":"model","op":"eq","val":11}]
        ]}
    ]}
]}

What do you guys think?

@franklx
Copy link
Author

franklx commented May 4, 2014

I'd prefer the first solution by @christophervalles being concise and retains the original flask-restless concept that the and operator is implicit (you always use "and" in queries).

To be fully expressive the second example should be written as:

GET /api/car?q={"filters":[
    {"op":"and","val":[
        {"name":"built","op":"ge","val":2004}, 
        {"op":"or","val":[
            {"op":"and","val":[
                {"name":"brand","op":"eq","val":2},
                {"name":"model","op":"eq","val":21}
            ]},
            {"op":"and","val":[
                {"name":"brand","op":"eq","val":3},
                {"name":"model","op":"eq","val":11}
            ]}
        ]}
    ]}
]}

that in my opinion is awfully cumbersome.

@christophervalles
Copy link

@franklx Yes, the fully expressive option might be too much so, if we stick with the idea of the AND operator being implicit the problem goes down to implement the new OR operator and we end up with a pretty "clean" way of dealing with this issue.

@jfinkels
Copy link
Owner

Can someone check out how other similar projects solve this problem? Here's just a few: https://flask-restless.readthedocs.org/en/latest/similarprojects.html

@madvas
Copy link

madvas commented Feb 9, 2015

is this resolved? This feature would help me a lot

jfinkels added a commit that referenced this pull request Feb 9, 2015
Before, filters in search queries were joined using only conjunction or
only disjunction, as controlled by the `disjunction` parameter. Now we
allow arbitrary Boolean expressions of filter objects. By default, if no
ANDs or ORs are specified, the filters are assumed to be a conjunction.

This supercedes pull request #198 and fixes it in a different way.
@jfinkels
Copy link
Owner

jfinkels commented Feb 9, 2015

I just committed a change to the code that will allow search queries to have arbitrary Boolean formulas consisting of filter objects. For example:

{'filters':
                [{'or':
                      [{'and':
                            [dict(name='name', op='like', val='%y%'),
                             dict(name='age', op='ge', val=10)]},
                       dict(name='name', op='eq', val='John')
                   ]
                  }]
            }

This supercedes this pull request by implementing the requested change in a slightly different way.

This feature will appear in the next tagged release, which I will publish soon.

@jfinkels jfinkels closed this Feb 9, 2015
@madvas
Copy link

madvas commented Feb 9, 2015

Sounds good, thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants