Skip to content

Support StopProcessing in ConvertTo-Json #6392

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
daxian-dbw merged 9 commits into from
Mar 19, 2018
Merged

Support StopProcessing in ConvertTo-Json #6392

Changes from 1 commit
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
fb52406
support StopProcessing in ConvertTo-Json
SteveL-MSFT Mar 14, 2018
a84bfcb
address Dan's feedback
SteveL-MSFT Mar 14, 2018
5bd2fec
address Mark's feedback
SteveL-MSFT Mar 14, 2018
3c076d4
move stop check to inside the loop
SteveL-MSFT Mar 14, 2018
8c2dc94
address Bruce and Mark's comments
SteveL-MSFT Mar 15, 2018
38a3115
address Ilya's feedback
SteveL-MSFT Mar 15, 2018
4a141b1
address Dongbo's feedback
SteveL-MSFT Mar 15, 2018
ce92da6
address Paul's feedback
SteveL-MSFT Mar 16, 2018
823d6f2
address Ilya's feedback
SteveL-MSFT Mar 16, 2018
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
Prev Previous commit
Next Next commit
address Mark's feedback
  • Loading branch information
@SteveL-MSFT
SteveL-MSFT committed Mar 14, 2018
commit 5bd2fec9918f3c838550ff355cedafa3e1eee8b2
72 changes: 29 additions & 43 deletions src/Microsoft.PowerShell.Commands.Utility/commands/utility/WebCmdlet/ConvertToJsonCommand.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -108,20 +108,18 @@ protected override void EndProcessing()
// Pre-process the object so that it serializes the same, except that properties whose
// values cannot be evaluated are treated as having the value null.
object preprocessedObject = ProcessValue(objectToProcess, 0);
if (!_stopping)
CheckStopping();

@PaulHigin PaulHigin Mar 15, 2018

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I am a little confused by this. The StoppingException is caught and ignored in one case but allowed to throw if ProcessValue() completes via CheckStopping(). What is the user experience? Is the user intended to see the exception or does the cmdlet just stop?

@daxian-dbw daxian-dbw Mar 15, 2018

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I don't think we need to check for stopping again here. You may argue that the user can have a final chance to stop the command before the JsonConvert.SerializeObject has the control, though :)

If you really want to check for stop another time here, then if (Stopping) { return } should be sufficient.

@SteveL-MSFT SteveL-MSFT Mar 16, 2018

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Yes, will change.

JsonSerializerSettings jsonSettings = new JsonSerializerSettings { TypeNameHandling = TypeNameHandling.None, MaxDepth = 1024 };
if (EnumsAsStrings)
{
JsonSerializerSettings jsonSettings = new JsonSerializerSettings { TypeNameHandling = TypeNameHandling.None, MaxDepth = 1024 };
if (EnumsAsStrings)
{
jsonSettings.Converters.Add(new StringEnumConverter());
}
if (!Compress)
{
jsonSettings.Formatting = Formatting.Indented;
}
string output = JsonConvert.SerializeObject(preprocessedObject, jsonSettings);
WriteObject(output);
jsonSettings.Converters.Add(new StringEnumConverter());
}
if (!Compress)
{
jsonSettings.Formatting = Formatting.Indented;
}
string output = JsonConvert.SerializeObject(preprocessedObject, jsonSettings);
WriteObject(output);
}
}

Expand Down Expand Up @@ -177,11 +175,7 @@ private string ConvertToPrettyJsonString(string json)
/// <returns></returns>
private int ConvertList(string json, int index, StringBuilder result, string padString, int numberOfSpaces)
{
if (_stopping)
{
throw new StopUpstreamCommandsException(this);
}

CheckStopping();

@dantraMSFT dantraMSFT Mar 14, 2018 •
edited
Loading

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I think this should be either inside in for loop or mirrored there. #Closed

result.Append("\r\n");
StringBuilder newPadString = new StringBuilder();
newPadString.Append(padString);
Expand Down Expand Up @@ -251,11 +245,7 @@ private int ConvertList(string json, int index, StringBuilder result, string pad
/// <returns></returns>
private int ConvertQuotedString(string json, int index, StringBuilder result)
{
if (_stopping)
{
throw new StopUpstreamCommandsException(this);
}

CheckStopping();

@dantraMSFT dantraMSFT Mar 14, 2018 •
edited
Loading

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Like above, consider moving this inside the for loop. #Closed

for (int i = index; i < json.Length; i++)
{
result.Append(json[i]);
Expand Down Expand Up @@ -296,11 +286,7 @@ private int ConvertQuotedString(string json, int index, StringBuilder result)
/// <returns></returns>
private int ConvertDictionary(string json, int index, StringBuilder result, string padString, int numberOfSpaces)
{
if (_stopping)
{
throw new StopUpstreamCommandsException(this);
}

CheckStopping();

@dantraMSFT dantraMSFT Mar 14, 2018 •
edited
Loading

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Again, inside the for loop. #Closed

result.Append("\r\n");
StringBuilder newPadString = new StringBuilder();
newPadString.Append(padString);
Expand Down Expand Up @@ -429,11 +415,7 @@ private ErrorRecord NewError()
/// <returns>An object suitable for serializing to JSON</returns>
private object ProcessValue(object obj, int depth)
{
if (_stopping)
{
throw new StopUpstreamCommandsException(this);
}

CheckStopping();
PSObject pso = obj as PSObject;

if (pso != null)
Expand Down Expand Up @@ -619,11 +601,7 @@ private void AppendPsProperties(PSObject psobj, IDictionary receiver, int depth,
/// <returns></returns>
private object ProcessDictionary(IDictionary dict, int depth)
{
if (_stopping)
{
throw new StopUpstreamCommandsException(this);
}

CheckStopping();

@dantraMSFT dantraMSFT Mar 14, 2018 •
edited
Loading

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Consider inside the for loop. #Closed

Dictionary<string, object> result = new Dictionary<string, object>(dict.Count);

foreach (DictionaryEntry entry in dict)
Expand Down Expand Up @@ -658,10 +636,7 @@ private object ProcessEnumerable(IEnumerable enumerable, int depth)

foreach (object o in enumerable)
{
if (_stopping)
{
throw new StopUpstreamCommandsException(this);
}
CheckStopping();

@daxian-dbw daxian-dbw Mar 15, 2018

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Same here. For each item in the enumerable, ProcessValue will be called and a check has been put in it.

@SteveL-MSFT SteveL-MSFT Mar 15, 2018

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Will remove

result.Add(ProcessValue(o, depth + 1));
}

Expand All @@ -686,7 +661,7 @@ private object ProcessCustomObject<T>(object o, int depth)

foreach (FieldInfo info in t.GetFields(BindingFlags.Public | BindingFlags.Instance))
{
if (!info.IsDefined(typeof(T), true) && !_stopping)
if (!info.IsDefined(typeof(T), true))
{
object value;
try
Expand All @@ -704,7 +679,7 @@ private object ProcessCustomObject<T>(object o, int depth)

foreach (PropertyInfo info2 in t.GetProperties(BindingFlags.Public | BindingFlags.Instance))
{
if (!info2.IsDefined(typeof(T), true) && !_stopping)
if (!info2.IsDefined(typeof(T), true))
{
MethodInfo getMethod = info2.GetGetMethod();
if ((getMethod != null) && (getMethod.GetParameters().Length <= 0))
Expand All @@ -725,5 +700,16 @@ private object ProcessCustomObject<T>(object o, int depth)
}
return result;
}

/// <summary>
/// Check if StopProcessing() was called and throw special exception to stop
/// </summary>
private void CheckStopping()

@iSazonov iSazonov Mar 16, 2018

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Seems we should remove the the help method - used one time.

@SteveL-MSFT SteveL-MSFT Mar 16, 2018

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Yes, with the removal of the unused code, this helper is no longer needed. Will remove.

{
if (_stopping)
{
throw new StopUpstreamCommandsException(this);

@daxian-dbw daxian-dbw Mar 15, 2018

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I don't think we should use this exception. It's supposed to be thrown from a ProcessRecord method, to stop the upstream Process blocks from running. For the ConvertTo-Json cmdlet, we do the real work at EndProcessing, and by that time all upstream commands have finished running.

I suggest you use a custom exception instead, can catch the exception in EndProcessing when calling ProcessValue, like this:

object preprocessedObject = null;
try
{
    preprocessedObject = ProcessValue(objectToProcess, 0);
}
catch (MyCustomException ex)
{
    return;
}

@SteveL-MSFT SteveL-MSFT Mar 15, 2018

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Makes sense, will update.

}
}
}
}