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.

Sign up for GitHub

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

Migrate household endpoints to new API structure #2038

Merged
merged 27 commits into from
Dec 20, 2024
+1,122 −419
Merged

Migrate household endpoints to new API structure #2038

Changes from 1 commit
Commits
Show all changes
27 commits
Select commit Hold shift + click to select a range
43d6a96
feat: Initial stages of migration of household endpoints
anth-volk Nov 30, 2024
5dfc1b0
feat: Migrate post_household to new structure
anth-volk Nov 30, 2024
506d233
feat: Migrate update household endpoint
anth-volk Nov 30, 2024
ffb42ec
chore: Format
anth-volk Nov 30, 2024
0d3473e
fix: Fix minor bugs
anth-volk Nov 30, 2024
2129cda
fix: Modify validate_country to return 400 on country not found and u…
anth-volk Dec 2, 2024
6bd8420
feat: Refactor household endpoints to match new structure
anth-volk Dec 4, 2024
3e2a8a8
fix: Refactor fixtures
anth-volk Dec 4, 2024
a3a78fc
fix: Adjust test to handle added validation
anth-volk Dec 4, 2024
de96431
fix: Redo blueprint generation
anth-volk Dec 12, 2024
c0fdde9
fix: Remove erroneous print statement
anth-volk Dec 12, 2024
f789559
feat: Basic Werkzeug route validators
anth-volk Dec 12, 2024
f0d2a6e
feat: Error handling module and application to routes
anth-volk Dec 13, 2024
e7737c6
fix: Properly register error routes
anth-volk Dec 13, 2024
082d7cc
fix: Remove unused comment code
anth-volk Dec 13, 2024
6487058
fix: Redefine metadata route against convention
anth-volk Dec 13, 2024
c085d1d
feat: Return household JSON in household updater func and update test
anth-volk Dec 13, 2024
ca4ad9f
test: Fix some failing tests, add tests for error handlers
anth-volk Dec 13, 2024
7c24017
fix: Fix tracer tests
anth-volk Dec 13, 2024
c668a92
fix: Fix failing simulation analysis test
anth-volk Dec 13, 2024
a40e430
fix: Fix manual raising within try-catch blocks
anth-volk Dec 13, 2024
7cc72fe
fix: Fix final household tests
anth-volk Dec 14, 2024
adbfaa9
fix: Fix tests
anth-volk Dec 14, 2024
0d931a3
test: Fix tests
anth-volk Dec 18, 2024
2e70641
feat: Use error response builder
anth-volk Dec 18, 2024
0f39c1b
chore: Refactor against new error implementation
anth-volk Dec 18, 2024
3268c82
fix: Improve error handling
anth-volk Dec 18, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Prev Previous commit
Loading
Loading status checks…
fix: Improve error handling
@anth-volk
anth-volk committed Dec 18, 2024
commit 3268c8246ad30d37ef7f2b25755085ac7ce3082b
28 changes: 12 additions & 16 deletions policyengine_api/routes/economy_routes.py
View file
Original file line number Diff line number Diff line change
@@ -3,9 +3,8 @@
from policyengine_api.utils import get_current_law_policy_id
from policyengine_api.utils.payload_validators import validate_country
from policyengine_api.constants import COUNTRY_PACKAGE_VERSIONS
from flask import request, Response
from flask import request
import json
from werkzeug.exceptions import InternalServerError

economy_bp = Blueprint("economy", __name__)
economy_service = EconomyService()
@@ -34,17 +33,14 @@ def get_economic_impact(country_id, policy_id, baseline_policy_id):
"version", COUNTRY_PACKAGE_VERSIONS.get(country_id)
)

try:
result = economy_service.get_economic_impact(
country_id,
policy_id,
baseline_policy_id,
region,
dataset,
time_period,
options,
api_version,
)
return result
except Exception as e:
raise InternalServerError(str(e))
result = economy_service.get_economic_impact(
country_id,
policy_id,
baseline_policy_id,
region,
dataset,
time_period,
options,
api_version,
)
return result
163 changes: 71 additions & 92 deletions policyengine_api/routes/household_routes.py
View file
Original file line number Diff line number Diff line change
@@ -1,11 +1,8 @@
from flask import Blueprint, Response, request, jsonify
from werkzeug.exceptions import InternalServerError
from flask import Blueprint, Response, request
from werkzeug.exceptions import NotFound, BadRequest
import json

from policyengine_api.routes.error_routes import response_400, response_404
from policyengine_api.constants import COUNTRY_PACKAGE_VERSIONS
from policyengine_api.services.household_service import HouseholdService
from policyengine_api.utils import hash_object
from policyengine_api.utils.payload_validators import (
validate_household_payload,
validate_country,
@@ -30,27 +27,22 @@ def get_household(country_id: str, household_id: int) -> Response:
"""
print(f"Got request for household {household_id} in country {country_id}")

Copy link
Collaborator

Choose a reason for hiding this comment

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

discussion, nonblocking -
Suggest this as an optimization after introducing "real" logging.

In the theme from yesterday of making our route functions as clean and simple as possible, logging is another function the framework should be able to do for us in many cases.

In this case the blueprint already defines what the path is, the names of the parameters, the method type and has the parsed values from the path, etc. so it should be possible to automate logging all the relevant "got a request" information.

  1. Reduces clutter/duplication
  2. reduces cases where we forget to manually add the log message
  3. makes it easy to add more detail later and get that for every method (I.e. maybe we want to log which user is making the request)

try:
household: dict | None = household_service.get_household(
country_id, household_id
)
if household is None:
return response_404(f"Household #{household_id} not found.")
else:
return Response(
json.dumps(
{
"status": "ok",
"message": None,
"result": household,
}
),
status=200,
mimetype="application/json",
)
except Exception as e:
raise InternalServerError(
f"An error occurred while fetching household #{household_id}. Details: {str(e)}"
household: dict | None = household_service.get_household(
country_id, household_id
)
if household is None:
raise NotFound(f"Household #{household_id} not found.")
else:
return Response(
Copy link
Collaborator

Choose a reason for hiding this comment

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

nitt, style - suggest shipping and optionally changing later to unblock me (please:P ), but clean code suggests

if error:
  raise Exception("...")

return success

instead of unnecessary else's or nesting.

If you don't have the book, this is an ok summary

json.dumps(
{
"status": "ok",
"message": None,
"result": household,
}
),
status=200,
mimetype="application/json",
)


@@ -68,37 +60,30 @@ def post_household(country_id: str) -> Response:
payload = request.json
is_payload_valid, message = validate_household_payload(payload)
if not is_payload_valid:
return response_400(
f"Unable to create new household; details: {message}"
)

try:
# The household label appears to be unimplemented at this time,
# thus it should always be 'None'
label: str | None = payload.get("label")
household_json: dict = payload.get("data")

household_id = household_service.create_household(
country_id, household_json, label
)

return Response(
json.dumps(
{
"status": "ok",
"message": None,
"result": {
"household_id": household_id,
},
}
),
status=201,
mimetype="application/json",
)
except Exception as e:
raise InternalServerError(
f"An error occurred while creating a new household. Details: {str(e)}"
)
raise BadRequest(f"Unable to create new household; details: {message}")

# The household label appears to be unimplemented at this time,
# thus it should always be 'None'
label: str | None = payload.get("label")
household_json: dict = payload.get("data")

household_id = household_service.create_household(
country_id, household_json, label
)

return Response(
json.dumps(
{
"status": "ok",
"message": None,
"result": {
"household_id": household_id,
},
}
),
status=201,
mimetype="application/json",
)


@household_bp.route(
@@ -118,41 +103,35 @@ def update_household(country_id: str, household_id: int) -> Response:
payload = request.json
is_payload_valid, message = validate_household_payload(payload)
if not is_payload_valid:
return response_400(
raise BadRequest(
f"Unable to update household #{household_id}; details: {message}"
)

try:

# First, attempt to fetch the existing household
label: str | None = payload.get("label")
household_json: dict = payload.get("data")

household: dict | None = household_service.get_household(
country_id, household_id
)
if household is None:
return response_404(f"Household #{household_id} not found.")

# Next, update the household
updated_household: dict = household_service.update_household(
country_id, household_id, household_json, label
)
return Response(
json.dumps(
{
"status": "ok",
"message": None,
"result": {
"household_id": household_id,
"household_json": updated_household["household_json"],
},
}
),
status=200,
mimetype="application/json",
)
except Exception as e:
raise InternalServerError(
f"An error occurred while updating household #{household_id}. Details: {str(e)}"
)
# First, attempt to fetch the existing household
label: str | None = payload.get("label")
household_json: dict = payload.get("data")

household: dict | None = household_service.get_household(
country_id, household_id
)
if household is None:
raise NotFound(f"Household #{household_id} not found.")

# Next, update the household
updated_household: dict = household_service.update_household(
country_id, household_id, household_json, label
)
return Response(
json.dumps(
{
"status": "ok",
"message": None,
"result": {
"household_id": household_id,
"household_json": updated_household["household_json"],
},
}
),
status=200,
mimetype="application/json",
)
30 changes: 12 additions & 18 deletions policyengine_api/routes/metadata_routes.py
View file
Original file line number Diff line number Diff line change
@@ -1,6 +1,5 @@
import json
from flask import Blueprint, Response
from werkzeug.exceptions import InternalServerError

from policyengine_api.utils.payload_validators import validate_country
from policyengine_api.services.metadata_service import MetadataService
@@ -17,21 +16,16 @@ def get_metadata(country_id: str) -> Response:
Args:
country_id (str): The country ID.
"""
try:
metadata = metadata_service.get_metadata(country_id)
metadata = metadata_service.get_metadata(country_id)

return Response(
json.dumps(
{
"status": "ok",
"message": None,
"result": metadata,
}
),
status=200,
mimetype="application/json",
)
except Exception as e:
raise InternalServerError(
f"An error occurred while fetching metadata for country {country_id}. Details: {str(e)}"
)
return Response(
json.dumps(
{
"status": "ok",
"message": None,
"result": metadata,
}
),
status=200,
mimetype="application/json",
)
55 changes: 24 additions & 31 deletions policyengine_api/routes/simulation_analysis_routes.py
View file
Original file line number Diff line number Diff line change
@@ -1,7 +1,5 @@
from flask import Blueprint, request, Response, stream_with_context
from werkzeug.exceptions import InternalServerError
import json
from policyengine_api.routes.error_routes import response_400
from werkzeug.exceptions import BadRequest
from policyengine_api.utils.payload_validators import validate_country
from policyengine_api.services.simulation_analysis_service import (
SimulationAnalysisService,
@@ -28,7 +26,7 @@ def execute_simulation_analysis(country_id):

is_payload_valid, message = validate_sim_analysis_payload(payload)
if not is_payload_valid:
return response_400(f"Invalid JSON data; details: {message}")
raise BadRequest(f"Invalid JSON data; details: {message}")

currency: str = payload.get("currency")
selected_version: str = payload.get("selected_version")
@@ -43,33 +41,28 @@ def execute_simulation_analysis(country_id):
)
audience = payload.get("audience", "")

try:
analysis = simulation_analysis_service.execute_analysis(
country_id,
currency,
selected_version,
time_period,
impact,
policy_label,
policy,
region,
relevant_parameters,
relevant_parameter_baseline_values,
audience,
)
analysis = simulation_analysis_service.execute_analysis(
country_id,
currency,
selected_version,
time_period,
impact,
policy_label,
policy,
region,
relevant_parameters,
relevant_parameter_baseline_values,
audience,
)

# Create streaming response
response = Response(
stream_with_context(analysis),
status=200,
)
# Create streaming response
response = Response(
stream_with_context(analysis),
status=200,
)

# Set header to prevent buffering on Google App Engine deployment
# (see https://cloud.google.com/appengine/docs/flexible/how-requests-are-handled?tab=python#x-accel-buffering)
response.headers["X-Accel-Buffering"] = "no"
# Set header to prevent buffering on Google App Engine deployment
# (see https://cloud.google.com/appengine/docs/flexible/how-requests-are-handled?tab=python#x-accel-buffering)
response.headers["X-Accel-Buffering"] = "no"
Copy link
Collaborator

Choose a reason for hiding this comment

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

suggestion, non-blocking again strongly suggest we don't iterate again over this, but I think this would be cleaner as a decorator like this. I don't think that is necessarily the right library so that would require a little research.

Suggest we look into how to set headers as part of integrating parsing/serialization automation.


return response
except Exception as e:
raise InternalServerError(
f"An error occurred while executing the simulation analysis. Details: {str(e)}"
)
return response
52 changes: 20 additions & 32 deletions policyengine_api/routes/tracer_analysis_routes.py
View file
Original file line number Diff line number Diff line change
@@ -1,14 +1,12 @@
from flask import Blueprint, request, Response, stream_with_context
from werkzeug.exceptions import InternalServerError
from werkzeug.exceptions import BadRequest
from policyengine_api.utils.payload_validators import (
validate_country,
validate_tracer_analysis_payload,
)
from policyengine_api.routes.error_routes import response_400, response_404
from policyengine_api.services.tracer_analysis_service import (
TracerAnalysisService,
)
import json

tracer_analysis_bp = Blueprint("tracer_analysis", __name__)
tracer_analysis_service = TracerAnalysisService()
@@ -22,37 +20,27 @@ def execute_tracer_analysis(country_id):

is_payload_valid, message = validate_tracer_analysis_payload(payload)
if not is_payload_valid:
return response_400(f"Invalid JSON data; details: {message}")
raise BadRequest(f"Invalid JSON data; details: {message}")

household_id = payload.get("household_id")
policy_id = payload.get("policy_id")
variable = payload.get("variable")

try:
# Create streaming response
response = Response(
stream_with_context(
tracer_analysis_service.execute_analysis(
country_id,
household_id,
policy_id,
variable,
)
),
status=200,
)

# Set header to prevent buffering on Google App Engine deployment
# (see https://cloud.google.com/appengine/docs/flexible/how-requests-are-handled?tab=python#x-accel-buffering)
response.headers["X-Accel-Buffering"] = "no"

return response
except KeyError as e:
"""
This exception is raised when the tracer can't find a household tracer record
"""
return response_404("No household simulation tracer found")
except Exception as e:
raise InternalServerError(
f"An error occurred while executing the tracer analysis. Details: {str(e)}"
)
# Create streaming response
response = Response(
stream_with_context(
tracer_analysis_service.execute_analysis(
country_id,
household_id,
policy_id,
variable,
)
),
status=200,
)

# Set header to prevent buffering on Google App Engine deployment
# (see https://cloud.google.com/appengine/docs/flexible/how-requests-are-handled?tab=python#x-accel-buffering)
response.headers["X-Accel-Buffering"] = "no"

return response
3 changes: 2 additions & 1 deletion policyengine_api/services/tracer_analysis_service.py
View file
Original file line number Diff line number Diff line change
@@ -5,6 +5,7 @@
import re
import anthropic
from policyengine_api.services.ai_analysis_service import AIAnalysisService
from werkzeug.exceptions import NotFound


class TracerAnalysisService(AIAnalysisService):
@@ -80,7 +81,7 @@ def get_tracer(
).fetchone()

if row is None:
raise KeyError("No tracer found for this household")
raise NotFound("No household simulation tracer found")

tracer_output_list = json.loads(row["tracer_output"])
return tracer_output_list
5 changes: 1 addition & 4 deletions tests/python/test_tracer.py
View file
Original file line number Diff line number Diff line change
@@ -2,9 +2,6 @@
from flask import Flask, json
from unittest.mock import patch

from policyengine_api.routes.tracer_analysis_routes import (
execute_tracer_analysis,
)
from policyengine_api.services.tracer_analysis_service import (
TracerAnalysisService,
)
@@ -118,7 +115,7 @@ def test_execute_tracer_analysis_ai_error(
)

assert response.status_code == 500
assert "An error occurred" in json.loads(response.data)["message"]
assert json.loads(response.data)["status"] == "error"


# Test invalid country