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 fix] Add a format validation step before format conversion. #36404

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

shuaiyuanxx
Copy link
Contributor

Summary of the Pull Request

This PR aims to fix the bug #35225 by introducing a new method IsJson to determine if a given text is in JSON format.
The IsJson method is then utilized in the ToJsonFromXmlOrCsvAsync method to optimize the processing logic.
If the text is already in JSON format, it is returned directly without further conversion from XML or CSV.

PR Checklist

Detailed Description of the Pull Request / Additional comments

Key Changes:

  1. Added System.Text.Json namespace.
  2. Implemented IsJson method to check if the text is in JSON format.
  3. Updated ToJsonFromXmlOrCsvAsync method to use IsJson and return the text directly if it is already JSON.

Validation Steps Performed

Manually tested

If it is already in JSON format, it should be returned directly

Signed-off-by: Shawn Yuan <[email protected]>
@htcfreek
Copy link
Collaborator

Before you merge plese check this similar PR: #34151

Copy link
Collaborator

@htcfreek htcfreek left a comment

Choose a reason for hiding this comment

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

Can't do a technical test yet. But code looks good to me.

I would prefer this solution over the one in the other PR.

Copy link

@yeelam-gordon yeelam-gordon left a comment

Choose a reason for hiding this comment

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

Not sure if we can add a unit test on it.

{
try
{
JsonDocument.Parse(text);

Choose a reason for hiding this comment

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

using JsonDocument doc = JsonDocument.Parse(text);

Looks like this function return something should be disposed after using.

Signed-off-by: Shawn Yuan <[email protected]>
{
try
{
using JsonDocument doc = JsonDocument.Parse(text);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is this really auto disposed? Not better to use underscore?

Suggested change
using JsonDocument doc = JsonDocument.Parse(text);
_ = JsonDocument.Parse(text);

@Jay-o-Way Jay-o-Way added the Product-Advanced Paste Refers to the Advanced Paste module label Dec 18, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Product-Advanced Paste Refers to the Advanced Paste module
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants