🚨(backend) fix search without refresh token

[mascarpon3]

i remove the decorator on the search function
and refresh the token in the function so
i can try catch it.

Purpose

fix of #2087

Proposal

• remove the refresh in the decorator
• try catch the refresh in the function

External contributions

Thank you for your contribution! 🎉

Please ensure the following items are checked before submitting your pull request:

• I have read and followed the contributing guidelines
• I have read and agreed to the Code of Conduct
• I have signed off my commits with git commit --signoff (DCO compliance)
• I have signed my commits with my SSH or GPG key (git commit -S)
• My commit messages follow the required format: <gitmoji>(type) title description
• I have added a changelog entry under ## [Unreleased] section (if noticeable change)
• I have added corresponding tests for new features or bug fixes (if applicable)

[lunika]

This method is not tested, you should have dedicated unit tests.

[lunika]

Also, in this function you are not testing if the access token is expired or not. I don't see the interest in refreshing a still valid access token.

Also you are not refreshing the oidc_id_token_expiration parameter in the session

[lunika]

Not sure this is the behavior we want.

You should probably put this code in the _search_with_indexer method and catch this exception in the try/except block calling the _search_with_indexer method.

[mascarpon3]

honestly i think writing this in this function makes the code more readable. the main search function is in charged of all the dispatch to the sub-functions.

what i realise i should have done though is writing it bellow the if indexer is None sheck

[lunika]

@drf.decorators.action(detail=False, methods=["get"], url_path="search")
    def search(self, request, *args, **kwargs):
        """
        Returns an ordered list of documents best matching the search query parameter 'q'.

        It depends on a search configurable Search Indexer. If no Search Indexer is configured
        or if it is not reachable, the function falls back to a basic title search.
        """
        params = serializers.SearchDocumentSerializer(data=request.query_params)
        params.is_valid(raise_exception=True)
        search_type = self._get_search_type()

        if search_type == SearchType.TITLE:
            return self._title_search(request, params.validated_data, *args, **kwargs)

        indexer = get_document_indexer()
        if indexer is None:
            # fallback on title search if the indexer is not configured
            return self._title_search(request, params.validated_data, *args, **kwargs)

        try:
            return self._search_with_indexer(
                indexer, request, params=params, search_type=search_type
            )
        except (requests.exceptions.RequestException, AuthenticationFailed) as e:
            logger.error(
                "Error while searching documents with indexer \n%s \nfall back on title search",
                e,
            )
            # fallback on title search if the indexer is not reached
            return self._title_search(request, params.validated_data, *args, **kwargs)

    def _get_search_type(self) -> SearchType:
        """
        Returns the search type to use for the search endpoint based on feature flags.
        If a user has both flags activated the most advanced search is used
        (HYBRID > FULL_TEXT > TITLE).
        A user with no flag will default to the basic title search.
        """
        if waffle.flag_is_active(self.request, FeatureFlag.FLAG_FIND_HYBRID_SEARCH):
            return SearchType.HYBRID
        if waffle.flag_is_active(self.request, FeatureFlag.FLAG_FIND_FULL_TEXT_SEARCH):
            return SearchType.FULL_TEXT
        return SearchType.TITLE

    @staticmethod
    def _search_with_indexer(indexer, request, params, search_type):
        """
        Returns a list of documents matching the query (q) according to the configured indexer.
        """
        request.session = refresh_access_token(request.session)
        queryset = models.Document.objects.all()

        results = indexer.search(
            q=params.validated_data["q"],
            search_type=search_type,
            token=request.session.get("oidc_access_token"),
            path=(
                params.validated_data["path"]
                if "path" in params.validated_data
                else None
            ),
            visited=get_visited_document_ids_of(queryset, request.user),
        )

        return drf_response.Response(
            {
                "count": len(results),
                "next": None,
                "previous": None,
                "results": results,
            }
        )

Well let me know if this code is less or more readable ?

Moreover, as the refresh_access_token is related to the refresh_access_token, I think it should be called in this method.

[lunika]

Suggested change

	@pytest.fixture(name="oidc_settings")
	def fixture_oidc_settings(settings):
	@pytest.fixture
	def oidc_settings(settings):

[lunika]

Can you also please update the readme ?
Thanks

[lunika]

I think a bit more at this implementation, we are loosing all the benefice to use the django-lasuite implementation.

Using the decorator on the _search_with_indexer should be I think better but to do this you have to refactor the search and _search_with_indexer method to be compatible with the decorator. But I think it is the best way to fix this issue.

[lunika]

Suggested change

	timeout=5,
	timeout=settings.OIDC_TIMEOUT,

[lunika]

Also, in this function you are not testing if the access token is expired or not. I don't see the interest in refreshing a still valid access token.

Also you are not refreshing the oidc_id_token_expiration parameter in the session

[lunika]

Closed in favor of #2097