Migrate `household` endpoints to new API structure #2038

anth-volk · 2024-12-04T00:14:22Z

Fixes #1988.
Fixes #2034.

This PR migrates the household endpoints to the new API structure.

policyengine_api/routes/household_routes.py

tests/python/test_household.py

anth-volk · 2024-12-07T10:04:19Z

Hey Mike,

Thanks again for your review on this. Here's what I'd like to propose as a roadmap forward for both this PR and the PR migrating policy endpoints in a similar way (currently open as #2025). Feel free to let me know what you think, then I can proceed:

Immediate, as part of PR
- Bug fixes raised as part of review
- Migrate to specifying full endpoint address upon registration (as suggested by Mike)
  - Update PR to apply this structure to household endpoints
  - Update policy endpoint PR to also apply this structure
  - Also apply this structure to previously migrated endpoints (economy, AI)
- Look into basic Flask validators and apply where necessary
- Create generic exception handlers and use those in place of manually specified error responses (assuming we can easily isolate standard errors; if not, move to “In future”)
In progress elsewhere
- Keep existing print statements, and then migrate these to log statements in logging PR (currently in progress, but no PR open)
- Test improvements (see Improve testing #2021)
In near future (i.e., PR following this and policy endpoint refactor)
- Chat further about improvements to testing
- Create a clearer specification regarding keys like “status”, “message”, “result” – may require coordination with relevant stakeholders (e.g., Nikhil)
- Look into implementing Marshmallow schema validation
In longer-term future
- Discuss how to flatten API structure, especially multi-nested endpoints

mikesmit · 2024-12-07T18:53:36Z

I approve of your plan. I have added some more discussion above, but I don't want to block this work on it.

…pdate tests to mirror that behavior

mikesmit

I added some suggestions/nitts to simplify but nonblocking.

policyengine_api/routes/economy_routes.py

policyengine_api/routes/error_routes.py

policyengine_api/routes/household_routes.py

anth-volk · 2024-12-16T19:14:27Z

@mikesmit I'd like to make these changes an example moving forward, so I will go back and address your suggestions probably tomorrow, and would love to incorporate another round of changes before moving forward. In particular, I would actually like to refactor the error handlers, as they also require a double exception raising pattern within the route blueprints (happy to point out a line specifically if desired) and I think the structure could be better.

mikesmit

I didn't see quite what I was expecting. Requesting feedback on why that happened in the review (I.e. my feedback was unclear, my feedback was not good, we both agree on the direction but the change will happen later, etc.)

policyengine_api/routes/error_routes.py

policyengine_api/routes/household_routes.py

anth-volk · 2024-12-19T12:23:30Z

This failing test is (I'm guessing) driven by PolicyEngine/policyengine-core#321.

mikesmit

Added a few notes for future consideration, but strongly suggest we move forward to unblock and iterate.

policyengine_api/routes/economy_routes.py

mikesmit · 2024-12-19T17:28:35Z

policyengine_api/routes/household_routes.py

+    )
+    if household is None:
+        raise NotFound(f"Household #{household_id} not found.")
+    else:


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

mikesmit · 2024-12-19T17:33:32Z

policyengine_api/routes/household_routes.py

+        country_id (str): The country ID.
+        household_id (int): The household ID.
+    """
+    print(f"Got request for household {household_id} in country {country_id}")


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.

Reduces clutter/duplication

reduces cases where we forget to manually add the log message

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)

policyengine_api/routes/household_routes.py

mikesmit · 2024-12-19T17:41:43Z

policyengine_api/routes/simulation_analysis_routes.py

-        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"


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.

anth-volk · 2024-12-20T19:37:20Z

@mikesmit Just merged. Thanks again for your help throughout this process. Hoping this unblocks your upcoming PR.

anth-volk requested a review from nikhilwoodruff December 4, 2024 00:14

MaxGhenis requested a review from mikesmit December 4, 2024 20:31

mikesmit reviewed Dec 4, 2024

View reviewed changes

anth-volk changed the title ~~Migrate household endpoints to new API structure~~ Migrate household endpoints to new API structure Dec 6, 2024

mikesmit mentioned this pull request Dec 7, 2024

Migrate policy endpoints to new API structure #2025

Open

anth-volk added 9 commits December 11, 2024 14:46

feat: Initial stages of migration of household endpoints

43d6a96

feat: Migrate post_household to new structure

5dfc1b0

feat: Migrate update household endpoint

506d233

chore: Format

ffb42ec

fix: Fix minor bugs

0d3473e

fix: Modify validate_country to return 400 on country not found and u…

2129cda

…pdate tests to mirror that behavior

feat: Refactor household endpoints to match new structure

6bd8420

fix: Refactor fixtures

3e2a8a8

fix: Adjust test to handle added validation

a3a78fc

anth-volk force-pushed the fix/1988-migrate-household branch from eb430a1 to a3a78fc Compare December 11, 2024 13:47

anth-volk added 5 commits December 12, 2024 22:30

fix: Redo blueprint generation

de96431

fix: Remove erroneous print statement

c0fdde9

feat: Basic Werkzeug route validators

f789559

feat: Error handling module and application to routes

f0d2a6e

fix: Properly register error routes

e7737c6

anth-volk mentioned this pull request Dec 13, 2024

Utilize a technology like Marshmallow to validate inbound requests #2054

Open

anth-volk added 7 commits December 13, 2024 21:39

fix: Remove unused comment code

082d7cc

fix: Redefine metadata route against convention

6487058

feat: Return household JSON in household updater func and update test

c085d1d

test: Fix some failing tests, add tests for error handlers

ca4ad9f

fix: Fix tracer tests

7c24017

fix: Fix failing simulation analysis test

c668a92

fix: Fix manual raising within try-catch blocks

a40e430

fix: Fix final household tests

7cc72fe

anth-volk requested a review from mikesmit December 14, 2024 00:37

fix: Fix tests

adbfaa9

mikesmit approved these changes Dec 16, 2024

View reviewed changes

mikesmit mentioned this pull request Dec 16, 2024

Migrate user_profile endpoints to new API structure #1989

Open

anth-volk added 3 commits December 18, 2024 02:06

test: Fix tests

0d931a3

feat: Use error response builder

2e70641

chore: Refactor against new error implementation

0f39c1b

anth-volk requested a review from mikesmit December 18, 2024 01:37

mikesmit requested changes Dec 18, 2024

View reviewed changes

policyengine_api/routes/error_routes.py Show resolved Hide resolved

policyengine_api/routes/household_routes.py Outdated Show resolved Hide resolved

fix: Improve error handling

3268c82

mikesmit approved these changes Dec 19, 2024

View reviewed changes

anth-volk merged commit 948f336 into master Dec 20, 2024
4 checks passed

anth-volk deleted the fix/1988-migrate-household branch December 20, 2024 19:36

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Migrate `household` endpoints to new API structure #2038

Migrate `household` endpoints to new API structure #2038

anth-volk commented Dec 4, 2024 •

edited

Loading

anth-volk commented Dec 7, 2024

mikesmit commented Dec 7, 2024

mikesmit left a comment

anth-volk commented Dec 16, 2024

mikesmit left a comment

anth-volk commented Dec 19, 2024

mikesmit left a comment

mikesmit Dec 19, 2024

mikesmit Dec 19, 2024

mikesmit Dec 19, 2024

anth-volk commented Dec 20, 2024

Migrate household endpoints to new API structure #2038

Migrate household endpoints to new API structure #2038

Conversation

anth-volk commented Dec 4, 2024 • edited Loading

anth-volk commented Dec 7, 2024

mikesmit commented Dec 7, 2024

mikesmit left a comment

Choose a reason for hiding this comment

anth-volk commented Dec 16, 2024

mikesmit left a comment

Choose a reason for hiding this comment

anth-volk commented Dec 19, 2024

mikesmit left a comment

Choose a reason for hiding this comment

mikesmit Dec 19, 2024

Choose a reason for hiding this comment

mikesmit Dec 19, 2024

Choose a reason for hiding this comment

mikesmit Dec 19, 2024

Choose a reason for hiding this comment

anth-volk commented Dec 20, 2024

Migrate `household` endpoints to new API structure #2038

Migrate `household` endpoints to new API structure #2038

anth-volk commented Dec 4, 2024 •

edited

Loading