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

Add new filtering API endpoint #1061

Merged
merged 17 commits into from
Feb 16, 2022
Merged

Add new filtering API endpoint #1061

merged 17 commits into from
Feb 16, 2022

Conversation

dmos62
Copy link
Contributor

@dmos62 dmos62 commented Feb 10, 2022

Adds the new filtering API endpoint /api/ui/v0/databases/1/filters/; but, does not replace the current filtering API: that will come in a subsequent PR. Includes the foundational logic that introduces the new endpoint and declares through it what the valid filters are for which Mathesar types.

This PR can be seen as adding the read aspect of the new filtering API, while the next one will add the write aspect (i.e. actually filtering with it).

This filtering API revamp is split into multiple PRs so as to make reviewing easier.

Also, introduces filters for the Mathesar number type (#385).

Technical details

The new endpoint's response looks like this:

GET /api/ui/v0/databases/1/filters/

[
    {
        "id": "empty",
        "name": "is empty",
        "parameters": [
            {
                "mathesar_types": [
                    "boolean",
                    "datetime",
                    "duration",
                    "money",
                    "number",
                    "text",
                    "other"
                ]
            }
        ]
    },
    {
        "id": "equal",
        "name": "is equal to",
        "parameters": [
            {
                "mathesar_types": [
                    "boolean",
                    "datetime",
                    "duration",
                    "money",
                    "number",
                    "text",
                    "other"
                ]
            },
            {
                "mathesar_types": [
                    "boolean",
                    "datetime",
                    "duration",
                    "money",
                    "number",
                    "text",
                    "other"
                ]
            }
        ]
    },
    {
        "id": "greater",
        "name": "is greater than",
        "parameters": [
            {
                "mathesar_types": [
                    "number"
                ]
            },
            {
                "mathesar_types": [
                    "number"
                ]
            }
        ]
    },
    {
        "id": "lesser",
        "name": "is lesser than",
        "parameters": [
            {
                "mathesar_types": [
                    "number"
                ]
            },
            {
                "mathesar_types": [
                    "number"
                ]
            }
        ]
    },
    {
        "id": "starts_with",
        "name": "starts with",
        "parameters": [
            {
                "mathesar_types": [
                    "text"
                ]
            },
            {
                "mathesar_types": [
                    "text"
                ]
            }
        ]
    },
    {
        "id": "to_lowercase",
        "name": "to lowercase",
        "parameters": [
            {
                "mathesar_types": [
                    "text"
                ]
            }
        ]
    },
    {
        "id": "greater_or_equal",
        "name": "is greater or equal to",
        "parameters": [
            {
                "mathesar_types": [
                    "number"
                ]
            },
            {
                "mathesar_types": [
                    "number"
                ]
            }
        ]
    },
    {
        "id": "lesser_or_equal",
        "name": "is lesser or equal to",
        "parameters": [
            {
                "mathesar_types": [
                    "number"
                ]
            },
            {
                "mathesar_types": [
                    "number"
                ]
            }
        ]
    }
]

To single out one of the filters:

 {
        "id": "greater",
        "name": "is greater than",
        "parameters": [
            {
                "mathesar_types": [
                    "number"
                ]
            },
            {
                "mathesar_types": [
                    "number"
                ]
            }
        ]
    },

This says that the is greater than filter takes two parameters and both of them should be Mathesar numbers.

Things to note:

  • API includes redundant filters like lesser_or_equal (combination of lesser and equal);
    • implementation solved relatively elegantly, so that we'll be able to workaround the filtering UX not supporting elaborate combination (this will be most relevant when introducing filters like uri_authority_contains);
  • filter's name attribute is meant to be used in a x {name} y type of scenarios, like x is lesser than y;
  • the parameters attribute declares the Mathesar type of each parameter used by filter;
    • given the current UI design, the parameters are ordered in such a way that frontend can always use the first parameter;
      • currently, frontend will only care about the mathesar type of the first parameter, but that might (or might not) change and this API should be flexible enough to not require changes backend in such a case;
  • the API does not declare combinators/operators like and, or, not: those will have to be hardcoded on the frontend (like it currently is with SA filters);
  • each parameter is a dict, and might include more information in the future, like id and/or name;
  • the format for submitting a filter expression to the backend will use the previously introduced functions API format, but that should be irrelevant for this PR;
    • one could say that this PR introduces a narrowed and simplified view into the functions API.

Checklist

  • My pull request has a descriptive title (not a vague title like Update index.md).
  • My pull request targets the master branch of the repository
  • My commit messages follow best practices.
  • My code follows the established code style of the repository.
  • I added tests for the changes I made (if applicable).
  • I added or updated documentation (if applicable).
  • I tried running the project locally and verified that there are no
    visible errors.

Developer Certificate of Origin

Developer Certificate of Origin
Developer Certificate of Origin
Version 1.1

Copyright (C) 2004, 2006 The Linux Foundation and its contributors.
1 Letterman Drive
Suite D4700
San Francisco, CA, 94129

Everyone is permitted to copy and distribute verbatim copies of this
license document, but changing it is not allowed.


Developer's Certificate of Origin 1.1

By making a contribution to this project, I certify that:

(a) The contribution was created in whole or in part by me and I
    have the right to submit it under the open source license
    indicated in the file; or

(b) The contribution is based upon previous work that, to the best
    of my knowledge, is covered under an appropriate open source
    license and I have the right under that license to submit that
    work with modifications, whether created in whole or in part
    by me, under the same open source license (unless I am
    permitted to submit under a different license), as indicated
    in the file; or

(c) The contribution was provided directly to me by some other
    person who certified (a), (b) or (c) and I have not modified
    it.

(d) I understand and agree that this project and the contribution
    are public and that a record of the contribution (including all
    personal information I submit with it, including my sign-off) is
    maintained indefinitely and may be redistributed consistent with
    this project or the open source license(s) involved.

@dmos62 dmos62 added affects: architecture Improvements or additions to architecture work: backend Related to Python, Django, and simple SQL status: draft labels Feb 10, 2022
@dmos62 dmos62 added this to the [07] Initial Data Types milestone Feb 10, 2022
@dmos62 dmos62 self-assigned this Feb 10, 2022
@kgodey kgodey self-assigned this Feb 11, 2022
@codecov-commenter
Copy link

codecov-commenter commented Feb 14, 2022

Codecov Report

Merging #1061 (ff5ce6a) into master (fe7c95b) will increase coverage by 0.04%.
The diff coverage is 94.80%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1061      +/-   ##
==========================================
+ Coverage   92.70%   92.74%   +0.04%     
==========================================
  Files         108      111       +3     
  Lines        3852     3985     +133     
==========================================
+ Hits         3571     3696     +125     
- Misses        281      289       +8     
Flag Coverage Δ
pytest-backend 92.74% <94.80%> (+0.04%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
db/functions/redundant.py 87.50% <87.50%> (ø)
mathesar/database/types.py 97.61% <91.66%> (-2.39%) ⬇️
db/functions/hints.py 97.05% <93.75%> (-2.95%) ⬇️
db/functions/base.py 90.40% <94.44%> (-0.59%) ⬇️
mathesar/filters/base.py 94.44% <94.44%> (ø)
db/functions/known_db_functions.py 100.00% <100.00%> (ø)
db/types/base.py 100.00% <100.00%> (ø)
mathesar/api/serializers/filters.py 100.00% <100.00%> (ø)
mathesar/api/ui/viewsets/databases.py 100.00% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update fe7c95b...ff5ce6a. Read the comment docs.

Copy link
Contributor

@kgodey kgodey left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good overall, thanks @dmos62.

Reading through this PR, I think we should rename the mathesar_types concept to ui_types instead - that way, they match the nomenclature of the API and it's clearer that they are UI abstractions.

I'm also wondering whether it makes sense to separate the parameters for the value being filtered (the first parameter) from the parameters that are part of the filtering function (the second and onwards). I think it would make the API clearer to use without accompanying documentation. I'm thinking something like:

 {
        "id": "greater",
        "display_name": "is greater than",
        "accepts": [
            {
                "display_name": "value",
                "ui_types": [
                    "number"
                ]
            },
        ]
        "filter_parameters": [
            {
                "display_name": "value",
                "ui_types": [
                    "number"
                ]
            }
        ]
    },

Please note a couple of other changes:

  • I renamed name to display_name to make it clearer that it's to be used for display.
  • I also added a display_name attribute to parameters that the frontend can use as a placeholder. This may be unnecessary, I defer to @pavish.

mathesar/tests/filters/test_filters.py Outdated Show resolved Hide resolved
@dmos62
Copy link
Contributor Author

dmos62 commented Feb 15, 2022

@kgodey I'm not sure. Notice that accepts and parameters are synonyms in this context. We could say something like first_parameter or primary_parameter and then other_parameters, but I'm not sure that's better.

As seen in the next filtering PR, the frontend uses the function API to submit filters to the backend, whose format is a sequence of parameters: [param1, param2, ...]. That goes well with the format proposed here ([{"mathesar_types": ["number"]}, "mathesar_types": ["number"]}]), since currently we're using positional parameters. Named parameters are planned, but they're a non-essential nice-to-have, so we shouldn't expect them to be implemented soon. We're using the function API to submit filters to save on development time and for general simplicity too.

I think it should be up to the UI-builder to decide what is the primary parameter. At the moment, I'm just making sure the order of parameters is what you'd expect. If more complicated signatures are ever needed, we can give parameters display names to remove ambiguity for the user.

Does this make sense?

@dmos62
Copy link
Contributor Author

dmos62 commented Feb 15, 2022

By the way, I'll rename mathesar_types to ui_types.

@dmos62
Copy link
Contributor Author

dmos62 commented Feb 15, 2022

@pavish
Copy link
Member

pavish commented Feb 15, 2022

@kgodey @dmos62

I also added a display_name attribute to parameters that the frontend can use as a placeholder. This may be unnecessary, I defer to @pavish.

This is not necessary at the moment since the parameters are currently accepted as an array and referenced by position, instead of an object where they are referenced by name (As mentioned in #1069). If & when we transform that in the future, the frontend will need a name for each parameter to form the request.

Edit: @dmos62 has the same comment in #1061 (comment)

Copy link
Contributor

@kgodey kgodey left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@dmos62 It seems like you've forgotten to update the mathesar_types key in the test. Please fix the test and merge this PR.

I'm fine with the positional parameters, thanks for the explanation. I'm still wary of using array positions to implicitly determine what's being filtered and what it's being filtered against, but we can solve that problem if/when it becomes a problem.

@dmos62 dmos62 marked this pull request as ready for review February 16, 2022 10:50
@dmos62 dmos62 requested a review from a team February 16, 2022 10:50
@dmos62 dmos62 merged commit 4efb974 into master Feb 16, 2022
@dmos62 dmos62 deleted the introduce-new-filtering-api branch February 16, 2022 10:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
affects: architecture Improvements or additions to architecture work: backend Related to Python, Django, and simple SQL
Projects
No open projects
Development

Successfully merging this pull request may close these issues.

4 participants