Skip to content

fix: Application deleted, workflow page error reported #2743

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
merged 1 commit into from
Mar 31, 2025
Merged

fix: Application deleted, workflow page error reported #2743

Show file tree
Hide file tree
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
5 changes: 3 additions & 2 deletions apps/application/serializers/application_serializers.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1209,7 +1209,9 @@ def get_application(self, app_id, with_valid=True):
self.is_valid(raise_exception=True)
if with_valid:
self.is_valid()
embed_application = QuerySet(Application).get(id=app_id)
embed_application = QuerySet(Application).filter(id=app_id).first()
if embed_application is None:
raise AppApiException(500, _('Application does not exist'))
if embed_application.type == ApplicationTypeChoices.WORK_FLOW:
work_flow_version = QuerySet(WorkFlowVersion).filter(application_id=embed_application.id).order_by(
'-create_time')[0:1].first()
Expand Down Expand Up @@ -1332,4 +1334,3 @@ async def get_mcp_tools(servers):
}
for tool in asyncio.run(get_mcp_tools({server: servers[server]}))]
return tools

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There are a few issues and improvements that can be made to the code:

  1. Use of QuerySet.first(): The use of filter().first() is generally preferred over get() because it won't raise an exception if no record is found; instead, it returns None. This makes the function more robust.

  2. Explicitly Handle Not Found Case: Instead of catching exceptions, you should explicitly handle the case where embed_application is None, which could happen if no application exists with the given ID.

  3. Optimization on Server List: In the get_mcp_tools asynchronous generator function, there is unnecessary double iteration on the server dictionary when using asyncio.wait(). You might need to rethink how this part handles the server list.

Here's the improved code:

def get_application(self, app_id, with_valid=True):
    if self.is_valid(raise_exception=True) and with_valid:
        embed_application = QuerySet(Application).filter(id=app_id).first()

        if embed_application is None:
            raise AppApiException(500, _('Application does not exist'))

        if embed_application.type == ApplicationTypeChoices.WORK_FLOW:
            work_flow_version = (
                QuerySet(WorkFlowVersion)
                .filter(application_id=embed_application.id)
                .order_by('-create_time')
                .first()
            )
            # Further processing...
        
# Other parts remain unchanged.

For the get_mcp_tools function, consider refactoring to avoid double iteration:

async def get_mcp_tools(servers: Dict[str, dict]) -> Generator[MCPToolResponse, Literal[''], str]:
    for server in servers.keys():
        await asyncio.sleep(0)  # Yield control between iterations

        response = {}
        try:
            mcp_server_response = await run_in_threadpool(cast(MCPRunResponse, call_mcpe_api(method='info', data={})))

            if 'players_count' in mcp_server_response:
                response['current_players'] = mcp_server_response.get('players_count')

            if response:
                yield MCPToolResponse(tool_type="SERVER_INFO", tool_name=f"info_{server}", details=response)

        except Exception as e:
            Logger.error(f'MPC Tool Info Error from `{server}` : {e}')

In the revised version, I've added a try-except block within each server loop, which helps in handling any errors gracefully without crashing the entire generator. Additionally, I used cast() to type-check mcp_server_response, although based on typical usage, it is already castable to MCPRunResponse.

9 changes: 4 additions & 5 deletions ui/src/workflow/nodes/application-node/index.vue
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -146,10 +146,9 @@
<template #label>
<div class="flex align-center">
<div class="mr-4">
<span
>{{ $t('views.applicationWorkflow.nodes.aiChatNode.returnContent.label')
}}</span
>
<span>{{
$t('views.applicationWorkflow.nodes.aiChatNode.returnContent.label')
}}</span>
</div>
<el-tooltip effect="dark" placement="right" popper-class="max-w-200">
<template #content>
Expand Down Expand Up @@ -288,7 +287,7 @@ const update_field = () => {
}
})
.catch((err) => {
// set(props.nodeModel.properties, 'status', 500)
set(props.nodeModel.properties, 'status', 500)
})
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The provided code snippet seems to be JavaScript with React/Vue syntax (since it includes this.$t, set). The changes look generally correct, but here are a few things that could be improved:

  1. Simplify the Line: In both occurrences of {{ $t('views.applicationWorkflow.nodes.aiChatNode.returnContent.label') }}, you can replace single-line HTML comments with template literals or remove them completely if they're just placeholders.

    <div class="mr-4">Return content label</div> // Remove these lines if not needed
<!-- <span>{{ $t('views.applicationWorkflow.nodes.aiChatNode.returnContent.label') }}</span> -->

  2. Avoid Redundancy: There's a redundant comment on the same line where you set an error status, though this might be intentional depending on the context.

  3. Code Structure: Ensure that all functions and logic adhere to best practices. This particular piece looks fine for updating properties and handling errors, but general code structure is recommended for clarity.

const update_field = () => {
  axios.post(`/api/nodes/${props.id}/update-field`, { ... }).then(
    (res) => {}
  ).catch((err) => {
    set(props.nodeModel.properties, 'status', 500);
  });
};

This should cover most minor improvements and ensure readability.

Expand Down