[Fixes #14035] Implementation of a common ResourceManager base class for all resource types#14053
[Fixes #14035] Implementation of a common ResourceManager base class for all resource types#14053
Conversation
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly refactors the application's resource management system by introducing a registry pattern and type-specific resource managers. This change centralizes the handling of different resource types, making the codebase more modular and easier to extend with new resource types or custom behaviors. It streamlines how resources are created, updated, and managed across the platform, enhancing overall maintainability and reducing boilerplate code in various application components. Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Changelog
Activity
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
![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 is an excellent and extensive refactoring that introduces a ResourceManagerRegistry and specialized manager classes for different resource types. This greatly improves the architecture by promoting separation of concerns, making the system more extensible and testable. The logic has been consistently moved from views and serializers into the new manager classes across the codebase.
My main feedback is on the implementation of the create methods in the new MapResourceManager and GeoAppResourceManager. They seem to replicate a pre-existing but inefficient pattern of calling update immediately after create, leading to redundant database operations. I've left specific comments with suggestions for simplification.
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #14053 +/- ##
==========================================
+ Coverage 74.58% 74.65% +0.07%
==========================================
Files 951 958 +7
Lines 57245 57755 +510
Branches 7771 7857 +86
==========================================
+ Hits 42694 43115 +421
- Misses 12833 12885 +52
- Partials 1718 1755 +37 🚀 New features to boost your workflow:
|
mattiagiupponi
left a comment
mattiagiupponi
left a comment
There was a problem hiding this comment.
I see that the resouce_registr has different method but few of them accept the same kind of input which makes confusing which one is responsible of what.
I think is better to divide a bit the methods to have something like:
get_for_instance
get_for_model
get_for_type
in this way it makes obvious which method to use and how.
I would be great also to have some type hint
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request refactors the resource management system by introducing a ResourceManagerRegistry and specialized managers for Datasets, Documents, Maps, and GeoApps. The previous global resource_manager is replaced with type-specific managers to centralize logic and improve extensibility. Feedback highlights a logic error in BaseResourceManager where default values were incorrectly assigned to the user object, redundant assignments in the GeoApp manager, and concerns regarding the use of QuerySet.update() bypassing model signals. Additionally, it is recommended to refine exception handling in the registry's lazy loader to avoid masking configuration issues.
| for field in instance.ROLE_BASED_MANAGED_FIELDS: | ||
| if not user.can_change_resource_field(instance, field): | ||
| logger.debug("User can perform the action, the default value is set") | ||
| setattr(user, field, getattr(ResourceBase, field).field.default) |
There was a problem hiding this comment.
There is a logic error here. The code is attempting to set the default value of a field on the user object instead of the instance (the resource being managed). This will likely cause a crash or unexpected behavior since user objects typically do not have these resource-specific fields.
| setattr(user, field, getattr(ResourceBase, field).field.default) | |
| setattr(instance, field, getattr(ResourceBase, field).field.default) |
4 participants
Fixes #14035
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.