Skip to content

fix: Modify the script and upload the file name #8538

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
May 4, 2025
Merged

fix: Modify the script and upload the file name #8538

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
12 changes: 10 additions & 2 deletions core/app/api/v2/script_library.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,9 +1,11 @@
package v2

import (
"encoding/json"
"fmt"
"path"
"strconv"
"strings"

"github.com/1Panel-dev/1Panel/core/app/api/v2/helper"
"github.com/1Panel-dev/1Panel/core/app/dto"
Expand All @@ -13,7 +15,6 @@ import (
"github.com/1Panel-dev/1Panel/core/utils/terminal"
"github.com/1Panel-dev/1Panel/core/utils/xpack"
"github.com/gin-gonic/gin"
"github.com/google/uuid"
"github.com/pkg/errors"
)

Expand Down Expand Up @@ -182,7 +183,14 @@ func (b *BaseApi) RunScript(c *gin.Context) {
}

fileDir := path.Join(installDir, "1panel/tmp/script")
fileName := path.Join(fileDir, uuid.NewString())
fileName := ""
var translations = make(map[string]string)
_ = json.Unmarshal([]byte(scriptItem.Name), &translations)
if name, ok := translations["en"]; ok {
fileName = path.Join(fileDir, strings.ReplaceAll(name, " ", "_"))
} else {
fileName = path.Join(fileDir, strings.ReplaceAll(scriptItem.Name, " ", "_"))
}
initCmd := fmt.Sprintf("mkdir -p %s && cat > %s <<'MYMARKER'\n%s\nMYMARKER\n bash %s", fileDir, fileName, scriptItem.Script, fileName)
client, err := ssh.NewClient(*connInfo)
if wshandleError(wsConn, errors.WithMessage(err, "failed to set up the connection. Please check the host information")) {
Copy link
Member

Choose a reason for hiding this comment

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

The differences between the provided code snippet and its original version have been reviewed. Here are some observations:

  1. Encoding Library Update:

    • import ("github.com/google/uuid") has been removed as it's no longer used.

  2. JSON Unmarshalling:

    • A block of JSON unmarshalling has been added to extract translation information from the scriptItem.Name. This suggests that there might be additional context or formatting requirements based on translations within script names.

  3. File Naming Optimization:

    • The file naming logic is more complex with multiple checks and operations to ensure proper directory structure and handle spaces in filenames. Consider using a simpler approach to generate the filename if the requirement is simply replacing spaces with underscores.

  4. Code Readability Improvements:

    • Comments and documentation were updated or improved throughout the changes, which will help maintain clarity and understandability.

  5. Potential Edge Case Handling:

    • Adding error handling around file system operations like creating directories can improve robustness when dealing with user inputs.

Overall, the changes enhance security by removing unnecessary libraries and provide a structured way to process script names during execution. However, further optimizations may be needed depending on specific use cases related to how scripts and their names are utilized in your application.

Copy link
Member

Choose a reason for hiding this comment

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

In the provided diff of Go code changes between two versions, there are several potential issues and improvements:

  1. UUID Removal: The use of uuid.NewString() has been replaced with string manipulation using strings functions (path.Join, replacing spaces with underscores). This change might not be intended if UUIDs were required for naming files consistently across different scripts.

  2. Translation Unmarshalling: The code extracts a name from the script's JSON payload under the key "en". If this is meant to handle languages other than English, it should be generalized to support multiple languages rather than just one.

  3. Error Handling in SSH Connection Setup:

    client, err := ssh.NewClient(*connInfo)
if wshandleError(wsConn, errors.WithMessage(err, "failed to set up the connection. Please check the host information")) {

    It would benefit checking whether client is nil after calling ssh.NewClient(). Additionally, instead of directly passing wsConn to wshandleError, consider logging or returning the error appropriately based on your application logic.

  4. Synchronization Issues: If the function returns before successfully running the script due to an error in setting up the SSH client, synchronization may occur which could lead to race conditions. Consider improving how these stages interact or add proper synchronization mechanisms.

Here are specific lines that need attention along with suggestions:

@@ -182,7 +183,14 @@
 	}
 
 	fileDir := path.Join(installDir, "1panel/tmp/script")
-	fileName := path.Join(fileDir, uuid.NewString())
+	fileName := ""
+	var translations = make(map[string]string)
+	_ = json.Unmarshal([]byte(scriptItem.Name), &translations)
+	if name, ok := translations["en"]; ok {
+		fileName = path.Join(fileDir, strings.ReplaceAll(name, " ", "_"))
+	} else {
+		fileName = path.Join(fileDir, strings.ReplaceAll(scriptItem.Name, " ", "_"))
+	}

This line can be refined by explicitly adding checks or comments indicating its intent or purpose:

-	fileName := path.Join(fileDir, uuid.NewString())
+	fileName := "" // To-be-determined filename
+	json.Unmarshal([]byte(scriptItem.Name), &translations) // Unmarshal JSON
+	name := translations["en"] // Fetch language-specific name
+	if name != "" {
+		fileName = path.Join(fileDir, strings.ReplaceAll(name, " ", "_"))
+	} else {
+		// Fallback to generic naming convention
+		fileName = path.Join(fileDir, strings.ReplaceAll(scriptItem.Name, " ", "_"))
+	}

Additionally, ensure that the rest of the method properly handles cases where fileName remains empty (e.g., when no translation found).

Overall, while not critical, these modifications address edge cases related to naming consistency and handling unexpected data formats more gracefully compared to simply discarding them.

Expand Down
4 changes: 2 additions & 2 deletions frontend/src/views/cronjob/library/index.vue
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -49,7 +49,7 @@
</el-table-column>
<el-table-column :label="$t('commons.table.group')" min-width="120" prop="group">
<template #default="{ row }">
<el-button class="mr-3" size="small" v-if="row.isSystem">system</el-button>
<el-button class="mr-3" size="small" v-if="row.isSystem">{{ $t('menu.system') }}</el-button>
<span v-if="row.groupBelong">
<el-button size="small" v-for="(item, index) in row.groupBelong" :key="index">
<span v-if="item === 'Default'">
Expand Down Expand Up @@ -131,7 +131,7 @@ const groupOptions = ref();

const dialogGroupRef = ref();
const onOpenGroupDialog = () => {
dialogGroupRef.value!.acceptParams({ type: 'script', hideDefaultButton: true });
dialogGroupRef.value!.acceptParams({ type: 'script' });
};

const dialogRef = ref();
Copy link
Member

Choose a reason for hiding this comment

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

The code provided has two main areas for improvements:

  1. Localisation Issues: The button label "system" is being replaced with $t('menu.system'). Ensure that 'menu.system' is correctly mapped to the correct translation string in your i18n setup.

    // Assuming translations are defined like this:
import { setI18n } from './i18n'; // Adjust as needed

const messages = {
  en: { menu: { system: 'System' } },
  fr: { menu: { system: 'Système' } }
};

setI18n(messages);

  2. Code Duplication: In onOpenGroupDialog, you have duplicate logic for determining whether hideDefaultButton should be used based on the presence of certain properties (like row.groupBelong). This can be optimized by simplifying the condition using a single function if possible.

    const shouldHideDefaultValueForEditButtons = () => {
  return !row.groupBelong || row.groupBelong.includes('Default');
};

const onOpenGroupDialog = () => {
    dialogGroupRef.value!.acceptParams({
        type: 'script',
        hideDefaultButton: shouldHideDefaultValueForEditButtons()
    });
};

These changes address the localization issues and optimize the duplication within the method call. Remember that ensuring proper localisation involves setting up an i18n framework and mapping strings accordingly across different languages. Optimization helps improve the maintainability and readability of the code, which is crucial for handling larger applications effectively.

Expand Down
9 changes: 9 additions & 0 deletions frontend/src/views/cronjob/library/operate/index.vue
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -127,6 +127,15 @@ const onSubmit = async (formEl: FormInstance | undefined) => {
const loadGroupOptions = async () => {
const res = await getGroupList('script');
groupOptions.value = res.data || [];
if (dialogData.value.title !== 'create') {
return;
}
for (const group of groupOptions.value) {
if (group.isDefault) {
dialogData.value.rowData.groupList = [group.id];
break;
}
}
};

defineExpose({
Copy link
Member

Choose a reason for hiding this comment

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

The code seems mostly correct, but here are some minor improvements to make it more concise and clear:

  1. Remove unnecessary semicolons: 
    const onSubmit = async (formEl: FormInstance | undefined) => {
    const res = await getGroupList('script');
    groupOptions.value = res.data || [];
  •  if (dialogData.value.title === 'edit') return;

  •  for (const group of groupOptions.value) {
     if (group.isDefault) {
         dialogData.value.rowData.groupList = [group.id];
         break;
     }
 }

    };

    defineExpose({


2. Add comments where necessary:
```javascript
const onSubmit = async (formEl: FormInstance | undefined) => {
   // Load groups based on script type
   const res = await getGroupList('script');
   groupOptions.value = res.data || [];

   // If editing the row data, skip loading default group ID
   if (dialogData.value.title === 'edit') return;

   // InitializerowData with default group ID from available options
   for (const group of groupOptions.value) {
       if (group.isDefault) {
           dialogData.value.rowData.groupList = [group.id];
           break;
       }
   }
};

defineExpose({
   ...
});

These changes improve readability by removing extraneous characters and adding explanatory comments that clarify the purpose each step in the function.

Copy link
Member

Choose a reason for hiding this comment

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

The differences between the provided code snippet appear to be modifications specifically related to handling default group options in a component with a dynamic dialog title. Here’s an explanation of these changes:

  1. Load Group Options Functionality:

    const loadGroupOptions = async () => {
    const res = await getGroupList('script');
    groupOptions.value = res.data || [];
};

    This function is responsible for fetching group data from an API endpoint when called, updating groupOptions with the received result.

  2. Conditional Logic on Dialog Title:

    if (dialogData.value.title !== 'create') {
    return;
}

    This condition checks whether the current dialog's title is not 'create'. If true, it skips executing further operations related to setting default groups. However, since this logic does not change based on certain conditions that need checking within its scope (like looping over different groups), it might not make sense without additional context about how dialogData.value.rowData.groupList gets manipulated elsewhere in the code.

  3. Setting Default Group:

    for (const group of groupOptions.value) {
    if (group.isDefault) {
        dialogData.value.rowData.groupList = [group.id];
        break;
    }
}

    This section iterates through each group option and sets the first one marked as 'isDefault' into dialogData.value.rowData.groupList, exiting after finding the first match. It implies that groups have an id property to identify which one should be set as a default.

In summary, the provided changes primarily address setting default group list entries based on specific conditions derived from the dialog’s title. These additions seem well-intentioned but could benefit from comments explaining why certain actions take place under some circumstances compared to others. Without more context around how dialogData.value and other variables interact elsewhere in your application, I cannot provide comprehensive feedback on their overall effectiveness or efficiency.

Expand Down
Loading