feat: add system-wide default for Geometry Viewer custom tile provider#10198
Open
asheshv wants to merge 3 commits into
Open
feat: add system-wide default for Geometry Viewer custom tile provider#10198asheshv wants to merge 3 commits into
asheshv wants to merge 3 commits into
Conversation
Follow-up to pgadmin-org#10142. Geometry Viewer's custom tile provider was only configurable per-user, with no way for an administrator to set an organization-wide default (e.g. an internal tile server) that applies to every user out of the box. Add DEFAULT_GEOMETRY_VIEWER_PROVIDER to config.py and source the five custom_tile_* preference defaults from it instead of hardcoded literals. Preference.get() only reads a per-user DB row if the user has explicitly saved one, otherwise falling back to this default - so this is pure override semantics: a user's own saved preference always wins, and there is no second/competing provider entry, so no naming-conflict surface is introduced.
…rrides DEFAULT_GEOMETRY_VIEWER_PROVIDER is replaced wholesale by config_local.py/config_distro.py/PGADMIN_CONFIG_*, not merged key-by-key. Direct dict indexing crashed preference registration on any partial override (KeyError) or non-dict value (AttributeError), and an out-of-range/wrong-type crs or max_zoom would have reached the frontend unvalidated. Move the defaulting logic into resolve_geometry_viewer_provider_defaults(), computed fresh each time register_query_tool_preferences() runs (previously computed once at import time, which also made it unmockable in tests): falls back per-field for missing/wrong-type values, validates crs against the 3 supported choices, and validates max_zoom is a non-bool int in [0, 25]. Adds test_geometry_viewer_provider_defaults.py covering fully valid, partial, non-dict, invalid crs, out-of-range/non-int/bool max_zoom, and non-string field scenarios - run via the project's actual regression harness (not just an ad hoc script).
WalkthroughAdds a configurable system-wide default for Geometry Viewer custom tile provider preferences, validates configured values, applies them during preference registration, tests fallback behavior, and documents precedence over saved user preferences. ChangesGeometry Viewer defaults
Estimated code review effort: 2 (Simple) | ~10 minutes 🚥 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@web/pgadmin/tools/sqleditor/tests/test_geometry_viewer_provider_defaults.py`:
- Line 59: Correct the continuation indentation in the geometry viewer provider
defaults test, specifically the closing `}))` lines at the repeated locations,
so they comply with pycodestyle E126. Align each continuation consistently with
its surrounding call structure without changing the test logic.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 0dd7b056-2f8a-41c7-9c04-bb1df8388d29
📒 Files selected for processing (4)
docs/en_US/preferences.rstweb/config.pyweb/pgadmin/tools/sqleditor/tests/test_geometry_viewer_provider_defaults.pyweb/pgadmin/tools/sqleditor/utils/query_tool_preferences.py
Factor the repeated fallback-defaults dict into _ALL_FALLBACK instead of fighting hanging-indent rules on deeply nested inline literals - CI's pycodestyle run caught E126 (over-indented) on the original nested dicts; a same-style de-indent then hit E123 (closing bracket mismatch), since this repo's .pycodestyle ignore list overrides pycodestyle's implicit defaults and leaves both checks active.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
web/pgadmin/tools/sqleditor/tests/test_geometry_viewer_provider_defaults.py (1)
36-41: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAddress the RUF012 mutable class attributes.
_ALL_FALLBACKand_partial_override_expectedare mutable dictionaries stored on the test class, which Ruff flags under RUF012. Addfrom typing import ClassVarand annotate these shared fixtures:_ALL_FALLBACK: ClassVar = {...}and_partial_override_expected: ClassVar = dict(_ALL_FALLBACK).🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@web/pgadmin/tools/sqleditor/tests/test_geometry_viewer_provider_defaults.py` around lines 36 - 41, In the test class, import ClassVar from typing and annotate both shared dictionary fixtures, _ALL_FALLBACK and _partial_override_expected, as ClassVar while preserving their existing values and initialization.Source: Linters/SAST tools
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@web/pgadmin/tools/sqleditor/tests/test_geometry_viewer_provider_defaults.py`:
- Around line 36-41: In the test class, import ClassVar from typing and annotate
both shared dictionary fixtures, _ALL_FALLBACK and _partial_override_expected,
as ClassVar while preserving their existing values and initialization.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: bbdda73c-ac55-434b-a050-4007107bcf15
📒 Files selected for processing (1)
web/pgadmin/tools/sqleditor/tests/test_geometry_viewer_provider_defaults.py
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Follow-up to #10142. Geometry Viewer's custom tile provider was only configurable per-user (Preferences), with no way for an administrator to set an organization-wide default (e.g. an internal tile server) that applies to every user out of the box.
Design
config.DEFAULT_GEOMETRY_VIEWER_PROVIDERis a plain dict, and the fivecustom_tile_*preference defaults (url,name,crs,attribution,max_zoom) are sourced from it instead of hardcoded literals.Preference.get()only reads a per-user DB row if the user has explicitly saved one, otherwise it falls back to this default — so this is override semantics: a user's own saved preference always wins, there is only ever one active provider (no second/competing entry), and no naming-conflict surface is introduced.An administrator sets it in
config_local.py/config_distro.py(orPGADMIN_CONFIG_DEFAULT_GEOMETRY_VIEWER_PROVIDERas a Python-dict-literal string, same mechanism already used forDEFAULT_BINARY_PATHS).Safety
config_local.py/config_distro.py/PGADMIN_CONFIG_*replace this config variable wholesale, not merged key-by-key. A naiveDEFAULT_GEOMETRY_VIEWER_PROVIDER['name']wouldKeyError-crash preference registration (and thus app startup) on any partial admin override, orAttributeErrorif the admin sets the wrong type entirely (e.g. a string instead of a dict).resolve_geometry_viewer_provider_defaults()guards against this: falls back per-field on missing/wrong-type values, validatescrsagainst the 3 supported choices, and validatesmax_zoomis a non-bool int in[0, 25]— degrading to the original hardcoded defaults instead of crashing.Security
No new attacker-reachable surface:
DEFAULT_GEOMETRY_VIEWER_PROVIDERis filesystem/deployment-level admin config, same trust boundary asDEFAULT_BINARY_PATHS/OAUTH2_CONFIG. The existing DOMPurify sanitization onname/attributionand thehttp(s)://+{x}/{y}/{z}URL validation inGeometryViewerUtils.js(from #10142) apply uniformly to config-sourced and per-user values alike — same code path, no bypass. Per-user preference isolation (user_id-keyed) is untouched.Testing
test_geometry_viewer_provider_defaults.py— 9 scenarios (fully valid, partial override, non-dict, invalid CRS, out-of-range/non-int/bool max_zoom, non-string fields, empty dict), run viaregression/runtests.py --pkg tools.sqleditor— all pass.pycodestyle --config=.pycodestyleclean (the exactmake check-pep8invocation).make docsequivalent) — 0 new warnings, newpreferences.rstparagraph renders correctly,config_pyref resolves.Known minor nit (not fixed here, low risk)
The 3 supported CRS values are listed twice — once in
GEOMETRY_VIEWER_CRS_CHOICES(validation) and once in the preference'soptions=[...]list (UI labels, from #10142). If a 4th CRS is ever added, both need updating in sync. Pre-existing pattern, not introduced by this PR; flagging for awareness.Summary by CodeRabbit
New Features
Documentation
Tests