Add URI and text filters #1079

dmos62 · 2022-02-17T17:26:54Z

Fixes #413, Fixes #406

Implements following filters:

x contains y,
x contains (case insensitive) y,
x starts with (case insensitive) y,
x URI authority contains y,
x URI scheme equals y.

Other filters required in #413 and #406 have been implemented in previous PRs.

Technical details

The new filters look like this on the filters endpoint:

    {
        "id": "contains",
        "name": "contains",
        "parameters": [
            {
                "ui_types": [
                    "text",
                    "uri"
                ]
            },
            {
                "ui_types": [
                    "text",
                    "uri"
                ]
            }
        ]
    },
    {
        "id": "starts_with_case_insensitive",
        "name": "starts with (case insensitive)",
        "parameters": [
            {
                "ui_types": [
                    "text",
                    "uri"
                ]
            },
            {
                "ui_types": [
                    "text",
                    "uri"
                ]
            }
        ]
    },
    {
        "id": "uri_authority_contains",
        "name": "URI authority contains",
        "parameters": [
            {
                "ui_types": [
                    "uri"
                ]
            },
            {
                "ui_types": [
                    "text",
                    "uri"
                ]
            }
        ]
    },
    {
        "id": "uri_scheme_equals",
        "name": "URI scheme equals",
        "parameters": [
            {
                "ui_types": [
                    "uri"
                ]
            },
            {
                "ui_types": [
                    "text",
                    "uri"
                ]
            }
        ]
    }

Notice that the Mathesar type uri is string-like, just like text, so it's allowed wherever text is allowed (see contains parameters' types for an example). In contrast, uri_authority_contains first parameter can only be uri. Same for uri_scheme_equals.

Tests for new filters have been added.

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
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 · 2022-02-21T17:57:16Z

Note that in this PR the filters endpoint response includes some filters multiple times. This is fixed in #1090.

codecov-commenter · 2022-02-21T18:10:43Z

Codecov Report

Merging #1079 (cb9a118) into master (02e22cb) will increase coverage by 0.13%.
The diff coverage is 97.29%.

@@            Coverage Diff             @@
##           master    #1079      +/-   ##
==========================================
+ Coverage   93.24%   93.37%   +0.13%     
==========================================
  Files         112      112              
  Lines        4084     4168      +84     
==========================================
+ Hits         3808     3892      +84     
  Misses        276      276

Flag	Coverage Δ
pytest-backend	`93.37% <97.29%> (+0.13%)`	⬆️

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

Impacted Files	Coverage Δ
db/functions/operations/check_support.py	`100.00% <ø> (ø)`
db/functions/redundant.py	`92.30% <89.47%> (+4.80%)`	⬆️
db/types/base.py	`97.22% <91.66%> (-2.78%)`	⬇️
db/functions/base.py	`93.63% <100.00%> (+2.86%)`	⬆️
db/functions/known_db_functions.py	`100.00% <100.00%> (ø)`
db/functions/operations/apply.py	`97.05% <100.00%> (+0.76%)`	⬆️
db/types/uri.py	`100.00% <100.00%> (+1.78%)`	⬆️
mathesar/database/types.py	`97.61% <100.00%> (ø)`
mathesar/models.py	`96.35% <100.00%> (+0.04%)`	⬆️
... and 1 more

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 02e22cb...cb9a118. Read the comment docs.

kgodey

A couple of changes requested in individual comments below. I'd also like @mathemancer to take a look at this before merge since it touches the URI type.

Notice that in the issue #413, the x URI scheme equals y filter is specified to allow the user to choose from a list of URI schemes. This PR does not implement that.

At the same time, I think that scheme suggestions is not an important feature right now, so I propose we create an issue for it and put it off.

I think we should do this, it is important for making the product friendly to non-technical users, who may not know what a URI scheme is.

It could be done somewhat easily. Just add to the relevant parameter a hint like hint.suggested_values(("http","https","ftp","ftps",...)).

This would require putting some and-or logic into the hint system though, which will be necessary down the road, but maybe not so much now.

I'm not sure why this would need "and-or logic", can you elaborate?

kgodey · 2022-02-23T00:22:16Z

db/functions/redundant.py

-# would involve creating an alternative to to_sa_expression: something like to_db_function
-# execution engine would see that to_sa_expression is not implemented, and it would look for
-# to_db_function.
+class RedundantDBFunction(DBFunction):


I find this name confusing. Perhaps something like DBFunctionCombination would be clearer?

I think DBFunctionCombination can be even more confusing, since a regular DBFunction instance can contain other DBFunction instances as parameters, and thus could be called a combination or supporting combination.

I like "redundant", because it highlights that it's made of DBFunctions that could already be combined together without this particular DBFunction.

I'm open to other names. Maybe SecondaryDBFunction? That's a bit abstract though. DBFunctionPackage has the same problem as DBFunctionCombination, but it goes hand in hand with the abstract unpack method.

I like DBFunctionPackage.

kgodey · 2022-02-23T00:23:03Z

db/functions/redundant.py

+
+class StartsWithCaseInsensitive(RedundantDBFunction):
+    id = 'starts_with_case_insensitive'
+    name = 'starts with (case insensitive)'


If this name is meant to be used in the frontend, please remove (case insensitive) from the name parameter.

It is. If the suffix is removed, it will have the same display name as a regular (case-sensitive) starts with filter: a user won't be able to distinguish the two. Why do you want to make this change?

There shouldn't be a case-sensitive "starts with" in the frontend, only the case insensitive version.

We don't want to overwhelm users with a lot of similar filtering options, it will make finding the filter they want harder. And non-technical users may not know what "case sensitive" refers to.

kgodey · 2022-02-23T00:40:59Z

db/functions/redundant.py

+
+class ContainsCaseInsensitive(RedundantDBFunction):
+    id = 'contains_case_insensitive'
+    name = 'contains (case insensitive)'


Case insensitive comment from above applies here as well.

dmos62 · 2022-02-23T12:38:25Z

It could be done somewhat easily. Just add to the relevant parameter a hint like hint.suggested_values(("http","https","ftp","ftps",...)).

This would require putting some and-or logic into the hint system though, which will be necessary down the road, but maybe not so much now.

I'm not sure why this would need "and-or logic", can you elaborate?

I wasn't clear enough. These are the options I mentioned:

It could be done somewhat easily. Just add to the relevant parameter a hint like hint.suggested_values(("http","https","ftp","ftps",...)).
Or, give the parameter a hint specifying that it must be either just a string-like or a uri-scheme: then the frontend would know what kinds of suggestions would be appropriate there. This would require putting some and-or logic into the hint system though, which will be necessary down the road, but maybe not so much now.

If this is wanted for alpha, I'll hack something together into a dedicated PR.

dmos62 · 2022-02-23T12:44:55Z

@kgodey @mathemancer note that the changes in the db/types/uri.py are just me adding URI-specific DBFunctions to that namespace. I don't make changes to the existing URI type code.

kgodey · 2022-02-23T13:50:56Z

If this is wanted for alpha, I'll hack something together into a dedicated PR.

Okay, thanks, please do. Option 1 seems easiest.

dmos62 · 2022-02-24T12:37:15Z

Ready for review.

I've removed case-sensitive string filters from the filters endpoint.

Other requested changes will be submitted as dedicated PR/s. Those changes are:

rename RedundantDBFunction to something better;
- see Add filters for money and boolean #1090 (comment)
  - pushed this change (accidentally) to the latest filtering PR
have appropriate filters suggest popular/standard URI schemes
- Suggest popular/standard URI schemes as values for appropriate filter parameters #1097

kgodey

Looks good to me, I'll leave it to @mathemancer to review/merge.

mathemancer

I mostly just had questions; the only real request is to try to unify the definitions of things like "string-like" "number-like", etc. between the definitions in the db.types.operations.cast module and the hint building logic.

mathemancer · 2022-02-25T12:32:17Z

db/functions/base.py

+def sa_call_sql_function(function_name, *parameters):
+    return getattr(func, function_name)(*parameters)


Awesome idea.

mathemancer · 2022-02-25T12:37:24Z

db/functions/base.py

@@ -225,6 +230,36 @@ def to_sa_expression(*values):
 class StartsWith(DBFunction):
    id = 'starts_with'
    name = 'starts with'
+    hints = tuple([


I'm curious why you went with this syntax rather than just

hints = ( hints.foo(bar), hings.baz, )

I find the shorthand tuple syntax ((x,)) to be errorprone. Just yesterday I ran into an annoying bug where I had forgotten the trailing comma and what was supposed to be a single-element tuple ended up being just the element. So I try to do the more awkward, but safer tuple([x]). This is not a very strong preference. After getting bit by this a few times you learn to append a comma to everything, I guess.

mathemancer · 2022-02-25T12:38:41Z

db/functions/base.py

@@ -73,7 +77,7 @@ class Literal(DBFunction):
    name = 'as literal'
    hints = tuple([
        hints.parameter_count(1),
-        hints.parameter(1, hints.literal),
+        hints.parameter(0, hints.literal),


Why this change? (and others like it)

I wanted to use 1-based indexing, but changed my mind. Everything in Python is 0-based, so I decided to go with the flow.

mathemancer · 2022-02-25T13:11:48Z

db/functions/redundant.py

+        raise Exception("UnpackabelDBFunction.to_sa_expression should never be used.")
+
+    @abstractmethod
+    def unpack(self):


For my own education: Should implementations of this unpack method recurse "in place"? I.e., if I happen to have a redundant (or whatever we decide for a term) function that's composed of other redundant functions, should the unpack method call the unpack methods of those functions as well, or should that be left to the caller? I'm not sure since you'd want to avoid having the recursion in multiple places, and _db_function_to_sa_expression is already recursive.

I think it's desirable not to have to do manual recursion inside unpack. I don't immediately recall if I implemented full recursion yet. I think I did, since I was concerned about infinite loops.

mathemancer · 2022-02-25T13:26:12Z

db/types/base.py

+    string_like_db_types = (
+        PostgresType.CHARACTER_VARYING,
+        PostgresType.CHARACTER,
+        PostgresType.TEXT,
+        MathesarCustomType.URI,


This sort of info is also in the cast.py file. Did you consider trying to unify these with those definitions? I ask because it makes me nervous to have this defined in more than one place.

I didn't notice the duplication. I'll investigate.

dmos62 · 2022-02-25T13:57:57Z

@mathemancer could we get this merged? It's the base for a few other PRs. I'll open an issue for the duplicated logic across db.types.operations.cast and db.types.base modules and see about fixing that in a new PR.

mathemancer

Approving, since I agree we should probably get this merged to unblock downstream PRs. Please don't forget to create the issue about the duplicated definitions of type tags (string-like, number-like, etc.)

dmos62 · 2022-02-28T21:45:24Z

@mathemancer the issue for that is here: #1100

dmos62 added 11 commits February 17, 2022 15:59

Allow DBFunctions to be defined with other DBFunctions

1f2eb2c

Add Contains DBFunction

93a0391

Fix specified trait

afffa09

Fix import

37ca6cc

Add tests for some redundant functions

19c741d

Add URIAuthorityContains

ee4b76f

Fix parameter indexes

929403b

Fix SA string concat

e747e91

Add tests for StartsWith, Contains, URIAuthorityContains

f1a9692

Change comment

46768ec

Merge branch 'replace-filtering-api' into add-uri-filters

f4ab28a

dmos62 added affects: architecture Improvements or additions to architecture work: backend Related to Python, Django, and simple SQL status: draft labels Feb 17, 2022

dmos62 added this to the [07] Initial Data Types milestone Feb 17, 2022

dmos62 self-assigned this Feb 17, 2022

kgodey changed the base branch from master to replace-filtering-api February 17, 2022 17:37

Base automatically changed from replace-filtering-api to master February 17, 2022 18:01

dmos62 added 6 commits February 18, 2022 11:29

Merge branch 'master' into add-uri-filters

dd2aff8

Add URISchemeEquals with tests

eccffc1

Linter fixes

f220966

Add test for URI Contains

cd6447c

Fix a few bugs

a0b120d

Simplify known_db_functions

3a4ef35

dmos62 marked this pull request as ready for review February 18, 2022 16:29

dmos62 requested a review from a team February 18, 2022 16:29

github-actions bot requested review from eito-fis, kgodey, mathemancer and pavish February 18, 2022 16:29

Implement case insensitive text filters

19a19f2

kgodey requested changes Feb 23, 2022

View reviewed changes

kgodey assigned mathemancer and unassigned kgodey Feb 23, 2022

kgodey reviewed Feb 23, 2022

View reviewed changes

dmos62 added 4 commits February 24, 2022 13:33

Merge branch 'master' into add-uri-filters

feedcb2

Make case insensitive string filters the only string filters

34a288f

Update tests

15e4ef7

Dead imports

b85949b

dmos62 mentioned this pull request Feb 24, 2022

Suggest popular/standard URI schemes as values for appropriate filter parameters #1097

Open

dmos62 added 2 commits February 24, 2022 14:18

Fix test

2475e26

Fix date test

f2fd9da

dmos62 requested a review from kgodey February 24, 2022 12:37

kgodey approved these changes Feb 24, 2022

View reviewed changes

mathemancer requested changes Feb 25, 2022

View reviewed changes

dmos62 mentioned this pull request Feb 25, 2022

Resolve logic duplication in the db.types namespace #1100

Closed

dmos62 added 2 commits February 28, 2022 18:43

Merge branch 'master' into add-uri-filters

90b4ee1

Linter fixes

cb9a118

mathemancer self-requested a review February 28, 2022 17:43

mathemancer approved these changes Feb 28, 2022

View reviewed changes

mathemancer merged commit 67c6439 into master Feb 28, 2022

mathemancer deleted the add-uri-filters branch February 28, 2022 17:46

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add URI and text filters #1079

Add URI and text filters #1079

dmos62 commented Feb 17, 2022 •

edited

Loading

dmos62 commented Feb 21, 2022

codecov-commenter commented Feb 21, 2022 •

edited

Loading

kgodey left a comment

kgodey Feb 23, 2022

dmos62 Feb 23, 2022 •

edited

Loading

kgodey Feb 23, 2022

kgodey Feb 23, 2022

dmos62 Feb 23, 2022 •

edited

Loading

kgodey Feb 23, 2022

kgodey Feb 23, 2022

dmos62 commented Feb 23, 2022

dmos62 commented Feb 23, 2022 •

edited

Loading

kgodey commented Feb 23, 2022

dmos62 commented Feb 24, 2022 •

edited

Loading

kgodey left a comment

mathemancer left a comment

mathemancer Feb 25, 2022

mathemancer Feb 25, 2022

dmos62 Feb 25, 2022

mathemancer Feb 25, 2022

dmos62 Feb 25, 2022

mathemancer Feb 25, 2022

dmos62 Feb 25, 2022

mathemancer Feb 25, 2022

dmos62 Feb 25, 2022

dmos62 commented Feb 25, 2022

mathemancer left a comment

dmos62 commented Feb 28, 2022

		def sa_call_sql_function(function_name, *parameters):
		return getattr(func, function_name)(*parameters)

Add URI and text filters #1079

Add URI and text filters #1079

Conversation

dmos62 commented Feb 17, 2022 • edited Loading

Checklist

Developer Certificate of Origin

dmos62 commented Feb 21, 2022

codecov-commenter commented Feb 21, 2022 • edited Loading

Codecov Report

kgodey left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dmos62 Feb 23, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dmos62 Feb 23, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dmos62 commented Feb 23, 2022

dmos62 commented Feb 23, 2022 • edited Loading

kgodey commented Feb 23, 2022

dmos62 commented Feb 24, 2022 • edited Loading

kgodey left a comment

Choose a reason for hiding this comment

mathemancer left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dmos62 commented Feb 25, 2022

mathemancer left a comment

Choose a reason for hiding this comment

dmos62 commented Feb 28, 2022

dmos62 commented Feb 17, 2022 •

edited

Loading

codecov-commenter commented Feb 21, 2022 •

edited

Loading

dmos62 Feb 23, 2022 •

edited

Loading

dmos62 Feb 23, 2022 •

edited

Loading

dmos62 commented Feb 23, 2022 •

edited

Loading

dmos62 commented Feb 24, 2022 •

edited

Loading