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

PrereleaseCompatibilityTransformer mishandles date/times in property bag #1577

Closed
ghost opened this issue Jul 16, 2019 · 1 comment · Fixed by #1688
Closed

PrereleaseCompatibilityTransformer mishandles date/times in property bag #1577

ghost opened this issue Jul 16, 2019 · 1 comment · Fixed by #1688

Comments

@ghost
Copy link

ghost commented Jul 16, 2019

Given the input file:
repro-1577.sarif.txt

the command

Sarif.Multitool.exe transform --pretty-print --force --output repro-1577-transformed.sarif repro-1577.sarif

produces the output file
repro-1577-transformed.sarif.txt

repro-1577

Note that invocation.startTimeUtc is correctly left alone, but a date/time value in the property bag is transformed from the quoted string "2019-06-22T19:06:31.209Z" to the unquoted and reformatted string 6/22/2019 7:06:31 PM.

@ghost ghost self-assigned this Aug 15, 2019
@ghost
Copy link
Author

ghost commented Aug 15, 2019

This is not a bug in the PrereleaseCompatibilityTransformer. It is a known bug in Newtonsoft.Json: JamesNK/Newtonsoft.Json#1241. Checking to see if there is a workaround. @michaelcfanning FYI.

ghost pushed a commit that referenced this issue Aug 16, 2019
@ghost ghost closed this as completed in 32fd1c7 Aug 22, 2019
@ghost ghost added the resolved-fixed label Aug 22, 2019
ghost pushed a commit that referenced this issue Sep 30, 2019
…#1688)

I added a comprehensive end-to-end test of round-tripping property bag properties. I also added a `DateTime` test to the `PropertyBagConverterTests` functional tests.

The problem that @ScottLouvau ran into with the `DateTime` test is related to my fix for #1577. I don't mean that my fix caused his problem; I mean that to avoid the problem, we need to do the same thing I did to fix #1577. For an explanation, see [this comment](https://github.com/microsoft/sarif-sdk/blob/83ecf0fa2d919a0be70a144464fc25d8b898a294/src/Sarif/SarifUtilities.cs#L190-L214) in SarifUtilities.cs.

In short, when Newtonsoft.Json deserializes a string-valued property that looks to it like a `DateTime`, its default behavior is to convert it to a `DateTime` object. That behavior breaks our property round-tripping. To avoid it, we need to call `JsonConvert.DeserializeObject` with a `JsonSerializerSettings` object that specifies `DateParseHandling.None`. That tells Newtonsoft.Json to leave string-valued properties alone and let us handle the `DateTime`-iness. In #1577, I introduced `SarifUtilities.DeserializeObject` to do exactly that.

@michaelcfanning and I have discussed this and we know it's not the end of the story. It's too easy to forget (or not to know about) the need to call the `SarifUtilities` method, which is exactly what happened in that existing unit test. But as far as verifying that our handling of property bag properties is good, I believe we now have sufficient test coverage to be confident of that.

Also:
- Fix a NuGet packaging warning.
- Improve our internal handling of property bag properties with `null` values.
@ghost ghost mentioned this issue Oct 17, 2019
This issue was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

1 participant