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

Search: implement stable API #7255

Merged
merged 16 commits into from Aug 5, 2020
Merged

Search: implement stable API #7255

merged 16 commits into from Aug 5, 2020

Conversation

stsewd
Copy link
Member

@stsewd stsewd commented Jul 2, 2020

  • Make the search view class based
  • Fix bug related to pagination
  • Rename highlight to highlights
  • Raname docstring to content (domains can be declared outside the code too)
  • Cache resolved domains while serializing (this reduced a LOT the number of queries in the dashboard from >200 to ~17)

How is this going to be released without breaking search for cached versions?

Requests with the new-api=true param will get the new api, requests that don't set that param will get the old api.
This is temporary, once we release and browsers fetch the new js (1 or 2 weeks?), we can always default to the new api and delete the old code. Also, having the additional query string will prevent from fetching the results from the cache.

Also, I we can hold off on the docs till we make the change if we want.

Closes #5821
Closes #6341

todo

  • update the search extension

@stsewd stsewd requested review from ericholscher and a team July 2, 2020 04:35
Copy link
Member

@ericholscher ericholscher left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks great. I realize a lot of my comments are on code that was already there, but I think trying to clean this up where possible to make it a bit nicer to read is useful. I don't want to do a lot more refactoring, but at least cleaning up some of the magic functional logic and data structures would be 💯

docs/api/v3.rst Outdated Show resolved Hide resolved
docs/server-side-search.rst Outdated Show resolved Hide resolved
docs/server-side-search.rst Outdated Show resolved Hide resolved
docs/server-side-search.rst Outdated Show resolved Hide resolved
readthedocs/core/static-src/core/js/doc-embed/search.js Outdated Show resolved Hide resolved
readthedocs/search/views.py Show resolved Hide resolved
readthedocs/search/views.py Outdated Show resolved Hide resolved
readthedocs/search/views.py Outdated Show resolved Hide resolved
readthedocs/search/views.py Outdated Show resolved Hide resolved
readthedocs/search/views.py Show resolved Hide resolved
@stsewd
Copy link
Member Author

stsewd commented Jul 31, 2020

I have refactored some bits and used another endpoint for the new search response (we will remove/stop supporting the old endpoint, right?). I want to test this a little more next week in case the refactor broke something.

@stsewd
Copy link
Member Author

stsewd commented Jul 31, 2020

Also, reminder for myself to update the search extension too https://github.com/readthedocs/readthedocs-sphinx-search/pull/55/files

@stsewd
Copy link
Member Author

stsewd commented Aug 3, 2020

Ok, nothing broke, this is ready for review again.

.. warning::

This API isn't stable yet, some small things may change in the future.
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm leaving this warning since the response will change when we start returning relative paths

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder if we should do the relative path work in another PR after merging this one, before shipping it? Seems nice to get it all done together if possible.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure, I can start working in another PR

Copy link
Member

@ericholscher ericholscher left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These changes look good -- I think this will be a good step forward 🎉

@stsewd stsewd merged commit 82b634e into master Aug 5, 2020
2 checks passed
@stsewd stsewd deleted the search-stable-api branch August 5, 2020 16:50
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

Successfully merging this pull request may close these issues.

Combining Search View and Docsearch Use full_path to show search results
2 participants