Skip to content
This repository was archived by the owner on Feb 23, 2025. It is now read-only.

Allow LogSerializer to be disabled to facilitate usage in Unity iOS #67

Merged

Conversation

peterhorsley
Copy link
Contributor

Unity iOS apps using the Unity equivalent of NewtonSoft.Json library crash when trying to JSON serialize objects with properties. This PR is to add a mechanism to disable the serialization of arbitrary objects for logging purposes, which is the only code change required to have this library working in Unity iOS apps.

…pps, which cannot JSON serialize objects with properties.
@leastprivilege
Copy link
Contributor

But that means that logging is broken in that situation. Any other ideas?

@peterhorsley
Copy link
Contributor Author

Probably the best alternative would be to remove the {get;set;} from AuthorizeState and OidcClientOptions classes (since these are the only two classes that get passed to LogSerializer.Serialize()). That will allow the logging to work properly on iOS. Or we could override ToString() on those classes and manually construct the log message instead of using JsonConvert.SerializeObject. Sadly fixing the root cause I believe requires a changes to the IL2CPP code generation that Unity employs for iOS builds, which seems to be stripping required code from the generated properties, or changes to the Unity-compatible Json.Net library to handle properties differently. We've had this problem with our own code, it's not something specific to this library, and our solution was to use fields instead of properties on classes that need to be serialized. There are two threads with info on this here and here. What do you think?

@@ -27,7 +29,7 @@ static LogSerializer()
/// <returns></returns>
public static string Serialize(object logObject)
{
return JsonConvert.SerializeObject(logObject, jsonSettings);
return Enabled ? JsonConvert.SerializeObject(logObject, jsonSettings) : "";
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you change that to "Logging has been disabled" - and put a comment on the Enabled property to describe why it is there.

@leastprivilege leastprivilege merged commit fd4d18c into DuendeArchive:dev Jun 19, 2018
@leastprivilege
Copy link
Contributor

thanks!

@peterhorsley
Copy link
Contributor Author

thank you! i'm thinking to write a sample unity app showing how to use this with unity for android/ios.
would that be something that makes sense to add to the IdentityModel.OidcClient.Samples repo?

@leastprivilege
Copy link
Contributor

Put it somewhere on github - i can link to it from the readme.

@peterhorsley peterhorsley deleted the optional-log-serialization branch June 21, 2018 09:13
@peterhorsley
Copy link
Contributor Author

Sample code added here:
https://github.com/peterhorsley/IdentityModel.OidcClient.Samples/tree/unity-sample

Videos showing the sample running:
https://codenature.info/pub/unityauth/android-identitymodel-unity-sample.mp4
https://codenature.info/pub/unityauth/iphone-identitymodel-unity-sample.mp4

If you'd prefer I can make a separate repo for this to keep it dis-associated. Or if you are happy to bring Unity into the list of platform samples I can raise a PR on the samples repo.

@leastprivilege
Copy link
Contributor

separate repo with a readme would be cool - so you can link to the videos. I will link to your repo from my readme. thanks!

@peterhorsley
Copy link
Contributor Author

@leastprivilege
Copy link
Contributor

Thanks!

@github-actions
Copy link

This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Feb 21, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants