fix for using other serializing formats and converters. #1417
Conversation
…rializers which are honoured by the dialogs and saved with the accessors they might not be read back if not same settings/converters are set. using f.i. Iso8601TimeSpanConverter this is not globally accessible. the change uses same global JsonConvert.DefaultSettings as the rest of the bot.
…Settings for this to work properly.
|
@helgeu, The change here is interesting, but I don't think "as-is" we could take this. However if you go a step further and have the serialize be injectable (via a property, or even via the CTor), then I think this would be great. |
|
@cleemullins by design of asp.net and seriliazers it is defacto injected as of now. it is of course implicit, but the idea is that the serilalizer set up elsewhere as default is to be used here, which (arguably) is no surprise to the user or otherwise. in fact that this serializer was NOT using the default serializer set up was a surprise! ill take a look at handling it via some ctor injection or property and try to make it "clean" so that it has full backward compability and the user may opt in to use another serializer. unless you of course Accept the above argument ;-) |
|
|
| @@ -39,13 +39,27 @@ public class CosmosDbStorage : IStorage | |||
| /// using the provided CosmosDB credentials, database ID, and collection ID. | |||
| /// </summary> | |||
| /// <param name="cosmosDbStorageOptions">Cosmos DB storage configuration options.</param> | |||
| public CosmosDbStorage(CosmosDbStorageOptions cosmosDbStorageOptions) | |||
| /// <param name="jsonSerializer">Optional JsonSerializer. Mind that the TypeNameHandling, NullvalueHandling and ContractResolver will be overridden.</param> | |||
| public CosmosDbStorage(CosmosDbStorageOptions cosmosDbStorageOptions, JsonSerializer jsonSerializer = null) | |||
There was a problem hiding this comment.
We can't take this, as is, as it breaking binary compat. For this approach, you need add an additional ctor that takes the JsonSerializer.
| else | ||
| { | ||
| // These need to be overridden to the following values. | ||
| jsonSerializer.TypeNameHandling = TypeNameHandling.All; |
There was a problem hiding this comment.
I'm not clear on this - if the developer passes in a serializer, we should use it "as is". We can certainly document "We recommend the following settings", but to change what they passed in will probably lead to grief and unexpected behaviors.
There was a problem hiding this comment.
The problem is that it will crash, if these two are not set, it will not work at all.
But yes, I might of course extract this to its own ctor.
By the way, how its working now, did in fact lead to grief and unexpected behaviour ;-)
I'll make another "clean" version of the ctor.
There was a problem hiding this comment.
If it crashes w/o these, then we should keep them in. Adding an an XmlCode Comment on the parameter to indicate this would be enough for me.
|
Fixes #1417 |
|
Added another version with clean ctor for jsonSerializer. |
|
another one bites the dust ;-) |
if used some serializer setup which are honoured by the dialogs and saved with the accessors they might not be read back if not same settings/converters are set. using f.i. Iso8601TimeSpanConverter this is not globally accessible. the change uses same global JsonConvert.DefaultSettings (AND the additional setting which was set in thie original code) as the rest of the bot.