json icon indicating copy to clipboard operation
json copied to clipboard

Introduce try_get() and try_deserialize()

Open oficsu opened this issue 4 years ago • 12 comments

I propose new method json.try_get<MyType>(), which works similar to json.get<MyType>(). It can be extended by providing adl_serializer<MyType>::try_deserialize() function. Unlike get method, the return type of try_get is assumed to have a different return type to MyType and can be std::optional, tl::expected or other wrapper

Why is it needed?

  1. Users may prefer to handle errors without exceptions for the sake of performance
  2. Or, some users like more functional program structuring, for example, using monads, this allows not only to handle errors, but also to return some additional info about deserialization, such as warnings or even detailed debug logs.
  3. Maybe something else?

Why not just allow json.get<MyType>() to return any type, different from MyType?

  1. This is not obvious code behavior and some users may not expect a different return type
  2. Some users may want to explicitly distinguish between json.get<MyType>() and json.try_get<MyType>(), or even have both

What about json.get<MyMonad<MyType>>()? Why not?

  • Users can define their own version of try_get() with custom return type once and don't bother about return type anymore every time get() is called. This is quite important in a large codebase where get() calls many times for same type
  • Or, even more importantly, I think json.try_get<MyType>() is easier to read and is a good abstraction compared to json.get<MyMonad<MyType>>()
  • Also, I think json.try_get<T>() is the better way to return optional than json.get<std::optional<T>>() from #2229

So, is noexcept our goal? Does json.try_get<X>() throw exceptions?

  • It's depend on user implementation of try_deserialize() and it is the responsibility of the users to mark try_deserialize() as noexcept(true) or noexcept(false).

Should we implement try_deserialize() for basic types?

  • I'm not sure. tl::expected or std::optional could be suitable for this, but most likely, the user will want to write their own monadic type or wrapper. Also, there may be a problem when users include two libraries that use nlohman_json and specializing different adl_serializer for the same basic type. Maybe we can implement try_deserialize lookup through tag dispatching with adl and allow additional arguments to be passed to try_get(), then users can create their own unambiguous overloads. More research is needed here and right now I think the answer is no

About naming

  • I'm not sure about the naming. I think, try_deserialize() is too contrast to from_json(), but I don't know what name is better

oficsu avatar Jul 18 '20 16:07 oficsu

Coverage Status

Coverage remained the same at 100.0% when pulling 7f4d035cfe468f05efdb548f3995bbba71c20250 on oficsu:try_deserialize_squash into 43ab8a2357de50279efec307b718d25c44176e98 on nlohmann:develop.

coveralls avatar Jul 18 '20 18:07 coveralls

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

stale[bot] avatar Aug 29 '20 15:08 stale[bot]

After thinking about this PR again, I decided that it requires some more work. Perhaps design could be better. I don't have much free time at the moment, so I'll come back later and think about this PR again. Do I need to close PR for this time or can I leave PR as a draft?

oficsu avatar Sep 10 '20 08:09 oficsu

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

stale[bot] avatar Oct 12 '20 00:10 stale[bot]

In addition, it would be handy to have the macros such as: NLOHMANN_DEFINE_TYPE_INTRUSIVE use a version which is ok if the data isn't found. try/catching everything can be expensive if "not found" is an acceptable state.

Expanding even further to the "optional is ok", bool from_json(.......) whereby validation can be reported as a warning, or diagnostic, but not as an error that is try/catch.

It means, NLOHMANN_DEFINE_TYPE_INTRUSIVE isn't as useful, or jsons become bigger with required params being null instead of leaving them out.

An example additional macro: NLOHMANN_DEFINE_TYPE_TRY_INTRUSIVE(.....) In this simple macro naming, it's "all or none", and that's ok as optional might be the common usecase.

Additionally, having from_json (and thus, macro) return true/false, or count of missing params, or a tuple of found / maxcount would be helpful and allow for custom detection.

SpiritDogstar avatar Nov 11 '20 19:11 SpiritDogstar

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

stale[bot] avatar Dec 25 '20 13:12 stale[bot]

Would it make sense extending this to have an optional default value?

I am in the process of porting over a project from jsoncpp to nlohmann-json, and the former has this as a very convenient feature (for us at least), i.e. .get("field", Json::object()) to get an optional empty object if not present.

Is there a best practice for this in the current library?

axic avatar Nov 07 '21 20:11 axic

Would it make sense extending this to have an optional default value?

I am in the process of porting over a project from jsoncpp to nlohmann-json, and the former has this as a very convenient feature (for us at least), i.e. .get("field", Json::object()) to get an optional empty object if not present.

Is there a best practice for this in the current library?

The value function should do this: https://json.nlohmann.me/api/basic_json/value/.

nlohmann avatar Nov 07 '21 20:11 nlohmann

@nlohmann oh wow, that certainly looks like it, thank you!

axic avatar Nov 07 '21 20:11 axic

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

stale[bot] avatar Jan 09 '22 02:01 stale[bot]

After thinking about this PR again, I decided that it requires some more work. Perhaps design could be better. I don't have much free time at the moment, so I'll come back later and think about this PR again. Do I need to close PR for this time or can I leave PR as a draft?

@oficsu Any progress here?

gregmarr avatar Jun 18 '22 17:06 gregmarr

@gregmarr, no, I couldn't come up with a better design

oficsu avatar Jun 19 '22 14:06 oficsu