Do not serialize familyId to remain compatible with ObjC API #1183
Conversation
| @@ -79,7 +79,12 @@ internal override JObject ToJObject() | |||
|
|
|||
| json[StorageJsonKeys.Environment] = Environment; | |||
| json[StorageJsonKeys.ClientId] = ClientId; | |||
| json[StorageJsonKeys.FamilyId] = FamilyId; | |||
|
|
|||
| // Workaround: MSAL-ObjC has a problem with | |||
There was a problem hiding this comment.
can we link to the github issue in their repo? do we have one?
|
Not convinced this is what we should be doing. Also not sure we have done enough testing to ensure this is a good approach. |
|
Semantically the 2 pieces of JSON below are identical: {}and {
"family_id": null
}When we rehydrate the cache, family_id will be null in both cases. I haven't tested if the cache remains valid with an ObjC app and to see if the scenario encountered still works, was hoping @jennyf19 can help. I can look at generalizing this fix but I haven't found an easy way to say "ignore nulls" |
|
I'll help anyway possible w/testing, but the more i think about it, the more i don't like this "workaround". especially w/the other null values that we discussed (and henrik brought up in the email). In reply to: 497392892 [](ancestors = 497392892) |
| json[StorageJsonKeys.FamilyId] = FamilyId; | ||
|
|
||
| // Workaround: MSAL-ObjC has a problem with | ||
| if (!String.IsNullOrEmpty(FamilyId)) |
There was a problem hiding this comment.
nit: lowercase string for type here
| @@ -85,7 +85,10 @@ internal override JObject ToJObject() | |||
| { | |||
| var json = base.ToJObject(); | |||
|
|
|||
| json[StorageJsonKeys.FamilyId] = FamilyId; | |||
| if (!string.IsNullOrEmpty(FamilyId)) | |||
There was a problem hiding this comment.
If we're going to follow this pattern, we should make a method to take the key and the value and do the nullcheck and only set if valid. will keep these easily to one line and we can call that method on any of the optional fields.
There was a problem hiding this comment.
Yes, will do. I was looking if it's possible to do it via newtonsoft configuration. I don't think it's possibble because we're at such a low level down the stack.
|
can't we use some thing like a general serializer setting to prevent null values to even be written? Like what Mark pointed out? |
|
I think #1189 fixes this consistently for not serializing null values at all. should we close this one out and use 1189 as the fix for this? |
|
Sure, no problem. I had a look at the suggestion Henrik made, but I think we're using an API that is too low level to be affected by those attributes. |
No description provided.