Skip to content

fix: portal setting #7796

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
wxg0103 merged 1 commit into from
Nov 10, 2026
Merged

fix: portal setting #7796

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
22 changes: 13 additions & 9 deletions apps/portal/serializers/portal.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -146,18 +146,22 @@ def edit(self, instance, with_valid=True):
portal = Portal.objects.first()
if portal is None:
raise AppApiException(500, _("Portal configuration does not exist"))
file_fields = ["logo", "id", "create_time", "update_time"]
license_valid = self._is_license_valid()
file_fields = {"logo", "id", "create_time", "update_time"}
for field, value in instance.items():
if hasattr(portal, field) and field not in file_fields:
if field == "auth_config" and isinstance(value, dict):
value = {**(portal.auth_config or {}), **value}
setattr(portal, field, value)
if not hasattr(portal, field) or field in file_fields:
continue
# auth_config 仅在授权有效时允许保存
if field == "auth_config":
if not license_valid or not isinstance(value, dict):
continue
value = {**(portal.auth_config or {}), **value}
setattr(portal, field, value)
# Logo 为授权功能，授权无效时忽略 Logo 上传/修改
for field_name in ["logo"]:
if field_name in instance and self._is_license_valid():
self._handle_file_field(portal, field_name, instance.get(field_name))
if "logo" in instance and license_valid:
self._handle_file_field(portal, "logo", instance.get("logo"))
portal.save()
return PortalSerializer.Model(portal).data
return self.one()


class PortalLoginSerializer(serializers.Serializer):

@shaohuzhang1 shaohuzhang1 Nov 10, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here is the code review focusing on irregularities, potential issues, and optimization suggestions:

1. Potential Logic Issue: setattr for File Fields in auth_config Merge

Issue: The merging logic for auth_config uses {**(portal.auth_config or {}), **value}. If portal.auth_config contains keys that are file paths or objects (e.g., if auth_config stores file references), a shallow merge dictionary update might not deep-merge nested structures correctly. However, this is minor compared to the next point.

2. Irregularity: Unused/Redundant Logic & Readability

Suggestion:
In the loop:

if field == "auth_config":
    if not license_valid or not isinstance(value, dict):
        continue
    value = {**(portal.auth_config or {}), **value}
setattr(portal, field, value)

If field == "auth_config" but license_valid is False or value is not a dict, the continue skips the setattr. This is correct.
However, if field != "auth_config", it executes setattr(portal, field, value) immediately after the if block.
This structure is slightly hard to follow because setattr is conditional on not entering the auth_config block OR successfully passing the checks inside auth_config.
A clearer structure:

for field, value in instance.items():
    if not hasattr(portal, field) or field in file_fields:
        continue
    
    if field == "auth_config":
        if not license_valid or not isinstance(value, dict):
            continue
        value = {**(portal.auth_config or {}), **value}
    
    setattr(portal, field, value)

Verdict: Your current logic is functionally equivalent but slightly less readable than the above. It works, but separating the setattr makes the flow of control clearer.

3. Optimization: Set Initialization

Suggestion:
Change file_fields = ["logo", "id", "create_time", "update_time"] (which was changed to a set) is good.
Ensure file_fields is indeed a set. If it’s a tuple/list, membership testing is O(n). A set is O(1).
In the diff: file_fields = {"logo", "id", "create_time", "update_time"} is a set literal. Good optimization.

4. Potential Bug: instance.items() and File Uploads

Critical Check:
How are file uploads handled in instance?
Typically, Django ModelForms or Serializers handle files via request.FILES.
If instance is a dictionary representing request.DATA or a JSON payload, instance.get("logo") will likely return a string path, URL, or None, not a file object.
Your _handle_file_field(portal, field_name, instance.get(field_name)) expects a file-like object.
If instance is from a JSON API, you won't be able to upload a new logo unless your custom serializer/view correctly extracts the file from the multipart request and puts it into instance as an UploadedFile object before calling edit.
Recommendation: Verify that self._handle_file_field receives a valid file object. If this endpoint is JSON-only, logo updates will fail silently or raise an error when trying to save a string to the file field.

5. Optimization: Early Return / Guard Clauses

The current if portal is None: raise... is fine.

6. Consistency: Return Value

You replaced return PortalSerializer.Model(portal).data with return self.one().

  • Pro: Reusing self.one() ensures consistency with other methods (like retrieve) to avoid duplicate serialization logic.
  • Con: Ensure self.one() fetches the fresh instance from the DB (it should, since it's called after portal.save()). If self.one() calls a separate query, it is safe. If it relies on cached state, it might be stale. Usually, .one() in such contexts re-fetches. Likely Good.

7. Security/Maintenance: License Check Caching

self._is_license_valid() is called once and stored in license_valid. Good practice. Avoid redundant license logic calls.

Refactored Suggestion (Improving Readability)

def edit(self, instance, with_valid=True):
    portal = Portal.objects.first()
    if portal is None:
        raise AppApiException(500, _("Portal configuration does not exist"))
    
    # Cache license validation result
    license_valid = self._is_license_valid()
    
    # Fields to ignore during standard attribute assignment
    ignored_fields = {"logo", "id", "create_time", "update_time"}
    
    for field, value in instance.items():
        # Skip unknown fields or special file/ID/time fields
        if not hasattr(portal, field) or field in ignored_fields:
            continue
            
        # Special handling for auth_config
        if field == "auth_config":
            # Only allow updating auth_config if license is valid and input is a dict
            if not license_valid or not isinstance(value, dict):
                continue
            # Deep merge strategy: existing config updated by new data
            value = {**(portal.auth_config or {}), **value}
            
        setattr(portal, field, value)

    # Handle Logo upload separately (FileField logic)
    if "logo" in instance:
        if license_valid:
            # Ensure _handle_file_field receives a proper file object
            file_obj = instance.get("logo")
            self._handle_file_field(portal, "logo", file_obj)
        # else: Ignore logo update if license invalid
        # Optional: Log that logo change was ignored due to invalid license

    portal.save()
    return self.one()

Summary of Findings

  1. Logic: Mostly sound. The control flow for auth_config is correct but slightly dense; refactoring improves readability.
  2. Performance: Using a set for ignored_fields is a good optimization.
  3. Risk: Crucial: Verify how file uploads reach instance["logo"]. If this is a JSON API, instance.get("logo") is likely invalid for _handle_file_field. Ensure your middleware/serializer maps multipart form data to the dictionary correctly.
  4. Consistency: Returning self.one() is better for DRY principles than manual serialization.

Expand Down
Loading