Skip to content

🐛 Inject API base url by active product 🗃️ ⚠️ #7641

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

Conversation

giancarloromeo
Copy link
Contributor

@giancarloromeo giancarloromeo commented May 6, 2025

What do these changes do?

This PR injects the API base URL by active product, getting it from its configuration currently stored in the DB.

Before, the DIRECTOR_V2_PUBLIC_API_BASE_URL env var was used. This is wrong in case of multiple products served by the same deployment. This caused a wrong OSPARC_API_BASE_URL injected to the dynamic services.

Related issue/s

How to test

Dev-ops

@matusdrobuliak66 Whenever a new product is registered, we should specify the base_url in the new column.

@giancarloromeo giancarloromeo added a:webserver issue related to the webserver service a:director-v2 issue related with the director-v2 service labels May 6, 2025
@giancarloromeo giancarloromeo added this to the Bazinga! milestone May 6, 2025
@giancarloromeo giancarloromeo self-assigned this May 6, 2025
Copy link

codecov bot commented May 6, 2025

Codecov Report

Attention: Patch coverage is 60.46512% with 17 lines in your changes missing coverage. Please review.

Project coverage is 87.32%. Comparing base (fb428dc) to head (098dd7e).
Report is 1 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #7641      +/-   ##
==========================================
- Coverage   87.72%   87.32%   -0.41%     
==========================================
  Files        1788     1695      -93     
  Lines       68979    65891    -3088     
  Branches     1134      958     -176     
==========================================
- Hits        60511    57537    -2974     
+ Misses       8156     8085      -71     
+ Partials      312      269      -43     
Flag Coverage Δ
integrationtests 61.03% <41.37%> (-3.90%) ⬇️
unittests 87.09% <60.46%> (+0.15%) ⬆️
Components Coverage Δ
api ∅ <ø> (∅)
pkg_aws_library ∅ <ø> (∅)
pkg_dask_task_models_library ∅ <ø> (∅)
pkg_models_library 92.80% <ø> (ø)
pkg_notifications_library 85.26% <ø> (ø)
pkg_postgres_database 88.41% <ø> (ø)
pkg_service_integration ∅ <ø> (∅)
pkg_service_library 72.85% <0.00%> (-0.18%) ⬇️
pkg_settings_library ∅ <ø> (∅)
pkg_simcore_sdk 85.72% <ø> (+0.23%) ⬆️
agent 96.46% <ø> (ø)
api_server 92.50% <ø> (ø)
autoscaling 96.08% <ø> (ø)
catalog 92.64% <ø> (ø)
clusters_keeper 99.25% <ø> (ø)
dask_sidecar 91.37% <ø> (ø)
datcore_adapter 98.12% <ø> (ø)
director 76.80% <ø> (ø)
director_v2 85.43% <100.00%> (-5.68%) ⬇️
dynamic_scheduler 96.76% <ø> (ø)
dynamic_sidecar 90.15% <ø> (ø)
efs_guardian 89.79% <ø> (ø)
invitations 93.28% <ø> (ø)
payments 92.66% <ø> (ø)
resource_usage_tracker 89.12% <ø> (-0.11%) ⬇️
storage 87.49% <ø> (ø)
webclient ∅ <ø> (∅)
webserver 86.04% <88.88%> (+0.01%) ⬆️

Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update fb428dc...098dd7e. Read the comment docs.

🚀 New features to boost your workflow:
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@giancarloromeo giancarloromeo changed the title 🐛 Inject API base url by active product 🐛 Inject API base url by active product ⚠️ May 6, 2025
@giancarloromeo giancarloromeo requested a review from Copilot May 6, 2025 14:00
Copy link
Contributor

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR injects the API base URL for a product by actively retrieving and processing the base URL from configuration and the database. The key changes include:

  • Introducing a new error (ProductBaseUrlNotFoundError) to handle missing base URLs.
  • Adding a helper function (_make_api_server_base_url) and an async service (get_product_api_base_url) to parse and ensure the API URL is correctly prefixed.
  • Extending the repository, model, RPC interface, and database migration to support the new base_url field.

Reviewed Changes

Copilot reviewed 10 out of 10 changed files in this pull request and generated no comments.

Show a summary per file
File Description
services/web/server/src/simcore_service_webserver/products/errors.py Added new error class for missing product base URL
services/web/server/src/simcore_service_webserver/products/_service.py Added helper for processing base URL and an async RPC method for retrieving the API base URL
services/web/server/src/simcore_service_webserver/products/_repository.py Introduced a new repository method for retrieving the base URL from the database
services/web/server/src/simcore_service_webserver/products/_models.py Updated the Product model to include a new base_url field and updated JSON schema defaults accordingly
services/web/server/src/simcore_service_webserver/products/_controller/rpc.py Added an RPC endpoint for retrieving the product API base URL
services/director-v2/src/simcore_service_director_v2/modules/osparc_variables/substitutions.py Replaced static URL settings with dynamic lookup via the new API base URL RPC
packages/service-library/src/servicelib/rabbitmq/rpc_interfaces/webserver/products.py Created a new RPC interface function to query the product API base URL
packages/pytest-simcore/src/pytest_simcore/helpers/faker_factories.py Updated fake product data to include a base_url field
packages/postgres-database/src/simcore_postgres_database/models/products.py Modified the products database model to add a new base_url column
packages/postgres-database/src/simcore_postgres_database/migration/versions/972981ff823a_add_base_url_column_to_products.py Added a migration to create the base_url column in the products table
Comments suppressed due to low confidence (1)

services/web/server/src/simcore_service_webserver/products/_service.py:151

  • [nitpick] Consider adding an explicit type annotation for the parameter 'product_name' (e.g. product_name: ProductName) for consistency with the rest of the codebase and improved type clarity.
async def get_product_api_base_url(app: web.Application, *, product_name) -> str:

@giancarloromeo giancarloromeo changed the title 🐛 Inject API base url by active product ⚠️ 🐛 Inject API base url by active product 🗃️ ⚠️ May 7, 2025
@giancarloromeo giancarloromeo marked this pull request as ready for review May 7, 2025 11:23
Copy link
Collaborator

@matusdrobuliak66 matusdrobuliak66 left a comment

Choose a reason for hiding this comment

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

Thanks 👍 few comments

Copy link
Collaborator

@matusdrobuliak66 matusdrobuliak66 left a comment

Choose a reason for hiding this comment

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

Thanks 👍 But I meant the RPC function directly (the one used by other services). Check how other functions in that rpc module are tested osparc-simcore/packages/service-library/src/servicelib/rabbitmq/rpc_interfaces/* - for example, search for this test: test_rpc_client_mark_project_as_job.

Copy link

sonarqubecloud bot commented May 8, 2025

Copy link
Member

@sanderegg sanderegg left a comment

Choose a reason for hiding this comment

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

can we discuss this concept?

Can we not pass that information over?
the webserver can now the scheme and url and hostname from the request that comes in I believe. I'd like to discuss this.

can't these be passed via an argument in one call?

@giancarloromeo
Copy link
Contributor Author

After an offline discussion with @sanderegg, we will try a simpler solution for that.

@giancarloromeo giancarloromeo marked this pull request as draft May 8, 2025 08:35
@giancarloromeo
Copy link
Contributor Author

Will continue #7619

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
a:director-v2 issue related with the director-v2 service a:webserver issue related to the webserver service
Projects
None yet
Development

Successfully merging this pull request may close these issues.

api-keys: DIRECTOR_V2_PUBLIC_API_BASE_URL does not account for active product!
4 participants