Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Possibility of custom attributes not being successfully sent to the API. #112

Closed
levibostian opened this issue Jan 12, 2022 · 3 comments · Fixed by #122
Closed

Possibility of custom attributes not being successfully sent to the API. #112

levibostian opened this issue Jan 12, 2022 · 3 comments · Fixed by #122
Labels
bug Something isn't working

Comments

@levibostian
Copy link
Contributor

levibostian commented Jan 12, 2022

version 1.0.0-alpha.25

Expected behavior

Public SDK functions that allow adding user attributes (such as CustomerIO.identify()) allow an optional JSONEncoder to be passed in. The main use case here is that if a customer wants user attributes to show up as "firstName" in fly.customer.io for example, they can. We want to give you the ability to modify the keys of the JSON.

Actual behavior

Customers providing their own JSONEncoder not only can modify the keys but also the values of JSON. This is a bad thing because our API requires dates to be in a certain format, for example. If a customer provides a custom JSONEncoder that encodes Date in a way that is not compatible with our API, there is a chance that value in the JSON will be ignored by the API.

@levibostian levibostian added the bug Something isn't working label Jan 12, 2022
@levibostian
Copy link
Contributor Author

This ticket needs a test case to see if this is actually a bug. A unit test should suffice.

If this is indeed a bug, one idea to fix the issue is to remove the ability to provide a custom JSONEncoder and instead encourage customers to use CodingKeys for their Encodable object. So they can modify the keys but our own JSONEncoder will encode the values the way we require.

@hownowstephen
Copy link
Contributor

This is a non-issue. Our API doesn't have any actual strong requirements on data formats, nor will it ignore data in the wrong format. Our impl of the JSONEncoder is built for best-practices when using Customer.io with dates (unix timestamp in seconds), but there's no specific reason that customers couldn't use an RFC3339 formatted date for a field.

Under the hood our segmentation does some clever tricks to decide when values are of a given type, but stores everything as strings. So in conclusion: I don't think we need to make any changes here - unless we think the CodingKeys change creates significant value @levibostian

@levibostian
Copy link
Contributor Author

I originally had the idea of allowing customers to provide their own JSONEncoder so that they can modify the key names of the JSON we produce for their custom attributes.

However, there are some flaws to this implementation:

  • The SDK code is more complex juggling between our own JSONEncoder and a provided one.
  • A customer providing their own JSONEncoder could mess up the HTTP request body being sent by the SDK which results in failed HTTP requests. This could lead to more tech support tickets.
  • There is an easier solution to allowing customers to provide their own JSON key names compared to providing a JSONEncoder: using CodingKeys in their Encodable object they pass to the SDK.

After reading the docs for JSONEncoder to see all the capabilities it provides, JSONEncoder isn't providing any more value to customers besides modifying their JSON keys.

I see this solution has more harms then value. Especially when there is a different solution that is not only easier to implement by a customer, but it removes all of the harms that the current implementation provides.

Not only that, but when you think about it, it doesn't make much sense to allow a custom JSONEncoder. A JSONEncoder instance should be designed to work against a specific API. We have already created a JSONEncoder that works great for the Customer.io API, so why the need to allow a different one?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants