Open whatamithinking opened 3 months ago
Thank you for this report. This seems indeed to be a good optimization to add. In fact, this optimization is already implemented for unions, but not for those resulting of multiple deserialization.
However, it may not solve your issue, because you may have fixed the deserialization, but I'm not sure you would be able to generate a JSON schema without encountering this recursion error. Fortunately, the feature you may be looking for is called coercion.
Why is there two overlapping features? Because coercion doesn't impact JSON schema, so you can keep a clean schema, displaying its intent without the noise, while having a more lenient API. It's slightly less optimized, but nobody should care. And its simpler than conversion for simple use case, e.g. stringified values.
Coercion should definitely be mentioned at the beginning of conversion documentation.
Fantastic library! Also, I kinda love your conversion system. It's fallback mechanism when the input data is not the same type as the source of the converter function is really beautiful.
I have just started using your library in a larger project and ran into a problem with recursion and unintuitive (for me) behavior. This library has extensive documentation, so it is possible I missed something though.
I need to support deserializing a bunch of scalar JSON types to other scalar JSON types which do not seem supported by apischema out of the box, so I registered some conversions and tested it out.
Output:
RecursionError: maximum recursion depth exceeded
So what seems to happen currently is that this will go through the conversions with str as target and hit (int -> str) and then jump to the conversions for target type of int and hit (str -> int) and then we keep looping.
The solution I am currently using is to monkeypatch the ConversionUnionMethod.deserialize method so it tries to find Conversion alternatives which are exact matches for the type of the given data before falling back to your current algorithm. So something like the following...
This is not exactly elegant, but it seems to work. The source types are not available in the
ConversionAlternative
objects here, so a hack is to use themethod
and map that to a type.I don't have the time to create a pull request but i thought this might help someone and might be something you would be interested in adding. Alternatively, if you have a better approach to solve the same problem, I would be interested.