fix(profile): exclude connection_url from selectable datasources - #2527
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (6)
WalkthroughThe datasource registry now provides a DataSource-enum-backed selectable list. Profile creation in the CLI and web UI uses this list, excluding ChangesDatasource profile options
Estimated code review effort: 2 (Simple) | ~10 minutes Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Summary
wren profile add --interactiveand--uiboth offerconnection_urlas a datasource choice, but it is not aDataSource— picking it saves a profile that no connector can ever resolve (ValueError: 'connection_url' is not a valid DataSource).get_selectable_datasources()for the two user-facing pickers.get_datasource_options()is left untouched, deliberately — see below.Motivation
get_datasource_options()returns every key ofDATASOURCE_MODELS, and that dict carries one entry which is a connection form, not a datasource. The repo says so itself, intests/test_field_registry.py:That list is fed straight to the pickers:
profile_cli.py→click.Choice(ds_choices, case_sensitive=False)for theData sourcepromptprofile_web.py→datasource_options→<option value="connection_url">in the formSo a user can pick it, and the saved profile is unusable:
Fix
The obvious fix — dropping
connection_urlfromget_datasource_options()— would introduce a secret leak, so I did not do it.profile.py::_registry_sensitive_keysiteratesget_datasource_options()to collect everySecretStrfield, andConnectionUrl.connection_urlis aSecretStr. Removing the entry dropsconnection_urlout of the mask set, andwren profile debugwould start printing full database URLs (user:password@host) in plaintext. The function's docstring states exactly why it spans everything:Change set, measured rather than reasoned about:
set(get_selectable_datasources()) == set(get_datasource_options()) - {"connection_url"}, and noDataSourceenum member is dropped.So the two consumers need two different semantics, and this PR gives them exactly that:
profile_cliprompt,profile_webformget_selectable_datasources()_registry_sensitive_keys(masking)docs.pyDATASOURCE_MODELSdirectlyconnection_url)Verification
tests/test_field_registry.py::test_selectable_datasources_excludes_non_datasource_entries— every option is a realDataSource.tests/test_field_registry.py::test_selectable_datasources_keeps_every_real_datasource— reverse anchor: filtering drops nothing genuine; the delta is exactly{connection_url}.tests/test_profile_web.py::test_form_omits_connection_url_datasource— the rendered form has no<option value="connection_url">.tests/test_profile_cli.py::test_interactive_add_does_not_offer_connection_url— the prompt'sclick.Choicelist excludes it, and still offerspostgres/bigquery.ImportErroron the new function — that is not a red check):assert '<option value="connection_url"' not in resp.text— REDassert 'connection_url' not in [... 'connection_url' ...]— REDtest_form_contains_datasource_optionsstays green under both, so it was not hollowed out.main, verbatim: UI job 55 → 59 passed,tests/test_profile_cli.py+tests/test_profile.py63 → 64 passed (+5 = exactly the new tests; 0 new failures). Lint clean (just lint).tests/test_profile_cli.pyis not run by any CI job (CI runstests/unit/,tests/connectors/*,tests/test_profile_web.py+tests/test_field_registry.py, and the memory/mcp files). The registry and web regressions above are covered by theui testsjob; the CLI one is local-only.License
core/**is Apache-2.0 per the repo LICENSE path map — this contribution is within an Apache-2.0 path.Summary by CodeRabbit
Bug Fixes
connection_urloption from interactive and web-based profile forms.Tests
connection_urlis excluded from both profile creation interfaces.