Bugfix when chaining expressions using OR

[developer992]

• Wraps expressions in brackets when using more than one expression so this /Employees/$count?$filter=%28ID%20eq%2023%20and%20NickName%20eq%20%27Steve%27%29%20or%20%28ID%20eq%2025%20and%20NickName%20eq%20%27Tim%27%29

(ID eq 23 and NickName eq 'Steve') or (ID eq 25 and NickName eq 'Tim')

becomes /Employees/$count?$filter=%28%28ID%20eq%2023%29%20and%20%28NickName%20eq%20%27Steve%27%29%29%20or%20%28%28ID%20eq%2025%29%20and%20%28NickName%20eq%20%27Tim%27%29%29

((ID eq 23) and (NickName eq 'Steve')) or ((ID eq 25) and (NickName eq 'Tim'))

• adds a test
• fixes existing tests
• addresses issue https://github.com/SAP/python-pyodata/issues/280

Example 2: consider this example, it demonstrates a bit better

startswith(NameFirst, 'Steve') eq true and ID eq 23 or ID eq 25 or ID eq 28 becomes (startswith(NameFirst, 'Steve') eq true) and (ID eq 23 or ID eq 25 or ID eq 28)

[cla-assistant[bot]]

All committers have signed the CLA.

[developer992]

Hi,

couple of notes

I couldn't get pylint to work, i get error:

AttributeError: module 'collections' has no attribute 'MutableMapping'

Must be some version mismatch but I couldn't (yet) track it down.

I added a test and fixed a few existing ones but i think these tests aren't the best. These basically just verify the constructed URL is correct and not actually test the remote service.

There could be a better way for this using pytest-vcr which does exactly that. Test remote services and their responses. But this would require a bit more effort to implement, if you all want to go this route.

Anyway, i probably forgot other things, let me know what you guys think.

We've been using "our" version for a little while now ( few weels ) and i've had no complaints so I think it works ok.

[phanak-sap]

Hi @developer992

Thanks for all your work. Greatly appreciated.

1. The linter was probably local problem, even that the version is fixed in the dev-requirements.txt, but it could be affected by other things (python version if nothing else). It passed in the PR actions build, so no worries.

2. For this PR, new test code works, but I would like to return to the point 4 from my comment in the original issue - https://github.com/SAP/python-pyodata/issues/280#issuecomment-2276607288. It is basically the same problem that the test is passing, because the fix matches the hardcoded URL in the new test.. but is this one URL the only valid for the chained expression in the python code?

from the issue comment:

filter1 eq 'foo' and ( filter2 eq true or filter3 eq 'bar' )
or
(filter1 eq 'foo' and  filter2 eq true) or filter3 eq 'bar' 

both should most probably return different set of data in the response.

You responded in the middle with the but we'll know more after running the tests :) But there is just one quite simple test added.

I am not sure if you are covering all possible groupings or pre-selected one grouping only for the ORM style - and any other complex filter query would have to be written directly in the standard pyodata way.

This is potentially breaking change, so if you are using the fixed version from your fork for several weeks, may I ask to add few more tests for these more complex chained expression that would use the experience you gained? This is moving in the right direction overall and I would love to merge it and release it - I have planned it for the next bugfix release. So this is not comment discarding the PR, on the contrary - request for improving a bit more.

3. You may also consider improving the documentation (this PR or perhaps second one, after this is merged) with your experience, not just tests. As you already know, the PR #113 is a community feature and not what was considered "good enough minimum " pyodata in the start, so if you see benefit in writing queries this ORM way, you can "clean the path" for anybody following you. https://github.com/SAP/python-pyodata/blob/master/docs/usage/querying.rst#get-entities-matching-a-complex-filter-in-orm-style

4. small detail, can do myself - you may update CHANGELOG.md with your fix and your name under https://github.com/SAP/python-pyodata/blob/master/CHANGELOG.md. It is your fix after all.