Add a fallback type to use_json_discriminator

[erdnaxeli]

Hi,

I use use_json_discriminator, it is a very useful feature, but I found it a bit restrictive. Indeed, the mapping must support all possible values of the discriminator, else it will raise an error about an unknown discriminator value (btw this is not documented). This means that if you implements some API using this mechanism (for example GeoJSON as stated in the doc) you have to implements all possible types. Even there, you are not protected from some new type coming into your input (especially when working with an external API).

So in my project I use a slightly different macro. It takes an extra parameter called "default" (but writing this I realize "fallback" is probably a better name) which a type that will be used to deserialize the input if the discriminator value is not found in the mapping.

Would it be a good idea to add it to the stdlib?

Thanks.

[asterite]

I think this is a good idea.

[asterite]

(but please wait for some more comments to see if others also think this is a good idea before jumping to a PR)

[straight-shoota]

That's exactly one of the features I predicted would keep adding complexity to discriminator) 😉

But I suppose it's a useful extension and does not actually change that much code.

[asterite]

I personally don't exactly understand the use case, but I think it's a small change.

This means that if you implements some API using this mechanism (for example GeoJSON as stated in the doc) you have to implements all possible types.

Well, if you don't implement all possible types then I guess you are not implementing the full spec, right? That means you have a latent bug in your code. And what will happen? An exception is raised. You can capture it and log it.

Even there, you are not protected from some new type coming into your input (especially when working with an external API)

Yes, you are. You get an exception: you handle it, log it, and move on.

Could you explain a bit more what do you want to do in these exceptional cases?

[erdnaxeli]

Ok my use case is the following. I am implementing a client lib for Matrix. This is a decentralized communication protocol, where main objects are event (to describe a room's state change, to send a message, …). The main API is a /sync endpoint where you get a array of events for each room (a room is like an IRC channel) you are in. The response to this endpoint is a big JSON content, containing new events for all the rooms you are in, so it can get pretty big. So first, if there is an unknown event in this big JSON, I don't want to have an exception and miss all the other events I could have handled.

About why I could receive an unknown event:

• my actual case: I did not implemented everything but I still want to start playing with my project :)
• the spec itself says that client don't have to implement the full spec. The spec specifies modules that clients can implements or not.
• a client can actually send any event they want. The spec specify a list of events but does not refuse others events (this allow clients to propose new behavior without needing a spec change).

Actually… I must be honest, there is also a mechanism in the Matrix API which allows a client to filter the events it want to receive (and I should probably implement that). But I still think having a fallback so you don't throw all the json when there is one unknown object in it is a good idea.

However I get @straight-shoota 's point, this is maybe a too specific feature to be in the stdlib. And duplicating and editing the macro (like I did) is not hard.

[straight-shoota]

I don't want to have an exception and miss all the other events I could have handled.

Well, then you could just handle the error for each individual event and use the values of those that don't error. Still, for such a use case where you expect to not handle all possible types, it would be great to avoid raising an exception and explicitly ask to return a default/fallback value or nil or maybe call a proc.

I'm actually not opposed to this. I was hesitant about adding use_json_discriminator in the first place, but now that it's there we can as well try to improve. And it should be a rather trivial enhancement to add declaration for fallback behaviour.

[erdnaxeli]

Actually if you click on the link "my project" on my first post you will see that I made the macro a lot more complex. So I definitely understand the point about keeping adding complexity ^^'.

We can still add the fallback, this modification is simple. What we could do:

• use_json_discriminator! raises
• use_json_discriminator returns nil (breaking change!)
• use_json_discriminator accepts a new parameter that it used as a fallback, and can be a type having a from_json method or a proc

If we don't want the breaking change, we can just say that to get nil instead of raising you have to specify it as a fallback, but that doesn't seem correct along the rest of the stdlib.

[straight-shoota]

Regarding the API, I'd probably just add an argument fallback. If not set, the generated method raises. If set, it tries to parse the respective type. And if set nil, it disregards the value and returns nil.

But actually, defining this on the macro call might not be that good. Ideally, you would want to define that behaviour at the callsite when doing a discrimination parse. This would be trivial with #8473.

[HertzDevil]

Duplicate of #13216

[straight-shoota]

It's related but not an exact duplicate. https://github.com/crystal-lang/crystal/issues/13216 is about a default that takes effect when the discriminator field is absent. This issue is about a fallback if there's no mapping for the discriminator value. It certainly makes sense to consider both in tandem (which not necessarily means both should be implemented).