Skip to content

feat: add knowledge_ids field to ApplicationVersion and populate from ResourceMapping - #6532

Closed
shaohuzhang1 wants to merge 2 commits into
v2from
pr@v2@feat_add_knowledge_ids_field
Closed

feat: add knowledge_ids field to ApplicationVersion and populate from ResourceMapping#6532
shaohuzhang1 wants to merge 2 commits into
v2from
pr@v2@feat_add_knowledge_ids_field

Conversation

@shaohuzhang1

Copy link
Copy Markdown
Contributor

feat: add knowledge_ids field to ApplicationVersion and populate from ResourceMapping

Copilot AI review requested due to automatic review settings July 29, 2026 03:13
@shaohuzhang1

Copy link
Copy Markdown
Contributor Author

Seems you are using me but didn't get OPENAI_API_KEY seted in Variables/Secrets for this repo. you could follow readme for more information

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Pull request overview

Adds a persisted knowledge_ids snapshot to ApplicationVersion so simple-application chat startup can read dataset bindings from the published version (instead of querying ResourceMapping on every request), with a migration to backfill existing data.

Changes:

  • Add knowledge_ids JSONField to ApplicationVersion.
  • Backfill knowledge_ids for existing SIMPLE applications from ResourceMapping via a data migration.
  • Populate knowledge_ids during SIMPLE application publish; use ApplicationVersion.knowledge_ids for non-debug simple chat open/reopen paths.

Reviewed changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 3 comments.

File Description
apps/chat/serializers/chat.py Switches simple chat open/reopen to use ApplicationVersion.knowledge_ids when not in debug mode.
apps/application/serializers/application.py Stores knowledge_ids into the newly created ApplicationVersion for SIMPLE app publishes; minor formatting changes.
apps/application/models/application.py Adds the knowledge_ids field to the ApplicationVersion model.
apps/application/migrations/0014_applicationversion_knowledge_ids.py Adds the DB column and backfills knowledge_ids for SIMPLE apps from ResourceMapping.
Comments suppressed due to low confidence (1)

apps/application/serializers/application.py:1853

  • This field definition exceeds the repository's configured Python max line length (120) (see .editorconfig). Please wrap it to match the codebase style.
    file_clean_time = serializers.IntegerField(required=True, min_value=1, max_value=100000, label=_("File clean time"))

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment on lines +598 to +600
application_version = QuerySet(ApplicationVersion).filter(application_id=application_id).order_by(
'-create_time')[0:1].first()
knowledge_id_list = application_version.knowledge_ids
Comment thread apps/chat/serializers/chat.py Outdated
Comment on lines 1793 to 1795
id_list = list(
QuerySet(Application)
.filter(id__in=id_list, workspace_id=workspace_id)
.values_list("id", flat=True)
QuerySet(Application).filter(id__in=id_list, workspace_id=workspace_id).values_list("id", flat=True)
)
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
Copilot AI review requested due to automatic review settings July 29, 2026 03:27
@shaohuzhang1

Copy link
Copy Markdown
Contributor Author

Seems you are using me but didn't get OPENAI_API_KEY seted in Variables/Secrets for this repo. you could follow readme for more information

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 4 out of 4 changed files in this pull request and generated no new comments.

Comments suppressed due to low confidence (6)

apps/chat/serializers/chat.py:507

  • application_version is re-queried here and then dereferenced without a null check. Even though the caller checks for a published version, this extra query introduces a small race window (or alternate call path) that can raise an AttributeError when application_version is None. Add a guard and normalize knowledge_ids to a list.
            application_version = QuerySet(ApplicationVersion).filter(application_id=application.id).order_by(
                '-create_time')[0:1].first()
            knowledge_id_list = application_version.knowledge_ids

apps/chat/serializers/chat.py:600

  • Same issue as above: application_version is queried again and then used without checking for None. If the version is deleted between the earlier publication check and this query, this will crash. Add a guard and normalize knowledge_ids to a list.
            application_version = QuerySet(ApplicationVersion).filter(application_id=application_id).order_by(
                '-create_time')[0:1].first()
            knowledge_id_list = application_version.knowledge_ids

apps/application/serializers/application.py:1795

  • This refactor collapses a multi-line queryset chain into a long single line, which is inconsistent with the surrounding style in this file (e.g., application.py:302-315 wraps long calls) and makes it harder to read/review. Consider restoring the multi-line formatting.
        id_list = list(
            QuerySet(Application).filter(id__in=id_list, workspace_id=workspace_id).values_list("id", flat=True)
        )

apps/application/serializers/application.py:1280

  • This persists knowledge_ids directly from ResourceMapping without de-duplication. Since resource_mapping has no uniqueness constraint, duplicates can be stored and later expand __in queries / downstream lists unnecessarily. Consider de-duping while preserving order (migration already de-dupes via dict.fromkeys).
        if application.type == ApplicationTypeChoices.SIMPLE:
            work_flow_version.knowledge_ids = [
                str(row.target_id)
                for row in QuerySet(ResourceMapping).filter(
                    source_id=str(application.id), source_type="APPLICATION", target_type="KNOWLEDGE"
                )
            ]

apps/application/migrations/0014_applicationversion_knowledge_ids.py:44

  • This data migration materializes all matching ApplicationVersion rows into an in-memory updates list before a single bulk_update, and iterates over all versions even though only SIMPLE applications are relevant. On large datasets this can consume a lot of memory/time during migrations. Filter to the relevant versions and flush bulk_update in batches.
    updates = []
    for obj in ApplicationVersion.objects.using(db_alias).iterator():
        app_id = str(obj.application_id)
        if app_id not in simple_application_ids:
            continue
        knowledge_ids = mapping.get(app_id)
        if knowledge_ids:
            obj.knowledge_ids = knowledge_ids
            updates.append(obj)
    if updates:
        ApplicationVersion.objects.using(db_alias).bulk_update(
            updates, ["knowledge_ids"], batch_size=500
        )

apps/application/serializers/application.py:1853

  • This line exceeds typical line-length/readability patterns used elsewhere in this file (e.g., multi-line serializer field declarations at application.py:302-304). Keeping it multi-line avoids overly long lines and matches the existing style.
    file_clean_time = serializers.IntegerField(required=True, min_value=1, max_value=100000, label=_("File clean time"))

@liuruibin liuruibin closed this Jul 29, 2026
@liuruibin
liuruibin deleted the pr@v2@feat_add_knowledge_ids_field branch July 29, 2026 03:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants