[Fixes #14309] Generalize remote service type registration and auth handling#14328
[Fixes #14309] Generalize remote service type registration and auth handling#14328sijandh35 wants to merge 7 commits into
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request refactors service type handling by introducing a centralized ServiceTypeRegistry and enhances remote resource upload handlers (including COG, FlatGeobuf, 3D Tiles, and WMS) to support authentication configurations (AuthConfig) during validation and import. It also integrates authentication credentials with GDAL configuration options for remote metadata extraction. The review feedback highlights several improvement opportunities: implementing lazy initialization in ServiceTypeRegistry to avoid inefficient re-initialization and loss of dynamic registrations, and adding safety guards in BaseRemoteResourceHandler to prevent potential AttributeErrors when service_url, _exec, or options are None or empty.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #14328 +/- ##
===========================================
+ Coverage 34.71% 74.90% +40.18%
===========================================
Files 982 986 +4
Lines 60496 60862 +366
Branches 8247 8318 +71
===========================================
+ Hits 21002 45586 +24584
+ Misses 38332 13418 -24914
- Partials 1162 1858 +696 🚀 New features to boost your workflow:
|
| return None | ||
|
|
||
| @staticmethod | ||
| def get_auth_config_for_import(_exec, service=None, to_update=None): |
There was a problem hiding this comment.
@sijandh35 I would simply name it get_auth_config_from_execution()
| return None | ||
|
|
||
| @staticmethod | ||
| def get_request_auth_for_import(execution_id): |
There was a problem hiding this comment.
@sijandh35 I would name it get_request_auth_from_execution()
| if auth_config_id: | ||
| return AuthConfig.objects.filter(pk=auth_config_id).first() | ||
|
|
||
| if service and service.auth_config: |
There was a problem hiding this comment.
This method requires the exec id otherwise it returns at line 186. But this conditional branch doesn't require the original exection id.
There's something strange in the signature. You ask for unrelated parameters, where the first is required, but not for the path that uses the second parameter?
There was a problem hiding this comment.
Updated. The service fallback is now handled by the caller, and this method now only resolves values coming from the execution.
3 participants
Fixes #14309
Checklist
For all pull requests:
The following are required only for core and extension modules (they are welcomed, but not required, for contrib modules):
Submitting the PR does not require you to check all items, but by the time it gets merged, they should be either satisfied or inapplicable.