Skip to content

Fix/restrict org id filtering#333

Open
PavanRaghavendraKulkarni wants to merge 9 commits into
mainfrom
fix/restrict-org-id-filtering
Open

Fix/restrict org id filtering#333
PavanRaghavendraKulkarni wants to merge 9 commits into
mainfrom
fix/restrict-org-id-filtering

Conversation

@PavanRaghavendraKulkarni

@PavanRaghavendraKulkarni PavanRaghavendraKulkarni commented Jun 24, 2026

Copy link
Copy Markdown
Contributor

Pull Request

Description

This PR fixes regional forecast and generation queries by resolving region names to location objects before querying the database, and restricts organization filtering strictly to SITE and SUBSTATION location types to keep regional endpoints unaffected.

Context

Prior to this change:

  1. Region endpoints (like /solar/ruvnl/generation and /solar/ruvnl/forecast) passed the region name string directly where location UUIDs were expected, causing database query failures.
  2. The organization filter was applied to region queries when an auth token was present, restricting public endpoints.
  3. If a region name was invalid, the API returned an internal 500 error instead of a clean 404.

Key Changes

  • Backend Client (client.py):
    • Confined organization ID filtering strictly to SITE and SUBSTATION location types in get_locations.
    • Restrained _check_user_access checks to SITE location types in get_actual_generation and put_actual_generation, preventing regional queries from being filtered or blocked.
  • Regions Router (router.py):
    • Introduced SITE_OBSERVER_NAME = "site_api" constant.
    • Added a reusable helper _resolve_region to resolve a region name to its location object, raising a clean 404 Not Found if the region doesn't exist.
    • Refactored get_historic_timeseries_route, get_forecast_timeseries_route, and get_forecast_csv to use the helper.
    • Hardcoded SITE_OBSERVER_NAME in get_actual_generation for regional endpoints to resolve the missing observer error.

How Has This Been Tested?

1. Automated Tests

Added a new unit test suite specifically for the regions router:

  • src/quartz_api/internal/service/regions/test_router.py

Run the router tests using:

uv run pytest src/quartz_api/internal/service/regions/test_router.py src/quartz_api/internal/service/sites/test_router.py

All 28 tests passed successfully.

2. Manual Verification

Verified API responses locally:

  • GET /solar/regions: Returns valid regions (200 OK).
  • GET /solar/ruvnl_solar/generation: Returns actual values (200 OK).
  • GET /solar/ruvnl_solar/forecast: Returns forecast values (200 OK).
  • GET /solar/invalid_region/forecast: Returns clean 404 Not Found with {"detail": "Region or GSP 'invalid_region' not found."}.
  • Yes

Checklist:

  • My code follows OCF's coding style guidelines
  • I have performed a self-review of my own code
  • I have made corresponding changes to the documentation
  • I have added tests that prove my fix is effective or that my feature works
  • I have checked my code and corrected any misspellings

@PavanRaghavendraKulkarni PavanRaghavendraKulkarni marked this pull request as ready for review June 25, 2026 06:50
)

router = APIRouter(tags=[pathlib.Path(__file__).parent.stem.capitalize()])
SITE_OBSERVER_NAME = "site_api"

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

The observer is called "site_api"? Does this mean every user writes their data to the same observer? I wonder if we're not better off with an observer per user...

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Yes, I discussed this with @peterdudfield . One of the main challenges was determining the observer name for a location when the user comes through the API endpoint. As a solution, we set it to site-api.

For debugging and audit purposes, we always have the auth data available, so we can determine who accessed the endpoint or performed the write operation based on that information.

That said, your suggestion makes sense, and I'm open to discussing a better approach if we think there's a more suitable solution.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I think for adani for this route, this should keep it simple

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants