Skip to content
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.

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

bug: Add improved support for single arguments in get_list_args function #3589

Merged
merged 24 commits into from
Jun 13, 2024

Conversation

TommyE123
Copy link
Contributor

@TommyE123 TommyE123 commented May 27, 2024

Fixes

Initial issue experienced in an Azure Devops pipeline.

When setting the CSHARP_DOTNET_FORMAT_ARGUMENTS variable to point to a solution file within a subfolder, the Dotnet_Format Linter encounters an error due to incorrect argument parsing. For example, setting:

CSHARP_DOTNET_FORMAT_ARGUMENTS: ".\SolutionFolder\SolutionFile.sln"

results in the following error: (notice the \ has gone)
Unrecognized command or argument “.SolutionFolderSolutionFile.sln".

Similarly, using forward slashes:
CSHARP_DOTNET_FORMAT_ARGUMENTS: "./SolutionFolder/SolutionFile.sln"

results in:
Unrecognized command or argument "./SolutionFolder/SolutionFile.sln".

The problem appears to arise when the solution file is in a subfolder and the argument is passed through the shlex.split method, which misinterprets the slashes.

Proposed Changes

To address this, I've modified the get_list_args function to check if the input string contains a space character before splitting it. If the string doesn't contain a space, it is assumed to be a single argument and is returned as a list containing the original string.

This change should hopefully (assuming its not been already lost before or after this method call) fix the issue when using a single argument for CSHARP_DOTNET_FORMAT_ARGUMENTS that includes a path with subfolders. However, if multiple arguments are required, further adjustments may be needed.

This change ensures that single arguments with paths are preserved correctly and passed as intended.

Benefits:

  • Correct Handling of Single Arguments: The change ensures that single arguments, especially paths, are not split incorrectly, which prevents errors when these arguments are passed to the linter.
  • Backward Compatibility: The change retains the existing logic for handling lists, empty strings, and None values, ensuring that the function's behavior remains consistent for other input types.
  • Simplicity: The additional check is straightforward and does not introduce unnecessary complexity. It simply ensures that single arguments are preserved as-is if they do not contain spaces.
  • Improve support for single-word arguments Although it does not address cases with multiple arguments, it significantly improves the function's reliability in common scenarios where only one argument is used.

PR UPDATE

After extensive further testing, I've identified several issues with the dotnet_format linters. While these issues require more targeted fixes within the dotnet_format code, this proposed change won't directly resolve them. However, this change is necessary for ultimately addressing these issues.

The change involves fixing some cases where shlex.split is unnecessarily called, which doesn't handle certain scenarios well. I believe it's better to merge this change separately since it's generic and will still resolve some issues.

I've created 70 unit tests to ensure the method behaves consistently before and after the change. These tests have highlighted a few scenarios that now work correctly and others that still fail. I appreciate some might not currently be in use or required however they still now don’t need to call shlex.split if not required to!

In parallel, I am working on addressing the specific issues within the csharp and vb dotnet_format code itself to provide a comprehensive solution to vastly improve the speed, relative paths and other issues found.

Does this approach sound acceptable, or would you suggest any changes?

regards
Tom

Testing

Tests added with existing code:

image
image
image
Sub-tests for [tests.test_megalinter.config_test.config_test.test_get_list_args]: Total=70 Passed=57 Failed=13

Tests failing with new code:

image
Sub-tests for [tests.test_megalinter.config_test.config_test.test_get_list_args]: Total=70 Passed=66 Failed=4

Tests left passing after commenting out broken tests with new code:

Sub-tests for [tests.test_megalinter.config_test.config_test.test_get_list_args]: Total=66 Passed=66 Failed=0

Readiness Checklist

Author/Contributor

  • Add entry to the CHANGELOG listing the change and linking to the corresponding issue (if appropriate)

Reviewing Maintainer

  • Label as either automation, bug, documentation, enhancement, infrastructure, or performance

@echoix
Copy link
Collaborator

echoix commented May 27, 2024

Wasn't there a bug recently in the dotnet cli/tools with relative solution paths, something like this? I remember hitting it, but not with visual studio. Maybe when listing packages. I remember it trying to be solved, and announced by Microsoft somewhere, but can't exactly re find that info.

How does this fix help when there is a space in the path?

@TommyE123
Copy link
Contributor Author

Hi @echoix,

The space is not really relevant. It simply means the shlex.split method doesn't need to be called if there isn't one {as that means theres only 1 argument), which I think is removing the \ from the path.

When I run dotnet format .\SolutionFolder locally from the repository root, it formats fine. Although, I could be mistaken, and the issue might be happening elsewhere in the megalinter code. However, I was thinking it would be better not to try and split a single argument when it's not required.

This fix would apply to arguments in general, not just the 'dotnet format' command.

I've tried a few workarounds like escaping the argument, but they often end up crashing the method (see attached).

Hope this make sense? Let me know if you need any clarification or have additional thoughts.

Traceback Errors.txt

@nvuillam
Copy link
Member

nvuillam commented May 28, 2024

The best with arguments of type "list" is to use the YML list way

For example, in .mega-linter.yml file:

CSHARP_DOTNET_FORMAT_ARGUMENTS: 
  - ".\SolutionFolder\SolutionFile.sln"

The split is a not pretty workaround, but i'm not sure what would happen with your update if we had something like :

CSHARP_DOTNET_FORMAT_ARGUMENTS: 
  - "unique element with spaces"

@TommyE123 TommyE123 changed the title bug: Add support for single argument in get_list_args function bug: Add improved support for single arguments in get_list_args function May 29, 2024
@TommyE123
Copy link
Contributor Author

Hi @echoix,
I've updated the PR comments and believe this to now be ready to merge.
Any questions or further tests you would like added please let me know.

Tom

@TommyE123 TommyE123 marked this pull request as ready for review June 13, 2024 19:24
@echoix
Copy link
Collaborator

echoix commented Jun 13, 2024

With all that proof, before and after, all these test cases, it would be hard to ask for more. Clearly the state is better with than without.

Thanks again for another amazing work!

@echoix echoix merged commit 46f129b into oxsecurity:main Jun 13, 2024
6 checks passed
@nvuillam
Copy link
Member

Impressive list of test cases indeed :)

@TommyE123 TommyE123 deleted the Args_Split_Improvement branch June 23, 2024 07:25
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.

3 participants