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

fix: status would generate invalid json #758

Merged
merged 35 commits into from Sep 11, 2018

Conversation

Projects
None yet
2 participants
@etschannen
Copy link
Contributor

etschannen commented Sep 8, 2018

No description provided.

@etschannen etschannen requested a review from satherton Sep 8, 2018

@satherton
Copy link
Contributor

satherton left a comment

While this problem is not new, the changes here highlight how JsonString is not very type safe, and we should fix that now before merging this.

Functions for adding to it make assumptions about what has been added to it before, but the class does not enforce that these assumptions are correct so it must be used very carefully or it will generate invalid JSON. Since there are so many places that use the class, it's hard to verify that all usages are in line with these assumptions. A comparison of code coverage events for status schema validation between 6.0.9 and current suggest that not all usages are correct.

@@ -194,8 +267,10 @@ JsonString& JsonString::append( bool value ) {
JsonString& JsonString::append( const JsonString& value ) {

This comment has been minimized.

@satherton

satherton Sep 8, 2018

Contributor

This should maybe point out that it assumes this is empty or a partial array and value is empty or a partial array.

satherton and others added some commits Sep 8, 2018

Added concept of type to JsonString. Appending single items or key/va…
…lue pairs is now type-safe and only allowed in certain cases. JsonString will refuse to produce invalid JSON. All duplicative functions have been replaced with templates. Encoding of values uses json_spirit's value writer which should be no worse performance than format() and it will escape everything properly. Final string form is now built directly using knowledge of type, such as when an instance becomes an Array or Object the appropriate opening character is written. This avoids a full copy just to prepend the opening character later. Index interface for key/value pairs no longer makes a temporary copy of the key string. JsonString is now only needed by Status.actor.cpp. Still more work to be done here.
JsonString refactored so that type compatibility is enforced at compi…
…le time. Specific JSON types are handled with subclasses that have type-specific access methods and a caller's intentions can no longer be ambiguous. Nothing that compiles should be able to produce malformed JSON.
fix: added missing commas
fix: const char* were being cast to bool
Some refactoring in JsonBuilder to remove code duplication and make s…
…ome expressions simpler. Some performance improvements, switched vector to VectorRef and used its bulk append method, and redirected basic mValue types to faster serialization. Added StringRef writeValue() method so that locality field writing in status does not have to use toString().
Simplified raw string writers and added StringRef version so that loc…
…ality field names in status do not have to be repeatedly copied.
Bug fix, slice was being used incorrectly which caused a missing inte…
…rnal character when combining object contents.
WriteValue() no longer uses format() which creates temporary strings,…
… instead if writes directly into allocated space in the target buffer using snprintf().
Rewrote ASCII -> JSON number support as sort of a filter which cleans…
… up ASCII numbers into valid JSON. Added tests for strange numbers which tests the generated output by parsing it as JSON.

@etschannen etschannen merged commit 23a979a into apple:release-6.0 Sep 11, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment