Police-Data-Accessibility-Project / data-sources-app

An API and UI for using and maintaining the Data Sources database
MIT License
4 stars 5 forks source link

Refactor `SearchTokens.py` #285

Closed maxachis closed 4 months ago

maxachis commented 6 months ago

The get method in the SearchTokens class in SearchTokens.py has a number of issues which merit addressing:

  1. The function currently is quite long (100 lines) and mixes multiple concerns: managing database transactions, conditionally executing different queries, and handling the business logic based on URL parameters. This makes the function difficult to test and maintain.
  2. Excess and redundant error handling: There are multiple nested try-except blocks, which makes the code complex and harder to maintain
  3. Inserting tokens into the database with hardcoded SQL mixes data access logic with controller logic and violates separation of concerns
  4. Exceptions are printed to standard output and internal error messages are potentially leaked to the user, which can lead to a leak of sensitive information.
  5. The line if type(self.psycopg2_connection) == dict: is unclear and seems incorrect for validating database connection status
  6. Some error handling branches return both a tuple with a dictionary and status code, while others return a dictionary, leading to inconsistent behavior
  7. Use of print() for logging is not advisable -- this is less flexible and harder to manage than logging frameworks which support levels and better output management
  8. Data is repeatedly fetched and then transformed in ways that could potentially be optimized and simplified.

Refactoring is merited in this situation to make the code more comprehensible, maintainable, and testable.

maxachis commented 6 months ago

@josh-chamberlain I may need a better understanding of the Search Tokens:

  1. Why do we need them in the first place?
  2. How is it being used on the front-end? The Search Token code appears to call several database-interfacing functions which are also used by other endpoints -- for example: /data-sources, and /quick-search. Why do we have multiple endpoints performing the same functionality, only one uses search tokens?
josh-chamberlain commented 6 months ago

@maxachis consider this a conversation in progress. search tokens were a quick way to get a prototype up, but definitely not the longest-term solution. I think the reason these concepts are tied together is that search is the first real action people can take in the app.

Here are some goals of auth:

@joshuagraber , anything to add about what would be helpful from your perspective?

joshuagraber commented 4 months ago

@josh-chamberlain @maxachis Sorry, I must've been away when you tagged me here. Closing this without any more work seems appropriate, and we can just move forward with the new search paradigm we've been discussing.