Skip to content
This repository has been archived by the owner on May 30, 2024. It is now read-only.

Support conversion from an LDUser to a JsonObject #136

Closed
wants to merge 2 commits into from

Conversation

austinjones
Copy link

We want to extract the LDUser state from server, so it can be used to initialize client-side properties. Since we are already sending a secureModeHash to the client, it is natural to collect user context on the server side as well.

I couldn't find any way to get access to LDUser.UserAdapter, or any other solution.

@eli-darkly
Copy link
Contributor

Based on what you said about your use case, you may not need a custom adapter. Calling Gson.toJson(user) on an LDUser object should capture all of its properties.

The reason UserAdapter exists is that when the SDK sends user data in analytics events, it may need to selectively omit some properties if you've used the configuration options allAttributesPrivate/privateAttributeNames. But if you're passing the user data to the front end with the intention of passing it to the JS client, you don't want those properties to be omitted— they may be needed for flag evaluation; the JS client will take care of stripping them out, if necessary, when it sends analytics events.

The one property that isn't captured by Gson's default serialization is privateAttributeNames, which is used to track whether you have specified any private attributes at the user level by calling privateName, privateEmail, etc. So if you're using that feature, and want to pass that state to the front end, we would need to give you a different way to serialize the user.

@austinjones
Copy link
Author

@eli-darkly You are right. I hadn't read the TypeAdapter impl closely enough.

I ran into the privateAttributeNames problem. I think I can workaround it by telling Gson to serialize transient fields, though that still wouldn't reflect private attributes set in the LDConfig.

I updated the PR with logic that seems to do what I want. Do you think there is value in merging this, or am I going to run into additional problems?


Set<String> privateAttributeSet = new HashSet<>();
privateAttributeSet.addAll(this.privateAttributeNames);
privateAttributeSet.addAll(config.privateAttrNames);
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure of the reason for this line. The front end already has a privateAttributeNames configuration option for the client, just like the back end; if you're using that option on the back end, presumably you would be doing the same on the front end. The privateAttributeNames property in the user is meant for attributes that have been marked private for this user.

I mean, doing this won't hurt anything, but I think it is unnecessary and it also conflicts with what is arguably the normal goal of JSON serialization: capturing the state of this particular object.

Copy link
Contributor

@eli-darkly eli-darkly Aug 28, 2018

Choose a reason for hiding this comment

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

Similarly, I think adding the allAttributesPrivate property from the client configuration into the user object is misleading (since the JS client will not actually use the property there). If you want your front-end client to always use the same configuration as the back end without having to specify it in code, I think it would be better to pass those configuration options separately.

@eli-darkly
Copy link
Contributor

I appreciate the work you've done on this, but I think I would be more comfortable with a simpler approach where we just make the default Gson behavior for this object more complete (i.e. allow privateAttributeNames to be serialized). I believe that could be done more easily, and wouldn't require the caller to use a special method. (And as I mentioned in comments, I think it would be best not to mix in global config options with the user properties.) I'll take a go at implementing this to see if I'm right that it is straightforward.

There also may be a valid use case for making the UserAdapter accessible, but if we're going to do that we need to make it very clear that it's not a general-purpose User serializer but rather is meant to implement the private attribute behavior for events.

@eli-darkly
Copy link
Contributor

Also, about this--

I think I can workaround it by telling Gson to serialize transient fields

--as far as I can tell there was never really a good reason for that field to be transient. That's only relevant when we're serializing users for events (in UserAdapter) in which case we're not using Gson's default serializer anyway, therefore it doesn't matter whether that field is transient. Not sure what we were thinking there!

@austinjones
Copy link
Author

austinjones commented Aug 28, 2018

I appreciate the work you've done on this, but I think I would be more comfortable with a simpler approach where we just make the default Gson behavior for this object more complete (i.e. allow privateAttributeNames to be serialized).

Removing transient would work for me!

I'll take a go at implementing this to see if I'm right that it is straightforward.

Yeah, starting to get the feeling I'm getting in over my head here. Thanks for your help!

Also, for background: I have several JS apps that need to pull the user context, and initialize themselves. I'm hoping to centralize all that via REST.

@eli-darkly
Copy link
Contributor

As of v4.3.1, the default Gson serialization for LDUser includes all properties so it is suitable for passing to the front end. As I mentioned, you'll need a different mechanism for passing configuration parameters like allAttributesPrivate since the front-end client does not expect to see those in the user object.

@eli-darkly eli-darkly closed this Sep 6, 2018
@austinjones
Copy link
Author

Perfect! Thanks Eli!

Yeah, we will just avoid setting allAttributesPrivate in the config.

LaunchDarklyCI pushed a commit that referenced this pull request Aug 19, 2019
avoid concurrency problem with date parser for event responses
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