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

Serializing json instances containing already serialized string values without escaping #385

Closed
orcun opened this issue Dec 7, 2016 · 9 comments
Labels
kind: enhancement/improvement state: please discuss please discuss the issue or vote for your favorite option

Comments

@orcun
Copy link

orcun commented Dec 7, 2016

I found it quite difficult to correctly describe my intention in the title, which is probably wrong. A picture is worth a thousand words, how much a code sample is worth?

// a user defined string literal _json_inlined, shorthand for json::make_inlined("") static function
json j = {"foo", 1, 2, 3, false, {{"one", 1}}, "{\"two\":2}"_json_inlined};
CHECK(j.dump() == "[\"foo\",1,2,3,false,{\"one\":1},{\"two\":2}]");

This enables passing json strings from a business logic layer to network layer without having to parse it. Please note that passing json instances may not be an option because of module boundaries. Opposite of this scenario (not fully parsing part of a json string) would also be great but seems very difficult without making big changes.

I have a working implementation which was quite straightforward and around 20 lines of code. However it requires definition of a new value type, so I wasn't able to design it as an extension. So my question is, do you think would allowing such extensions (perhaps using some template meta-programming techniques) is feasible? If so, perhaps I can help with that and implement this as an extension. If not, would you consider a pull request for just this capability? Or perhaps both 🐱

@nlohmann
Copy link
Owner

nlohmann commented Dec 7, 2016

This is a very specific use case, and you are right: it would require a new value type.

I wonder what other people think of this.

@nlohmann nlohmann added the state: please discuss please discuss the issue or vote for your favorite option label Dec 7, 2016
@orcun
Copy link
Author

orcun commented Dec 7, 2016

Thanks a lot for the quick response. I somehow 🤔 bumped into this use case in many projects, using different programming languages and libraries and have always found a way. Simplest one that comes up to my mind is wrapping a json request without parsing it. In fact, if there were no need to define a custom value type, one could even prevent creating a temporary string in such particular case using another custom type.

In the mean time, I have looked at #338 and it does not enable such feature. However it looks like a pretty straightforward change too. I am not sure if a more comprehensive approach is needed to solve both requirements. I think introducing an inlined string and/or raw stream value type together with #338 can cover many use cases.

@gregmarr
Copy link
Contributor

gregmarr commented Dec 7, 2016

@orcun So this is a value type that when dumped inserts the provided json string? What happens to it when other operations are done on it? It seems to be really easily to embed a string that isn't actually valid json, and thus the dump() function would return an invalid string.

@orcun
Copy link
Author

orcun commented Dec 7, 2016

@gregmarr If library user provides an invalid string, user will get an invalid string. If checking validity is a requirement, it is a separate issue. Even so, user can check validity by json::parse("...") beforehand and there is still a performance gain in dump (i.e. thin wrapper around a very big request). If you think that might be a common use case, perhaps json::make_inline("...") can be overloaded to check for validity. In that case, we might even think of something like json::is_valid("...") without the overhead of creating an instance.

Nevertheless, I think validity is not the job of the library when not required. Who knows, it might even be a feature itself, can be used to produce partially invalid json for test cases.

@TurpentineDistillery
Copy link

@orcun

The serialization use-case and garbage-in/garbage-out issues are pretty clear; the question that @gremarr is raising however, is what would the partially-deserialized semantics be outside of the context of serialization - should it behave as if it was fully-deserialized (mutable/deserialize-on-demand)? If not, should the partially-deserialized state be considered invalid, except for serialization? Should the API prohibit access to inline_json except for construction via make_inline or some-such, or should inline_json be a first-class-citizen value-type, and if so, what are the implications of nlohmann::json type losing the isomorphism to JSON format.

@orcun
Copy link
Author

orcun commented Dec 7, 2016

@TurpentineDistillery Thanks for pointing that.

Without thinking that much, I assumed it should not be used except for serialization in my current implementation. Deserialization on demand is possible but having non-const read operations is not worth it in my opinion. It would only make sense if deserialization could be partially deferred in general. Again, my initial implementation limits its construction and try to limit its implications.

If majority thinks that this will be useful, I can make a pull request and we can discuss the details over the code.

@nlohmann
Copy link
Owner

nlohmann commented Dec 7, 2016

I am currently not convinced to add this feature to the library. It covers a very specific use case which bears some "rough edges" when used the wrong way. So far, all value types of the library can be mapped to value types of JSON. I am worried that when breaking up this mapping, the library becomes harder to understand and also harder to test.

@nlohmann
Copy link
Owner

I don't think extending the library to this feature is helpful in the long run. It would mean too many limitations that need to be documented or worked around. Sorry @orcun.

@orcun
Copy link
Author

orcun commented Dec 12, 2016

No worries, we will be using my fork for now (I can put it online if someone somehow finds this thread). In the mean time, this inspired me to start a new project.

So long and thanks for all the 🐟

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind: enhancement/improvement state: please discuss please discuss the issue or vote for your favorite option
Projects
None yet
Development

No branches or pull requests

4 participants