Postgres Full Text search. #1
Conversation
In `SearchCollection`, the ts_query is currently created every time the do_search method is called. This is unnecessary if ts_query only depends on the search term, which is not expected to change throughout the lifetime of the object.
We need to update this in the background using the event API from sqlalchemy directly. The reason for this was that before we used `@observes` for the files, this seems to not work in some cases if used in combination with `asssociated` 💡
…when dealing with SearchableAssociatedFiles
src/privatim/orm/associable.py
Outdated
| @@ -242,6 +245,7 @@ def descriptor(cls: type['Base']) -> Mapped[list[_M]]: | |||
| return relationship( | |||
| argument=associated_cls, | |||
| secondary=association_table, | |||
| back_populates=back_populates, | |||
There was a problem hiding this comment.
This should never be None. I think you messed things up by inheriting from File, but rather than making File polymorphic you used a different table. Associable assumes polymorphism, i.e. the class that's before Associable in the MRO is considered the polymorphic base, so if you want to share attributes between both file types then you need to split those out into a common abstract base class (__abstract__ = True without setting __tablename__) that both File and SearchableFile inherit from, as well as Associable at the same time.
There was a problem hiding this comment.
This will mean you will need to query the abstract base class if you want to query both File and SearchableFile at the same time. But it's probably good to be explicit about this anyways, since this implicitly involves a union of two distinct tables.
There was a problem hiding this comment.
Some context: I added this because @observes (from sqlalchemy_utils) didn't work for files, which is what I initially planned to use instead of sqlalchemy's event. I saw the comment in onegov about this potentially being an issue and wanted to check if this would fix it.
This is sort of from onegov, but a more simpler version, I noticted we also set back_populates = None in onegov, though I'm not sure I fully grasp the implications.
There was a problem hiding this comment.
I first used @observes as seen here. The docstring migrated then and silently diverged from it's original meaning
d692261#diff-33826b0cae4b335e638aec7708f9e99573759086e592e1a4a331d6596d63fb81R86-R96
There was a problem hiding this comment.
observes didn't work because you broke the back reference with your inheritance structure of SearchableFile using its own table.
There was a problem hiding this comment.
More concretely, if you look more closely at Associable you'll notice that all the links are put on the association_base_cls which is going to be File both for File and SearchableFile. So right now even the links on SearchableFile are put into File, so the links property is incorrect.
I assume you got rid of the whole logic about backref and back_populates from onegov, because SQLAlchemy 2.0 emits a warning when using backref instead of back_populates. But this is one of the cases where you do need to use backref so you get a proper bidirectional relationship that can be followed by the @observes decorator.
What the decorator does under the hood is, it looks at the reverse relationship and installs a hook on the related model that looks at changes on the reverse relationship. So it properly does the backwards-logic thing, where the changes that would trigger this observer don't happen in this table but rather in a different table.
Co-authored-by: David Salvisberg <[email protected]>
This is a prettly tough nut to crack for what concerns type checking
Welcome to Codecov 🎉Once you merge this PR into your default branch, you're all set! Codecov will compare coverage reports and display results in all future pull requests. Thanks for integrating Codecov - We've got you covered ☂️ |
- Fix teardown in client fixture not working (ForeignKey error). - Adjust view `download_general_file` to work with the new SearchableFile
|
Summary:
|
The search works.