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

[#12048] Migrate AdminSearchPageE2ETest #12838

Merged
merged 19 commits into from
Feb 26, 2024

Conversation

domoberzin
Copy link
Contributor

@domoberzin domoberzin commented Feb 25, 2024

Part of #12048
Outline of Solution

This PR migrates Account and AccountRequests used in this E2E test only.

The necessary cleanup methods are also implemented, as well as a method to remove search documents for AccountRequest. Note that when uploaded to Solr, the entry id's are prefixed with java.util.UUID:, hence removal of the document is done with the appended prefix as well.

   {
       "institute":"TEAMMATES Test Institute 1",
       "id":"java.util.UUID:9a9e8728-30ce-4809-a83a-af6910dde3fc",
       "email":"[email protected]",
       "_version_":1791928579971350528},
     {
       "institute":"TEAMMATES Test Institute 1",
       "id":"java.util.UUID:96b7df30-a2a0-41d9-a06a-ce8af6aafad8",
       "email":"[email protected]",
       "_version_":1791928580011196416},
     {
       "institute":"TEAMMATES Test Institute 1",
       "id":"java.util.UUID:41ffea73-d5cb-4567-a83b-48b31873836d",
       "email":"[email protected]",
       "_version_":1791928580069916672}]
 }

This also fixes the issue with searching, that occurred due to the following original execution pathway:

  1. Search request initiated (doesn't matter which)
  2. Sql search initiates first, as with all actions
  3. Since entity is found by search as the entity already exists in datastore, the following code is executed, from /sqlsearch/SearchManager.java
List<T> convertDocumentToEntities(List<SolrDocument> documents) {
        if (documents == null) {
            return new ArrayList<>();
        }

        List<T> result = new ArrayList<>();

        for (SolrDocument document : documents) {
            T entity = getEntityFromDocument(document);

            // Entity will be null if document corresponds to entity in datastore
            if (entity == null) {
                // search engine out of sync as SearchManager may fail to delete documents
                // the chance is low and it is generally not a big problem
                String id = (String) document.getFirstValue("id");
                deleteDocuments(Collections.singletonList(id));
                continue;
            }
            result.add(entity);
        }
        sortResult(result);

        return result;
    }
  1. The code above essentially deletes the search result and the search document entirely, and does not add it to the result, so afterwards in the subsequent datastore search, no results are returned since the document has been removed from Solr entirely.

Here is a video documenting an example execution, on the right screen is a direct call to the locally hosted solr for all search results, on the left is an execution of search via the admin search page. Once the search is executed from the admin page, the solr results on the right become empty.

Screen.Recording.2024-02-26.at.10.44.43.AM.mov

Additionally, in the equivalent code for non-sql search in /search/SearchManager.java, so nulls can be added to the result list as there is no handling for it, and the resultant conversion to output data will throw an NPE when properties of the entity are accessed.

List<T> convertDocumentToAttributes(List<SolrDocument> documents) {
        if (documents == null) {
            return new ArrayList<>();
        }

        List<T> result = new ArrayList<>();

        for (SolrDocument document : documents) {
            T attribute = getAttributeFromDocument(document);
            // Disabled for now
            // Entity will be null if document corresponds to entity in datastore
            // if (attribute == null) {
            // // search engine out of sync as SearchManager may fail to delete documents
            // // the chance is low and it is generally not a big problem
            // String id = (String) document.getFirstValue("id");
            // deleteDocuments(Collections.singletonList(id));
            // continue;
            // }
            result.add(attribute);
        }
        sortResult(result);

        return result;
    }

@domoberzin domoberzin marked this pull request as draft February 25, 2024 16:08
@domoberzin domoberzin marked this pull request as ready for review February 26, 2024 03:50
Copy link
Contributor

@cedricongjh cedricongjh left a comment

Choose a reason for hiding this comment

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

LGTM! thanks for the work on this, just some nits

Copy link
Contributor

@cedricongjh cedricongjh left a comment

Choose a reason for hiding this comment

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

Very nice, thanks!

@cedricongjh cedricongjh added the s.FinalReview The PR is ready for final review label Feb 26, 2024
@cedricongjh cedricongjh requested a review from weiquu February 26, 2024 04:38
Copy link
Contributor

@weiquu weiquu left a comment

Choose a reason for hiding this comment

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

1 small question, otherwise LGTM! Great work (:

@domoberzin domoberzin requested a review from weiquu February 26, 2024 11:45
Copy link
Contributor

@weiquu weiquu left a comment

Choose a reason for hiding this comment

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

LGTM

@weiquu weiquu merged commit 203ec24 into TEAMMATES:master Feb 26, 2024
6 of 9 checks passed
@cedricongjh cedricongjh added this to the V9.0.0-beta.0 milestone Mar 10, 2024
@weiquu weiquu added the c.Task Other non-user-facing works, e.g. refactoring, adding tests label Mar 10, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
c.Task Other non-user-facing works, e.g. refactoring, adding tests s.FinalReview The PR is ready for final review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants