Closes: #31 - Polymorphic object fields

[bctiemann]

Closes: #31

This PR addresses a major feature addition: polymorphic object/multi-object fields (using a GenericForeignKey pattern). This means that the related object type for these fields can be any of a selected list, rather than just a single related object type.

Editing a custom object with such a field shows an individual field in the edit form for each object type supported by the object/multiobject field, allowing the selection lists to work with existing object-type-based selection lists:

Unit tests have been added covering all UI and API routes, including validating that only the selected related object types can be assigned to the field.

NOTE:

        # KNOWN LIMITATION: polymorphic fields (is_polymorphic=True) store their
        # allowed target types on the related_object_types M2M, not on the
        # related_object_type FK.  This DFS therefore does not traverse edges
        # introduced by polymorphic fields.  A cycle that passes entirely through
        # polymorphic legs (e.g. A →(poly) B →(poly) A) will go undetected.
        #
        # Fixing this requires also iterating field.related_object_types.filter(
        # app_label=APP_LABEL) and recursing into each.  The check_polymorphic_recursion
        # signal already guards the direct A→B assignment, but cannot see multi-hop
        # cycles that depend on polymorphic fields already on intermediate types.

[bctiemann]

@claude /review

[bctiemann]

@claude /review

[bctiemann]

@claude /review

[bctiemann]

@claude /review

[bctiemann]

@claude /review

[bctiemann]

@claude /review

[bctiemann]

@claude /review

[bctiemann]

@claude /review

[claude]

Claude finished @bctiemann's task in 2m 51s —— View job

---

Code Review: Polymorphic Object Fields (PR #442)

• Gather context and read changed files
• Review models.py changes
• Review field_types.py changes
• Review serializers.py changes
• Review views.py changes
• Review forms.py changes
• Review tests
• Compile final review

---

Overview

This is a substantial, well-architected feature addition. The PR introduces polymorphic GFK (GenericForeignKey) and polymorphic M2M through-table fields to the plugin's custom object system. The code quality is high, with thorough inline documentation explaining non-obvious design decisions. Below are specific findings.

---

Security

GitHub Advanced Security Alert — serializers.py exception exposure

The GHAS alert flagged PolymorphicObjectSerializerField.to_internal_value(). The current code catches known exceptions (DoesNotExist, ValueError, TypeError, OverflowError), but an unexpected DB-level exception (e.g., OperationalError, ProgrammingError) would propagate unhandled and could expose a stack trace to the caller via DRF's error response. This is a low-to-medium severity issue in production (DRF in production mode should not expose raw tracebacks), but it's best practice to handle it:

# serializers.py:106-109
try:
    return model_class.objects.get(pk=obj_id)
except (model_class.DoesNotExist, ValueError, TypeError, OverflowError):
    raise serializers.ValidationError("No matching object found.") from None
# Consider also catching Exception as a fallback:
except Exception:
    raise serializers.ValidationError("Unable to retrieve the specified object.") from None

Fix this →

---

Bugs

1. PolymorphicResultList cache is per-call, not per-instance

PolymorphicM2MDescriptor.__get__ creates a new PolymorphicManyToManyManager on every attribute access. That manager's .all() creates a new PolymorphicResultList with an empty _cache. So calling obj.poly_field.all() twice still issues two rounds of DB queries — the caching in PolymorphicResultList only helps within a single all() call's lifetime. The docstring says "subsequent accesses of the same instance use a cached list" but this won't work as described.

This is a minor performance issue, not a correctness bug, but the misleading docstring should be updated. [field_types.py]

2. CustomObjectEditView polymorphic single-object: "first non-empty wins" is silent (views.py:732-742)

When a user fills in multiple type sub-fields for the same GFK (which custom_clean detects and adds form errors for), the form correctly blocks submission. However, the custom_save method still uses "first non-empty wins" without re-checking — if custom_clean is somehow bypassed, data silently drops. This is adequately protected by the form validation layer and is low risk.

---

Design Concerns

3. Known limitation: multi-hop polymorphic cycles are undetected (documented in PR description)

The check_polymorphic_recursion m2m_changed handler only checks direct cycles (A → poly → B → A). A cycle through multiple polymorphic hops (A →poly→ B →poly→ A) is undetected. The known limitation comment in models.py accurately describes the fix needed. This is a reasonable scope deferral but should be tracked as a follow-up issue.

4. get_serializer_class() is not cached (serializers.py:395)

The comment explains this is intentional ("keeping serializer generation fresh"). However, this means every API request re-evaluates two .filter() queries on CustomObjectTypeField and calls type() to create a new serializer class. For read-heavy APIs with many COTs this could cause measurable overhead. Consider caching the result keyed by (model.__name__, cache_timestamp) to match the existing model cache pattern.

5. PolymorphicManyToManyManager._get_objects() N+1 by content type

The method issues one SELECT per distinct content type group. This is better than one-per-row, but with many different types it degrades. For templates that render a list, this hits the DB per content type on every page render. Consider documenting a prefetch_polymorphic() utility as a future improvement.

---

Code Quality — Positives

• suppress_clear_cache mechanism (utilities.py): The thread-local depth counter is an excellent, thread-safe solution to the re-entrant clear_cache problem. The detailed inline comment explaining the recursion hazard is exemplary.
• extract_cot_id_from_model_name (utilities.py): The regex-based replacement is a solid improvement over the fragile .replace("table", "").replace("model", "") pattern that corrupts names containing those substrings.
• _safe_index_name (field_types.py): Good handling of PostgreSQL's 63-byte identifier limit.
• CustomManyToManyManager.set() bug fix: The indentation bug where self.add(*objs) always ran (even when clear=False) is correctly fixed.
• alter_db_table argument fix (models.py): The schema_editor.alter_db_table call previously passed old_through_model as the first argument; it now correctly passes new_through_model (which Django uses to resolve associated sequences). This is a subtle correctness fix.
• Immutability guards: Preventing is_polymorphic flag flipping and field renames post-creation at both model clean() and serializer validate() levels is thorough.
• Through-table drop before main table delete (models.py): The FK ordering for schema cleanup is correct.
• Test coverage: 1,055 lines covering API create/read/update/delete, form submission, recursion detection, and invalid-type rejection. Very solid.

---

Minor Issues

6. _parse_poly_sub_name has no error handling (views.py:50-53)

If a form submission contains a sub-field name that doesn't match the expected {field_name}__{app_label}__{model} format, split("__", 1) will not produce two parts and will raise ValueError. This path is only reached from custom_init (loading initial values for known sub-names) so it's unlikely to be triggered by user input, but defensive error handling here would be more robust.

7. related_object_types validation gap in clean() (models.py:1235-1264)

For polymorphic fields, the clean() method validates that related_object_type is null but cannot validate that related_object_types is non-empty (it's M2M, set after save). The form's clean() method does validate this, but direct model saves (e.g., via management commands or tests) can bypass this check. The check_polymorphic_recursion m2m_changed signal handler provides partial protection, but an empty related_object_types set on a polymorphic field is a valid DB state that won't be caught at the model level.

8. _TABLE_MODEL_PATTERN not shown but used in serializer (serializers.py)

The _TABLE_MODEL_PATTERN regex and _PUBLIC_APP_LABEL constant are referenced in _resolve_object_type() but not visible in the diff — they exist in the original file. Worth confirming these are still accurate for the new polymorphic model name format if custom object type IDs appear in unexpected positions.

---

Summary

This is a mature, production-quality implementation of a genuinely complex feature. The most actionable items are:

1. Security: Broad exception catch in PolymorphicObjectSerializerField.to_internal_value() to prevent stack trace exposure (GHAS alert).
2. Correctness: Fix/update the misleading docstring on PolymorphicResultList about per-instance caching.
3. Design: Track the multi-hop polymorphic cycle detection as a follow-up issue.

The rest are minor polish items. The PR is well-positioned to merge after addressing the security finding.

[bctiemann]

1. Not fixing this as GHAS has flagged this same chunk of code in different ways about 10 times, the last two being literally to go back and forth on its recommendation to include a catch-all Except or not.
2. Updated docstring
3. OK

[arthanson]

The caching stuff and extract_cot_id_from_model_name can be separated out into a separate PR as they are functionally separate and would make this very large PR easier to review / more targeted.

suppress_clear_cache is only used in utilities.py and can be a private method. install_clear_cache_suppressor is only Called in init so that code can be separated out into another PR (the stuff in utilities and init).

[bctiemann]

• Moved cot_field_id resolution and cache clearing out into an (already-merged) other PR
• Renumbered migrations
• Added missing internationalization
• Fixed several other gaps identified via local review post merging feature into this branch

[arthanson]

feature now has the changes for the tests to pass but has merge conflicts.

[arthanson]

This is returning double form-fields for items (see two form items for Circuits, two for Region, etc..):

Also, I think a better UI for this would be like we do for scope (see below) where we have a dropdown for the type and dynamically switch the field under it. Also in the list view probably want to show the Poly Type next to Poly so you know what it actually is.

Claude also came up with a couple points:

1. through_table_name can exceed PostgreSQL's 63-char identifier limit

IMPORTANT — data integrity risk on large deployments.

through_table_name is computed as:
f"custom_objects_{self.custom_object_type_id}_{self.name}"

The name field allows up to 50 characters (max_length=50). A realistic worst-case (e.g. COT id = 99999, name = 50 chars): 15 + 5 + 1 + 50 = 71 chars — 8 over PostgreSQL's hard 63-byte
limit. PostgreSQL silently truncates identifiers, which can cause silent name collisions between tables sharing a long prefix.

_safe_index_name() already exists in field_types.py for this purpose. An analogous _safe_table_name() should be applied to through_table_name (and correspondingly to
through_model_name).

Note: This doesn't require a data migration for any existing installations that haven't hit the limit yet, but should be addressed before GA.

---

2. Orphaned field row if check_polymorphic_recursion signal raises

The save path for a new polymorphic field via API:

1. CustomObjectTypeField.save() — creates DB columns/through table (committed, or at least schema-altered)
2. DRF create() → instance.related_object_types.set(resolved) — fires m2m_changed
3. check_polymorphic_recursion (pre_add) raises ValidationError → DRF surfaces 400

At step 3, the field row + DB schema already exist, but related_object_types is never set. The schema is in a partially-initialized state: the GFK columns (or through table) exist, but
the field instance has no allowed types.

The serializer's validate() catches some problems early, but it does NOT check for circular references (it can't — it runs before any DB writes). The signal is the only guard.

Mitigation options:

• Wrap the save() + set() in the DRF create() in an atomic block, catching ValidationError from the signal and deleting the field row in the exception handler.
• Or document clearly that related_object_types.set() must succeed for the field to be usable, and add a check in get_model() that skips is_polymorphic fields with zero
  related_object_types.