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

UserWarning: Detected filter using positional arguments. #705

Closed
XGeffrier opened this issue Apr 25, 2023 · 5 comments
Closed

UserWarning: Detected filter using positional arguments. #705

XGeffrier opened this issue Apr 25, 2023 · 5 comments
Labels
api: firestore Issues related to the googleapis/python-firestore API. type: docs Improvement to the documentation for an API. type: question Request for information or clarification. Not an issue.

Comments

@XGeffrier
Copy link

XGeffrier commented Apr 25, 2023

Hello python-firestore team!
Thanks for this very useful lib and product.

A lot of my queries have recently triggered a UserWarning.

Minimal reproducible example:

import firebase_admin
from firebase_admin import credentials, firestore

# auth and instanciate client
cred = credentials.Certificate({...})
firebase_admin.initialize_app(cred)
db = firestore.client()

# perform simple "where" query
collection = db.collection("myCollection")
out = collection.where("field", "==", "value").get()

The final line triggers: UserWarning: Detected filter using positional arguments. Prefer using the 'filter' keyword argument instead.

It looks like it comes from this snippet, but I'm not skilled enough to investigate further.

I may use the filter keyword but I can't find any explanation in the doc on how to use it, and feel like I'm using the where method the way it was intended. Could you please either document the usage of filter keyword (and tell me what I'm doing wrong) or remove the warning if it should not be triggered?

Thanks a lot.

@product-auto-label product-auto-label bot added the api: firestore Issues related to the googleapis/python-firestore API. label Apr 25, 2023
@GoktugCagiran
Copy link

GoktugCagiran commented Apr 26, 2023

I had the same warning. There is no explanation about this in the documents and I couldn't find the filter usage in the changelogs as well. But the classes that are required can be found in the documents.

The filter argument takes BaseFilter class which is an abstract class and it is implemented in FieldFilter and BaseCompositeFilter. These classes can be imported from google.cloud.firestore_v1.base_query.

FieldFilter can be used to query against one field. Constructor of FieldFilter takes three positional arguments FieldFilter(field_path: str, op_string: str, value: Any | None = None) and it is the same arguments you use in where function.

BaseCompositeFilter can be used to combine or chain multiple fields to query. Constructor of BaseCompositeFilter takes two arguments BaseCompositeFilter(operator: Operator = StructuredQuery.CompositeFilter.Operator.OPERATOR_UNSPECIFIED, filters: Any | None = None). filters argument is a list of BaseFilter so it can contain both FieldFilter and BaseCompositeFilter (I am not exactly sure how it works when it contains BaseCompositeFilter but it works). The operator argument specifies how the filters are combined. The Operator class required for the operator argument is not implemented in the base_query and requires importing StructuredQuery from google.cloud.firestore_v1.types. You have two options for the operator argument StructuredQuery.CompositeFilter.Operator.AND and StructuredQuery.CompositeFilter.Operator.OR.

So, your simple "where" query can be written as:

# Import necessary classes
from google.cloud.firestore_v1.base_query import FieldFilter, BaseCompositeFilter
from google.cloud.types import StructuredQuery
# Create CollectionReference
collection_ref = db.collection("myCollection")
# Create FieldFilter
field_filter = FieldFilter("field1", "==", "value")
# Simple "where" query as in your example
query_on_single_field = collection_ref.where(filter=field_filter)

# Assume there are multiple fields you want to query on and you want to combine filters with OR operator. (Meaning it will combine BaseFilter's in filters list with OR and retrieve a document if any of the BaseFilter's matches the document)
composite_filter = BaseCompositeFilter(
   # If you use StructuredQuery.CompositeFilter.Operator.AND here it gives the same effect as chaining "where" functions
   operator=StructuredQuery.CompositeFilter.Operator.OR,
   filters=[
      FieldFilter("field1", "==", "value1"),
      FieldFilter("field2", "==", "value2")
   ]
)

# Query chaining multiple fields with OR
composite_query = collection_ref.where(filter=composite_filter)

To conclude, I don't think this feature is complete yet. It works when you use it but the naming and the importing style doesn't feel complete. There is another FieldFilter under StructuredQuery for example and if you use that it doesn't work. I also couldn't manage to create an instance of StructuredQuery or any of the classes defined with it. Also, if you take a look at the relevant section in the document it says "Snippet not available." for Python. So, I think at some point they will change how to query Firestore using Python but it is not ready yet. That is why even though I'm getting the same warning I decided not to use the filter keyword and just stick with the old where function.

@XGeffrier
Copy link
Author

Thanks a lot!

It's very good news that we now can combine queries with AND/OR operators, but I don't understand why the old-fashioned queries trigger a Warning. Unless these queries are soon deprecated, which should at least be mentioned in release notes and in documentation. If the intention is to keep it and maintain them, then the warning is useless.

I'm going to ignore the warnings just like you (on some of my scripts I run dozens of queries and have a warning each time), but I'm afraid to miss really important warnings hidden in the mass.

By reading the PR discussion (#698), I got the impression that the contributors just missed that point. Please contributors, fix it in a way or another (remove the warning, or explain why we should move to filters and document how if the feature is mature enough), I would be much grateful.

@klanderson
Copy link

I'm still getting the warning when I use keywords:

xxx.where(field_path="abc", op_string="==", value="xyz")

still raises the warning:

UserWarning: Detected filter using positional arguments. Prefer using the 'filter' keyword argument instead."

Is the warning also raised for keyword arguments? Seems like a confusing message if so.

Also, are keyword/positional arguments expected to be deprecated? Is there some issue with using them? People have to go into their projects and make changes to deal with these warnings, so we should only be warned if there is good reason for it

@kolea2
Copy link
Contributor

kolea2 commented Jul 12, 2023

Hi all, thanks for the feedback here! I recently created a PR to update the samples reflecting this change: GoogleCloudPlatform/python-docs-samples#10407.

As @XGeffrier mentions, this was introduced in #698, example https://github.com/googleapis/python-firestore/pull/698/files#r1147816611.

@GoktugCagiran - from your comment, "There is another FieldFilter under StructuredQuery for example and if you use that it doesn't work." could you explain this a bit more? Thank you!

@klanderson thanks for raising, will leave this issue open to investigate warnings when using keywords further.

@kolea2 kolea2 added type: question Request for information or clarification. Not an issue. type: docs Improvement to the documentation for an API. labels Jul 12, 2023
helperbot-recidiviz pushed a commit to Recidiviz/pulse-data that referenced this issue Sep 28, 2023
…iz-data#23923)

## Description of the change

A few weeks ago, Firestore started spitting out errors like this:
```
UserWarning: Detected filter using positional arguments. Prefer using the 'filter' keyword argument instead.
```

Looking at
https://stackoverflow.com/questions/76110267/firestore-warning-on-filtering-with-positional-arguments-how-to-use-filter-kw
and googleapis/python-firestore#705, it is
unclear if google intentionally changed their API or if this was a bug.
Either way, they are radio silent.

This PR updates our call to their SDK to be slightly more explicit which
removes the errors.

I tested this by running
`recidiviz.tools.workflows.run_etl_from_local_branch` locally. The
errors went away and the ETL completed successfully, including deleting
outdated documents.

## Type of change

> All pull requests must have at least one of the following labels
applied (otherwise the PR will fail):

| Label | Description |
|-----------------------------
|-----------------------------------------------------------------------------------------------------------
|
| Type: Bug | non-breaking change that fixes an issue |
| Type: Feature | non-breaking change that adds functionality |
| Type: Breaking Change | fix or feature that would cause existing
functionality to not work as expected |
| Type: Non-breaking refactor | change addresses some tech debt item or
prepares for a later change, but does not change functionality |
| Type: Configuration Change | adjusts configuration to achieve some end
related to functionality, development, performance, or security |
| Type: Dependency Upgrade | upgrades a project dependency - these
changes are not included in release notes |

## Related issues

Closes #XXXX

## Checklists

### Development

**This box MUST be checked by the submitter prior to merging**:
- [x] **Double- and triple-checked that there is no Personally
Identifiable Information (PII) being mistakenly added in this pull
request**

These boxes should be checked by the submitter prior to merging:
- [ ] ~Tests have been written to cover the code changed/added as part
of this pull request~ Change to API of external SDK

### Code review

These boxes should be checked by reviewers prior to merging:

- [ ] This pull request has a descriptive title and information useful
to a reviewer
- [ ] Potential security implications or infrastructural changes have
been considered, if relevant

GitOrigin-RevId: 3d9e53dbdfd1db50e2c78091f1e6a1923b5699c4
danghai added a commit to kernelci/kcidb that referenced this issue Dec 3, 2023
Fix: #431
UserWarning in build log: Detected filter using positional arguments.
Prefer using the 'filter' keyword argument instead
The filter argument takes BaseFilter class which is an abstract class
and it is implemented in FieldFilter. This class can be imported from
google.cloud.firestore_v1.base_query
Reference: googleapis/python-firestore#705
	   GoogleCloudPlatform/python-docs-samples#10407
spbnick pushed a commit to kernelci/kcidb that referenced this issue Dec 5, 2023
Fix: #431
UserWarning in build log: Detected filter using positional arguments.
Prefer using the 'filter' keyword argument instead
The filter argument takes BaseFilter class which is an abstract class
and it is implemented in FieldFilter. This class can be imported from
google.cloud.firestore_v1.base_query
Reference: googleapis/python-firestore#705
	   GoogleCloudPlatform/python-docs-samples#10407
@dhimmel
Copy link

dhimmel commented Mar 13, 2024

@triplequark closed this as completed on Oct 18, 2023

@triplequark could you elaborate on whether any changes were made before closing the issue as completed (as opposed to not planned with commentary)?

And weigh in on @klanderson's questions:

Is the warning also raised for keyword arguments? Seems like a confusing message if so. Also, are keyword/positional arguments expected to be deprecated? Is there some issue with using them? People have to go into their projects and make changes to deal with these warnings, so we should only be warned if there is good reason for it

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api: firestore Issues related to the googleapis/python-firestore API. type: docs Improvement to the documentation for an API. type: question Request for information or clarification. Not an issue.
Projects
None yet
Development

No branches or pull requests

6 participants