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

FireO Should check for valid operators on ID field in filter() query #172

Open
dhodun opened this issue Dec 2, 2022 · 1 comment
Open

Comments

@dhodun
Copy link
Collaborator

dhodun commented Dec 2, 2022

As far as I can tell, the only operator that will work on __name__ / ID field is in:
https://cloud.google.com/firestore/docs/query-data/queries#query_operators

The errors out of Firestore API are generally pretty clear, but given that FireO is already checking for a List when filtering on id field, does it make sense to also ensure the user passes a valid comparison, i.e. in clause?

FireO is otherwise not currently not providing operator validation, but firestore library is providing and not catching in this case. In theory the firestore library could be expanded to support more operators. It is worth noting that the current use of this __name__ sentinenal and in operator doesn't appear to be documented.

Example from:

test1 = TestModel(name="test1")
test1.save()

TestModel.collection.filter("name", "has", [test1.id]).fetch()
  File "/Users/dhodun/developer/FireO/src/fireo/queries/query_iterator.py", line 22, in __init__
    self.docs = query.query().stream(query.query_transaction)
  File "/Users/dhodun/developer/FireO/src/fireo/queries/filter_query.py", line 257, in query
    ref = ref.where(*f)
  File "/Users/dhodun/developer/FireO/.venv/lib/python3.9/site-packages/google/cloud/firestore_v1/base_collection.py", line 268, in where
    return query.where(field_path, op_string, value)
  File "/Users/dhodun/developer/FireO/.venv/lib/python3.9/site-packages/google/cloud/firestore_v1/base_query.py", line 340, in where
    op=_enum_from_op_string(op_string),
  File "/Users/dhodun/developer/FireO/.venv/lib/python3.9/site-packages/google/cloud/firestore_v1/base_query.py", line 960, in _enum_from_op_string
    raise ValueError(msg)
ValueError: Operator string 'has' is invalid. Valid choices are: !=, <, <=, ==, >, >=, array_contains, array_contains_any, in, not-in.

To remain consistent either I would:

  1. remove the type checking here and continue to leave validation to firestore client and server: https://github.com/octabytes/FireO/blob/e59a1a92b85df026063fcd73a62723eaeb055beb/src/fireo/queries/filter_query.py#L245
  2. Leave in the type checking and add operator type checking as well
  • this is very special case of the filter() query and easy to mess up, since it isn't documented
  • The document_ref conversion here is actually pretty helpful since FireO users generally don't have to think about document refs or collection names
  • accept this is special case so extra validation is warranted, whereas other queries are checked by Firestore client instead of FireO
  • understand that if Firestore API accepts more operators on this sentinel in the future, FireO validation will need to be updated to allow them

Here is an example of the server side type checking for this error. It's not the most clear. Also, the emulator returns an empty list here and no error. You have to target Firestore itself to get the error:

class TestModel(Model):
    name = TextField()

test1 = TestModel(name="test1")
test1.save()

from google.cloud import firestore

# writing in firestore Client since FireO won't allow the non-list in the query
db = firestore.Client()
print(db.collection('test_model').where("__name__", "==", test1.id).get())
  File "/Users/dhodun/developer/FireO/.venv/lib/python3.9/site-packages/google/api_core/retry.py", line 190, in retry_target
    return target()
  File "/Users/dhodun/developer/FireO/.venv/lib/python3.9/site-packages/google/api_core/grpc_helpers.py", line 166, in error_remapped_callable
    raise exceptions.from_grpc_error(exc) from exc
google.api_core.exceptions.InvalidArgument: 400 __key__ filter value must be a Key
@ADR-007
Copy link
Collaborator

ADR-007 commented Mar 1, 2023

I made some changes to it, so now you can do this using FireO:

print(db.collection('test_model').where("__name__", "==", test1.id).get())

Another staff is still actual. The simple step can be to add an enum for operations. Then you will see an error from mypy at least.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants