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

circular import error caused by directly import search_results method #10699

Closed
whatisgalen opened this issue Mar 19, 2024 · 4 comments · Fixed by #10769
Closed

circular import error caused by directly import search_results method #10699

whatisgalen opened this issue Mar 19, 2024 · 4 comments · Fixed by #10769

Comments

@whatisgalen
Copy link
Member

In search_export.py, the search_results method from the search view is imported and used here:

https://github.com/archesproject/arches/blob/dev/7.6.x/arches/app/search/search_export.py#L151

If a project overrides the search view, however, the statement importing this method results in an error due to circular imports.

@chiatt chiatt added this to pipeline Mar 19, 2024
whatisgalen added a commit that referenced this issue Mar 19, 2024
@chiatt chiatt moved this to 🏗 In Progress in pipeline Mar 26, 2024
@jacobtylerwalls jacobtylerwalls moved this from 🏗 In Progress to 👀 In Review in pipeline Mar 26, 2024
@jacobtylerwalls jacobtylerwalls linked a pull request Mar 26, 2024 that will close this issue
6 tasks
whatisgalen added a commit that referenced this issue Mar 28, 2024
@whatisgalen
Copy link
Member Author

whatisgalen commented Mar 28, 2024

Unfortunately there aren't great options here. If a project developer overrides the search view (or at least search_results method in Search view) then there are only 2 options for a fix in core arches:

  • Either transform the method call to a full http request to the url "search_results" so that it gets routed to the project's version if that url has also been redeclared in the project
  • Or somehow tell Arches to look for views that have been overridden in the project and have them somehow point to the authoritative view directly

There's a 3rd option for the project developer which is to also override SearchExport.py to point to the project-local SearchView's search_results method, but this feels like a hack since a project should be able to override only the views they actually want to modify.

Neither of these options seem ideal so it would be great if folks could weigh in (my PR #10701 goes the https request route) to form a consensus on this issue (since it also applies more broadly to other views that may be overridden in a project but still have internal references from views that weren't overridden).

@whatisgalen
Copy link
Member Author

created #10721 since through the aforementioned PR I discovered there aren't any tests for SearchResultsExporter

@jacobtylerwalls
Copy link
Member

Neither of these options seem ideal so it would be great if folks could weigh in (my PR #10701 goes the https request route) to form a consensus on this issue

Rather than using the test client to issue a request, I think this is a use case for resolve:

In [1]: from django.urls import resolve, reverse

In [2]: resolve(reverse('search_results')).func
INFO:rdflib:RDFLib Version: 4.2.2
Out[2]: <function arches.app.views.search.search_results(request, returnDsl=False)>

@whatisgalen
Copy link
Member Author

Neither of these options seem ideal so it would be great if folks could weigh in (my PR #10701 goes the https request route) to form a consensus on this issue

Rather than using the test client to issue a request, I think this is a use case for resolve:

In [1]: from django.urls import resolve, reverse

In [2]: resolve(reverse('search_results')).func
INFO:rdflib:RDFLib Version: 4.2.2
Out[2]: <function arches.app.views.search.search_results(request, returnDsl=False)>

@jacobtylerwalls thanks for an excellent suggestion! Want to review my PR? :)

@jacobtylerwalls jacobtylerwalls linked a pull request Apr 15, 2024 that will close this issue
6 tasks
jacobtylerwalls added a commit that referenced this issue Apr 15, 2024
Uses django.urls resolve() to send request to search_results wherever it exists in Arches, prj, or app re #10699

Co-authored-by: Jacob Walls <[email protected]>
@github-project-automation github-project-automation bot moved this from 👀 In Review to ✅ Done in pipeline Apr 15, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Development

Successfully merging a pull request may close this issue.

2 participants