feat: Instance-Aware Networking (RequestInterceptor) - #677
RawanMatar89 wants to merge 28 commits into
Conversation
Develop to main v1.4
Develop to main v1.5
Develop to main v1.5.1
Develop to main. Release v2
Develop to main v2.1
Release v2.2 (Ulmo.1)
fix: typo in translation.py inline comments
Release v2.3 (Verawood.1)
Adds the Sendable Tenant value type and TenantsConfig container that later multi-tenant layers build on: per-tenant API/SSO/OAuth config, UI feature flags (UIComponentsConfig, now @unchecked Sendable), and raw branding fields for a later theming layer to render. Tenant(dictionary:) parses config.yaml's new TENANTS block. Not included here: remote catalog fetching, tenant selection/session state, and anything reading TenantProvider — those land as separate PRs. TENANTS data in config.yaml is illustrative, not real tenants.
Renames the Tenant domain model and its wire schema to Instance, per community naming feedback. Tenant -> Instance, TenantsConfig -> InstancesConfig, TenantProvider -> InstanceProvider, TenantThemeColors -> InstanceThemeColors; config.yaml's TENANTS block -> INSTANCES, TENANT_NAME -> INSTANCE_NAME, IS_SWITCH_TENANT_LOGIN_ENABLED -> IS_SWITCH_INSTANCE_LOGIN_ENABLED. Pure rename, no behavior change. KEY is left as-is (already generic). SSO_URL_SUCCESSFUL_LOGIN/successfulSSOLoginURL is untouched here too -- its rename to SSO_FINISHED_URL is separate, upcoming work.
Adds TenantStore: the single source of truth for the selected tenant and the tenant catalog, persisted to UserDefaults and posting .tenantDidChange for observers. Auto-selects the sole tenant on a single-tenant catalog, since there's no picker to choose it through. Registered in NetworkAssembly (TenantStore/TenantProvider). Not consumed by anything yet — RequestInterceptor and the rest of the storage/session layers read it starting in later PRs. Also fixes a real gap found while extracting this: the original TenantStoreTests.swift was never added to Core.xcodeproj, so it silently never ran as part of CoreTests.
Renames TenantStore -> InstanceStore (and its test file), plus the DI registrations in NetworkAssembly.swift, per the community's naming feedback. Matches the rename already applied on top of infra/01-tenant-model.
Adds TenantApiService to fetch the tenant catalog JSON from a remote endpoint, and TenantConfigLoader to orchestrate a live-fetch-first, cache-fallback policy: try the network first, fall back to the last successfully-parsed config on failure. The remote JSON uses lowercase snake_case keys, so TenantsConfig gains a normalizeRemoteKeys step that maps them to the existing YAML upper-snake-case keys and reuses Tenant(dictionary:) for parsing — no separate remote-specific model. Registers TenantApiServiceProtocol and TenantConfigLoader in NetworkAssembly. Nothing calls TenantConfigLoader.load() yet; wiring it into app launch is a later PR.
Renames TenantApiService -> InstanceApiService, TenantConfigLoader -> InstanceConfigLoader, TenantsConfigJSONTests -> InstancesConfigJSONTests, and the remote JSON schema's wire keys (tenant_name -> instance_name, is_switch_tenant_login_enabled -> is_switch_instance_login_enabled, the TENANTS/tenants catalog wrapper -> INSTANCES/instances), per the community's naming feedback.
Adds an optional INSTANCES_CATALOG_URL config key and a config.json
bundled with the app. InstanceConfigLoader now falls back to this
bundled catalog when no URL is configured (or the live fetch and
cache both fail), instead of falling straight to an empty catalog.
- ConfigProtocol.instancesCatalogURL: URL? (nil when unconfigured)
- InstanceApiService.url is now optional; throws .notConfigured when nil
- InstanceConfigLoader.loadBundled() reads config.json from the bundle
- process_config.py copies json_files (config.json) into the built
bundle alongside config.plist
- default_config/{dev,stage,prod}/file_mappings.yaml gain a json_files
list; only dev ships an actual config.json for now
- Regenerated ConfigProtocolMock (Mockolo) in every module to add the
new instancesCatalogURL property
Also removes API_HOST_URL_HIDDEN_LOGIN entirely: the Instance field
(baseURLHiddenLogin), its InstanceKeys/CodingKeys entries, remote
JSON key mapping, and the example value in config.yaml/config.json.
It was unused outside InstancesConfig.swift's own parsing.
Same restructuring as infra/01-tenant-model: LOGO_URL/HEADER_BACKGROUND_URL now live inside THEME (as logo_url/header_background_url, siblings of light/dark) instead of their own top-level instance fields. Removed the now-unused remoteToYAMLKeyMap entries for them -- with no map entry they pass through normalizeRemoteKeys unchanged (lowercase), landing exactly where Instance.init?(dictionary:) now looks for them. - default_config/dev/config.json and config.yaml's example instance updated to match: THEME.light/dark now carry only accent_color (ThemeColorSet.derived(fromHex:light:dark:) derives the rest), logo_url moved under THEME, and the flat color field is dropped. - Updated InstancesConfigJSONTests to the nested shape.
Same fix as infra/01-tenant-model: NAME/COLOR get explicit uppercase raw values, and THEME's LIGHT/DARK/LOGO_URL/HEADER_BACKGROUND_URL wrapper keys go uppercase instead of the lowercase shortcut that let them pass the remote-key normalizer unmapped. Added matching entries to remoteToYAMLKeyMap for all six so remote catalog responses (still naturally lowercase/snake_case) keep normalizing correctly -- without this, a remote instance missing an explicit uppercase NAME would have silently been dropped as malformed. Left untouched: the light/dark palette dicts' own internal field names (accent_color and siblings) -- Theme's already-shipped contract from the merged theme/01-theme-engine PR, not this file's to rename. config.json's own casing isn't touched here -- it's about to be restructured wholesale (app-level keys + INSTANCES wrapper) in the next commit, so fixing it twice would be wasted work.
…le replace) Replaces the old live-fetch-first/cache/bundled/empty fallback chain with product's actual merge rule: - Baseline = last-cached successful remote response, else the bundled catalog, else empty. - A live fetch that returns >0 instances wholesale-replaces the baseline and becomes the new cache -- so a later offline launch shows the same catalog the user had last time, not a reset to the bundled default. - A fetch that fails, or succeeds with zero instances, leaves the baseline (and the cache) untouched -- a reachable-but-empty catalog is not the same as an unreachable one and must not wipe out a good cache. - No catalog URL configured -> InstanceApiService throws immediately, same fallback-to-baseline path as any other fetch failure. fatalError guard for "no catalog URL and no valid local instance" is deliberately not here -- that's launch-sequencing (RouteController), not loader concern; it belongs with the DI/launch wiring work.
- default_config/{dev,stage,prod}/config.json restructured to
{app-level keys, INSTANCES}: FIREBASE/FACEBOOK/MICROSOFT/GOOGLE/
APPLE_SIGNIN/BRANCH/URI_SCHEME/APP_STORE_ID/INSTANCES_CATALOG_URL
alongside the instance array under INSTANCES. Remote catalog
responses are unaffected -- still a bare array, only the local file
gained the wrapper.
- config.yaml deleted in all three environments; nothing read it
anymore once process_config.py's build-time source moved to JSON
(dev's INSTANCES block there was already dead code -- confirmed
nothing but InstanceStoreTests hand-constructs Instance dictionaries
directly, no runtime path ever parsed it).
- process_config.py: PlistManager now parses json_files via
json.load() instead of config_files via yaml.safe_load() --
load_config()/yaml_to_plist() -> load_config()/json_to_plist(),
otherwise unchanged (same merge_dicts, same plist output shape).
whitelabel.py's own PlistManager call site (imports the class from
this file) updated to match -- it was about to silently feed YAML
paths into a JSON-only loader.
- file_mappings.yaml (all three envs): dropped the now-unused `files:`
(YAML) list, kept `json_files:`.
- NetworkAssembly.swift: InstanceApiServiceProtocol's URL now comes
from the bundled config.json directly (new
InstanceConfigLoader.bundledCatalogURL()), not
ConfigProtocol.instancesCatalogURL -- that property is left in place
but unused for now, to be removed together with the rest of
ConfigProtocol's app-level shrinkage (firebase/facebook/etc. moving
off YAML-via-Config) rather than regenerating all 9 Mockolo
ConfigProtocolMocks twice for two separate small removals.
Deliberately NOT touched: config_settings.yaml and file_mappings.yaml
themselves stay YAML -- build-routing metadata, not app config content,
never part of what was agreed to move to JSON. Documentation/
CONFIGURATION_MANAGEMENT.md still describes the old YAML-based flow in
detail and needs a real rewrite, not just the one-line README pointer
fixed here -- flagging rather than attempting that in this commit.
…IENT_ID as a temporary bridge Config (the sole ConfigProtocol implementation still wired into DI pre-PR-9) hard-fatalError()s on launch if these 4 top-level plist keys are missing. Removing them from config.yaml in the YAML-retirement commit broke app launch entirely in the current (PR-4-without-PR-9) state, since InstanceAwareConfig doesn't exist yet to take over. Bridge values mirror the example instance's own values (dev) / the same placeholder pattern already used elsewhere in this file (stage, prod). Remove once PR-9's InstanceAwareConfig replaces Config as the wired-in ConfigProtocol and these top-level keys are no longer read.
…erceptor) Reuses the tenants branch's already-built host-rewrite approach (renamed tenant -> instance): API is still constructed once against the app's default ConfigProtocol.baseURL, and RequestInterceptor.adapt(...) re-targets any request built against that host at the currently selected instance's baseURL, preserving path/query/fragment. Requests that already point elsewhere (SSO webviews, third-party SDKs) are left untouched. refreshToken(...) prefers the selected instance's baseURL/ oAuthClientId, falling back to the app config for single-instance deployments or before an instance is picked. instanceStore comes in via the InstanceProvider protocol, already registered in DI by PR-2 -- no new DI wiring beyond passing it through to RequestInterceptor's initializer.
|
Thanks for the pull request, @RawanMatar89! This repository is currently maintained by Once you've gone through the following steps feel free to tag them in a comment and let them know that your changes are ready for engineering review. 🔘 Get product approvalIf you haven't already, check this list to see if your contribution needs to go through the product review process.
🔘 Provide contextTo help your reviewers and other members of the community understand the purpose and larger context of your changes, feel free to add as much of the following information to the PR description as you can:
🔘 Get a green buildIf one or more checks are failing, continue working on your changes until this is no longer the case and your build turns green. 🔘 Update the status of your PRYour PR is currently marked as a draft. After completing the steps above, update its status by clicking "Ready for Review", or removing "WIP" from the title, as appropriate. Where can I find more information?If you'd like to get more details on all aspects of the review process for open source pull requests (OSPRs), check out the following resources: When can I expect my changes to be merged?Our goal is to get community contributions seen and reviewed as efficiently as possible. However, the amount of time that it takes to review and merge a PR can vary significantly based on factors such as:
💡 As a result it may take up to several weeks or months to complete a review and merge your PR. |
|
@RawanMatar89 Stop, nooooooo... And inside a JSON you are setup all the instances. As example something like this: We need to support JSON file by URL or locally by DIRECTORY_FILE value. We need them both. Please, do not remove config.yaml🙌 |
@ivan-stepanok 😄 I don't think we're actually disagreeing on the shape here, let me untangle it: The URL-or-local-file JSON directory you're describing is exactly what's already built on the branch this PR stacks on — The one thing that did change is the outer local app config — instead of keeping that as YAML and adding a separate JSON directory format on top, it's now JSON too ( If there's something specific relying on |
|
Thanks, @RawanMatar89 – the remote catalog looks good, and I'm fine with JSON becoming the build-time config source. The plist output is unchanged, so nothing shifts at runtime. A few things I'd like in before this merges, though: A migration path. Anyone upgrading with an existing YAML config in their Docs in the same PR. Pick one format. None of this is a blocker on the approach – just don't want to hand operators a silent break and stale docs. |
|
Thank you @IvanStepanok 🙏🏻 all three are fair, and none of them are done yet, so no pushback: Migration path: confirmed the gap. I'll add an explicit check up front and ship a small converter script alongside it, since the schema didn't reshape anything moving formats. Will push all three as part of this PR before asking for another look. 🙌 |
process_config.py previously just fell into the generic "config files not found" exit when config.json didn't exist yet, with no indication that config.yaml (retired in 85aa83f) was the reason. fail_if_unmigrated_yaml_config now detects a leftover config.yaml with no sibling config.json and fails with a message naming both files and the converter to run. Add config_script/yaml_to_json_config.py: a small standalone converter (reuses this project's existing PyYAML/json deps, no new dependency) so operators don't hand-write the JSON. See Ivan's review on openedx#677.
Last two YAML files in default_config/ -- everything else there has been JSON since 85aa83f. Converts config_settings.yaml -> config_settings.json and each environment's file_mappings.yaml -> file_mappings.json (via yaml_to_json_config.py), updates process_config.py and whitelabel.py (which duplicates the same config_settings/file_mappings parsing) to read JSON instead. Addresses Ivan's review on openedx#677: migration path, docs, and one format instead of JSON+YAML side by side in the same folder.
|
Hi @IvanStepanok, the three changes from your last review are in. Pushed and ready for another look. 👀 One thing worth flagging before you dig in: these are stacked PRs, so this one's diff only makes sense on top of the branches below it. Branch names show the order — infra/01-tenant-model → infra/02-tenant-store → infra/03-remote-config-fetch → infra/04-instance-aware-networking (this one). Could we review and approve/merge in that order, starting from infra/01-tenant-model? Reviewing this one in isolation will show you everything from the branches underneath it too, which makes it harder to tell what's actually new here. One naming note: any branch still says "tenant" was created before we agreed on the tenant → instance rename, and its content is already renamed to instance throughout. Just a stale branch name, not a leftover reference. |
Base branch
infra/03-remote-config-fetchDescription
Routes outgoing requests through the currently selected instance's host and credentials instead of the app-level config, and refreshes tokens against that instance too. This is where the instance model (PR-1) and instance selection (PR-2) actually start affecting network traffic — before this PR,
InstanceStoretracked a selection but nothing used it.What's in this PR
RequestInterceptorinstanceStore: InstanceProviderdependency (already registered by PR-2 — no new DI wiring needed).adapt(...)now rewrites the host/scheme/port of any request built against the app's base URL to the selected instance'sbaseURL, via a newrewriteHostIfNeeded(_:). Path and query are left untouched. A request with no selected instance, or one not built against the app base URL in the first place (e.g. an SSO webview, a third-party SDK call), passes through unmodified.refreshToken(...)now prefers the selected instance'sbaseURL/oAuthClientIdover the app-level config, falling back to app config when no instance is selected.token_typestays app-level (unchanged) — no instance-level override for that field exists yet.DI
NetworkAssembly.swift:RequestInterceptorregistration now resolves and passesInstanceProvider.Tests
RequestInterceptorTests.swift(new): selected-instance host rewrite (host/path/query preserved correctly), no-instance-selected passthrough, and non-app-base-URL requests left untouched.Out of scope