Guarantee 'AND', 'OR', and 'NOT' filter fields get evaluated last by …

[TWeidi]

…sorting 'filters.__strawberry_definition__.fields'; fixed issue #422;

Description

Types of Changes

• [ ] Core
• [x] Bugfix
• [ ] New feature
• [ ] Enhancement/optimization
• [ ] Documentation

Issues Fixed or Closed by This PR

422

Checklist

• [x] My code follows the code style of this project.
• [ ] My change requires a change to the documentation.
• [ ] I have updated the documentation accordingly.
• [ ] I have read the CONTRIBUTING document.
• [ ] I have added tests to cover my changes.
• [x] I have tested the changes and verified that they work and don't break anything (as well as I can manage).

[bellini666]

It would be nice to have a test that would break without this change, not only to evaluate that this change is in fact fixing the issue, but also to avoid regressions in the future

[codecov-commenter]

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Comparison is base (c7f4643) 87.27% compared to head (6eda3e1) 87.27%. Report is 3 commits behind head on main.

Additional details and impacted files

```diff @@ Coverage Diff @@ ## main #424 +/- ## ======================================= Coverage 87.27% 87.27% ======================================= Files 37 37 Lines 3119 3119 ======================================= Hits 2722 2722 Misses 397 397 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

[TWeidi]

Ok, I'll try to add the two test cases :)

[TWeidi]

@bellini666:

I'm having a hard time using the debugger in VSCode to debug into the tests. My breakpoints are are outright ignored when i try to run a test in debug mode. Do i have to provide specific settings to correctly enable debugging?

[bellini666]

@TWeidi not sure TBH as I don't use VSCode and I'm also a print debugger dev =P

@patrick91 do you have any knowledge to share regarding that?

[TWeidi]

Hey guys,

Regarding test debugging in VSCode

I found this article, which states that debugging tests in vscode does not work with pytest's --cov option, so commenting out the line starting with 'addopts' in pyproject.toml led to test debugging working again (just in case someone else encounters the same issue).

Regarding the requested test

The test test_resolver_filter_with_inheritance in _tests/filters/testfilters.py fulfills your requirement to fail with the current version but succeeds with the proposed fix.

I added another test (test_filter_field_order_with_inheritance) in tests/filters/test_types.py just to show, that the order of fields of a filter class depends on class inheritance. This test will fail for both the original and the fixed version and is not supposed to be included in case this PR is merged. Maybe it would be a better solution to fix the issue by defining a base Filter class with a dedicated __init_subclass__() method that takes care about the order of filter fields and which must be subclassed by every filter, just in case the fields order is relevant in other parts of the code as well?

[TWeidi]

Do you want me to remove the failing test (it was just to show you the issue arising from the fields order) or will you do that after investigating it further?

[bellini666]

Do you want me to remove the failing test (it was just to show you the issue arising from the fields order) or will you do that after investigating it further?

If you can modify it in a way that it would break without this changes but passes with them is better :)

[TWeidi]

Ok, I looked into this and I would rather drop the failing test. The mixed up order is not a wrong behavior, but the one expected from the typing module:

The order of the fields in all of the generated methods is the order in which they appear in the class definition. (1)

So, the better approach to the issue would be to not rely at all on the order of fields in the build_filter_kwargs function in filters.py. Unfortunately, I don't know your library well enough to dare to rewrite that function myself. As a quickfix I guess this PR will do, though.

[bellini666]

@TWeidi nice, this LGTM

Do you want to remove it from draft? Then I can merge it

edit: took the liberty to remove it myself :)