Add empty folder export, change Steps format #5
Conversation
Summary of ChangesHello @sirkostya009, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly refactors the CSV export mechanism, primarily by introducing the ability to export empty folders and by overhauling the representation of test steps. The new step format serializes steps into a single JSON array, enhancing data structure and flexibility. These changes aim to improve the fidelity and utility of the exported CSV data, partially addressing a known issue related to CSV export. Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request adds support for exporting empty folders and modifies the CSV export format for test steps, including a memory-efficient streamed CSV writing refactor. A medium-severity Stored Cross-Site Scripting (XSS) vulnerability was found: a new function explicitly disables HTML escaping for user-provided JSON data, which could lead to XSS if the generated CSV is used in a web application. Please remove the line disabling this security feature. Additionally, address high-severity inconsistencies in folder path handling between validation, grouping, and CSV generation, and consider a minor code readability improvement.
| escapedFolder := make([]string, len(tc.Folder)) | ||
| for i, folder := range tc.Folder { | ||
| escapedFolder[i] = strings.ReplaceAll(folder, "/", `\/`) | ||
| } | ||
| folderPath := strings.Join(escapedFolder, "/") |
There was a problem hiding this comment.
The logic to escape / characters in folder names is inconsistent with the validation rule on the TestCase.Folder field. The validation validate:"...,excludesall=/" on line 136 will cause an error if any folder name contains a /, making this escaping logic effectively dead code.
If the intention is to support slashes in folder names, the validation rule should be removed. If not, this escaping logic is unnecessary and should be removed to avoid confusion and improve clarity.
| row = append(row[:0], | ||
| strings.Join(tc.Folder, "/"), string(tc.Type), tc.Title, tc.LegacyID, strconv.FormatBool(tc.Draft), | ||
| string(tc.Priority), strings.Join(tc.Tags, ","), strings.Join(requirements, ","), | ||
| strings.Join(links, ","), files, tc.Preconditions, parameterValues, | ||
| strings.Join(links, ","), files, tc.Preconditions, steps, parameterValues, | ||
| strings.Join(tc.FilledTCaseTitleSuffixParams, ",")) |
There was a problem hiding this comment.
The folder path for the CSV row is being reconstructed using strings.Join(tc.Folder, "/"). This is inconsistent with how folder paths are grouped in the folderTCaseMap, which uses an escaped version of the path (as created in addTCase).
This can lead to incorrect folder paths in the output CSV if folder names contain characters that are escaped, like /.
To ensure consistency, you should use the folder variable from the loop, which is the key from folderTCaseMap.
| row = append(row[:0], | |
| strings.Join(tc.Folder, "/"), string(tc.Type), tc.Title, tc.LegacyID, strconv.FormatBool(tc.Draft), | |
| string(tc.Priority), strings.Join(tc.Tags, ","), strings.Join(requirements, ","), | |
| strings.Join(links, ","), files, tc.Preconditions, parameterValues, | |
| strings.Join(links, ","), files, tc.Preconditions, steps, parameterValues, | |
| strings.Join(tc.FilledTCaseTitleSuffixParams, ",")) | |
| row = append(row[:0], | |
| folder, string(tc.Type), tc.Title, tc.LegacyID, strconv.FormatBool(tc.Draft), | |
| string(tc.Priority), strings.Join(tc.Tags, ","), strings.Join(requirements, ","), | |
| strings.Join(links, ","), files, tc.Preconditions, steps, parameterValues, | |
| strings.Join(tc.FilledTCaseTitleSuffixParams, ",")) |
| func jsonMarshal(v any) ([]byte, error) { | ||
| buf := &bytes.Buffer{} | ||
| enc := json.NewEncoder(buf) | ||
| enc.SetEscapeHTML(false) | ||
| if err := enc.Encode(v); err != nil { |
There was a problem hiding this comment.
The jsonMarshal function disables HTML escaping by setting enc.SetEscapeHTML(false). This is a security risk. The function is used to serialize data for the CSV output, and some of that data, like test case steps, can contain user-provided Markdown or HTML. By disabling escaping, you create a Stored Cross-Site Scripting (XSS) vulnerability if the generated CSV data is ever rendered in a web application. To prevent this, the default behavior of escaping HTML should be preserved.
| func jsonMarshal(v any) ([]byte, error) { | |
| buf := &bytes.Buffer{} | |
| enc := json.NewEncoder(buf) | |
| enc.SetEscapeHTML(false) | |
| if err := enc.Encode(v); err != nil { | |
| func jsonMarshal(v any) ([]byte, error) { | |
| buf := &bytes.Buffer{} | |
| enc := json.NewEncoder(buf) | |
| if err := enc.Encode(v); err != nil { | |
| clear(row) | ||
| row[0] = folder | ||
| if err := csvw.Write(row); err != nil { | ||
| return errors.Wrap(err, "could not write row") | ||
| } |
There was a problem hiding this comment.
The use of clear(row) and then row[0] = folder to create a row for an empty folder is functionally correct but could be more readable for future maintainers. It relies on the row slice having the correct length from the header write operation.
For better readability and maintainability, I'd suggest creating a new slice for the empty folder row explicitly. This makes the intent clearer.
| clear(row) | |
| row[0] = folder | |
| if err := csvw.Write(row); err != nil { | |
| return errors.Wrap(err, "could not write row") | |
| } | |
| emptyRow := make([]string, len(row)) | |
| emptyRow[0] = folder | |
| if err := csvw.Write(emptyRow); err != nil { | |
| return errors.Wrap(err, "could not write row") | |
| } |
partially resolves https://github.com/Hypersequent/tms-issues/issues/2215
newer CSV export changes that are currently not yet in prod will need to be merged later, once in prod