Fix and enhance StringConverter() to handle more special characters
#361
Conversation
Added unit tests to verify correct handling of special characters
Added special character handling for FormFeed, Backspace, Backslash, and Tab
|
Important Review skippedReview was skipped due to path filters ⛔ Files ignored due to path filters (4)
CodeRabbit blocks several paths by default. You can override this behavior by explicitly including those paths in the path filters. For example, including You can disable this status message by setting the Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
|
So, this is not properly working as explained in Discord. Here is the investigation: https://github.com/nanoframework/nanoFramework.Json/tree/pr361 And here are the elements:
To solve this problem:
|
|
And the case already exists: So, in short, it's about using the function I've commented for that case and calling it in the if. Otherwise, we should not use it. |
Added a bypass to return a string that as already been deserialized when it lands at StringConverter.ToType() Removed portion of string escape character processing in JsonConvert as it was implemented in StringConverter (unicode escaped character processing still remains in JsonConvert) Added Tests for multiple escaped characters in a string and in an object containing a string property. Added test case for string converter test for serializing and deserializing strings with multiple escaped characters
|
I noticed that the discord thread was not actually linked here. Discord thread RFC8259 - https://datatracker.ietf.org/doc/html/rfc8259 Seems like we are handling all the escaped characters correctly now from what I can tell based on testing. |
|
Are the |
The original unicode processing part of he code is still used for unicode characters. The existing tests that test unicode properly serializing and deserializing have all passed. |
Added unit tests to verify correct handling of special characters
Added special character handling for FormFeed, Backspace, Backslash, and Tab
Added a bypass to return a string that as already been deserialized when it lands at StringConverter.ToType() Removed portion of string escape character processing in JsonConvert as it was implemented in StringConverter (unicode escaped character processing still remains in JsonConvert) Added Tests for multiple escaped characters in a string and in an object containing a string property. Added test case for string converter test for serializing and deserializing strings with multiple escaped characters
Added test cases in StringConverterTests to further test serialization (StringConverter.ToJson) Added test cases in StringConverterTests to further test StringConverter.ToType and corrected the existing one (did not have removal of quotes which should happen when deserializing) Added test cases to JsonSerializerOptionsTests for multiple escaped characters and combinations such as escaped character being at the end of a string for both simple string objects and complex objects containing string properties. Corrected test CanDeserializeInvocationReceiveMessage_05 which had an expected value including quotes for a string that did not have quotes originally in it as "" becomes " and would have been removed during deserialization. In StringConverter I have made it so that it returns the string without quotes (as expected) when there are no escaped characters present as before if there were no escaped characters there would be an extra set of quotes left around the returned value. I also inverted the logic on the loop in order to ensure that the loop can exit without appending the letter portion of the escaped character if it is the last character in the string (such as a newline on the end would replace the escaped value as expected but would then attach the 'n' character to the end. I have replaced the deserialization of escaped characters to correctly replace them when deserializing an object with a string component. I also added the missing escaped characters that were added to the StringConverter in previous commit.
During de-serialization of strings that are part of complex objects there is a set of quotes removed that shouldn't be when escaped. This conditional block replaces those quotes.
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
There was a problem hiding this comment.
Before approving, can you add a test with a complex object, something that is already existing with a string that contains escaped chars so we are sure we cover properly that case as well?
For the rest, looks all goos! Thanks a lot for the efforts getting there.
|
The test labeled "Can_serialize_and_deserialize_object_containing_string_with_escaped_characters" located in the JsonSerializerOptionsTests has a data row for the case of escaped quotes at the beginning and end. Also in the same test class is "Can_serialize_and_deserialize_string_with_escaped_characters" which has all the same data rows for a string that is not part of a complex object. So unless I missed something (which of course is possible) then there should be tests handling all of these cases. |
|
@mizady from my understanding of @Ellerbach request: you can grab Can_serialize_and_deserialize_complex_object, duplicate it and sprinkle a couple of escaped strings there. |
Add 18 DataRows to the two tests for serialization and deserialization of simple strings and complex objects containing strings.
|
I've added 18 test cases to each of the two tests for simple string and complex object containing a string serialization and deserialization. this is now the list of tested combinations for both. Totaling 68 tests between the two. [DataRow("a")] // Single character |
|
Amazing! Thanks a lot! |
|
@josesimoes we do have 2 of the actions not running, any idea why before I force the merge? |
They were waiting for a maintainer approval to run. (github is getting heavier and heavier on security 😉 ) |
|
@mizady thank you again for your contribution! 🙏😄 .NET nanoFramework is all about community involvement, and no contribution is too small. Please edit it and add an entry with your GitHub username in the appropriate location (names are sorted alphabetically): (Feel free to adjust your name if it's not correct) |
josesimoes
changed the title
StringConverter() to handle more special characters
Description
Motivation and Context
How Has This Been Tested?
Types of changes
Checklist: