Skip to content

fix(pairedData): return null source for deleted parent projects DEV-2034#7218

Open
platreth wants to merge 2 commits into
mainfrom
hugo/dev-2034-deleted-connected-projects-in-paired-data-source-list
Open

fix(pairedData): return null source for deleted parent projects DEV-2034#7218
platreth wants to merge 2 commits into
mainfrom
hugo/dev-2034-deleted-connected-projects-in-paired-data-source-list

Conversation

@platreth

@platreth platreth commented Jul 7, 2026

Copy link
Copy Markdown
Contributor

📣 Summary

When a source project used for a connected (paired) dataset is deleted, it is now clearly reported as deleted rather than still appearing as a normal source.

📖 Description

Connected projects pull data from a "source" project. If that source project was deleted, the paired-data endpoint kept listing it as if it were a live source, which the connected-projects UI couldn't reliably detect. The deleted source's source link is now returned as null, giving the frontend a dependable signal.

💭 Notes

paired_data is stored as a JSON blob on the destination Asset keyed by source uid, with no cascading FK, so deleting a source asset leaves a dangling reference (DEV-2034). PairedDataSerializer.to_representation now emits source: null when the source Asset row no longer exists, detected by reusing the source__names mapping the viewset already builds (uid absent ⇒ row deleted) — no extra query on the list endpoint, with a direct .exists() fallback when context is absent. Runtime-only change: no OpenAPI schema drift, no migration.

👀 Preview steps

  1. ℹ️ have an account with two projects; enable data sharing on project A (the source).
  2. Connect project B to A (paired data) and GET /api/v2/assets/<B_uid>/paired-data/.
  3. Delete project A.
  4. GET /api/v2/assets/<B_uid>/paired-data/ again.
  5. 🔴 [on main] the deleted source still appears with a populated "source" URL (only "source__name" is null).
  6. 🟢 [on PR] that entry's "source" is null.

@platreth platreth requested review from jnm and noliveleger as code owners July 7, 2026 08:55
@greptile-apps

greptile-apps Bot commented Jul 7, 2026

Copy link
Copy Markdown
Contributor

Greptile Summary

This PR fixes a bug where deleting a source (parent) project left its paired-data entry in a zombie state — the list endpoint kept returning a populated source URL instead of null, making the frontend unable to reliably detect the deletion. The fix adds a _source_is_deleted helper that reuses the source__names dict the viewset already builds (uid absent → row deleted), and guards both get_source__name and __get_source_asset_url so they emit null when the source is gone.

  • _source_is_deleted is decorated with @cache_for_request so the two callers in to_representation share a single lookup; a direct .exists() fallback handles the rare case when context is absent.
  • The added test (test_list_with_deleted_source) covers the full list-endpoint flow: create pairing → delete source → assert both source and source__name are null.
  • A pre-existing NameError lurks in the except KeyError fallback of get_source__name (line 64: source.name with source undefined) — the viewset always populates source__names so the path is practically unreachable today, but it should still be cleaned up.

Confidence Score: 4/5

Safe to merge for the paired-data list endpoint; a latent NameError in the context-fallback branch of get_source__name would affect callers that instantiate the serializer without the viewset context.

The viewset always injects source__names into context, so the broken else: source__name = source.name branch (undefined source) is unreachable through normal usage. However, it is a real defect: any test or integration that bypasses the viewset context would hit a NameError rather than returning the asset name.

The get_source__name fallback path in kpi/serializers/v2/paired_data.py (lines 62-64) contains the source.name reference that needs cleanup.

Sequence Diagram

%%{init: {'theme': 'neutral'}}%%
sequenceDiagram
    participant Client
    participant Viewset as PairedDataViewset
    participant Serializer as PairedDataSerializer
    participant DB as Database

    Client->>Viewset: GET paired-data list
    Viewset->>DB: Query uid+name for all source_uids
    DB-->>Viewset: Rows for existing assets only
    Viewset->>Serializer: serialize(context with source__names dict)

    loop For each PairedData entry
        Serializer->>Serializer: _source_is_deleted(instance)
        Note over Serializer: uid NOT in source__names means deleted
        alt Source still exists
            Serializer-->>Client: source URL + source__name populated
        else Source was deleted
            Serializer-->>Client: source null + source__name null
        end
    end

    Note over Serializer: cache_for_request on _source_is_deleted shared between callers
Loading
%%{init: {'theme': 'base', 'themeVariables': {"darkMode": true, "background": "#0d1117", "primaryColor": "#21262d", "primaryTextColor": "#e6edf3", "primaryBorderColor": "#8b949e", "lineColor": "#8b949e", "textColor": "#e6edf3", "edgeLabelBackground": "#161b22", "actorBkg": "#21262d", "actorBorder": "#8b949e", "actorTextColor": "#e6edf3", "actorLineColor": "#8b949e", "signalColor": "#8b949e", "signalTextColor": "#e6edf3", "noteBkgColor": "#373320", "noteBorderColor": "#d4a72c", "noteTextColor": "#f0e6c0", "labelBoxBkgColor": "#21262d", "labelBoxBorderColor": "#8b949e", "labelTextColor": "#e6edf3", "loopTextColor": "#e6edf3", "activationBkgColor": "#30363d", "activationBorderColor": "#8b949e"}}}%%
sequenceDiagram
    participant Client
    participant Viewset as PairedDataViewset
    participant Serializer as PairedDataSerializer
    participant DB as Database

    Client->>Viewset: GET paired-data list
    Viewset->>DB: Query uid+name for all source_uids
    DB-->>Viewset: Rows for existing assets only
    Viewset->>Serializer: serialize(context with source__names dict)

    loop For each PairedData entry
        Serializer->>Serializer: _source_is_deleted(instance)
        Note over Serializer: uid NOT in source__names means deleted
        alt Source still exists
            Serializer-->>Client: source URL + source__name populated
        else Source was deleted
            Serializer-->>Client: source null + source__name null
        end
    end

    Note over Serializer: cache_for_request on _source_is_deleted shared between callers
Loading

Comments Outside Diff (1)

  1. kpi/serializers/v2/paired_data.py, line 62-64 (link)

    P1 Undefined source variable in fallback path

    In the except KeyError fallback block, Asset.objects.values_list('name', flat=True).get(...) already assigns the name string directly to source__name. The else clause then overwrites it with source.name, but source is never defined in this scope — this would raise NameError. The else block was likely a remnant from before the query was changed to use values_list. The else branch (lines 63-64) can simply be removed; source__name is already correctly set by the try block.

Reviews (2): Last reviewed commit: "refactor(pairedData): push deleted-sourc..." | Re-trigger Greptile

@platreth platreth self-assigned this Jul 8, 2026
@noliveleger noliveleger removed the request for review from jnm July 9, 2026 13:43
Comment thread kpi/serializers/v2/paired_data.py Outdated
return source__name

def to_representation(self, instance):
source_deleted = self._source_is_deleted(instance)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Let's move this line inside __get_source_asset_url() and get_source__name() and return None (change their signature accordingly).

You can decorate _source_is_deleted with @cache_for_request to avoid calling the DB twice.

in both methods, if self._source_is_deleted() returns None, we return right away, in the representation, no need to add conditions anymore. It would be handled inside other methods.

On the side note. That's not great. I think we should move away from the JSON Field and give PairedData its own db model and use FKs. Would you mind to create a linear issue for that?

@platreth platreth requested a review from noliveleger July 10, 2026 07:44

@noliveleger noliveleger left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants