onegov-cloud icon indicating copy to clipboard operation
onegov-cloud copied to clipboard

Feature/ogc 508 replace elastic search by postgres v2

Open Tschuppi81 opened this issue 1 year ago • 6 comments

Please fill in the commit message below and work through the checklist. You can delete parts that are not needed, e.g. the optional description, the link to a ticket or irrelevant options of the checklist.

Commit message

<Module>: <Message>

TYPE: <Feature|Bugfix> LINK: <Ticket-Number> HINT: <Optional upgrade hint>

Checklist

  • [ ] I have performed a self-review of my code
  • [ ] I considered adding a reviewer
  • [ ] I have added an upgrade hint such as data migration commands to be run
  • [ ] I have updated the PO files
  • [ ] I have added new modules to the docs
  • [ ] I made changes/features for both org and town6
  • [ ] I have updated the election_day API docs
  • [ ] I have tested my code thoroughly by hand
    • [ ] I have tested styling changes/features on different browsers
    • [ ] I have tested javascript changes/features on different browsers
    • [ ] I have tested database upgrades
    • [ ] I have tested sending mails
    • [ ] I have tested building the documentation
  • [ ] I have added tests for my changes/features
    • [ ] I am using freezegun for tests depending on date and times
    • [ ] I considered using browser tests für javascript changes/features
    • [ ] I have added/updated jest tests for d3rendering (election_day, swissvotes)

Tschuppi81 avatar Jun 05 '24 14:06 Tschuppi81

Codecov Report

:x: Patch coverage is 91.29412% with 37 lines in your changes missing coverage. Please review. :white_check_mark: Project coverage is 89.46%. Comparing base (bdc57ed) to head (1b0e741). :warning: Report is 772 commits behind head on master.

Files with missing lines Patch % Lines
src/onegov/org/models/search.py 87.12% 17 Missing :warning:
src/onegov/org/views/search.py 68.18% 7 Missing :warning:
src/onegov/fsi/models/course_event.py 63.63% 4 Missing :warning:
src/onegov/fsi/models/course_attendee.py 87.50% 2 Missing :warning:
src/onegov/fsi/views/search.py 80.00% 2 Missing :warning:
src/onegov/agency/models/person.py 92.85% 1 Missing :warning:
src/onegov/event/models/occurrence.py 83.33% 1 Missing :warning:
src/onegov/feriennet/models/activity.py 85.71% 1 Missing :warning:
src/onegov/search/cli.py 0.00% 1 Missing :warning:
src/onegov/ticket/model.py 92.30% 1 Missing :warning:
Additional details and impacted files
Files with missing lines Coverage Δ
src/onegov/activity/models/attendee.py 98.66% <100.00%> (+0.03%) :arrow_up:
src/onegov/agency/models/agency.py 96.15% <100.00%> (+0.26%) :arrow_up:
src/onegov/agency/models/membership.py 97.14% <100.00%> (-2.86%) :arrow_down:
src/onegov/agency/views/search.py 100.00% <100.00%> (ø)
src/onegov/core/elements.py 97.27% <100.00%> (+0.02%) :arrow_up:
src/onegov/directory/models/directory.py 93.44% <100.00%> (+0.36%) :arrow_up:
src/onegov/directory/models/directory_entry.py 98.50% <100.00%> (+3.26%) :arrow_up:
src/onegov/event/models/event.py 95.37% <100.00%> (+0.02%) :arrow_up:
src/onegov/file/models/file.py 93.79% <100.00%> (ø)
src/onegov/form/models/submission.py 94.51% <100.00%> (ø)
... and 44 more

... and 4 files with indirect coverage changes


Continue to review full report in Codecov by Sentry.

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

:rocket: New features to boost your workflow:
  • :package: JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

codecov[bot] avatar Jun 06 '24 12:06 codecov[bot]

@Daverball I am working on the tests in test_views_search.py and can't make them pass. I narrowed the issue down to the hybrid_property resp. expression of GeneralFile and News. While debugging for News I see that es_public is True for all the three elements in the DB. Same for access and published. Once I apply the query.filter(model.es_public == True) no elements are left. Screenshot from 2024-10-14 06-29-53 Any idea why my filter statement or my hybrid expression does not work?

Tschuppi81 avatar Oct 14 '24 10:10 Tschuppi81

@Daverball I am working on the tests in test_views_search.py and can't make them pass. I narrowed the issue down to the hybrid_property resp. expression of GeneralFile and News. While debugging for News I see that es_public is True for all the three elements in the DB. Same for access and published. Once I apply the query.filter(model.es_public == True) no elements are left. Screenshot from 2024-10-14 06-29-53 Any idea why my filter statement or my hybrid expression does not work?

You're relying a lot on implicit type conversions here and hoping that SQLAlchemy is doing the right thing for you. You could try .is_(True) rather than == True, which is the recommended way for boolean and none checks in SQLAlchemy anyways. Although you can probably just filter on the column itself, since a boolean column can be used as a filter so filter(model.es_public) should technically work as well.

Either way to properly debug this, you would have to look at the final SQL statement SQLAlchemy generates for you. Sometimes you will just have to manually define an @{property_name}.expression for hybrid properties to get something correct. There's already several hybrid properties that define a manual expression, like published.

Daverball avatar Oct 14 '24 11:10 Daverball

Also as a side note query.count is quite inefficient for checking whether or not you have any results, since Postgres will have to find and count all the entries. It's usually better to do if session.query(query.exists()).scalar(): which will create an EXISTS subquery, which only has to find a single row that satisfies the predicate before returning a result.

Daverball avatar Oct 14 '24 11:10 Daverball

I will try the approach of calculating and storing the properties in a separate column while indexing and use them for searching.

Tschuppi81 avatar Oct 24 '24 10:10 Tschuppi81